Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> The `reword` command used to call `git commit` in a manner that asks for
> the prepare-commit-msg and commit-msg hooks to do their thing.
>
> Converting that part of the interactive rebase to C code introduced the
> regression where those hooks were no longer run.
>
> Let's fix this.
>
> Note: the flag is called `VERIFY_MSG` instead of the more intuitive
> `RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the
> `--no-verify` flag (which may do other things in the future in addition
> to suppressing the commit message hooks, too).
Yup, this is a sound reasoning. I would have made it not a "Note"
but just a regular description of the design decision, e.g.
Call the flag bit "VERIFY_MSG", because this is to suppress the
"--no-verify" flag (do not call it RUN_COMMIT_MSG_HOOKS, as the
purpose of the bit does not have to stay only to run the hooks).
or somesuch, but I can go either way.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> sequencer.c | 12 +++++++++---
> t/t7504-commit-msg-hook.sh | 2 +-
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> 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.
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.
> if ((flags & AMEND_MSG))
> argv_array_push(&cmd.args, "--amend");
> if (opts->gpg_sign)
> @@ -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?
Thanks.