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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 12, 2018, 20:31 UTC
Message-ID: <CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=Q@mail.gmail.com>
In-Reply-To: <xmqq7erit2wo.fsf@gitster-ct.c.googlers.com>

On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> 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.
>
> I like the approach taken by this replacement better.  Just to make
> sure I understand the basic idea, let me rephrase what these two
> patches are doing:
>
>  - "path" that is made absolute in this step is where the new
>    worktree is created, i.e. the top-level of the working tree in
>    the new worktree.  We chdir there and then run the hook script.

Sorry for misleading. The "absolute path" stuff in this patch is unnecessary; it's probably just left-over from Lars's proposal which did need to make it absolute when setting GIT_WORK_TREE, and I likely didn't think hard enough to realize that it doesn't need to be absolute just for chdir(). I'll drop the unnecessary absolute_pathdup() in the re-roll.

(The hook path in patch 1/2, on the other hand, does need to be made absolute since find_hook() returns a relative path before we've chdir()'d into the new worktree.)

>  - Even though we often see hooks executed inside .git/ directory,
>    for post-checkout, the top-level of the working tree is the right
>    place, as that is where the hook is run by "git checkout" [...]

Patch 1/2's commit message is a bit sloppy in its description of this. I'll tighten it up in the re-roll.

I'm also not fully convinced that these new overloads of run_hook_*() are warranted since it's hard to imagine any other case when they would be useful. It may make sense just to have builtin/worktree.c run the hook itself for this one-off case.

Show 6 quoted lines

> I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the
> environment, though.  When a user with a funny configuration (where
> these two environment variables are pointing at unusual places) uses
> "git worktree add" to create another worktree for the repository, it
> would not be sufficient to chdir to defeat them that are appropriate
> for the original, and not for the new, worktree, would it?

Good point. I'll look into sanitizing the environment.