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

Fix recent test_copy_file_prevent_dotgit_placement failure #5258

merged 4 commits into from Dec 17, 2020


Copy link

@kyleam kyleam commented Dec 17, 2020

This last commit in this series fixes the git-version-dependent test_copy_file_prevent_dotgit_placement failure reported in gh-5248. The first three commits are changes related to copy-files .git/ guards.

test_copy_file_prevent_dotgit_placement() has a copy_file() call that
is supposed to return an impossible result because the destination
ends with .git/.  This doesn't work as intended, though.  Due to a
typo, it is actually a list of three items, the last a plain .git
string [*].  The impossible results comes from the "some" source file
not being found.

Fix the typo so that the intended .git/ guard is triggered.

[*] This is treated as a target directory, but the target directory
    code path does not have the same ".git" as explicit destinations.
    The next commit will address that.
copy-file already does some .git safety checks.  One is that the
destination path that comes out of _yield_specs() doesn't end in .git.
But when there is not an explicit destination path, the target
directory is used, bypassing this check.

Abort if any part of the target directory is ".git".  The handling is
a bit different than the destination paths.  A ValueError is raised
instead of yielding impossible results, because this check can be done
up front on a single value.  Also, with one value, it seems worth the
cost to check all the parts rather than just the final name.
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #5258 (6b3bbe0) into maint (ee25838) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5258      +/-   ##
+ Coverage   90.43%   90.44%   +0.01%     
  Files         294      294              
  Lines       40936    40986      +50     
+ Hits        37021    37071      +50     
  Misses       3915     3915              
Impacted Files Coverage Δ
datalad/local/ 96.17% <100.00%> (+0.04%) ⬆️
datalad/local/tests/ 100.00% <100.00%> (ø)
datalad/support/ 90.70% <100.00%> (+0.02%) ⬆️
datalad/support/tests/ 100.00% <100.00%> (ø)
datalad/core/distributed/ 90.02% <0.00%> (+0.08%) ⬆️
datalad/core/distributed/tests/ 96.91% <0.00%> (+0.12%) ⬆️

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 ee25838...6b3bbe0. Read the comment docs.

It appears that copying items _from_ underneath .git/ to a destination
outside of .git/ is intentionally supported, so add two explicit cases
for this.  Also, document a known case where the destination path
checks aren't sufficient to prevent a file from being copied under the
destination .git/.
`git ls-files -o .git/` incorrectly traverses the .git/ directory
before Git 2.25, specifically b9670c1f5e (dir: fix checks on common
prefix directory, 2019-12-19).  To get consistent behavior with older
versions of Git that we support, filter out ls-files results that
start with ".git/".

This change fixes a recent test_copy_file_prevent_dotgit_placement()
failure on master with Git versions before 2.25.  Despite the .git/
checks, there are cases where a file can get copied to a destination
in .git/.  Due to the `ls-files -o` bug above, save() considers
.git/config as a file of interest, leading to `git annex add` being
called with a file in .git/.  The `git annex add` call fails,
presumably due to the ls-files bug also affecting it internal logic:

  {"command":"add","note":"non-large file; adding content to git repository",
  error: invalid path '.git/config'
  error: unable to add '.git/config' to index
  fatal: adding files failed
  git-annex: user error
  (xargs ["-0","git","--git-dir=.git","--work-tree=.","--literal-pathspecs","add","--"]
   exited 123)

The reason we now see these failures on master is that, with the
switch to _call_annex_records() in 46d78a8 (RF: Replace all usage
of AnnexRepo._run_annex_command_json(), 2020-11-18), a CommandError is
raised for the `git annex add` call.  Before _run_annex_command_json()
returned the successful looking record above.

With a Git after 2.25, this failure doesn't happen because `ls-files
-o` doesn't report the file, and so the save() call doesn't result in
the .git/ file being passed to `git annex add`.  Even if it were
passed to `git annex add`, git-annex would ignore it rather than fail.

Fixes datalad#5248.
mih approved these changes Dec 17, 2020
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.

LGTM! Thx!

Copy link
Collaborator Author

kyleam commented Dec 17, 2020

@mih Thanks for the review.

@kyleam kyleam merged commit 5140e58 into datalad:maint Dec 17, 2020
1 check passed
@kyleam kyleam deleted the contentinfo-filter-dotgit branch December 17, 2020 21:30
@yarikoptic yarikoptic added this to the 0.13.7 milestone Jan 3, 2021
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