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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 2, 2025, 06:34 UTC
Message-ID: <aN4c9sqHTgn2wot7@pks.im>
In-Reply-To: <20250925125352.1728840-4-adrian.ratiu@collabora.com>

On Thu, Sep 25, 2025 at 03:53:46PM +0300, Adrian Ratiu wrote:

Show 18 quoted lines

> 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 */
> +
> +	ret = write_in_full(hook_stdin_fd, to_pipe->buf, to_pipe->len);

One thing I wondered in previous patches was whether we now have the potential for deadlocks. If we feed data to a child that exceeds the buffered I/O size, and that child writes data that is consumed by Git larger than the buffered I/O size, as well. Wouldn't that mean that we may now not make any progress at all?

I guess that's no different compared to before though, as we also used `write_in_full()` there.

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

Style: let's drop the curly braces around single-line statements.

Show 5 quoted lines

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

Is this really necessary? I would've expected that we return a positive value from this callback, and as a consequence the run-command subsystem should notice that we're done with writing stdin and close the file descriptor for us. Afterwards, it shouldn't invoke this callback ever again, shouldn't it?

Show 24 quoted lines

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

This change looks completely unrelated to me?

Patrick