Junio C Hamano <gitster@xxxxxxxxx> writes:
> Luke Diamand <luke@xxxxxxxxxxx> writes:
> ...
> if (os.path.exists(path + "/HEAD")
> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> return True;
> +
> + # secondary working tree managed by "git worktree"?
> + if (os.path.exists(path + "/HEAD")
> + and os.path.exists(path + "/gitdir")):
> + return True
> +
> return False
Wait a bit. How is this expected to work?
I assume "path" is something like "$somewhere/.git" (either
absolute or relative), and by concatenating HEAD, refs and objects
under it the original checks form paths to expected directories,
because "$somewhere/.git" is a directory.
A worktree created with "git worktree add" gets ".git" that is a
file, and the contents of the file records the path to a
subdirectory "worktrees/$name" of the primary repository the
worktree is borrowing from, i.e. "$somewhere/.git/worktrees/$name".
So for this patch to work correctly, the caller, when in a "git
worktree" managed secondary working tree, MUST have found the ".git"
file at the root level of the working tree, MUST have read its
contents to learn that "$somewhere/.git/worktrees/$name" path, and
then feeding it to this function. But doesn't such a caller already
know that it is a valid repository? After all, it trusted the
contents of that ".git" file at the root level.
... goes and looks at the callers ...
Between the two call sites of isValidGitDir() in main(), the first
one (i.e. if ".git" in the current directory, made into abspath,
does not look like a repository, ask "rev-parse --git-dir" for one)
looks correct and I think it would grab the correct $GIT_DIR just
fine [*1*]. Isn't the real problem the second one, i.e. the one
that feeds a correct cmd.gitdir that we just obtained by asking
"rev-parse --git-dir" to the sloppy isValidGitDir() again. The
latter second guesses "rev-parse --git-dir" and botches.
I have a feeling that adding more to isValidGitDir() like this patch
does is a step in the wrong direction. The first fix would be not
to do the "if isValidGitDir() says no, then do something else" step
if you already asked "rev-parse --git-dir" and got a valid $GIT_DIR,
perhaps?
Stepping back even more.
I wonder why the script needs to do os.environ.get("GIT_DIR") in the
first place to initialize cmd.gitdir field. If it instead asked
"rev-parse --gitdir", the script would get the right answer that
already takes GIT_DIR environment into account.
[Footnote]
*1* This ends up asking "rev-parse --git-dir" only because
isValidGitDir() is sloppy. If you are in a secondary working
tree whose ".git" is a file, the function would say "no", and
that is the only thing that allows you to make a call to
"rev-parse --git-dir" to obtain the correct result.