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

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 30, 2011, 09:47 UTC
Message-ID: <4EFD88CB.3050403@kdbg.org>
In-Reply-To: <1325207240-22622-2-git-send-email-joey@kitenet.net>

Am 30.12.2011 02:07, schrieb Joey Hess:

Show 7 quoted lines

> Finally, the API is designed to be extended in the future, to support
> running multiple programs for a single hook action (such as the contents
> of a .git/hooks/hook.d/ , or a system-wide hook). This design goal led
> to the "generator" and "reader" members of the struct hook, which are
> specified such that they can be called repeatedly, with data flowing
> between them (via the "data" member), like this:
>     generator | hook_prog_1 | reader | generator | hook_prog_2 | reader

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.

IOW, none of the three function pointers should be needed (not even the feeder, see below), at least not in a first step.

IMO, as the first step, the user of this infrastructure should only be required to construct the hook input as a strbuf, and receive the hook output, if needed, also as a strbuf.

Show 11 quoted lines

> +`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.)

Show 18 quoted lines

>  
>  Data structures
>  ---------------
> @@ -241,3 +252,45 @@ a forked process otherwise:
>  
>  . It must not change the program's state that the caller of the
>    facility also uses.
> +
> +* `struct hook`
> +
> +This describes a hook to run.
> +
> +The caller:
> +
> +1. allocates and clears (memset(&hook, 0, sizeof(hook));) a
> +   struct hook variable;
> +2. initializes the members;
> +3. calls hook();

run_hook_complex()?

> +4. if necessary, accesses data read from the hook in .data
> +5. frees the struct hook.

> +The `feeder` is run asynchronously to feed the generated stdin into the hook.
> +It is passed the handle to write to, the strbuf containing the stdin, and 
> +a pointer to the `struct hook`, and should return non-zero on failure.
> +Typically it is set to either `feed_hook_in_full`, or `feed_hook_incomplete`.

IMO, this is overengineered. See below.

> +	res |= start_command(&child);
> +	if (res) goto hook_out;

Please write this conditional in two lines.

Show 5 quoted lines

> +/* 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.

Show 5 quoted lines

> +		return 1;
> +	}
> +	else
> +		return 0;
> +}

-- Hannes