Re: [PATCH v2 04/10] transport: convert pre-push to hook API
On Tue, 21 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
Show 22 quoted lines
> 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. :)
Show 19 quoted lines
>> + 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.
Show 11 quoted lines
>
>> - 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. Adrian Ratiu <adrian.ratiu@collabora.com>