Reimplement _get_dot_git() to be more efficient#6325
Conversation
It now tests the common scenarios first, rather then leaving them for last. Moreover, it introduces an explicit `resolved` flag for result path reporting, and applies it consistently when enabled -- rather than just for symlinked directories, but not files, or gitdirs specified in .git files. Searching through the code, I did not spot a case where this features is needed, nor desirable. Fixes datalad#6321 Fixes datalad#6324
mih
added
semver-minor
Codecov Report
@@ Coverage Diff @@
## master #6325 +/- ##
===========================================
- Coverage 89.87% 61.74% -28.14%
===========================================
Files 331 331
Lines 43210 43189 -21
===========================================
- Hits 38835 26666 -12169
- Misses 4375 16523 +12148
Continue to review full report at Codecov.
|
|
FWIW, IIRC I think resolving any path was "accepted" as a practice a while ago for obscure setups like mine where an entire TMPDIR might be a symlink to somewhere else and then comparing resolved paths to non-resolved ones. I think we have some travis matrix runs simulating some scenario like that ... one of the succeeds and another one fails, but there are other fails as well, so I guess those should be addressed first |
Hmm, I thought this was all about I am not saying that it has to be one or the other. However the current state is broken. The documentation says and later (let's ignore misleading POSIX notation) So the return value must be a resolved path. However the code looks like this (note that I labeled the conditionals for easier reference and also note that they are mutually exclusive): dot_git = pathobj / '.git'
# 1
if dot_git.is_file():
with dot_git.open() as f:
line = f.readline()
if line.startswith("gitdir: "):
dot_git = pathobj / line[7:].strip()
else:
raise InvalidGitRepositoryError("Invalid .git file")
# 2
elif dot_git.is_symlink():
dot_git = dot_git.resolve()
# 3
elif not dot_git.exists() and \
(pathobj / 'HEAD').exists() and \
(pathobj / 'config').exists():
# looks like a bare repo
dot_git = pathobj
# 4
elif not (ok_missing or dot_git.exists()):
raise RuntimeError("Missing .git in %s." % pathobj)
return dot_git(1) IF we have an existing (symlinked) .git file that points to a symlinked location: not resolved (2) ELIF we have a symlinked directory or non-existing file: resolved (and since PY3.6 no longer crash in case of the symlink to a non-existing file) (3) ELIF we have a bare repo: not resolved (4) ELIF (also not that So when we are talking about acceptance here, it is the acceptance implied by "my uncle is a lunatic, there is nothing I can do about it". ;-) I would prefer to sort this out and fix it up, even if that means a behavior change. |
It is made redundant by `datalad.dataset.tests.test_gitrepo.py:test_get_dot_git` This old test also required an undesirable .git path resolving, as an implied special case that did not match the described behavior. Resolving paths is now done consistently and explicitly when requested.
I don't currently think it's [resolving the path] necessary. But: If we ever switch the design, to represent the repository and a/the worktree separately, then the identifier for the actual Repo needs to become the resolved .git I think Don't see another reason ATM I think it's OK to change the test accordingly. It may have only be done for
|
Resolving is now an explicit action. If it is explicitly off, and relative paths are passed, the only way to turn them absolute is to consider them relative to CWD -- this is by no means guaranteed to be meaningful or intentional. I now document the behavior. All internal usage is with absolute paths anyways.
2 participants
It now tests the common scenarios first, rather then leaving them for last.
Moreover, it introduces an explicit
resolvedflag for result path reporting, and applies it consistently when enabled -- rather than just for symlinked directories, but not files, or gitdirs specified in .git files. Searching through the code, I did not spot a case where this features is needed, nor desirable.Fixes #6321
Fixes #6324