Re: [PATCH 1/3] expanded hook api with stdio support

[Joey Hess <joey@kitenet.net>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

Johannes Sixt wrote:
> IMHO, this is overengineered. I don't think that we need something like
> this in the foreseeable future, particularly because such a pipeline or
> multi-hook infrastructure can easily be constructed by the (single) hook
> script itself.

Junio seemed to think this was a good direction to move in and gave some
examples in <7vlipz930t.fsf@alter.siamese.dyndns.org>

Anyway, the minimum cases for run_hook_complex() to support are:

* no stdin, no stdout
* only stdin
* stdin and stdout (needed for tweak-fetch)
* only stdout (perhaps)

The generator and reader members of struct hook allow the caller to
easily specify which of these cases applies to a hook, and also provides
a natural separation of the caller's stdin generation and stdout parsing
code.

That leaves the feeder and data members of struct hook as possible
overengineering. See below regarding the feeder. The data member could
be eliminated and global variables used by callers that need that,
but I prefer designs that don't require global variables.

> > +`run_hook_complex`::
> > +
> > +	Run a hook, with the caller providing its stdin and/or parsing its
> > +	stdout.
> > +	Takes a pointer to a `struct hook` that specifies the details,
> > +	including the name of the hook, any parameters to pass to it,
> > +	and how to handle the stdin and stdout. (See below.)
> > +	If the hook does not exist or is not executable, the return value
> > +	will be zero.
> > +	If it is executable, the hook will be executed and the exit
> > +	status of the hook is returned.
> 
> What is the rationale for these error modes? It is as if a non-existent
> or non-executable hook counts as 'success'. (I'm not saying that this
> would be wrong, I'm just asking.)

They are identical to how run_hook already works.
A non-existant/non-executable hook *is* a valid configuration,
indeed it's the most likely configuration.

> > +/* A feeder that ensures the hook consumes all its stdin. */
> > +int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)
> > +{
> > +	if (write_in_full(handle, buf->buf, buf->len) != buf->len) {
> > +		warning("%s hook failed to consume all its input", hook->name);
> 
> Really? Could there not be any other error condition? The warning would
> be correct only if errno == EPIPE, and this error will be returned only
> if SIGPIPE is ignored. Does this happen anywhere?
> 
> Futhermore, if all data was written to the pipe successfully, there is
> no way to know whether the reader consumed everything.
> 
> IOW, I don't it is worth to make the distinction. However, I do think
> that the parent process must be protected against SIGPIPE.

Yes, this was not thought through, I missed that a write to a pipe can
succeed (due to buffering) despite not being fully consumed.

Dealing with the hook SIGPIPE issue is complicated as Jeff explains in
<20111205214351.GA15029@sigill.intra.peff.net>, and I don't feel I'm the
one to do it. I'm feeling like ripping the "feeder" stuff out of my
patch, and not having my patch change the status quo on this issue.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]