Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 1abe559fe86..377af91c475 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
> >  #define EDIT_MSG    (1<<1)
> >  #define AMEND_MSG   (1<<2)
> >  #define CLEANUP_MSG (1<<3)
> > +#define VERIFY_MSG  (1<<4)
> >  
> >  /*
> >   * If we are cherry-pick, and if the merge did not result in
> > @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> >  	}
> >  
> >  	argv_array_push(&cmd.args, "commit");
> > -	argv_array_push(&cmd.args, "-n");
> >  
> > +	if (!(flags & VERIFY_MSG))
> > +		argv_array_push(&cmd.args, "-n");
> 
> OK, so by default we pass "--no-verify" but selected callers can
> set the bit in the flags word to disable it.  That sounds sensible,
> especially given that the original callers, cherry-pick and revert, 
> did want "--no-verify".  "reword" in "rebase -i" is currently the
> only one we want to supress "--no-verify" and the place that does so
> are all shown in this patch.

Indeed, my reasoning was to keep the default to be the previous behavior.

> Just to see if there are other callers that may want to do the same
> suppressing of "--no-verify" as a follow-up, I looked at the whole
> file after applying the patch, and I think everything looked good
> as-is.

Thank you for being thorough; That is exactly the type of review I hoped
for. I did the same research myself, of course, but it is most reassuring
if an independent reviewer comes to the same conclusion.

> > @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  			write_author_script(msg.message);
> >  		res = fast_forward_to(commit->object.oid.hash, head, unborn,
> >  			opts);
> > -		if (res || command != TODO_REWORD)
> > +		if (res)
> > +			goto leave;
> > +		else if (command == TODO_REWORD)
> > +			flags |= VERIFY_MSG;
> > +		else
> >  			goto leave;
> 
> Both before and after are your code, but wouldn't this:
> 
> 	if (res || command != TODO_REWORD)
> 		goto leave;
> +	if (command == TODO_REWORD)
> +		flags |= VERIFY_MSG
> 
> result in smaller changes relative to the original and still be much
> easier to understand than the above?

Yes. I just did not like the repetition.

But thinking about it again, the previous logic was only concerned about
an early exit, and the clause I added is all about the flags. Therefore, I
agree with you that it should be untangled again.

v2 coming,
Dscho