From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id AC2DD1F955 for ; Fri, 29 Jul 2016 16:50:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752819AbcG2QuW (ORCPT ); Fri, 29 Jul 2016 12:50:22 -0400 Received: from cloud.peff.net ([50.56.180.127]:51144 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752283AbcG2QuW (ORCPT ); Fri, 29 Jul 2016 12:50:22 -0400 Received: (qmail 1322 invoked by uid 102); 29 Jul 2016 16:50:21 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.84) with SMTP; Fri, 29 Jul 2016 12:50:21 -0400 Received: (qmail 2984 invoked by uid 107); 29 Jul 2016 16:50:47 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.84) with SMTP; Fri, 29 Jul 2016 12:50:47 -0400 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Fri, 29 Jul 2016 12:50:18 -0400 Date: Fri, 29 Jul 2016 12:50:18 -0400 From: Jeff King To: Lars Schneider Cc: Jakub =?utf-8?B?TmFyxJlic2tp?= , Git Mailing List , Junio C Hamano , Torsten =?utf-8?Q?B=C3=B6gershausen?= , mlbright@gmail.com, Remi Galan Alfonso , Nguyen Thai Ngoc Duy , e@80x24.org, ramsay@ramsayjones.plus.com Subject: Re: [PATCH v2 0/5] Git filter protocol Message-ID: <20160729165018.GA6553@sigill.intra.peff.net> References: <20160722154900.19477-1-larsxschneider@gmail.com> <20160727000605.49982-1-larsxschneider@gmail.com> <579906C5.1050809@gmail.com> <20160728132906.GA21311@sigill.intra.peff.net> <579B087F.7090108@gmail.com> <31219A33-CA8A-44D1-9DE0-6448AA1A7571@gmail.com> <20160729155740.GB29773@sigill.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 ", "ok -1" (for streaming), or "fail " > > 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 " 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> git> git> ...pkt-lines... git> pktline-flush git< ok 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= git> size= git> pktline-flush git> ...pkt-lines of data... git> pktline-flush git< ok (or success, or whatever status code you like) git< 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. > "success\n" --> everything was alright > "reject\n" --> the filter rejected the operation but this is no error > if "filter..required = false" > --> 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