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

diff: complete support for git patchid by herrerog · Pull Request #5306 · libgit2/libgit2 · GitHub

/ libgit2 Public

diff: complete support for git patchid - #5306

Merged
pks-t merged 4 commits into
libgit2:masterfrom
herrerog:patchid
Nov 28, 2019
ConversationCommits (4)ChecksFiles changed
Merged

diff: complete support for git patchid#5306
pks-t merged 4 commits into
libgit2:masterfrom
herrerog:patchid

Conversation

herrerog commented Nov 19, 2019

Copy link
Copy Markdown
Contributor

Hi,
I noticed that git_diff_patchid wasn't returning a correct hash in some cases.
So I fixed those and added some tests for it.
Please let me know what you think.
Thanks!

diff_print: add a new 'print_index' flag when printing diff.
accd784 
Add a new 'print_index' flag to let the caller decide whether or not
'index <oid>..<oid>' should be printed.
Since patch id needs not to have index when hashing a patch, it will be
useful soon.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
diff_print: add support for GIT_DIFF_FORMAT_PATCH_ID.
b921964 
Git is generating patch-id using a stripped down version of a patch
where hunk header and index information are not present.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
patch_parse: correct parsing of patch containing not shown binary data.
048e94a 
When not shown binary data is added or removed in a patch, patch parser
is currently returning 'error -1 - corrupt git binary header at line 4'.
Fix it by correctly handling case where binary data is added/removed.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>

ethomson commented Nov 25, 2019

Copy link
Copy Markdown
Member

Overall, this looks very nice to me. Thanks for the PR!

/cc @pks-t to look at some of the factoring, especially since patchid was his work originally and he has more familiarity.

Note that the build failures appear to be due to #5308.

pks-t requested changes Nov 28, 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

Thanks a lot for your pull request! I like the changes quite a lot, they make the code simpler while still extending our support for patch IDs. I hate to say that my only issue is a stylistic one, so please forgive me, but please remove the braces around function invocations as they do not match our style. Other than that there was a needless call to git_buf_sanitize that I'd ask you to remove.

All in all, very well done and I'm happy to merge as soon as these two small issues are fixed!

Comment thread src/diff_print.c
Comment thread src/diff.c Outdated
Comment thread src/diff.c
Comment thread src/diff.c Outdated
Comment thread src/diff.c Outdated
diff: make patchid computation work with all types of commits.
ece5bb5 
Current implementation of patchid is not computing a correct patchid
when given a patch where, for example, a new file is added or removed.
Some more corner cases need to be handled to have same behavior as git
patch-id command.
Add some more tests to cover those corner cases.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>

herrerog commented Nov 28, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for your nice feedback, happy to help :)
I updated the branch.

pks-t merged commit fb439c9 into libgit2:master Nov 28, 2019

pks-t commented Nov 28, 2019

Copy link
Copy Markdown
Member

Thanks a lot for this well-crafted pull request, @herrerog!

snyk-bot mentioned this pull request Feb 23, 2020
Open
snyk-bot mentioned this pull request Mar 12, 2020
Merged
snyk-bot mentioned this pull request Apr 22, 2020
Open
snyk-bot mentioned this pull request May 5, 2020
Open
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 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.

3 participants

Back | FazBrowse Home | New Git URL