Re: [PATCH v4 11/12] convert: add filter.<driver>.process option

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

On Fri, Aug 05, 2016 at 03:06:28PM -0700, Junio C Hamano wrote:

quoted

Torsten Bögershausen [off-list ref] writes:

quoted

On 2016-08-03 18.42, larsxschneider@gmail.com wrote:

quoted

The filter is expected to respond with the result content in zero
or more pkt-line packets and a flush packet at the end. Finally, a
"result=success" packet is expected if everything went well.
------------------------
packet:          git< SMUDGED_CONTENT
packet:          git< 0000
packet:          git< result=success\n
------------------------

I would really send the diagnostics/return codes before the content.

I smell the assumption "by the time the filter starts output, it
must have finished everything and knows both size and the status".

I'd prefer to have a protocol that allows us to do streaming I/O on
both ends when possible, even if the initial version of the filters
(and the code that sits on the Git side) hold everything in-core
before starting to talk.

I think you really want to handle both cases:

 - the server says "no, I can't fulfill your request" (e.g., HTTP 404)

You can do this with the current protocol:

packet:          git< 0000
packet:          git< result=reject\n

Admittedly the flush packet could be consider overhead but I think
that is neglectable.

 - the server can abort an in-progress response to indicate that it
   could not be fulfilled completely (in HTTP chunked encoding, this
   requires hanging up before sending the final EOF chunk)

Also already supported with the following sequence:

packet:          git< HALF_WRITTEN_ERRONEOUS_CONTENT
packet:          git< 0000
packet:          git< result=error\n

If we expect the second case to be rare, then hanging up before sending
the flush packet is probably OK. But we could also have a trailing error
code after the data to say "ignore that, we saw an error, but I can
still handle more requests".

It is true that you don't need the up-front status code in that case
(you can send an empty body and say "ignore that, we saw an error") but
that feels a little weird.

I understand your argument. However, I think "0000" indicates 
"I have nothing for you" and therefore I think it would be OK in the
reject case.

And I expect it makes the lives of the client
easier to get a code up front, before it starts taking steps to handle
what it _thinks_ is probably a valid response.

I am not sure I can follow you here. Which actor are you referring to when
you write "client" -- Git, right? If the response is rejected right away
then Git just needs to read a single flush. If the response experiences
an error only later, then the filter wouldn't know about the error when
it starts sending. Therefore I don't see how an error code up front could
make it easier for Git.

- Lars