Skip to content

fix: parse local diffs whatever the user's git diff config says - #85

Open
tsapeta wants to merge 3 commits into
mainfrom
tsapeta/git-diff-no-ext-diff
Open

tsapeta wants to merge 3 commits into
mainfrom
tsapeta/git-diff-no-ext-diff

Conversation

@tsapeta

@tsapeta tsapeta commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Why

A local ecr review (and so the /expo-review skill in expo/expo) reported "No reviewable changes" for me, because my git config has diff.external=difft. LocalGitSource runs plain git diff, which follows the user's config, so the parser got difftastic's output instead of a unified diff. Forced color (color.diff=always) and diff.noprefix or diff.mnemonicPrefix break it the same way: escape codes land before diff --git, or the a/ and b/ prefixes that parseUnifiedDiff matches are missing or turn into c/ and w/.

How

Every git diff that LocalGitSource parses (staged, base...head, working tree, and the --no-index diffs for untracked files) now passes --no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/, which gives the plain unified diff whatever the config says. CI isn't affected, since ecr ci reads the PR diff from GitHub's API. LLP 0008's "Local Git Source" section now records this.

Test Plan

The new src/__tests__/local-git.test.ts builds a repo whose local config sets diff.external, color.diff=always and diff.noprefix, and checks that the working-tree (with an untracked file), base...head and staged diffs all parse. All three fail without the fix. bun test (798 pass), bun run typecheck, bun run lint, bun run fmt:check and bun run llp:check pass.

Follow-up commit (1c58670)

From a multi-model review of this PR:

  • --no-textconv and --find-renames join the shared flags. A textconv driver showed converted text instead of the real file, or hid a change entirely when both sides converted alike. diff.renames=copies turned a copied file into a copy from/copy to entry with no hunk, so its contents went unreviewed.
  • The tests now fail when any one flag is removed; before, dropping both prefix flags still passed. The fixture has a b/inner.txt path that diff.noprefix breaks, and new cases cover diff.mnemonicPrefix, textconv and copy detection.
  • Fixture commits pass -c commit.gpgsign=false and --no-verify, so a global signing setup or hooks path no longer fails all of them.
  • LLP 0008's line references are updated.

Not changed: repository attributes (binary, -diff) still mark a file binary, and noise filtering drops it. The checkout is trusted in local mode (LLP 0001). bun test (801 pass), typecheck, lint, fmt:check and llp:check pass.

Follow-up commit (eb72d19)

From a second multi-model review:

  • --submodule=short and --ignore-submodules=dirty join the shared flags. diff.submodule=log printed commit subjects that parsed as nothing, diff.submodule=diff showed the submodule's own files instead of the pointer change, and diff.ignoreSubmodules=all or ignore = all in .gitmodules dropped the change entirely. dirty (not git's default none) also keeps edits inside a submodule whose pointer didn't move out of the review.
  • The tests no longer reach outside their temp repos. They remove every GIT_* variable, so bun test from a pre-commit hook can't write fixtures into the commit being made through GIT_INDEX_FILE. They also point HOME and XDG_CONFIG_HOME at an empty directory, so a global hooks path, signing, or a global ignore or attributes file can't fail them, on any git version.
  • New tests cover a deleted file under diff.noprefix and diff.mnemonicPrefix, the only case where --src-prefix matters, and the submodule settings above. Removing any one of the eight flags fails at least one test.
  • LLP 0004 and 0008 line references are updated.

Not changed: other test files (for example init-scaffold) still use the caller's GIT_DIR when run from a hook; a suite-wide bunfig.toml preload would fix that separately. bun test (808 pass), typecheck, lint, fmt:check and llp:check pass.

`git diff` follows the user's git config. An external diff tool
(`diff.external`, for example difftastic) replaces the unified diff, forced
color puts escape codes before `diff --git`, and `diff.noprefix` or
`diff.mnemonicPrefix` drop or change the `a/` and `b/` prefixes. With any of
them, a local `ecr review` parsed no files and reported "No reviewable
changes".

Every `git diff` that LocalGitSource parses now passes `--no-ext-diff
--no-color --src-prefix=a/ --dst-prefix=b/`. CI is unaffected: it reads
the PR diff from GitHub's API.
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

🤖 AI code review

Decision: Ready for human review

Overall PR risk: Low. This change modifies all four local diff calls to override user configurations with fixed git flags, ensuring unified diff output. The blast radius is limited to local-mode reviews; GitHub-based CI and PR sources are unaffected. Most plausible breakage: older git versions rejecting new flags, or changed behavior where submodule edits no longer appear when the pointer did not move.

🔗 Review setup:

  • This PR changes code the reviewer prompts cite (src, src/tests). Check that the guidance quoting it is still correct.

No findings.


This review is advisory — it never blocks a merge and never auto-approves.

@tsapeta
tsapeta marked this pull request as ready for review October 20, 2026 22:36
@tsapeta
tsapeta requested a review from brentvatne October 20, 2026 22:36
…den the tests

A textconv driver showed converted text instead of the real file, or hid a change
when both sides converted alike, and `diff.renames=copies` turned a copied file
into a hunk-less entry whose contents went unreviewed. `--no-textconv` and
`--find-renames` join the shared flags.

The tests now fail when any one flag is removed: the fixture has a `b/` path that
`diff.noprefix` breaks, and new cases cover `diff.mnemonicPrefix`, textconv and copy
detection. Fixture commits ignore global signing and hooks, and LLP 0008's line
references are current again.
…late the local-git tests

- Local diffs also pass --submodule=short and --ignore-submodules=dirty, so
  diff.submodule=log|diff, diff.ignoreSubmodules=all and ignore = all in
  .gitmodules can't hide or rewrite a submodule pointer change, and edits
  inside a submodule whose pointer didn't move aren't sent for review.
- The tests remove every GIT_* variable and point HOME and XDG_CONFIG_HOME
  at an empty directory, so a hook's GIT_DIR/GIT_INDEX_FILE or the user's
  global git files can't touch another repo or fail the fixtures.
- New tests: deletions under diff.noprefix and diff.mnemonicPrefix (the only
  case --src-prefix protects), and the submodule settings above.
- LLP 0004 and 0008 line references updated.
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.

2 participants