Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler

> On 03 Aug 2016, at 23:24, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Aug 03, 2016 at 06:42:20PM +0200, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Some commands might need to perform cleanup tasks on exit. Let's give
>> them an interface for doing this.
>> 
>> Please note, that the cleanup callback is not executed if Git dies of a
>> signal. The reason is that only "async-signal-safe" functions would be
>> allowed to be call in that case. Since we cannot control what functions
>> the callback will use, we will not support the case. See 507d7804 for
>> more details.
> 
> I'm not clear on why we want this cleanup filter. It looks like you use
> it in the final patch to send an explicit shutdown to any filters we
> start. But I see two issues with that:
> 
>  1. This shutdown may come at any time, and you have no idea what state
>     the protocol conversation with the filter is in. You could be in
>     the middle of sending another pkt-line, or in a sequence of non-command
>     pkt-lines where "shutdown" is not recognized.

>  2. If your protocol does bad things when it is cut off in the middle
>     without an explicit shutdown, then it's a bad protocol. As you
>     note, this patch doesn't cover signal death, nor could it ever
>     cover something like "kill -9", or a bug which prevented git from
>     saying "shutdown".
> 
>     You're much better off to design the protocol so that a premature
>     EOF is detected as an error.  For example, if we're feeding file
>     data to the filter, and we're worried it might be writing it to
>     a data store (like LFS), we would not want it to see EOF and say
>     "well, I guess I got all the data; time to store this!". Instead,
>     it should know how many bytes are coming, or should have some kind
>     of framing so that the sender says "and now you have seen all the
>     bytes" (like a pkt-line flush).
> 
>     AFAIK, your protocol _does_ do those things sensibly, so this
>     explicit shutdown isn't really accomplishing anything.