Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location

[Lars Schneider <larsxschneider@gmail.com>]

> On 16 Feb 2018, at 00:09, Eric Sunshine <sunshine@sunshineco.com> 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@gmail.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> 
> 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@sunshineco.com/
> 
> 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
>