Skip to content
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 repo.index.diff("HEAD", create_patch=True) always returning an empty list #948

Merged
merged 2 commits into from Oct 22, 2019

Conversation

@thetwoj
Copy link
Contributor

@thetwoj thetwoj commented Oct 21, 2019

This PR resolves the issue reported in #852

As @jadinm correctly identified in #852, this bug centers around the fact that the regex utilized by _index_from_patch_format() in git/diff.py always expects the first line of the diff to have the a file first, then b:

diff --git a/the_file.py b/the_file.py

When the other passed to repo.index.diff(other) is not None the -R argument is added to the git command, resulting in the positions of the a and b files being flipped in the output and causing the _index_from_patch_format() function invoked by the create_patch option to not find a regex match:

diff --git b/the_file.py a/the_file.py

This fix makes the regex slightly more permissive, allowing it to match either order of the a and b files in the output of git diff.

For test coverage of this change I added assertions to an existing test within test_diff.py that set up everything needed to reproduce this bug.

Massive props to @jadinm, their troubleshooting and feedback on the issue significantly shortened the investigation required to get this one sorted out 🎉

@Byron Byron added this to the v3.0.4 - Bugfixes milestone Oct 22, 2019
@Byron
Byron approved these changes Oct 22, 2019
Copy link
Member

@Byron Byron left a comment

I am absolutely amazed by your contribution, and truly value the time and energy you must put into them!

Please let me know if you can think of anything that I can do to make your life as a contributor easier.

@Byron Byron merged commit 38c624f into gitpython-developers:master Oct 22, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
DeepSource: Python Analysis Passed: No blocking issues detected.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thetwoj
Copy link
Contributor Author

@thetwoj thetwoj commented Oct 22, 2019

Thanks @Byron! I appreciate your willingness to entertain these PRs and how promptly you review them, it makes contributing fun 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.