> On 16 Feb 2018, at 00:09, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > Although "git worktree add" learned to run the 'post-checkout' hook in > ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it > neglected to change to the directory of the newly-created worktree > before running the hook. Instead, the hook runs 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. > > Further, ade546be47 failed to sanitize the environment before running > the hook, which means that user-assigned values of GIT_DIR and > GIT_WORK_TREE could mislead the hook about the location of the new > worktree. In the case of "git worktree add" being run from a bare > repository, the GIT_DIR="." assigned by Git itself leaks into the hook's > environment and breaks Git commands; this is so even when the working > directory is correctly changed to the new worktree before the hook runs > since ".", relative to the new worktree directory, does not point at the > bare repository. > > Fix these problems by (1) changing to the new worktree's directory > before running the hook, and (2) sanitizing the environment of GIT_DIR > and GIT_WORK_TREE so hooks can't be confused by misleading values. > > Enhance the t2025 'post-checkout' tests to verify that the hook is > indeed run within the correct directory and that Git commands invoked by > the hook compute Git-dir and top-level worktree locations correctly. > > While at it, also add two new tests: (1) verify that the hook is run > within the correct directory even when the new worktree is created from > a sibling worktree (as opposed to the main worktree); (2) verify that > the hook is provided with correct context when the new worktree is > created from a bare repository (test provided by Lars Schneider). Thanks! This patch works great and fixes the problem! More comments below. > Implementation Notes: > > Rather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an > alternative would be to set them explicitly, as is already done for > other Git commands run internally by "git worktree add". This patch opts > instead to sanitize the environment in order to clearly document that > the worktree is fully functional by the time the hook is run, thus does > not require special environmental overrides. > > The hook is run manually, rather than via run_hook_le(), since it needs > to change the working directory to that of the worktree, and > run_hook_le() does not provide such functionality. As this is a one-off > case, adding 'run_hook' overloads which allow the directory to be set > does not seem warranted at this time. Although this is an one-off case, I would still prefer it if all hook invocations would happen in a central place to avoid future surprises. > Reported-by: Lars Schneider <larsxschneider@xxxxxxxxx> > Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> > --- > > This is a re-roll of [1] which fixes "git worktree add" to provide > proper context to the 'post-checkout' hook so that the hook knows the > location of the newly-created worktree. > > Changes since v2: > > * Fix crash due to missing NULL-terminator on 'env' list passed to > run_command(). > > [1]: https://public-inbox.org/git/20180215191841.40848-1-sunshine@xxxxxxxxxxxxxx/ > > builtin/worktree.c | 20 ++++++++++++--- > t/t2025-worktree-add.sh | 54 ++++++++++++++++++++++++++++++++++------- > 2 files changed, 62 insertions(+), 12 deletions(-) > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 7cef5b120b..f69f862947 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -345,9 +345,23 @@ static int add_worktree(const char *path, const char *refname, > * Hook failure does not warrant worktree deletion, so run hook after > * is_junk is cleared, but do return appropriate code when hook fails. > */ > - if (!ret && opts->checkout) > - ret = run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid), > - oid_to_hex(&commit->object.oid), "1", NULL); > + if (!ret && opts->checkout) { > + const char *hook = find_hook("post-checkout"); > + if (hook) { > + const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL }; > + cp.git_cmd = 0; > + cp.no_stdin = 1; > + cp.stdout_to_stderr = 1; > + cp.dir = path; > + cp.env = env; > + cp.argv = NULL; > + argv_array_pushl(&cp.args, absolute_path(hook), > + oid_to_hex(&null_oid), > + oid_to_hex(&commit->object.oid), > + "1", NULL); > + ret = run_command(&cp); > + } > + } > > argv_array_clear(&child_env); > strbuf_release(&sb); > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > index 2b95944973..d0d2e4f7ec 100755 > --- a/t/t2025-worktree-add.sh > +++ b/t/t2025-worktree-add.sh > @@ -451,32 +451,68 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' ' > ' > > post_checkout_hook () { > - test_when_finished "rm -f .git/hooks/post-checkout" && > - mkdir -p .git/hooks && > - write_script .git/hooks/post-checkout <<-\EOF > - echo $* >hook.actual > + gitdir=${1:-.git} > + test_when_finished "rm -f $gitdir/hooks/post-checkout" && > + mkdir -p $gitdir/hooks && > + write_script $gitdir/hooks/post-checkout <<-\EOF > + { > + echo $* > + git rev-parse --git-dir --show-toplevel I also checked `pwd` here in my suggested test case. I assume you think this check is not necessary? > + } >hook.actual > EOF > } > > test_expect_success '"add" invokes post-checkout hook (branch)' ' > post_checkout_hook && > - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect && > + { > + echo $_z40 $(git rev-parse HEAD) 1 && > + echo $(pwd)/.git/worktrees/gumby && > + echo $(pwd)/gumby > + } >hook.expect && > git worktree add gumby && > - test_cmp hook.expect hook.actual > + test_cmp hook.expect gumby/hook.actual > ' > > test_expect_success '"add" invokes post-checkout hook (detached)' ' > post_checkout_hook && > - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect && > + { > + echo $_z40 $(git rev-parse HEAD) 1 && > + echo $(pwd)/.git/worktrees/grumpy && > + echo $(pwd)/grumpy > + } >hook.expect && > git worktree add --detach grumpy && > - test_cmp hook.expect hook.actual > + test_cmp hook.expect grumpy/hook.actual > ' > > test_expect_success '"add --no-checkout" suppresses post-checkout hook' ' > post_checkout_hook && > rm -f hook.actual && > git worktree add --no-checkout gloopy && > - test_path_is_missing hook.actual > + test_path_is_missing gloopy/hook.actual > +' > + > +test_expect_success '"add" in other worktree invokes post-checkout hook' ' > + post_checkout_hook && > + { > + echo $_z40 $(git rev-parse HEAD) 1 && > + echo $(pwd)/.git/worktrees/guppy && > + echo $(pwd)/guppy > + } >hook.expect && > + git -C gloopy worktree add --detach ../guppy && > + test_cmp hook.expect guppy/hook.actual > +' > + > +test_expect_success '"add" in bare repo invokes post-checkout hook' ' > + rm -rf bare && > + git clone --bare . bare && > + { > + echo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 && > + echo $(pwd)/bare/worktrees/goozy && > + echo $(pwd)/goozy > + } >hook.expect && > + post_checkout_hook bare && > + git -C bare worktree add --detach ../goozy && > + test_cmp hook.expect goozy/hook.actual > ' > > test_done > -- > 2.16.1.370.g5c508858fb >
