Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout

From: Orgad Shaneh <orgads@gmail.com>
Date: Dec 29, 2018, 21:31 UTC
Message-ID: <CAGHpTBK+1JxhqGDNsjY4Qevd65gSYYeVMBVpHxa0=0OrKbbrEg@mail.gmail.com>
In-Reply-To: <xmqqimzdl0v4.fsf@gitster-ct.c.googlers.com>

On Sat, Dec 29, 2018 at 12:53 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

>
> orgads@gmail.com writes:
>
> > From: Orgad Shaneh <orgads@gmail.com>
>
> > Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout
>
> There is no explanation here?
>
> Is this a regression fix (i.e. scripted version of "rebase" used to
> run the hook)?  Or a new feature (i.e. no earlier version of
> "rebase" run the hook but you think it ought to run it)?

Added an explanation.

[snip]

Show 7 quoted lines

>
> > +#define RESET_HEAD_RUN_HOOK (1<<2)
>
> Would it be plausible that the only possible hook that can be run by
> reset_head() function will always be post-checkout and nothing else,
> I wonder?  Shouldn't this bit be called *_RUN_POST_CHECKOUT to make
> sure it is specific enough?

Done

[snip]

Show 7 quoted lines

>
> > +test_expect_success 'post-checkout is triggered on rebase' '
> > +     git checkout -b rebase-test master &&
> > +     rm -f .git/post-checkout.args &&
>
> Read the title of this whole test script file; it should verify what
> is in the file before removing it.

Why? Other tests already do it. This test is meant for rebase, not plain checkout.

Show 5 quoted lines

>
> > +     git rebase rebase-on-me &&
> > +     read old new flag < .git/post-checkout.args &&
>
> No SP between "<" and ".git/post-checkout.args".

Done

Show 6 quoted lines

> > +     test $old != $new && test $flag = 1 &&
> > +     rm -f .git/post-checkout.args
> > +'
>
> Regarding the clean-up of this file, see my review on the previous
> one.

Done

> > +test_expect_success 'post-checkout is triggered on rebase with fast-forward' '
>
> The same comment as above applies to this.

Done

Thanks,
- Orgad