Re: [PATCH 1/2] merge: don't run post-hook logic on --no-verify

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 23:57 UTC
Message-ID: <xmqqa6enybgz.fsf@gitster.g>
In-Reply-To: <patch-1.2-9b5144daee6-20220218T203834Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 29 quoted lines

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 133831d42fd..fab553e3bc4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -845,15 +845,18 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	struct strbuf msg = STRBUF_INIT;
>  	const char *index_file = get_index_file();
>  
> -	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
> -		abort_commit(remoteheads, NULL);
> -	/*
> -	 * Re-read the index as pre-merge-commit hook could have updated it,
> -	 * and write it out as a tree.  We must do this before we invoke
> -	 * the editor and after we invoke run_status above.
> -	 */
> -	if (hook_exists("pre-merge-commit"))
> -		discard_cache();
> +	if (!no_verify) {
> +		if (run_commit_hook(0 < option_edit, index_file,
> +				    "pre-merge-commit", NULL))
> +			abort_commit(remoteheads, NULL);
> +		/*
> +		 * Re-read the index as pre-merge-commit hook could have updated it,
> +		 * and write it out as a tree.  We must do this before we invoke
> +		 * the editor and after we invoke run_status above.
> +		 */
> +		if (hook_exists("pre-merge-commit"))
> +			discard_cache();
> +	}

The updated code not just is more correct, but it is much easier to follow.

I wonder if run_commit_hook() can return "I failed to run the hook", "I ran the hook and the hook failed", "I successfully run the hook", and "I didn't find the hook", instead of the current "yes/no". That would allow us to express this part in an even cleaner way, I would think.

Looking good.

Thanks.

>  	read_cache_from(index_file);
>  	strbuf_addbuf(&msg, &merge_msg);
>  	if (squash)