Inconsistent results from git rev-parse --show-toplevel

Inconsistent results from git rev-parse --show-toplevel

[Crabtree, Andrew]

I ran into an issue where 'git rev-parse --show-toplevel' is reporting the current directory instead of the root of the repository.
In my case I'm running it from within the pre-commit hook, and it seems like it might be related to GIT_DIR being set. 
It may also be related to using worktrees.  Haven't had a chance to test thoroughly.

Regards,
-Andrew

Git Version -  2.25.0

pre-commit hook

#!/bin/bash
env
pwd
git rev-parse --show-toplevel
cd subdir
pwd
git-rev-parse --show-toplevel

Create a repo with a subdirectory.   Commit as normal.  Output from the hook looks as expected.  Create a worktree, and commit there and it appears as though it is returning incorrect results after changing directory.

Git repo is in /tmp/revparse_test
Worktree is in /tmp/revparse_test_worktree

Committing in /tmp/revparse_test yields the below, as expected 
/tmp/revparse_test
/tmp/revparse_test
/tmp/revparse_test/subdir
/tmp/revparse_test


Committing in the worktree yields below, with the final line seeming incorrect.
/tmp/revparse_test_worktree
/tmp/revparse_test_worktree
/tmp/revparse_test_worktree/subdir
/tmp/revparse_test_worktree/subdir

Re: Inconsistent results from git rev-parse --show-toplevel

[Junio C Hamano]

"Crabtree, Andrew" <andrew.crabtree@hpe.com> writes:

> I ran into an issue where 'git rev-parse --show-toplevel' is reporting the current directory instead of the root of the repository.
> In my case I'm running it from within the pre-commit hook, and it seems like it might be related to GIT_DIR being set. 

Running "git" with GIT_DIR exported (or "git --git-dir=<dir>") tells
Git that it should not perform the usual "does this directory has a
subdirectory .git that is a repository?  If not, go one level up and
repeat the check" repository discovery.  Because the repository
discovery serves the purpose of finding both the repository and the
top of the working tree, disabling the discovery means you either
need to tell Git where the top of the working tree is, or you are
already at the top of the working tree by chdir()'ing to it.

Because you do not know where the top level is (otherwise you would
not be asking "rev-parse --show-toplevel" about it), either is
an option for you, but you can "unset GIT_DIR" to stop telling Git
that it should not perform the repository discovery.

RE: Inconsistent results from git rev-parse --show-toplevel

[Crabtree, Andrew]

> From: Junio C Hamano [mailto:gitster@pobox.com] 
> Because you do not know where the top level is (otherwise you would not be asking "rev-parse --show-toplevel" about it), either is an option for you, 
> but you can "unset GIT_DIR" to stop telling Git that it should not perform the repository discovery.

Gotcha, thanks.  Would it make sense to have 'git rev-parse --show-toplevel' error out if GIT_DIR is set?  Or update the rev-parse documentation with a warning about GIT_DIR? 

Regards,
-Andrew

Re: Inconsistent results from git rev-parse --show-toplevel

[Jeff King]

On Sat, Jan 25, 2020 at 07:31:20PM +0000, Crabtree, Andrew wrote:

> > From: Junio C Hamano [mailto:gitster@pobox.com] 
> > Because you do not know where the top level is (otherwise you would not be asking "rev-parse --show-toplevel" about it), either is an option for you, 
> > but you can "unset GIT_DIR" to stop telling Git that it should not
> > perform the repository discovery.
> 
> Gotcha, thanks.  Would it make sense to have 'git rev-parse
> --show-toplevel' error out if GIT_DIR is set?  Or update the rev-parse
> documentation with a warning about GIT_DIR? 

No, it shouldn't be an error; setting GIT_DIR means that your current
directory is the worktree. That _is_ confusing in some situations, but
at this point it's a historical thing that I don't think we want to
change. Discussing it in the documentation might be good (it's probably
in there somewhere already, but a pointer from --show-toplevel doesn't
seem unreasonable).

But the bigger thing, I think, is: who is setting GIT_DIR but not
setting GIT_WORK_TREE to match? Because IMHO that's the situation that
is causing the confusion.

-Peff

RE: Inconsistent results from git rev-parse --show-toplevel

[Crabtree, Andrew]

> But the bigger thing, I think, is: who is setting GIT_DIR but not setting GIT_WORK_TREE to match? Because IMHO that's the situation that is causing the confusion.
Pre-commit hook when worktrees are used? 

pre-commit
#!/bin/bash
env | grep GIT

/tmp/pre_commit_test_worktree  (new_branch)$ git add frob
/tmp/pre_commit_test_worktree  (new_branch)$ git commit -m "frob"
GIT_DIR=/tmp/pre_commit_test/.git/worktrees/pre_commit_test_worktree
GIT_EDITOR=:
GIT_INDEX_FILE=/tmp/pre_commit_test/.git/worktrees/pre_commit_test_worktree/index
GIT_PREFIX=
GIT_AUTHOR_DATE=@1579990789 -0800
GIT_AUTHOR_NAME=Andrew Crabtree
GIT_EXEC_PATH=/usr/local/libexec/git-core
GIT_AUTHOR_EMAIL=andrew.crabtree@hpe.com
[new_branch 7b1b747] frob
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 frob

Re: Inconsistent results from git rev-parse --show-toplevel

[Jeff King]

On Sat, Jan 25, 2020 at 10:22:45PM +0000, Crabtree, Andrew wrote:

> > But the bigger thing, I think, is: who is setting GIT_DIR but not setting GIT_WORK_TREE to match? Because IMHO that's the situation that is causing the confusion.
> Pre-commit hook when worktrees are used? 
> 
> pre-commit
> #!/bin/bash
> env | grep GIT
> 
> /tmp/pre_commit_test_worktree  (new_branch)$ git add frob
> /tmp/pre_commit_test_worktree  (new_branch)$ git commit -m "frob"
> GIT_DIR=/tmp/pre_commit_test/.git/worktrees/pre_commit_test_worktree
> GIT_EDITOR=:
> GIT_INDEX_FILE=/tmp/pre_commit_test/.git/worktrees/pre_commit_test_worktree/index
> GIT_PREFIX=
> GIT_AUTHOR_DATE=@1579990789 -0800
> GIT_AUTHOR_NAME=Andrew Crabtree
> GIT_EXEC_PATH=/usr/local/libexec/git-core
> GIT_AUTHOR_EMAIL=andrew.crabtree@hpe.com
> [new_branch 7b1b747] frob
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 frob

I dug into this a little bit. I think the culprit is actually that our
internal set_git_dir() puts GIT_DIR into the environment, but the
matching setup_work_tree() doesn't touch GIT_WORK_TREE unless it's
already set.

This is mostly fine because we chdir() to the top-level of the working
tree, meaning that any sub-processes would see the correct environment.
But if we execute some arbitrary script (like a hook) that does a chdir,
the results are surprising.

Something like this seems like it would be an improvement:

diff --git a/setup.c b/setup.c
index e2a479a64f..75e2d1393c 100644
--- a/setup.c
+++ b/setup.c
@@ -394,12 +394,7 @@ void setup_work_tree(void)
 	if (!work_tree || chdir_notify(work_tree))
 		die(_("this operation must be run in a work tree"));
 
-	/*
-	 * Make sure subsequent git processes find correct worktree
-	 * if $GIT_WORK_TREE is set relative
-	 */
-	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
-		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+	setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1);
 
 	initialized = 1;
 }

but it fails a test in t5601 around git-clone. So there may be some
weird subtle interaction here (or possibly just a bug in git-clone, if
it isn't careful enough to clean its environment when moving into the
newly created repo).

-Peff

RE: Inconsistent results from git rev-parse --show-toplevel

[Crabtree, Andrew]

From: Jeff King [mailto:peff@peff.net] 
Sent: Thursday, January 30, 2020 2:30 AM
To: Crabtree, Andrew <andrew.crabtree@hpe.com>
Cc: Junio C Hamano <gitster@pobox.com>; git@vger.kernel.org
Subject: Re: Inconsistent results from git rev-parse --show-toplevel
> > But the bigger thing, I think, is: who is setting GIT_DIR but not setting GIT_WORK_TREE to match? Because IMHO that's the situation that is causing the confusion.

> but it fails a test in t5601 around git-clone. 

Thanks jeff.  It looks like this might have been tried previously and abandoned?  I'm pretty far out of my league here in terms of how things are supposed to operate and any history around the previous attempts at making it work.

-A 


commit d95138e695d99d32dcad528a2a7974f434c51e79
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Fri Jun 26 17:37:35 2015 +0700

setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR

Which was subsequently reverted 

commit 86d26f240fcb4f287258ad459efc2b5e30e60cfd
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Sun Dec 20 14:50:18 2015 +0700

    setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..
    
    Commit d95138e [1] attempted to fix a .git file problem by
    setting GIT_WORK_TREE whenever GIT_DIR is set. It sounded harmless
    because we handle GIT_DIR and GIT_WORK_TREE side by side for most
    commands, with two exceptions: git-init and git-clone.
    
    "git clone" is not happy with d95138e.

Re: Inconsistent results from git rev-parse --show-toplevel

[Jeff King]

On Thu, Jan 30, 2020 at 09:59:31PM +0000, Crabtree, Andrew wrote:

> > > But the bigger thing, I think, is: who is setting GIT_DIR but not
> > > setting GIT_WORK_TREE to match? Because IMHO that's the situation
> > > that is causing the confusion.
> 
> > but it fails a test in t5601 around git-clone. 
> 
> Thanks jeff.  It looks like this might have been tried previously and
> abandoned?  I'm pretty far out of my league here in terms of how
> things are supposed to operate and any history around the previous
> attempts at making it work.
> [...]
> commit d95138e695d99d32dcad528a2a7974f434c51e79

Yeah, the commit you found was doing exactly the thing I suggested. IMHO
the right path forward is to actually fix the weirdness in git-clone. It
would be a backwards incompatibility, but a pretty obscure one. I think
we're likely to help more people than hurt by being able to handle
$GIT_WORK_TREE consistently. At least that's my gut feeling.

I guess one way to fix it without breaking compatibility would be for us
to set $GIT_WORK_TREE alongside $GIT_DIR, but _also_ set a special
$GIT_CLONE_NO_RESPECT_WORK_TREE variable that would instruct it that the
caller isn't trying to do trigger the special $GIT_WORK_TREE behavior.

But we'd have to carry that hack around forever, which doesn't excite
me.

-Peff