protocol transition

Create protocol.{c,h} and provide functions which future servers and clients can use to determine which protocol to use or is being used.

Also introduce the 'GIT_PROTOCOL' environment variable which will be used to communicate a colon separated list of keys with optional values to a server. Unknown keys and values must be tolerated. This mechanism is used to communicate which version of the wire protocol a client would like to use with a server.

Signed-off-by Brandon Williams <[email protected]>

---
 Documentation/config.txt | 16 +++++++++++
 Documentation/git.txt    |  5 ++++
 Makefile                 |  1 +
 cache.h                  |  7 +++++
 protocol.c               | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 protocol.h               | 15 ++++++++++
 6 files changed, 116 insertions(+)
 create mode 100644 protocol.c
 create mode 100644 protocol.h

Documentation/config.txt +0 -0

index dc4e3f58a..d5b28a32c 100644

Quoted 26 lines · Brandon Williams

Create protocol.{c,h} and provide functions which future servers and clients can use to determine which protocol to use or is being used.

Also introduce the 'GIT_PROTOCOL' environment variable which will be used to communicate a colon separated list of keys with optional values to a server. Unknown keys and values must be tolerated. This mechanism is used to communicate which version of the wire protocol a client would like to use with a server.

Signed-off-by Brandon Williams <[email protected]>

---
 Documentation/config.txt | 16 +++++++++++
 Documentation/git.txt    |  5 ++++
 Makefile                 |  1 +
 cache.h                  |  7 +++++
 protocol.c               | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 protocol.h               | 15 ++++++++++
 6 files changed, 116 insertions(+)
 create mode 100644 protocol.c
 create mode 100644 protocol.h

Documentation/config.txt +0 -0

index dc4e3f58a..d5b28a32c 100644

Documentation/config.txt The protocol names currently used by git are: · line 2520

Quoted 4 lines

Documentation/config.txt +1 -0

@@ -2517,6 +2517,22 @@ The protocol names currently used by git are:
     `hg` to allow the `git-remote-hg` helper)
 --
 
+protocol.version::

It would be cool to set a set of versions that are good. (I am not sure if that can be deferred to a later patch.)

Consider we'd have versions 0,1,2,3,4 in the future:
In an ideal world the client and server would talk using v4 as it is the most advanced protocol, right?
Maybe a security/performance issue is found on the server side with say protocol v3. Still no big deal as we are speaking v4. But then consider an issue is found on the client side with v4. Then the client would happily talk 0..3 while the server would love to talk using 0,1,2,4.

The way I think about protocol version negotiation is that both parties involved have a set of versions that they tolerate to talk (they might understand more than the tolerated set, but the user forbade some), and the goal of the negotiation is to find the highest version number that is part of both the server set and client set. So quite naturally with this line of thinking the configuration is to configure a set of versions, which is what I propose here. Maybe even in the wire format, separated with colons?

Documentation/config.txt The protocol names currently used by git are: · line 2524

Quoted 5 lines

Documentation/config.txt +4 -0

+       If set, clients will attempt to communicate with a server using
+       the specified protocol version.  If unset, no attempt will be
+       made by the client to communicate using a particular protocol
+       version, this results in protocol version 0 being used.

This sounds as if we're going to be really shy at first and only users that care will try out new versions at their own risk. From a users POV this may be frustrating as I would imagine that people want to run

  git config --global protocol.version 2

to try it out and then realize that some of their hosts do not speak 2, so they have to actually configure it per repo/remote.

Documentation/config.txt The protocol names currently used by git are: · line 2529

Quoted 3 lines

Documentation/config.txt +2 -0

+       Supported versions:
 
+* `0` - the original wire protocol.

In the future this may be misleading as it doesn't specify the date of when it was original. e.g. are capabilities already supported in "original"?

Maybe phrase it as "wire protocol as of v2.14" ? (Though this sounds as if new capabilities added in the future are not allowed)

Documentation/config.txt The protocol names currently used by git are: · line 2532

Quoted 7 lines

Documentation/config.txt +5 -0

+
+extern enum protocol_version parse_protocol_version(const char *value);
+extern enum protocol_version get_protocol_version_config(void);
+extern enum protocol_version determine_protocol_version_server(void);
+extern enum protocol_version determine_protocol_version_client(const char *server_response);

Here is a good place to have some comments.

Quoted 5 lines

From a users POV this may be frustrating as I would imagine that people want to run

  git config --global protocol.version 2

Quoted 2 lines

to try it out and then realize that some of their hosts do not speak 2, so they have to actually configure it per repo/remote.

The point would be to be able to set this globally, not per-repo. Even if a repo doesn't speak v2 then it should be able to gracefully degrade to v1 without the user having to do anything. The reason why there is this escape hatch is if doing the protocol negotiation out of band causing issues with communicating with a server that it can be shut off.

Quoted 5 lines

Quoted 5 lines

From a users POV this may be frustrating as I would imagine that people want to run

  git config --global protocol.version 2

Quoted 8 lines

Quoted 2 lines

to try it out and then realize that some of their hosts do not speak 2, so they have to actually configure it per repo/remote.

The point would be to be able to set this globally, not per-repo. Even if a repo doesn't speak v2 then it should be able to gracefully degrade to v1 without the user having to do anything. The reason why there is this escape hatch is if doing the protocol negotiation out of band causing issues with communicating with a server that it can be shut off.

In the current situation it is easy to assume that if v1 (and not v0) is configured, that the users intent is "to try out v1 and fallback gracefully to v0".

But this will change over time in the future!

Eventually people will have the desire to say:
"Use version N+1, but never version N", because N has performance or security issues; the user might not want to bother to try N or even actively want to be affirmed that Git will never use version N.

In this future we need a mechanism, that either contains a white list or black list of protocols. To keep it simple (I assume there won't be many protocol versions), a single white list will do.

However transitioning from the currently proposed "try the new configured thing and fallback to whatever" to "this is the exact list of options that Git will try for you" will be hard, as we may break people if we do not unconditionally fall back to v0.

That is why I propose to start with an explicit white list as then we do not have to have a transition plan or otherwise work around the issue. Also it doesn't hurt now to use

    git config --global protocol.version v1,v0

instead compared to the proposed configuration above. (Even better yet, then people could play around with "v1 only" and see how it falls apart on old servers)

Quoted 3 lines

instead compared to the proposed configuration above. (Even better yet, then people could play around with "v1 only" and see how it falls apart on old servers)

Except we can't start with an explicit whitelist because we must fallback to v0 if v1 isn't supported otherwise we would break people.

That is unless we have the semantics of: If not configured v0 will be used, otherwise only use the configured protocol versions.

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host.

In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.

Signed-off-by Brandon Williams <[email protected]>

---
 daemon.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 10 deletions(-)

daemon.c +40 -7

index 30747075f..250dbf82c 100644
@@ -282,7 +282,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
        return NULL;            /* Fallthrough. Deny by default */
 }
 
-typedef int (*daemon_service_fn)(void);
+typedef int (*daemon_service_fn)(const struct argv_array *env);
 struct daemon_service {
        const char *name;
        const char *config_name;

@@ -363,7 +363,7 @@ static int run_access_hook(struct daemon_service *service, const char *dir,
 }
 
 static int run_service(const char *dir, struct daemon_service *service,
-                      struct hostinfo *hi)
+                      struct hostinfo *hi, const struct argv_array *env)
 {
        const char *path;
        int enabled = service->enabled;

@@ -422,7 +422,7 @@ static int run_service(const char *dir, struct daemon_service *service,
         */
        signal(SIGTERM, SIG_IGN);
 
-       return service->fn();
+       return service->fn(env);
 }
 
 static void copy_to_log(int fd)

@@ -462,25 +462,34 @@ static int run_service_command(struct child_process *cld)
        return finish_command(cld);
 }
 
-static int upload_pack(void)
+static int upload_pack(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_pushl(&cld.args, "upload-pack", "--strict", NULL);
        argv_array_pushf(&cld.args, "--timeout=%u", timeout);
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 
-static int upload_archive(void)
+static int upload_archive(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_push(&cld.args, "upload-archive");
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 
-static int receive_pack(void)
+static int receive_pack(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_push(&cld.args, "receive-pack");
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 

@@ -574,7 +583,7 @@ static void canonicalize_client(struct strbuf *out, const char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
        char *val;
        int vallen;

@@ -602,6 +611,39 @@ static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
                if (extra_args < end && *extra_args)
                        die("Invalid request");
        }
+
+       return extra_args;
+}
+
+static void parse_extra_args(struct argv_array *env, const char *extra_args,
+                            int buflen)
+{
+       const char *end = extra_args + buflen;
+       struct strbuf git_protocol = STRBUF_INIT;
+
+       for (; extra_args < end; extra_args += strlen(extra_args) + 1) {
+               const char *arg = extra_args;
+
+               /*
+                * Parse the extra arguments, adding most to 'git_protocol'
+                * which will be used to set the 'GIT_PROTOCOL' envvar in the
+                * service that will be run.
+                *
+                * If there ends up being a particular arg in the future that
+                * git-daemon needs to parse specificly (like the 'host' arg)
+                * then it can be parsed here and not added to 'git_protocol'.
+                */
+               if (*arg) {
+                       if (git_protocol.len > 0)

daemon.c +12 -4

 /*

@@ -695,6 +737,7 @@ static int execute(void)
        int pktlen, len, i;
        char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
        struct hostinfo hi;
+       struct argv_array env = ARGV_ARRAY_INIT;
 
        hostinfo_init(&hi);
 

@@ -716,8 +759,14 @@ static int execute(void)
                pktlen--;
        }
 
-       if (len != pktlen)
-               parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
+       if (len != pktlen) {
+               const char *extra_args;
+               /* retrieve host */
+               extra_args = parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
+
+               /* parse additional args hidden behind a second NUL byte */
+               parse_extra_args(&env, extra_args + 1, pktlen - (extra_args - line) - 1);
+       }
 
        for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
                struct daemon_service *s = &(daemon_service[i]);

@@ -730,13 +779,15 @@ static int execute(void)
                         * Note: The directory here is probably context sensitive,
                         * and might depend on the actual service being performed.
                         */
-                       int rc = run_service(arg, s, &hi);
+                       int rc = run_service(arg, s, &hi, &env);
                        hostinfo_clear(&hi);
+                       argv_array_clear(&env);
                        return rc;
                }
        }
 
        hostinfo_clear(&hi);
+       argv_array_clear(&env);
        logerror("Protocol error: '%s'", line);
        return -1;
 }
--

2.14.1.690.gbb1197296e-goog

Quoted 124 lines · Brandon Williams

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host.

In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.

Signed-off-by Brandon Williams <[email protected]>

---
 daemon.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 10 deletions(-)

daemon.c +40 -7

index 30747075f..250dbf82c 100644
@@ -282,7 +282,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
        return NULL;            /* Fallthrough. Deny by default */
 }
 
-typedef int (*daemon_service_fn)(void);
+typedef int (*daemon_service_fn)(const struct argv_array *env);
 struct daemon_service {
        const char *name;
        const char *config_name;

@@ -363,7 +363,7 @@ static int run_access_hook(struct daemon_service *service, const char *dir,
 }
 
 static int run_service(const char *dir, struct daemon_service *service,
-                      struct hostinfo *hi)
+                      struct hostinfo *hi, const struct argv_array *env)
 {
        const char *path;
        int enabled = service->enabled;

@@ -422,7 +422,7 @@ static int run_service(const char *dir, struct daemon_service *service,
         */
        signal(SIGTERM, SIG_IGN);
 
-       return service->fn();
+       return service->fn(env);
 }
 
 static void copy_to_log(int fd)

@@ -462,25 +462,34 @@ static int run_service_command(struct child_process *cld)
        return finish_command(cld);
 }
 
-static int upload_pack(void)
+static int upload_pack(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_pushl(&cld.args, "upload-pack", "--strict", NULL);
        argv_array_pushf(&cld.args, "--timeout=%u", timeout);
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 
-static int upload_archive(void)
+static int upload_archive(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_push(&cld.args, "upload-archive");
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 
-static int receive_pack(void)
+static int receive_pack(const struct argv_array *env)
 {
        struct child_process cld = CHILD_PROCESS_INIT;
        argv_array_push(&cld.args, "receive-pack");
+
+       argv_array_pushv(&cld.env_array, env->argv);
+
        return run_service_command(&cld);
 }
 

@@ -574,7 +583,7 @@ static void canonicalize_client(struct strbuf *out, const char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
        char *val;
        int vallen;

@@ -602,6 +611,39 @@ static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
                if (extra_args < end && *extra_args)
                        die("Invalid request");
        }
+
+       return extra_args;
+}
+
+static void parse_extra_args(struct argv_array *env, const char *extra_args,
+                            int buflen)
+{
+       const char *end = extra_args + buflen;
+       struct strbuf git_protocol = STRBUF_INIT;
+
+       for (; extra_args < end; extra_args += strlen(extra_args) + 1) {
+               const char *arg = extra_args;
+
+               /*
+                * Parse the extra arguments, adding most to 'git_protocol'
+                * which will be used to set the 'GIT_PROTOCOL' envvar in the
+                * service that will be run.
+                *
+                * If there ends up being a particular arg in the future that
+                * git-daemon needs to parse specificly (like the 'host' arg)
+                * then it can be parsed here and not added to 'git_protocol'.
+                */
+               if (*arg) {
+                       if (git_protocol.len > 0)

daemon.c static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen) · line 647

Quoted 10 lines

daemon.c +9 -0

+                               strbuf_addch(&git_protocol, ':');
+                       strbuf_addstr(&git_protocol, arg);
+               }
+       }
+
+       if (git_protocol.len > 0)
+               argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
+                                git_protocol.buf);
+       strbuf_release(&git_protocol);
 }

I wonder if this could be written as

  begin = extra_args;
  p = extra_args;
  end = extra_args + buflen;

  while (p < end) {
    if (!*p)
        *p = ':';
    p++;
  }
  argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s", begin);

to ease the load on the server side, as then we do not
have to copy the partial strings into strbufs and then
count the length individually? (maybe performance is no big deal here?)

Quoted 49 lines

daemon.c +12 -4

 /*

@@ -695,6 +737,7 @@ static int execute(void)
        int pktlen, len, i;
        char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
        struct hostinfo hi;
+       struct argv_array env = ARGV_ARRAY_INIT;
 
        hostinfo_init(&hi);
 

@@ -716,8 +759,14 @@ static int execute(void)
                pktlen--;
        }
 
-       if (len != pktlen)
-               parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
+       if (len != pktlen) {
+               const char *extra_args;
+               /* retrieve host */
+               extra_args = parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
+
+               /* parse additional args hidden behind a second NUL byte */
+               parse_extra_args(&env, extra_args + 1, pktlen - (extra_args - line) - 1);
+       }
 
        for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
                struct daemon_service *s = &(daemon_service[i]);

@@ -730,13 +779,15 @@ static int execute(void)
                         * Note: The directory here is probably context sensitive,
                         * and might depend on the actual service being performed.
                         */
-                       int rc = run_service(arg, s, &hi);
+                       int rc = run_service(arg, s, &hi, &env);
                        hostinfo_clear(&hi);
+                       argv_array_clear(&env);
                        return rc;
                }
        }
 
        hostinfo_clear(&hi);
+       argv_array_clear(&env);
        logerror("Protocol error: '%s'", line);
        return -1;
 }
--

2.14.1.690.gbb1197296e-goog

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host.

In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.

daemon.c +25 -1

@@ -574,7 +583,7 @@ static void canonicalize_client(struct strbuf *out, const char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
 	char *val;
 	int vallen;

@@ -602,6 +611,39 @@ static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 		if (extra_args < end && *extra_args)
 			die("Invalid request");
 	}
+
+	return extra_args;
+}
+
+static void parse_extra_args(struct argv_array *env, const char *extra_args,
+			     int buflen)
+{
+	const char *end = extra_args + buflen;
+	struct strbuf git_protocol = STRBUF_INIT;
+
+	for (; extra_args < end; extra_args += strlen(extra_args) + 1) {
+		const char *arg = extra_args;
+
+		/*
+		 * Parse the extra arguments, adding most to 'git_protocol'
+		 * which will be used to set the 'GIT_PROTOCOL' envvar in the
+		 * service that will be run.
+		 *
+		 * If there ends up being a particular arg in the future that
+		 * git-daemon needs to parse specificly (like the 'host' arg)
+		 * then it can be parsed here and not added to 'git_protocol'.
+		 */
+		if (*arg) {
+			if (git_protocol.len > 0)

Quoted 6 lines · On Wed, 13 Sep 2017 14:54:43 -0700 Brandon Williams <[email protected]> wrote:

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host.

Quoted 6 lines

In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.

A test in this patch (if possible) would be nice, but it is probably clearer to test this when one of the commands (e.g. upload-pack) is done. Could a test be added to the next patch to verify (using GIT_TRACE_PACKET, maybe) that the expected strings are sent? Then mention in this commit message that this will be tested in the next patch too.

Quoted 16 lines · On Wed, 20 Sep 2017 17:24:43 -0700 Jonathan Tan <[email protected]>…

Quoted 12 lines · On Wed, 13 Sep 2017 14:54:43 -0700 Brandon Williams <[email protected]> wrote:

A normal request to git-daemon is structured as
"command path/to/repo\0host=..\0" and due to a bug in an old version of git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the command, 2009-06-04) we aren't able to place any extra args (separated by NULs) besides the host.

In order to get around this limitation teach git-daemon to recognize additional request arguments hidden behind a second NUL byte. Requests can then be structured like:
"command path/to/repo\0host=..\0\0version=1\0key=value\0". git-daemon can then parse out the extra arguments and set 'GIT_PROTOCOL' accordingly.

Quoted 6 lines

A test in this patch (if possible) would be nice, but it is probably clearer to test this when one of the commands (e.g. upload-pack) is done. Could a test be added to the next patch to verify (using GIT_TRACE_PACKET, maybe) that the expected strings are sent? Then mention in this commit message that this will be tested in the next patch too.

Ah, I see that it is tested in 6/8. You can ignore this comment.

Commit eb398797c (connect: advertized capability is not a ref, 2016-09-09) taught 'get_remote_heads()' to recognize that the 'capabilities^{}' line isn't a ref but required that the 'capabilities^{}' line came during the first response from the server. A future patch will introduce a version string sent by the server during its first response which can then cause a client to unnecessarily die if a 'capabilities^{}' line sent as the first ref.

Teach 'get_remote_heads()' to instead die if a 'capabilities^{}' line is sent after a ref.

connect.c +0 -0

index df56c0cbf..af5096ec6 100644

Quoted 8 lines · Brandon Williams

Commit eb398797c (connect: advertized capability is not a ref, 2016-09-09) taught 'get_remote_heads()' to recognize that the 'capabilities^{}' line isn't a ref but required that the 'capabilities^{}' line came during the first response from the server. A future patch will introduce a version string sent by the server during its first response which can then cause a client to unnecessarily die if a 'capabilities^{}' line sent as the first ref.

Quoted 2 lines

Teach 'get_remote_heads()' to instead die if a 'capabilities^{}' line is sent after a ref.

Hmm. I think I understand why you'd want this loosening. But why are we sending a version line to a client that we don't know is speaking v2? IOW, shouldn't we be reporting the version to the client in the normal capabilities when we don't know for sure that they can handle the new field? Otherwise we're breaking existing clients.

Or is this only for v2 clients, and we've changed the protocol but get_remote_heads() just needs to be updated, too?

Quoted 6 lines

connect.c +0 -0

index df56c0cbf..af5096ec6 100644

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 131

Quoted 9 lines

connect.c +2 -1

@@ -124,10 +124,11 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * response does not necessarily mean an ACL problem, though.
 	 */
 	int saw_response;
+	int seen_ref;
 	int got_dummy_ref_with_capabilities_declaration = 0;
 
 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
+	for (saw_response = 0, seen_ref = 0; ; saw_response = 1) {

If we're not going to update it in the right-hand side of the for-loop, should we perhaps not be initializing it in the left-hand side? I.e., can we just do:

  seen_ref = 0;

above the loop, like we initialize "list"?

(For that matter, could we just be checking whether *list is NULL?)

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 173

Quoted 10 lines

connect.c +2 -0

@@ -165,6 +166,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 
 		name_len = strlen(name);
 		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
+			if (seen_ref)
+				; /* NEEDSWORK: Error out for multiple capabilities lines? */
 			free(server_capabilities);
 			server_capabilities = xstrdup(name + name_len + 1);
 		}

Interesting question. Probably it would be fine to. Coincidentally I ran across a similar case. It seems that upload-pack will read multiple capabilities lines back from the client. I.e., if it gets:

want 1234abcd... foo
want 5678abcd... bar

then it will turn on both the "foo" and "bar" capabilities. I'm pretty sure this is unintended, and is somewhat counter to the way that clients handle multiple lines (which is to forget the old line and respect only the new one, as shown in the quoted hunk).

I wonder if we should be outlawing extra capabilities in both directions. I don't _think_ we've ever relied on that working, and I don't have much sympathy for any 3rd-party implementation that does (though I doubt that any exists).

That tangent aside, I do this hunk is kind of orthogonal to the point of your patch. We're talking about potential _tightening_ here, whereas the point of your patch is loosening. And it's not clear to me what we want to tighten:

- should capabilities come as part of the first response, even if we have no refs? In which case we really want "if (saw_response)" here.

- should they came as part of the first ref (or pseudo-ref), in which case "if (seen_ref)" is the right thing.

- should we loosen it to complaining when there are multiple capabilities sent. In which case "if (server_capabilities)" is the right thing.

I'm not sure which we'd want, but it really seems like a separate topic that should be explored on top.

Quoted 1 line · Jeff King

(For that matter, could we just be checking whether *list is NULL?)

True, that would probably be the better way to do this.

I wasn't sure either, which is why I added the comment to prod discussion. I agree that is is orthogonal to this series so I'll most likely drop it, as it doesn't help with the protocol transition discussion.

Quoted 2 lines

Or is this only for v2 clients, and we've changed the protocol but get_remote_heads() just needs to be updated, too?

A client which didn't request protocol v1 (I'm calling the current protocol v0, and v1 is just v0 with the initial response from the server containing a version string) should not receive a version string in the initial response. The problem is that when introducing the version string to protocol version 1, I didn't want to have to do a huge refactoring of ALL of the current transport code so I stuck the version check in get_remote_heads() since v1 is exactly the same as v0, except for the first line from the server.

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

Signed-off-by Jonathan Tan <[email protected]>

---
In some in-office discussion, I was informed that my original patch relaxed the ordering of ".keep" lines. Here is an update.

I'm also using a switch statement now, which avoids having multiple lines of read_remote_ref().

 connect.c | 159 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 93 insertions(+), 66 deletions(-)

connect.c +25 -0

index 49b28b83b..e0821dbff 100644
@@ -107,6 +107,84 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 	string_list_clear(&symref, 0);
 }
 
+/*
+ * Read one line of a server's ref advertisement into packet_buffer.
+ */
+static int read_remote_ref(int in, char **src_buf, size_t *src_len,
+			   int *responded)
+{
+	int len = packet_read(in, src_buf, src_len,
+			      packet_buffer, sizeof(packet_buffer),
+			      PACKET_READ_GENTLE_ON_EOF |
+			      PACKET_READ_CHOMP_NEWLINE);
+	const char *arg;
+	if (len < 0)
+		die_initial_contact(*responded);
+	if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))
+		die("remote error: %s", arg);
+
+	*responded = 1;
+
+	return len;
+}
+
+#define EXPECTING_REF 0
+#define EXPECTING_SHALLOW 1
+
+static int process_ref(int *state, int len, struct ref ***list,

connect.c +18 -0

+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);
+		server_capabilities = xstrdup(name + name_len + 1);
+	}
+
+	if (extra_have && !strcmp(name, ".have")) {
+		oid_array_append(extra_have, &old_oid);
+	} else if (!strcmp(name, "capabilities^{}")) {
+		if (**list)
+			/* cannot coexist with other refs */
+			die("protocol error: unexpected capabilities^{}");
+		/* There should be no more refs; proceed to the next state. */
+		(*state)++;

connect.c +30 -66

+{
+	const char *arg;
+	struct object_id old_oid;
+
+	if (!skip_prefix(packet_buffer, "shallow ", &arg))
+		return 0;
+
+	if (get_oid_hex(arg, &old_oid))
+		die("protocol error: expected shallow sha-1, got '%s'", arg);
+	if (!shallow_points)
+		die("repository on the other end cannot be shallow");
+	oid_array_append(shallow_points, &old_oid);
+	return 1;
+}
+
 /*
  * Read all the refs from the other end
  */

@@ -123,76 +201,25 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF;
 
 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
-		struct ref *ref;
-		struct object_id old_oid;
-		char *name;
-		int len, name_len;
-		char *buffer = 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(saw_response);
-
-		if (!len)
-			break;
-
-		if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
-			die("remote error: %s", arg);
-
-		if (len == GIT_SHA1_HEXSZ + strlen("shallow ") &&
-			skip_prefix(buffer, "shallow ", &arg)) {
-			if (get_oid_hex(arg, &old_oid))
-				die("protocol error: expected shallow sha-1, got '%s'", arg);
-			if (!shallow_points)
-				die("repository on the other end cannot be shallow");
-			oid_array_append(shallow_points, &old_oid);
-			continue;
-		}
-
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
-			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
-
-		name_len = strlen(name);
-		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
-		}
 
-		if (extra_have && !strcmp(name, ".have")) {
-			oid_array_append(extra_have, &old_oid);
-			continue;
-		}
-
-		if (!strcmp(name, "capabilities^{}")) {
-			if (saw_response)
-				die("protocol error: unexpected capabilities^{}");
-			if (got_dummy_ref_with_capabilities_declaration)
-				die("protocol error: multiple capabilities^{}");
-			got_dummy_ref_with_capabilities_declaration = 1;
-			continue;
+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */
+		case EXPECTING_SHALLOW:
+			if (process_shallow(&state, shallow_points))
+				break;
+			die("protocol error: unexpected '%s'", packet_buffer);
+		default:
+			die("unexpected state %d", state);
 		}
-
-		if (!check_ref(name, flags))
-			continue;
-
-		if (got_dummy_ref_with_capabilities_declaration)
-			die("protocol error: unexpected ref after capabilities^{}");
-
-		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
-		oidcpy(&ref->old_oid, &old_oid);
-		*list = ref;
-		list = &ref->next;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);

Signature · 1 line

2.14.1.728.g20a5b67d5.dirty

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

Signed-off-by Jonathan Tan <[email protected]>

---
I sent the wrong version of this patch :-(

This should be the correct one. A bit less clean because I introduced a 3rd state, however.

 connect.c | 167 +++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 101 insertions(+), 66 deletions(-)

connect.c +101 -66

index 49b28b83b..ef6358cfc 100644
@@ -107,6 +107,91 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 	string_list_clear(&symref, 0);
 }
 
+/*
+ * Read one line of a server's ref advertisement into packet_buffer.
+ */
+static int read_remote_ref(int in, char **src_buf, size_t *src_len,
+			   int *responded)
+{
+	int len = packet_read(in, src_buf, src_len,
+			      packet_buffer, sizeof(packet_buffer),
+			      PACKET_READ_GENTLE_ON_EOF |
+			      PACKET_READ_CHOMP_NEWLINE);
+	const char *arg;
+	if (len < 0)
+		die_initial_contact(*responded);
+	if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))
+		die("remote error: %s", arg);
+
+	*responded = 1;
+
+	return len;
+}
+
+#define EXPECTING_REF_WITH_CAPABILITIES 0
+#define EXPECTING_REF 1
+#define EXPECTING_SHALLOW 2
+
+static int process_ref(int *state, int len, struct ref ***list,
+		       unsigned int flags, struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	char *name;
+	int name_len;
+
+	if (len < GIT_SHA1_HEXSZ + 2 ||
+	    get_oid_hex(packet_buffer, &old_oid) ||
+	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
+		*state = EXPECTING_SHALLOW;
+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
+	    len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);
+		server_capabilities = xstrdup(name + name_len + 1);
+	} else if (*state == EXPECTING_REF) {
+		if (len != name_len + GIT_SHA1_HEXSZ + 1)
+			die("unexpected capabilities after ref name");
+	}
+
+	if (extra_have && !strcmp(name, ".have")) {
+		oid_array_append(extra_have, &old_oid);
+	} else if (!strcmp(name, "capabilities^{}")) {
+		if (**list)
+			/* cannot coexist with other refs */
+			die("protocol error: unexpected capabilities^{}");
+		/* There should be no more refs; proceed to the next state. */
+		*state = EXPECTING_SHALLOW;
+		return 1;
+	} else if (check_ref(name, flags)) {
+		struct ref *ref = alloc_ref(name);
+		oidcpy(&ref->old_oid, &old_oid);
+		**list = ref;
+		*list = &ref->next;
+	}
+	*state = EXPECTING_REF;
+	return 1;
+}
+
+static int process_shallow(struct oid_array *shallow_points)
+{
+	const char *arg;
+	struct object_id old_oid;
+
+	if (!skip_prefix(packet_buffer, "shallow ", &arg))
+		return 0;
+
+	if (get_oid_hex(arg, &old_oid))
+		die("protocol error: expected shallow sha-1, got '%s'", arg);
+	if (!shallow_points)
+		die("repository on the other end cannot be shallow");
+	oid_array_append(shallow_points, &old_oid);
+	return 1;
+}
+
 /*
  * Read all the refs from the other end
  */

@@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF_WITH_CAPABILITIES;
 
 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
-		struct ref *ref;
-		struct object_id old_oid;
-		char *name;
-		int len, name_len;
-		char *buffer = 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(saw_response);
-
-		if (!len)
-			break;
-
-		if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
-			die("remote error: %s", arg);
-
-		if (len == GIT_SHA1_HEXSZ + strlen("shallow ") &&
-			skip_prefix(buffer, "shallow ", &arg)) {
-			if (get_oid_hex(arg, &old_oid))
-				die("protocol error: expected shallow sha-1, got '%s'", arg);
-			if (!shallow_points)
-				die("repository on the other end cannot be shallow");
-			oid_array_append(shallow_points, &old_oid);
-			continue;
-		}
-
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
-			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
-
-		name_len = strlen(name);
-		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
-		}
-
-		if (extra_have && !strcmp(name, ".have")) {
-			oid_array_append(extra_have, &old_oid);
-			continue;
-		}
 
-		if (!strcmp(name, "capabilities^{}")) {
-			if (saw_response)
-				die("protocol error: unexpected capabilities^{}");
-			if (got_dummy_ref_with_capabilities_declaration)
-				die("protocol error: multiple capabilities^{}");
-			got_dummy_ref_with_capabilities_declaration = 1;
-			continue;
+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF_WITH_CAPABILITIES:
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */
+		case EXPECTING_SHALLOW:
+			if (process_shallow(shallow_points))
+				break;
+			die("protocol error: unexpected '%s'", packet_buffer);
+		default:
+			die("unexpected state %d", state);
 		}
-
-		if (!check_ref(name, flags))
-			continue;
-
-		if (got_dummy_ref_with_capabilities_declaration)
-			die("protocol error: unexpected ref after capabilities^{}");
-
-		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
-		oidcpy(&ref->old_oid, &old_oid);
-		*list = ref;
-		list = &ref->next;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);

Quoted 5 lines · Jonathan Tan

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

Quoted 3 lines

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

Sounds sensible. This still replaces the earlier 1.5?

Quoted 11 lines

connect.c +11 -0

+static int process_ref(int *state, int len, struct ref ***list,
+		       unsigned int flags, struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	char *name;
+	int name_len;
+
+	if (len < GIT_SHA1_HEXSZ + 2 ||
+	    get_oid_hex(packet_buffer, &old_oid) ||
+	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
+		*state = EXPECTING_SHALLOW;

connect.c static void annotate_refs_with_symref_info(struct ref *ref) · line 153

Quoted 8 lines

connect.c +8 -0

+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
+	    len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);

Quoted 22 lines

connect.c +13 -2

+		server_capabilities = xstrdup(name + name_len + 1);
+	} else if (*state == EXPECTING_REF) {
+		if (len != name_len + GIT_SHA1_HEXSZ + 1)
+			die("unexpected capabilities after ref name");
+	}
+	...
+	}
+	*state = EXPECTING_REF;
+	return 1;
+}

@@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF_WITH_CAPABILITIES;
 
 	*list = NULL;

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 223

Quoted 7 lines

connect.c +7 -0

+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF_WITH_CAPABILITIES:
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Jonathan Tan

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

Quoted 1 line

Sounds sensible. This still replaces the earlier 1.5?

Well, it does, but it also invalidates how the new "pick the version offered and used" feature is integrated to this callchain. I guess we'd need a new "we are now expecting the version info" state in a patch to replace "connect: teach client to recognize v1 server response".

Quoted 12 lines

Quoted 12 lines

connect.c +11 -0

+static int process_ref(int *state, int len, struct ref ***list,
+		       unsigned int flags, struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	char *name;
+	int name_len;
+
+	if (len < GIT_SHA1_HEXSZ + 2 ||
+	    get_oid_hex(packet_buffer, &old_oid) ||
+	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
+		*state = EXPECTING_SHALLOW;

Quoted 8 lines

connect.c static void annotate_refs_with_symref_info(struct ref *ref) · line 153

Quoted 8 lines

connect.c +8 -0

+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
+	    len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);

Is this free() still needed? After hitting this block, you'd set *state to EXPECTING_REF before you return, so nobody would set server_capabilities by hitting this block twice, and an attempt to do so will hit the die("unexpected cap") below, no?

Or it may be a signal that this patch tightens it too much and breaks older or third-party implementations of the other side that can emit more than one refs with capability advertisement?

Quoted 23 lines

Quoted 22 lines

connect.c +13 -2

+		server_capabilities = xstrdup(name + name_len + 1);
+	} else if (*state == EXPECTING_REF) {
+		if (len != name_len + GIT_SHA1_HEXSZ + 1)
+			die("unexpected capabilities after ref name");
+	}
+	...
+	}
+	*state = EXPECTING_REF;
+	return 1;
+}

@@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF_WITH_CAPABILITIES;
 
 	*list = NULL;

Quoted 7 lines

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 223

Quoted 7 lines

connect.c +7 -0

+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF_WITH_CAPABILITIES:
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */

OK. This fallthrough is because expecting-ref is really expecting ref or shallow and once we see a shallow, we no longer expect ref and expect only shallow. So from that point of view, an assignment to set state to EXPECTING_SHALLOW could happen here, not inside process_ref. I mention this because in general, passing state around and let it be updated in helper functions would make the state transition harder to follow, not easier, even though refactoring the processing needed in different stages into helper functions like this patch does ought to make it easier to see by shrinking the outer loop (i.e. this one) that controls the whole process.

I think if we split process_ref() further into two, then we no longer need to pass &state to that function? We start this loop with "expecting the dummy ref (or other)" state, have a new process_dummy_ref() function check if we got "capabilities^{}" thing and do its thing if that is the case (otherwise we fall through to the call to process_ref(), just like the above code falls through to call process_shallow() when it realizes what it got is not a ref), and after the first call to process_dummy_ref() we'd be in the "expecting ref (or other)" state---and the state transition can happen in this caller, not in process_dummy_ref() or process_ref().

Inside process_dummy_ref() and process_ref(), there would be a call to the same helper that notices and extracts the server capability and stores it (or barfs against the second line that advertises the capability, by noticing that server_capabilities is not NULL).

Wouldn't that make the presentation of the state machine cleaner?

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

As part of this change, this patch interprets capabilities only on the first line in the ref advertisement, ignoring all others.

Signed-off-by Jonathan Tan <[email protected]>

---
I've updated state transitions to occur in get_remote_heads() instead, as suggested. I didn't want to do that previously because each step in the state machine needed to communicate if (i) the line is "consumed" and (ii) the state needed to be advanced, but with Junio's suggestion to reorganize the methods, that is no longer true.

As Junio said, the free(server_capabilities) can be removed.

As for whether how capabilities on subsequent lines are handled, I think it's better to ignore them - they are behind NULs, after all.

Yes, "connect: teach client to recognize v1 server response" will need to be modified.

This change does have the side effect that if the server sends a ref advertisement with "shallow"s only (and no refs), things will still work, and the server can even tuck capabilities on the first "shallow" line. I think that's fine, and it does make the client code cleaner.

---
 connect.c | 171 ++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 105 insertions(+), 66 deletions(-)

connect.c +19 -0

index 49b28b83b..978d01359 100644
@@ -11,6 +11,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "transport.h"
+#include "strbuf.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);

@@ -107,6 +108,86 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 	string_list_clear(&symref, 0);
 }
 
+/*
+ * Read one line of a server's ref advertisement into packet_buffer.
+ */
+static int read_remote_ref(int in, char **src_buf, size_t *src_len,
+			   int *responded)
+{
+	int len = packet_read(in, src_buf, src_len,
+			      packet_buffer, sizeof(packet_buffer),
+			      PACKET_READ_GENTLE_ON_EOF |
+			      PACKET_READ_CHOMP_NEWLINE);
+	const char *arg;
+	if (len < 0)
+		die_initial_contact(*responded);
+	if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))
+		die("remote error: %s", arg);
+
+	*responded = 1;
+

connect.c +51 -55

+{
+	struct object_id old_oid;
+	const char *name;
+
+	if (parse_oid_hex(packet_buffer, &old_oid, &name))
+		return 0;
+	if (*name != ' ')
+		return 0;
+	name++;
+
+	if (extra_have && !strcmp(name, ".have")) {
+		oid_array_append(extra_have, &old_oid);
+	} else if (check_ref(name, flags)) {
+		struct ref *ref = alloc_ref(name);
+		oidcpy(&ref->old_oid, &old_oid);
+		**list = ref;
+		*list = &ref->next;
+	}
+	return 1;
+}
+
+static int process_shallow(struct oid_array *shallow_points)
+{
+	const char *arg;
+	struct object_id old_oid;
+
+	if (!skip_prefix(packet_buffer, "shallow ", &arg))
+		return 0;
+
+	if (get_oid_hex(arg, &old_oid))
+		die("protocol error: expected shallow sha-1, got '%s'", arg);
+	if (!shallow_points)
+		die("repository on the other end cannot be shallow");
+	oid_array_append(shallow_points, &old_oid);
+	return 1;
+}
+
 /*
  * Read all the refs from the other end
  */

@@ -123,76 +204,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_FIRST_REF;
 
 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
-		struct ref *ref;
-		struct object_id old_oid;
-		char *name;
-		int len, name_len;
-		char *buffer = 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(saw_response);
-
-		if (!len)
-			break;
 
-		if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
-			die("remote error: %s", arg);
-
-		if (len == GIT_SHA1_HEXSZ + strlen("shallow ") &&
-			skip_prefix(buffer, "shallow ", &arg)) {
-			if (get_oid_hex(arg, &old_oid))
-				die("protocol error: expected shallow sha-1, got '%s'", arg);
-			if (!shallow_points)
-				die("repository on the other end cannot be shallow");
-			oid_array_append(shallow_points, &old_oid);
-			continue;
-		}
-
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
-			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
-
-		name_len = strlen(name);
-		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
-		}
-
-		if (extra_have && !strcmp(name, ".have")) {
-			oid_array_append(extra_have, &old_oid);
-			continue;
-		}
-
-		if (!strcmp(name, "capabilities^{}")) {
-			if (saw_response)
-				die("protocol error: unexpected capabilities^{}");
-			if (got_dummy_ref_with_capabilities_declaration)
-				die("protocol error: multiple capabilities^{}");
-			got_dummy_ref_with_capabilities_declaration = 1;
-			continue;
+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_FIRST_REF:
+			process_capabilities(len);
+			if (process_dummy_ref()) {
+				state = EXPECTING_SHALLOW;
+				break;
+			}
+			state = EXPECTING_REF;
+			/* fallthrough */
+		case EXPECTING_REF:

connect.c +0 -11

 		}
-
-		if (!check_ref(name, flags))
-			continue;
-
-		if (got_dummy_ref_with_capabilities_declaration)
-			die("protocol error: unexpected ref after capabilities^{}");
-
-		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
-		oidcpy(&ref->old_oid, &old_oid);
-		*list = ref;
-		list = &ref->next;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);

Signature · 1 line

2.14.1.728.g20a5b67d5.dirty

connect.c static void annotate_refs_with_symref_info(struct ref *ref) · line 138

Quoted 3 lines

connect.c +3 -0

+static void process_capabilities(int len)
+{
+	int nul_location = strlen(packet_buffer);

It may make more sense to not rely on accessing a global buffer here directly and instead pass in the buff you're working on, much like your are doing with len.

I'm not the biggest fan of dynamically allocating this and then using it to compare. Maybe we can check to make sure that the oid matches the null_oid and that the name matches the "capabilities^{}" string? That way you can avoid the allocation?

connect.c static void annotate_refs_with_symref_info(struct ref *ref) · line 153

Quoted 2 lines

connect.c +2 -0

+static int process_ref(struct ref ***list, unsigned int flags,
+		       struct oid_array *extra_have)

So from comparing this to the current code it doesn't look like there is a check in 'process_ref' that ensures that a 'capabilities^{}' line doesn't show up after a normal ref, or am I missing something?

Currently, get_remote_heads() parses the ref advertisement in one loop, allowing refs and shallow lines to intersperse, despite this not being allowed by the specification. Refactor get_remote_heads() to use two loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other lines in the ref advertisement, which will be done in a subsequent patch.

As part of this change, this patch interprets capabilities only on the first line in the ref advertisement, ignoring all others.

Signed-off-by Jonathan Tan <[email protected]>

---
I've updated state transitions to occur in get_remote_heads() instead, as suggested. I didn't want to do that previously because each step in the state machine needed to communicate if (i) the line is "consumed" and (ii) the state needed to be advanced, but with Junio's suggestion to reorganize the methods, that is no longer true.

As Junio said, the free(server_capabilities) can be removed.

As for whether how capabilities on subsequent lines are handled, I think it's better to ignore them - they are behind NULs, after all.

This change does have the side effect that if the server sends a ref advertisement with "shallow"s only (and no refs), things will still work, and the server can even tuck capabilities on the first "shallow" line. I think that's fine, and it does make the client code cleaner.

connect.c +21 -0

+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_FIRST_REF:
+			process_capabilities(len);
+			if (process_dummy_ref()) {
+				state = EXPECTING_SHALLOW;
+				break;
+			}
+			state = EXPECTING_REF;
+			/* fallthrough */
+		case EXPECTING_REF:
+			if (process_ref(&list, flags, extra_have))
+				break;
+			state = EXPECTING_SHALLOW;
+			/* fallthrough */
+		case EXPECTING_SHALLOW:
+			if (process_shallow(shallow_points))
+				break;
+			die("protocol error: unexpected '%s'", packet_buffer);
+		default:
+			die("unexpected state %d", state);
 		}
 	}