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

Eric Sunshine [off-list ref] writes:

Although "git worktree add" learned to run the 'post-checkout' hook in
ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
neglects to change to the directory of the newly-created worktree
before running the hook. Instead, the hook is run within the directory
from which the "git worktree add" command itself was invoked, which
effectively neuters the hook since it knows nothing about the new
worktree directory.

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.

 - 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" (which
   does the "cd to the toplevel" thing upfront and then runs hooks
   without doing anything special) and "git clone" (which goes to
   the newly created repository's working tree by calling
   setup.c::setup_work_tree() before builtin/clone.c::checkout(),
   which may call post-checkout hook).
 
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?

On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

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.