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: Warn if problematic Git version is detected (maint) #4866

merged 11 commits into from Sep 4, 2020


Copy link

@kyleam kyleam commented Sep 3, 2020

This brings the minimum git version changes from gh-4650 (which is already in master) to maint, replacing the error with a warning. Aside from the merge of gh-4650, the tip commit is the only new commit.

Re: #4650 (comment)

kyleam added 11 commits June 22, 2020 14:26
In tests with Git v2.9 (which is the oldest Git version that will
build for me on Debian Buster), test_AnnexRepo_file_has_content()

    File "[...]/datalad/support/tests/", line 318, in
     test_AnnexRepo_file_has_content [ar.supports_unlocked_pointers])
    AssertionError: [False] != [True]

The failure is coming from the 'git diff' call unexpectedly reporting
a staged type change as modified.  Adding a 'git status' call before
the 'git diff' call is enough to convince Git that there is no
modification, so perhaps it's an issue related to not triggering the
clean filter.

This failure disappears with Git v2.13.
test_duecredit() expect the output to be entirely empty when duecredit
doesn't emit anything.  At least one time when that isn't the case is
when running with an older Git:

    AssertionError: '[...]/datalad/support/
    OutdatedExternalDependencyWarning: Your git version (2.13.0) is
    too old, we will not safe-guard against creating a new repository
    under already known to git subdirectory\n
    OutdatedExternalDependencyWarning\n' != ''
MissingExternalDependency already tacks on a period to its str value,
so unless the caller provides more text via msg, the output has an
extra period: cmd:git of
    version >= 2.19.1 is missing.. You have version 2.13.0

An extra period could also end up in the output when a caller
specifies msg, but none of the callers at the moment end their message
with a period.
'git config' doesn't raise the expected error until 2.13.2,
specifically 25cd291963 (config: complain about --local outside of a
git repo, 2017-05-12).
This test depends on a change that came with v2.13.2: fdb69d33c4
(fetch-pack: always allow fetching of literal SHA1s, 2017-05-15).
We do not specify a minimum supported Git version, which makes it
difficult for us to decide whether we can use a newer feature and
means that users have to rely on trial and error to know whether their
version is recent enough.

Set a minimum Git version, handling it in a way that closely follows
what AnnexRepo already does for the minimum git-annex version.  Choose
2.19.1 (September 2018) as the minimum because that (1) matches what
conda currently has for its 32-bit Linux builds [0] and (2) is below
what is currently in Debian stable (v2.20.1).

Note that if we decide to lower the minimum more, the hard floor
should probably be considered v2.13.0 (May 2017).  As described a few
commits back, AnnexRepo._check_files() doesn't work reliably until
then and testing with v2.13.0 didn't reveal major problems [1], though
the skips added in the two previous commits were needed.

[0] datalad#4636 (comment)

Closes datalad#4636.
With the previous commit, we now specify and verify a minimum Git
version of 2.19.1.
external_versions["cmd:git"] is now stored as GitRepo.git_version.
Use that in a few tests.
The master branch (what will be 0.14) already contains the previous
commits in this series, declaring a minimum git version of 2.19.1 and
aborting if we detect something below it.  As described in ff042b8
(ENH: gitrepo: Declare and check minimum Git version, 2020-06-22),
this minimum version is forward-looking and based off what's available
from particular distributions.  The first Git release with a known
issue is actually much earlier, v2.13.

Merging this series to the maint branch as is then would be too
aggressive for a maintenance release because no prior release has
declared a minimum version, yet we'd be making DataLad non-functional
with a range of Git versions that don't have known issues.  And even
raising an error if Git is below v2.13 could be problematic because
the subset of functionality that a user relies on may not be affected.

Instead issue a warning if the detected Git is below v2.13.0 to let
users that run into problems know that their Git version is likely the

Re: datalad#4650
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #4866 into maint will increase coverage by 0.01%.
The diff coverage is 85.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4866      +/-   ##
+ Coverage   89.68%   89.69%   +0.01%     
  Files         275      288      +13     
  Lines       37103    40359    +3256     
+ Hits        33275    36200    +2925     
- Misses       3828     4159     +331     
Impacted Files Coverage Δ
datalad/ 96.90% <ø> (+0.30%) ⬆️
datalad/core/local/ 98.70% <ø> (ø)
datalad/core/local/ 98.68% <ø> (ø)
datalad/distributed/ 74.67% <ø> (ø) 100.00% <ø> (ø)
datalad/distribution/ 83.87% <ø> (+0.26%) ⬆️
datalad/distribution/ 98.79% <ø> (ø)
datalad/distribution/ 98.87% <ø> (ø)
datalad/distribution/tests/ 95.77% <ø> (+0.18%) ⬆️
datalad/interface/tests/ 100.00% <ø> (ø)
... and 357 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 e216dc3...bc9c9c7. Read the comment docs.

Copy link

@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.


Copy link

so we just need not forget to revert bc9c9c7 whenever this is merged into master.
Thank you @kyleam

@yarikoptic yarikoptic merged commit eac6cfc into datalad:maint Sep 4, 2020
2 checks passed
Copy link
Collaborator Author

kyleam commented Sep 4, 2020

so we just need not forget to revert bc9c9c7 whenever this is merged into master.

Yes, I will of course take care that. (And in fact was taking care of both steps locally when you merged this.)

@kyleam kyleam deleted the min-git-version-warning branch September 4, 2020 15:41
@yarikoptic yarikoptic added this to the 0.13.5 milestone Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants