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

gitrepo: Call git with '-c diff.ignoreSubmodules=none' #5453

Merged
merged 3 commits into from Mar 3, 2021

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 3, 2021

Prevent a save failure when diff.ignoreSubmodules is configured to a non-default value.

Fixes #3462.

GitRepo's _GIT_COMMON_OPTIONS is used in _call_git(), _call_annex(),
and AnnexRepo._batched, but several places in GitRepo construct a git
command without _GIT_COMMON_OPTIONS.  This doesn't matter at the
moment because it's an empty list, but it will once an upcoming commit
adds an option.

Audit the git calls in GitRepo, adding _GIT_COMMON_OPTIONS to them.
Also move _GIT_COMMON_OPTIONS to a class attribute so that it can be
used in the class method clone().
test_cfg_originorigin() checks that git-annex-init is called by
looking at the log output, but it doesn't account for the fact that
_GIT_COMMON_OPTIONS can come between "git" and "annex".  At the
moment, this passes because _GIT_COMMON_OPTIONS is empty, but that
will no longer be the case with the next commit.
If a user has configured diff.ignoreSubmodules to a value other than
the default "none", get_content_info() and friends will see the
submodule changes (as intended and expected by the code) because the
low-level commands used are not affected by diff.ignoreSubmodules.
However, a save_() call will fail at the git-commit step when the only
modifications are those that would be ignored according to the
diff.ignoreSubmodules setting.

Configure diff.ignoreSubmodules=none for all our git calls because
ignoring submodules isn't something we currently account for and
probably not something we want to support.

Fixes datalad#3462.
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.

LGTM! Thx! The appveyor failure seems to be a fluke, I restarted it.

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #5453 (d596505) into maint (03d0fd3) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5453      +/-   ##
==========================================
- Coverage   89.55%   89.53%   -0.02%     
==========================================
  Files         296      296              
  Lines       41763    41775      +12     
==========================================
+ Hits        37401    37405       +4     
- Misses       4362     4370       +8     
Impacted Files Coverage Δ
datalad/core/distributed/tests/test_clone.py 96.87% <100.00%> (ø)
datalad/core/local/tests/test_save.py 96.08% <100.00%> (+0.07%) ⬆️
datalad/support/gitrepo.py 91.96% <100.00%> (+0.01%) ⬆️
datalad/downloaders/tests/test_http.py 88.59% <0.00%> (-1.91%) ⬇️

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 03d0fd3...d596505. Read the comment docs.

@kyleam
Copy link
Contributor Author

kyleam commented Mar 3, 2021

Thanks for reviewing, @mih

@kyleam kyleam merged commit ad30e70 into datalad:maint Mar 3, 2021
@kyleam kyleam deleted the diff-ignore-submodules branch March 3, 2021 15:09
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