Skip to content

patch_parse: handle missing newline indicator in old file#5159

Merged
ethomson merged 4 commits intolibgit2:masterfrom
pks-t:pks/patch-parse-old-missing-nl
Jul 20, 2019
Merged

patch_parse: handle missing newline indicator in old file#5159
ethomson merged 4 commits intolibgit2:masterfrom
pks-t:pks/patch-parse-old-missing-nl

Conversation

@pks-t
Copy link
Member

@pks-t pks-t commented Jul 5, 2019

When either the old or new file contents have no newline at the end of
the file, then git-diff(1) will print out a "\ No newline at end of
file" indicator. While we do correctly handle this in the case where the
new file has this indcator, we fail to parse patches where the old file
is missing a newline at EOF.

Fix this bug by handling and missing newline indicators in the old file.
Add tests to verify that we can parse such files.

I've tried to modify our patch printing logic to also emit the "No newline at end of file" markers, but it turned out to be non-trivial. Many of the diff/patch structs are part of our public API and thus cannot be easily changed, so I didn't quite know where to put the missing newline indicators. So I've stopped short of actually verifying that we round-trip such patches correctly when parsing and printing them, as we do not do this correctly right now.

Fixes #5157

@pks-t pks-t mentioned this pull request Jul 5, 2019
Closed
@pks-t
Copy link
Member Author

pks-t commented Jul 5, 2019

Fixes #5153

@pks-t pks-t mentioned this pull request Jul 5, 2019
Closed
@albfan
Copy link
Contributor

albfan commented Jul 5, 2019

@pks-t Nice! didn't see that. we can add the patch until it is fixed, really great! looking for this to be merged and the roadmap for releasing.

@eaigner
Copy link

eaigner commented Jul 5, 2019

I don't know if this is related, but I tested your PR and it seems to work. But when I try to apply this patch, I cannot.

I applied the following patch in git without problems to the index

diff --git a/test/diff2.txt b/test/diff2.txt
index 212a1d6d8..b27c6ecb3 100644
--- a/test/diff2.txt
+++ b/test/diff2.txt
@@ -1,4 +1,4 @@
 This is just
 a text file to
 test git diff file
-changes
\ No newline at end of file
+changes

with

era$ git apply --cached -- p1.patch

However, when I try to apply it via git_apply, it gives the following error

hunk at line 1 did not apply

@eaigner
Copy link

eaigner commented Jul 6, 2019

I have noticed that none of the patch parse tests actually verify if a patch was correctly parsed.

For instance parsing the following snippet with git_diff_from_buffer

diff --git a/test/diff2.txt b/test/diff2.txt
index 212a1d6..0dff799 100644
--- a/test/diff2.txt
+++ b/test/diff2.txt
@@ -1,3 +1,2 @@
 This is just
-a text file to
 changes
\ No newline at end of file

and then printing the resulting diff using git_diff_to_buf results in

diff --git a/test/diff2.txt b/test/diff2.txt
index 212a1d6..0dff799 100644
--- a/test/diff2.txt
+++ b/test/diff2.txt
@@ -1,3 +1,2 @@
 This is just
-a text file to
 changes

Notice that the \ No newline at end of file is missing afterward.
The patch could not be applied again in this state.

@albfan
Copy link
Contributor

albfan commented Jul 6, 2019 •
edited
Loading

That can be either we don't print out the diff about newline or we are just ignoring it. I will do some checks to provide feedback

@eaigner
Copy link

eaigner commented Jul 6, 2019

But \ No newline at end of file cannot be ignored, or the patch will not be correct.

@eaigner eaigner mentioned this pull request Jul 6, 2019
Closed
@eaigner
Copy link

eaigner commented Jul 6, 2019

I fixed this in #5161, please check!

@pks-t
Copy link
Member Author

pks-t commented Jul 11, 2019

I have noticed that none of the patch parse tests actually verify if a patch was correctly parsed.

I know, as noticed in my commit message :) Anyway, I'm closing this PR in favour of yours, so thanks a lot for fixing this!

@pks-t pks-t closed this Jul 11, 2019
pks-t and others added 4 commits July 11, 2019 11:34
@pks-t
diff: ignore EOFNL for computing patch IDs
001d76e 
The patch ID is supposed to be mostly context-insignificant and
thus only includes added or deleted lines. As such, we shouldn't honor
end-of-file-without-newline markers in diffs.

Ignore such lines to fix how we compute the patch ID for such diffs.
@pks-t
apply: refactor to use a switch statement
b30dab8
@pks-t
patch_parse: handle missing newline indicator in old file
3f855fe 
When either the old or new file contents have no newline at the end of
the file, then git-diff(1) will print out a "\ No newline at end of
file" indicator. While we do correctly handle this in the case where the
new file has this indcator, we fail to parse patches where the old file
is missing a newline at EOF.

Fix this bug by handling and missing newline indicators in the old file.
Add tests to verify that we can parse such files.
@pks-t
patch_parse: ensure valid patch output with EOFNL
b089328
@pks-t pks-t reopened this Jul 11, 2019
@pks-t pks-t force-pushed the pks/patch-parse-old-missing-nl branch from ea99541 to b089328 Compare July 11, 2019 10:16
@pks-t
Copy link
Member Author

pks-t commented Jul 11, 2019

This supersedes #5161 again.

@ethomson
Copy link
Member

ethomson commented Jul 20, 2019

Fixes #5177

@ethomson
Copy link
Member

ethomson commented Jul 20, 2019

Nice fix, thanks @eaigner and @pks-t for the code, and @albfan, @ddevault and @mkaito for the helpful reports.

@ethomson ethomson merged commit fd7a384 into libgit2:master Jul 20, 2019
@ddevault
Copy link
Contributor

ddevault commented Jul 20, 2019

Thanks!

@pks-t pks-t deleted the pks/patch-parse-old-missing-nl branch July 20, 2019 16:37
@pks-t pks-t mentioned this pull request Jul 20, 2019
Merged
@implausible implausible mentioned this pull request Jul 23, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 23, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
@snyk-bot snyk-bot mentioned this pull request May 5, 2020
Open
@BBerabi BBerabi mentioned this pull request Jul 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ethomson ethomson ethomson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

diff: git_diff_from_buffer fails to parse diff with comment lines

5 participants

@pks-t @albfan @eaigner @ethomson @ddevault