[PATCH v4 0/1] Advertise multiple supported proto versions

Fix several bugs identified in v3, clarify commit message, and clean up extern keyword in protocol.h.

Josh Steadmon (1):
  protocol: advertise multiple supported versions

 builtin/archive.c        |   3 +
 builtin/clone.c          |   4 ++
 builtin/fetch-pack.c     |   4 ++
 builtin/fetch.c          |   5 ++
 builtin/ls-remote.c      |   5 ++
 builtin/pull.c           |   5 ++
 builtin/push.c           |   4 ++
 builtin/receive-pack.c   |   3 +
 builtin/send-pack.c      |   3 +
 builtin/upload-archive.c |   3 +
 builtin/upload-pack.c    |   4 ++
 connect.c                |  47 +++++++--------
 protocol.c               | 122 ++++++++++++++++++++++++++++++++++++---
 protocol.h               |  23 +++++++-
 remote-curl.c            |  28 ++++++---
 t/t5570-git-daemon.sh    |   2 +-
 t/t5700-protocol-v1.sh   |   8 +--
 t/t5702-protocol-v2.sh   |  16 +++--
 transport-helper.c       |   6 ++
 19 files changed, 237 insertions(+), 58 deletions(-)

Range-diff against v3:
1:  b9968e3fb0 ! 1:  3c023991c0 protocol: advertise multiple supported versions
    @@ -4,22 +4,25 @@
     
         Currently the client advertises that it supports the wire protocol
         version set in the protocol.version config. However, not all services
    -    support the same set of protocol versions. When connecting to
    -    git-receive-pack, the client automatically downgrades to v0 if
    -    config.protocol is set to v2, but this check is not performed for other
    -    services.
    +    support the same set of protocol versions. For example, git-receive-pack
    +    supports v1 and v0, but not v2. If a client connects to git-receive-pack
    +    and requests v2, it will instead be downgraded to v0. Other services,
    +    such as git-upload-archive, do not do any version negotiation checks.
     
    -    This patch creates a protocol version registry. Individual operations
    -    register all the protocol versions they support prior to communicating
    -    with a server. Versions should be listed in preference order; the
    -    version specified in protocol.version will automatically be moved to the
    -    front of the registry.
    +    This patch creates a protocol version registry. Individual client and
    +    server programs register all the protocol versions they support prior to
    +    communicating with a remote instance. Versions should be listed in
    +    preference order; the version specified in protocol.version will
    +    automatically be moved to the front of the registry.
     
         The protocol version registry is passed to remote helpers via the
         GIT_PROTOCOL environment variable.
     
         Clients now advertise the full list of registered versions. Servers
    -    select the first recognized version from this advertisement.
    +    select the first allowed version from this advertisement.
    +
    +    While we're at it, remove unnecessary externs from function declarations
    +    in protocol.h.
     
         Signed-off-by: Josh Steadmon <steadmon@google.com>
    @@ -165,6 +168,20 @@
      	git_config(git_push_config, &flags);
      	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
     
    + diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
    + --- a/builtin/receive-pack.c
    + +++ b/builtin/receive-pack.c
    +@@
    + 
    + 	packet_trace_identity("receive-pack");
    + 
    ++	register_allowed_protocol_version(protocol_v1);
    ++	register_allowed_protocol_version(protocol_v0);
    ++
    + 	argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);
    + 
    + 	if (argc > 1)
    +
      diff --git a/builtin/send-pack.c b/builtin/send-pack.c
      --- a/builtin/send-pack.c
      +++ b/builtin/send-pack.c
    @@ -179,6 +196,42 @@
      	argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
      	if (argc > 0) {
     
    + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
    + --- a/builtin/upload-archive.c
    + +++ b/builtin/upload-archive.c
    +@@
    + #include "builtin.h"
    + #include "archive.h"
    + #include "pkt-line.h"
    ++#include "protocol.h"
    + #include "sideband.h"
    + #include "run-command.h"
    + #include "argv-array.h"
    +@@
    + 	if (argc == 2 && !strcmp(argv[1], "-h"))
    + 		usage(upload_archive_usage);
    + 
    ++	register_allowed_protocol_version(protocol_v0);
    ++
    + 	/*
    + 	 * Set up sideband subprocess.
    + 	 *
    +
    + diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
    + --- a/builtin/upload-pack.c
    + +++ b/builtin/upload-pack.c
    +@@
    + 	packet_trace_identity("upload-pack");
    + 	read_replace_refs = 0;
    + 
    ++	register_allowed_protocol_version(protocol_v2);
    ++	register_allowed_protocol_version(protocol_v1);
    ++	register_allowed_protocol_version(protocol_v0);
    ++
    + 	argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
    + 
    + 	if (argc != 1)
    +
      diff --git a/connect.c b/connect.c
      --- a/connect.c
      +++ b/connect.c
    @@ -311,7 +364,7 @@
     +static enum protocol_version *allowed_versions;
     +static int nr_allowed_versions;
     +static int alloc_allowed_versions;
    -+static int have_advertised_versions_already = 0;
    ++static int version_registration_locked = 0;
     +
     +static const char protocol_v0_string[] = "0";
     +static const char protocol_v1_string[] = "1";
    @@ -357,8 +410,8 @@
      
     +void register_allowed_protocol_version(enum protocol_version version)
     +{
    -+	if (have_advertised_versions_already)
    -+		BUG(_("attempting to register an allowed protocol version after advertisement"));
    ++	if (version_registration_locked)
    ++		BUG("late attempt to register an allowed protocol version");
     +
     +	ALLOC_GROW(allowed_versions, nr_allowed_versions + 1,
     +		   alloc_allowed_versions);
    @@ -384,13 +437,23 @@
     +	string_list_clear(&version_list, 0);
     +}
     +
    ++static int is_allowed_protocol_version(enum protocol_version version)
    ++{
    ++	int i;
    ++	version_registration_locked = 1;
    ++	for (i = 0; i < nr_allowed_versions; i++)
    ++		if (version == allowed_versions[i])
    ++			return 1;
    ++	return 0;
    ++}
    ++
     +void get_client_protocol_version_advertisement(struct strbuf *advert)
     +{
    -+	int tmp_nr = nr_allowed_versions;
    ++	int i, tmp_nr = nr_allowed_versions;
     +	enum protocol_version *tmp_allowed_versions, config_version;
     +	strbuf_reset(advert);
     +
    -+	have_advertised_versions_already = 1;
    ++	version_registration_locked = 1;
     +
     +	config_version = get_protocol_version_config();
     +	if (config_version == protocol_v0) {
    @@ -409,20 +472,19 @@
     +	}
     +
     +	if (tmp_allowed_versions[0] != config_version)
    -+		for (int i = 1; i < nr_allowed_versions; i++)
    ++		for (i = 1; i < nr_allowed_versions; i++)
     +			if (tmp_allowed_versions[i] == config_version) {
    -+				enum protocol_version swap =
    -+					tmp_allowed_versions[0];
    -+				tmp_allowed_versions[0] =
    -+					tmp_allowed_versions[i];
    -+				tmp_allowed_versions[i] = swap;
    ++				SWAP(tmp_allowed_versions[0],
    ++				     tmp_allowed_versions[i]);
     +			}
     +
     +	strbuf_addf(advert, "version=%s",
     +		    format_protocol_version(tmp_allowed_versions[0]));
    -+	for (int i = 1; i < tmp_nr; i++)
    ++	for (i = 1; i < tmp_nr; i++)
     +		strbuf_addf(advert, ":version=%s",
     +			    format_protocol_version(tmp_allowed_versions[i]));
    ++
    ++	free(tmp_allowed_versions);
     +}
     +
      enum protocol_version determine_protocol_version_server(void)
    @@ -447,7 +509,8 @@
      			if (skip_prefix(item->string, "version=", &value)) {
      				v = parse_protocol_version(value);
     -				if (v > version)
    -+				if (v != protocol_unknown_version) {
    ++				if (v != protocol_unknown_version &&
    ++				    is_allowed_protocol_version(v)) {
      					version = v;
     +					break;
     +				}
    @@ -459,29 +522,46 @@
      --- a/protocol.h
      +++ b/protocol.h
     @@
    +  * 'protocol.version' config.  If unconfigured, a value of 'protocol_v0' is
    +  * returned.
       */
    - extern enum protocol_version get_protocol_version_config(void);
    - 
    +-extern enum protocol_version get_protocol_version_config(void);
    ++enum protocol_version get_protocol_version_config(void);
    ++
     +/*
     + * Register an allowable protocol version for a given operation. Registration
     + * must occur before attempting to advertise a version to a server process.
     + */
    -+extern void register_allowed_protocol_version(enum protocol_version version);
    ++void register_allowed_protocol_version(enum protocol_version version);
     +
     +/*
     + * Register allowable protocol versions from the GIT_PROTOCOL environment var.
     + */
    -+extern void register_allowed_protocol_versions_from_env(void);
    ++void register_allowed_protocol_versions_from_env(void);
     +
     +/*
     + * Fill a strbuf with a version advertisement string suitable for use in the
     + * GIT_PROTOCOL environment variable or similar version negotiation field.
     + */
    -+extern void get_client_protocol_version_advertisement(struct strbuf *advert);
    -+
    ++void get_client_protocol_version_advertisement(struct strbuf *advert);
    + 
      /*
       * Used by a server to determine which protocol version should be used based on
    -  * a client's request, communicated via the 'GIT_PROTOCOL' environment variable
    +@@
    +  * request a particular protocol version, a default of 'protocol_v0' will be
    +  * used.
    +  */
    +-extern enum protocol_version determine_protocol_version_server(void);
    ++enum protocol_version determine_protocol_version_server(void);
    + 
    + /*
    +  * Used by a client to determine which protocol version the server is speaking
    +  * based on the server's initial response.
    +  */
    +-extern enum protocol_version determine_protocol_version_client(const char *server_response);
    ++enum protocol_version determine_protocol_version_client(const char *server_response);
    + 
    + #endif /* PROTOCOL_H */
     
      diff --git a/remote-curl.c b/remote-curl.c
      --- a/remote-curl.c