From: Lars Schneider <larsxschneider@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git <git@vger.kernel.org>,
matthew.k.gumbel@intel.com, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location
Date: Fri, 16 Feb 2018 17:55:02 +0100 [thread overview]
Message-ID: <E978DDBD-AD31-41EC-969B-E6AAC7D4FAF3@gmail.com> (raw)
In-Reply-To: <20180215230952.51887-1-sunshine@sunshineco.com>
> On 16 Feb 2018, at 00:09, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> Although "git worktree add" learned to run the 'post-checkout' hook in
> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
> neglected to change to the directory of the newly-created worktree
> before running the hook. Instead, the hook runs within the directory
> from which the "git worktree add" command itself was invoked, which
> effectively neuters the hook since it knows nothing about the new
> worktree directory.
>
> Further, ade546be47 failed to sanitize the environment before running
> the hook, which means that user-assigned values of GIT_DIR and
> GIT_WORK_TREE could mislead the hook about the location of the new
> worktree. In the case of "git worktree add" being run from a bare
> repository, the GIT_DIR="." assigned by Git itself leaks into the hook's
> environment and breaks Git commands; this is so even when the working
> directory is correctly changed to the new worktree before the hook runs
> since ".", relative to the new worktree directory, does not point at the
> bare repository.
>
> Fix these problems by (1) changing to the new worktree's directory
> before running the hook, and (2) sanitizing the environment of GIT_DIR
> and GIT_WORK_TREE so hooks can't be confused by misleading values.
>
> Enhance the t2025 'post-checkout' tests to verify that the hook is
> indeed run within the correct directory and that Git commands invoked by
> the hook compute Git-dir and top-level worktree locations correctly.
>
> While at it, also add two new tests: (1) verify that the hook is run
> within the correct directory even when the new worktree is created from
> a sibling worktree (as opposed to the main worktree); (2) verify that
> the hook is provided with correct context when the new worktree is
> created from a bare repository (test provided by Lars Schneider).
Thanks! This patch works great and fixes the problem!
More comments below.
> Implementation Notes:
>
> Rather than sanitizing the environment of GIT_DIR and GIT_WORK_TREE, an
> alternative would be to set them explicitly, as is already done for
> other Git commands run internally by "git worktree add". This patch opts
> instead to sanitize the environment in order to clearly document that
> the worktree is fully functional by the time the hook is run, thus does
> not require special environmental overrides.
>
> The hook is run manually, rather than via run_hook_le(), since it needs
> to change the working directory to that of the worktree, and
> run_hook_le() does not provide such functionality. As this is a one-off
> case, adding 'run_hook' overloads which allow the directory to be set
> does not seem warranted at this time.
Although this is an one-off case, I would still prefer it if all hook
invocations would happen in a central place to avoid future surprises.
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is a re-roll of [1] which fixes "git worktree add" to provide
> proper context to the 'post-checkout' hook so that the hook knows the
> location of the newly-created worktree.
>
> Changes since v2:
>
> * Fix crash due to missing NULL-terminator on 'env' list passed to
> run_command().
>
> [1]: https://public-inbox.org/git/20180215191841.40848-1-sunshine@sunshineco.com/
>
> builtin/worktree.c | 20 ++++++++++++---
> t/t2025-worktree-add.sh | 54 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 62 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..f69f862947 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -345,9 +345,23 @@ static int add_worktree(const char *path, const char *refname,
> * Hook failure does not warrant worktree deletion, so run hook after
> * is_junk is cleared, but do return appropriate code when hook fails.
> */
> - if (!ret && opts->checkout)
> - ret = run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid),
> - oid_to_hex(&commit->object.oid), "1", NULL);
> + if (!ret && opts->checkout) {
> + const char *hook = find_hook("post-checkout");
> + if (hook) {
> + const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
> + cp.git_cmd = 0;
> + cp.no_stdin = 1;
> + cp.stdout_to_stderr = 1;
> + cp.dir = path;
> + cp.env = env;
> + cp.argv = NULL;
> + argv_array_pushl(&cp.args, absolute_path(hook),
> + oid_to_hex(&null_oid),
> + oid_to_hex(&commit->object.oid),
> + "1", NULL);
> + ret = run_command(&cp);
> + }
> + }
>
> argv_array_clear(&child_env);
> strbuf_release(&sb);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..d0d2e4f7ec 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -451,32 +451,68 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '
> '
>
> post_checkout_hook () {
> - test_when_finished "rm -f .git/hooks/post-checkout" &&
> - mkdir -p .git/hooks &&
> - write_script .git/hooks/post-checkout <<-\EOF
> - echo $* >hook.actual
> + gitdir=${1:-.git}
> + test_when_finished "rm -f $gitdir/hooks/post-checkout" &&
> + mkdir -p $gitdir/hooks &&
> + write_script $gitdir/hooks/post-checkout <<-\EOF
> + {
> + echo $*
> + git rev-parse --git-dir --show-toplevel
I also checked `pwd` here in my suggested test case.
I assume you think this check is not necessary?
> + } >hook.actual
> EOF
> }
>
> test_expect_success '"add" invokes post-checkout hook (branch)' '
> post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/.git/worktrees/gumby &&
> + echo $(pwd)/gumby
> + } >hook.expect &&
> git worktree add gumby &&
> - test_cmp hook.expect hook.actual
> + test_cmp hook.expect gumby/hook.actual
> '
>
> test_expect_success '"add" invokes post-checkout hook (detached)' '
> post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/.git/worktrees/grumpy &&
> + echo $(pwd)/grumpy
> + } >hook.expect &&
> git worktree add --detach grumpy &&
> - test_cmp hook.expect hook.actual
> + test_cmp hook.expect grumpy/hook.actual
> '
>
> test_expect_success '"add --no-checkout" suppresses post-checkout hook' '
> post_checkout_hook &&
> rm -f hook.actual &&
> git worktree add --no-checkout gloopy &&
> - test_path_is_missing hook.actual
> + test_path_is_missing gloopy/hook.actual
> +'
> +
> +test_expect_success '"add" in other worktree invokes post-checkout hook' '
> + post_checkout_hook &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/.git/worktrees/guppy &&
> + echo $(pwd)/guppy
> + } >hook.expect &&
> + git -C gloopy worktree add --detach ../guppy &&
> + test_cmp hook.expect guppy/hook.actual
> +'
> +
> +test_expect_success '"add" in bare repo invokes post-checkout hook' '
> + rm -rf bare &&
> + git clone --bare . bare &&
> + {
> + echo $_z40 $(git --git-dir=bare rev-parse HEAD) 1 &&
> + echo $(pwd)/bare/worktrees/goozy &&
> + echo $(pwd)/goozy
> + } >hook.expect &&
> + post_checkout_hook bare &&
> + git -C bare worktree add --detach ../goozy &&
> + test_cmp hook.expect goozy/hook.actual
> '
>
> test_done
> --
> 2.16.1.370.g5c508858fb
>
next prev parent reply other threads:[~2018-02-16 16:55 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E978DDBD-AD31-41EC-969B-E6AAC7D4FAF3@gmail.com \
--to=larsxschneider@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=matthew.k.gumbel@intel.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).