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

addurls: Don't trip over "./" in the file name format #4504

Merged
merged 2 commits into from May 14, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented May 8, 2020

As described in d468467 (DOC: addurls: Clarify what filename format
is relative to, 2019-08-02) and the tests, the file format is intended
to be taken relative to the dataset. However, when the file name
format begins with "./", we keep "./" on the relative name, while
constructing the absolute file name without the subdirectory as
documented. The relative file name is passed to AnnexRepo.addurl(),
causing the file to be treated as relative to the current working
directory and downloaded there.

Achieve the documented behavior by applying the same handling to the
top-level dataset as we use for subdatasets.

Note that there is mention of changing this relative path handling in
gh-4498. Even if we decide that this change in behavior is
worthwhile, it still makes sense to fix this bug, and doing so doesn't
make the future change harder.

kyleam added 2 commits May 8, 2020
test_addurls_unbound_dataset() looks at relative path handling, and
one of its checks asserts that the expected files exist.  But the same
dataset is used across two calls to addurls(), which means the check
for the second call could pass due to the previous call.

Use a separate dataset for each call.
As described in d468467 (DOC: addurls: Clarify what filename format
is relative to, 2019-08-02) and the tests, the file format is intended
to be taken relative to the dataset.  However, when the file name
format begins with "./", we keep "./" on the relative name, while
constructing the absolute file name without the subdirectory as
documented.  The relative file name is passed to AnnexRepo.addurl(),
causing the file to be treated as relative to the current working
directory and downloaded there.

Achieve the documented behavior by applying the same handling to the
top-level dataset as we use for subdatasets.

Note that there is mention of changing this relative path handling in
dataladgh-4498.  Even if we decide that this change in behavior is
worthwhile, it still makes sense to fix this bug, and doing so doesn't
make the future change harder.
@codecov
Copy link

@codecov codecov bot commented May 8, 2020

Codecov Report

Merging #4504 into maint will increase coverage by 47.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #4504       +/-   ##
===========================================
+ Coverage   42.41%   89.68%   +47.26%     
===========================================
  Files         275      275               
  Lines       37103    37105        +2     
===========================================
+ Hits        15738    33277    +17539     
+ Misses      21365     3828    -17537     
Impacted Files Coverage Δ
datalad/plugin/addurls.py 99.15% <100.00%> (+81.73%) ⬆️
datalad/plugin/tests/test_addurls.py 100.00% <100.00%> (+100.00%) ⬆️
datalad/core/local/create.py 94.77% <0.00%> (+0.74%) ⬆️
datalad/core/local/tests/test_create.py 100.00% <0.00%> (+0.90%) ⬆️
datalad/ui/progressbars.py 83.10% <0.00%> (+1.35%) ⬆️
datalad/support/external_versions.py 95.62% <0.00%> (+1.45%) ⬆️
datalad/dochelpers.py 87.40% <0.00%> (+1.48%) ⬆️
datalad/support/tests/test_network.py 100.00% <0.00%> (+1.83%) ⬆️
datalad/core/local/status.py 98.03% <0.00%> (+1.96%) ⬆️
datalad/support/tests/test_external_versions.py 91.94% <0.00%> (+2.01%) ⬆️
... and 187 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 5e10bbb...0dd7cb8. Read the comment docs.

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 14, 2020

thanks, looks good to me. I restarted github actions for completeness but they still trip with

##[error]fatal: reference is not a tree: 0dd7cb8d3ec8d8e8bea2f3706b6e815166640c67

on checkout. I expect them to not carry any related addurls usage, so let's just proceed.

@yarikoptic yarikoptic merged commit debd468 into datalad:maint May 14, 2020
6 of 10 checks passed
@yarikoptic yarikoptic added this to the 0.12.7 milestone May 14, 2020
@kyleam kyleam deleted the addurls-dotslash-confusion branch May 14, 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