Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 26, 2025, 14:12 UTC
Message-ID: <f408e46c-650a-4632-9628-cf817e393e7f@gmail.com>
In-Reply-To: <20250925125352.1728840-4-adrian.ratiu@collabora.com>

Hi Adrian

Thanks for working on this, it would be really good to be able to run hooks in parallel.

On 25/09/2025 13:53, Adrian Ratiu wrote:

> From: Emily Shaffer <emilyshaffer@google.com>
> 
> By using 'hook.h' for 'post-rewrite', we simplify hook invocations by
> not needing to put together our own 'struct child_process'.

Right so instead we use the new api to feed an strbuf into the hook's stdin, sounds reasonable.

Show 27 quoted lines

> The signal handling that's being removed by this commit now takes
> place in run-command.h:run_processes_parallel(), so it is OK to remove
> them here.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>   sequencer.c | 62 ++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 9ae40a91b2..93cd6ab1f2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1298,32 +1298,46 @@ int update_head_with_reflog(const struct commit *old_head,
>   	return ret;
>   }
>   
> +static int pipe_from_strbuf(int hook_stdin_fd, void *pp_cb, void *pp_task_cb UNUSED)
> +{
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct strbuf *to_pipe = hook_cb->options->feed_pipe_ctx;
> +	int ret;
> +
> +	if (!to_pipe || !to_pipe->len)
> +		return 1; /* nothing to feed */

Why are we running the hook if there is nothing to pass to it?

> +	ret = write_in_full(hook_stdin_fd, to_pipe->buf, to_pipe->len);

This will block until the hook has read all of the input. Unless the hook drains and closes stdin before it does anything else it will block the parallel execution of other hooks.

> +	if (ret < 0) {
> +		if (errno == EPIPE) {
> +			return 1; /* child closed pipe, nothing more to feed */
> +		}

Style: we don't use braces for single statement bodies.

Show 5 quoted lines

> +		return ret;
> +	}
> +
> +	/* Reset the input buffer to avoid sending it again */
> +	strbuf_reset(to_pipe);

Shouldn't the return value do that?

> +	return ret;
> +}

The changes to run_rewrite_hook() look fine. I'm not sure whats happening in commit_post_rewrite() below though - am I missing something or have you just renamed "child" -> "notes_cp". If so I don't see what that has to do with using the new api.

Thanks

Phillip

Show 25 quoted lines

>   void commit_post_rewrite(struct repository *r,
> @@ -5140,16 +5154,16 @@ static int pick_commits(struct repository *r,
>   		flush_rewritten_pending();
>   		if (!stat(rebase_path_rewritten_list(), &st) &&
>   				st.st_size > 0) {
> -			struct child_process child = CHILD_PROCESS_INIT;
> +			struct child_process notes_cp = CHILD_PROCESS_INIT;
>   			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
>   
> -			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
> -			child.git_cmd = 1;
> -			strvec_push(&child.args, "notes");
> -			strvec_push(&child.args, "copy");
> -			strvec_push(&child.args, "--for-rewrite=rebase");
> +			notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY);
> +			notes_cp.git_cmd = 1;
> +			strvec_push(&notes_cp.args, "notes");
> +			strvec_push(&notes_cp.args, "copy");
> +			strvec_push(&notes_cp.args, "--for-rewrite=rebase");
>   			/* we don't care if this copying failed */
> -			run_command(&child);
> +			run_command(&notes_cp);
>   
>   			hook_opt.path_to_stdin = rebase_path_rewritten_list();
>   			strvec_push(&hook_opt.args, "rebase");