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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure a subdataset is saved with a complete .gitmodules record #6790

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jul 2, 2022

Previously, the test for adding a gitmodules record was incomplete
and relied on the assumption that for a new subdataset the type
dataset was not determined by the initial status() inspection.
However, this is not the case, when an explicit subdataset path
with a trailing path-separator is given to save().

This change amends the test condition to also test for the state
being untracked. In principle this alone should be a sufficient
condition. However, due to the complexity of the decision making
in the present implementation, the previous condition as also
kept as an alternative, in order to prevent unexpected side-effect.

Fixes #6547

Changelog

馃悰 Bug Fixes

@mih mih added the semver-patch Increment the patch version when merged label Jul 2, 2022
@mih
Copy link
Member Author

mih commented Jul 2, 2022

Test failure is real, investigating...Fixed.

mih added 2 commits July 2, 2022 20:20
Previously, the test for adding a gitmodules record was incomplete
and relied on the assumption that for a new subdataset the type
`dataset` was not determined by the initial `status()` inspection.
However, this is not the case, when an explicit subdataset path
with a trailing path-separator is given to `save()`.

This change amends the test condition to also test for the state
being `untracked`. In principle this alone should be a sufficient
condition. However, due to the complexity of the decision making
in the present implementation, the previous condition as also
kept as an alternative, in order to prevent unexpected side-effect.

Fixes datalad#6547
Previously, `save` treated them all as 'untracked' leading to
unconditional and unnecessary reevaluation of their state.
@codeclimate
Copy link

codeclimate bot commented Jul 2, 2022

Code Climate has analyzed commit 2c1d4ab and detected 0 issues on this pull request.

View more on Code Climate.

@mih
Copy link
Member Author

mih commented Jul 3, 2022

The changes in this PR are now a precondition to an upcoming PR addressing #6791 -- depending on the timing of review, I will close this PR and integrate it into a substantially larger update that introduces typechange detection.

state='untracked',
type='dataset')
dict(state='untracked', type='dataset')
)
Copy link
Member

Choose a reason for hiding this comment

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

smells like we would benefit from more type annotations and adding type checking

@yarikoptic
Copy link
Member

seems to fix a bug and has a test and all green -- what could stop us? ;) I will CP to maint within yet another PR

@yarikoptic yarikoptic merged commit 935f4b1 into datalad:master Jul 5, 2022
@yarikoptic
Copy link
Member

FWIW -- decided not to bother with a PR, CPed, ran the test, pushed with tip to 6ec85d4

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

馃殌 PR was released in 0.16.7 馃殌

@bpoldrack bpoldrack mentioned this pull request Jul 7, 2022
40 tasks
@mih mih deleted the bf-6547 branch July 19, 2022 11:35
mih added a commit to datalad/datalad-next that referenced this pull request Jul 24, 2022
mih added a commit to datalad/datalad-next that referenced this pull request Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-maint released semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save -d . submodule - doesn't add to .gitmodules
2 participants