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

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/

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?

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.

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/

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.

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.

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.

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.

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.