Fix how Diff handles commits that contain submodule changes#947
Fix how Diff handles commits that contain submodule changes#947Byron merged 4 commits intogitpython-developers:masterfrom thetwoj:891
Conversation
…re recursively cloned as well
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Byron
left a comment
There was a problem hiding this comment.
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.
This PR addresses the issue reported in #891
The problem was that
Diffwas receiving the parentRepobut commit hashes from the submodule repo, resulting inDiffbeing unable to find the commits. This was due to the output ofgit diff-tree <parent_commit_1> <parent_commit_2>containing the hashes from the submodule:This implementation fixes that issue by comparing the
a_rawpathto all of the repo's submodulepaths and overwritingrepoto 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.pyand resolved a deprecation warning within thetest_diff.pyfile while I was in there. I also addedgit submodule update --init --recursivetoinit-tests-after-clone.shbecause there are a decent number of tests that fail withoutgitdbandsmmapcloned.