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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Aug 6, 2016, 11:55 UTC
Message-ID: <87D4BF17-67BB-4AFA-9B27-40DBB44C0456@gmail.com>
In-Reply-To: <20160805222710.chefh5kiktyzketh@sigill.intra.peff.net>

Show 28 quoted lines

> On 06 Aug 2016, at 00:27, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Aug 05, 2016 at 03:06:28PM -0700, Junio C Hamano wrote:
> 
>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>>> On 2016-08-03 18.42, larsxschneider@gmail.com wrote:
>>>> 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

Show 8 quoted lines

> 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