Skip to content

Replace sebastian/diff with own strict unified-diff parser - #83

Draft
janedbal wants to merge 1 commit into
masterfrom
jn-own-unified-diff-parser
Draft

Replace sebastian/diff with own strict unified-diff parser#83
janedbal wants to merge 1 commit into
masterfrom
jn-own-unified-diff-parser

Conversation

@janedbal

Copy link
Copy Markdown
Member

Why

SebastianBergmann\Diff\Parser classifies each patch line with regexes. Its header-skip regex [+-]{3} [ab] also matches ordinary content lines such as +-- alternative ... (an added SQL comment). The parser drops such lines silently. All later line numbers in the hunk then shift by one.

For coverage-guard this has two outcomes:

  • a false Patch file ... has added line #N that does not match actual content error, or
  • silently wrong coverage attribution, when the shifted content happens to match (duplicate or empty lines).

We hit the first outcome in a real CI run: a patch added a SQL migration with a comment line starting -- alternative ..., and the check failed on an up-to-date patch. Upstream does not accept fixes for this class of parser bugs (sebastianbergmann/diff#132), so the parser is replaced instead.

How

The new UnifiedDiffParser consumes each hunk body by the line counts that the @@ header declares. While inside a hunk body, the parser never inspects a line for header-likeness, so content lines cannot be misclassified — no matter what text they start with. Any structural inconsistency (counts that do not match the body, an unexpected line, a truncated hunk, a quoted path, a binary patch) causes a hard ErrorException instead of a silent skip.

Supported input stays the same: git diff output with the standard b/ prefix, plus plain diff -u style ---/+++ sections. Handled explicitly: new/deleted files, renames, mode-only changes, Binary files ... differ, \ No newline at end of file, zero-context (-U0) hunks, CRLF patches, and empty context lines whose single space was stripped by some tool.

sebastian/diff is no longer needed and is removed from require-dev, the dependency-analyser config, and the README optional-dependencies list.

Note on fixtures

The hand-made fixtures (sample.patch and friends) declared hunk counts that did not match their bodies (@@ -10,4 +10,10 @@ for a body with 5 old / 9 new lines). sebastian/diff never validated counts, so this went unnoticed. The strict parser validates them, so the fixtures now carry correct counts.

Validation

  • 22 new unit tests for the parser, including a regression test for the +-- a... case and for diff-of-a-diff content.
  • Full composer check passes.
  • The real 38-file CI patch that triggered the incident now parses correctly (verified locally).

The same fix will be ported to shipmonk-rnd/copy-paste-detector, which copied this PatchParser.

Co-Authored-By: Claude Code

The sebastian/diff parser classifies each patch line with regexes.
Its header-skip regex '[+-]{3} [ab]' also matches content lines such
as '+-- alternative ...' (an added SQL comment line). The parser
drops such lines silently. All later line numbers in the hunk then
shift by one. This causes false "patch mismatch" errors, or silently
wrong coverage attribution when the shifted content happens to match.
Upstream does not accept fixes for this class of bugs
(see sebastianbergmann/diff#132).

The new UnifiedDiffParser consumes each hunk body by the line counts
that the '@@' header declares. A content line inside a hunk can never
be mistaken for a header. Any structural inconsistency causes a hard
error instead of a silent skip.

The hand-made test fixtures declared wrong hunk counts and
sebastian/diff never validated them. The strict parser does, so the
fixtures now carry correct counts.

Co-Authored-By: Claude Code
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.

1 participant