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

[Johannes Sixt <j6t@kdbg.org>]

Am 30.12.2011 18:13, schrieb Joey Hess:
> 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.

But as long as the generator only needs to generate a strbuf *and* only
one hook is run, there is no value to have it as a callback; the caller
can just specify the strbuf itself, run_hook_* does not need to care how
it was generated.

I can see some value in a reader callback to avoid allocating yet
another strbuf.

> ... 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.

Absolutely.

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

So, it is so that the caller does not itself have to check whether a
hook exists. That may be worth a word in the API documentation.

-- Hannes