Skip to content
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

BF: Depend on Gitpython >= 2.1.12 #4070

Merged
merged 1 commit into from Jan 23, 2020
Merged

Conversation

bpoldrack
Copy link
Member

No description provided.

Copy link
Collaborator

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

This is fine with me, though it would be nice to see the commit message lay out the thought process a bit.

My first thought seeing this was "wait, GitPython's clone didn't accept options until that version? Weird." But it's actually just support for options that can be given multiple times. Given that we started passing in multi_options to support --branch, we could get by without multi_options:

diff --git a/datalad/support/gitrepo.py b/datalad/support/gitrepo.py
index fabe1760d..458e4222a 100644
--- a/datalad/support/gitrepo.py
+++ b/datalad/support/gitrepo.py
@@ -889,9 +889,9 @@ def clone(cls, url, path, *args, clone_options=None, **kwargs):
                         # tailored list of "multi options" to make a future
                         # non-GitPy based implementation easier. Do conversion
                         # here
-                        multi_options=to_options(**clone_options) if clone_options else None,
                         odbt=default_git_odbt,
-                        progress=git_progress
+                        progress=git_progress,
+                        **clone_options
                     )
                 # Note/TODO: signature for clone from:
                 # (url, to_path, progress=None, env=None, **kwargs)

But making that change probably doesn't make sense if we want adjust datalad clone to take arbitrary options (gh-4035), in which case we'll need to bump the gitpython version for multi_options anyway.

@bpoldrack
Copy link
Member Author

Actually it's about a bug in the function that converts python-style parameters to command line options. In older version it would convert some=None to --some='None' rather than just not passing it on (which would be achieved by some=False instead. This changed and is cleaner on our side the "new" way.

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #4070 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4070      +/-   ##
==========================================
- Coverage   89.65%   89.63%   -0.02%     
==========================================
  Files         274      274              
  Lines       36667    36667              
==========================================
- Hits        32872    32866       -6     
- Misses       3795     3801       +6
Impacted Files Coverage Δ
datalad/support/tests/test_cookies.py 85.71% <0%> (-14.29%) ⬇️
datalad/support/tests/test_annexrepo.py 95.44% <0%> (-0.31%) ⬇️

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 a1a2f9b...70296bc. Read the comment docs.

This is about a change in the function that converts python-style parameters to command line options (transform_kwargs as imported by our gitrepo.py:to_options() ). In older gitpy version it would convert some=None to --some='None' rather than just not passing it on (which would be achieved by some=False instead. This changed and is cleaner on our side the "new" way. We use that approach in several places to pass command options through to git calls. Just failed to notice since our CI builds will install more recent python packages anyway.
@bpoldrack
Copy link
Member Author

@kyleam : Changed commit message to include my comment above. Will cancel new CI runs, since nothing changed besides that commit message.

@bpoldrack bpoldrack merged commit 5cb174b into datalad:master Jan 23, 2020
@bpoldrack bpoldrack deleted the bf-gitpy-dep branch September 2, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants