[PATCH v1] worktree: set worktree environment in post-checkout hook

[PATCH v1] worktree: set worktree environment in post-checkout hook

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

In ade546be47 (worktree: invoke post-checkout hook (unless
--no-checkout), 2017-12-07) we taught Git to run the post-checkout hook
in worktrees. Unfortunately, the environment of the hook was not made
aware of the worktree. Consequently, a 'git rev-parse --show-toplevel'
call in the post-checkout hook would return a wrong result.

Fix this by setting the 'GIT_WORK_TREE' environment variable to make
Git calls within the post-checkout hook aware of the worktree.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

Hi,

I think this is a bug in Git 2.16. We noticed it because it caused a
problem in Git LFS [1]. The modified test case fails with Git 2.16 and
succeeds with this patch.

Cheers,
Lars


[1] https://github.com/git-lfs/git-lfs/issues/2848


Notes:
    Base Ref: v2.16.1
    Web-Diff: https://github.com/larsxschneider/git/commit/214e9342e7
    Checkout: git fetch https://github.com/larsxschneider/git fix-worktree-add-v1 && git checkout 214e9342e7

 builtin/worktree.c      |  7 +++++--
 t/t2025-worktree-add.sh | 11 +++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..032f9b86bf 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -345,9 +345,12 @@ 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),
+	if (!ret && opts->checkout) {
+		struct argv_array env = ARGV_ARRAY_INIT;
+		argv_array_pushf(&env, "GIT_WORK_TREE=%s", absolute_path(path));
+		ret = run_hook_le(env.argv, "post-checkout", oid_to_hex(&null_oid),
 				  oid_to_hex(&commit->object.oid), "1", NULL);
+	}

 	argv_array_clear(&child_env);
 	strbuf_release(&sb);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..d022ac0c26 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -455,19 +455,26 @@ post_checkout_hook () {
 	mkdir -p .git/hooks &&
 	write_script .git/hooks/post-checkout <<-\EOF
 	echo $* >hook.actual
+	git rev-parse --show-toplevel >>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 &&
+	cat >hook.expect <<-EOF &&
+		$_z40 $(git rev-parse HEAD) 1
+		$(pwd)/gumby
+	EOF
 	git worktree add gumby &&
 	test_cmp hook.expect 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 &&
+	cat >hook.expect <<-EOF &&
+		$_z40 $(git rev-parse HEAD) 1
+		$(pwd)/grumpy
+	EOF
 	git worktree add --detach grumpy &&
 	test_cmp hook.expect hook.actual
 '

base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa
--
2.16.1

Re: [PATCH v1] worktree: set worktree environment in post-checkout hook

[Lars Schneider]

> On 10 Feb 2018, at 02:01, lars.schneider@autodesk.com wrote:
> 
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> In ade546be47 (worktree: invoke post-checkout hook (unless
> --no-checkout), 2017-12-07) we taught Git to run the post-checkout hook
> in worktrees. Unfortunately, the environment of the hook was not made
> aware of the worktree. Consequently, a 'git rev-parse --show-toplevel'
> call in the post-checkout hook would return a wrong result.
> 
> Fix this by setting the 'GIT_WORK_TREE' environment variable to make
> Git calls within the post-checkout hook aware of the worktree.
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> 
> Hi,
> 
> I think this is a bug in Git 2.16. We noticed it because it caused a
> problem in Git LFS [1]. The modified test case fails with Git 2.16 and
> succeeds with this patch.
> 
> Cheers,
> Lars
> 
> 
> [1] https://github.com/git-lfs/git-lfs/issues/2848
> 
> 
> Notes:
>    Base Ref: v2.16.1
>    Web-Diff: https://github.com/larsxschneider/git/commit/214e9342e7
>    Checkout: git fetch https://github.com/larsxschneider/git fix-worktree-add-v1 && git checkout 214e9342e7
> 
> builtin/worktree.c      |  7 +++++--
> t/t2025-worktree-add.sh | 11 +++++++++--
> 2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..032f9b86bf 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -345,9 +345,12 @@ 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),
> +	if (!ret && opts->checkout) {
> +		struct argv_array env = ARGV_ARRAY_INIT;
> +		argv_array_pushf(&env, "GIT_WORK_TREE=%s", absolute_path(path));
> +		ret = run_hook_le(env.argv, "post-checkout", oid_to_hex(&null_oid),
> 				  oid_to_hex(&commit->object.oid), "1", NULL);

As I hit "send" I realized that I forgot to cleanup.
@Junio: Can you squash this in?

	argv_array_clear(&env);

Thanks,
Lars

> +	}
> 
> 	argv_array_clear(&child_env);
> 	strbuf_release(&sb);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..d022ac0c26 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -455,19 +455,26 @@ post_checkout_hook () {
> 	mkdir -p .git/hooks &&
> 	write_script .git/hooks/post-checkout <<-\EOF
> 	echo $* >hook.actual
> +	git rev-parse --show-toplevel >>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 &&
> +	cat >hook.expect <<-EOF &&
> +		$_z40 $(git rev-parse HEAD) 1
> +		$(pwd)/gumby
> +	EOF
> 	git worktree add gumby &&
> 	test_cmp hook.expect 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 &&
> +	cat >hook.expect <<-EOF &&
> +		$_z40 $(git rev-parse HEAD) 1
> +		$(pwd)/grumpy
> +	EOF
> 	git worktree add --detach grumpy &&
> 	test_cmp hook.expect hook.actual
> '
> 
> base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa
> --
> 2.16.1
> 

[PATCH 0/2] worktree: change to new worktree dir before running hook(s)

[Eric Sunshine]

This patch series replaces "worktree: set worktree environment in
post-checkout hook"[1] from Lars, which is a proposed bug fix for
ade546be47 (worktree: invoke post-checkout hook, 2017-12-07).

The problem that patch addresses is that "git worktree add" does not
provide proper context to the invoked 'post-checkout' hook, so the hook
doesn't know where the newly-created worktree is. Lars's approach was to
set GIT_WORK_TREE to point at the new worktree directory, however, doing
so has a few drawbacks:

1. GIT_WORK_TREE is normally assigned in conjunction with GIT_DIR; it is
   unusual and possibly problematic to set one but not the other.

2. Assigning GIT_WORK_TREE unconditionally may lead to unforeseen
   interactions and problems with end-user scripts and aliases or even
   within Git itself. It seems better to avoid unconditional assignment
   rather than risk problems such as those described and worked around
   by 86d26f240f (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE
   when .., 2015-12-20)

3. Assigning GIT_WORK_TREE is too specialized a solution; it "fixes"
   only Git commands run by the hook, but does nothing for other
   commands ('mv', 'cp', etc.) that the hook might invoke.

The real problem with ade546be47 is that it neglects to change to the
directory of the newly-created worktree before running the hook, thus
the hook incorrectly runs in the directory in which "git worktree add"
was invoked. Rather than messing with GIT_WORK_TREE, this replacement
patch series fixes the problem by ensuring that the directory is changed
before the hook is invoked.

[1]: https://public-inbox.org/git/20180210010132.33629-1-lars.schneider@autodesk.com/

Eric Sunshine (2):
  run-command: teach 'run_hook' about alternate worktrees
  worktree: add: change to new worktree directory before running hook

 builtin/worktree.c      | 11 ++++++++---
 run-command.c           | 23 +++++++++++++++++++++--
 run-command.h           |  4 ++++
 t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---
 4 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.16.1.291.g4437f3f132

[PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees

[Eric Sunshine]

Git commands which run hooks do so at the top level of the worktree in
which the command itself was invoked. However, the 'git worktree'
command may need to run hooks within some other directory. For
instance, when "git worktree add" runs the 'post-checkout' hook, the
hook must be run within the newly-created worktree, not within the
worktree from which "git worktree add" was invoked.

To support this case, add 'run-hook' overloads which allow the
worktree directory to be specified.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 run-command.c | 23 +++++++++++++++++++++--
 run-command.h |  4 ++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index 31fc5ea86e..0e3995bbf9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)
 	return path.buf;
 }
 
-int run_hook_ve(const char *const *env, const char *name, va_list args)
+int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,
+		   va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
 	const char *p;
@@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
 	if (!p)
 		return 0;
 
-	argv_array_push(&hook.args, p);
+	argv_array_push(&hook.args, absolute_path(p));
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&hook.args, p);
+	hook.dir = dir;
 	hook.env = env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
@@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
 	return run_command(&hook);
 }
 
+int run_hook_ve(const char *const *env, const char *name, va_list args)
+{
+	return run_hook_cd_ve(NULL, env, name, args);
+}
+
+int run_hook_cd_le(const char *dir, const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_cd_ve(dir, env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
 int run_hook_le(const char *const *env, const char *name, ...)
 {
 	va_list args;
diff --git a/run-command.h b/run-command.h
index 3932420ec8..8beddffea8 100644
--- a/run-command.h
+++ b/run-command.h
@@ -66,7 +66,11 @@ int run_command(struct child_process *);
 extern const char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
 extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_cd_le(const char *dir, const char *const *env,
+			  const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+extern int run_hook_cd_ve(const char *dir, const char *const *env,
+			  const char *name, va_list args);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
-- 
2.16.1.291.g4437f3f132

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

[Eric Sunshine]

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.

While at it, also add a test to 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).

Reported-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c      | 11 ++++++++---
 t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..b55c55a26c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -345,9 +345,14 @@ 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) {
+		char *p = absolute_pathdup(path);
+		ret = run_hook_cd_le(p, NULL, "post-checkout",
+				     oid_to_hex(&null_oid),
+				     oid_to_hex(&commit->object.oid),
+				     "1", NULL);
+		free(p);
+	}
 
 	argv_array_clear(&child_env);
 	strbuf_release(&sb);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..cf0aaeaf88 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -454,20 +454,29 @@ 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
+	{
+		echo $*
+		git rev-parse --show-toplevel
+	} >../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)/gumby
+	} >hook.expect &&
 	git worktree add gumby &&
 	test_cmp hook.expect 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)/grumpy
+	} >hook.expect &&
 	git worktree add --detach grumpy &&
 	test_cmp hook.expect hook.actual
 '
@@ -479,4 +488,14 @@ test_expect_success '"add --no-checkout" suppresses post-checkout hook' '
 	test_path_is_missing hook.actual
 '
 
+test_expect_success '"add" within worktree invokes post-checkout hook' '
+	post_checkout_hook &&
+	{
+		echo $_z40 $(git rev-parse HEAD) 1 &&
+		echo $(pwd)/guppy
+	} >hook.expect &&
+	git -C gloopy worktree add --detach ../guppy &&
+	test_cmp hook.expect hook.actual
+'
+
 test_done
-- 
2.16.1.291.g4437f3f132

Re: [PATCH v1] worktree: set worktree environment in post-checkout hook

[Eric Sunshine]

On Fri, Feb 9, 2018 at 8:01 PM,  <lars.schneider@autodesk.com> wrote:
> In ade546be47 (worktree: invoke post-checkout hook (unless
> --no-checkout), 2017-12-07) we taught Git to run the post-checkout hook
> in worktrees. Unfortunately, the environment of the hook was not made
> aware of the worktree. Consequently, a 'git rev-parse --show-toplevel'
> call in the post-checkout hook would return a wrong result.
>
> Fix this by setting the 'GIT_WORK_TREE' environment variable to make
> Git calls within the post-checkout hook aware of the worktree.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> I think this is a bug in Git 2.16. We noticed it because it caused a
> problem in Git LFS [1]. The modified test case fails with Git 2.16 and
> succeeds with this patch.

Thanks for reporting and diagnosing the problem.

I have some concerns about this patch's fix of setting GIT_WORK_TREE
unconditionally. In particular, such unconditional setting of
GIT_WORK_TREE might cause unforeseen problems. Although the
circumstances may not be quite the same, but the tale told by
86d26f240f (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE
when .., 2015-12-20) makes me cautious.

More significantly, though, setting GIT_WORK_TREE seems too
specialized a solution. While it may "fix" Git commands invoked by the
hook, it does nothing for other commands ('cp', 'mv', etc.) which the
hook may employ.

As a review comment, I was going to suggest that you chdir() to the
new worktree directory instead of messing with GIT_WORK_TREE, but when
I tested it myself before making the suggestion, I discovered that the
issue is a bit more involved. The result is that I ended up posting a
patch series[1] to replace this one, with what I believe is a more
correct fix.

[1]: https://public-inbox.org/git/20180212031526.40039-1-sunshine@sunshineco.com/

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> 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?

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

[Lars Schneider]

> On 12 Feb 2018, at 04:15, 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
> 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.
> 
> While at it, also add a test to 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).
> 
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> builtin/worktree.c      | 11 ++++++++---
> t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---
> 2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..b55c55a26c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -345,9 +345,14 @@ 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) {
> +		char *p = absolute_pathdup(path);
> +		ret = run_hook_cd_le(p, NULL, "post-checkout",
> +				     oid_to_hex(&null_oid),
> +				     oid_to_hex(&commit->object.oid),
> +				     "1", NULL);
> +		free(p);
> +	}
> 
> 	argv_array_clear(&child_env);
> 	strbuf_release(&sb);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..cf0aaeaf88 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -454,20 +454,29 @@ 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
> +	{
> +		echo $*
> +		git rev-parse --show-toplevel
> +	} >../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)/gumby
> +	} >hook.expect &&
> 	git worktree add gumby &&
> 	test_cmp hook.expect 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)/grumpy
> +	} >hook.expect &&
> 	git worktree add --detach grumpy &&
> 	test_cmp hook.expect hook.actual
> '
> @@ -479,4 +488,14 @@ test_expect_success '"add --no-checkout" suppresses post-checkout hook' '
> 	test_path_is_missing hook.actual
> '
> 
> +test_expect_success '"add" within worktree invokes post-checkout hook' '
> +	post_checkout_hook &&
> +	{
> +		echo $_z40 $(git rev-parse HEAD) 1 &&
> +		echo $(pwd)/guppy
> +	} >hook.expect &&
> +	git -C gloopy worktree add --detach ../guppy &&
> +	test_cmp hook.expect hook.actual
> +'
> +
> test_done
> -- 
> 2.16.1.291.g4437f3f132
> 

Looks good but I think we are not quite there yet. It does not work
for bare repos. You can test this if you apply the following patch on
top of your changes. Or get the patch from here:
https://github.com/larsxschneider/git/commit/253130e65b37a2ef250e9c6369d292ec725e62e7.patch

Please note that also '"add" within worktree invokes post-checkout hook'
seems to fail with my extended test case.

Thanks,
Lars


diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cf0aaeaf88..0580b12d50 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -451,20 +451,41 @@ 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
+	test_when_finished "rm -f $1/hooks/post-checkout" &&
+	mkdir -p $1/hooks &&
+	write_script $1/hooks/post-checkout <<-\EOF
 	{
 		echo $*
-		git rev-parse --show-toplevel
+		git rev-parse --git-dir --show-toplevel
+		pwd
 	} >../hook.actual
 	EOF
 }
 
+test_expect_success '"add" invokes post-checkout hook (branch, bare)' '
+	rm -rf bare &&
+	git clone --bare . bare &&
+	{
+		echo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&
+		echo $(pwd)/bare/worktrees/wt-bare
+		echo $(pwd)/wt-bare
+		echo $(pwd)/wt-bare
+	} >hook.expect &&
+	post_checkout_hook bare &&
+	(
+		cd bare &&
+		git worktree add -b branch ../wt-bare master
+	) &&
+	test_cmp hook.expect hook.actual
+'
+
+
 test_expect_success '"add" invokes post-checkout hook (branch)' '
-	post_checkout_hook &&
+	post_checkout_hook .git &&
 	{
 		echo $_z40 $(git rev-parse HEAD) 1 &&
+		echo $(pwd)/.git/worktrees/gumby
+		echo $(pwd)/gumby
 		echo $(pwd)/gumby
 	} >hook.expect &&
 	git worktree add gumby &&
@@ -472,9 +493,11 @@ test_expect_success '"add" invokes post-checkout hook (branch)' '
 '
 
 test_expect_success '"add" invokes post-checkout hook (detached)' '
-	post_checkout_hook &&
+	post_checkout_hook .git &&
 	{
 		echo $_z40 $(git rev-parse HEAD) 1 &&
+		echo $(pwd)/.git/worktrees/grumpy
+		echo $(pwd)/grumpy
 		echo $(pwd)/grumpy
 	} >hook.expect &&
 	git worktree add --detach grumpy &&
@@ -482,16 +505,18 @@ test_expect_success '"add" invokes post-checkout hook (detached)' '
 '
 
 test_expect_success '"add --no-checkout" suppresses post-checkout hook' '
-	post_checkout_hook &&
+	post_checkout_hook .git &&
 	rm -f hook.actual &&
 	git worktree add --no-checkout gloopy &&
 	test_path_is_missing hook.actual
 '
 
 test_expect_success '"add" within worktree invokes post-checkout hook' '
-	post_checkout_hook &&
+	post_checkout_hook .git &&
 	{
 		echo $_z40 $(git rev-parse HEAD) 1 &&
+		echo $(pwd)/.git/worktrees/guppy
+		echo $(pwd)/guppy
 		echo $(pwd)/guppy
 	} >hook.expect &&
 	git -C gloopy worktree add --detach ../guppy &&

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

[Eric Sunshine]

On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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.

Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees

[Lars Schneider]

> On 12 Feb 2018, at 04:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> Git commands which run hooks do so at the top level of the worktree in
> which the command itself was invoked. However, the 'git worktree'
> command may need to run hooks within some other directory. For
> instance, when "git worktree add" runs the 'post-checkout' hook, the
> hook must be run within the newly-created worktree, not within the
> worktree from which "git worktree add" was invoked.
> 
> To support this case, add 'run-hook' overloads which allow the
> worktree directory to be specified.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> run-command.c | 23 +++++++++++++++++++++--
> run-command.h |  4 ++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 31fc5ea86e..0e3995bbf9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)
> 	return path.buf;
> }
> 
> -int run_hook_ve(const char *const *env, const char *name, va_list args)
> +int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,
> +		   va_list args)
> {
> 	struct child_process hook = CHILD_PROCESS_INIT;
> 	const char *p;
> @@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
> 	if (!p)
> 		return 0;
> 
> -	argv_array_push(&hook.args, p);
> +	argv_array_push(&hook.args, absolute_path(p));
> 	while ((p = va_arg(args, const char *)))
> 		argv_array_push(&hook.args, p);
> +	hook.dir = dir;
> 	hook.env = env;
> 	hook.no_stdin = 1;
> 	hook.stdout_to_stderr = 1;
> @@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
> 	return run_command(&hook);
> }
> 
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
> +{
> +	return run_hook_cd_ve(NULL, env, name, args);
> +}

I think we have only one more user for this function:
	builtin/commit.c:       ret = run_hook_ve(hook_env.argv,name, args);

The other function 'run_hook_le' is used in a few places:
	builtin/am.c:   ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL);
	builtin/am.c:   if (run_hook_le(NULL, "pre-applypatch", NULL))
	builtin/am.c:   run_hook_le(NULL, "post-applypatch", NULL);
	builtin/checkout.c:     return run_hook_le(NULL, "post-checkout",
	builtin/clone.c:        err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
	builtin/gc.c:   if (run_hook_le(NULL, "pre-auto-gc", NULL))
	builtin/merge.c:        run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
	builtin/receive-pack.c: if (run_hook_le(env->argv, push_to_checkout_hook,

Would it be an option to just use the new function signature
everywhere and remove the wrapper? Or do we value the old interface?

- Lars



> +
> +int run_hook_cd_le(const char *dir, const char *const *env, const char *name, ...)
> +{
> +	va_list args;
> +	int ret;
> +
> +	va_start(args, name);
> +	ret = run_hook_cd_ve(dir, env, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}
> +
> int run_hook_le(const char *const *env, const char *name, ...)
> {
> 	va_list args;
> diff --git a/run-command.h b/run-command.h
> index 3932420ec8..8beddffea8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -66,7 +66,11 @@ int run_command(struct child_process *);
> extern const char *find_hook(const char *name);
> LAST_ARG_MUST_BE_NULL
> extern int run_hook_le(const char *const *env, const char *name, ...);
> +extern int run_hook_cd_le(const char *dir, const char *const *env,
> +			  const char *name, ...);
> extern int run_hook_ve(const char *const *env, const char *name, va_list args);
> +extern int run_hook_cd_ve(const char *dir, const char *const *env,
> +			  const char *name, va_list args);
> 
> #define RUN_COMMAND_NO_STDIN 1
> #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
> -- 
> 2.16.1.291.g4437f3f132
> 

Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees

[Eric Sunshine]

On Mon, Feb 12, 2018 at 3:58 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>> On 12 Feb 2018, at 04:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +int run_hook_ve(const char *const *env, const char *name, va_list args)
>> +{
>> +     return run_hook_cd_ve(NULL, env, name, args);
>> +}
>
> I think we have only one more user for this function:
>         builtin/commit.c:       ret = run_hook_ve(hook_env.argv,name, args);
>
> Would it be an option to just use the new function signature
> everywhere and remove the wrapper? Or do we value the old interface?

I did note that there was only one existing caller and considered
simply modifying run_hook_ve()'s signature to accept the new 'dir'
argument. I don't feel strongly one way or the other.

However, as mentioned elsewhere[1], I'm still not convinced that this
one-off case of needing to chdir() warrants adding these overloads to
'run-command' at all, so I'm strongly considering (and was considering
even for v1) instead just running this hook manually in
builtin/worktree.c.

[1]: https://public-inbox.org/git/CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=Q@mail.gmail.com/

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

[Eric Sunshine]

On Mon, Feb 12, 2018 at 3:01 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>> On 12 Feb 2018, at 04:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.
>
> Looks good but I think we are not quite there yet. It does not work
> for bare repos. You can test this if you apply the following patch on
> top of your changes.

Thanks for providing a useful test case.

The problem is that Git itself exports GIT_DIR with value "." (which
makes sense since "git worktree add" is invoked within a bare repo)
and that GIT_DIR leaks into the hook's environment. However, since the
hook is running within the worktree, into which we've chdir()'d, the
relative "." is wrong, and setup.c:is_git_directory() (invoked
indirectly by setup_bare_git_dir()) correctly reports that the
worktree itself is not a valid Git directory. As a result, 'rev-parse'
run by the hook fails with "fatal: Not a git repository: '.'". The fix
is either to remove GIT_DIR from the environment or make it absolute
so the chdir() doesn't invalidate it.

However, it turns out that builtin/worktree.c already _does_ export
GIT_DIR and GIT_WORK_TREE for the commands it invokes ('update-ref',
'symbolic-ref', 'reset --hard') to create the new worktree, which
makes perfect sense since these commands need to know the location of
the new worktree. So, a second approach/fix is also to use these
existing exports when running the hook. The (minor) catch is that they
are relative and break upon chdir() but that is easily fixed by making
them absolute.

So, either approach works: removing GIT_DIR or using "worktree add"'s
existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is
consistent with how "worktree add" invokes other command already and,
especially, because it also addresses the issue Junio raised of
user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's
environment.

> Please note that also '"add" within worktree invokes post-checkout hook'
> seems to fail with my extended test case.

Also fixed by either approach.

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

[Eric Sunshine]

On Mon, Feb 12, 2018 at 11:42 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> So, either approach works: removing GIT_DIR or using "worktree add"'s
> existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is
> consistent with how "worktree add" invokes other command already and,
> especially, because it also addresses the issue Junio raised of
> user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's
> environment.

Just to be clear: Regardless of which fix is used, we still want to
chdir() to the new worktree to guarantee that the directory in which
the 'post-checkout' hook is run is predictable.

In the re-roll, I'm going with the latter approach of re-using
builtin/worktree.c's existing GIT_DIR/GIT_WORK_TREE which it already
exports to other commands it invokes.

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

[Johannes Sixt]

Am 12.02.2018 um 04:15 schrieb Eric Sunshine:
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -454,20 +454,29 @@ 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
> +	{
> +		echo $*
> +		git rev-parse --show-toplevel
> +	} >../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)/gumby

$(pwd) is here and in the other tests correct. $PWD would be wrong on 
Windows. Thanks for being considerate.

> +	} >hook.expect &&
>   	git worktree add gumby &&
>   	test_cmp hook.expect hook.actual

-- Hannes

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

[Eric Sunshine]

On Tue, Feb 13, 2018 at 2:27 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 12.02.2018 um 04:15 schrieb Eric Sunshine:
>> +               echo $_z40 $(git rev-parse HEAD) 1 &&
>> +               echo $(pwd)/gumby
>
> $(pwd) is here and in the other tests correct. $PWD would be wrong on
> Windows. Thanks for being considerate.

Thanks for the confirmation. I'm always a bit leery of using "pwd" in
tests due to Windows concerns; doubly so since I don't have a Windows
installation on which to test it.

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

[Eric Sunshine]

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).

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.

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. Thanks to Junio for his review
comments and to Lars for pointing out bare repository failure and for
providing a test case.

Changes since v1:

* Sanitize environment so user-assigned GIT_DIR and GIT_WORK_TREE don't
  confuse hook. (Junio)

* Fix failure of Git commands run by hook when "git worktree add" is
  invoked from within a bare repository. (Lars)

* Run hook manually in builtin/worktree.c rather than adding new
  'run_hook' overloads to 'run-command' which allow the directory to be
  set. This one-off case doesn't warrant adding 'run_hook' overloads at
  this time.

No interdiff since almost everything changed.

[1]: https://public-inbox.org/git/20180212031526.40039-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..604a0292b0 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" };
+			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
+	} >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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>  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
>  '

This seems to segfault on me, without leaving hook.actual anywhere.

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

[Eric Sunshine]

On Thu, Feb 15, 2018 at 12:52:11PM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >  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
> >  '
> 
> This seems to segfault on me, without leaving hook.actual anywhere.

I'm unable to reproduce the segfault, but I'm guessing it's because
I'm a dummy. Can you squash in the following and retry?

--- >8 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 604a0292b0..f69f862947 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -348,7 +348,7 @@ static int add_worktree(const char *path, const char *refname,
 	if (!ret && opts->checkout) {
 		const char *hook = find_hook("post-checkout");
 		if (hook) {
-			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE" };
+			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
 			cp.git_cmd = 0;
 			cp.no_stdin = 1;
 			cp.stdout_to_stderr = 1;
--- >8 ---

If that fixes it, can you squash it locally or should I re-send?

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I'm a dummy. Can you squash in the following and retry?

I haven't tried, but that does look like the right thing to do,
whether it is the root cause of the fault I am seeing.

Thanks.

>
> --- >8 ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 604a0292b0..f69f862947 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -348,7 +348,7 @@ static int add_worktree(const char *path, const char *refname,
>  	if (!ret && opts->checkout) {
>  		const char *hook = find_hook("post-checkout");
>  		if (hook) {
> -			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE" };
> +			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
>  			cp.git_cmd = 0;
>  			cp.no_stdin = 1;
>  			cp.stdout_to_stderr = 1;
> --- >8 ---
>
> If that fixes it, can you squash it locally or should I re-send?

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

[Eric Sunshine]

On Thu, Feb 15, 2018 at 4:27 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Feb 15, 2018 at 12:52:11PM -0800, Junio C Hamano wrote:
>> This seems to segfault on me, without leaving hook.actual anywhere.
>
> I'm unable to reproduce the segfault, but I'm guessing it's because
> I'm a dummy. Can you squash in the following and retry?
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> -                       const char *env[] = { "GIT_DIR", "GIT_WORK_TREE" };
> +                       const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
>
> If that fixes it, can you squash it locally or should I re-send?

Okay, I was able to reproduce the crash on FreeBSD (but not MacOS or
Linux), and the above change does indeed fix it. I'll send v3 in a
moment to address it.

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

[Eric Sunshine]

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).

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.

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
+	} >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

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

[Lars Schneider]

> 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
> 

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> ...
> 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.
>
> 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().

Thanks.  This matches what I had (v2 plus manual fixup while
queueing) and looks good.

As long as the "hand-rolled" implementation uses the same
find_hook() helper used in run_hook[lv]e, I do not think a one-off
invocation of the hook is not too bad, at least for now.

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

[Eric Sunshine]

On Fri, Feb 16, 2018 at 11:55 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>> On 16 Feb 2018, at 00:09, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.

A number of other places in the codebase run hooks manually, so this
is not unprecedented. Rather than adding 'run_hook' overload(s)
specific to this particular case, it would make sense to review all
such places and design the API of the new overloads to handle _all_
those cases (with the hope of avoiding adding new ad-hoc overloads
each time). But, that's outside the scope of this bug fix.

>> post_checkout_hook () {
>> +     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?

I do think it's a good idea, and it is still being tested but not in
quite the same way. I removed the explicit 'pwd' from the output
because I didn't want to deal with potential fallout on Windows. In
particular, your test used raw 'pwd' for the "actual" file but
'$(pwd)' for "expect", which I think would have run afoul on Windows
since '$(pwd)' is meant only to compare output of _Git_ commands,
whereas raw 'pwd' is not a Git command. So, I think the test would
have needed to use raw 'pwd' for the "expect" file, as well. But,
since I don't have Windows on which to test, I decided to avoid that
potential mess by checking 'pwd' in a different way. Details below.

>> +     } >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
>> '

The explicit 'pwd' check from your test is still here, but is now
implicit, so more subtle. Specifically, the hook now emits "actual"
within the current working directory (the location 'pwd' would
report), and 'test_cmp' looks for the "actual" file at that location.
The net result is 'pwd' is effectively, though implicitly, recorded by
the location of the "actual" file itself. If 'pwd' is wrong (that is,
if the chdir() was wrong or missing), then "actual" would not end up
at the correct location and the 'test_cmp' would fail.