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

From: Jeff King <peff@peff.net>
Date: Aug 3, 2016, 22:53 UTC
Message-ID: <20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net>
In-Reply-To: <826967FE-BFF8-4387-83F7-AE7036D97FEC@gmail.com>

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

Show 12 quoted lines

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

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

Though...

Show 9 quoted lines

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

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

-Peff