Re: [PATCH] hook: make stdout_to_stderr optional

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> diff --git a/hook.c b/hook.c
> index 35211e5ed7..ebd9d9e26e 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;

So stdout_to_stderr used to get always forced to 1, but now the
value comes from the hook option, which ...

> diff --git a/hook.h b/hook.h
> index ae502178b9..2488db7133 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -39,6 +39,11 @@ struct run_hooks_opt
>  	 */
>  	unsigned int ungroup:1;
>  
> +	/**
> +	 * 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.
>  	 */
> @@ -93,6 +98,7 @@ struct run_hooks_opt
>  #define RUN_HOOKS_OPT_INIT { \
>  	.env = STRVEC_INIT, \
>  	.args = STRVEC_INIT, \
> +	.stdout_to_stderr = 1, \
>  }

... defaults to 1 for everybody, unless a specific caller opts out
by ...

>  struct hook_cb_data {
> diff --git a/transport.c b/transport.c
> index 6d0f02be5d..8f0e5987ab 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,
>  
>  	opt.feed_pipe = pre_push_hook_feed_stdin;
>  	opt.feed_pipe_cb_data = &data;
> +	opt.stdout_to_stderr = 0;

... setting it to 0 in their hook option.

> The last batch of hooks converted to the hook.[ch] API introduced
> a regression because pick_next_hook() always sets stdout_to_stderr
> for its child processes.
>
> Pre-push is the only hook API user which requires stdout_to_stderr
> to be 0, so it can be argued that pre-push needs fixing, however
> this will likely break many pre-push hooks, so it's better to allow
> it to be 0, i.e. to match the previous behavior.

What was the previous behaviour of code paths that ran other hooks?
Was pre-push the only one that didn't divert standard output to
standard error?  This patch does look like a proper regression fix
in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
the change for "Merge branch 'ar/run-command-hook'") and random
sampling (like run_receive_hook() that used run_and_feed_hook(),
which set stdout_to_stderr to 1) seems to indicate that it is the
case.

> We can introduce an extension for the breaking change of all hooks
> sending stdout to stderr, however this just fixes the regression.

Thanks.