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: Don't overwrite subdataset source candidates #6168

Merged
merged 2 commits into from Nov 18, 2021
Merged

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Nov 10, 2021

datalad-get used to overwrite possibly existing source candidate configs
in newly cloned subdatasets. While inheriting is generally fine, it
didn't take into account that postclone_cfg routines may already have
had a better idea.

"Better idea" in that I think the principle of looking up subdatasets in the same RIA store their super came from should apply at all levels recursively. Hence, when super came from storeA, storeA is also considered for sub. If it, however, turns out that sub ultimately came from storeB, then subsub should be looked for in storeB. And that's the situation where that overwrite matters AFAICT.

Closes #6159

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #6168 (a58b5c3) into maint (0df06be) will decrease coverage by 2.07%.
The diff coverage is 100.00%.

❗ Current head a58b5c3 differs from pull request most recent head cd694da. Consider uploading reports for the commit cd694da to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6168      +/-   ##
==========================================
- Coverage   90.21%   88.14%   -2.08%     
==========================================
  Files         312      312              
  Lines       42235    42254      +19     
==========================================
- Hits        38104    37245     -859     
- Misses       4131     5009     +878     
Impacted Files Coverage Δ
datalad/distribution/get.py 96.28% <100.00%> (-1.66%) ⬇️
datalad/distribution/tests/test_get.py 100.00% <100.00%> (ø)
datalad/version.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/add_readme.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/check_dates.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/export_archive.py 0.00% <0.00%> (-100.00%) ⬇️
... and 78 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 0df06be...cd694da. Read the comment docs.

datalad-get used to overwrite possibly existing source candidate configs
in newly cloned subdatasets. While inheriting is generally fine, it
didn't take into account that postclone_cfg routines may already have
had a better idea.

Closes datalad#6159
@bpoldrack bpoldrack changed the base branch from master to maint November 10, 2021 13:03
@bpoldrack bpoldrack added the semver-patch Increment the patch version when merged label Nov 10, 2021
@bpoldrack
Copy link
Member Author

FTR: There are failing tests, b/c I based that fix on maint while calling create-sibling-ria with the new new_store_ok option.
So, I guess I will make that two PRs instead, since it's still a bug fix that should go into maint but not reintroduce unadjusted calls when merging back into master.

@mih
Copy link
Member

mih commented Nov 11, 2021

Travis failure is a known fluke, but windows appveyor looks real.

@mih
Copy link
Member

mih commented Nov 17, 2021

What is the status here? The fix it there, but the test issues seem unaddressed? Having this PR just sitting in this limbo state is not a good thing.

@bpoldrack
Copy link
Member Author

@mih
Verdict from call was to skip that test on windows, since RIA+ORA are not working on windows anyway and this is the only scenario deemed worth testing. Pushed accordingly.

Otherwise it remained somewhat unclear to me whether there are objections to the fix itself.

@bpoldrack
Copy link
Member Author

Edited description to point the reasoning behind this fix.

An alternative would be to take the config from super and try to find an available spot with higher cost within whatever sub already has set. Given that so far only clones from RIA store would get into that situation to begin with, that feels like too big a gun to me, though.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Here as in all other places, it would be so important to have a design document, where this could simply be added to the description of the implemented behavior. I hope we will get there...

@mih mih merged commit 0b01622 into datalad:maint Nov 18, 2021
@mih mih deleted the bf-6159 branch November 18, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datalad get fails on subdataset with different ria URL than superdataset
2 participants