Repository navigation
fix: diff parsers keep lines starting with -- or ++ inside a hunk - #270
Merged
Merged
Conversation
A removed line whose content starts with `--` (diff line `--- comment`)
and an added line starting with `++` (diff line `+++x`) were dropped,
because the `---`/`+++` file-header filter also ran inside hunks. A .sql
file losing a `-- comment` came back as a hunk with only its context
lines, and every later line number shifted.
Every parser now bounds a hunk body by the counts of its `@@` header:
while they are not used up, a line is classified by its first character
only ('+', '-', ' '; an empty string is never context). `---`/`+++` are
file headers only outside a body. A combined `@@@` hunk stays open until
the next `diff ` line, since its counts are per parent.
Fixed parsers:
- Rust: parse_diff_hunks (git_diff), git_show, extract_staged_added_lines
(secrets scan, where `+++ token` also renamed the file),
gl_diff_stats_from_files. Shared helpers parse_hunk_header / HunkBody.
- dev-server.mjs: /api/git-diff, /api/git-file-diff, /api/git-show,
/api/scan-secrets (devHunkBody helpers, kept in sync with Rust).
- @gitwand/core: new hunk-body helpers (exported), used by isPureMove.
- CLI: `gitwand scan` extractStagedAddedLines.
- Desktop: utils/unifiedDiff parseFileDiff, useAbsorb parseDiffHunks
(`--- a/x` content reassigned hunks to file `x`), usePrSummary
countChanges; PullRequestPanel drops its copy for utils/unifiedDiff.
Tests: Rust unit tests on real temp repos (git_diff staged/unstaged,
multi-file git_show, secrets scan) and on raw text (multi-file headers,
no-newline marker, empty line, combined hunk); vitest for every TS parser;
parity cases for git-diff and scan-secrets, plus a new git-show parity
test (probe gains a git-show command).
Review follow-up to the `--`/`++` hunk-line fix.
- Blank context lines: with `diff.suppressBlankEmpty=true` git prints a
blank context line as "". The TS parser counted it as context, while
Rust `HunkBody` and the dev-server dropped it without consuming a count,
so later line numbers shifted and rebuilt patches failed `git apply`.
One rule everywhere now: inside a hunk, while counts remain on both the
old and new side, "" is a context line; outside a hunk or once a count
is used up, it is ignored. Implemented in core `classifyHunkLine` (so
unifiedDiff no longer maps "" to " " itself) and Rust `HunkBody::classify`
(`push_hunk_line` no longer slices an empty line).
- AGENTS.md "Diff Parsing" states the bounded rule, keeping the warning
against unbounded `!startsWith('\\')` parsers; core and desktop CLAUDE.md
point to it.
- Core API: `hunkBodyFromHeader(line)` parses the header itself (counts
default to 1) and replaces the parse-then-build pairs at every call site;
`hunkBodyActive` is no longer exported. Rust gains `HunkBody::from_header`.
- dev-server.mjs: the devHunkBody/devClassifyHunkLine copies are gone; a
single `devParseDiffHunks` built on @gitwand/core serves /api/git-diff,
/api/git-file-diff and /api/git-show, and /api/scan-secrets uses core's
helpers and `parseHunkHeader` instead of its own regex.
- git_show now splits on `diff --git` and parses each section with
`parse_diff_hunks` (new `parse_multi_file_diff`), like /api/git-show,
instead of carrying its own hunk loop.
Tests: Rust real-repo tests with diff.suppressBlankEmpty for git_diff
(staged and unstaged) and git_show, plus raw-text cases; core and desktop
vitest for the bounded "" rule and hunkBodyFromHeader; parity cases for
git-diff and git-show with diff.suppressBlankEmpty.
Second review follow-up.
- Strict counts: core `classifyHunkLine` and Rust `HunkBody::classify`
accept '+' only while new-side lines remain, '-' only while old-side
lines remain, and context (' ' or "") only while both do. A header that
undercounts its body now ends the hunk there, so a following `--- a/x`
is read as a header (fails closed) instead of a deletion. A combined
`@@@` hunk keeps first-character classification. Rust drops the now
unused `HunkBody::active`.
- dev:web / dev:server build @gitwand/core first (`build:core`, ~4 s):
dev-server.mjs reads the shared diff parser from packages/core/dist.
`loadDevCore()` imports core once per process, checks the helpers it
needs and fails with "run `pnpm --filter @gitwand/core build`" when dist
is missing or stale; a failed load is retried on the next request.
- `devParseDiffHunks(core, text)` is synchronous; /api/git-show loads core
once per request instead of once per file section; /api/scan-secrets
goes through `loadDevCore()` as well.
- git_file_diff (#269): Rust tests and parity cases with `-- x`/`--- x`
removed and `++x`/`+++ y` added, plain and blob-to-blob (renamed), with
and without diff.suppressBlankEmpty.
- AGENTS.md "Diff Parsing" states the strict per-side rule and lists every
user of the shared parsers; apps/desktop/CLAUDE.md documents the core
build that dev:web and dev:server now run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every unified-diff parser in the repo applied its
---/+++file-header filter inside hunks too. A removed line whose content starts with--(diff line--- comment, e.g. a SQL or Lua comment) and an added line starting with++(diff line+++x) were silently dropped, and every later line number in the hunk shifted. Reproduced onmainwithgit_diffon a.sqlfile: the hunk header said@@ -1,3 +1,3 @@and only the context lines came back.Some copies did worse:
secrets.rs, CLIscan.ts): an added++ tokenline was taken for the+++ tokenheader, so the line was skipped and the file path becametoken.useAbsorb: a removed-- a/xline moved every following hunk onto a file namedx.What
@@ -a,b +c,d @@counts. Inside it, a line is classified only by its first character:+while new-side lines remain,-while old-side lines remain, context (' ', or""whendiff.suppressBlankEmptyprints a blank context line) while both remain.---/+++are headers only outside a hunk;diff --gitresets. Combined@@@hunks stay open until the nextdiffline.git/parse.rs:HunkBody,parse_hunk_header,push_hunk_line. Used byparse_diff_hunks(git_diff,git_file_diff),git_show(nowparse_multi_file_diff→parse_diff_hunksper file),secrets.rs,gitlab.rs(MR stats).@gitwand/corehunk-body.ts(pure TS):parseHunkHeader,hunkBodyFromHeader,classifyHunkLine. Used by coreisPureMove, CLIscan.ts, desktopunifiedDiff(PullRequestPanel.vuedropped its own copy),useAbsorb,usePrSummary, and the dev-server.devParseDiffHunksuses core for/api/git-diff,/api/git-file-diff,/api/git-show, and/api/scan-secretsuses the helpers directly.dev:web/dev:servernow buildpackages/corefirst (build:core, ~4 s warm), since the dev-server diff routes import it. If core is missing or stale, those routes answer with a message saying which command to run.apps/desktop/src/CLAUDE.mdandpackages/core/CLAUDE.md.Tests
git_diffstaged/unstaged andgit_showmulti-file on a.sqlfile removing-- old comment/--- xand adding++x/+++ y, exact line numbers; the same withdiff.suppressBlankEmpty=true;git_file_diffplain and blob-to-blob (renamed); the secrets scan finding a++ key…secret on the right file and line. Raw-text tests for headers after an exhausted hunk,\ No newline, counts per side,@@ -1 +1 @@, combined hunks.hunk-body+isPureMove, CLIscan, desktopunifiedDiff,usePrSummary,useAbsorb. Two fixtures were corrected because the parser now trusts counts (one header claimed fewer lines than its body, one had no@@header).git-diff, newgit-show(probe gainedgit-show),git-file-diff,scan-secrets, includingsuppressBlankEmpty.cargo test --lib640, core 1294, desktop 2043, CLI green;clippy -D warnings,vue-tscclean. Parity 181/183 locally: the 2 misses areauto-merge-refusal, where a freshly built probe waits on its first keychain prompt — unrelated to diffs.Behaviour change to know
A diff whose header counts are wrong now gets that hunk cut where the counts end. No forge path in the app was found to send such diffs (GitLab uses
glab --raw, Azuregit diff, Gitea/Bitbucket git format), but it is the trade-off of trusting the header.Follow-ups (not in this PR)
git_show/ the dev-server join each file section back into a string before parsing it.@@@header numbers differ: Rust reads old (1,1) / new (0,1), core and the dev-server fall back to 0,1,0,1 (hunk bodies agree);isPureMoveon a combined hunk.