Hi Junio, On Wed, 22 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin <[email protected]> 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
