FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

patch_parse: properly parse EOFNL comment lines by eaigner · Pull Request #5161 · libgit2/libgit2 · GitHub

/ libgit2 Public

patch_parse: properly parse EOFNL comment lines - #5161

Closed
eaigner wants to merge 2 commits into
libgit2:masterfrom
eaigner:apply
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

eaigner commented Jul 6, 2019

Copy link
Copy Markdown

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

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 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 commented Jul 6, 2019
View reviewed changes
Comment thread src/patch_parse.c
eaigner mentioned this pull request Jul 6, 2019
Merged
eaigner changed the title patch_parse: properly parse EOF comment lines patch_parse: properly parse EOFNL comment lines Jul 6, 2019

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 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 mentioned this pull request Jul 8, 2019
Merged

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 requested changes Jul 11, 2019
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

But the project is set to indent with TAB

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

pks-t closed this Jul 11, 2019

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 commented Jul 11, 2019

Copy link
Copy Markdown
Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL