Skip to content

fix(loaders): normalize mnemonic pager prefixes#267

Merged
benvinegar merged 1 commit intomainfrom
fix/mnemonic-pager-prefixes
May 9, 2026
Merged

fix(loaders): normalize mnemonic pager prefixes#267
benvinegar merged 1 commit intomainfrom
fix/mnemonic-pager-prefixes

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

Fixes hunk pager / hunk patch handling for Git patch input emitted with diff.mnemonicPrefix=true.

Git emits mnemonic side prefixes like i/, w/, c/, 1/, and 2/ instead of the parser-friendly a/ / b/ prefixes. After #240, those patches can parse, but file paths keep the side prefix (for example w/example.ts). This normalizes mnemonic prefix pairs back to Pierre's expected a/... b/... shape before parsing so the review path is example.ts.

Fixes #265.

Tests

  • bun test src/core/loaders.test.ts
  • bun run typecheck
  • bun run lint

This PR description was generated by Pi using OpenAI GPT-5

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 9, 2026

Greptile Summary

This PR normalizes Git mnemonic side-prefixes (i/, w/, c/, 1/, 2/) in patch text produced with diff.mnemonicPrefix=true back to the a//b/ form expected by the downstream Pierre parser, fixing hunk pager and hunk patch stripping those prefixes from file paths.

  • Adds splitGitMnemonicPrefix, canonicalizeKnownGitPathPair, and canonicalizeGitPathPair helpers; refactors rewriteGitDiffHeader to return a rewriteMode so rewriteUnifiedFileLine knows whether to insert or strip a prefix on ---/+++ lines.
  • Introduces an integration test that runs a real git diff -c diff.mnemonicPrefix=true and asserts the resulting loadAppBootstrap produces an unprefix-stripped path; changelog entry added.

Confidence Score: 3/5

The mnemonic-prefix detection heuristic can silently corrupt paths for a noprefix rename between root directories whose names happen to match mnemonic characters, making this change risky to merge without an additional guard.

The core fix is well-structured, but the heuristic in canonicalizeKnownGitPathPair that identifies a mnemonic prefix pair by checking that both halves start with distinct characters from [ciow12] is ambiguous: a noprefix rename from c/foo.ts to w/bar.ts is indistinguishable from a true mnemonic pair, and the new code silently drops both directory segments from the resulting paths. The old two-token rename fallback preserved the full path in this case.

src/core/loaders.ts — specifically the canonicalizeKnownGitPathPair function and the interaction between its mnemonic-detection branch and the two-token rename fallback.

Important Files Changed

Filename Overview
src/core/loaders.ts Introduces splitGitMnemonicPrefix, canonicalizeKnownGitPathPair, and canonicalizeGitPathPair to normalize Git mnemonic side-prefixes to a//b/ before parsing. The mnemonic-detection heuristic introduces a false-positive for noprefix renames between root directories named with mnemonic characters, silently dropping directory segments from paths.
src/core/loaders.test.ts Adds an integration test for diff.mnemonicPrefix=true patch input via a real temp git repo. Covers simple modification but does not cover renames with mnemonic prefixes.
CHANGELOG.md Adds a changelog entry for the mnemonic prefix fix under the current unreleased section.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/core/loaders.ts:382-390
**False positive for noprefix renames between mnemonic-named directories**

`canonicalizeKnownGitPathPair` decides a path pair uses mnemonic side-prefixes if both halves start with a character in `[ciow12]` and the characters differ. A repo that has top-level directories literally named `c/` and `w/` (or any other two distinct mnemonic characters) and renames a file between them in noprefix mode will trip this branch. `c/foo.ts w/bar.ts` is treated as `i/example.ts w/example.ts`, producing `a/foo.ts b/bar.ts` instead of the correct `a/c/foo.ts b/w/bar.ts`, silently dropping the directory segment from every subsequent file path lookup.

Before this PR the two-token rename fallback preserved the full path. The regression only manifests for repos with mnemonic-named root directories, but the failure is silent and produces wrong paths downstream.

### Issue 2 of 3
src/core/loaders.ts:358-364
**Redundant `GIT_MNEMONIC_PREFIXES.has` guard after regex already restricts the character class**

The regex `^([ciow12])\/(.+)$` can only capture a character in `[ciow12]`, so `GIT_MNEMONIC_PREFIXES.has(match[1]!)` will always be `true` when the regex matches. The guard is dead code and adds a small amount of noise; the set itself is the more readable declaration of valid prefixes, so either the regex character class should delegate to it (not currently expressible inline) or the `.has` check can be removed.

### Issue 3 of 3
src/core/loaders.test.ts:1085-1109
**Missing test coverage for mnemonic-prefix rename diffs**

The new test exercises only a simple modification (`value = 1` to `value = 2`). A rename with `diff.mnemonicPrefix=true` emits `diff --git i/old.ts w/new.ts` with `rename from old.ts` / `rename to new.ts` (no prefix on the `rename` lines), which exercises a different code path through `canonicalizeKnownGitPathPair`. Adding a test that commits a rename and diffs with `diff.mnemonicPrefix=true` would confirm the `rename from`/`rename to` lines are handled correctly and guard the path described in the P1 comment above.

Reviews (1): Last reviewed commit: "fix(loaders): normalize mnemonic pager p..." | Re-trigger Greptile

Comment thread src/core/loaders.ts Outdated
Comment thread src/core/loaders.ts Outdated
Comment thread src/core/loaders.test.ts
@benvinegar benvinegar force-pushed the fix/mnemonic-pager-prefixes branch from 98b53c0 to d2af79d Compare May 9, 2026 23:44
@benvinegar
Copy link
Copy Markdown
Member Author

@greptile-apps thanks for the review — addressed all three points in d2af79d.

  • The mnemonic-prefix detection now has access to the whole diff --git block and checks rename from / rename to metadata before stripping c/, w/, etc., so noprefix renames like c/foo.ts -> w/bar.ts keep those as real directory names.
  • splitGitMnemonicPrefix now uses the prefix set directly instead of duplicating the character class plus a redundant .has() guard.
  • Added regression tests for both mnemonic rename diffs and the noprefix mnemonic-looking-directory rename false positive.

This comment was generated by Pi using OpenAI GPT-5

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 9, 2026

All three changes look correct and well-executed.

Mnemonic rename disambiguationshouldStripMnemonicPair cleanly resolves the ambiguity by comparing rename from/rename to metadata against both the raw paths and the stripped paths:

  • rename from c/foo.ts matches the full oldPath → real directory, don't strip
  • rename from old.ts matches oldMnemonic.path → genuine mnemonic, strip
  • No rename metadata → must be mnemonic, strip

splitGitMnemonicPrefix cleanup — using GIT_MNEMONIC_PREFIXES.has(prefix) directly is cleaner and removes the redundant character-class duplication.

Regression tests — the three tests cover exactly the cases that needed coverage:

  1. Basic mnemonic prefix (i/, w/)
  2. Mnemonic rename (c/old.tsi/new.ts)
  3. Noprefix rename with real c/ and w/ directories (the false positive)

One minor observation: in canonicalizeKnownGitPathPair, the canonical-path branch sets rewriteMode: "add" as const even though changed: false means that field is never consumed (the caller hits the isCanonical branch and returns rewriteMode: null). It's a dead value but harmless — not worth a follow-up unless you want to tighten the types.

LGTM overall. Nice fix.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@benvinegar
Copy link
Copy Markdown
Member Author

Thanks — small nuance on that rewriteMode: "add": it is dead for the unquoted canonical path because rewriteGitDiffHeader returns rewriteMode: null from the isCanonical branch. It is still intentionally consumed through the quoted-header path, where canonicalizeGitPathPair() can receive already-prefixed quoted input like "a/foo" "b/foo"; we still need a non-null rewrite mode there so the matching quoted --- / +++ lines are canonicalized into Pierre’s unquoted form.

Given that, I’m going to leave it as-is rather than add a type-only refactor in this PR.

This comment was generated by Pi using OpenAI GPT-5

@benvinegar benvinegar merged commit 633815c into main May 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hunk pager fails with git diff when diff.mnemonicPrefix = true (no output or "no files match the filter")

1 participant