Skip to content

Fix how Diff handles commits that contain submodule changes #947

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

Merged
merged 4 commits into from
Oct 21, 2019
Merged

Conversation

thetwoj
Copy link
Contributor

@thetwoj thetwoj commented Oct 21, 2019

This PR addresses the issue reported in #891

The problem was that Diff was receiving the parent Repo but commit hashes from the submodule repo, resulting in Diff being unable to find the commits. This was due to the output of git diff-tree <parent_commit_1> <parent_commit_2> containing the hashes from the submodule:

$ git diff-tree 53a7920fe21c49edb4471978361c1f23bf4034b1 d4f583c621da537db510ee19b2198d2192df84a9
:160000 160000 8324c24a8c6d60bd83a5356e81e5278e54ef49fc 5ac7216dccc0c1352d53a17efbc5b3bfafc0fd46 M	sub

This implementation fixes that issue by comparing the a_rawpath to all of the repo's submodule paths and overwriting repo to the submodule's repo if there's a match. If there are ideas concerning a more efficient way to identify this situation I'm more than happy to entertain other approaches.

The test added by this PR is based off of the repro provided in #891. While it's a great repro case I don't feel like it's a particularly elegant test so I'm open to suggestions there as well.

Fixed a typo in a comment in cmd.py and resolved a deprecation warning within the test_diff.py file while I was in there. I also added git submodule update --init --recursive to init-tests-after-clone.sh because there are a decent number of tests that fail without gitdb and smmap cloned.

@codecov-io
Copy link

Codecov Report

Merging #947 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   93.65%   93.68%   +0.03%     
==========================================
  Files          59       59              
  Lines        9851     9887      +36     
==========================================
+ Hits         9226     9263      +37     
+ Misses        625      624       -1
Impacted Files Coverage Δ
git/cmd.py 82.94% <ø> (ø) ⬆️
git/test/test_diff.py 100% <100%> (ø) ⬆️
git/diff.py 98.03% <100%> (+0.03%) ⬆️
git/objects/submodule/base.py 93.25% <0%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fd0902...dd546a1. Read the comment docs.

@Byron Byron added this to the v3.0.4 - Bugfixes milestone Oct 21, 2019
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks so much @thetwoj! This is truly fantastic work.

A particular highlight of this PR is the explanatory summary, which succinctly says it all!

When looking at the code, I wouldn't know how to make it better with the options at hand. For the lack of specific regression tests and facilities to analyse them, I would assume the additional checking code will not add considerable runtime. And if it does, it can be fixed in a later version.

@Byron Byron merged commit 44496c6 into gitpython-developers:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants