From: Stefan Beller <sbeller@google.com> To: Jeff King <peff@peff.net> Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Duy Nguyen <pclouds@gmail.com>, Junio C Hamano <gitster@pobox.com> Subject: Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack Date: Wed, 27 May 2015 10:40:37 -0700 Message-ID: <CAGZ79kYaoViDrz9TKXWebif4mfyUjzJ6b3id8ozTqtwsmjAC1A@mail.gmail.com> (raw) In-Reply-To: <20150527063558.GB885@peff.net> On Tue, May 26, 2015 at 11:35 PM, Jeff King <peff@peff.net> wrote: > On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote: > >> --- a/upload-pack.c >> +++ b/upload-pack.c >> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref) >> strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util); >> } >> >> -static const char *advertise_capabilities = "multi_ack thin-pack side-band" >> +static char *advertise_capabilities = "multi_ack thin-pack side-band" >> " side-band-64k ofs-delta shallow no-progress" >> " include-tag multi_ack_detailed"; > > If we are upload-pack-2, should we advertise that in the capabilities? I > think it may make things easier later if we try to provide some > opportunistic out-of-band data. E.g., if see tell git-daemon: > > git-upload-pack repo\0host=whatever\0\0version=2 > > how do we know whether version=2 was understood and kicked us into v2 > mode, versus an old server that ignored it? So in my vision we would call git-upload-pack-2 instead of having a "version=2". and if git-upload-pack-2 doesn't exist, the whole conversation is over, the client it is left to make up some good error message or retry version 1. But I think advertising both which versions the server could deal with, as well as the currently expected version is a good thing. capability: can_speak=1,2 capability: speaking_now=2 > > It also just makes the protocol more self-describing; from the > conversation you can see which version is in use. > >> +static void send_capabilities(void) >> +{ >> + char buf[100]; >> + >> + while (next_capability(buf)) >> + packet_write(1, "capability:%s\n", buf); > > Having a static-sized buffer and then passing it to a function which has > no idea of the buffer size seems like a recipe for a buffer overflow. :) Yes. All patches I proposed are very brittle. My first goal was to have the last test passing (an actual fetch with version 2). Now I will start looking at making things robust, as by now you all seem to not disagree totally. > > You are fine here because you are parsing the hard-coded capabilities > string, and we know 100 is enough there. But it's nice to avoid such > magic. > > Like Eric, I find the whole next_capability thing a little ugly. His > suggestion to pass in the parsing state is an improvement, but I wonder > why we need to parse at all. Can we keep the capabilities as: > > const char *capabilities[] = { > "multi_ack", > "thin-pack", > ... etc ... > }; > > and then loop over the array? The v1 writer will need to be modified, > but it should be fairly straightforward (loop and add them to the buffer > rather than dumping the whole string). Thanks for the design guidance! I'll change that. > > Also, do we need the capability noise-word? I thought it opens up a new possible door in the future. As we ignore anything not starting with "capability" for now, you could introduce your foo and bar ping pong easily and still be version 2 compatible. S: capability: thin S: capability: another-capability S: ping-pong foo S: done C: (not having understood ping-pong) just answering with capability: thin C: done, let's proceed to refs advertisement The alternative client would do: C: ping-pong: foo-data1a S: ping-pong: foo-data1b C: ping-pong: foo-data2a C: capability: thin ... > They all have it, except > for: > >> + packet_write(1, "agent:%s\n", git_user_agent_sanitized()); > > But isn't that basically a capability, too (I agree it is stretching the > definition of "capability", but I think that ship has sailed; the > client's response is not "I support this, too" but "I want to enable > this"). > > IOW, is there a reason that the initial conversation is not just: > > pkt-line("multi_ack"); > pkt-line("thin-pack"); > ... > pkt-line("agent=v2.5.0"); > pkt-line(0000); > > We already have framing for each capability due to the use of pkt-line. > The "capability:" is just one more thing the client has to parse past. > The keys are already unique up to any "=", so I don't think there is any > ambiguity (e.g., we don't care about "capability:agent"; we have already > assigned a meaning to the term "agent", and will never introduce a > standalone capability with the same name). It looks clearer to me (we're not wasting band-width), maybe this ping pong thing can be part of the capabilities announcement too, so we're good this way. > > Likewise, if we introduce new protocol items here, the client should > either ignore them (if it does not understand them) or know what to do > with them. > >> +static void receive_capabilities(void) >> +{ >> + int done = 0; >> + while (1) { >> + char *line = packet_read_line(0, NULL); >> + if (!line) >> + break; >> + if (starts_with(line, "capability:")) >> + parse_features(line + strlen("capability:")); >> + } > > Use: > > const char *cap; > if (skip_prefix(line, "capability:", &cap)) > parse_features(cap); > > Or better yet, if you take my suggestion above: > > parse_features(line); will do. > > :) > >> +static int endswith(const char *teststring, const char *ending) >> +{ >> + int slen = strlen(teststring); >> + int elen = strlen(ending); >> + return !strcmp(teststring + slen - elen, ending); >> +} > > Eric mentioned the underflow problems here, but I wonder even more: > what's wrong with the global ends_with() that we already provide? I was missing knowledge we have that, and apparently I was thinking it's faster to come up with my own version than to look for it. :) > > -Peff
next prev parent reply other threads:[~2015-05-27 17:40 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-05-26 22:01 [RFC/WIP PATCH 00/11] Protocol version 2, again! Stefan Beller 2015-05-26 22:01 ` [RFC/WIP PATCH 01/11] upload-pack: make client capability parsing code a separate function Stefan Beller 2015-05-26 22:01 ` [RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first "want" line Stefan Beller 2015-05-26 22:17 ` Junio C Hamano 2015-05-26 22:20 ` Stefan Beller 2015-05-26 22:01 ` [RFC/WIP PATCH 03/11] upload-pack: move capabilities out of send_ref Stefan Beller 2015-05-26 22:01 ` [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack Stefan Beller 2015-05-27 2:30 ` Eric Sunshine 2015-05-27 6:35 ` Jeff King 2015-05-27 17:30 ` Eric Sunshine 2015-05-27 20:14 ` Jeff King 2015-05-27 17:40 ` Stefan Beller [this message] 2015-05-27 20:34 ` Jeff King 2015-05-27 20:45 ` Stefan Beller 2015-05-27 21:46 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number Stefan Beller 2015-05-27 6:39 ` Jeff King 2015-05-27 19:01 ` Stefan Beller 2015-05-27 20:17 ` Jeff King 2015-05-27 19:10 ` Junio C Hamano 2015-05-26 22:01 ` [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities Stefan Beller 2015-05-27 3:25 ` Eric Sunshine 2015-05-27 6:50 ` Jeff King 2015-05-27 17:19 ` Eric Sunshine 2015-05-27 20:09 ` Jeff King 2015-05-27 6:45 ` Jeff King 2015-05-29 19:39 ` Stefan Beller 2015-05-29 22:08 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol Stefan Beller 2015-05-26 22:19 ` Junio C Hamano 2015-05-26 22:23 ` Stefan Beller 2015-05-27 6:53 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number Stefan Beller 2015-05-26 22:21 ` Junio C Hamano 2015-05-26 22:31 ` Stefan Beller 2015-05-27 5:09 ` Junio C Hamano 2015-05-27 6:56 ` Jeff King 2015-05-27 3:33 ` Eric Sunshine 2015-05-27 7:02 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs Stefan Beller 2015-05-27 5:37 ` Eric Sunshine 2015-05-27 7:06 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol Stefan Beller 2015-05-27 5:34 ` Eric Sunshine 2015-05-27 7:12 ` Jeff King 2015-05-26 22:01 ` [RFC/WIP PATCH 11/11] Document protocol version 2 Stefan Beller 2015-05-29 20:35 ` Junio C Hamano 2015-05-29 21:36 ` Stefan Beller 2015-05-29 21:52 ` Junio C Hamano 2015-05-29 22:21 ` Jeff King 2015-06-01 23:14 ` Stefan Beller 2015-06-01 23:40 ` Stefan Beller 2015-06-04 13:18 ` Jeff King 2015-06-04 17:01 ` Junio C Hamano 2015-06-02 17:06 ` Junio C Hamano 2015-05-27 6:18 ` [RFC/WIP PATCH 00/11] Protocol version 2, again! Jeff King 2015-05-27 7:08 ` Jeff King 2015-06-01 17:49 ` Stefan Beller 2015-06-02 10:10 ` Duy Nguyen 2015-06-04 13:09 ` Jeff King 2015-06-04 16:44 ` Stefan Beller
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGZ79kYaoViDrz9TKXWebif4mfyUjzJ6b3id8ozTqtwsmjAC1A@mail.gmail.com \ --to=sbeller@google.com \ --cc=git@vger.kernel.org \ --cc=gitster@pobox.com \ --cc=pclouds@gmail.com \ --cc=peff@peff.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git