Re: [PATCH v4 08/35] connect: discover protocol version outside of get_remote_heads

> In order to prepare for the addition of protocol_v2 push the protocol
> version discovery outside of 'get_remote_heads()'.  This will allow for
> keeping the logic for processing the reference advertisement for
> protocol_v1 and protocol_v0 separate from the logic for protocol_v2.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>

> ---
>  builtin/fetch-pack.c | 16 +++++++++++++++-
>  builtin/send-pack.c  | 17 +++++++++++++++--
>  connect.c            | 27 ++++++++++-----------------
>  connect.h            |  3 +++
>  remote-curl.c        | 20 ++++++++++++++++++--
>  remote.h             |  5 +++--
>  transport.c          | 24 +++++++++++++++++++-----
>  7 files changed, 83 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 366b9d13f..85d4faf76 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -4,6 +4,7 @@
>  #include "remote.h"
>  #include "connect.h"
>  #include "sha1-array.h"
> +#include "protocol.h"
>  
>  static const char fetch_pack_usage[] =
>  "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
> @@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  	struct fetch_pack_args args;
>  	struct oid_array shallow = OID_ARRAY_INIT;
>  	struct string_list deepen_not = STRING_LIST_INIT_DUP;
> +	struct packet_reader reader;
>  
>  	packet_trace_identity("fetch-pack");
>  
> @@ -193,7 +195,19 @@ 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);
> +
> +	packet_reader_init(&reader, fd[0], NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_GENTLE_ON_EOF);
> +
> +	switch (discover_version(&reader)) {
> +	case protocol_v1:
> +	case protocol_v0:
> +		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
> +		break;
> +	case protocol_unknown_version:
> +		BUG("unknown protocol version");
> +	}
>  
>  	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
>  			 &shallow, pack_lockfile_ptr);
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index fc4f0bb5f..83cb125a6 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -14,6 +14,7 @@
>  #include "sha1-array.h"
>  #include "gpg-interface.h"
>  #include "gettext.h"
> +#include "protocol.h"
>  
>  static const char * const send_pack_usage[] = {
>  	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
> @@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	int progress = -1;
>  	int from_stdin = 0;
>  	struct push_cas_option cas = {0};
> +	struct packet_reader reader;
>  
>  	struct option options[] = {
>  		OPT__VERBOSITY(&verbose),
> @@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  			args.verbose ? CONNECT_VERBOSE : 0);
>  	}
>  
> -	get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL,
> -			 &extra_have, &shallow);
> +	packet_reader_init(&reader, fd[0], NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_GENTLE_ON_EOF);
> +
> +	switch (discover_version(&reader)) {
> +	case protocol_v1:
> +	case protocol_v0:
> +		get_remote_heads(&reader, &remote_refs, REF_NORMAL,
> +				 &extra_have, &shallow);
> +		break;
> +	case protocol_unknown_version:
> +		BUG("unknown protocol version");
> +	}
>  
>  	transport_verify_remote_names(nr_refspecs, refspecs);
>  
> diff --git a/connect.c b/connect.c
> index c82c90b7c..0b111e62d 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
>  		      "and the repository exists."));
>  }
>  
> -static enum protocol_version discover_version(struct packet_reader *reader)
> +enum protocol_version discover_version(struct packet_reader *reader)
>  {
>  	enum protocol_version version = protocol_unknown_version;
>  
> @@ -233,7 +233,7 @@ enum get_remote_heads_state {
>  /*
>   * Read all the refs from the other end
>   */
> -struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> +struct ref **get_remote_heads(struct packet_reader *reader,
>  			      struct ref **list, unsigned int flags,
>  			      struct oid_array *extra_have,
>  			      struct oid_array *shallow_points)
> @@ -241,24 +241,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  	struct ref **orig_list = list;
>  	int len = 0;
>  	enum get_remote_heads_state state = EXPECTING_FIRST_REF;
> -	struct packet_reader reader;
>  	const char *arg;
>  
> -	packet_reader_init(&reader, in, src_buf, src_len,
> -			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> -
> -	discover_version(&reader);
> -
>  	*list = NULL;
>  
>  	while (state != EXPECTING_DONE) {
> -		switch (packet_reader_read(&reader)) {
> +		switch (packet_reader_read(reader)) {
>  		case PACKET_READ_EOF:
>  			die_initial_contact(1);
>  		case PACKET_READ_NORMAL:
> -			len = reader.pktlen;
> -			if (len > 4 && skip_prefix(reader.line, "ERR ", &arg))
> +			len = reader->pktlen;
> +			if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
>  				die("remote error: %s", arg);
>  			break;
>  		case PACKET_READ_FLUSH:
> @@ -270,22 +263,22 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  
>  		switch (state) {
>  		case EXPECTING_FIRST_REF:
> -			process_capabilities(reader.line, &len);
> -			if (process_dummy_ref(reader.line)) {
> +			process_capabilities(reader->line, &len);
> +			if (process_dummy_ref(reader->line)) {
>  				state = EXPECTING_SHALLOW;
>  				break;
>  			}
>  			state = EXPECTING_REF;
>  			/* fallthrough */
>  		case EXPECTING_REF:
> -			if (process_ref(reader.line, len, &list, flags, extra_have))
> +			if (process_ref(reader->line, len, &list, flags, extra_have))
>  				break;
>  			state = EXPECTING_SHALLOW;
>  			/* fallthrough */
>  		case EXPECTING_SHALLOW:
> -			if (process_shallow(reader.line, len, shallow_points))
> +			if (process_shallow(reader->line, len, shallow_points))
>  				break;
> -			die("protocol error: unexpected '%s'", reader.line);
> +			die("protocol error: unexpected '%s'", reader->line);
>  		case EXPECTING_DONE:
>  			break;
>  		}
> diff --git a/connect.h b/connect.h
> index 01f14cdf3..cdb8979dc 100644
> --- a/connect.h
> +++ b/connect.h
> @@ -13,4 +13,7 @@ extern int parse_feature_request(const char *features, const char *feature);
>  extern const char *server_feature_value(const char *feature, int *len_ret);
>  extern int url_is_local_not_ssh(const char *url);
>  
> +struct packet_reader;
> +extern enum protocol_version discover_version(struct packet_reader *reader);
> +
>  #endif
> diff --git a/remote-curl.c b/remote-curl.c
> index 0053b0954..9f6d07683 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "remote.h"
> +#include "connect.h"
>  #include "strbuf.h"
>  #include "walker.h"
>  #include "http.h"
> @@ -13,6 +14,7 @@
>  #include "credential.h"
>  #include "sha1-array.h"
>  #include "send-pack.h"
> +#include "protocol.h"
>  
>  static struct remote *remote;
>  /* always ends with a trailing slash */
> @@ -176,8 +178,22 @@ static struct discovery *last_discovery;
>  static struct ref *parse_git_refs(struct discovery *heads, int for_push)
>  {
>  	struct ref *list = NULL;
> -	get_remote_heads(-1, heads->buf, heads->len, &list,
> -			 for_push ? REF_NORMAL : 0, NULL, &heads->shallow);
> +	struct packet_reader reader;
> +
> +	packet_reader_init(&reader, -1, heads->buf, heads->len,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_GENTLE_ON_EOF);
> +
> +	switch (discover_version(&reader)) {
> +	case protocol_v1:
> +	case protocol_v0:
> +		get_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,
> +				 NULL, &heads->shallow);
> +		break;
> +	case protocol_unknown_version:
> +		BUG("unknown protocol version");
> +	}
> +
>  	return list;
>  }
>  
> diff --git a/remote.h b/remote.h
> index 1f6611be2..2016461df 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);
>  void free_refs(struct ref *ref);
>  
>  struct oid_array;
> -extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> +struct packet_reader;
> +extern struct ref **get_remote_heads(struct packet_reader *reader,
>  				     struct ref **list, unsigned int flags,
>  				     struct oid_array *extra_have,
> -				     struct oid_array *shallow);
> +				     struct oid_array *shallow_points);
>  
>  int resolve_remote_symref(struct ref *ref, struct ref *list);
>  int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
> diff --git a/transport.c b/transport.c
> index 8e8779096..63c3dbab9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -18,6 +18,7 @@
>  #include "sha1-array.h"
>  #include "sigchain.h"
>  #include "transport-internal.h"
> +#include "protocol.h"
>  
>  static void set_upstreams(struct transport *transport, struct ref *refs,
>  	int pretend)
> @@ -190,13 +191,26 @@ static int connect_setup(struct transport *transport, int for_push)
>  static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
>  {
>  	struct git_transport_data *data = transport->data;
> -	struct ref *refs;
> +	struct ref *refs = NULL;
> +	struct packet_reader reader;
>  
>  	connect_setup(transport, for_push);
> -	get_remote_heads(data->fd[0], NULL, 0, &refs,
> -			 for_push ? REF_NORMAL : 0,
> -			 &data->extra_have,
> -			 &data->shallow);
> +
> +	packet_reader_init(&reader, data->fd[0], NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_GENTLE_ON_EOF);
> +
> +	switch (discover_version(&reader)) {
> +	case protocol_v1:
> +	case protocol_v0:
> +		get_remote_heads(&reader, &refs,
> +				 for_push ? REF_NORMAL : 0,
> +				 &data->extra_have,
> +				 &data->shallow);
> +		break;
> +	case protocol_unknown_version:
> +		BUG("unknown protocol version");
> +	}
>  	data->got_remote_heads = 1;
>  
>  	return refs;
> -- 
> 2.16.2.395.g2e18187dfd-goog
>