Re: [PATCH v7 06/12] hook: allow separate std[out|err] streams
- From
Adrian Ratiu <adrian.ratiu@collabora.com>
- Date
- Jan 23, 2026, 07:47 UTC
- Message-ID
- <874iocrcr6.fsf@collabora.com>
- In-Reply-To
- <aXMg-SKKhYzIXvv8@pks.im>
On Fri, 23 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:
Show 50 quoted lines
> On Wed, Jan 21, 2026 at 11:54:30PM +0200, Adrian Ratiu wrote:
>> The hook API assumed that all hooks merge stdout to stderr.
>
> Tiny nit, not worth rerolling over: we typically write the observation
> in past tense. So s/assumed/assumes/
>
>> diff --git a/hook.c b/hook.c
>> index 5ddd7678d1..fde1f88ce8 100644
>> --- a/hook.c
>> +++ b/hook.c
>> @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,
>> cp->in = -1;
>> }
>>
>> - cp->stdout_to_stderr = 1;
>> + cp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;
>> cp->trace2_hook_name = hook_cb->hook_name;
>> cp->dir = hook_cb->options->dir;
>
> The implementation looks easy enough. We convert the static value we had
> before into a configurable one, and...
>
>> diff --git a/hook.h b/hook.h
>> index 2169d4a6bd..7cbeef0a1e 100644
>> --- a/hook.h
>> +++ b/hook.h
>> @@ -34,6 +34,11 @@ struct run_hooks_opt
>> */
>> int *invoked_hook;
>>
>> + /**
>> + * Send the hook's stdout to stderr.
>> + */
>> + unsigned int stdout_to_stderr:1;
>> +
>> /**
>> * Path to file which should be piped to stdin for each hook.
>> */
>
> Another tiny nit that is not worth a reroll: might be worth mentioning
> that this is the default behaviour.
>
>> @@ -80,6 +85,7 @@ struct run_hooks_opt
>> #define RUN_HOOKS_OPT_INIT { \
>> .env = STRVEC_INIT, \
>> .args = STRVEC_INIT, \
>> + .stdout_to_stderr = 1, \
>> }
>
> ... make the old behaviour the default.Thanks for the review, your understanding is correct.
I will address all the nits you pointed out in the v8 reroll I plan to do anyway, to address all the received feedback.
Will leave v7 up for about one more week to gather more feedback.