Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 27, 2015, 05:57 UTC
Message-ID: <CACsJy8AuO5PNy=oFN1W9EQKK5_=jRkSNx8KhQYjL-0mW+jvr0A@mail.gmail.com>
In-Reply-To: <20150626115603.GB4315@peff.net>

On Fri, Jun 26, 2015 at 6:56 PM, Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

> On Fri, Jun 26, 2015 at 05:37:35PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This is where the "fun" is. The legacy behavior is, if $GIT_WORK_TREE is
>> not set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen
>> to stand at worktree's top when you do this, all is well. If you are in
>> a subdir "foo/bar" (real worktree's top is "foo"), this behavior bites
>> you: your detected worktree is now "foo/bar", but the first run
>> correctly detected worktree as "foo". You get "internal error: work tree
>> has already been set" as a result.
>
> I think this makes sense. I feel like we've dealt with this before, but
> the two previous rounds I found were basically:
>
>   - we have GIT_IMPLICIT_WORK_TREE, but that is for the _opposite_ case.
>     I.e., when we do not have a work tree and must communicate so to
>     later code (including sub-processes).
>
>   - a discussion about switching the "work tree defaults to '.' when
>     $GIT_DIR is set" behavior yielded almost the identical patch:
>
>       http://article.gmane.org/gmane.comp.version-control.git/219196
>
>     but we were so wrapped up in the greater discussion we did not apply
>     that simple fix. :)

There's also the patch "[PATCH v2] setup.c: set workdir when gitdir is not default" from Max Kirillov last year (gmane down, no link), so we have one patch about this every year since 2013 :)

Junio if you are worried about unnecessary setenv, I think Max's approach is safer as he solves a specific use case. If this problem pops up again in another use case, we can deal with that again.

Show 6 quoted lines

>> +     if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
>> +             error("Could not set GIT_WORK_TREE to '%s'", work_tree);
>
> Should this be die()? setenv() should basically never fail, but if it
> does, it could be confusing and/or dangerous to run without the variable
> set.

It's a straight copy from set_git_dir() but I guess the situation is a bit different. If setenv fails in gitdir, no repo is found but if it fails here we may select a wrong worktree and could wipe it out by mistake. Will fix if Junio chooses this patch instead of Max's.

-- 
Duy