Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook
- Date: Mon, 12 Feb 2018 15:31:28 -0500
- From: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
- Subject: Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook
On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
> 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.