On Fri, Feb 16, 2018 at 11:55 AM, Lars Schneider
<larsxschneider@xxxxxxxxx> wrote:
>> On 16 Feb 2018, at 00:09, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
