Re: [PATCHv1 1/2] git-p4: support git-workspaces

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2016, 21:54 UTC
Message-ID: <xmqqzika2ga5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

> Luke Diamand <luke@diamand.org> 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.