Fix submodules listing error #818
Conversation
|
Thanks a lot for the contribution and the test. I will be happy to merge after some clarification. Could you have a look at the test that actually wants the ‘InvavalidGitRepository’ exception? It causes travis to fail consistently. Maybe that usecase is now obsolete by the new behaviour, which is when the test can be adjusted to claim that ‘nothing should happen’ if the submodule is invalid. Maybe that test, however, could also show that the ‘InvalidGitRepository’ error should rather be cought in the spot that interests you in particular, for example, when listing submodules, instead of removing it entirely. |
|
Sorry I didn't comment yesterday, I was trying to solve that failing test but some tests are not passing locally (master), need to figure out that first. |
I'm not following, sorry. Do you mean to keep the exception and passing the failed submodule as an attribute? |
| @@ -1184,8 +1184,9 @@ def iter_items(cls, repo, parent_commit='HEAD'): | |||
| entry = index.entries[index.entry_key(p, 0)] | |||
| sm = Submodule(repo, entry.binsha, entry.mode, entry.path) | |||
| except KeyError: | |||
| raise InvalidGitRepositoryError( | |||
There was a problem hiding this comment.
Is there a way for this Exception to stay, just as there is a test that expects this behaviour (see the test-failure on travis).
There was a problem hiding this comment.
If we do that, we wouldn't be able to keep listing the submodules here.
There was a problem hiding this comment.
@stsewd I think all that's left before the merge is to fix the now failing test. It could be changed to now expect to no failure and to see a listing of the existing submodules - if you think there is not too much overlap with the test you added.
|
Also, reading the test looks like the code is doing what we want to avoid GitPython/git/test/test_submodule.py Lines 482 to 489 in 8b3ffcd The tests are expecting to raise an exception if it can't find the correct path of the submodule, while git just list the valid ones. |
Codecov Report
@@ Coverage Diff @@
## master #818 +/- ##
==========================================
+ Coverage 94.67% 94.68% +<.01%
==========================================
Files 59 59
Lines 9393 9399 +6
==========================================
+ Hits 8893 8899 +6
Misses 500 500
Continue to review full report at Codecov.
|
|
I just updated the test, sorry for the long wait. Now py37 is failing, but doesn't look related to this change. |
31d99b5 to
6a235bc
Compare
|
@Byron tests are passing now, any change to get this merged? Thank you! Appveyor is failing in master (isn't triggering for this PR I think), though |
Currently we show a generic message. Related: readthedocs#4371 This can be removed when gitpython-developers/GitPython#818 gets merged.
|
Sorry for the wait, and thanks a lot for the fixes! |
Fix #279
When the path doesn't exist, gitpython throws an error, this usually happens when the user deletes the submodule directory but not the submodule entry in
.gitmodules.