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

On 04 Aug 2016, at 00:53, Jeff King [off-list ref] wrote:

On Thu, Aug 04, 2016 at 12:15:46AM +0200, Lars Schneider wrote:

quoted

quoted

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?

OK, then maybe I am doubly missing the point. I thought this cleanup was
here to hit the case where we call die() and git exits unexpectedly.

If you only want to cover the "we are done, no errors, goodbye" case,
then why don't you just write shutdown when we're done?

I think I tried that at some point but the filter code is called from
multiple places and therefore I looked into atexit() (via run-command)
and it seemed easier. Do you have a place in mind where you would call 
the shutdown after all blobs are processed explicitly?

I realize you may have multiple filters, but I don't think it should be
run-command's job to iterate over them. You are presumably keeping a
list of active filters, and should have a function to iterate over that.

Yes, that would be easy.

Or better yet, do not require a shutdown at all. The filter sees EOF and
knows there is nothing more to do. If we are in the middle of an
operation, then it knows git died. If not, then presumably git had
nothing else to say (and really, it is not the filter's business if git
saw an error or not).

EOF? The filter is supposed to process multiple files. How would one EOF
indicate that we are done?

Though...

quoted

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

OK, so it is not really "tell the filter to shutdown" but "I am done
with you, filter, but I will wait for you to tell me you are all done,
so that I can tell the user".

Correct!

I'm not sure if calling that "shutdown" makes sense, though. It's almost
more of a checkpoint (and I wonder if git would ever want to
"checkpoint" without hanging up the connection).

OK, I agree that the naming might not be ideal. But "checkpoint" does not
convey that it is only executed once after all blobs are filtered?!

I understand that Git might not want to wait for the filter...

- Lars