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

From: Phillip Wood <[email-masked]>
Date: Fri, 26 Sep 2025 15:12:43 +0100

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 <[email-masked]>
> 
> 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.

> 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 <[email-masked]>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> Signed-off-by: Adrian Ratiu <[email-masked]>
> ---
>   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.

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

>   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");