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: clone: Account for relative paths when inheriting local origins #4045

Merged
merged 2 commits into from Jan 17, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Jan 17, 2020

As of 367454e (NF: Auto-configure local origin of local origin to
make annex available, 2019-12-29), we check whether origin's URL for a
newly cloned annex dataset points to a local dataset. If it does, we
add that dataset's origin (if any) to the newly cloned dataset. And
as of d2a25d0 (ENH: Make configuration of local origin siblings work
recursively, 2020-01-05), we continue walking up the chain of local
origin's until we hit an origin that is not a local path.

However, we create the corresponding dataset instance with the wrong
path when origin is a relative path: it should be relative to the
cloned repository, not the current working directory. The incorrect
path means that there's no "origin of origin" to find, and an
"origin-N" remote isn't added.

Note that this bug isn't currently very accessible because git clone
converts relative paths to absolute ones, and the first attempt to
adjust these back to relative paths (02e2b4c and 0a80bb4) was
reverted in e8c5d70 (BF: clone: Revert incorrect relative path
adjustment to URLs, 2020-01-14). But relative paths will be a common
case when the second attempt from gh-4026 lands.

kyleam added 2 commits Jan 17, 2020
The next commit will add a level 3 clone, and "clone_clone_clone" is a
bit unwieldy.  Remap all the clone_* variables to a more manageable
"clone_levN", where N should match the last "origin-N" remote.
As of 367454e (NF: Auto-configure local origin of local origin to
make annex available, 2019-12-29), we check whether origin's URL for a
newly cloned annex dataset points to a local dataset.  If it does, we
add _that_ dataset's origin (if any) to the newly cloned dataset.  And
as of d2a25d0 (ENH: Make configuration of local origin siblings work
recursively, 2020-01-05), we continue walking up the chain of local
origin's until we hit an origin that is not a local path.

However, we create the corresponding dataset instance with the wrong
path when origin is a _relative_ path: it should be relative to the
cloned repository, not the current working directory.  The incorrect
path means that there's no "origin of origin" to find, and an
"origin-N" remote isn't added.

Note that this bug isn't currently very accessible because `git clone`
converts relative paths to absolute ones, and the first attempt to
adjust these back to relative paths (02e2b4c and 0a80bb4) was
reverted in e8c5d70 (BF: clone: Revert incorrect relative path
adjustment to URLs, 2020-01-14).  But relative paths will be a common
case when the second attempt from dataladgh-4026 lands.
@kyleam
Copy link
Contributor Author

@kyleam kyleam commented Jan 17, 2020

Note that this bug isn't currently very accessible because git clone
converts relative paths to absolute ones, and the first attempt to
adjust these back to relative paths (02e2b4c and 0a80bb4) was
reverted in e8c5d70 (BF: clone: Revert incorrect relative path
adjustment to URLs, 2020-01-14). But relative paths will be a common
case when the second attempt from gh-4026 lands.

I've confirmed locally that, without the fix, the new test triggers the expected failure on top of gh-4026.

@codecov
Copy link

@codecov codecov bot commented Jan 17, 2020

Codecov Report

Merging #4045 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4045      +/-   ##
==========================================
- Coverage   89.78%   89.76%   -0.02%     
==========================================
  Files         272      272              
  Lines       36536    36540       +4     
==========================================
- Hits        32804    32801       -3     
- Misses       3732     3739       +7
Impacted Files Coverage Δ
datalad/core/distributed/tests/test_clone.py 90.6% <100%> (+0.12%) ⬆️
datalad/core/distributed/clone.py 92.79% <100%> (ø) ⬆️
datalad/downloaders/tests/test_http.py 60.58% <0%> (-1.22%) ⬇️
datalad/downloaders/http.py 74.9% <0%> (-0.4%) ⬇️
datalad/downloaders/base.py 75.18% <0%> (-0.38%) ⬇️

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 47980eb...9033cd8. Read the comment docs.

mih
mih approved these changes Jan 17, 2020
Copy link
Member

@mih mih left a comment

Thanks for catching that!

@kyleam kyleam merged commit 9033cd8 into datalad:master Jan 17, 2020
17 checks passed
@kyleam kyleam deleted the configure-origins-fix-dspath branch Jan 17, 2020
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