Re: [PATCH] push: Use sideband channel for hook messages

[Johannes Sixt <j.sixt@viscovery.net>]

Shawn O. Pearce schrieb:
> Rather than sending hook messages over stderr, and losing them
> entirely on git:// and smart HTTP transports,

I don't think "losing them entirely" is true for the git:// protocol:
git-daemon writes receive-pack's stderr to the syslog.

The question is whether hook errors are intended for the remote side or
for the repository owner. Generally, I'd say for the latter. But since
your patch is about pushing, a repository owner must already trust the
remote side, and then it can be argued that in this case errors can be
sent to the remote.

> diff --git a/run-command.c b/run-command.c
> index cf2d8f7..7d1fd88 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -94,6 +94,9 @@ fail_pipe:
>  		else if (need_err) {
>  			dup2(fderr[1], 2);
>  			close_pair(fderr);
> +		} else if (cmd->err > 1) {
> +			dup2(cmd->err, 2);
> +			close(cmd->err);
>  		}
>  
>  		if (cmd->no_stdout)
> @@ -228,6 +231,8 @@ fail_pipe:
>  
>  	if (need_err)
>  		close(fderr[1]);
> +	else if (cmd->err)
> +		close(cmd->err);
>  
>  	return 0;
>  }

This requires similar adjustments in the Windows part.

Documentation/technical/api-runcommand.txt should be an update, too.

> @@ -326,10 +331,19 @@ static unsigned __stdcall run_thread(void *data)
>  int start_async(struct async *async)
>  {
>  	int pipe_out[2];
> +	int proc_fd, call_fd;
>  
>  	if (pipe(pipe_out) < 0)
>  		return error("cannot create pipe: %s", strerror(errno));
> -	async->out = pipe_out[0];
> +
> +	if (async->is_reader) {
> +		proc_fd = pipe_out[0];
> +		call_fd = pipe_out[1];
> +	} else {
> +		call_fd = pipe_out[0];
> +		proc_fd = pipe_out[1];
> +	}
> +	async->out = call_fd;

I don't particularly like this approach because it restricts the async
procedures to a one-way communication.

What would you think about passing both channels to the async callback,
and the communicating parties must agree on which channel they communicate
by closing the unused one? It would require slight changes to all current
async users, though. (It also requires in the threaded case that we pass
dup()s of the pipe channels.)

-- Hannes