Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 13, 2018, 04:42 UTC
Message-ID: <CAPig+cTLQ6h+stLLns-837hP0nNOpE3vwu8_ZeO2GoAaDs7buw@mail.gmail.com>
In-Reply-To: <C2FFE6FB-4B3C-4246-9BCA-272EC874FA8B@gmail.com>

On Mon, Feb 12, 2018 at 3:01 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 8 quoted lines

>> On 12 Feb 2018, at 04:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Fix this by changing to the new worktree's directory before running
>> the hook, and adjust the tests to verify that the hook is indeed run
>> within the correct directory.
>
> Looks good but I think we are not quite there yet. It does not work
> for bare repos. You can test this if you apply the following patch on
> top of your changes.

Thanks for providing a useful test case.

The problem is that Git itself exports GIT_DIR with value "." (which makes sense since "git worktree add" is invoked within a bare repo) and that GIT_DIR leaks into the hook's environment. However, since the hook is running within the worktree, into which we've chdir()'d, the relative "." is wrong, and setup.c:is_git_directory() (invoked indirectly by setup_bare_git_dir()) correctly reports that the worktree itself is not a valid Git directory. As a result, 'rev-parse' run by the hook fails with "fatal: Not a git repository: '.'". The fix is either to remove GIT_DIR from the environment or make it absolute so the chdir() doesn't invalidate it.

However, it turns out that builtin/worktree.c already _does_ export GIT_DIR and GIT_WORK_TREE for the commands it invokes ('update-ref', 'symbolic-ref', 'reset --hard') to create the new worktree, which makes perfect sense since these commands need to know the location of the new worktree. So, a second approach/fix is also to use these existing exports when running the hook. The (minor) catch is that they are relative and break upon chdir() but that is easily fixed by making them absolute.

So, either approach works: removing GIT_DIR or using "worktree add"'s existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is consistent with how "worktree add" invokes other command already and, especially, because it also addresses the issue Junio raised of user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's environment.

> Please note that also '"add" within worktree invokes post-checkout hook'
> seems to fail with my extended test case.

Also fixed by either approach.