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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2017, 18:43 UTC
Message-ID: <xmqq60j1kv8n.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <9b83b6f7012368e970ec2b310f63b4daf66602e5.1490194846.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 12 quoted lines

> 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.

Show 26 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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.

Show 14 quoted lines

>  	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.