From: Adrian Ratiu Date: Tue, 21 Oct 2025 16:04:24 GMT Subject: Re: [PATCH v2 04/10] transport: convert pre-push to hook API Message-ID: <87sefcp7g7.fsf@collabora.com> In-Reply-To: On Tue, 21 Oct 2025, Patrick Steinhardt 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.