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

BF: More precise test for valid repo paths (fixes gh-3473) #3475

Merged
merged 2 commits into from Jun 12, 2019

Conversation

@mih
Copy link
Member

@mih mih commented Jun 9, 2019

Additionally check that a .git directory is not empty

@codecov
Copy link

@codecov codecov bot commented Jun 9, 2019

Codecov Report

Merging #3475 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3475      +/-   ##
==========================================
- Coverage   91.26%   91.22%   -0.04%     
==========================================
  Files         272      269       -3     
  Lines       34903    34898       -5     
==========================================
- Hits        31853    31837      -16     
- Misses       3050     3061      +11
Impacted Files Coverage Δ
datalad/support/gitrepo.py 90.2% <100%> (ø) ⬆️
datalad/downloaders/http.py 83.73% <0%> (-2.78%) ⬇️
datalad/support/repodates.py 96.66% <0%> (-1.12%) ⬇️
datalad/cmd.py 95.29% <0%> (-0.84%) ⬇️
tools/coverage-bin/git-annex-remote-datalad
tools/coverage-bin/datalad
...ols/coverage-bin/git-annex-remote-datalad-archives

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 3fd8b2a...9a6672a. Read the comment docs.

return (Path(path) / '.git').exists()
path = Path(path) / '.git'
# the aim here is to have this test as cheap as possible, because
# it is performed a lot
Copy link
Contributor

@kyleam kyleam Jun 9, 2019

OK, this is why we avoid using a git rev-parse-based solution.

# file or symlink
return path.exists() and (
(path.is_dir() and any(path.iterdir())) \
or not path.is_dir()
Copy link
Contributor

@kyleam kyleam Jun 9, 2019

Very unlikely to be noticeable in practice, but since the comment says the aim is "as cheap as possible", you could avoid a is_dir() call:

diff --git a/datalad/support/gitrepo.py b/datalad/support/gitrepo.py
index c6658d010..8dbadd041 100644
--- a/datalad/support/gitrepo.py
+++ b/datalad/support/gitrepo.py
@@ -995,9 +995,8 @@ def is_valid_repo(cls, path):
         # repo: 1) a non-empty .git directory (#3473) and 2) a pointer
         # file or symlink
         return path.exists() and (
-            (path.is_dir() and any(path.iterdir())) \
-            or not path.is_dir()
-        )
+            not path.is_dir() or
+            any(path.iterdir()))
 
     @staticmethod
     def get_git_dir(repo):

Copy link
Member Author

@mih mih Jun 12, 2019

That is better. Thx!

Copy link
Member Author

@mih mih Jun 12, 2019

Done.

Additionally check that a .git directory is not empty
This would fail before the previous commit.
@mih
Copy link
Member Author

@mih mih commented Jun 12, 2019

You are an angel!

@adswa
Copy link
Member

@adswa adswa commented Jun 12, 2019

(Seconding mih on that)

@kyleam
Copy link
Contributor

@kyleam kyleam commented Jun 12, 2019

The 3.7-dev run is failing with a segfault. (I repeated the run once.)

https://travis-ci.org/datalad/datalad/jobs/544787756#L1472

That happens outside the added test, and I think it's safe to assume that it's unrelated.

@kyleam kyleam merged commit 3e79639 into datalad:master Jun 12, 2019
2 of 3 checks passed
kyleam added a commit that referenced this issue Jun 12, 2019
@mih mih deleted the bf-3473 branch Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants