Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree

From: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>
Date: Jul 30, 2010, 11:04 UTC
Message-ID: <4C52B1BE.8020804@nokia.com>
In-Reply-To: <7v4ofjw6t6.fsf@alter.siamese.dyndns.org>

Hey, and thanks for your feedback Junio!

On 29.07.10 01.00, ext Junio C Hamano wrote:

Show 14 quoted lines

> Tor Arne Vestbø <tor.arne.vestbo@nokia.com> writes:
>
>  > The documentation describes require_work_tree as guarding against
>  > bare repositories, and that's also the way it's used from porcelain
>  > such as git-rebase. When implemented using --is-inside-work-tree
>  > the samantics change, causing git-rebase to fail if run from outside
>  > GIT_WORK_TREE, even if GIT_WORK_TREE is valid.
>  >
>  > Signed-off-by: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>
>  > ---
>
> The "requirement" is that we _have_ work tree somewhere that we can
> cd-to-toplevel to if we wanted to, not that we _are_ already in the work
> tree. I can buy that rationale.

Right. You put it much nicer than me :)

Show 8 quoted lines

> However, I notice that "git bisect", "git mergetool" and "git submodule"
> do not seem to do cd_to_topleve immediately after require_work_tree. The
> last one has cd_to_toplevel in later parts of the codepath, presumably so
> that it can collect paths relative to the subdirectory in the work tree.
> I wonder if all of them actually need to be run from inside a work tree?
> Don't they need a separate "git rev-parse --is-inside-work-tree || die"
> check after require_work_tree (or perhaps cd_to_toplevel) if we apply this
> patch?

I think if we have a work tree somewhere, we can at least do "git rev-parse --is-inside-work-tree || cd_to_toplevel" instead of dying, unless there's some danger to that (the user running a command from outside GIT_WORK_TREE but expecting GIT_WORK_TREE to not be touched).

For "git bisect" and "git submodule" running them in a sub-directory of the work tree complains about needing to be run from the top-level, so I assume we can do an unconditional cd_to_toplevel after the require_work_tree?

For "git mergetool" we should probably do it conditionally only if the user is not inside a work tree already, so that the behavior of running the tool in a sub-directory is not changed.

Tor Arne