Re: [PATCH 05/10] reference-transaction: use hook.h to run hooks
- From
Adrian Ratiu <adrian.ratiu@collabora.com>
- Date
- Oct 8, 2025, 12:26 UTC
- Message-ID
- <87v7kp37gn.fsf@collabora.com>
- In-Reply-To
- <aN4c_DWtqBBScKEh@pks.im>
Hi Patrick and sorry for the delayed reply!
On Thu, 02 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
Show 37 quoted lines
> On Thu, Sep 25, 2025 at 03:53:48PM +0300, Adrian Ratiu wrote:
>> diff --git a/refs.c b/refs.c index 4ff55cf24f..5a2b6ad1fc
>> 100644 --- a/refs.c +++ b/refs.c @@ -2377,31 +2377,16 @@ static
>> int ref_update_reject_duplicates(struct string_list *refnames,
>> return 0; }
>> -static int run_transaction_hook(struct ref_transaction
>> *transaction, - const char *state)
>> +static int transaction_hook_feed_stdin(int hook_stdin_fd, void
>> *pp_cb, void *pp_task_cb UNUSED)
>> {
>> - struct child_process proc = CHILD_PROCESS_INIT; +
>> struct hook_cb_data *hook_cb = pp_cb; + struct
>> run_hooks_opt *opt = hook_cb->options; + struct
>> ref_transaction *transaction = opt->feed_pipe_ctx;
>> struct strbuf buf = STRBUF_INIT;
>> - const char *hook; - int ret = 0, i; - - hook =
>> find_hook(transaction->ref_store->repo,
>> "reference-transaction"); - if (!hook) - return
>> ret; - - strvec_pushl(&proc.args, hook, state, NULL); -
>> proc.in = -1; - proc.stdout_to_stderr = 1; -
>> proc.trace2_hook_name = "reference-transaction"; - - ret =
>> start_command(&proc); - if (ret) - return
>> ret; - - sigchain_push(SIGPIPE, SIG_IGN);
>>
>> - for (i = 0; i < transaction->nr; i++) { + for (int i
>> = 0; i < transaction->nr; i++) {
>> struct ref_update *update =
>> transaction->updates[i];
>> + int ret;
>> if (update->flags & REF_LOG_ONLY) continue;
>
> Hm. In the "pre-push" hook you converted the callback to process
> one ref per invocation. Why don't we do the same over here, with
> one transaction per invocation?
>
> Not saying that either one of these is better, but it left me
> puzzled why we use two different patterns now. Good catch! It's a good idea to make them consistent.
We should do it here like we did for pre-push, to avoid processing all data at once and risk blocking on the write side (one extreme).
However, writing just once/one-line per callback (the other extreme) might add unnecessary delay/waiting on the hook child side.
So I'll modify both to batch let's say 50-100 in one call, to ensure a good balance.
This ties into my previous message about how much data should we write in one batch to ensure good throughtput while also not blocking for too long. Hope it makes sense.
Will improve this in v2.
Again, many thanks for your careful review, really appreciate it.