Skip to content

patch_parse: properly parse EOFNL comment lines#5161

Closed
eaigner wants to merge 2 commits into
libgit2:masterfrom
eaigner:apply
Closed

patch_parse: properly parse EOFNL comment lines#5161
eaigner wants to merge 2 commits into
libgit2:masterfrom
eaigner:apply

Conversation

@eaigner

@eaigner eaigner commented Jul 6, 2019

Copy link
Copy Markdown

The patch output of #5159 did not produce a valid EOF comment

@pks-t @eaigner
patch_parse: handle missing newline indicator in old file
685687c 
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.
@eaigner

eaigner commented Jul 6, 2019

Copy link
Copy Markdown
Author

@pks-t I based this on your PR and validated the patch output. Would appreciate a quick look if I missed something.

eaigner
eaigner commented Jul 6, 2019
View reviewed changes
Comment thread src/patch_parse.c
@eaigner eaigner mentioned this pull request Jul 6, 2019
Merged
@eaigner eaigner changed the title patch_parse: properly parse EOF comment lines patch_parse: properly parse EOFNL comment lines Jul 6, 2019
@eaigner

eaigner commented Jul 6, 2019

Copy link
Copy Markdown
Author

apply::fromdiff::patching_correctly_truncates_source still failing, but i'm not quite sure what the goal of this test is

@albfan

albfan commented Jul 8, 2019

Copy link
Copy Markdown
Contributor 
* 329eedb84 (HEAD -> apply) ensure valid patch output
* c8406a3b5 (pks/patch-parse-old-missing-nl) patch_parse: handle missing newline indicator in old file
* 960d2a070 (origin/wip/albfan/diff_buffers, wip/albfan/diff_buffers) examples: consolidate includes into "common.h"
* 3be09b6c0 Compare buffers in diff example
*   398412ccd (upstream/master, upstream/HEAD, pks-t/master, origin/master, master) Merge pull request #5143 from libgit2/ethomson/warnings

I just rebase all the patches to test with examples, and will review (just if that helps). I called it https://github.com/albfan/libgit2/pull/new/fix-patch-to-diff-no-newline

@albfan albfan mentioned this pull request Jul 8, 2019
Merged
@albfan

albfan commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

@pks-t without a deeper lot at this, lg2 diff --no-index works as it should with files without newline at end.

Still not sure how this is translated to a hunk, so I will test with our vala integration project: https://gitlab.gnome.org/albfan/diferencia

pks-t
pks-t requested changes Jul 11, 2019
View reviewed changes

@pks-t pks-t left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks a lot for this! It mostly looks good to me, but we definitely need to fix our patch ID generation first before we can merge this. As said, just let me know whether you want to tackle that, otherwise I'll take over again.

Comment thread src/apply.c
next = git_array_get(patch->lines, linenum + 1);
if (next->origin == GIT_DIFF_LINE_CONTEXT_EOFNL ||
next->origin == GIT_DIFF_LINE_DEL_EOFNL ||
next->origin == GIT_DIFF_LINE_ADD_EOFNL) {

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indentation is off here. If conditionals span multiple lines, then the conditions should be indented by four spaces relative to the if, only:

if (next->origin == GIT_DIFF_LINE_CONTEXT_EOFNL ||
    next->origin == GIT_DIFF_LINE_DEL_EOFNL ||
    next->origin == GIT_DIFF_LINE_ADD_EOFNL) {
        ...
}

@eaigner eaigner Jul 11, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the project is set to indent with TAB

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We had some misunderstandings around that in the past, but our rule is to follow git.git and thus linux.git's style. They use 8-space tabs in general, but indent multiline conditionals with four spaces. I've never used editorconfig, but if it is possible to specify this rule then we should certainly fix it.

Comment thread tests/patch/parse.c

cl_git_pass(git_patch_from_buffer(&patch, content, strlen(content), NULL));
cl_git_pass(git_patch_to_buf(&buf, patch));
cl_assert_equal_strn(git_buf_cstr(&buf), content, strlen(content));

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a cl_assert_equal_str, otherwise we wouldn't notice if the generated patch had unexpected trailing content

Comment thread tests/patch/parse.c
cl_assert_equal_i(0, delta->new_file.size);
}

static void ensure_identical_patch_inout(const char *content) {

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: Opening braces belong on their own line

Comment thread tests/diff/patchid.c
void test_diff_patchid__filename_with_spaces(void)
{
verify_patch_id(PATCH_APPEND_NO_NL, "f0ba05413beaef743b630e796153839462ee477a");
verify_patch_id(PATCH_APPEND_NO_NL, "cc0d67fc516f3378baa8a72b248da6e26c5dbf4a");

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, though. git-patch-id(1) prints "f0ba05413beaef743b630e796153839462ee477a", so we cannot simply alter the expected ID here. So this indicates that we have to adjust how we calculate the patch ID when there's no trailing newline. Just let me know whether you want to amend that or whether I should take over again :)

@eaigner eaigner Jul 11, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please take over. probably better someone fixes this that knows what the issue is ^^

Comment thread src/diff.c
break;
default:
git_error_set(GIT_ERROR_PATCH, "invalid line origin for patch");
return -1;

@pks-t pks-t Jul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to avoid re-indenting the whole block so that it's easier to spot what exactly has changed

@pks-t pks-t closed this Jul 11, 2019
@pks-t

pks-t commented Jul 11, 2019

Copy link
Copy Markdown
Member

Oops, sorry for closing your PR, I confused this one with #5159. Anyway, I've added your fix to #5159 now and fixed two issues with patch IDs and another one with how we strip newlines when applying the patch.

I can reopen this one if you feel like the combined fix in #5159 isn't sufficient yet, but I didn't want to force-push over your PR as I did quite some changes to your patch.

@eaigner

eaigner commented Jul 11, 2019

Copy link
Copy Markdown
Author

It's fine! Thanks for filling in the missing stuff. /closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pks-t pks-t pks-t requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eaigner @albfan @pks-t