RFC patch, 11 partsupload-pack: make client capability parsing code a separate function

> Stefan Beller <sbeller@google.com> writes:
>
>> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> pack-protocol.txt says so and fetch-pack also follows it even though
>> upload-pack is a bit lax. Fix it.
>
> Hmm, I actually think the .txt file unsuccessfully tried to close
> the barn door after horse has long left.  The existing clients that
> read protocol capabilities keep the last-read copy and then
> overwrite it when a new one came, which is why we couldn't update
> the protocol by sending only incremental changes, etc. without
> breaking existing clients.

> In upload-pack-2 we send each capability in its own packet.
> By reusing the advertise_capabilities and eventually setting it to
> NULL we will be able to reuse the methods for refs advertisement.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/upload-pack-2.c b/upload-pack-2.c
> new file mode 120000
> index 0000000..e30a871
> --- /dev/null
> +++ b/upload-pack-2.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";
>
> +/*
> + * Reads the next capability and puts it into dst as a null terminated string.
> + * Returns true if more capabilities can be read.
> + * */
> +static int next_capability(char *dst)
> +{
> +       int len = 0;
> +       if (!*advertise_capabilities) {
> +               /* make sure to not advertise capabilities afterwards */
> +               advertise_capabilities = NULL;

> +               return 0;
> +       }
> +
> +       while (advertise_capabilities[len] != '\0' &&
> +              advertise_capabilities[len] != ' ')
> +               len ++;

> +       strncpy(dst, advertise_capabilities, len);
> +       dst[len] = '\0';
> +
> +       advertise_capabilities += len;
> +       if (*advertise_capabilities == ' ')
> +               advertise_capabilities++;

> +static void send_capabilities(void)
> +{
> +       char buf[100];
> +
> +       while (next_capability(buf))
> +               packet_write(1, "capability:%s\n", buf);
> +
> +       packet_write(1, "agent:%s\n", git_user_agent_sanitized());
> +       packet_flush(1);
> +}
> +
>  static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
>  {
>
> @@ -794,6 +831,28 @@ static void upload_pack(void)
>         }
>  }
>
> +static void receive_capabilities(void)
> +{
> +       int done = 0;
> +       while (1) {
> +               char *line = packet_read_line(0, NULL);

> +       }
> +}
> +
> +static void upload_pack_version_2(void)
> +{
> +       send_capabilities();
> +       receive_capabilities();
> +
> +       /* The rest of the protocol stays the same, capabilities advertising
> +          is disabled though. */

> +       upload_pack();
> +}
> +
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
>         if (!strcmp("uploadpack.allowtipsha1inwant", var))
> @@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>         return parse_hide_refs_config(var, value, "uploadpack");
>  }
>
> +static int endswith(const char *teststring, const char *ending)
> +{
> +       int slen = strlen(teststring);
> +       int elen = strlen(ending);

> +       return !strcmp(teststring + slen - elen, ending);
> +}
> +
>  int main(int argc, char **argv)
>  {
>         char *dir;
> +       const char *cmd;
>         int i;
>         int strict = 0;
>
>         git_setup_gettext();
>
>         packet_trace_identity("upload-pack");
> -       git_extract_argv0_path(argv[0]);
> +       cmd = git_extract_argv0_path(argv[0]);
>         check_replace_refs = 0;
>
>         for (i = 1; i < argc; i++) {
> @@ -857,6 +924,10 @@ int main(int argc, char **argv)
>                 die("'%s' does not appear to be a git repository", dir);
>
>         git_config(upload_pack_config, NULL);
> -       upload_pack();
> +
> +       if (endswith(cmd, "-2"))
> +               upload_pack_version_2();
> +       else
> +               upload_pack();
>         return 0;
>  }
> --
> 2.4.1.345.gab207b6.dirty

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

> +static void send_capabilities(void)
> +{
> +	char buf[100];
> +
> +	while (next_capability(buf))
> +		packet_write(1, "capability:%s\n", buf);

> +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:"));
> +	}

> +static int endswith(const char *teststring, const char *ending)
> +{
> +	int slen = strlen(teststring);
> +	int elen = strlen(ending);
> +	return !strcmp(teststring + slen - elen, ending);
> +}

> On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:
>> +static void send_capabilities(void)
>> +{
>> +     char buf[100];
>> +
>> +     while (next_capability(buf))
>> +             packet_write(1, "capability:%s\n", buf);
>
> 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?

> > 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?
> 
> Yes, that would be much nicer. I also had this in mind but didn't know
> if there was a strong reason for the capabilities to be shoehorned
> into a single string as they are currently.

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

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

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

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

>
> 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);

>
> :)
>
>> +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?

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

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

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

> On Wed, May 27, 2015 at 10:40:37AM -0700, Stefan Beller wrote:
>
>> > 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.
>
> I'd like for that to be a starting point for us, and then to be able to
> add-on "hints" to ease the transition path in whatever way we want. We
> may even not do that in the long run, but I want to leave the door open
> if we can.
>
>> 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
>
> I was thinking just "speaking_now=2", but it probably makes sense to do
> both. I do not think using it to "downgrade" will ever be particularly
> useful (certainly not from v2 to v1, since to understand the flag both
> sides must be v2 in the first place). But advertising that via the v1
> conversation will be a good way to tell the other side that upgrade is
> possible.

>
>> > 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
>> ...
>
> Right, but I think (and please correct me if there's a case I'm missing)
> that the behavior is the same whether it is spelled "ping-pong" or
> "capability:ping-pong". That is, the rule for "capability:" is "if you
> do not understand it, ignore it and do not mention it in your
> capabilities; the server is required to assume you were written before
> that capability was invented". But that is _also_ the rule for
> ping-pong, I think.

>
>> > 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. :)
>
> Makes sense. :)
>
> -Peff

> > Right, but I think (and please correct me if there's a case I'm
> > missing) that the behavior is the same whether it is spelled
> > "ping-pong" or "capability:ping-pong". That is, the rule for
> > "capability:" is "if you do not understand it, ignore it and do not
> > mention it in your capabilities; the server is required to assume
> > you were written before that capability was invented". But that is
> > _also_ the rule for ping-pong, I think.
> 
> The rules are the same, right. But the allowed characters are limited
> (in theory) as the regular expressions given for the capabilities
> don't allow for binary data for example, but only well formed ASCII
> text, space separated.  The "ping-pong" keyword could introduce a
> binary stream there including line feeds. (Today it sounds like a
> stupid idea though)

> On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:
>
>> +     OPT_STRING('y', "transport-version", &transport_version,
>> +                N_("transport-version"),
>> +                N_("specify transport version to be used")),
>
> Interesting choice for the short option ("-v" would be nice, but
> obviously it is taken). Do we want to delay on claiming the
> short-and-sweet 'y' until we are sure this is something people will use
> a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks
> become good enough that nobody bothers to specify it manually).

> > Interesting choice for the short option ("-v" would be nice, but
> > obviously it is taken). Do we want to delay on claiming the
> > short-and-sweet 'y' until we are sure this is something people will use
> > a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks
> > become good enough that nobody bothers to specify it manually).
> [...]
> Or do you rather hint on dropping the short option at all, and just having NULL
> in the field?

> On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:
>
>> +	OPT_STRING('y', "transport-version", &transport_version,
>> +		   N_("transport-version"),
>> +		   N_("specify transport version to be used")),
>
> Interesting choice for the short option ("-v" would be nice, but
> obviously it is taken). Do we want to delay on claiming the
> short-and-sweet 'y' until we are sure this is something people will use
> a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks
> become good enough that nobody bothers to specify it manually).

> Instead of calling get_remote_heads as a first command during the
> protocol exchange, we need to have fine grained control over the
> capability negotiation in version 2 of the protocol.
>
> Introduce get_remote_capabilities, which will just listen to
> capabilities of the remote and request_capabilities which will
> tell the selection of capabilities to the remote.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/connect.c b/connect.c
> index c0144d8..bb17618 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref *ref)
>         string_list_clear(&symref, 0);
>  }
>
> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
> +{
> +       struct strbuf capabilities_string = STRBUF_INIT;
> +       for (;;) {
> +               int len;
> +               char *line = packet_buffer;
> +               const char *arg;
> +
> +               len = packet_read(in, &src_buf, &src_len,
> +                                 packet_buffer, sizeof(packet_buffer),
> +                                 PACKET_READ_GENTLE_ON_EOF |
> +                                 PACKET_READ_CHOMP_NEWLINE);
> +               if (len < 0)
> +                       die_initial_contact(0);
> +
> +               if (!len)
> +                       break;
> +
> +               if (len > 4 && skip_prefix(line, "ERR ", &arg))

> +                       strbuf_addch(&capabilities_string, ' ');
> +               }
> +       }
> +       free(server_capabilities);
> +       server_capabilities = xmalloc(capabilities_string.len + 1);
> +       server_capabilities = strbuf_detach(&capabilities_string, NULL);

> +}
> +
> +int request_capabilities(int out)
> +{
> +       fprintf(stderr, "request_capabilities\n");
> +       // todo: send our capabilities
> +       packet_write(out, "capability:foo");
> +       packet_flush(out);
> +}
> +
>  /*
> - * Read all the refs from the other end
> + * Read all the refs from the other end,
> + * in is a file descriptor input stream
> + * src_buf and src_len may be an alternative way to specify the input.

>   */
>  struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>                               struct ref **list, unsigned int flags,
> diff --git a/remote.h b/remote.h
> index 04e2310..7381ddf 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);
>  void free_refs(struct ref *ref);
>
>  struct sha1_array;
> +
> +extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);
> +extern int request_capabilities(int out);
>  extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>                                      struct ref **list, unsigned int flags,
>                                      struct sha1_array *extra_have,
> --
> 2.4.1.345.gab207b6.dirty

> > +               len = packet_read(in, &src_buf, &src_len,
> > +                                 packet_buffer, sizeof(packet_buffer),
> > +                                 PACKET_READ_GENTLE_ON_EOF |
> > +                                 PACKET_READ_CHOMP_NEWLINE);
> > +               if (len < 0)
> > +                       die_initial_contact(0);
> > +
> > +               if (!len)
> > +                       break;
> > +
> > +               if (len > 4 && skip_prefix(line, "ERR ", &arg))
> 
> The 'len > 4' check is needed because there's no guarantee that 'line'
> is NUL-terminated. Correct?

> On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote:
>
>> > +               len = packet_read(in, &src_buf, &src_len,
>> > +                                 packet_buffer, sizeof(packet_buffer),
>> > +                                 PACKET_READ_GENTLE_ON_EOF |
>> > +                                 PACKET_READ_CHOMP_NEWLINE);
>> > +               if (len < 0)
>> > +                       die_initial_contact(0);
>> > +
>> > +               if (!len)
>> > +                       break;
>> > +
>> > +               if (len > 4 && skip_prefix(line, "ERR ", &arg))
>>
>> The 'len > 4' check is needed because there's no guarantee that 'line'
>> is NUL-terminated. Correct?
>
> I think this was just blindly copied from get_remote_heads(). And I
> think that code was being overly paranoid. Ever since f3a3214 (Make
> send/receive-pack be closer to doing something interesting, 2005-06-29),
> the pkt-line reader will add an extra NUL to the buffer to ease cases
> like this.

> >> The 'len > 4' check is needed because there's no guarantee that 'line'
> >> is NUL-terminated. Correct?
> >
> > I think this was just blindly copied from get_remote_heads(). And I
> > think that code was being overly paranoid. Ever since f3a3214 (Make
> > send/receive-pack be closer to doing something interesting, 2005-06-29),
> > the pkt-line reader will add an extra NUL to the buffer to ease cases
> > like this.
> 
> Thanks. I had started digging into packet_read() to determine whether
> it guaranteed NUL-termination, but didn't get far enough to decide. I
> agree that if NUL-termination is guaranteed, then the 'len > 4' check
> is superfluous (and confusing, which is why it caught my attention in
> the first place).

> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
> +{
> +	struct strbuf capabilities_string = STRBUF_INIT;
> +	for (;;) {
> +		int len;
> +		char *line = packet_buffer;
> +		const char *arg;
> +
> +		len = packet_read(in, &src_buf, &src_len,
> +				  packet_buffer, sizeof(packet_buffer),
> +				  PACKET_READ_GENTLE_ON_EOF |
> +				  PACKET_READ_CHOMP_NEWLINE);
> +		if (len < 0)
> +			die_initial_contact(0);
> +
> +		if (!len)
> +			break;
> +
> +		if (len > 4 && skip_prefix(line, "ERR ", &arg))
> +			die("remote error: %s", arg);
> +
> +		if (starts_with(line, "capability:")) {
> +			strbuf_addstr(&capabilities_string, line + strlen("capability:"));
> +			strbuf_addch(&capabilities_string, ' ');
> +		}
> +	}

> On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:
>
>> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
>> +{
>> +     struct strbuf capabilities_string = STRBUF_INIT;
>> +     for (;;) {
>> +             int len;
>> +             char *line = packet_buffer;
>> +             const char *arg;
>> +
>> +             len = packet_read(in, &src_buf, &src_len,
>> +                               packet_buffer, sizeof(packet_buffer),
>> +                               PACKET_READ_GENTLE_ON_EOF |
>> +                               PACKET_READ_CHOMP_NEWLINE);
>> +             if (len < 0)
>> +                     die_initial_contact(0);
>> +
>> +             if (!len)
>> +                     break;
>> +
>> +             if (len > 4 && skip_prefix(line, "ERR ", &arg))
>> +                     die("remote error: %s", arg);
>> +
>> +             if (starts_with(line, "capability:")) {
>> +                     strbuf_addstr(&capabilities_string, line + strlen("capability:"));
>> +                     strbuf_addch(&capabilities_string, ' ');
>> +             }
>> +     }
>
> I think this is the reverse case of next_capabilities in the upload-pack
> side, so I'll make the reverse suggestion. :) Would it make things nicer
> if both v1 and v2 parsed the capabilities into a string_list?

>
>> +     free(server_capabilities);
>> +     server_capabilities = xmalloc(capabilities_string.len + 1);
>> +     server_capabilities = strbuf_detach(&capabilities_string, NULL);
>
> Is this xmalloc line left over? The strbuf_detach will write over it.

>
>> +     strbuf_release(&capabilities_string);
>
> No need to release if we just detached.
>
>> +int request_capabilities(int out)
>> +{
>> +     fprintf(stderr, "request_capabilities\n");
>
> Debug cruft, I presume.
>
>> +     // todo: send our capabilities
>> +     packet_write(out, "capability:foo");
>
> No C99 comments. But I think this is just a placeholder. :)
>
> -Peff

> > I think this is the reverse case of next_capabilities in the upload-pack
> > side, so I'll make the reverse suggestion. :) Would it make things nicer
> > if both v1 and v2 parsed the capabilities into a string_list?
> 
> Ok, I'll do that. Though this makes future enhancements a bit uneasy.
> Say we want to transport a message by the server admins, this might be
> the right place to do.
> 
>     if (starts_with("message"))
>         fprintf(stderr, ....
> 
> Of course we can later add this in the future, but it would break older
> clients (clients as of this patch series). That's why I like the idea of
> adding a prefix here. Maybe just a "c:" as an abbreviation for capability.

> @@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  		if (!conn)
>  			return args.diag_url ? 0 : 1;
>  	}
> -	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +
> +	switch (args.version) {
> +	default:
> +	case 2:
> +		get_remote_capabilities(fd[0], NULL, 0);
> +		request_capabilities(fd[1]);
> +		break;
> +	case 1: /* fall through */
> +	case 0:
> +		get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +		break;
> +	}

> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>>               if (!conn)
>>                       return args.diag_url ? 0 : 1;
>>       }
>> -     get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
>> +
>> +     switch (args.version) {
>> +     default:
>> +     case 2:
>> +             get_remote_capabilities(fd[0], NULL, 0);
>> +             request_capabilities(fd[1]);
>> +             break;

>> +     case 1: /* fall through */
>> +     case 0:
>> +             get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
>> +             break;
>> +     }
>
> Nice ;-)

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 4a6b340..32dc8b0 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -127,6 +127,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  			args.update_shallow = 1;
>  			continue;
>  		}
> +		if (!strcmp("--transport-version", arg)) {
> +			args.version = strtol(arg + strlen("--transport-version"), NULL, 0);
> +			continue;
> +		}

> -	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +
> +	switch (args.version) {
> +	default:
> +	case 2:
> +		get_remote_capabilities(fd[0], NULL, 0);
> +		request_capabilities(fd[1]);
> +		break;

> +	if (transport->smart_options
> +	    && transport->smart_options->transport_version) {
> +		buf = xmalloc(strlen(remote_program) + 12);
> +		sprintf(buf, "%s-%d", remote_program,
> +			transport->smart_options->transport_version);
> +		remote_program = buf;
> +	}

> Stefan Beller <sbeller@google.com> writes:
>
>> +     if (transport->smart_options
>> +         && transport->smart_options->transport_version) {
>> +             buf = xmalloc(strlen(remote_program) + 12);
>> +             sprintf(buf, "%s-%d", remote_program,
>> +                     transport->smart_options->transport_version);
>> +             remote_program = buf;
>> +     }
>
> Bikeshedding: so the versioning scheme is that the current one is
> zero, and the next one is two?  I would have expected that there
> would be something like

>
>         if (...->version < 2) {
>                 ... append "-%d" ...
>         }
>
> involved.

> On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>
>>         if (...->version < 2) {
>>                 ... append "-%d" ...
>>         }
>>
>> involved.
>
> Oh! I see here you would count the current one as 1, which has no
> number extension, and any further would have a -${version}. That
> would transport the intention much better I guess.

> Stefan Beller <sbeller@google.com> writes:
> 
> > On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> >>
> >>         if (...->version < 2) {
> >>                 ... append "-%d" ...
> >>         }
> >>
> >> involved.
> >
> > Oh! I see here you would count the current one as 1, which has no
> > number extension, and any further would have a -${version}. That
> > would transport the intention much better I guess.
> 
> Yeah, except that I screwed up my comparison.  Obviously, I should
> have said "If version is 2 or later, then append -%d to the name,
> otherwise use the name as-is".

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/transport.c b/transport.c
> index 3ef15f6..33644a6 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,
>  static int connect_setup(struct transport *transport, int for_push, int verbose)
>  {
>         struct git_transport_data *data = transport->data;
> +       const char *remote_program;
> +       char *buf = 0;

>         if (data->conn)
>                 return 0;
>
> +       remote_program = (for_push ? data->options.receivepack
> +                                  : data->options.uploadpack);
> +
> +       if (transport->smart_options
> +           && transport->smart_options->transport_version) {
> +               buf = xmalloc(strlen(remote_program) + 12);
> +               sprintf(buf, "%s-%d", remote_program,
> +                       transport->smart_options->transport_version);
> +               remote_program = buf;
> +       }
> +
>         data->conn = git_connect(data->fd, transport->url,
> -                                for_push ? data->options.receivepack :
> -                                data->options.uploadpack,
> +                                remote_program,
>                                  verbose ? CONNECT_VERBOSE : 0);
>
> +       free(buf);
> +
>         return 0;
>  }
>
> --
> 2.4.1.345.gab207b6.dirty

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  transport.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/transport.c b/transport.c
> index 3ef15f6..33644a6 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,
>  static int connect_setup(struct transport *transport, int for_push, int verbose)
>  {
>  	struct git_transport_data *data = transport->data;
> +	const char *remote_program;
> +	char *buf = 0;

> +	if (transport->smart_options
> +	    && transport->smart_options->transport_version) {
> +		buf = xmalloc(strlen(remote_program) + 12);
> +		sprintf(buf, "%s-%d", remote_program,
> +			transport->smart_options->transport_version);
> +		remote_program = buf;
> +	}

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  transport.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 33644a6..1cd9b77 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -526,12 +526,33 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
>  {
>         struct git_transport_data *data = transport->data;
>         struct ref *refs;
> +       int version = 0;
>
> +       if (transport->smart_options)
> +               version = transport->smart_options->transport_version;
>         connect_setup(transport, for_push, 0);
> -       get_remote_heads(data->fd[0], NULL, 0, &refs,
> -                        for_push ? REF_NORMAL : 0,
> -                        &data->extra_have,
> -                        &data->shallow);
> +       switch (version) {
> +               default: /*
> +                         * Configured a protocol version > 2?
> +                         * Try version 2 as it's the most future proof.
> +                         */
> +                       /* fall through */
> +               case 2: /* first talk about capabilities, then get the heads */
> +                       get_remote_capabilities(data->fd[0], NULL, 0);
> +                       request_capabilities(data->fd[1]);
> +                       get_remote_heads(data->fd[0], NULL, 0, &refs,
> +                                        for_push ? REF_NORMAL : 0,
> +                                        &data->extra_have,
> +                                        &data->shallow);
> +                       break;
> +               case 1: /* configured version 1, fall through */
> +               case 0: /* unconfigured, use first protocol */
> +                       get_remote_heads(data->fd[0], NULL, 0, &refs,
> +                                        for_push ? REF_NORMAL : 0,
> +                                        &data->extra_have,
> +                                        &data->shallow);
> +                       break;
> +       }
>         data->got_remote_heads = 1;
>
>         return refs;
> --
> 2.4.1.345.gab207b6.dirty

> +	switch (version) {
> +		default: /*
> +			  * Configured a protocol version > 2?
> +			  * Try version 2 as it's the most future proof.
> +			  */
> +			/* fall through */

> +		case 2: /* first talk about capabilities, then get the heads */
> +			get_remote_capabilities(data->fd[0], NULL, 0);
> +			request_capabilities(data->fd[1]);
> +			get_remote_heads(data->fd[0], NULL, 0, &refs,
> +					 for_push ? REF_NORMAL : 0,
> +					 &data->extra_have,
> +					 &data->shallow);
> +			break;
> +		case 1: /* configured version 1, fall through */
> +		case 0: /* unconfigured, use first protocol */
> +			get_remote_heads(data->fd[0], NULL, 0, &refs,
> +					 for_push ? REF_NORMAL : 0,
> +					 &data->extra_have,
> +					 &data->shallow);
> +			break;
> +	}

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/t/t5544-fetch-2.sh b/t/t5544-fetch-2.sh
> new file mode 100755
> index 0000000..beee46c
> --- /dev/null
> +++ b/t/t5544-fetch-2.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2015 Stefan Beller
> +#
> +
> +test_description='Testing version 2 of the fetch protocol'
> +
> +. ./test-lib.sh
> +
> +mk_repo_pair () {
> +       rm -rf client server &&
> +       test_create_repo client &&
> +       test_create_repo server &&
> +       (
> +               cd server &&
> +               git config receive.denyCurrentBranch warn
> +       ) &&
> +       (
> +               cd client &&
> +               git remote add origin ../server

> +               git config remote.origin.transportversion 2
> +       )
> +}
> +
> +test_expect_success 'setup' '
> +       mk_repo_pair &&
> +       (
> +               cd server &&
> +               test_commit one
> +       ) &&
> +       (
> +               cd client &&
> +               git fetch origin master
> +       )
> +'
> +
> +# More to come here, similar to t5515 having a sub directory full of expected
> +# data going over the wire.
> +
> +test_done
> --
> 2.4.1.345.gab207b6.dirty

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t5544-fetch-2.sh | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100755 t/t5544-fetch-2.sh

> @@ -1,11 +1,11 @@
>  Packfile transfer protocols
>  ===========================
>  
> -Git supports transferring data in packfiles over the ssh://, git:// and
> +Git supports transferring data in packfiles over the ssh://, git://, http:// and

> +Capability discovery (v2)
> +-------------------------
> ...
> +  capability-list  =  *(capability) [agent LF] flush-pkt
> +  capability       =  PKT-LINE("capability:" keyvaluepair LF)
> +  agent            =  keyvaluepair LF
> +  keyvaluepair     =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")

> +  LC_ALPHA         =  %x61-7A
> +----
> +
> +The client MUST ignore any data on pkt-lines starting with anything
> +different than "capability" for future ease of extension.
> +
> +The client MUST NOT ask for capabilities the server did not say it
> +supports. The server MUST diagnose and abort if capabilities it does
> +not understand was requested. The server MUST NOT ignore capabilities
> +that client requested and server advertised.  As a consequence of these
> +rules, server MUST NOT advertise capabilities it does not understand.

> @@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised
>  ref.  If HEAD is not a valid ref, HEAD MUST NOT appear in the
>  advertisement list at all, but other refs may still appear.
>  
> -The stream MUST include capability declarations behind a NUL on the
> -first ref. The peeled value of a ref (that is "ref^{}") MUST be
> -immediately after the ref itself, if presented. A conforming server
> -MUST peel the ref if it's an annotated tag.
> +In version 1 the stream MUST include capability declarations behind
> +a NUL on the first ref. The peeled value of a ref (that is "ref^{}")
> +MUST be immediately after the ref itself, if presented. A conforming
> +server MUST peel the ref if it's an annotated tag.
> +
> +In version 2 the capabilities are already negotiated, so the first ref
> +MUST NOT be followed by any capability advertisement, but it should be
> +treated as any other refs advertising line.

> @@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.
>    shallow          =  PKT-LINE("shallow" SP obj-id)
>  
>    capability-list  =  capability *(SP capability)
> -  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_")
> +  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")

> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -1,11 +1,11 @@
>>  Packfile transfer protocols
>>  ===========================
>>
>> -Git supports transferring data in packfiles over the ssh://, git:// and
>> +Git supports transferring data in packfiles over the ssh://, git://, http:// and
>
> When you have chance, can you do things like this, which is a clear
> improvement of the current document even if we never had v2, as
> separate patches?

>
>> +Capability discovery (v2)
>> +-------------------------
>> ...
>> +  capability-list  =  *(capability) [agent LF] flush-pkt
>> +  capability       =  PKT-LINE("capability:" keyvaluepair LF)
>> +  agent            =  keyvaluepair LF
>> +  keyvaluepair     =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
>
> What is the "=" doing there?  If you meant to cover things like
> "lang=en" with this, I do not think it is a good idea.  Rather, it
> should be more like this:
>
>         capability = 1*(LC_ALPHA / DIGIT / "-" / "_") [ "=" value ]
>         value = 0*( any octet other than LF, NUL )
>
> in order to leave us wiggle room to have more than very limited
> subset of US-ASCII in 'value'.  I suspect that we may want to allow
> anything other than LF (unlike v1 that allowed anything other than
> SP and LF).

>
>> +  LC_ALPHA         =  %x61-7A
>> +----
>> +
>> +The client MUST ignore any data on pkt-lines starting with anything
>> +different than "capability" for future ease of extension.
>> +
>> +The client MUST NOT ask for capabilities the server did not say it
>> +supports. The server MUST diagnose and abort if capabilities it does
>> +not understand was requested. The server MUST NOT ignore capabilities
>> +that client requested and server advertised.  As a consequence of these
>> +rules, server MUST NOT advertise capabilities it does not understand.
>
> I think it was already discussed that we shouldn't do the
> "capability:" and "cap:" prefixes in reviews of earlier parts, so
> the details of this part would be updated?

>
>> @@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised
>>  ref.  If HEAD is not a valid ref, HEAD MUST NOT appear in the
>>  advertisement list at all, but other refs may still appear.
>>
>> -The stream MUST include capability declarations behind a NUL on the
>> -first ref. The peeled value of a ref (that is "ref^{}") MUST be
>> -immediately after the ref itself, if presented. A conforming server
>> -MUST peel the ref if it's an annotated tag.
>> +In version 1 the stream MUST include capability declarations behind
>> +a NUL on the first ref. The peeled value of a ref (that is "ref^{}")
>> +MUST be immediately after the ref itself, if presented. A conforming
>> +server MUST peel the ref if it's an annotated tag.
>> +
>> +In version 2 the capabilities are already negotiated, so the first ref
>> +MUST NOT be followed by any capability advertisement, but it should be
>> +treated as any other refs advertising line.
>
> Sensible.
>
>> @@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.
>>    shallow          =  PKT-LINE("shallow" SP obj-id)
>>
>>    capability-list  =  capability *(SP capability)
>> -  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_")
>> +  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
>
> Ditto.
>
> Thanks.

>>> +Capability discovery (v2)
>>> +-------------------------
>>> ...
>>> +  capability-list  =  *(capability) [agent LF] flush-pkt
>>> +  capability       =  PKT-LINE("capability:" keyvaluepair LF)
>>> +  agent            =  keyvaluepair LF
>>> +  keyvaluepair     =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
>>
>> What is the "=" doing there?  If you meant to cover things like
>> "lang=en" with this, I do not think it is a good idea.  Rather, it
>> should be more like this:
>>
>>         capability = 1*(LC_ALPHA / DIGIT / "-" / "_") [ "=" value ]
>>         value = 0*( any octet other than LF, NUL )
>>
>> in order to leave us wiggle room to have more than very limited
>> subset of US-ASCII in 'value'.  I suspect that we may want to allow
>> anything other than LF (unlike v1 that allowed anything other than
>> SP and LF).
>
> Currently we can do a = as part of the line after the first ref, such as
>
>     symref=HEAD:refs/heads/master agent=git/2:2.4.0
>
> so I thought we want to keep this.

> > Currently we can do a = as part of the line after the first ref, such as
> >
> >     symref=HEAD:refs/heads/master agent=git/2:2.4.0
> >
> > so I thought we want to keep this.
> 
> I do not understand that statement.
> 
> Capability exchange in v2 is one packet per cap, so the above
> example would be expressed as:
> 
> 	symref=HEAD:refs/heads/master
>         agent=git/2:2.4.0
> 
> right?  Your "keyvaluepair" is limited to [a-z0-9-_=]*, and neither
> of the above two can be expressed with that, which was why I said
> you need two different set of characters before and after "=".  Left
> hand side of "=" is tightly limited and that is OK.  Right hand side
> may contain characters like ':', '.' and '/', so your alphabet need
> to be more lenient, even in v1 (which I would imagine would be "any
> octet other than SP, LF and NUL").

> On Fri, May 29, 2015 at 02:52:14PM -0700, Junio C Hamano wrote:
>
>> > Currently we can do a = as part of the line after the first ref, such as
>> >
>> >     symref=HEAD:refs/heads/master agent=git/2:2.4.0
>> >
>> > so I thought we want to keep this.
>>
>> I do not understand that statement.
>>
>> Capability exchange in v2 is one packet per cap, so the above
>> example would be expressed as:
>>
>>       symref=HEAD:refs/heads/master
>>         agent=git/2:2.4.0
>>
>> right?  Your "keyvaluepair" is limited to [a-z0-9-_=]*, and neither
>> of the above two can be expressed with that, which was why I said
>> you need two different set of characters before and after "=".  Left
>> hand side of "=" is tightly limited and that is OK.  Right hand side
>> may contain characters like ':', '.' and '/', so your alphabet need
>> to be more lenient, even in v1 (which I would imagine would be "any
>> octet other than SP, LF and NUL").

>
> Yes. See git_user_agent_sanitized(), for example, which allows basically
> any printable ASCII except for SP.
>
> I think the v2 capabilities do not even need to have that restriction.
> It can allow arbitrary binary data, because it has an 8bit-clean framing
> mechanism (pkt-lines). Of course, that means such capabilities cannot be
> represented in a v1 conversation (whose framing mechanism involves SP
> and NUL). But it's probably acceptable to introduce new capabilities
> which are only available in a v2 conversation. Old clients that do not
> understand v2 would not understand the capability either. It does
> require new clients implementing the capability to _also_ implement v2
> if they have not done so, but I do not mind pushing people in that
> direction.
>
> The initial v2 client implementation should probably do a few cautionary
> things, then:
>
>   1. Do _not_ fold the per-pkt capabilities into a v1 string; that loses
>      the robust framing. I suggested string_list earlier, but probably
>      we want a list of ptr/len pair, so that it can remain NUL-clean.
>
>   2. Avoid holding on to unknown packets longer than necessary. Some
>      capability pkt-lines may be arbitrarily large (up to 64K). If we do
>      not understand them during the v2 read of the capabilities, there
>      is no point hanging on to them. It's not _wrong_ to do so, but just
>      inefficient; if we know that clients will just throw away unknown
>      packets, then we can later introduce new packets with large data,
>      without worrying about wasting the client's resources.
>
>      I suspect it's not that big a deal either way, though. I have no
>      plans for sending a bunch of large packets, and anyway network
>      bandwidth is probably more precious than client memory.

> On Fri, May 29, 2015 at 3:21 PM, Jeff King <peff@peff.net> wrote:
>> On Fri, May 29, 2015 at 02:52:14PM -0700, Junio C Hamano wrote:
>>
>>> > Currently we can do a = as part of the line after the first ref, such as
>>> >
>>> >     symref=HEAD:refs/heads/master agent=git/2:2.4.0
>>> >
>>> > so I thought we want to keep this.
>>>
>>> I do not understand that statement.
>>>
>>> Capability exchange in v2 is one packet per cap, so the above
>>> example would be expressed as:
>>>
>>>       symref=HEAD:refs/heads/master
>>>         agent=git/2:2.4.0
>>>
>>> right?  Your "keyvaluepair" is limited to [a-z0-9-_=]*, and neither
>>> of the above two can be expressed with that, which was why I said
>>> you need two different set of characters before and after "=".  Left
>>> hand side of "=" is tightly limited and that is OK.  Right hand side
>>> may contain characters like ':', '.' and '/', so your alphabet need
>>> to be more lenient, even in v1 (which I would imagine would be "any
>>> octet other than SP, LF and NUL").
>
> I think the recent issue with the push certificates shows that having arbitrary
> data after the = is a bad idea. So we need to be very cautious when to allow
> which data after the =.
>
> I'll try split up the patch.
>
>>
>> Yes. See git_user_agent_sanitized(), for example, which allows basically
>> any printable ASCII except for SP.
>>
>> I think the v2 capabilities do not even need to have that restriction.
>> It can allow arbitrary binary data, because it has an 8bit-clean framing
>> mechanism (pkt-lines). Of course, that means such capabilities cannot be
>> represented in a v1 conversation (whose framing mechanism involves SP
>> and NUL). But it's probably acceptable to introduce new capabilities
>> which are only available in a v2 conversation. Old clients that do not
>> understand v2 would not understand the capability either. It does
>> require new clients implementing the capability to _also_ implement v2
>> if they have not done so, but I do not mind pushing people in that
>> direction.
>>
>> The initial v2 client implementation should probably do a few cautionary
>> things, then:
>>
>>   1. Do _not_ fold the per-pkt capabilities into a v1 string; that loses
>>      the robust framing. I suggested string_list earlier, but probably
>>      we want a list of ptr/len pair, so that it can remain NUL-clean.
>>
>>   2. Avoid holding on to unknown packets longer than necessary. Some
>>      capability pkt-lines may be arbitrarily large (up to 64K). If we do
>>      not understand them during the v2 read of the capabilities, there
>>      is no point hanging on to them. It's not _wrong_ to do so, but just
>>      inefficient; if we know that clients will just throw away unknown
>>      packets, then we can later introduce new packets with large data,
>>      without worrying about wasting the client's resources.
>>
>>      I suspect it's not that big a deal either way, though. I have no
>>      plans for sending a bunch of large packets, and anyway network
>>      bandwidth is probably more precious than client memory.
>
> That's very sensible thoughts after rereading this email. The version
> I'll be sending out today will not follow those suggestions though. :(

> So how does parse_capability scale w.r.t the number of capabilities?
> If parse_capability is just a linear search then it is O(n) and with n
> capabilities the client faces an O(n^2) computation which is bad. So
> if we were to require alphabetic capabilities, you could internally
> keep track and the whole operation is O(n). I just wonder if this is
> premature optimization or some thought we need to think of.

> My preference for a string suffix instead of a sequential number is
> motiviated by the discussion of sha1 vs sequential numbers to describe
> a state of a repository. The main difference here is however how often
> we expect changes. Version 1 of the protocol is current for 10 years by
> now, so I do not changes to the protocol number often, which makes it
> suitable for just having a counter appended to the binary.

> On Wed, May 27, 2015 at 02:18:18AM -0400, Jeff King wrote:
>
>> > The new protocol works just like the old protocol, except for
>> > the capabilities negotiation being before any exchange of real data.
>>
>> I like this approach. [...]
>
> So now I've read through all the patches. I still like it. :)
>
> There's a lot of work to be done still, but I think this is a great
> start. Thanks for getting the ball rolling.
>
> -Peff

> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack
> is a bit of a mystery to me, as I cannot fully grasp the difference between
>  * connect.{h,c}
>  * remote.{h.c}
>  * transport.{h.c}
> there. All of it seems to be doing network related stuff, but I have trouble
> getting the big picture. I am assuming all of these 3 are rather a low level,
> used like a library, though there must be even more hierarchy in there,
> connect is most low level judging from the header file and used by
> the other two.
> transport.h seems to provide the most toplevel library stuff as it includes
> remote.h in its header?

> The problem I am currently trying to tackle, is passing the options through all
> the layers early on. so in a few places we have code like
>
>     switch (version) {
>     case 2: /* first talk about capabilities, then get the heads */
>         get_remote_capabilities(data->fd[0], NULL, 0);
>         select_capabilities();
>         request_capabilities(data->fd[1]);
>         /* fall through */
>     case 1:
>         get_remote_heads(data->fd[0], NULL, 0, &refs,
>                  for_push ? REF_NORMAL : 0,
>                  &data->extra_have,
>                  &data->shallow);
>         break;
>     default:
>         die("BUG: Transport version %d not supported", version);
>         break;
>     }
>
> and the issue I am concerned about is the select_capabilities as well as
> the request_capabilities function here. The select_capabilities functionality
> is currently residing in the high level parts of the code as it both depends on
> the advertised server capabilities and on the user input (--use-frotz or config
> options), so the capability selection is done in fetchpack.c
>
> So there are 2 routes to go: Either we leave the select_capabilities in the
> upper layers (near the actual high level command, fetch, fetchpack) or we put
> it into the transport layer and just passing in a struct what the user desires.
> And when the users desire doesn't meet the server capabilities we die deep down
> in the transport layer.

> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack
> is a bit of a mystery to me, as I cannot fully grasp the difference between
>  * connect.{h,c}
>  * remote.{h.c}
>  * transport.{h.c}
> there. All of it seems to be doing network related stuff, but I have trouble
> getting the big picture. I am assuming all of these 3 are rather a low level,
> used like a library, though there must be even more hierarchy in there,
> connect is most low level judging from the header file and used by
> the other two.
> transport.h seems to provide the most toplevel library stuff as it includes
> remote.h in its header?

> and the issue I am concerned about is the select_capabilities as well as
> the request_capabilities function here. The select_capabilities functionality
> is currently residing in the high level parts of the code as it both depends on
> the advertised server capabilities and on the user input (--use-frotz or config
> options), so the capability selection is done in fetchpack.c
> 
> So there are 2 routes to go: Either we leave the select_capabilities in the
> upper layers (near the actual high level command, fetch, fetchpack) or we put
> it into the transport layer and just passing in a struct what the user desires.
> And when the users desire doesn't meet the server capabilities we die deep down
> in the transport layer.

> On Mon, Jun 01, 2015 at 10:49:45AM -0700, Stefan Beller wrote:
>
>> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack
>> is a bit of a mystery to me, as I cannot fully grasp the difference between
>>  * connect.{h,c}
>>  * remote.{h.c}
>>  * transport.{h.c}
>> there. All of it seems to be doing network related stuff, but I have trouble
>> getting the big picture. I am assuming all of these 3 are rather a low level,
>> used like a library, though there must be even more hierarchy in there,
>> connect is most low level judging from the header file and used by
>> the other two.
>> transport.h seems to provide the most toplevel library stuff as it includes
>> remote.h in its header?
>
> connect.c was originally "the git protocol", and was used by send-pack
> and fetch-pack. Other individual programs implemented other transports.
> Later, as the interface moved towards everybody running "fetch" and
> "push", and those delegating work to the individual transports, we got
> transport.c, which is an abstract interface for all transports. It
> delegates actual git-protocol work to the functions in connect.c (or
> bundle work elsewhere, or handles remote-helpers itself).
>
> And then remote.c contains routines for dealing with the remotes at a
> logical level. E.g., which refs to fetch or push, etc.
>
> So in theory, the flow is something like:
>
>   - fetch.c knows "the user wants to fetch from 'foo'"
>
>   - fetch asks remote.c: "who is remote 'foo'"; we get back a URL
>
>   - fetch asks transport.c: "what are the refs for $URL"
>
>   - it turns out to be a git URL. transport.c calls into connect.c to
>     implement get_refs_via_connect.

>
>   - after fetch gets back the list of refs, it uses routines in remote.c
>     to figure out which refs we are interested in, handle refspecs, etc
>
>   - now fetch asks transport.c: "OK, fetch just these refs"
>
>   - transport.c again calls into connect.c to handle the actual fetch
>
> Of course over the years a lot of cruft has grown around all of them. I
> wouldn't be surprised if there are functions which cross these
> abstractions, or other random functions inside each file that do not
> belong.
>
>> and the issue I am concerned about is the select_capabilities as well as
>> the request_capabilities function here. The select_capabilities functionality
>> is currently residing in the high level parts of the code as it both depends on
>> the advertised server capabilities and on the user input (--use-frotz or config
>> options), so the capability selection is done in fetchpack.c
>>
>> So there are 2 routes to go: Either we leave the select_capabilities in the
>> upper layers (near the actual high level command, fetch, fetchpack) or we put
>> it into the transport layer and just passing in a struct what the user desires.
>> And when the users desire doesn't meet the server capabilities we die deep down
>> in the transport layer.
>
> I think you have to leave it in the fetch-pack code. As you note, it's
> the place where we know about what the user is asking for and can
> manipulate the list. And not all transports even support capabilities
> like this.
>
> -Peff