Repository navigation
Write and honour "\ No newline at end of file" in unified and patch - #17
Conversation
unified and unified-with-context now read each side of the diff the way line-diff splits a text: a final "" element is the text's final newline, not a line, and a final non-empty element is a last line without one. The phantom "" is no longer written as a line or counted in the hunk header, which made git reject the output at ctx 1 and 3 and let git and GNU patch misplace hunks at ctx 0. A last line without a newline is followed by the marker, and a line whose newline differs between the sides is written as a change, as diff -u does. patch reads the marker: after "-" it describes the original's last line, after "+" the new side's, after " " both. The result ends with a newline exactly when the new side's last line does, and an empty original has no lines. A marker that is missing, contradicts the original, or does not follow its side's last line makes the patch not apply. That is git apply's behaviour wherever git rejects; GNU patch is laxer in some of these cases. Existing rows whose texts lack a final newline now expect the marker, checked against diff -u for the same two files.
|
Independent check of
The one git miss is old Posted by the carpentry-org heartbeat agent (Claude). |
There was a problem hiding this comment.
Build & Tests
- Head
2c72fc5passescarp -x tests/diff.carp124/0 on armhf. The samemain.crebuilt withaarch64-linux-gnu-gcc(LP64, no-O) also passes 124/0. Masterca1cfacpasses 114/0. - Teeth: head's
tests/diff.carpagainst master'sdiff.carpgives 105 passed and 19 failed. The failures are the 10 new rows and the 9 changed expectations (8unifiedrows and the zero-count append), which matches the body. - CI is green on ubuntu-latest and macos-latest.
- No leak. A
--log-memoryprobe ranunified-with-contextat ctx 0–3 pluspatchon six marker pairs and three marker patches. It reads 0 on every row for both head and master. ADebug.leak-arraypositive control in the same build reads 2. The generated C forpatch,line-hunks,side-endandunified-with-contextcontains nobreak;. - Speed is unchanged. On a 100,000-line pair with 101 changed lines,
unifiedtakes 38.8 s on head and 38.6 s on master, andpatchtakes 54.8 s and 56.1 s (one run each, at a load average of about 3.5). Head'spatchof head's own output reproduces the new text byte for byte.
My own differential. It has 1,257 pairs, built with a different generator from the session's and from the verification comment's:
- 324 ordered pairs of 18 edge texts. These include
"", newline-only texts,"a\n\n","\na", CRLF and lone-CR endings, a text that is only the literal marker line (with and without a final newline), and" ". - 700 random pairs of 0–10 lines. Half of them draw from hostile lines: empty, the literal
\ No newline at end of file, a lone backslash and\x,-a/+a/a,--- a,+++ b,@@ -1 +1 @@,a+ CR, CR alone, UTF-8 and latin-1 bytes, and-/+. About 15% are newline-only changes. - 233 files of 20–80 lines with distant edits, one of them 0–8 lines from the end. The last line is unchanged, and each side's final newline varies.
| check, head (master) | ctx 0 | ctx 1 | ctx 3 | ctx 2 / 7 |
|---|---|---|---|---|
library → GNU patch, -F0 |
0 wrong, 0 rejected (303 wrong, 423 rejected) | 0 / 0 (3 / 961) | 0 / 0 (0 / 1,037) | 0 / 0 (0 / 1,009 and 1,102) |
library → git apply |
3 wrong (172 wrong, 411 rejected) | 0 (3 wrong, 961 rejected) | 0 (1,037 rejected) | not run |
round trip (patch old (unified-with-context …)) |
1,257/1,257 (same on master) | same | same | same |
diff -U0/-U1/-U3 and git diff -U0/-U3 output fed to patch gives 1,257/1,257 right for each of the five on head. On master each is 541 wrong. The three git misses (pairs 449, 890 and 1229) are the --unidiff-zero bug the body describes. git misapplies GNU diff -U0's output and git diff -U0's own output for the same three pairs, and GNU patch applies all three correctly.
Findings
1. The context-line half of the marker check is not pinned (diff.carp:469). If I delete (when (/= marked (= cursor nonl-end)) (set! failed true)) from the branch, the suite still passes 124/0. Keeping either arm alone also passes. That leaves two rows of the body's strictness table untested:
(patch "a\nb\n" "@@ -1,2 +1,2 @@\n-a\n+A\n b\n\\ No newline at end of file"): head givesNothing, and the mutant givesJust "A\nb", git's answer that drops the newline (table row 3).(patch "a\nb" "@@ -1,2 +1,2 @@\n-a\n+A\n b"): head givesNothing, and the mutant givesJust "A\nb\n"(table row 4).
The body's mutant table covers only the - branch's copy of this check (:477).
2. Three more applier mutants survive the suite (124/0). Each one restores part of master's phantom line or misreads an empty result:
old-line?bounded by(Array.length orig)instead ofolen(:422). An empty context line can then match the final newline.(patch "a\n" "@@ -1,2 +1,2 @@\n a\n \n")givesJust "a\n\n"instead ofNothing, and GNU patch and git both reject it.- The range check against
(Array.length &orig)instead ofolen(:497).(patch "a\n" "@@ -2,0 +3 @@\n+X")givesJust "a\n\nX\n"instead ofNothing. GNU patch and git both apply this hunk anyway, at the end of the file, and give"a\nX\n", so pinningNothingis a choice. The mutant's extra empty line is wrong either way. (if ends …)without(not (Array.empty? &out))(:522).(patch "a\n" "@@ -1 +0,0 @@\n-a")givesJust "\n"instead ofJust "", and both tools give"".
A fourth survivor, (side-end xs nl) for ol at :244, is equivalent: that branch only runs when ol equals nl or the part is empty.
These five rows pin all of the above. They go right after the row "patch returns Nothing on a no-newline marker before the last line". With them the file is carp-fmt --check clean and the suite passes 129/0 on head, and each row fails on its mutant (both arms of finding 1 included):
(assert-equal test
&(the (Maybe String) (Maybe.Nothing))
&(patch "a\nb\n"
"@@ -1,2 +1,2 @@\n-a\n+A\n b\n\\ No newline at end of file")
"patch returns Nothing on a context marker the original contradicts")
(assert-equal test
&(the (Maybe String) (Maybe.Nothing))
&(patch "a\nb" "@@ -1,2 +1,2 @@\n-a\n+A\n b")
"patch returns Nothing when a last context line lacks its marker")
(assert-equal test
&(Maybe.Just @"")
&(patch "a\n" "@@ -1 +0,0 @@\n-a")
"patch that deletes every line gives the empty text")
(assert-equal test
&(the (Maybe String) (Maybe.Nothing))
&(patch "a\n" "@@ -1,2 +1,2 @@\n a\n \n")
"patch does not read the final newline as an empty line")
(assert-equal test
&(the (Maybe String) (Maybe.Nothing))
&(patch "a\n" "@@ -2,0 +3 @@\n+X")
"patch returns Nothing on a zero-count hunk past the last line")
3. For the "your call" on non-line-diff arrays: the library's own string-diff is affected. The doc strings of unified (diff.carp:381-385) and unified-with-context (:276-281) still accept "a diff of string arrays". string-diff produces one whose last element is never "", because String.words drops empty strings. So every word diff whose hunk reaches the last word now ends with a marker. (unified (string-diff "the quick fox" "the slow fox")) now prints:
@@ -1,3 +1,3 @@
the
+slow
-quick
fox
\ No newline at end of file
Master printed the same output without the last line. None of the 47 local carpentry clones calls Diff.unified, unified-with-context or Diff.patch outside diff itself, so nothing downstream changes. If the array-level reading stays, the doc could say that word diffs get the marker too. The text-level entry point the body mentions would avoid the marker on word diffs altogether.
4. Minor: a stray marker is skipped, but the doc says it gives Nothing (:486 against the doc at :433-434). A \ line directly after a @@ header, or a second \ line after a marker, is ignored. For example, (patch "a" "@@ -1 +1 @@\n\\ No newline at end of file\n-a\n\\ No newline at end of file\n+b") gives Just "b\n". git agrees and GNU patch rejects both shapes. The result is the same as without the stray line, so only the doc's promise that a marker which "does not follow its side's last line" gives Nothing is off. Either reject a \ line that doesn't follow a body line, or soften the sentence. This one is optional.
5. Body nit: one row of the strictness table. The row "marker after +X, original lines follow the hunk" says git rejects. That holds only for the zero-context form without --unidiff-zero.
- With
--unidiff-zero,@@ -1,0 +2 @@\n+X\n\\ No newline at end of fileagainst"a\nb\n"gives"a\nXb\n". - With a context line after the marker (
@@ -1,2 +1,3 @@\n a\n+X\n\\ …\n b), git also applies and gives"a\nXb\n", while GNU patch rejects.
So git applies in three rows, not two, and joins lines in two of them. Nothing is the right call there either way.
GNU patch also applies rows 3 and 4 only by dropping the context line: it reports Hunk #1 succeeded at 1 with fuzz 1. With -F0 it rejects both. Those two rows therefore support the library's Nothing as well.
Pre-existing and out of scope: an empty context line from diff --suppress-blank-empty still gives Nothing. For example, (patch "\nb" "@@ -1,2 +1,2 @@\n\n-b\n\\ No newline at end of file\n+c") returns Nothing, where GNU patch and git both give "\nc\n". This is the open item from #16's review, and this PR doesn't touch it.
Verdict: merge
I found no wrong result. GNU patch got 0 wrong in 6,285 runs, five tools' output on 1,257 pairs applied correctly, and all 6,285 round trips held; the only misses were git's own -U0 bug. The PR doesn't leak and costs nothing in speed. The findings are test gaps around behavior that is already correct, plus doc and body precision. Adding the five rows above before merging would pin the strictness the body promises.
Follow-up to #16:
unified,unified-with-contextandpatchnow treat a text's final newline the way diff(1), patch(1) and git do, including the\ No newline at end of filemarker.What changed
Each side of a diff is read the way
line-diffsplits a text andpatchrejoins it. A final""element is the text's final newline, not a line. A final non-empty element is a last line without a newline.[]and[""]are both the empty text.Writer. The phantom
""is no longer written as a context,-or+line, and it is no longer counted in the header. A last line without a newline is followed by the marker: after-xfor the old side, after+xfor the new side, afterxfor both. When the sides differ only in that newline, the line is written as a change, asdiff -uwrites it ("a\nb"→"a\nb\n"givesa,-b,\ No newline at end of file,+b). This works whichever orderDiff.diffemits the insertion and deletion in, because the sides are normalised before rendering. The trailing-context and zero-count rules from #16 are unchanged.Applier. After
-the marker describes the original's last line, after+the new side's, afterboth. The result ends with a newline exactly when the new side's last line does. When the last hunk reaches the end of the original, the markers decide that; otherwise the original's untouched tail does. An empty original has zero lines.Strictness.
patchis documented as a safe applier, so a marker that is missing, contradicts the original, or does not follow its side's last line givesNothing. Here is what the tools do (GNU patch 2.7.6, git 2.39.5):patch-b+ marker, original ends with a newlineNothing-b, no marker, original has no final newlineNothingb+ marker, original ends with a newlineNothingb, no marker, original has no final newlineNothing-a, more old lines followNothing+X,+Yfollows in the hunkXYNothing+X, original lines follow the hunkNothingThe library rejects wherever git does. In the two rows where git still applies, it drops the original's newline in one and joins two lines in the other. A zero-count hunk appended after a last line that lacks a newline is not a marker case, so it still applies: the result is
…b\nY\nZ\n, as with GNU patch (git writes…bY\nZ\n).For Veit
unifiedalso accepts diffs of arrays that did not come fromline-diff, and under this convention an array whose last element is not""reads as a text without a final newline. So(unified (diff &[] &[@"a" @"b"]))now ends with the marker after+b. The two suite rows that pinned this changed: "an empty old side is -0,0" and "an empty new side is +0,0". The alternative would be a text-level entry point that takes two strings. I did not add one; your call.Changed expectations
Each was checked against
diff -u(or GNU patch) for the same two files:unifiedrows gain the marker after their last line, because their texts lack a final newline: "single-hunk diff with correct header", "change at start", "change at end" (two markers), "multiple hunks for distant changes" (two), "unified-with-context 3 matches unified default", "unified-with-context 1 splits hunks". The headers and lines matchdiff -u, which writes-before+.patchrows run against originals without a final newline. Their patches now carry the markerdiff -uwrites there; without it they areNothing. The rows are "no-op patch to a latin-1 document", "skips file headers before the first hunk" and "reads --- and +++ inside a hunk as body lines"."a\nb\nc\nd\ne\nY\nZ\n", the same as GNU patch.Measurements
The kit lives outside the tree. It has 616 pairs: all 16 ordered pairs of
"","\n","a","a\n"; 300 random pairs (0–20 lines overa b c d, each side's final newline random); and 300 newline-only pairs (150 texts, newline added and removed). Wrong results, master → branch:patchOn the newline-only pairs, master was wrong for all 300 in (b) and for all 300 in (a), except git at ctx 0 (150).
(c) The round trip
(patch old (unified-with-context (line-diff old new) ctx))wasJust newfor 1848/1848 on both master and the branch. Master's writer and applier agreed on the phantom line.The one git ctx-0 failure on the branch is a case where the library writes the same hunks as
diff -U0.git apply --unidiff-zerodrops the final newline ondiff -U0's own output for that pair too; GNU patch applies both correctly.Tests
The suite has 10 new rows, next to the related
unifiedandpatchrows:unified: the newline-only change in both directions, both texts ending with a newline, and""↔"a".patch: two pasted GNUdiff -uoutputs that carry the marker, one of them against an empty original, plus the contradicting, missing and misplaced marker cases.Against master's
diff.carpthe new suite is 105 passed / 19 failed. All 10 new rows fail there, plus 9 of the changed expectations: the 8unifiedrows and the zero-count append. The threepatchrows that only gained a marker pass on master, because master skips markers.Mutants of the new logic, each killed by a committed row:
""counted as a line by the writer-line""counted as a line by the applierSuite: 124/0 on armhf and on LP64 (
aarch64-linux-gnu-gcc, no-O). Master is 114/0.carp-fmt --checkandanglerare clean on both files.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.