From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 23 Jan 2026 07:47:57 GMT
Subject: Re: [PATCH v7 06/12] hook: allow separate std[out|err] streams
Message-ID: <874iocrcr6.fsf@collabora.com>
In-Reply-To: <aXMg-SKKhYzIXvv8@pks.im>

On Fri, 23 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.