Re: [PATCH v3] worktree: add: fix 'post-checkout' not knowing new worktree location

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 16, 2018, 18:27 UTC
Message-ID: <CAPig+cS5NRAY7jgnKzcZNciF9-s3jo8m=YCh+MU23S-yFu1ZNA@mail.gmail.com>
In-Reply-To: <E978DDBD-AD31-41EC-969B-E6AAC7D4FAF3@gmail.com>

On Fri, Feb 16, 2018 at 11:55 AM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 9 quoted lines

>> 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.

Show 11 quoted lines

>> 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.

Show 16 quoted lines

>> +     } >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.