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

More flexible clone URL generation for submodules #3828

Merged
merged 14 commits into from Oct 28, 2019

Conversation

mih
Copy link
Member

@mih mih commented Oct 23, 2019

This fixes gh-3827, while also tweaking a little bit on the sides -- this is rather old code.

This is technically ready. Once agreed on, there will be a use case description in the handbook on feature.

mih added 6 commits Oct 23, 2019
For which our wrapper had to be extended. One could argue that
`.call_git_items_()` would be at least as adequate, but I chose to
invest in my prior investments ;-)
The desire is to slim the interface.
No StopIteration could have come out of that code block
@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #3828 into master will increase coverage by 0.02%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3828      +/-   ##
==========================================
+ Coverage   80.71%   80.73%   +0.02%     
==========================================
  Files         273      273              
  Lines       35905    35925      +20     
==========================================
+ Hits        28979    29004      +25     
+ Misses       6926     6921       -5
Impacted Files Coverage Δ
datalad/distribution/utils.py 89.74% <ø> (ø) ⬆️
datalad/metadata/aggregate.py 14.24% <0%> (ø) ⬆️
datalad/metadata/search.py 35.39% <0%> (ø) ⬆️
datalad/distribution/tests/test_get.py 100% <100%> (ø) ⬆️
datalad/distribution/get.py 81.73% <100%> (+1.73%) ⬆️
datalad/support/tests/test_gitrepo.py 99.88% <100%> (ø) ⬆️
datalad/support/gitrepo.py 83.5% <71.42%> (-0.11%) ⬇️
datalad/distribution/siblings.py 54.46% <0%> (+0.3%) ⬆️
datalad/log.py 65.86% <0%> (+0.96%) ⬆️
... and 1 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 1592db4...ad04e53. Read the comment docs.

mih added 2 commits Oct 24, 2019
This touches upon an idea by @yaricoptic expressed in
datalad#3827 (comment)
and merely lays the foundation for further work.

Names are not unique (and I don't see how they could be, or what we
would gain from it), hence no dict.

Choice of names is debatable too. But given that they are not used ATM,
it doesn't really matter and can easily be changed.
@mih mih marked this pull request as ready for review Oct 24, 2019
@@ -1308,6 +1309,9 @@ def for_each_ref_(self, fields=('objectname', 'objecttype', 'refname'),
descending order.
count : int, optional
Stop iteration after the given number of matches.
contains : str, optional
Only list refs which contain the specified commit (HEAD if not
specified).
Copy link
Contributor

@kyleam kyleam Oct 25, 2019

Choose a reason for hiding this comment

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

If not specified, it's None, and --contains isn't added. That's not the same as restricting the results to --contains=HEAD.

Copy link
Member Author

@mih mih Oct 28, 2019

Choose a reason for hiding this comment

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

Fixed, by removing that part from the docs (copied from Git).

return [
remote
for remote in repo.get_remotes(with_urls_only=with_urls_only)
if any(rb.startswith(remote + '/') for rb in remote_branches)
Copy link
Contributor

@kyleam kyleam Oct 25, 2019

Choose a reason for hiding this comment

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

I'm not sure I see the point of using get_remote(with_urls_only=...) here, especially given this is only used in one spot. Wouldn't for_each_ref_s output (with an additional layer stripped) be sufficient?

Copy link
Member Author

@mih mih Oct 28, 2019

Choose a reason for hiding this comment

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

This is just keeping what the keep did before. I tried to get rid of it, but couldn't figure out why things would break. I'll leave it for another time.

Copy link
Contributor

@kyleam kyleam Oct 28, 2019

Choose a reason for hiding this comment

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

This is just keeping what the keep did before.

Yes...

I tried to get rid of it, but couldn't figure out why things would break.

ah, all right, so my guess that it isn't need is wrong. Thanks.

datalad/support/gitrepo.py Show resolved Hide resolved
except CommandError as e:
if 'does not have any commits' in e.stderr:
if 'unknown revision or path not in the working tree' in e.stderr:
Copy link
Contributor

@kyleam kyleam Oct 25, 2019

Choose a reason for hiding this comment

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

From my POV it'd be best to avoid brittle error message matching if we can. Given we're in the error handling arm, another git call seems acceptable, so this could be replaced by if self.get_hexsha() is None. (get_hexsha() relies on format_commit which itself does error message matching, but at least we'd avoid introducing another spot with a new message.)

Copy link
Member Author

@mih mih Oct 28, 2019

Choose a reason for hiding this comment

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

Done.

# outside. Additional guesswork can only make it slower
clone_urls.append((name, url))

# CANDIDATE: the actual configured gitmodule URL
Copy link
Contributor

@kyleam kyleam Oct 25, 2019

Choose a reason for hiding this comment

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

I find this new wording more confusing than what it replaced. In Git's eyes, relative paths have the following resolution: (1) relative to the URL of the tracking branch, (2) if there is not a tracking branch, relative to the URL of "origin", or (3) if there is not a configured remote, relative to the working directory. So the new description is only accurately describing what Git considers to be the configured URL in cases that fall through to no. 3.

(As a sidenote that's not directly related to these changes, it looks like _get_flexible_source_candidates_for_submodule doesn't handle case no. 2, so there's a scenario where git submodule can clone something while datalad install can't.)

ds.path,
alternate_suffix=False)
clone_urls.extend(
('origin', url)
Copy link
Contributor

@kyleam kyleam Oct 25, 2019

Choose a reason for hiding this comment

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

It sounds like these names aren't used for anything at this point, so it doesn't really matter, but perhaps "local" would be a better description?

Copy link
Member Author

@mih mih Oct 28, 2019

Choose a reason for hiding this comment

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

Funny enough local is what I had. Now it is local again ;-)

kyleam
kyleam approved these changes Oct 28, 2019
Copy link
Contributor

@kyleam kyleam left a comment

@mih Thanks for the updates. Looks good to me (I know you haven't responded to the # CANDIDATE: bit yet, but it's fine with me if that remains unchanged).

@mih
Copy link
Member Author

mih commented Oct 28, 2019

@kyleam Love'ya!

@mih mih merged commit 236204c into datalad:master Oct 28, 2019
16 checks passed
@mih mih deleted the enh-submoduleclone branch Oct 28, 2019
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.

2 participants