Re: [PATCH v1] worktree: set worktree environment in post-checkout hook
On Fri, Feb 9, 2018 at 8:01 PM,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 > --- > 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-sunsh...@sunshineco.com/
Re: [PATCH v1] worktree: set worktree environment in post-checkout hook
> On 10 Feb 2018, at 02:01, lars.schnei...@autodesk.com wrote: > > From: Lars Schneider> > 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 > --- > > 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(_oid), > + if (!ret && opts->checkout) { > + struct argv_array env = ARGV_ARRAY_INIT; > + argv_array_pushf(, "GIT_WORK_TREE=%s", absolute_path(path)); > + ret = run_hook_le(env.argv, "post-checkout", > oid_to_hex(_oid), > oid_to_hex(>object.oid), "1", NULL); As I hit "send" I realized that I forgot to cleanup. @Junio: Can you squash this in? argv_array_clear(); Thanks, Lars > + } > > argv_array_clear(_env); > strbuf_release(); > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > index 2b95944973..d022ac0c26 100755 > --- a/t/t2025-worktree-add.sh > +++ b/t/t2025-worktree-add.sh > @@ -455,19 +455,26 @@ post_checkout_hook () { > mkdir -p .git/hooks && > write_script .git/hooks/post-checkout <<-\EOF > echo $* >hook.actual > + git rev-parse --show-toplevel >>hook.actual > EOF > } > > test_expect_success '"add" invokes post-checkout hook (branch)' ' > post_checkout_hook && > - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect && > + cat >hook.expect <<-EOF && > + $_z40 $(git rev-parse HEAD) 1 > + $(pwd)/gumby > + EOF > git worktree add gumby && > test_cmp hook.expect hook.actual > ' > > test_expect_success '"add" invokes post-checkout hook (detached)' ' > post_checkout_hook && > - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect && > + cat >hook.expect <<-EOF && > + $_z40 $(git rev-parse HEAD) 1 > + $(pwd)/grumpy > + EOF > git worktree add --detach grumpy && > test_cmp hook.expect hook.actual > ' > > base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa > -- > 2.16.1 >
[PATCH v1] worktree: set worktree environment in post-checkout hook
From: Lars SchneiderIn 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 --- 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(_oid), + if (!ret && opts->checkout) { + struct argv_array env = ARGV_ARRAY_INIT; + argv_array_pushf(, "GIT_WORK_TREE=%s", absolute_path(path)); + ret = run_hook_le(env.argv, "post-checkout", oid_to_hex(_oid), oid_to_hex(>object.oid), "1", NULL); + } argv_array_clear(_env); strbuf_release(); 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