Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply fails to handle diffs with files without newline or patches that remove newline at end of the file #57

Open
fzakaria opened this issue Mar 4, 2025 · 4 comments

Comments

@fzakaria
Copy link

fzakaria commented Mar 4, 2025

I encountered this issue via aspect-build/rules_lint#487

tl;dr; If a file is missing a newline at the end or the patch removes the newline at the end, go-gitdiff fails applying the patch with the error:

conflict: fragment line does not match src line

I'll push a reproduction example through a failed test, where git succeeds.

> git apply gitdiff/testdata/apply/file_text_modify.patch
> echo $?
0
> go test ./...
--- FAIL: TestApplyFile (0.00s)
    --- FAIL: TestApplyFile/textModify (0.00s)
        apply_test.go:190: unexpected error applying: at 4: fragment 1 at 5: conflict: fragment line does not match src line
FAIL
FAIL    github.com/bluekeyes/go-gitdiff/gitdiff 0.321s
FAIL

The offending check can be found

func applyTextLine(dst io.Writer, line Line, preimage [][]byte, i int64) (err error) {

@fzakaria
Copy link
Author

fzakaria commented Mar 4, 2025

Investigating more; seems to be that diff -u can produce the "No newline" statement multiple times.

> diff -u  gitdiff/testdata/apply/file_text.src gitdiff/testdata/apply/file_text_modify.out
--- gitdiff/testdata/apply/file_text.src        2025-03-04 14:16:41
+++ gitdiff/testdata/apply/file_text_modify.out 2025-03-04 13:35:04
@@ -1,4 +1,4 @@
-this is line 1
+the first line is different
 this is line 2
 this is line 3
 this is line 4
\ No newline at end of file
@@ -17,10 +17,10 @@
 this is line 17
 this is line 18
 this is line 19
+this line offsets all the line numbers!
 this is line 20
 this is line 21
-this is line 22
-this is line 23
+until here, now we're back on track!
 this is line 24
 this is line 25
 this is line 26
\ No newline at end of file
@@ -53,10 +53,10 @@
 this is line 53
 this is line 54
 this is line 55
-this is line 56
-this is line 57
-this is line 58
-this is line 59
+once upon a time, a line
+  in a text
+    file
+  changed
 this is line 60
 this is line 61
 this is line 62
\ No newline at end of file
@@ -130,8 +130,8 @@
 this is line 130
 this is line 131
 this is line 132
-this is line 133
-this is line 134
+this line was bad and has been removed
+this line was REDACTED and has been REDACTED
 this is line 135
 this is line 136
 this is line 137
\ No newline at end of file
@@ -161,12 +161,7 @@
 this is line 161
 this is line 162
 this is line 163
-this is line 164
-this is line 165
-this is line 166
-this is line 167
-this is line 168
-this is line 169
+the number on the remaining lines is 5 ahead of their actual position in the file
 this is line 170
 this is line 171
 this is line 172
\ No newline at end of file
@@ -197,4 +192,4 @@
 this is line 197
 this is line 198
 this is line 199
-this is line 200
+this is line 200
\ No newline at end of file

@fzakaria
Copy link
Author

fzakaria commented Mar 4, 2025

Okay even more discovery!
The ability for multiple No newline at end of file seems to be a part of Apple BSD diff :(

I did the same with GNU diff and found:

> /opt/homebrew/bin/diff -u  gitdiff/testdata/apply/file_text.src gitdiff/testdata/apply/file_text_modify.out
--- gitdiff/testdata/apply/file_text.src        2025-03-04 15:11:22.172538791 -0800
+++ gitdiff/testdata/apply/file_text_modify.out 2025-03-04 13:35:04.387135108 -0800
@@ -1,4 +1,4 @@
-this is line 1
+the first line is different
 this is line 2
 this is line 3
 this is line 4
@@ -17,10 +17,10 @@
 this is line 17
 this is line 18
 this is line 19
+this line offsets all the line numbers!
 this is line 20
 this is line 21
-this is line 22
-this is line 23
+until here, now we're back on track!
 this is line 24
 this is line 25
 this is line 26
@@ -53,10 +53,10 @@
 this is line 53
 this is line 54
 this is line 55
-this is line 56
-this is line 57
-this is line 58
-this is line 59
+once upon a time, a line
+  in a text
+    file
+  changed
 this is line 60
 this is line 61
 this is line 62
@@ -130,8 +130,8 @@
 this is line 130
 this is line 131
 this is line 132
-this is line 133
-this is line 134
+this line was bad and has been removed
+this line was REDACTED and has been REDACTED
 this is line 135
 this is line 136
 this is line 137
@@ -161,12 +161,7 @@
 this is line 161
 this is line 162
 this is line 163
-this is line 164
-this is line 165
-this is line 166
-this is line 167
-this is line 168
-this is line 169
+the number on the remaining lines is 5 ahead of their actual position in the file
 this is line 170
 this is line 171
 this is line 172
@@ -197,4 +192,4 @@
 this is line 197
 this is line 198
 this is line 199
-this is line 200
+this is line 200
\ No newline at end of file

Git seems to fail on that diff style as well.

@bluekeyes
Copy link
Owner

Thanks for reporting this. If I understand your investigation, this is specifically a problem with the output of the BSD diff tool, which includes the \ No newline at end of file marker after every fragment instead of only after the last one? I'm not familiar with the rules_lint project you mentioned, but it sounds like it might be running the version of the diff tool that exists on the local machine and then trying to apply the result as a patch.

If Git supports this type of patches, I should fix this library to also accept them. If Git fails on this style of patch, I'm less inclined to add support here, but could at least see how hard it might be. I'm reasonably confident that this works correctly with Git-style patches as there were existing tests for adding a final newline and changing content without changing the final newline. You were correct that there was no test for removing the final newline, but that seems to work as expected in the current version.

@fzakaria
Copy link
Author

fzakaria commented Mar 5, 2025

@bluekeyes it's kind of hard to replicate because the "algorithm" for producing the hunks is a bit tricky.

I think git is failing to apply this same patch which I sent an email to the mailing list.
https://lore.kernel.org/git/CACCo2jkKraSnGUeRqJSuis9EfiubxEbm1dT2WZk7gfk9xjQ1rg@mail.gmail.com/T/#u

It's not clear to me if it's a "bug" in Git or something purposeful.

Given the prevalence of MacOS for developers (Although I daily drive NixOS :P) seems like a foot gun given that patch works with it.

Anyways, this caused me a lot of hair pulling trying to debug; thought I'd record the info here.

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

No branches or pull requests

2 participants