-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix issue #19: emtpy new git file #86
Conversation
Hi @matiasb, Do you mind taking a look here? It's a kinda annoying issue for us. We use your library in a CI, and it's blind for empty new files currently. |
Hi @Felixoid, thanks, happy new year for you too! Thanks for the PR 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I will try some refactoring after merging (added inline notes, fyi).
Thanks!
@@ -473,8 +475,10 @@ def _parse(self, diff, encoding, metadata_only): | |||
# check for a git file rename | |||
is_diff_git_header = RE_DIFF_GIT_HEADER.match(line) | |||
if is_diff_git_header: | |||
if patch_info is None: | |||
patch_info = PatchInfo() | |||
if current_file is not None and not is_file_appended: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could refactor this to avoid introducing the is_file_appended
flag by always setting up a PatchedFile
when a git header is found (and some minor changes to accommodate for that), will check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAI remember I was in a rush of fixing it. Most probably, there is a more elegant way to fix it. I am 100% sure you are more confident with code! =)
Merging, thanks 👍 |
This is needed to fix a bug with it not noticing new empty files being added. The fix first appears in v0.7.1[[1]] [[2]] It was reported in the pip project[[3]]. [1]: matiasb/python-unidiff#19 [2]: matiasb/python-unidiff#86 [3]: pypa/pip#11969
Hello. I've faced the issue today in ClickHouse/ClickHouse#32911 that the empty new files aren't fetched. It means, our CI may not work correctly for some other edge cases.
Here I've implemented the parsing for such diffs and tests for them:
diff --git a/boo.bin b/boo.bin new file mode 100644 index 0000000..ae000000
The PR fixes #19