> On 29 Jul 2016, at 18:50, Jeff King <peff@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html