> On 04 Aug 2016, at 00:53, Jeff King <peff@xxxxxxxx> wrote: > > On Thu, Aug 04, 2016 at 12:15:46AM +0200, Lars Schneider wrote: > >>> 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... > >> 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-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html