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

From: Jeff King <peff@peff.net>
Date: Jul 29, 2016, 16:50 UTC
Message-ID: <20160729165018.GA6553@sigill.intra.peff.net>
In-Reply-To: <FA73FFDF-51D1-49FD-A24E-72A2C033E2F3@gmail.com>

On Fri, Jul 29, 2016 at 06:20:51PM +0200, Lars Schneider wrote:

Show 17 quoted lines

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

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.

Show 9 quoted lines

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

-Peff