{"thread":{"id":"47803","subject":"[PATCH v1] worktree: set worktree environment in post-checkout hook","startedAt":"2018-02-10T01:02:02Z","lastAt":"2018-02-16T18:27:40Z","messageCount":24,"participants":["lars.schneider@autodesk.com","Lars Schneider","Eric Sunshine","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338965","messageId":"20180210010132.33629-1-lars.schneider@autodesk.com","threadId":"47803","inReplyTo":null,"subject":"[PATCH v1] worktree: set worktree environment in post-checkout hook","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-10T01:01:32Z","receivedAt":"2018-02-10T01:02:02Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nIn ade546be47 (worktree: invoke post-checkout hook (unless\n--no-checkout), 2017-12-07) we taught Git to run the post-checkout hook\nin worktrees. Unfortunately, the environment of the hook was not made\naware of the worktree. Consequently, a 'git rev-parse --show-toplevel'\ncall in the post-checkout hook would return a wrong result.\n\nFix this by setting the 'GIT_WORK_TREE' environment variable to make\nGit calls within the post-checkout hook aware of the worktree.\n\nSigned-off-by: Lars Schneider \n---\n\nHi,\n\nI think this is a bug in Git 2.16. We noticed it because it caused a\nproblem in Git LFS [1]. The modified test case fails with Git 2.16 and\nsucceeds with this patch.\n\nCheers,\nLars\n\n\n[1] https://github.com/git-lfs/git-lfs/issues/2848\n\n\nNotes:\n Base Ref: v2.16.1\n Web-Diff: https://github.com/larsxschneider/git/commit/214e9342e7\n Checkout: git fetch https://github.com/larsxschneider/git fix-worktree-add-v1 && git checkout 214e9342e7\n\n builtin/worktree.c | 7 +++++--\n t/t2025-worktree-add.sh | 11 +++++++++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..032f9b86bf 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -345,9 +345,12 @@ static int add_worktree(const char *path, const char *refname,\n \t * Hook failure does not warrant worktree deletion, so run hook after\n \t * is_junk is cleared, but do return appropriate code when hook fails.\n \t */\n-\tif (!ret && opts->checkout)\n-\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n+\tif (!ret && opts->checkout) {\n+\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\t\targv_array_pushf(&env, \"GIT_WORK_TREE=%s\", absolute_path(path));\n+\t\tret = run_hook_le(env.argv, \"post-checkout\", oid_to_hex(&null_oid),\n \t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n+\t}\n\n \targv_array_clear(&child_env);\n \tstrbuf_release(&sb);\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 2b95944973..d022ac0c26 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -455,19 +455,26 @@ post_checkout_hook () {\n \tmkdir -p .git/hooks &&\n \twrite_script .git/hooks/post-checkout <<-\\EOF\n \techo $* >hook.actual\n+\tgit rev-parse --show-toplevel >>hook.actual\n \tEOF\n }\n\n test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\tcat >hook.expect <<-EOF &&\n+\t\t$_z40 $(git rev-parse HEAD) 1\n+\t\t$(pwd)/gumby\n+\tEOF\n \tgit worktree add gumby &&\n \ttest_cmp hook.expect hook.actual\n '\n\n test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\tcat >hook.expect <<-EOF &&\n+\t\t$_z40 $(git rev-parse HEAD) 1\n+\t\t$(pwd)/grumpy\n+\tEOF\n \tgit worktree add --detach grumpy &&\n \ttest_cmp hook.expect hook.actual\n '\n\nbase-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa\n--\n2.16.1\n\n"},{"id":"338968","messageId":"E0DC5330-F3D5-4945-A206-B583F83F0DFD@gmail.com","threadId":"47803","inReplyTo":"20180210010132.33629-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] worktree: set worktree environment in post-checkout hook","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-10T01:28:16Z","receivedAt":"2018-02-10T01:28:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Feb 2018, at 02:01, lars.schneider@autodesk.com wrote:\n> \n> From: Lars Schneider \n> \n> In ade546be47 (worktree: invoke post-checkout hook (unless\n> --no-checkout), 2017-12-07) we taught Git to run the post-checkout hook\n> in worktrees. Unfortunately, the environment of the hook was not made\n> aware of the worktree. Consequently, a 'git rev-parse --show-toplevel'\n> call in the post-checkout hook would return a wrong result.\n> \n> Fix this by setting the 'GIT_WORK_TREE' environment variable to make\n> Git calls within the post-checkout hook aware of the worktree.\n> \n> Signed-off-by: Lars Schneider \n> ---\n> \n> Hi,\n> \n> I think this is a bug in Git 2.16. We noticed it because it caused a\n> problem in Git LFS [1]. The modified test case fails with Git 2.16 and\n> succeeds with this patch.\n> \n> Cheers,\n> Lars\n> \n> \n> [1] https://github.com/git-lfs/git-lfs/issues/2848\n> \n> \n> Notes:\n> Base Ref: v2.16.1\n> Web-Diff: https://github.com/larsxschneider/git/commit/214e9342e7\n> Checkout: git fetch https://github.com/larsxschneider/git fix-worktree-add-v1 && git checkout 214e9342e7\n> \n> builtin/worktree.c | 7 +++++--\n> t/t2025-worktree-add.sh | 11 +++++++++--\n> 2 files changed, 14 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 7cef5b120b..032f9b86bf 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -345,9 +345,12 @@ static int add_worktree(const char *path, const char *refname,\n> \t * Hook failure does not warrant worktree deletion, so run hook after\n> \t * is_junk is cleared, but do return appropriate code when hook fails.\n> \t */\n> -\tif (!ret && opts->checkout)\n> -\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n> +\tif (!ret && opts->checkout) {\n> +\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n> +\t\targv_array_pushf(&env, \"GIT_WORK_TREE=%s\", absolute_path(path));\n> +\t\tret = run_hook_le(env.argv, \"post-checkout\", oid_to_hex(&null_oid),\n> \t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n\nAs I hit \"send\" I realized that I forgot to cleanup.\n@Junio: Can you squash this in?\n\n\targv_array_clear(&env);\n\nThanks,\nLars\n\n> +\t}\n> \n> \targv_array_clear(&child_env);\n> \tstrbuf_release(&sb);\n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index 2b95944973..d022ac0c26 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -455,19 +455,26 @@ post_checkout_hook () {\n> \tmkdir -p .git/hooks &&\n> \twrite_script .git/hooks/post-checkout <<-\\EOF\n> \techo $* >hook.actual\n> +\tgit rev-parse --show-toplevel >>hook.actual\n> \tEOF\n> }\n> \n> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\tcat >hook.expect <<-EOF &&\n> +\t\t$_z40 $(git rev-parse HEAD) 1\n> +\t\t$(pwd)/gumby\n> +\tEOF\n> \tgit worktree add gumby &&\n> \ttest_cmp hook.expect hook.actual\n> '\n> \n> test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\tcat >hook.expect <<-EOF &&\n> +\t\t$_z40 $(git rev-parse HEAD) 1\n> +\t\t$(pwd)/grumpy\n> +\tEOF\n> \tgit worktree add --detach grumpy &&\n> \ttest_cmp hook.expect hook.actual\n> '\n> \n> base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa\n> --\n> 2.16.1\n> \n\n"},{"id":"339031","messageId":"20180212031526.40039-1-sunshine@sunshineco.com","threadId":"47803","inReplyTo":"20180210010132.33629-1-lars.schneider@autodesk.com","subject":"[PATCH 0/2] worktree: change to new worktree dir before running hook(s)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T03:15:24Z","receivedAt":"2018-02-12T03:17:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This patch series replaces \"worktree: set worktree environment in\npost-checkout hook\"[1] from Lars, which is a proposed bug fix for\nade546be47 (worktree: invoke post-checkout hook, 2017-12-07).\n\nThe problem that patch addresses is that \"git worktree add\" does not\nprovide proper context to the invoked 'post-checkout' hook, so the hook\ndoesn't know where the newly-created worktree is. Lars's approach was to\nset GIT_WORK_TREE to point at the new worktree directory, however, doing\nso has a few drawbacks:\n\n1. GIT_WORK_TREE is normally assigned in conjunction with GIT_DIR; it is\n unusual and possibly problematic to set one but not the other.\n\n2. Assigning GIT_WORK_TREE unconditionally may lead to unforeseen\n interactions and problems with end-user scripts and aliases or even\n within Git itself. It seems better to avoid unconditional assignment\n rather than risk problems such as those described and worked around\n by 86d26f240f (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE\n when .., 2015-12-20)\n\n3. Assigning GIT_WORK_TREE is too specialized a solution; it \"fixes\"\n only Git commands run by the hook, but does nothing for other\n commands ('mv', 'cp', etc.) that the hook might invoke.\n\nThe real problem with ade546be47 is that it neglects to change to the\ndirectory of the newly-created worktree before running the hook, thus\nthe hook incorrectly runs in the directory in which \"git worktree add\"\nwas invoked. Rather than messing with GIT_WORK_TREE, this replacement\npatch series fixes the problem by ensuring that the directory is changed\nbefore the hook is invoked.\n\n[1]: https://public-inbox.org/git/20180210010132.33629-1-lars.schneider@autodesk.com/\n\nEric Sunshine (2):\n run-command: teach 'run_hook' about alternate worktrees\n worktree: add: change to new worktree directory before running hook\n\n builtin/worktree.c | 11 ++++++++---\n run-command.c | 23 +++++++++++++++++++++--\n run-command.h | 4 ++++\n t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---\n 4 files changed, 55 insertions(+), 8 deletions(-)\n\n-- \n2.16.1.291.g4437f3f132\n"},{"id":"339032","messageId":"20180212031526.40039-3-sunshine@sunshineco.com","threadId":"47803","inReplyTo":"20180212031526.40039-1-sunshine@sunshineco.com","subject":"[PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T03:15:26Z","receivedAt":"2018-02-12T03:17:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Although \"git worktree add\" learned to run the 'post-checkout' hook in\nade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\nneglects to change to the directory of the newly-created worktree\nbefore running the hook. Instead, the hook is run within the directory\nfrom which the \"git worktree add\" command itself was invoked, which\neffectively neuters the hook since it knows nothing about the new\nworktree directory.\n\nFix this by changing to the new worktree's directory before running\nthe hook, and adjust the tests to verify that the hook is indeed run\nwithin the correct directory.\n\nWhile at it, also add a test to verify that the hook is run within the\ncorrect directory even when the new worktree is created from a sibling\nworktree (as opposed to the main worktree).\n\nReported-by: Lars Schneider \nSigned-off-by: Eric Sunshine \n---\n builtin/worktree.c | 11 ++++++++---\n t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---\n 2 files changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..b55c55a26c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -345,9 +345,14 @@ static int add_worktree(const char *path, const char *refname,\n \t * Hook failure does not warrant worktree deletion, so run hook after\n \t * is_junk is cleared, but do return appropriate code when hook fails.\n \t */\n-\tif (!ret && opts->checkout)\n-\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n-\t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n+\tif (!ret && opts->checkout) {\n+\t\tchar *p = absolute_pathdup(path);\n+\t\tret = run_hook_cd_le(p, NULL, \"post-checkout\",\n+\t\t\t\t oid_to_hex(&null_oid),\n+\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t \"1\", NULL);\n+\t\tfree(p);\n+\t}\n \n \targv_array_clear(&child_env);\n \tstrbuf_release(&sb);\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 2b95944973..cf0aaeaf88 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -454,20 +454,29 @@ post_checkout_hook () {\n \ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n \tmkdir -p .git/hooks &&\n \twrite_script .git/hooks/post-checkout <<-\\EOF\n-\techo $* >hook.actual\n+\t{\n+\t\techo $*\n+\t\tgit rev-parse --show-toplevel\n+\t} >../hook.actual\n \tEOF\n }\n \n test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/gumby\n+\t} >hook.expect &&\n \tgit worktree add gumby &&\n \ttest_cmp hook.expect hook.actual\n '\n \n test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/grumpy\n+\t} >hook.expect &&\n \tgit worktree add --detach grumpy &&\n \ttest_cmp hook.expect hook.actual\n '\n@@ -479,4 +488,14 @@ test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n \ttest_path_is_missing hook.actual\n '\n \n+test_expect_success '\"add\" within worktree invokes post-checkout hook' '\n+\tpost_checkout_hook &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/guppy\n+\t} >hook.expect &&\n+\tgit -C gloopy worktree add --detach ../guppy &&\n+\ttest_cmp hook.expect hook.actual\n+'\n+\n test_done\n-- \n2.16.1.291.g4437f3f132\n\n"},{"id":"339033","messageId":"20180212031526.40039-2-sunshine@sunshineco.com","threadId":"47803","inReplyTo":"20180212031526.40039-1-sunshine@sunshineco.com","subject":"[PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T03:15:25Z","receivedAt":"2018-02-12T03:17:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Git commands which run hooks do so at the top level of the worktree in\nwhich the command itself was invoked. However, the 'git worktree'\ncommand may need to run hooks within some other directory. For\ninstance, when \"git worktree add\" runs the 'post-checkout' hook, the\nhook must be run within the newly-created worktree, not within the\nworktree from which \"git worktree add\" was invoked.\n\nTo support this case, add 'run-hook' overloads which allow the\nworktree directory to be specified.\n\nSigned-off-by: Eric Sunshine \n---\n run-command.c | 23 +++++++++++++++++++++--\n run-command.h | 4 ++++\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..0e3995bbf9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,\n+\t\t va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n@@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \tif (!p)\n \t\treturn 0;\n \n-\targv_array_push(&hook.args, p);\n+\targv_array_push(&hook.args, absolute_path(p));\n \twhile ((p = va_arg(args, const char *)))\n \t\targv_array_push(&hook.args, p);\n+\thook.dir = dir;\n \thook.env = env;\n \thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n@@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \treturn run_command(&hook);\n }\n \n+int run_hook_ve(const char *const *env, const char *name, va_list args)\n+{\n+\treturn run_hook_cd_ve(NULL, env, name, args);\n+}\n+\n+int run_hook_cd_le(const char *dir, const char *const *env, const char *name, ...)\n+{\n+\tva_list args;\n+\tint ret;\n+\n+\tva_start(args, name);\n+\tret = run_hook_cd_ve(dir, env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\n+\n int run_hook_le(const char *const *env, const char *name, ...)\n {\n \tva_list args;\ndiff --git a/run-command.h b/run-command.h\nindex 3932420ec8..8beddffea8 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -66,7 +66,11 @@ int run_command(struct child_process *);\n extern const char *find_hook(const char *name);\n LAST_ARG_MUST_BE_NULL\n extern int run_hook_le(const char *const *env, const char *name, ...);\n+extern int run_hook_cd_le(const char *dir, const char *const *env,\n+\t\t\t const char *name, ...);\n extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n+extern int run_hook_cd_ve(const char *dir, const char *const *env,\n+\t\t\t const char *name, va_list args);\n \n #define RUN_COMMAND_NO_STDIN 1\n #define RUN_GIT_CMD\t 2\t/*If this is to be git sub-command */\n-- \n2.16.1.291.g4437f3f132\n\n"},{"id":"339034","messageId":"CAPig+cQ7MRQVT-1yNF8t6RMHNrgMtYvzOToamaLO==ymxTtLYg@mail.gmail.com","threadId":"47803","inReplyTo":"20180210010132.33629-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] worktree: set worktree environment in post-checkout hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T03:27:27Z","receivedAt":"2018-02-12T03:27:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 9, 2018 at 8:01 PM, wrote:\n> In ade546be47 (worktree: invoke post-checkout hook (unless\n> --no-checkout), 2017-12-07) we taught Git to run the post-checkout hook\n> in worktrees. Unfortunately, the environment of the hook was not made\n> aware of the worktree. Consequently, a 'git rev-parse --show-toplevel'\n> call in the post-checkout hook would return a wrong result.\n>\n> Fix this by setting the 'GIT_WORK_TREE' environment variable to make\n> Git calls within the post-checkout hook aware of the worktree.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> I think this is a bug in Git 2.16. We noticed it because it caused a\n> problem in Git LFS [1]. The modified test case fails with Git 2.16 and\n> succeeds with this patch.\n\nThanks for reporting and diagnosing the problem.\n\nI have some concerns about this patch's fix of setting GIT_WORK_TREE\nunconditionally. In particular, such unconditional setting of\nGIT_WORK_TREE might cause unforeseen problems. Although the\ncircumstances may not be quite the same, but the tale told by\n86d26f240f (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE\nwhen .., 2015-12-20) makes me cautious.\n\nMore significantly, though, setting GIT_WORK_TREE seems too\nspecialized a solution. While it may \"fix\" Git commands invoked by the\nhook, it does nothing for other commands ('cp', 'mv', etc.) which the\nhook may employ.\n\nAs a review comment, I was going to suggest that you chdir() to the\nnew worktree directory instead of messing with GIT_WORK_TREE, but when\nI tested it myself before making the suggestion, I discovered that the\nissue is a bit more involved. The result is that I ended up posting a\npatch series[1] to replace this one, with what I believe is a more\ncorrect fix.\n\n[1]: https://public-inbox.org/git/20180212031526.40039-1-sunshine@sunshineco.com/\n"},{"id":"339097","messageId":"xmqq7erit2wo.fsf@gitster-ct.c.googlers.com","threadId":"47803","inReplyTo":"20180212031526.40039-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-12T19:37:43Z","receivedAt":"2018-02-12T19:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Although \"git worktree add\" learned to run the 'post-checkout' hook in\n> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\n> neglects to change to the directory of the newly-created worktree\n> before running the hook. Instead, the hook is run within the directory\n> from which the \"git worktree add\" command itself was invoked, which\n> effectively neuters the hook since it knows nothing about the new\n> worktree directory.\n>\n> Fix this by changing to the new worktree's directory before running\n> the hook, and adjust the tests to verify that the hook is indeed run\n> within the correct directory.\n\nI like the approach taken by this replacement better. Just to make\nsure I understand the basic idea, let me rephrase what these two\npatches are doing:\n\n - \"path\" that is made absolute in this step is where the new\n worktree is created, i.e. the top-level of the working tree in\n the new worktree. We chdir there and then run the hook script.\n\n - Even though we often see hooks executed inside .git/ directory,\n for post-checkout, the top-level of the working tree is the right\n place, as that is where the hook is run by \"git checkout\" (which\n does the \"cd to the toplevel\" thing upfront and then runs hooks\n without doing anything special) and \"git clone\" (which goes to\n the newly created repository's working tree by calling\n setup.c::setup_work_tree() before builtin/clone.c::checkout(),\n which may call post-checkout hook).\n \nI wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the\nenvironment, though. When a user with a funny configuration (where\nthese two environment variables are pointing at unusual places) uses\n\"git worktree add\" to create another worktree for the repository, it\nwould not be sufficient to chdir to defeat them that are appropriate\nfor the original, and not for the new, worktree, would it?\n"},{"id":"339099","messageId":"C2FFE6FB-4B3C-4246-9BCA-272EC874FA8B@gmail.com","threadId":"47803","inReplyTo":"20180212031526.40039-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-12T20:01:01Z","receivedAt":"2018-02-12T20:01:10Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:\n> \n> Although \"git worktree add\" learned to run the 'post-checkout' hook in\n> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\n> neglects to change to the directory of the newly-created worktree\n> before running the hook. Instead, the hook is run within the directory\n> from which the \"git worktree add\" command itself was invoked, which\n> effectively neuters the hook since it knows nothing about the new\n> worktree directory.\n> \n> Fix this by changing to the new worktree's directory before running\n> the hook, and adjust the tests to verify that the hook is indeed run\n> within the correct directory.\n> \n> While at it, also add a test to verify that the hook is run within the\n> correct directory even when the new worktree is created from a sibling\n> worktree (as opposed to the main worktree).\n> \n> Reported-by: Lars Schneider \n> Signed-off-by: Eric Sunshine \n> ---\n> builtin/worktree.c | 11 ++++++++---\n> t/t2025-worktree-add.sh | 25 ++++++++++++++++++++++---\n> 2 files changed, 30 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 7cef5b120b..b55c55a26c 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -345,9 +345,14 @@ static int add_worktree(const char *path, const char *refname,\n> \t * Hook failure does not warrant worktree deletion, so run hook after\n> \t * is_junk is cleared, but do return appropriate code when hook fails.\n> \t */\n> -\tif (!ret && opts->checkout)\n> -\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n> -\t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n> +\tif (!ret && opts->checkout) {\n> +\t\tchar *p = absolute_pathdup(path);\n> +\t\tret = run_hook_cd_le(p, NULL, \"post-checkout\",\n> +\t\t\t\t oid_to_hex(&null_oid),\n> +\t\t\t\t oid_to_hex(&commit->object.oid),\n> +\t\t\t\t \"1\", NULL);\n> +\t\tfree(p);\n> +\t}\n> \n> \targv_array_clear(&child_env);\n> \tstrbuf_release(&sb);\n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index 2b95944973..cf0aaeaf88 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -454,20 +454,29 @@ post_checkout_hook () {\n> \ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n> \tmkdir -p .git/hooks &&\n> \twrite_script .git/hooks/post-checkout <<-\\EOF\n> -\techo $* >hook.actual\n> +\t{\n> +\t\techo $*\n> +\t\tgit rev-parse --show-toplevel\n> +\t} >../hook.actual\n> \tEOF\n> }\n> \n> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/gumby\n> +\t} >hook.expect &&\n> \tgit worktree add gumby &&\n> \ttest_cmp hook.expect hook.actual\n> '\n> \n> test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/grumpy\n> +\t} >hook.expect &&\n> \tgit worktree add --detach grumpy &&\n> \ttest_cmp hook.expect hook.actual\n> '\n> @@ -479,4 +488,14 @@ test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n> \ttest_path_is_missing hook.actual\n> '\n> \n> +test_expect_success '\"add\" within worktree invokes post-checkout hook' '\n> +\tpost_checkout_hook &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/guppy\n> +\t} >hook.expect &&\n> +\tgit -C gloopy worktree add --detach ../guppy &&\n> +\ttest_cmp hook.expect hook.actual\n> +'\n> +\n> test_done\n> -- \n> 2.16.1.291.g4437f3f132\n> \n\nLooks good but I think we are not quite there yet. It does not work\nfor bare repos. You can test this if you apply the following patch on\ntop of your changes. Or get the patch from here:\nhttps://github.com/larsxschneider/git/commit/253130e65b37a2ef250e9c6369d292ec725e62e7.patch\n\nPlease note that also '\"add\" within worktree invokes post-checkout hook'\nseems to fail with my extended test case.\n\nThanks,\nLars\n\n\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex cf0aaeaf88..0580b12d50 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -451,20 +451,41 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '\n '\n \n post_checkout_hook () {\n-\ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n-\tmkdir -p .git/hooks &&\n-\twrite_script .git/hooks/post-checkout <<-\\EOF\n+\ttest_when_finished \"rm -f $1/hooks/post-checkout\" &&\n+\tmkdir -p $1/hooks &&\n+\twrite_script $1/hooks/post-checkout <<-\\EOF\n \t{\n \t\techo $*\n-\t\tgit rev-parse --show-toplevel\n+\t\tgit rev-parse --git-dir --show-toplevel\n+\t\tpwd\n \t} >../hook.actual\n \tEOF\n }\n \n+test_expect_success '\"add\" invokes post-checkout hook (branch, bare)' '\n+\trm -rf bare &&\n+\tgit clone --bare . bare &&\n+\t{\n+\t\techo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/bare/worktrees/wt-bare\n+\t\techo $(pwd)/wt-bare\n+\t\techo $(pwd)/wt-bare\n+\t} >hook.expect &&\n+\tpost_checkout_hook bare &&\n+\t(\n+\t\tcd bare &&\n+\t\tgit worktree add -b branch ../wt-bare master\n+\t) &&\n+\ttest_cmp hook.expect hook.actual\n+'\n+\n+\n test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n-\tpost_checkout_hook &&\n+\tpost_checkout_hook .git &&\n \t{\n \t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/gumby\n+\t\techo $(pwd)/gumby\n \t\techo $(pwd)/gumby\n \t} >hook.expect &&\n \tgit worktree add gumby &&\n@@ -472,9 +493,11 @@ test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n '\n \n test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n-\tpost_checkout_hook &&\n+\tpost_checkout_hook .git &&\n \t{\n \t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/grumpy\n+\t\techo $(pwd)/grumpy\n \t\techo $(pwd)/grumpy\n \t} >hook.expect &&\n \tgit worktree add --detach grumpy &&\n@@ -482,16 +505,18 @@ test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n '\n \n test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n-\tpost_checkout_hook &&\n+\tpost_checkout_hook .git &&\n \trm -f hook.actual &&\n \tgit worktree add --no-checkout gloopy &&\n \ttest_path_is_missing hook.actual\n '\n \n test_expect_success '\"add\" within worktree invokes post-checkout hook' '\n-\tpost_checkout_hook &&\n+\tpost_checkout_hook .git &&\n \t{\n \t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/guppy\n+\t\techo $(pwd)/guppy\n \t\techo $(pwd)/guppy\n \t} >hook.expect &&\n \tgit -C gloopy worktree add --detach ../guppy &&\n\n\n"},{"id":"339104","messageId":"CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=Q@mail.gmail.com","threadId":"47803","inReplyTo":"xmqq7erit2wo.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T20:31:28Z","receivedAt":"2018-02-12T20:31:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> Fix this by changing to the new worktree's directory before running\n>> the hook, and adjust the tests to verify that the hook is indeed run\n>> within the correct directory.\n>\n> I like the approach taken by this replacement better. Just to make\n> sure I understand the basic idea, let me rephrase what these two\n> patches are doing:\n>\n> - \"path\" that is made absolute in this step is where the new\n> worktree is created, i.e. the top-level of the working tree in\n> the new worktree. We chdir there and then run the hook script.\n\nSorry for misleading. The \"absolute path\" stuff in this patch is\nunnecessary; it's probably just left-over from Lars's proposal which\ndid need to make it absolute when setting GIT_WORK_TREE, and I likely\ndidn't think hard enough to realize that it doesn't need to be\nabsolute just for chdir(). I'll drop the unnecessary\nabsolute_pathdup() in the re-roll.\n\n(The hook path in patch 1/2, on the other hand, does need to be made\nabsolute since find_hook() returns a relative path before we've\nchdir()'d into the new worktree.)\n\n> - Even though we often see hooks executed inside .git/ directory,\n> for post-checkout, the top-level of the working tree is the right\n> place, as that is where the hook is run by \"git checkout\" [...]\n\nPatch 1/2's commit message is a bit sloppy in its description of this.\nI'll tighten it up in the re-roll.\n\nI'm also not fully convinced that these new overloads of run_hook_*()\nare warranted since it's hard to imagine any other case when they\nwould be useful. It may make sense just to have builtin/worktree.c run\nthe hook itself for this one-off case.\n\n> I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the\n> environment, though. When a user with a funny configuration (where\n> these two environment variables are pointing at unusual places) uses\n> \"git worktree add\" to create another worktree for the repository, it\n> would not be sufficient to chdir to defeat them that are appropriate\n> for the original, and not for the new, worktree, would it?\n\nGood point. I'll look into sanitizing the environment.\n"},{"id":"339108","messageId":"42C41062-D27D-4BB8-8D8D-9272D37FAE88@gmail.com","threadId":"47803","inReplyTo":"20180212031526.40039-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-12T20:58:50Z","receivedAt":"2018-02-12T20:58:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:\n> \n> Git commands which run hooks do so at the top level of the worktree in\n> which the command itself was invoked. However, the 'git worktree'\n> command may need to run hooks within some other directory. For\n> instance, when \"git worktree add\" runs the 'post-checkout' hook, the\n> hook must be run within the newly-created worktree, not within the\n> worktree from which \"git worktree add\" was invoked.\n> \n> To support this case, add 'run-hook' overloads which allow the\n> worktree directory to be specified.\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> run-command.c | 23 +++++++++++++++++++++--\n> run-command.h | 4 ++++\n> 2 files changed, 25 insertions(+), 2 deletions(-)\n> \n> diff --git a/run-command.c b/run-command.c\n> index 31fc5ea86e..0e3995bbf9 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)\n> \treturn path.buf;\n> }\n> \n> -int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,\n> +\t\t va_list args)\n> {\n> \tstruct child_process hook = CHILD_PROCESS_INIT;\n> \tconst char *p;\n> @@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n> \tif (!p)\n> \t\treturn 0;\n> \n> -\targv_array_push(&hook.args, p);\n> +\targv_array_push(&hook.args, absolute_path(p));\n> \twhile ((p = va_arg(args, const char *)))\n> \t\targv_array_push(&hook.args, p);\n> +\thook.dir = dir;\n> \thook.env = env;\n> \thook.no_stdin = 1;\n> \thook.stdout_to_stderr = 1;\n> @@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n> \treturn run_command(&hook);\n> }\n> \n> +int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +{\n> +\treturn run_hook_cd_ve(NULL, env, name, args);\n> +}\n\nI think we have only one more user for this function:\n\tbuiltin/commit.c: ret = run_hook_ve(hook_env.argv,name, args);\n\nThe other function 'run_hook_le' is used in a few places:\n\tbuiltin/am.c: ret = run_hook_le(NULL, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n\tbuiltin/am.c: if (run_hook_le(NULL, \"pre-applypatch\", NULL))\n\tbuiltin/am.c: run_hook_le(NULL, \"post-applypatch\", NULL);\n\tbuiltin/checkout.c: return run_hook_le(NULL, \"post-checkout\",\n\tbuiltin/clone.c: err |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n\tbuiltin/gc.c: if (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n\tbuiltin/merge.c: run_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n\tbuiltin/receive-pack.c: if (run_hook_le(env->argv, push_to_checkout_hook,\n\nWould it be an option to just use the new function signature\neverywhere and remove the wrapper? Or do we value the old interface?\n\n- Lars\n\n\n\n> +\n> +int run_hook_cd_le(const char *dir, const char *const *env, const char *name, ...)\n> +{\n> +\tva_list args;\n> +\tint ret;\n> +\n> +\tva_start(args, name);\n> +\tret = run_hook_cd_ve(dir, env, name, args);\n> +\tva_end(args);\n> +\n> +\treturn ret;\n> +}\n> +\n> int run_hook_le(const char *const *env, const char *name, ...)\n> {\n> \tva_list args;\n> diff --git a/run-command.h b/run-command.h\n> index 3932420ec8..8beddffea8 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -66,7 +66,11 @@ int run_command(struct child_process *);\n> extern const char *find_hook(const char *name);\n> LAST_ARG_MUST_BE_NULL\n> extern int run_hook_le(const char *const *env, const char *name, ...);\n> +extern int run_hook_cd_le(const char *dir, const char *const *env,\n> +\t\t\t const char *name, ...);\n> extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n> +extern int run_hook_cd_ve(const char *dir, const char *const *env,\n> +\t\t\t const char *name, va_list args);\n> \n> #define RUN_COMMAND_NO_STDIN 1\n> #define RUN_GIT_CMD\t 2\t/*If this is to be git sub-command */\n> -- \n> 2.16.1.291.g4437f3f132\n> \n\n"},{"id":"339120","messageId":"CAPig+cSbtATV8daCsDxXTfGPgGh-qrvoVzfBSB+WP3EuiDXNxQ@mail.gmail.com","threadId":"47803","inReplyTo":"42C41062-D27D-4BB8-8D8D-9272D37FAE88@gmail.com","subject":"Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-12T21:49:21Z","receivedAt":"2018-02-12T21:49:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 12, 2018 at 3:58 PM, Lars Schneider\n wrote:\n>> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:\n>> +int run_hook_ve(const char *const *env, const char *name, va_list args)\n>> +{\n>> + return run_hook_cd_ve(NULL, env, name, args);\n>> +}\n>\n> I think we have only one more user for this function:\n> builtin/commit.c: ret = run_hook_ve(hook_env.argv,name, args);\n>\n> Would it be an option to just use the new function signature\n> everywhere and remove the wrapper? Or do we value the old interface?\n\nI did note that there was only one existing caller and considered\nsimply modifying run_hook_ve()'s signature to accept the new 'dir'\nargument. I don't feel strongly one way or the other.\n\nHowever, as mentioned elsewhere[1], I'm still not convinced that this\none-off case of needing to chdir() warrants adding these overloads to\n'run-command' at all, so I'm strongly considering (and was considering\neven for v1) instead just running this hook manually in\nbuiltin/worktree.c.\n\n[1]: https://public-inbox.org/git/CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=Q@mail.gmail.com/\n"},{"id":"339167","messageId":"CAPig+cTLQ6h+stLLns-837hP0nNOpE3vwu8_ZeO2GoAaDs7buw@mail.gmail.com","threadId":"47803","inReplyTo":"C2FFE6FB-4B3C-4246-9BCA-272EC874FA8B@gmail.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-13T04:42:51Z","receivedAt":"2018-02-13T04:42:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 12, 2018 at 3:01 PM, Lars Schneider\n wrote:\n>> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:\n>> Fix this by changing to the new worktree's directory before running\n>> the hook, and adjust the tests to verify that the hook is indeed run\n>> within the correct directory.\n>\n> Looks good but I think we are not quite there yet. It does not work\n> for bare repos. You can test this if you apply the following patch on\n> top of your changes.\n\nThanks for providing a useful test case.\n\nThe problem is that Git itself exports GIT_DIR with value \".\" (which\nmakes sense since \"git worktree add\" is invoked within a bare repo)\nand that GIT_DIR leaks into the hook's environment. However, since the\nhook is running within the worktree, into which we've chdir()'d, the\nrelative \".\" is wrong, and setup.c:is_git_directory() (invoked\nindirectly by setup_bare_git_dir()) correctly reports that the\nworktree itself is not a valid Git directory. As a result, 'rev-parse'\nrun by the hook fails with \"fatal: Not a git repository: '.'\". The fix\nis either to remove GIT_DIR from the environment or make it absolute\nso the chdir() doesn't invalidate it.\n\nHowever, it turns out that builtin/worktree.c already _does_ export\nGIT_DIR and GIT_WORK_TREE for the commands it invokes ('update-ref',\n'symbolic-ref', 'reset --hard') to create the new worktree, which\nmakes perfect sense since these commands need to know the location of\nthe new worktree. So, a second approach/fix is also to use these\nexisting exports when running the hook. The (minor) catch is that they\nare relative and break upon chdir() but that is easily fixed by making\nthem absolute.\n\nSo, either approach works: removing GIT_DIR or using \"worktree add\"'s\nexisting GIT_DIR and GIT_WORK_TREE. I favor the latter since it is\nconsistent with how \"worktree add\" invokes other command already and,\nespecially, because it also addresses the issue Junio raised of\nuser-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's\nenvironment.\n\n> Please note that also '\"add\" within worktree invokes post-checkout hook'\n> seems to fail with my extended test case.\n\nAlso fixed by either approach.\n"},{"id":"339168","messageId":"CAPig+cT+SkmFKBFc3rbh7SMk10dU8E-y4s+WR10GPNtGk+7S7g@mail.gmail.com","threadId":"47803","inReplyTo":"CAPig+cTLQ6h+stLLns-837hP0nNOpE3vwu8_ZeO2GoAaDs7buw@mail.gmail.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-13T04:48:41Z","receivedAt":"2018-02-13T04:48:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 12, 2018 at 11:42 PM, Eric Sunshine wrote:\n> So, either approach works: removing GIT_DIR or using \"worktree add\"'s\n> existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is\n> consistent with how \"worktree add\" invokes other command already and,\n> especially, because it also addresses the issue Junio raised of\n> user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's\n> environment.\n\nJust to be clear: Regardless of which fix is used, we still want to\nchdir() to the new worktree to guarantee that the directory in which\nthe 'post-checkout' hook is run is predictable.\n\nIn the re-roll, I'm going with the latter approach of re-using\nbuiltin/worktree.c's existing GIT_DIR/GIT_WORK_TREE which it already\nexports to other commands it invokes.\n"},{"id":"339172","messageId":"b24e2f26-d5e4-d17d-04d1-1bd12eaf9faa@kdbg.org","threadId":"47803","inReplyTo":"20180212031526.40039-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-02-13T07:27:17Z","receivedAt":"2018-02-13T07:27:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.02.2018 um 04:15 schrieb Eric Sunshine:\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -454,20 +454,29 @@ post_checkout_hook () {\n> \ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n> \tmkdir -p .git/hooks &&\n> \twrite_script .git/hooks/post-checkout <<-\\EOF\n> -\techo $* >hook.actual\n> +\t{\n> +\t\techo $*\n> +\t\tgit rev-parse --show-toplevel\n> +\t} >../hook.actual\n> \tEOF\n> }\n> \n> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/gumby\n\n$(pwd) is here and in the other tests correct. $PWD would be wrong on \nWindows. Thanks for being considerate.\n\n> +\t} >hook.expect &&\n> \tgit worktree add gumby &&\n> \ttest_cmp hook.expect hook.actual\n\n-- Hannes\n"},{"id":"339173","messageId":"CAPig+cQH+a0UwN9Kqen0gmWbTwZZWTVtsPjDKqzw+U7t2Uk-LA@mail.gmail.com","threadId":"47803","inReplyTo":"b24e2f26-d5e4-d17d-04d1-1bd12eaf9faa@kdbg.org","subject":"Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-13T07:34:33Z","receivedAt":"2018-02-13T07:34:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 13, 2018 at 2:27 AM, Johannes Sixt wrote:\n> Am 12.02.2018 um 04:15 schrieb Eric Sunshine:\n>> + echo $_z40 $(git rev-parse HEAD) 1 &&\n>> + echo $(pwd)/gumby\n>\n> $(pwd) is here and in the other tests correct. $PWD would be wrong on\n> Windows. Thanks for being considerate.\n\nThanks for the confirmation. I'm always a bit leery of using \"pwd\" in\ntests due to Windows concerns; doubly so since I don't have a Windows\ninstallation on which to test it.\n"},{"id":"339496","messageId":"20180215191841.40848-1-sunshine@sunshineco.com","threadId":"47803","inReplyTo":"20180212031526.40039-1-sunshine@sunshineco.com","subject":"[PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-15T19:18:41Z","receivedAt":"2018-02-15T19:19:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Although \"git worktree add\" learned to run the 'post-checkout' hook in\nade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\nneglected to change to the directory of the newly-created worktree\nbefore running the hook. Instead, the hook runs within the directory\nfrom which the \"git worktree add\" command itself was invoked, which\neffectively neuters the hook since it knows nothing about the new\nworktree directory.\n\nFurther, ade546be47 failed to sanitize the environment before running\nthe hook, which means that user-assigned values of GIT_DIR and\nGIT_WORK_TREE could mislead the hook about the location of the new\nworktree. In the case of \"git worktree add\" being run from a bare\nrepository, the GIT_DIR=\".\" assigned by Git itself leaks into the hook's\nenvironment and breaks Git commands; this is so even when the working\ndirectory is correctly changed to the new worktree before the hook runs\nsince \".\", relative to the new worktree directory, does not point at the\nbare repository.\n\nFix these problems by (1) changing to the new worktree's directory\nbefore running the hook, and (2) sanitizing the environment of GIT_DIR\nand GIT_WORK_TREE so hooks can't be confused by misleading values.\n\nEnhance the t2025 'post-checkout' tests to verify that the hook is\nindeed run within the correct directory and that Git commands invoked by\nthe hook compute Git-dir and top-level worktree locations correctly.\n\nWhile at it, also add two new tests: (1) verify that the hook is run\nwithin the correct directory even when the new worktree is created from\na sibling worktree (as opposed to the main worktree); (2) verify that\nthe hook is provided with correct context when the new worktree is\ncreated from a bare repository (test provided by Lars Schneider).\n\nImplementation Notes:\n\nRather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an\nalternative would be to set them explicitly, as is already done for\nother Git commands run internally by \"git worktree add\". This patch opts\ninstead to sanitize the environment in order to clearly document that\nthe worktree is fully functional by the time the hook is run, thus does\nnot require special environmental overrides.\n\nThe hook is run manually, rather than via run_hook_le(), since it needs\nto change the working directory to that of the worktree, and\nrun_hook_le() does not provide such functionality. As this is a one-off\ncase, adding 'run_hook' overloads which allow the directory to be set\ndoes not seem warranted at this time.\n\nReported-by: Lars Schneider \nSigned-off-by: Eric Sunshine \n---\n\nThis is a re-roll of [1] which fixes \"git worktree add\" to provide\nproper context to the 'post-checkout' hook so that the hook knows the\nlocation of the newly-created worktree. Thanks to Junio for his review\ncomments and to Lars for pointing out bare repository failure and for\nproviding a test case.\n\nChanges since v1:\n\n* Sanitize environment so user-assigned GIT_DIR and GIT_WORK_TREE don't\n confuse hook. (Junio)\n\n* Fix failure of Git commands run by hook when \"git worktree add\" is\n invoked from within a bare repository. (Lars)\n\n* Run hook manually in builtin/worktree.c rather than adding new\n 'run_hook' overloads to 'run-command' which allow the directory to be\n set. This one-off case doesn't warrant adding 'run_hook' overloads at\n this time.\n\nNo interdiff since almost everything changed.\n\n[1]: https://public-inbox.org/git/20180212031526.40039-1-sunshine@sunshineco.com/\n\nbuiltin/worktree.c | 20 ++++++++++++---\n t/t2025-worktree-add.sh | 54 ++++++++++++++++++++++++++++++++++-------\n 2 files changed, 62 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..604a0292b0 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -345,9 +345,23 @@ static int add_worktree(const char *path, const char *refname,\n \t * Hook failure does not warrant worktree deletion, so run hook after\n \t * is_junk is cleared, but do return appropriate code when hook fails.\n \t */\n-\tif (!ret && opts->checkout)\n-\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n-\t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n+\tif (!ret && opts->checkout) {\n+\t\tconst char *hook = find_hook(\"post-checkout\");\n+\t\tif (hook) {\n+\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\" };\n+\t\t\tcp.git_cmd = 0;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.stdout_to_stderr = 1;\n+\t\t\tcp.dir = path;\n+\t\t\tcp.env = env;\n+\t\t\tcp.argv = NULL;\n+\t\t\targv_array_pushl(&cp.args, absolute_path(hook),\n+\t\t\t\t\t oid_to_hex(&null_oid),\n+\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t \"1\", NULL);\n+\t\t\tret = run_command(&cp);\n+\t\t}\n+\t}\n \n \targv_array_clear(&child_env);\n \tstrbuf_release(&sb);\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 2b95944973..d0d2e4f7ec 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -451,32 +451,68 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '\n '\n \n post_checkout_hook () {\n-\ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n-\tmkdir -p .git/hooks &&\n-\twrite_script .git/hooks/post-checkout <<-\\EOF\n-\techo $* >hook.actual\n+\tgitdir=${1:-.git}\n+\ttest_when_finished \"rm -f $gitdir/hooks/post-checkout\" &&\n+\tmkdir -p $gitdir/hooks &&\n+\twrite_script $gitdir/hooks/post-checkout <<-\\EOF\n+\t{\n+\t\techo $*\n+\t\tgit rev-parse --git-dir --show-toplevel\n+\t} >hook.actual\n \tEOF\n }\n \n test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/gumby &&\n+\t\techo $(pwd)/gumby\n+\t} >hook.expect &&\n \tgit worktree add gumby &&\n-\ttest_cmp hook.expect hook.actual\n+\ttest_cmp hook.expect gumby/hook.actual\n '\n \n test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/grumpy &&\n+\t\techo $(pwd)/grumpy\n+\t} >hook.expect &&\n \tgit worktree add --detach grumpy &&\n-\ttest_cmp hook.expect hook.actual\n+\ttest_cmp hook.expect grumpy/hook.actual\n '\n \n test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n \tpost_checkout_hook &&\n \trm -f hook.actual &&\n \tgit worktree add --no-checkout gloopy &&\n-\ttest_path_is_missing hook.actual\n+\ttest_path_is_missing gloopy/hook.actual\n+'\n+\n+test_expect_success '\"add\" in other worktree invokes post-checkout hook' '\n+\tpost_checkout_hook &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/guppy &&\n+\t\techo $(pwd)/guppy\n+\t} >hook.expect &&\n+\tgit -C gloopy worktree add --detach ../guppy &&\n+\ttest_cmp hook.expect guppy/hook.actual\n+'\n+\n+test_expect_success '\"add\" in bare repo invokes post-checkout hook' '\n+\trm -rf bare &&\n+\tgit clone --bare . bare &&\n+\t{\n+\t\techo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/bare/worktrees/goozy &&\n+\t\techo $(pwd)/goozy\n+\t} >hook.expect &&\n+\tpost_checkout_hook bare &&\n+\tgit -C bare worktree add --detach ../goozy &&\n+\ttest_cmp hook.expect goozy/hook.actual\n '\n \n test_done\n-- \n2.16.1.370.g5c508858fb\n"},{"id":"339504","messageId":"xmqqh8qi7z7o.fsf@gitster-ct.c.googlers.com","threadId":"47803","inReplyTo":"20180215191841.40848-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-15T20:52:11Z","receivedAt":"2018-02-15T20:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/.git/worktrees/gumby &&\n> +\t\techo $(pwd)/gumby\n> +\t} >hook.expect &&\n> \tgit worktree add gumby &&\n> -\ttest_cmp hook.expect hook.actual\n> +\ttest_cmp hook.expect gumby/hook.actual\n> '\n\nThis seems to segfault on me, without leaving hook.actual anywhere.\n\n"},{"id":"339507","messageId":"20180215212751.GA42108@flurp.local","threadId":"47803","inReplyTo":"xmqqh8qi7z7o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-15T21:27:51Z","receivedAt":"2018-02-15T21:28:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 15, 2018 at 12:52:11PM -0800, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> > \tpost_checkout_hook &&\n> > -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> > +\t{\n> > +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> > +\t\techo $(pwd)/.git/worktrees/gumby &&\n> > +\t\techo $(pwd)/gumby\n> > +\t} >hook.expect &&\n> > \tgit worktree add gumby &&\n> > -\ttest_cmp hook.expect hook.actual\n> > +\ttest_cmp hook.expect gumby/hook.actual\n> > '\n> \n> This seems to segfault on me, without leaving hook.actual anywhere.\n\nI'm unable to reproduce the segfault, but I'm guessing it's because\nI'm a dummy. Can you squash in the following and retry?\n\n--- >8 ---\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 604a0292b0..f69f862947 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -348,7 +348,7 @@ static int add_worktree(const char *path, const char *refname,\n \tif (!ret && opts->checkout) {\n \t\tconst char *hook = find_hook(\"post-checkout\");\n \t\tif (hook) {\n-\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\" };\n+\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n \t\t\tcp.git_cmd = 0;\n \t\t\tcp.no_stdin = 1;\n \t\t\tcp.stdout_to_stderr = 1;\n--- >8 ---\n\nIf that fixes it, can you squash it locally or should I re-send?\n"},{"id":"339509","messageId":"xmqq8tbu7x64.fsf@gitster-ct.c.googlers.com","threadId":"47803","inReplyTo":"20180215212751.GA42108@flurp.local","subject":"Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-15T21:36:19Z","receivedAt":"2018-02-15T21:36:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I'm a dummy. Can you squash in the following and retry?\n\nI haven't tried, but that does look like the right thing to do,\nwhether it is the root cause of the fault I am seeing.\n\nThanks.\n\n>\n> --- >8 ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 604a0292b0..f69f862947 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -348,7 +348,7 @@ static int add_worktree(const char *path, const char *refname,\n> \tif (!ret && opts->checkout) {\n> \t\tconst char *hook = find_hook(\"post-checkout\");\n> \t\tif (hook) {\n> -\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\" };\n> +\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n> \t\t\tcp.git_cmd = 0;\n> \t\t\tcp.no_stdin = 1;\n> \t\t\tcp.stdout_to_stderr = 1;\n> --- >8 ---\n>\n> If that fixes it, can you squash it locally or should I re-send?\n"},{"id":"339521","messageId":"CAPig+cTjGygRjy8Yagdd=U=5j7PqHqunbYVvm0dkq23QjLOaOQ@mail.gmail.com","threadId":"47803","inReplyTo":"20180215212751.GA42108@flurp.local","subject":"Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-15T23:09:48Z","receivedAt":"2018-02-15T23:09:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 15, 2018 at 4:27 PM, Eric Sunshine wrote:\n> On Thu, Feb 15, 2018 at 12:52:11PM -0800, Junio C Hamano wrote:\n>> This seems to segfault on me, without leaving hook.actual anywhere.\n>\n> I'm unable to reproduce the segfault, but I'm guessing it's because\n> I'm a dummy. Can you squash in the following and retry?\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> - const char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\" };\n> + const char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n>\n> If that fixes it, can you squash it locally or should I re-send?\n\nOkay, I was able to reproduce the crash on FreeBSD (but not MacOS or\nLinux), and the above change does indeed fix it. I'll send v3 in a\nmoment to address it.\n"},{"id":"339522","messageId":"20180215230952.51887-1-sunshine@sunshineco.com","threadId":"47803","inReplyTo":"20180215191841.40848-1-sunshine@sunshineco.com","subject":"[PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-15T23:09:52Z","receivedAt":"2018-02-15T23:10:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Although \"git worktree add\" learned to run the 'post-checkout' hook in\nade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\nneglected to change to the directory of the newly-created worktree\nbefore running the hook. Instead, the hook runs within the directory\nfrom which the \"git worktree add\" command itself was invoked, which\neffectively neuters the hook since it knows nothing about the new\nworktree directory.\n\nFurther, ade546be47 failed to sanitize the environment before running\nthe hook, which means that user-assigned values of GIT_DIR and\nGIT_WORK_TREE could mislead the hook about the location of the new\nworktree. In the case of \"git worktree add\" being run from a bare\nrepository, the GIT_DIR=\".\" assigned by Git itself leaks into the hook's\nenvironment and breaks Git commands; this is so even when the working\ndirectory is correctly changed to the new worktree before the hook runs\nsince \".\", relative to the new worktree directory, does not point at the\nbare repository.\n\nFix these problems by (1) changing to the new worktree's directory\nbefore running the hook, and (2) sanitizing the environment of GIT_DIR\nand GIT_WORK_TREE so hooks can't be confused by misleading values.\n\nEnhance the t2025 'post-checkout' tests to verify that the hook is\nindeed run within the correct directory and that Git commands invoked by\nthe hook compute Git-dir and top-level worktree locations correctly.\n\nWhile at it, also add two new tests: (1) verify that the hook is run\nwithin the correct directory even when the new worktree is created from\na sibling worktree (as opposed to the main worktree); (2) verify that\nthe hook is provided with correct context when the new worktree is\ncreated from a bare repository (test provided by Lars Schneider).\n\nImplementation Notes:\n\nRather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an\nalternative would be to set them explicitly, as is already done for\nother Git commands run internally by \"git worktree add\". This patch opts\ninstead to sanitize the environment in order to clearly document that\nthe worktree is fully functional by the time the hook is run, thus does\nnot require special environmental overrides.\n\nThe hook is run manually, rather than via run_hook_le(), since it needs\nto change the working directory to that of the worktree, and\nrun_hook_le() does not provide such functionality. As this is a one-off\ncase, adding 'run_hook' overloads which allow the directory to be set\ndoes not seem warranted at this time.\n\nReported-by: Lars Schneider \nSigned-off-by: Eric Sunshine \n---\n\nThis is a re-roll of [1] which fixes \"git worktree add\" to provide\nproper context to the 'post-checkout' hook so that the hook knows the\nlocation of the newly-created worktree.\n\nChanges since v2:\n\n* Fix crash due to missing NULL-terminator on 'env' list passed to\n run_command().\n\n[1]: https://public-inbox.org/git/20180215191841.40848-1-sunshine@sunshineco.com/\n\nbuiltin/worktree.c | 20 ++++++++++++---\n t/t2025-worktree-add.sh | 54 ++++++++++++++++++++++++++++++++++-------\n 2 files changed, 62 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..f69f862947 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -345,9 +345,23 @@ static int add_worktree(const char *path, const char *refname,\n \t * Hook failure does not warrant worktree deletion, so run hook after\n \t * is_junk is cleared, but do return appropriate code when hook fails.\n \t */\n-\tif (!ret && opts->checkout)\n-\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n-\t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n+\tif (!ret && opts->checkout) {\n+\t\tconst char *hook = find_hook(\"post-checkout\");\n+\t\tif (hook) {\n+\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n+\t\t\tcp.git_cmd = 0;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.stdout_to_stderr = 1;\n+\t\t\tcp.dir = path;\n+\t\t\tcp.env = env;\n+\t\t\tcp.argv = NULL;\n+\t\t\targv_array_pushl(&cp.args, absolute_path(hook),\n+\t\t\t\t\t oid_to_hex(&null_oid),\n+\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t \"1\", NULL);\n+\t\t\tret = run_command(&cp);\n+\t\t}\n+\t}\n \n \targv_array_clear(&child_env);\n \tstrbuf_release(&sb);\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 2b95944973..d0d2e4f7ec 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -451,32 +451,68 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '\n '\n \n post_checkout_hook () {\n-\ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n-\tmkdir -p .git/hooks &&\n-\twrite_script .git/hooks/post-checkout <<-\\EOF\n-\techo $* >hook.actual\n+\tgitdir=${1:-.git}\n+\ttest_when_finished \"rm -f $gitdir/hooks/post-checkout\" &&\n+\tmkdir -p $gitdir/hooks &&\n+\twrite_script $gitdir/hooks/post-checkout <<-\\EOF\n+\t{\n+\t\techo $*\n+\t\tgit rev-parse --git-dir --show-toplevel\n+\t} >hook.actual\n \tEOF\n }\n \n test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/gumby &&\n+\t\techo $(pwd)/gumby\n+\t} >hook.expect &&\n \tgit worktree add gumby &&\n-\ttest_cmp hook.expect hook.actual\n+\ttest_cmp hook.expect gumby/hook.actual\n '\n \n test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n \tpost_checkout_hook &&\n-\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/grumpy &&\n+\t\techo $(pwd)/grumpy\n+\t} >hook.expect &&\n \tgit worktree add --detach grumpy &&\n-\ttest_cmp hook.expect hook.actual\n+\ttest_cmp hook.expect grumpy/hook.actual\n '\n \n test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n \tpost_checkout_hook &&\n \trm -f hook.actual &&\n \tgit worktree add --no-checkout gloopy &&\n-\ttest_path_is_missing hook.actual\n+\ttest_path_is_missing gloopy/hook.actual\n+'\n+\n+test_expect_success '\"add\" in other worktree invokes post-checkout hook' '\n+\tpost_checkout_hook &&\n+\t{\n+\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/.git/worktrees/guppy &&\n+\t\techo $(pwd)/guppy\n+\t} >hook.expect &&\n+\tgit -C gloopy worktree add --detach ../guppy &&\n+\ttest_cmp hook.expect guppy/hook.actual\n+'\n+\n+test_expect_success '\"add\" in bare repo invokes post-checkout hook' '\n+\trm -rf bare &&\n+\tgit clone --bare . bare &&\n+\t{\n+\t\techo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&\n+\t\techo $(pwd)/bare/worktrees/goozy &&\n+\t\techo $(pwd)/goozy\n+\t} >hook.expect &&\n+\tpost_checkout_hook bare &&\n+\tgit -C bare worktree add --detach ../goozy &&\n+\ttest_cmp hook.expect goozy/hook.actual\n '\n \n test_done\n-- \n2.16.1.370.g5c508858fb\n\n"},{"id":"339540","messageId":"E978DDBD-AD31-41EC-969B-E6AAC7D4FAF3@gmail.com","threadId":"47803","inReplyTo":"20180215230952.51887-1-sunshine@sunshineco.com","subject":"Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-16T16:55:02Z","receivedAt":"2018-02-16T16:55:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 16 Feb 2018, at 00:09, Eric Sunshine wrote:\n> \n> Although \"git worktree add\" learned to run the 'post-checkout' hook in\n> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it\n> neglected to change to the directory of the newly-created worktree\n> before running the hook. Instead, the hook runs within the directory\n> from which the \"git worktree add\" command itself was invoked, which\n> effectively neuters the hook since it knows nothing about the new\n> worktree directory.\n> \n> Further, ade546be47 failed to sanitize the environment before running\n> the hook, which means that user-assigned values of GIT_DIR and\n> GIT_WORK_TREE could mislead the hook about the location of the new\n> worktree. In the case of \"git worktree add\" being run from a bare\n> repository, the GIT_DIR=\".\" assigned by Git itself leaks into the hook's\n> environment and breaks Git commands; this is so even when the working\n> directory is correctly changed to the new worktree before the hook runs\n> since \".\", relative to the new worktree directory, does not point at the\n> bare repository.\n> \n> Fix these problems by (1) changing to the new worktree's directory\n> before running the hook, and (2) sanitizing the environment of GIT_DIR\n> and GIT_WORK_TREE so hooks can't be confused by misleading values.\n> \n> Enhance the t2025 'post-checkout' tests to verify that the hook is\n> indeed run within the correct directory and that Git commands invoked by\n> the hook compute Git-dir and top-level worktree locations correctly.\n> \n> While at it, also add two new tests: (1) verify that the hook is run\n> within the correct directory even when the new worktree is created from\n> a sibling worktree (as opposed to the main worktree); (2) verify that\n> the hook is provided with correct context when the new worktree is\n> created from a bare repository (test provided by Lars Schneider).\n\nThanks! This patch works great and fixes the problem!\nMore comments below.\n\n\n> Implementation Notes:\n> \n> Rather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an\n> alternative would be to set them explicitly, as is already done for\n> other Git commands run internally by \"git worktree add\". This patch opts\n> instead to sanitize the environment in order to clearly document that\n> the worktree is fully functional by the time the hook is run, thus does\n> not require special environmental overrides.\n> \n> The hook is run manually, rather than via run_hook_le(), since it needs\n> to change the working directory to that of the worktree, and\n> run_hook_le() does not provide such functionality. As this is a one-off\n> case, adding 'run_hook' overloads which allow the directory to be set\n> does not seem warranted at this time.\n\nAlthough this is an one-off case, I would still prefer it if all hook \ninvocations would happen in a central place to avoid future surprises.\n\n\n> Reported-by: Lars Schneider \n> Signed-off-by: Eric Sunshine \n> ---\n> \n> This is a re-roll of [1] which fixes \"git worktree add\" to provide\n> proper context to the 'post-checkout' hook so that the hook knows the\n> location of the newly-created worktree.\n> \n> Changes since v2:\n> \n> * Fix crash due to missing NULL-terminator on 'env' list passed to\n> run_command().\n> \n> [1]: https://public-inbox.org/git/20180215191841.40848-1-sunshine@sunshineco.com/\n> \n> builtin/worktree.c | 20 ++++++++++++---\n> t/t2025-worktree-add.sh | 54 ++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 62 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 7cef5b120b..f69f862947 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -345,9 +345,23 @@ static int add_worktree(const char *path, const char *refname,\n> \t * Hook failure does not warrant worktree deletion, so run hook after\n> \t * is_junk is cleared, but do return appropriate code when hook fails.\n> \t */\n> -\tif (!ret && opts->checkout)\n> -\t\tret = run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n> -\t\t\t\t oid_to_hex(&commit->object.oid), \"1\", NULL);\n> +\tif (!ret && opts->checkout) {\n> +\t\tconst char *hook = find_hook(\"post-checkout\");\n> +\t\tif (hook) {\n> +\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n> +\t\t\tcp.git_cmd = 0;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.stdout_to_stderr = 1;\n> +\t\t\tcp.dir = path;\n> +\t\t\tcp.env = env;\n> +\t\t\tcp.argv = NULL;\n> +\t\t\targv_array_pushl(&cp.args, absolute_path(hook),\n> +\t\t\t\t\t oid_to_hex(&null_oid),\n> +\t\t\t\t\t oid_to_hex(&commit->object.oid),\n> +\t\t\t\t\t \"1\", NULL);\n> +\t\t\tret = run_command(&cp);\n> +\t\t}\n> +\t}\n> \n> \targv_array_clear(&child_env);\n> \tstrbuf_release(&sb);\n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index 2b95944973..d0d2e4f7ec 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -451,32 +451,68 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '\n> '\n> \n> post_checkout_hook () {\n> -\ttest_when_finished \"rm -f .git/hooks/post-checkout\" &&\n> -\tmkdir -p .git/hooks &&\n> -\twrite_script .git/hooks/post-checkout <<-\\EOF\n> -\techo $* >hook.actual\n> +\tgitdir=${1:-.git}\n> +\ttest_when_finished \"rm -f $gitdir/hooks/post-checkout\" &&\n> +\tmkdir -p $gitdir/hooks &&\n> +\twrite_script $gitdir/hooks/post-checkout <<-\\EOF\n> +\t{\n> +\t\techo $*\n> +\t\tgit rev-parse --git-dir --show-toplevel\n\nI also checked `pwd` here in my suggested test case.\nI assume you think this check is not necessary?\n\n\n> +\t} >hook.actual\n> \tEOF\n> }\n> \n> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/.git/worktrees/gumby &&\n> +\t\techo $(pwd)/gumby\n> +\t} >hook.expect &&\n> \tgit worktree add gumby &&\n> -\ttest_cmp hook.expect hook.actual\n> +\ttest_cmp hook.expect gumby/hook.actual\n> '\n> \n> test_expect_success '\"add\" invokes post-checkout hook (detached)' '\n> \tpost_checkout_hook &&\n> -\tprintf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/.git/worktrees/grumpy &&\n> +\t\techo $(pwd)/grumpy\n> +\t} >hook.expect &&\n> \tgit worktree add --detach grumpy &&\n> -\ttest_cmp hook.expect hook.actual\n> +\ttest_cmp hook.expect grumpy/hook.actual\n> '\n> \n> test_expect_success '\"add --no-checkout\" suppresses post-checkout hook' '\n> \tpost_checkout_hook &&\n> \trm -f hook.actual &&\n> \tgit worktree add --no-checkout gloopy &&\n> -\ttest_path_is_missing hook.actual\n> +\ttest_path_is_missing gloopy/hook.actual\n> +'\n> +\n> +test_expect_success '\"add\" in other worktree invokes post-checkout hook' '\n> +\tpost_checkout_hook &&\n> +\t{\n> +\t\techo $_z40 $(git rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/.git/worktrees/guppy &&\n> +\t\techo $(pwd)/guppy\n> +\t} >hook.expect &&\n> +\tgit -C gloopy worktree add --detach ../guppy &&\n> +\ttest_cmp hook.expect guppy/hook.actual\n> +'\n> +\n> +test_expect_success '\"add\" in bare repo invokes post-checkout hook' '\n> +\trm -rf bare &&\n> +\tgit clone --bare . bare &&\n> +\t{\n> +\t\techo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&\n> +\t\techo $(pwd)/bare/worktrees/goozy &&\n> +\t\techo $(pwd)/goozy\n> +\t} >hook.expect &&\n> +\tpost_checkout_hook bare &&\n> +\tgit -C bare worktree add --detach ../goozy &&\n> +\ttest_cmp hook.expect goozy/hook.actual\n> '\n> \n> test_done\n> -- \n> 2.16.1.370.g5c508858fb\n> \n\n"},{"id":"339562","messageId":"xmqqeflk7qth.fsf@gitster-ct.c.googlers.com","threadId":"47803","inReplyTo":"20180215230952.51887-1-sunshine@sunshineco.com","subject":"Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-16T18:05:46Z","receivedAt":"2018-02-16T18:05:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ...\n> The hook is run manually, rather than via run_hook_le(), since it needs\n> to change the working directory to that of the worktree, and\n> run_hook_le() does not provide such functionality. As this is a one-off\n> case, adding 'run_hook' overloads which allow the directory to be set\n> does not seem warranted at this time.\n>\n> Reported-by: Lars Schneider \n> Signed-off-by: Eric Sunshine \n> ---\n>\n> This is a re-roll of [1] which fixes \"git worktree add\" to provide\n> proper context to the 'post-checkout' hook so that the hook knows the\n> location of the newly-created worktree.\n>\n> Changes since v2:\n>\n> * Fix crash due to missing NULL-terminator on 'env' list passed to\n> run_command().\n\nThanks. This matches what I had (v2 plus manual fixup while\nqueueing) and looks good.\n\nAs long as the \"hand-rolled\" implementation uses the same\nfind_hook() helper used in run_hook[lv]e, I do not think a one-off\ninvocation of the hook is not too bad, at least for now.\n"},{"id":"339564","messageId":"CAPig+cS5NRAY7jgnKzcZNciF9-s3jo8m=YCh+MU23S-yFu1ZNA@mail.gmail.com","threadId":"47803","inReplyTo":"E978DDBD-AD31-41EC-969B-E6AAC7D4FAF3@gmail.com","subject":"Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-16T18:27:21Z","receivedAt":"2018-02-16T18:27:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 16, 2018 at 11:55 AM, Lars Schneider\n wrote:\n>> On 16 Feb 2018, at 00:09, Eric Sunshine wrote:\n>> The hook is run manually, rather than via run_hook_le(), since it needs\n>> to change the working directory to that of the worktree, and\n>> run_hook_le() does not provide such functionality. As this is a one-off\n>> case, adding 'run_hook' overloads which allow the directory to be set\n>> does not seem warranted at this time.\n>\n> Although this is an one-off case, I would still prefer it if all hook\n> invocations would happen in a central place to avoid future surprises.\n\nA number of other places in the codebase run hooks manually, so this\nis not unprecedented. Rather than adding 'run_hook' overload(s)\nspecific to this particular case, it would make sense to review all\nsuch places and design the API of the new overloads to handle _all_\nthose cases (with the hope of avoiding adding new ad-hoc overloads\neach time). But, that's outside the scope of this bug fix.\n\n>> post_checkout_hook () {\n>> + gitdir=${1:-.git}\n>> + test_when_finished \"rm -f $gitdir/hooks/post-checkout\" &&\n>> + mkdir -p $gitdir/hooks &&\n>> + write_script $gitdir/hooks/post-checkout <<-\\EOF\n>> + {\n>> + echo $*\n>> + git rev-parse --git-dir --show-toplevel\n>\n> I also checked `pwd` here in my suggested test case.\n> I assume you think this check is not necessary?\n\nI do think it's a good idea, and it is still being tested but not in\nquite the same way. I removed the explicit 'pwd' from the output\nbecause I didn't want to deal with potential fallout on Windows. In\nparticular, your test used raw 'pwd' for the \"actual\" file but\n'$(pwd)' for \"expect\", which I think would have run afoul on Windows\nsince '$(pwd)' is meant only to compare output of _Git_ commands,\nwhereas raw 'pwd' is not a Git command. So, I think the test would\nhave needed to use raw 'pwd' for the \"expect\" file, as well. But,\nsince I don't have Windows on which to test, I decided to avoid that\npotential mess by checking 'pwd' in a different way. Details below.\n\n>> + } >hook.actual\n>> EOF\n>> }\n>>\n>> test_expect_success '\"add\" invokes post-checkout hook (branch)' '\n>> post_checkout_hook &&\n>> - printf \"%s %s 1\\n\" $_z40 $(git rev-parse HEAD) >hook.expect &&\n>> + {\n>> + echo $_z40 $(git rev-parse HEAD) 1 &&\n>> + echo $(pwd)/.git/worktrees/gumby &&\n>> + echo $(pwd)/gumby\n>> + } >hook.expect &&\n>> git worktree add gumby &&\n>> - test_cmp hook.expect hook.actual\n>> + test_cmp hook.expect gumby/hook.actual\n>> '\n\nThe explicit 'pwd' check from your test is still here, but is now\nimplicit, so more subtle. Specifically, the hook now emits \"actual\"\nwithin the current working directory (the location 'pwd' would\nreport), and 'test_cmp' looks for the \"actual\" file at that location.\nThe net result is 'pwd' is effectively, though implicitly, recorded by\nthe location of the \"actual\" file itself. If 'pwd' is wrong (that is,\nif the chdir() was wrong or missing), then \"actual\" would not end up\nat the correct location and the 'test_cmp' would fail.\n"}]}