Skip to content

BF: use get, not casting get_value while dealing with submodule path/url etc #667

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2017
Merged

BF: use get, not casting get_value while dealing with submodule path/url etc #667

merged 1 commit into from
Sep 21, 2017

Conversation

yarikoptic
Copy link
Contributor

hopefully closes #613
did a quick shot, didn't do any tests besides locally:

(git)hopa:~deb/python-git[upstream-master]git
$> s=123; builtin cd /tmp/; rm -rf /tmp/gitsub; mkdir /tmp/gitsub; cd /tmp/gitsub; git init; mkdir $s; cd $s; git init; touch 1; git add 1; git commit -m msg 1; cd ..; git submodule add ./$s $s; git add .gitmodules; git commit -m "added subm $s"                                                                                                                                                                                                               Initialized empty Git repository in /tmp/gitsub/.git/                                        
Initialized empty Git repository in /tmp/gitsub/123/.git/
[master (root-commit) c883ce5] msg
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 1
123/
Adding existing repo at '123' to the index
[master (root-commit) 7c1e5c4] added subm 123
 2 files changed, 4 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 123
(dev) 2 10683.....................................:Thu 07 Sep 2017 11:21:59 PM EDT:.
(git)hopa:/tmp/gitsub[master]git
$> python -c 'import git; r=git.Repo("."); print(r.submodules)'
[git.Submodule(name=123, path=123, url=./123, branch_path=refs/heads/master)]
(dev) 2 10684.....................................:Thu 07 Sep 2017 11:22:00 PM EDT:.
(git)hopa:/tmp/gitsub[master]git
$> s=false; builtin cd /tmp/; rm -rf /tmp/gitsub; mkdir /tmp/gitsub; cd /tmp/gitsub; git init; mkdir $s; cd $s; git init; touch 1; git add 1; git commit -m msg 1; cd ..; git submodule add ./$s $s; git add .gitmodules; git commit -m "added subm $s"                                                                                                                                                                                                             
Initialized empty Git repository in /tmp/gitsub/.git/
Initialized empty Git repository in /tmp/gitsub/false/.git/
[master (root-commit) 02f47b7] msg
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 1
false/
Adding existing repo at 'false' to the index
[master (root-commit) a5547f5] added subm false
 2 files changed, 4 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 false
(dev) 2 10685.....................................:Thu 07 Sep 2017 11:22:06 PM EDT:.
(git)hopa:/tmp/gitsub[master]git
$> python -c 'import git; r=git.Repo("."); print(r.submodules)'
[git.Submodule(name=false, path=false, url=./false, branch_path=refs/heads/master)]

@yarikoptic
Copy link
Contributor Author

ok -- ready to be merged! as #668 experiment showed -- last two runs of appveyor seems to be present also in current master, so have nothing to do with this PR.

yarikoptic
yarikoptic commented Sep 8, 2017
View reviewed changes
git/objects/submodule/base.py
b = cls.k_head_default
if parser.has_option(sms, cls.k_head_option):
b = str(parser.get_value(sms, cls.k_head_option))
b = str(parser.get(sms, cls.k_head_option))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could even strip away us of str here... note that get is still advisable instead of get_value since it could cast+str 1.0 to 1 and then false to False.

@yarikoptic
Copy link
Contributor Author

FWIW our battery of tests pass now, which puts more confidence into this fix, since we do use submodules extensively. quick merge/release would be appreciated

meanwhile, if there would be no activity, I will absorb the patch to at least (Neuro)Debian packages

@yarikoptic
Copy link
Contributor Author

knock knock @Byron -- this PR seems to be quite an easy fix to digest, merge and it would be very appreciated... some folks might have had submodules like 20180911/ and now be out of luck ;)

@codecov-io
Copy link

Codecov Report

Merging #667 into master will increase coverage by 1.8%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #667     +/-   ##
=========================================
+ Coverage   92.57%   94.38%   +1.8%     
=========================================
  Files          61       61             
  Lines        9968     9969      +1     
=========================================
+ Hits         9228     9409    +181     
+ Misses        740      560    -180
Impacted Files Coverage Δ
git/objects/submodule/base.py 94.54% <100%> (+2%) ⬆️
git/test/test_remote.py 97.82% <0%> (ø) ⬆️
git/refs/remote.py 91.11% <0%> (ø) ⬆️
git/compat.py 63.19% <0%> (+0.22%) ⬆️
git/refs/symbolic.py 96.5% <0%> (+0.31%) ⬆️
git/config.py 92.76% <0%> (+0.32%) ⬆️
git/test/test_submodule.py 99.22% <0%> (+0.38%) ⬆️
git/test/test_docs.py 100% <0%> (+0.39%) ⬆️
git/cmd.py 85.54% <0%> (+0.47%) ⬆️
git/repo/base.py 95.82% <0%> (+0.69%) ⬆️
... and 13 more

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 cf8dc25...52912b5. Read the comment docs.

@yarikoptic yarikoptic merged commit fad63e8 into gitpython-developers:master Sep 21, 2017
riley-martine pushed a commit to riley-martine/GitPython that referenced this pull request Dec 7, 2023
@yarikoptic
Merge pull request gitpython-developers#667 from yarikoptic/bf-useget
cd28a87 
BF: use get, not casting get_value while dealing with submodule path/url etc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.submodules leads to IndexError: list index out of range on info = self._cache[item]
2 participants
@yarikoptic @codecov-io