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.

Copy link

Member

Byron commentedDec 24, 2018

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.

Byron added the waiting for feedback label

Dec 24, 2018

Byron self-requested a review

December 24, 2018 08:11

Copy link

ContributorAuthor

stsewd commentedDec 24, 2018

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.

Copy link

ContributorAuthor

stsewd commentedDec 24, 2018

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.

I'm not following, sorry. Do you mean to keep the exception and passing the failed submodule as an attribute?

Byron requested changes

Dec 31, 2018

View reviewed changes

Copy link

Member

Byron left a comment

Choose a reason for hiding this comment

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

I left a few comments were they matter, maybe this helps to understand what I mean. If anything remains unclear, please let me know.

git/objects/submodule/base.py

		entry=index.entries[index.entry_key(p,0)]
		sm=Submodule(repo,entry.binsha,entry.mode,entry.path)
		exceptKeyError:
		raiseInvalidGitRepositoryError(

Copy link

Member

ByronDec 31, 2018

Choose a reason for hiding this comment

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

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).

Copy link

ContributorAuthor

stsewdJan 6, 2019

Choose a reason for hiding this comment

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

If we do that, we wouldn't be able to keep listing the submodules here.

Copy link

Member

ByronJan 12, 2019

Choose a reason for hiding this comment

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

@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.

git/test/test_submodule.pyShow resolvedHide resolved

Copy link

ContributorAuthor

stsewd commentedJan 6, 2019

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 in8b3ffcd

	# update fails as list_items in such a situations cannot work, as it cannot
	# find the entry at the changed path
	self.failUnlessRaises(InvalidGitRepositoryError,rm.update,recursive=False)

	# move it properly - doesn't work as it its path currently points to an indexentry
	# which doesn't exist ( move it to some path, it doesn't matter here )
	self.failUnlessRaises(InvalidGitRepositoryError,sm.move,pp)
	# reset the path(cache) to where it was, now it works

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.

Byron approved these changes

Jan 12, 2019

View reviewed changes

Copy link

codecov-io commentedFeb 2, 2019•
edited
Loading

Codecov Report

Merging#818 intomaster willincrease coverage by<.01%.
The diff coverage is100%.

@@            Coverage Diff             @@##           master     #818      +/-   ##==========================================+ Coverage   94.67%   94.68%   +<.01%==========================================  Files          59       59                Lines        9393     9399       +6     ==========================================+ Hits         8893     8899       +6  Misses        500      500

Impacted Files	Coverage Δ
git/objects/submodule/base.py	`94.35% <100%> (ø)`	⬆️
git/test/test_submodule.py	`99.25% <100%> (+0.01%)`	⬆️
git/cmd.py	`83.83% <0%> (-0.14%)`	⬇️
git/test/test_remote.py	`97.8% <0%> (-0.02%)`	⬇️
git/repo/base.py	`95.24% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update6971a93...6a235bc. Read thecomment docs.

Copy link

ContributorAuthor

stsewd commentedFeb 2, 2019

I just updated the test, sorry for the long wait. Now py37 is failing, but doesn't look related to this change.

stsewd mentioned this pull request

Feb 6, 2019

Build error with build id #8504705readthedocs/readthedocs.org#5223

Closed

stsewd mentioned this pull request

Feb 25, 2019

Build error with build id #8647215 - no errors in logreadthedocs/readthedocs.org#5349

Closed

stsewd mentioned this pull request

Mar 6, 2019

gitpython sometimes fails when checking for submodulesreadthedocs/readthedocs.org#4371

Closed

stsewd added3 commits

June 25, 2019 10:10

Add test

0e577f5

Skip on keyerror

4bed109

Fix test

6a235bc

stsewd force-pushed thefix-submodules-listing branch from31d99b5 to6a235bcCompare

June 25, 2019 15:10

Copy link

ContributorAuthor

stsewd commentedJun 25, 2019•
edited
Loading

@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

stsewd added a commit to stsewd/readthedocs.org that referenced this pull request

Jul 10, 2019

Better msg when gitpython fails

538e291

Currently we show a generic message.Related:readthedocs#4371This can be removed whengitpython-developers/GitPython#818gets merged.

stsewd mentioned this pull request

Jul 10, 2019

Better msg when gitpython failsreadthedocs/readthedocs.org#5903

Merged

Byron merged commite93ffe1 intogitpython-developers:master

Jul 20, 2019

Copy link

Member

Byron commentedJul 20, 2019

Sorry for the wait, and thanks a lot for the fixes!

stsewd deleted the fix-submodules-listing branch

July 20, 2019 16:33

AlanCoding mentioned this pull request

Mar 6, 2020

AWX 9.2 python 3.6 erroransible/awx#6175

Closed

Labels

waiting for feedback

Movatterモバイル変換

Uh oh!

Fix submodules listing error#818

Fix submodules listing error#818

Uh oh!

Conversation

stsewd commentedDec 24, 2018

Uh oh!

Byron commentedDec 24, 2018

Uh oh!

stsewd commentedDec 24, 2018

Uh oh!

stsewd commentedDec 24, 2018

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

ByronDec 31, 2018

Choose a reason for hiding this comment

Uh oh!

stsewdJan 6, 2019

Choose a reason for hiding this comment

Uh oh!

ByronJan 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stsewd commentedJan 6, 2019

Uh oh!

codecov-io commentedFeb 2, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stsewd commentedFeb 2, 2019

Uh oh!

stsewd commentedJun 25, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Byron commentedJul 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov-io commentedFeb 2, 2019•
edited
Loading

stsewd commentedJun 25, 2019•
edited
Loading