Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

[John Keeping <john@keeping.me.uk>]

On Tue, Mar 29, 2016 at 11:00:03PM +0100, John Keeping wrote:
> On Tue, Mar 29, 2016 at 05:21:43PM -0400, Jeff King wrote:
> > On Tue, Mar 29, 2016 at 09:52:08PM +0100, John Keeping wrote:
> > 
> > > > Yeah, I think this is a bug. Presumably what is happening is that we are
> > > > too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> > > > we ask "are we in a work tree", the answer has become yes. But the
> > > > caller really wants to know "am _I_ inside the work tree".
> > > 
> > > I don't think that's what's happening.  Try:
> > > 
> > > 	$ cd .git/
> > > 	$ GIT_WORK_TREE=.. git rev-parse --is-inside-work-tree
> > > 	true
> > > 
> > > so I think it's that we refuse to assume that the directory above a Git
> > > directory is a working tree (something similar happens when the
> > > "core.worktree" config variable is set).  I'm not convinced that's
> > > unreasonable.
> > 
> > Yeah, you're right, but I'm not sure how your example shows that, (isn't
> > it basically the same as Elliott's original, except using a relative
> > path?). A more compelling counter-example to my hypothesis is:
> > 
> >   $ cd .git
> >   $ GIT_WORK_TREE=/tmp git rev-parse --is-inside-work-tree
> >   false
> > 
> > So it is not that we chdir too early, but just that we blindly check "is
> > $(pwd) inside $GIT_WORK_TREE". And it does not create a problem for the
> > normal discovered-path cases, because either:
> > 
> >   - we discovered .git by walking up the directory tree, which means we
> >     must be in a work-tree
> > 
> >   - we discovered that we are inside a .git directory, and therefore
> >     take it to be bare (and thus there is no work tree, and we cannot be
> >     inside it). This is what happens in Elliott's original example that
> >     behaves differently than the $GIT_WORK_TREE case.
> > 
> > I'd be tempted to say that "inside the work tree" is further clarified
> > to "not inside the $GIT_DIR".
> 
> Yes, I think that's reasonable.  But...
> 
> > > However, the case above also gives:
> > > 
> > > 	$ GIT_WORK_TREE=.. git rev-parse --is-inside-git-dir
> > > 	false
> > > 	$ test $(pwd) = $(GIT_WORK_TREE=.. git rev-parse --git-dir); echo $?
> > > 	0
> > > 
> > > so even though $PWD *is* the Git directory, we're not in the Git
> > > directory!  Setting GIT_DIR=$(pwd) makes no different to that.
> > 
> > We seem to get that wrong. I'm also not sure if it would make sense if
> > you explicitly set the two to be equal, like:
> > 
> >   # checking in your own refs?
> >   GIT_WORK_TREE=$(pwd) GIT_DIR=$(pwd) git add refs packed-refs
> > 
> > So the current behavior may just be weird-but-true.
> 
> This case definitely feels wrong:
> 
> 	$ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git rev-parse --is-inside-git-dir
> 	false
> 
> Shouldn't that be the same as if GIT_WORK_TREE and GIT_DIR aren't set?
> (It's also potentially surprising since "git rev-parse --git-dir" does
> give the right answer in this case.)
> 
> If GIT_WORK_TREE points somewhere unrelated then it is correct:
> 
> 	$ GIT_WORK_TREE=/tmp GIT_DIR=$(pwd) git rev-parse --is-inside-git-dir
> 	true

It seems that this is a result of changing the working directory to the
root of the working tree if we're inside it.  is_inside_dir() doesn't
take account of startup_info->prefix and changing to:

	real_path(startup_info->prefix)

instead of xgetcwd() means that these tests are less surprising.

But I haven't run the test suite or thought about what else this could
break.