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.

Sign up for GitHub

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

Jump to bottom

BF: Don't make create-sibling recursive by default #6116

Merged
merged 3 commits into from
Oct 30, 2021
Merged

BF: Don't make create-sibling recursive by default #6116

merged 3 commits into from
Oct 30, 2021

Conversation

adswa
Copy link
Member

@adswa adswa commented Oct 25, 2021

This fixes #5614: I prevents an accidental recursion of one level by not evaluating subdataset states - which should not play any role for the sibling creation of the toplevel ds - only when the recursive flag is set.
It also adds a check for concurrent occurrence of --since with --recursive - Given the description of --since, I believe that it requires recursion and is thus useless if used without --recursive.

@adswa
BF: Prevent default recursion of create-sibling
06c5301 
By default, create-sibling was erroneously recursing into the first
level of subdatasets of a dataset, creating additional, unwanted
siblings.
(see datalad#5614 for a corresponding issue)
This was due to a flawed logic with regard to --since:
--since by definition is only operating on subdatasets, but that
would require a recursive flag to be set. Yet even without a
recursive flag, create-sibling evaluated subdatasets states for
since, and with this, it added the first level of subdatasets
of any given dataset into the list of to-be-considered datasets
for sibling creation.

The change in this commit catches the use of since without a
correspoding recursive flag early on and raises a ValueError.
It also only evaluates state of subdatasets if there is any
need for recursion - if not, it simply just considers the
given dataset.
@adswa
TST: add a minimal testcase for datalad#5614 (unwanted recursive of c…
920a158 
…reate-sibling)
@adswa adswa added the semver-patch Increment the patch version when merged label Oct 25, 2021
@adswa adswa changed the title Don't make create-sibling recursive by default BF: Don't make create-sibling recursive by default Oct 25, 2021
@adswa adswa changed the base branch from master to maint October 25, 2021 16:05
@codecov
Copy link

codecov bot commented Oct 25, 2021 •
edited
Loading

Codecov Report

Merging #6116 (b04d8a1) into maint (ece1cbc) will decrease coverage by 2.25%.
The diff coverage is 94.44%.

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

@@            Coverage Diff             @@
##            maint    #6116      +/-   ##
==========================================
- Coverage   90.23%   87.97%   -2.26%     
==========================================
  Files         312      312              
  Lines       42219    42235      +16     
==========================================
- Hits        38095    37157     -938     
- Misses       4124     5078     +954
Impacted Files Coverage Δ
datalad/core/local/create.py 97.05% <ø> (-0.74%) ⬇️
datalad/core/local/tests/test_create.py 100.00% <ø> (ø)
datalad/distribution/create_sibling.py 66.17% <87.50%> (+0.30%) ⬆️
datalad/distribution/get.py 97.93% <100.00%> (+<0.01%) ⬆️
datalad/distribution/tests/test_create_sibling.py 76.56% <100.00%> (+0.19%) ⬆️
datalad/interface/download_url.py 98.13% <100.00%> (+0.01%) ⬆️
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%) ⬇️
... and 87 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 ece1cbc...87985de. Read the comment docs.

mih
mih reviewed Oct 26, 2021
View reviewed changes
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.

I'd say that the reasoning and approach are correct. But a test isn't happy about it. It may well be that it actually tests undesirable behavior.

@adswa
Copy link
Member Author

adswa commented Oct 26, 2021

I'd say that the reasoning and approach are correct. But a test isn't happy about it. It may well be that it actually tests undesirable behavior.

Ahh, thx, I hadn't enabled those locally. Mhh, I think it is a question of defining what --since should operate on. According to the failing test, only on subdatasets. I've pushed a commit that should ensure this. If --since should allow not only subdatasets, but also the parent to get a sibling, the test needs an adjustment.

@adswa
Copy link
Member Author

adswa commented Oct 26, 2021

The Appveyor failure is unrelated (but it is already the second time that I saw it in currently open PRs):

git clone -q https://github.com/datalad/datalad.git /home/appveyor/projects/datalad
git fetch -q origin +refs/pull/6116/merge:
git checkout -qf FETCH_HEAD
Error making request with Error Code NotFound and Http Status Code NotFound. No further error information was returned by the service.

I can squish the two commits in this PR if my interpretation of what --since should do in here is correct

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Oct 29, 2021
@yarikoptic
Copy link
Member

We accumulated a good number of fixes in maint. Marked this PR as a candidate to trigger a release. I have also restarted incomplete runs on appveyor to hopefully get it into green zone.

mih
mih approved these changes Oct 30, 2021
View reviewed changes
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!

I took the liberty to amend and force-push the last commit (no longer TMP, and adjusts the since docstring to make explicit that this is about changed subdatasets).

Will merge as soon as the tests have finished.

@mih mih merged commit c79b5bd into datalad:maint Oct 30, 2021
@adswa adswa deleted the bf-5614 branch November 1, 2021 07:52
@adswa adswa mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mih mih mih approved these changes

Assignees
No one assigned
Labels
release Create a release when this pr is merged semver-patch Increment the patch version when merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

create-sibling goes recursive without --recursive
3 participants
@adswa @yarikoptic @mih