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

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

On Thu, Aug 04, 2016 at 01:09:57AM +0200, Lars Schneider wrote:

quoted

quoted

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?

I think we may be talking about two different EOFs.

Git sends a file in pkt-line format, and the flush marks EOF for that
file. But the filter keeps running, waiting for more input. This can
happen multiple times.

Correct.

Eventually git calls close() on the descriptor, and the filter sees the
"real" EOF (i.e., read() returns 0). That is the signal that git is
done.

Right.

quoted

quoted

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?!

Does the filter need to care? It's told to do any deferred work, and to
report back when it's done. The fact that git is calling it before it
decides to exit is not the filter's business (and you can imagine for
something like fast-import, it might want to feed files to something
like LFS, too; it already checkpoints occasionally to avoid lost work,
and would presumably want to ask LFS to checkpoint, too).

quoted

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

If git _doesn't_ want to wait for the filter, I don't think you need a
checkpoint at all.

True. However, I wonder if it could be useful if the filter is allowed
to do some finishing work *before* Git returns to the user.

The filter just does its deferred work when it sees
git hang up the connection (i.e., the "real" EOF from above).

Yeah it could do that. But then the filter cannot do things like
modifying the index after the fact... however, that might be considered
nasty by the Git community anyways... I am thinking about dropping
this patch in the next roll as it is not strictly necessary for my
current use case.

Thanks,
Lars