On Sat, Dec 29, 2018 at 12:53 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > orgads@xxxxxxxxx writes: > > > From: Orgad Shaneh <orgads@xxxxxxxxx> > > > 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] > > > +#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] > > > +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. > > > + git rebase rebase-on-me && > > + read old new flag < .git/post-checkout.args && > > No SP between "<" and ".git/post-checkout.args". Done > > + 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
