Re: [RFC] require-work-tree wants more than what its name says

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: May 4, 2011, 08:47 UTC
Message-ID: <4DC112A1.5090805@drmicha.warpmail.net>
In-Reply-To: <7vhb9bgy0a.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 04.05.2011 01:33:

Show 30 quoted lines

> Somebody tried "git pull" from a random place completely outside the work
> tree, while exporting GIT_DIR and GIT_WORK_TREE that are set to correct
> places, e.g.
> 
> 	GIT_WORK_TREE=$HOME/git.git
>         GIT_DIR=$GIT_WORK_TREE/.git
>         export GIT_WORK_TREE GIT_DIR
>         cd /tmp
>         git pull
> 
> At the beginning of git-pull, we check "require-work-tree" and then
> "cd-to-toplevel".  I _think_ the original intention when I wrote the
> command was "we MUST have a work tree, our $(cwd) might not be at the
> top-level directory of it", and no stronger than that.  That check is a
> very sensible thing to do before doing cd-to-toplevel.  We check that the
> place we would want to go exists, and then go there.
> 
> But the implementation of require_work_tree we have today is quite
> different.  I don't have energy to dig the history, but currently it says:
> 
>  	test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
>  	die "fatal: $0 cannot be used without a working tree."
> 
> Which is completely bogus.  Even though we may happen to be just outside
> of it right now, we may have a working tree that we can cd_to_toplevel
> back to.
> 
> I recall there was a discussion sometime last year about this topic, and
> vaguely recall somebody proposed to swap the order of cd-to-toplevel and
> require-work-tree.  While I agree that would sweep the issue under the rug,

I may have been that one that year or another...

Show 7 quoted lines

> I think the right solution would be to apply the attached patch; and then
> audit all the callers that call "require_work_tree" to see if any of them
> meant "No, it is not Ok just to have working tree somewhere! I want you to
> be IN that working tree when you call me", and convert them to call the
> new require_to_be_in_work_tree instead.
> 
> Thoughts?

Same thoughts as Jeff. git-sh-setup is explicitly meant to be used by scripters, and the users in git.git serve as an example how to use it. If that usage changes it requires a long migration plan.

The only thing I can imagine doing right now is changing require_work_tree() to actually cd to toplevel when possible, so that (like before) on success we're really within. But that changes cwd, of course. In summary, a require_work_tree() now can have three assumptions when it returns with success:

- we have a worktree
- we are within worktree
- cwd has not changed

I'd rather break the last one than the second one, but breaking any may be a problem, depending on the caller.

Michael