Re: [PATCH v5 14/15] convert: add filter.<driver>.process option

On Fri, Aug 12, 2016 at 9:38 AM, Jeff King [off-list ref] wrote:

On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:

quoted

quoted

If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet:          git< status=success\n
packet:          git< 0000
packet:          git< 0000  # empty content!
packet:          git< 0000  # empty list!
------------------------

Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?

There was prior discussion in the thread, but basically, it is there to
be able to signal an error that is encountered midway through sending
the file (i.e., to say "status=error"). If you do not have a final
flush, then you would send nothing, and the receiver would be left
wondering if you were successful, or if it simply did not get your error
report yet.

    I did not follow the prior discussion, so I approached this review with
    no prior knowledge from prior reviews, but instead read through and
    was asking a lot of questions that came up immediately. In case my
    questions are too dumb just omit them, but I thought they were good
    material to answer in a commit message ("Why did we do it this way
    and not differently").

Thanks for the explanation. So this is similar as the situation below
that we wait for the flush and then an error/success report?

quoted

quoted

If the filter experiences an error during processing, then it can
send the status "error". Depending on the `filter.<driver>.required`
flag Git will interpret that as error but it will not stop or restart
the filter process.
------------------------
packet:          git< status=success\n

So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?

No, the first success says "good so far, here's the file content". The
second says "I succeeded in sending you the file content".

You _can_ drop the first one, but it may be more convenient for the
receiver to know up-front that there was a failure.

If there was a failure upfront, it would become

packet:          git< 0000
# no content is encapsulated here
packet:          git< 0000
packet:          git< status=error\n
packet:          git< 0000

so from a protocol side I'd claim it doesn't look bad.
I assume with convenient you mean the implementation
side of things?

If we do the success first and then error out halfway, we
still have to clean up, so I do not see how this impacts
implementation?

Thanks,
Stefan