From: Lars Schneider <larsxschneider@gmail.com>
Date: Fri, 29 Jul 2016 17:43:49 GMT
Subject: Re: [PATCH v2 0/5] Git filter protocol
Message-ID: <9C2A3438-84BA-49FE-87AA-1084995A1E89@gmail.com>
In-Reply-To: <20160729165018.GA6553@sigill.intra.peff.net>


> On 29 Jul 2016, at 18:50, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Jul 29, 2016 at 06:20:51PM +0200, Lars Schneider wrote:
> 
>>>> That being said a "fail" response is a very good idea! This allows
>>>> the filter to communicate to git that a non required filter process
>>>> failed. I will add that to the protocol. Thanks :) 
>>> 
>>> Maybe just send "ok <size>", "ok -1" (for streaming), or "fail <reason>"
>>> followed by the content? That is similar to other Git protocols, though
>>> I am not sure they are good models for sanity or extensibility. :)
>>> 
>>> I don't know if you would want to leave room for other "headers" in the
>>> response, but you could also do something more HTTP-like, with a status
>>> code, and arbitrary headers. And presumably git would just ignore
>>> headers it doesn't know about. I think that's what Jakub's example was
>>> leaning towards. I'm just not sure what other headers are really useful,
>>> but it does leave room for extensibility.
>> 
>> Well, "ok <size>" wouldn't make much sense as we already transmitted
>> the size upfront I think. Right now I have implemented the following options:
> 
> Maybe I'm confused about where in the protocol we are. I was imagining:
> 
>  git> smudge
>  git> <filename>
>  git> <size>
>  git> ...pkt-lines...
>  git> pktline-flush
> 
>  git< ok <size>
>  git< ...pkt-lines...
>  git< flush
> 
> That is, we should say "I have something for you" or "I do not" before
> sending a size, because in the "I do not" case we have no size to send.


Right now the protocol is like that in the happy case (non-streaming):

git> smudge
git> <filename>
git> <size>
git> ...pkt-lines...
git> pktline-flush

git< <size>
git< ...pkt-lines...
git< flush
git< success

(diff to your version: no "ok" in front of size answer ... plus the
size answer is not present in the streaming case)


Here is the reject case (non-streaming):

git> smudge
git> <filename>
git> <size>
git> ...pkt-lines...
git> pktline-flush

git< 0
git< reject


Do you see a problem with this approach?


> A more extensible protocol might look like:
> 
>  git> smudge
>  git> filename=<filename>
>  git> size=<size>
>  git> pktline-flush
>  git> ...pkt-lines of data...
>  git> pktline-flush
> 
>  git< ok (or success, or whatever status code you like)
>  git< size=<size>
>  git< pkt-line-flush
>  git< ...pkt-lines of data...
>  git< pktline-flush
> 
> That leaves room for new "keys" to be added before the first pkt-flush,
> without having to change the parsing at all.

Alright. Will be in v3!


>> "success\n" --> everything was alright
>> "reject\n" --> the filter rejected the operation but this is no error 
>>               if "filter.<driver>.required = false"
>> <anything else> --> failure that stops/restarts the filter process
>> 
>> I don't think sending any failure reason makes sense because if a failure
>> happens then we are likely in a bad state already (that's why I restart the
>> filter process. I think the filter can report trouble on its own via stdout,
>> no? I think this is what Git-LFS already does.
> 
> Git-LFS sends to stderr because there's no other option. I wonder if it
> would be nicer to make it Git's responsibility to talk to the user,
> because then it could respect things like "--quiet". I guess error
> messages are generally printed regardless of verbosity, though, so
> printing them unconditionally is OK.

OK!

Thanks,
Lars