Re: [PATCH v2 04/10] transport: convert pre-push to hook API

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

On Tue, 21 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, Oct 17, 2025 at 05:15:38PM +0300, Adrian Ratiu wrote: 
>> diff --git a/transport.c b/transport.c index 
>> c7f06a7382..67368754bf 100644 --- a/transport.c +++ 
>> b/transport.c @@ -1316,65 +1316,56 @@ static void 
>> die_with_unpushed_submodules(struct string_list *needs_pushing) 
>>  	die(_("Aborting.")); }  
>> -static int run_pre_push_hook(struct transport *transport, - 
>> struct ref *remote_refs) +static int 
>> pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void 
>> *pp_task_cb UNUSED) 
>>  { 
>> -	int ret = 0, x; -	struct ref *r; -	struct 
>> child_process proc = CHILD_PROCESS_INIT; -	struct strbuf buf; 
>> -	const char *hook_path = find_hook(the_repository, 
>> "pre-push"); +	struct hook_cb_data *hook_cb = pp_cb; + 
>> struct ref *r = hook_cb->options->feed_pipe_ctx; +	struct 
>> strbuf *buf = hook_cb->options->feed_pipe_cb_data; 
> 
> Same question here, isn't `feed_pipe_cb_data` accessible via 
> `pp_task_cb`? May very well be that I misunderstand the two 
> callback context and data, I found that part to be a bit hard to 
> follow. 

I hope I ansewered this in the other replies, so I won't repeat 
here. :)
 
>> +	int ret = 0; 
>>   
>> -	if (!hook_path) -		return 0; +	if (!r) + 
>> return 1; /* no more refs */ 
>>   
>> -	strvec_push(&proc.args, hook_path); - 
>> strvec_push(&proc.args, transport->remote->name); - 
>> strvec_push(&proc.args, transport->url); +	if (!buf) + 
>> BUG("pipe_task_cb must contain a valid strbuf"); 
>>   
>> -	proc.in = -1; -	proc.trace2_hook_name = "pre-push"; + 
>> hook_cb->options->feed_pipe_ctx = r->next; 
> 
> I think that the lines between the "task data" and "task 
> context" are being blurred here. I understood it so that the 
> task data is what is specific to the callback, and that data may 
> be changed to keep track of the state. Subsequent commits do it 
> that way, so shouldn't we also treat the context as immutable 
> here and instead handle iteration via the data? 

Excellent observation! I will do exactly that in v3. Thanks!
 
>> +	strbuf_reset(buf); 
> 
> Nit: it would make sense to move this reset down a bit close to 
> the first call that writes to it.

Yes, will do.
 
> 
>> -	if (start_command(&proc)) { - 
>> finish_command(&proc); -		return -1; -	} - - 
>> sigchain_push(SIGPIPE, SIG_IGN); +	if (!r->peer_ref) return 
>> 0; +	if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) return 
>> 0; +	if (r->status == REF_STATUS_REJECT_STALE) return 0; +	if 
>> (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) return 0; +	if 
>> (r->status == REF_STATUS_UPTODATE) return 0; 
> 
> Nit, feel free to ignore: this might read a tiny bit nicer with 
> a switch statement. 

Ack, will fix in v3.