Re: [PATCH v2 0/5] Git filter protocol

From: Lars Schneider <larsxschneider@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Jakub Narębski" <jnareb@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Torsten Bögershausen" <tboegi@web.de>,
	mlbright@gmail.com,
	"Remi Galan Alfonso" <remi.galan-alfonso@ensimag.grenoble-inp.fr>,
	"Nguyen Thai Ngoc Duy" <pclouds@gmail.com>,
	e@80x24.org, ramsay@ramsayjones.plus.com
Subject: Re: [PATCH v2 0/5] Git filter protocol
Date: Fri, 29 Jul 2016 18:20:51 +0200	[thread overview]
Message-ID: <FA73FFDF-51D1-49FD-A24E-72A2C033E2F3@gmail.com> (raw)
In-Reply-To: <20160729155740.GB29773@sigill.intra.peff.net>

> On 29 Jul 2016, at 17:57, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Jul 29, 2016 at 10:14:17AM +0200, Lars Schneider wrote:
> 
>> My current implementation supports only two cases. Either the filter
>> knows the size and sends it back. Or the filter doesn't know the size
>> and Git reads until the flush packet (your "unknown" case). "Approx" is 
>> probably hard to do and fail shouldn't be part of the size, no?
> 
> Ah, OK, I missed that you could handle both cases. I think that is a
> reasonable approach. It means the filter has to bother with pkt-lines,
> but beyond that, it can choose the simple or streaming approach as
> appropriate.

Right.

>> 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:

"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.

I am working on the docs right now and afterwards I will send a v3 :-)

- Lars