Johannes Sixt <j.sixt@xxxxxxxxxxxxx> wrote:
> 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.
I think everyone expects hook errors on the client.
Most people push over authenticated SSH, where hook messages come
back on stderr. For these folks, if they wanted something in syslog,
they'd send it there directly from the hook.
I doubt there are many anonymous pushes allowed over git://. But,
yea, sure, I could see someone setting up their server with an
update hook to only permit pushes to the refs/heads/mob branch and
logging access failures to syslog by echoing to stderr. I think
these people are in the vast minority, and might not even want this
behavior by default. Maybe I'm just a jerk, but if they want their
hooks to continue to echo to syslog rather than to the client,
they can build a patch on top of this to git daemon which passes
a flag down into receive-pack to disable the side band channel.
> > diff --git a/run-command.c b/run-command.c
> > index cf2d8f7..7d1fd88 100644
> > @@ -228,6 +231,8 @@ fail_pipe:
> >
> > if (need_err)
> > close(fderr[1]);
> > + else if (cmd->err)
> > + close(cmd->err);
>
> This requires similar adjustments in the Windows part.
>
> Documentation/technical/api-runcommand.txt should be an update, too.
Ouch, good catch. Will fix.
> > @@ -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.
Well, its always been one way. With the async function writing to the
main application consumer.
> 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.)
Yup, I could do that. I feel like it might be over-engineering the
solution a bit. But I'll respin the patch by splitting it apart,
and doing a bidirectional async here, since you asked nicely.
--
Shawn.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html