Re: [PATCH v2 04/10] transport: convert pre-push to hook API
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 21, 2025, 07:41 UTC
- Message-ID
- <aPc5IE1Ie3i_Axqs@pks.im>
- In-Reply-To
- <20251017141544.1538542-5-adrian.ratiu@collabora.com>
On Fri, Oct 17, 2025 at 05:15:38PM +0300, Adrian Ratiu wrote:
Show 20 quoted lines
> 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.
Show 16 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?
> + 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.
Patrick