Hi Junio,
On Wed, 22 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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