From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 02 Oct 2025 06:34:30 GMT
Subject: Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h
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:
> 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.

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

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