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

On 03 Aug 2016, at 23:24, Jeff King [off-list ref] wrote:

On Wed, Aug 03, 2016 at 06:42:20PM +0200, larsxschneider@gmail.com wrote:

quoted

From: Lars Schneider <redacted>

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.

Maybe I am missing something, but I don't think that can happen because 
the cleanup callback is *only* executed if Git exits normally without error. 
In that case we would be in a sane protocol state, no?

 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.

Thanks. The shutdown command is not intended to be a mechanism to tell
the filter that everything went well. At this point - as you mentioned -
the filter already received all data in the right way. The shutdown
command is intended to give the filter some time to perform some post
processing before Git returns.

See here for some brainstorming how this feature could be useful
in filters similar to Git LFS:
https://github.com/github/git-lfs/issues/1401#issuecomment-236133991

- Lars