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

[Adrian Ratiu <adrian.ratiu@collabora.com>]

Hi Patrick!

On Thu, 02 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

That is correct, yes, it's the same as before.

Deadlocks can happen, however they are the result of bugs, for 
example a hook child waits for stdin, the parent doesn't feed 
anything and decides to wait for the child to finish. :)

I hit quite a few of these during development, however they should 
all be fixed (deadlocks are usually easily fixed once detected).

Related, but also important, is thoroughtput when feeding the 
pipes: sending input too granularly (e.g. by calling the callback 
on each line when we send many lines) can add unnecessary 
latencies / delays due to the run-command ppoll mechanism.

It's a balance we must find: for most hooks it doesn't matter 
because the input is small (in this case it's a single write), 
however hooks like post-receive get large amount of data, so there 
I had to batch 300-500 lines to get simliar performance as before 
the callback.

So yes, it's improtant to have no deadlocks and it's also 
important to have roughly the same throughtput through the pipes.
 
>> +	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. 

Ack, will do, others pointed it out as well.
 
>> +		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?

That is correct: it is not necessary. Phillip already made me 
aware that I can significantly simplify this function, which I 
will do in v2 shortly. :)
 
> 
>> @@ -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? 

Yes, Phillip pointed this out as well, I will drop it in v2.

Thanks!