* [PATCH v1] worktree: set worktree environment in post-checkout hook @ 2018-02-10 1:01 lars.schneider 2018-02-10 1:28 ` Lars Schneider ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: lars.schneider @ 2018-02-10 1:01 UTC (permalink / raw) To: git; +Cc: sunshine, matthew.k.gumbel, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v1] worktree: set worktree environment in post-checkout hook 2018-02-10 1:01 [PATCH v1] worktree: set worktree environment in post-checkout hook lars.schneider @ 2018-02-10 1:28 ` Lars Schneider 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine 2018-02-12 3:27 ` [PATCH v1] worktree: set worktree environment in post-checkout hook Eric Sunshine 2 siblings, 0 replies; 24+ messages in thread From: Lars Schneider @ 2018-02-10 1:28 UTC (permalink / raw) To: lars.schneider; +Cc: git, sunshine, matthew.k.gumbel > 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/2] worktree: change to new worktree dir before running hook(s) 2018-02-10 1:01 [PATCH v1] worktree: set worktree environment in post-checkout hook lars.schneider 2018-02-10 1:28 ` Lars Schneider @ 2018-02-12 3:15 ` Eric Sunshine 2018-02-12 3:15 ` [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees Eric Sunshine ` (2 more replies) 2018-02-12 3:27 ` [PATCH v1] worktree: set worktree environment in post-checkout hook Eric Sunshine 2 siblings, 3 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 3:15 UTC (permalink / raw) To: git; +Cc: Lars Schneider, matthew.k.gumbel, 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine @ 2018-02-12 3:15 ` Eric Sunshine 2018-02-12 20:58 ` Lars Schneider 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine 2018-02-15 19:18 ` [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location Eric Sunshine 2 siblings, 1 reply; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 3:15 UTC (permalink / raw) To: git; +Cc: Lars Schneider, matthew.k.gumbel, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees 2018-02-12 3:15 ` [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees Eric Sunshine @ 2018-02-12 20:58 ` Lars Schneider 2018-02-12 21:49 ` Eric Sunshine 0 siblings, 1 reply; 24+ messages in thread From: Lars Schneider @ 2018-02-12 20:58 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, matthew.k.gumbel > 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees 2018-02-12 20:58 ` Lars Schneider @ 2018-02-12 21:49 ` Eric Sunshine 0 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 21:49 UTC (permalink / raw) To: Lars Schneider; +Cc: Git List, Gumbel, Matthew K 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/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine 2018-02-12 3:15 ` [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees Eric Sunshine @ 2018-02-12 3:15 ` Eric Sunshine 2018-02-12 19:37 ` Junio C Hamano ` (2 more replies) 2018-02-15 19:18 ` [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location Eric Sunshine 2 siblings, 3 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 3:15 UTC (permalink / raw) To: git; +Cc: Lars Schneider, matthew.k.gumbel, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine @ 2018-02-12 19:37 ` Junio C Hamano 2018-02-12 20:31 ` Eric Sunshine 2018-02-12 20:01 ` Lars Schneider 2018-02-13 7:27 ` Johannes Sixt 2 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2018-02-12 19:37 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Lars Schneider, matthew.k.gumbel 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 19:37 ` Junio C Hamano @ 2018-02-12 20:31 ` Eric Sunshine 0 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 20:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Lars Schneider, Gumbel, Matthew K 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine 2018-02-12 19:37 ` Junio C Hamano @ 2018-02-12 20:01 ` Lars Schneider 2018-02-13 4:42 ` Eric Sunshine 2018-02-13 7:27 ` Johannes Sixt 2 siblings, 1 reply; 24+ messages in thread From: Lars Schneider @ 2018-02-12 20:01 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, matthew.k.gumbel, Junio C Hamano > 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 && ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 20:01 ` Lars Schneider @ 2018-02-13 4:42 ` Eric Sunshine 2018-02-13 4:48 ` Eric Sunshine 0 siblings, 1 reply; 24+ messages in thread From: Eric Sunshine @ 2018-02-13 4:42 UTC (permalink / raw) To: Lars Schneider; +Cc: git, Gumbel, Matthew K, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-13 4:42 ` Eric Sunshine @ 2018-02-13 4:48 ` Eric Sunshine 0 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-13 4:48 UTC (permalink / raw) To: Lars Schneider; +Cc: git, Gumbel, Matthew K, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine 2018-02-12 19:37 ` Junio C Hamano 2018-02-12 20:01 ` Lars Schneider @ 2018-02-13 7:27 ` Johannes Sixt 2018-02-13 7:34 ` Eric Sunshine 2 siblings, 1 reply; 24+ messages in thread From: Johannes Sixt @ 2018-02-13 7:27 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Lars Schneider, matthew.k.gumbel 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook 2018-02-13 7:27 ` Johannes Sixt @ 2018-02-13 7:34 ` Eric Sunshine 0 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-13 7:34 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git List, Lars Schneider, Gumbel, Matthew K 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine 2018-02-12 3:15 ` [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees Eric Sunshine 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine @ 2018-02-15 19:18 ` Eric Sunshine 2018-02-15 20:52 ` Junio C Hamano 2018-02-15 23:09 ` [PATCH v3] " Eric Sunshine 2 siblings, 2 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-15 19:18 UTC (permalink / raw) To: git; +Cc: Lars Schneider, matthew.k.gumbel, Junio C Hamano, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 19:18 ` [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location Eric Sunshine @ 2018-02-15 20:52 ` Junio C Hamano 2018-02-15 21:27 ` Eric Sunshine 2018-02-15 23:09 ` [PATCH v3] " Eric Sunshine 1 sibling, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2018-02-15 20:52 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Lars Schneider, matthew.k.gumbel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 20:52 ` Junio C Hamano @ 2018-02-15 21:27 ` Eric Sunshine 2018-02-15 21:36 ` Junio C Hamano 2018-02-15 23:09 ` Eric Sunshine 0 siblings, 2 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-15 21:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Lars Schneider, matthew.k.gumbel 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? ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 21:27 ` Eric Sunshine @ 2018-02-15 21:36 ` Junio C Hamano 2018-02-15 23:09 ` Eric Sunshine 1 sibling, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2018-02-15 21:36 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Lars Schneider, matthew.k.gumbel 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 21:27 ` Eric Sunshine 2018-02-15 21:36 ` Junio C Hamano @ 2018-02-15 23:09 ` Eric Sunshine 1 sibling, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-15 23:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Lars Schneider, Gumbel, Matthew K 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 19:18 ` [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location Eric Sunshine 2018-02-15 20:52 ` Junio C Hamano @ 2018-02-15 23:09 ` Eric Sunshine 2018-02-16 16:55 ` Lars Schneider 2018-02-16 18:05 ` Junio C Hamano 1 sibling, 2 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-15 23:09 UTC (permalink / raw) To: git; +Cc: Lars Schneider, matthew.k.gumbel, Junio C Hamano, 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 23:09 ` [PATCH v3] " Eric Sunshine @ 2018-02-16 16:55 ` Lars Schneider 2018-02-16 18:27 ` Eric Sunshine 2018-02-16 18:05 ` Junio C Hamano 1 sibling, 1 reply; 24+ messages in thread From: Lars Schneider @ 2018-02-16 16:55 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, matthew.k.gumbel, Junio C Hamano > 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-16 16:55 ` Lars Schneider @ 2018-02-16 18:27 ` Eric Sunshine 0 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-16 18:27 UTC (permalink / raw) To: Lars Schneider; +Cc: git, Gumbel, Matthew K, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location 2018-02-15 23:09 ` [PATCH v3] " Eric Sunshine 2018-02-16 16:55 ` Lars Schneider @ 2018-02-16 18:05 ` Junio C Hamano 1 sibling, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2018-02-16 18:05 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Lars Schneider, matthew.k.gumbel 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v1] worktree: set worktree environment in post-checkout hook 2018-02-10 1:01 [PATCH v1] worktree: set worktree environment in post-checkout hook lars.schneider 2018-02-10 1:28 ` Lars Schneider 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine @ 2018-02-12 3:27 ` Eric Sunshine 2 siblings, 0 replies; 24+ messages in thread From: Eric Sunshine @ 2018-02-12 3:27 UTC (permalink / raw) To: Lars Schneider; +Cc: Git List, Gumbel, Matthew K, Lars Schneider 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/ ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2018-02-16 18:27 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-10 1:01 [PATCH v1] worktree: set worktree environment in post-checkout hook lars.schneider 2018-02-10 1:28 ` Lars Schneider 2018-02-12 3:15 ` [PATCH 0/2] worktree: change to new worktree dir before running hook(s) Eric Sunshine 2018-02-12 3:15 ` [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees Eric Sunshine 2018-02-12 20:58 ` Lars Schneider 2018-02-12 21:49 ` Eric Sunshine 2018-02-12 3:15 ` [PATCH 2/2] worktree: add: change to new worktree directory before running hook Eric Sunshine 2018-02-12 19:37 ` Junio C Hamano 2018-02-12 20:31 ` Eric Sunshine 2018-02-12 20:01 ` Lars Schneider 2018-02-13 4:42 ` Eric Sunshine 2018-02-13 4:48 ` Eric Sunshine 2018-02-13 7:27 ` Johannes Sixt 2018-02-13 7:34 ` Eric Sunshine 2018-02-15 19:18 ` [PATCH v2] worktree: add: fix 'post-checkout' not knowing new worktree location Eric Sunshine 2018-02-15 20:52 ` Junio C Hamano 2018-02-15 21:27 ` Eric Sunshine 2018-02-15 21:36 ` Junio C Hamano 2018-02-15 23:09 ` Eric Sunshine 2018-02-15 23:09 ` [PATCH v3] " Eric Sunshine 2018-02-16 16:55 ` Lars Schneider 2018-02-16 18:27 ` Eric Sunshine 2018-02-16 18:05 ` Junio C Hamano 2018-02-12 3:27 ` [PATCH v1] worktree: set worktree environment in post-checkout hook Eric Sunshine
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).