Re: [PATCH] push: Use sideband channel for hook messages
- From
Johannes Sixt <j.sixt@viscovery.net>
- Date
- Feb 5, 2010, 11:58 UTC
- Message-ID
- <4B6C07E3.5030705@viscovery.net>
- In-Reply-To
- <20100205033748.GA19255@spearce.org>
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.
Show 23 quoted lines
> 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.
Show 18 quoted lines
> @@ -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