-
-
Notifications
You must be signed in to change notification settings - Fork 953
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
Conversation
...re recursively cloned as well
codecov-io
commented
Oct 21, 2019
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.
|
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.