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

[Lars Schneider <larsxschneider@gmail.com>]

> On 08 Aug 2016, at 17:02, Jeff King <peff@peff.net> wrote:
> 
> On Sat, Aug 06, 2016 at 08:19:28PM +0200, Lars Schneider wrote:
> 
>>> I dunno. It's not _that_ big a deal to code around. I was just surprised
>>> not to see an up-front status when responding to a request. It seems
>>> like the normal thing in just about every protocol I've ever used.
>> 
>> Alright. The fact that it "surprised" you is a bad sign. 
>> How about this:
>> 
>> Happy answer:
>> ------------------------
>> packet:          git< status=accept\n
>> packet:          git< SMUDGED_CONTENT
>> packet:          git< 0000
>> packet:          git< status=success\n
>> ------------------------
> 
> I notice that the status pkt-lines are by themselves. I had assumed we'd
> be sending other data, too (presumably before, but I guess possibly
> after, too). Something like:
> 
>  git< status=accept
>  git< 0000
>  git< SMUDGED_CONTENT
>  git< 0000
>  git< status=success
>  git< 0000
> 
> I don't have any particular meta-information in mind, but I thought
> stuff like the tentative "size" field would be here.
> 
> I had imagined it at the front, but I guess it could go in either place.
> I wonder if keys at the end could simply replace ones from the beginning
> (so if you say "foo=bar" at the front, that is tentative, but if you
> then say "foo=revised" at the end, that takes precedence).
> 
> And so the happy answer is really:
> 
>  git< status=success
>  git< 0000
>  git< SMUDGED_CONTENT
>  git< 0000
>  git< 0000  # empty list!
> 
> i.e., no second status. The original "success" still holds.

OK, that sounds sensible to me.


> And then:
> 
>> Happy answer with no content:
>> ------------------------
>> packet:          git< status=success\n
>> ------------------------
> 
> This can just be spelled:
> 
>  git< status=success
>  git< 0000
>  git< 0000   # empty content!
>  git< 0000   # empty list!

Is the first flush packet one too many?
If there is nothing then I think we shouldn't
send any packets?!

I agree with the remaining two flush packets.


>> Rejected content:
>> ------------------------
>> packet:          git< status=reject\n
>> ------------------------
> 
> I'd assume that an error status would end the output for that file
> immediately, no empty lists necessary (so what you have here). I'd
> probably just call this "error" (see below).

OK!


>> Error during content response:
>> ------------------------
>> packet:          git< status=accept\n
>> packet:          git< HALF_WRITTEN_ERRONEOUS_CONTENT
>> packet:          git< 0000
>> packet:          git< status=error\n
>> ------------------------
> 
> And then this would be:
> 
>  git< status=success
>  git< 0000
>  git< HALF_OF_CONTENT
>  git< 0000
>  git< status=error
>  git< 0000
> 
> And then you have only two status codes: success and error. Which keeps
> things simple.
> 
> There's one other case, which is when the filter dies halfway through
> the conversation, like:
> 
>  git< status=success
>  git< 0000
>  git< CONTENT
>  git< 0000
>  ... EOF on pipe ...
> 
> Any time git does not get the conversation all the way to the final
> flush after the trailers, it should be considered an error (because we
> can never know if the filter was about to say "whoops, status=error").

Right. I agree with the protocol above and I will implement it
that way.

There is one more thing: I introduced a return value "status=error-all".
Using this the filter can signal Git that it does not want to process
any other file using the particular command.

Jakub came up with this idea here:

"Another response, which I think should be standarized, or at
least described in the documentation, is filter driver refusing
to filter further (e.g. git-LFS and network is down), to be not
restarted by Git."

http://public-inbox.org/git/607c07fe-5b6f-fd67-13e1-705020c267ee%40gmail.com/

I think it is a good idea. Do you see arguments against it?

Thanks,
Lars