[RFCv2 00/16] Protocol version 2

[RFCv2 00/16] Protocol version 2

[Stefan Beller]

patches 1-4 are for the uploading side (upload-pack)
and 5-13 for the fetching side.

patches 14-16 are documentation and the tiny test.

Nguyễn Thái Ngọc Duy (1):
  upload-pack: make client capability parsing code a separate function

Stefan Beller (15):
  stringlist: add from_space_separated_string
  connect: rewrite feature parsing to work on string_list
  upload-pack-2: Implement the version 2 of upload-pack
  remote.h: Change get_remote_heads return to void
  remote.h: add new struct for options
  transport: add infrastructure to support a protocol version number
  transport: select transport version via command line or config
  remote.h: add get_remote_capabilities, request_capabilities
  transport: connect_setup appends protocol version number
  remote: have preselect_capabilities
  transport: get_refs_via_connect exchanges capabilities before refs.
  fetch-pack: use the configured transport protocol
  t5544: add a test case for the new protocol
  Documentation/technical/pack-protocol: Mention http as possible
    protocol
  Document protocol version 2

 .gitignore                                        |   1 +
 Documentation/technical/pack-protocol.txt         |  85 ++++++++++--
 Documentation/technical/protocol-capabilities.txt |  15 ---
 Makefile                                          |   2 +
 builtin/fetch-pack.c                              |  22 +++-
 builtin/fetch.c                                   |   6 +
 builtin/receive-pack.c                            |  13 +-
 connect.c                                         | 154 +++++++++++++++-------
 connect.h                                         |   2 +-
 fetch-pack.c                                      | 109 ++++++++-------
 fetch-pack.h                                      |   1 +
 remote.c                                          |   2 +
 remote.h                                          |  28 +++-
 string-list.c                                     |  12 ++
 string-list.h                                     |   1 +
 t/t5544-fetch-2.sh                                |  40 ++++++
 transport-helper.c                                |   1 +
 transport.c                                       |  60 ++++++++-
 transport.h                                       |  11 ++
 upload-pack-2.c                                   |   1 +
 upload-pack.c                                     | 151 +++++++++++++++------
 21 files changed, 535 insertions(+), 182 deletions(-)
 create mode 100755 t/t5544-fetch-2.sh
 create mode 120000 upload-pack-2.c

-- 
2.4.1.345.gab207b6.dirty

[RFCv2 01/16] stringlist: add from_space_separated_string

[Stefan Beller]

This function parses a space separated string into a string list.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 string-list.c | 12 ++++++++++++
 string-list.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/string-list.c b/string-list.c
index 2a32a3f..f71384f 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,6 +7,18 @@ void string_list_init(struct string_list *list, int strdup_strings)
 	list->strdup_strings = strdup_strings;
 }
 
+void from_space_separated_string(struct string_list *list, char *line)
+{
+	char *save_ptr;
+	const char delimiters[] = " \t\n";
+	const char *token = strtok_r(line, delimiters, &save_ptr);
+
+	while (token) {
+		string_list_append(list, xstrdup(token));
+		token = strtok_r(NULL, delimiters, &save_ptr);
+	}
+}
+
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
 static int get_entry_index(const struct string_list *list, const char *string,
diff --git a/string-list.h b/string-list.h
index d3809a1..88c18e9 100644
--- a/string-list.h
+++ b/string-list.h
@@ -19,6 +19,7 @@ struct string_list {
 #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
 
 void string_list_init(struct string_list *list, int strdup_strings);
+void from_space_separated_string(struct string_list *list, char *line);
 
 void print_string_list(const struct string_list *p, const char *text);
 void string_list_clear(struct string_list *list, int free_util);
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 02/16] upload-pack: make client capability parsing code a separate function

[Stefan Beller]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 upload-pack.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 745fda8..5449ff7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -531,6 +531,28 @@ error:
 	}
 }
 
+static void parse_features(const char *features)
+{
+	if (parse_feature_request(features, "multi_ack_detailed"))
+		multi_ack = 2;
+	else if (parse_feature_request(features, "multi_ack"))
+		multi_ack = 1;
+	if (parse_feature_request(features, "no-done"))
+		no_done = 1;
+	if (parse_feature_request(features, "thin-pack"))
+		use_thin_pack = 1;
+	if (parse_feature_request(features, "ofs-delta"))
+		use_ofs_delta = 1;
+	if (parse_feature_request(features, "side-band-64k"))
+		use_sideband = LARGE_PACKET_MAX;
+	else if (parse_feature_request(features, "side-band"))
+		use_sideband = DEFAULT_PACKET_MAX;
+	if (parse_feature_request(features, "no-progress"))
+		no_progress = 1;
+	if (parse_feature_request(features, "include-tag"))
+		use_include_tag = 1;
+}
+
 static void receive_needs(void)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -540,7 +562,6 @@ static void receive_needs(void)
 	shallow_nr = 0;
 	for (;;) {
 		struct object *o;
-		const char *features;
 		unsigned char sha1_buf[20];
 		char *line = packet_read_line(0, NULL);
 		reset_timeout();
@@ -575,26 +596,7 @@ static void receive_needs(void)
 			die("git upload-pack: protocol error, "
 			    "expected to get sha, not '%s'", line);
 
-		features = line + 45;
-
-		if (parse_feature_request(features, "multi_ack_detailed"))
-			multi_ack = 2;
-		else if (parse_feature_request(features, "multi_ack"))
-			multi_ack = 1;
-		if (parse_feature_request(features, "no-done"))
-			no_done = 1;
-		if (parse_feature_request(features, "thin-pack"))
-			use_thin_pack = 1;
-		if (parse_feature_request(features, "ofs-delta"))
-			use_ofs_delta = 1;
-		if (parse_feature_request(features, "side-band-64k"))
-			use_sideband = LARGE_PACKET_MAX;
-		else if (parse_feature_request(features, "side-band"))
-			use_sideband = DEFAULT_PACKET_MAX;
-		if (parse_feature_request(features, "no-progress"))
-			no_progress = 1;
-		if (parse_feature_request(features, "include-tag"))
-			use_include_tag = 1;
+		parse_features(line + 45);
 
 		o = parse_object(sha1_buf);
 		if (!o)
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 03/16] connect: rewrite feature parsing to work on string_list

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/receive-pack.c | 13 +++++----
 connect.c              | 79 ++++++++++++++++++++++----------------------------
 connect.h              |  2 +-
 upload-pack.c          | 11 +++++--
 4 files changed, 51 insertions(+), 54 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2ec52b..518ce85 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1391,16 +1391,19 @@ static struct command *read_head_info(struct sha1_array *shallow)
 
 		linelen = strlen(line);
 		if (linelen < len) {
-			const char *feature_list = line + linelen + 1;
-			if (parse_feature_request(feature_list, "report-status"))
+			struct string_list feature_list = STRING_LIST_INIT_DUP;
+			from_space_separated_string(&feature_list, line + linelen + 1);
+			if (parse_feature_request(&feature_list, "report-status"))
 				report_status = 1;
-			if (parse_feature_request(feature_list, "side-band-64k"))
+			if (parse_feature_request(&feature_list, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
-			if (parse_feature_request(feature_list, "quiet"))
+			if (parse_feature_request(&feature_list, "quiet"))
 				quiet = 1;
 			if (advertise_atomic_push
-			    && parse_feature_request(feature_list, "atomic"))
+			    && parse_feature_request(&feature_list, "atomic"))
 				use_atomic = 1;
+
+			string_list_clear(&feature_list, 1);
 		}
 
 		if (!strcmp(line, "push-cert")) {
diff --git a/connect.c b/connect.c
index c0144d8..4295ba1 100644
--- a/connect.c
+++ b/connect.c
@@ -10,8 +10,8 @@
 #include "string-list.h"
 #include "sha1-array.h"
 
-static char *server_capabilities;
-static const char *parse_feature_value(const char *, const char *, int *);
+struct string_list server_capabilities = STRING_LIST_INIT_DUP;
+static const char *parse_feature_value(struct string_list *, const char *, int *);
 
 static int check_ref(const char *name, unsigned int flags)
 {
@@ -80,18 +80,18 @@ reject:
 
 static void annotate_refs_with_symref_info(struct ref *ref)
 {
+	struct string_list_item *item;
 	struct string_list symref = STRING_LIST_INIT_DUP;
-	const char *feature_list = server_capabilities;
 
-	while (feature_list) {
-		int len;
+	for_each_string_list_item(item, &server_capabilities) {
 		const char *val;
 
-		val = parse_feature_value(feature_list, "symref", &len);
-		if (!val)
-			break;
-		parse_one_symref_info(&symref, val, len);
-		feature_list = val + 1;
+		if (skip_prefix(item->string, "symref", &val)) {
+			if (!val)
+				continue;
+			val++; /* skip the = */
+			parse_one_symref_info(&symref, val, strlen(val));
+		}
 	}
 	string_list_sort(&symref);
 
@@ -153,8 +153,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 
 		name_len = strlen(name);
 		if (len != name_len + 41) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
+			string_list_clear(&server_capabilities, 1);
+			from_space_separated_string(&server_capabilities, name + name_len + 1);
 		}
 
 		if (extra_have && !strcmp(name, ".have")) {
@@ -176,51 +176,40 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	return list;
 }
 
-static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
+static const char *parse_feature_value(struct string_list *feature_list, const char *feature, int *lenp)
 {
-	int len;
-
-	if (!feature_list)
-		return NULL;
-
-	len = strlen(feature);
-	while (*feature_list) {
-		const char *found = strstr(feature_list, feature);
-		if (!found)
-			return NULL;
-		if (feature_list == found || isspace(found[-1])) {
-			const char *value = found + len;
-			/* feature with no value (e.g., "thin-pack") */
-			if (!*value || isspace(*value)) {
-				if (lenp)
-					*lenp = 0;
-				return value;
-			}
-			/* feature with a value (e.g., "agent=git/1.2.3") */
-			else if (*value == '=') {
-				value++;
-				if (lenp)
-					*lenp = strcspn(value, " \t\n");
-				return value;
-			}
-			/*
-			 * otherwise we matched a substring of another feature;
-			 * keep looking
-			 */
+	const char *value;
+	struct string_list_item *item;
+
+	for_each_string_list_item(item, feature_list) {
+		if (!skip_prefix(item->string, feature, &value))
+			continue;
+
+		/* feature with no value (e.g., "thin-pack") */
+		if (!*value) {
+			if (lenp)
+				*lenp = 0;
+			return value;
+		}
+		/* feature with a value (e.g., "agent=git/1.2.3") */
+		else if (*value == '=') {
+			value++;
+			if (lenp)
+				*lenp = strlen(value);
+			return value;
 		}
-		feature_list = found + 1;
 	}
 	return NULL;
 }
 
-int parse_feature_request(const char *feature_list, const char *feature)
+int parse_feature_request(struct string_list *feature_list, const char *feature)
 {
 	return !!parse_feature_value(feature_list, feature, NULL);
 }
 
 const char *server_feature_value(const char *feature, int *len)
 {
-	return parse_feature_value(server_capabilities, feature, len);
+	return parse_feature_value(&server_capabilities, feature, len);
 }
 
 int server_supports(const char *feature)
diff --git a/connect.h b/connect.h
index c41a685..47492e5 100644
--- a/connect.h
+++ b/connect.h
@@ -7,7 +7,7 @@ extern struct child_process *git_connect(int fd[2], const char *url, const char
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);
 extern int server_supports(const char *feature);
-extern int parse_feature_request(const char *features, const char *feature);
+extern int parse_feature_request(struct string_list *, 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);
 
diff --git a/upload-pack.c b/upload-pack.c
index 5449ff7..2493964 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -531,7 +531,7 @@ error:
 	}
 }
 
-static void parse_features(const char *features)
+static void parse_features(struct string_list *features)
 {
 	if (parse_feature_request(features, "multi_ack_detailed"))
 		multi_ack = 2;
@@ -563,7 +563,10 @@ static void receive_needs(void)
 	for (;;) {
 		struct object *o;
 		unsigned char sha1_buf[20];
-		char *line = packet_read_line(0, NULL);
+		int pkt_len;
+		struct string_list list = STRING_LIST_INIT_DUP;
+		char *line = packet_read_line(0, &pkt_len);
+
 		reset_timeout();
 		if (!line)
 			break;
@@ -596,7 +599,9 @@ static void receive_needs(void)
 			die("git upload-pack: protocol error, "
 			    "expected to get sha, not '%s'", line);
 
-		parse_features(line + 45);
+		from_space_separated_string(&list, xstrdup(line + 45));
+		parse_features(&list);
+		string_list_clear(&list, 1);
 
 		o = parse_object(sha1_buf);
 		if (!o)
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

[Stefan Beller]

In upload-pack-2 we send each capability in its own packet buffer.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    Moved the capabilities into a struct containing all the capabilities,
    and then we selectively cancel out unwanted capabilities.

 .gitignore      |   1 +
 Makefile        |   2 ++
 upload-pack-2.c |   1 +
 upload-pack.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++++----------
 4 files changed, 86 insertions(+), 18 deletions(-)
 create mode 120000 upload-pack-2.c

diff --git a/.gitignore b/.gitignore
index 422c538..2213af4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -165,6 +165,7 @@
 /git-update-server-info
 /git-upload-archive
 /git-upload-pack
+/git-upload-pack-2
 /git-var
 /git-verify-commit
 /git-verify-pack
diff --git a/Makefile b/Makefile
index 323c401..f83f02d 100644
--- a/Makefile
+++ b/Makefile
@@ -560,6 +560,7 @@ PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
+PROGRAM_OBJS += upload-pack-2.o
 PROGRAM_OBJS += remote-testsvn.o
 
 # Binary suffix, set to .exe for Windows builds
@@ -626,6 +627,7 @@ OTHER_PROGRAMS = git$X
 # what test wrappers are needed and 'install' will install, in bindir
 BINDIR_PROGRAMS_NEED_X += git
 BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-upload-pack-2
 BINDIR_PROGRAMS_NEED_X += git-receive-pack
 BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-shell
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
@@ -0,0 +1 @@
+upload-pack.c
\ No newline at end of file
diff --git a/upload-pack.c b/upload-pack.c
index 2493964..7477ca3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -716,33 +716,69 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
 }
 
+static int advertise_capabilities = 1;
+const char *all_capabilities[] = {
+	"multi_ack",
+	"thin-pack",
+	"side-band",
+	"side-band-64k",
+	"ofs-delta",
+	"shallow",
+	"no-progress",
+	"include-tag",
+	"multi_ack_detailed",
+	"allow-tip-sha1-in-want",
+	"no-done",
+};
+
+static void send_capabilities(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
+		const char *cap = all_capabilities[i];
+		if (!strcmp(cap, "allow-tip-sha1-in-want") && !allow_tip_sha1_in_want)
+			continue;
+		if (!strcmp(cap, "no-done") && !stateless_rpc)
+			continue;
+		packet_write(1,"%s", cap);
+	}
+
+	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)
 {
-	static const char *capabilities = "multi_ack thin-pack side-band"
-		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag multi_ack_detailed";
+
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
 	if (mark_our_ref(refname, sha1))
 		return 0;
 
-	if (capabilities) {
-		struct strbuf symref_info = STRBUF_INIT;
-
-		format_symref_info(&symref_info, cb_data);
-		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
-			     sha1_to_hex(sha1), refname_nons,
-			     0, capabilities,
-			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
-			     stateless_rpc ? " no-done" : "",
-			     symref_info.buf,
-			     git_user_agent_sanitized());
-		strbuf_release(&symref_info);
+	if (advertise_capabilities) {
+		int i;
+		struct strbuf capabilities = STRBUF_INIT;
+
+		for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
+			const char *cap = all_capabilities[i];
+			if (!strcmp(cap, "allow-tip-sha1-in-want") && !allow_tip_sha1_in_want)
+				continue;
+			if (!strcmp(cap, "no-done") && !stateless_rpc)
+				continue;
+			strbuf_addf(&capabilities, " %s", cap);
+		}
+
+		format_symref_info(&capabilities, cb_data);
+		strbuf_addf(&capabilities, " agent=%s", git_user_agent_sanitized());
+
+		packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname_nons,
+			     0, capabilities.buf);
+		strbuf_release(&capabilities);
+		advertise_capabilities = 0;
 	} else {
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
 	}
-	capabilities = NULL;
 	if (!peel_ref(refname, peeled))
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
 	return 0;
@@ -792,6 +828,29 @@ static void upload_pack(void)
 	}
 }
 
+static void receive_capabilities(void)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+	char *line = packet_read_line(0, NULL);
+	while (line) {
+		string_list_append(&list, line);
+		line = packet_read_line(0, NULL);
+	}
+	parse_features(&list);
+	string_list_clear(&list, 1);
+}
+
+static void upload_pack_version_2(void)
+{
+	send_capabilities();
+	receive_capabilities();
+
+	/* The rest of the protocol stays the same, capabilities advertising
+	   is disabled though. */
+	advertise_capabilities = 0;
+	upload_pack();
+}
+
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
 	if (!strcmp("uploadpack.allowtipsha1inwant", var))
@@ -807,13 +866,14 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 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++) {
@@ -855,6 +915,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 (ends_with(cmd, "-2"))
+		upload_pack_version_2();
+	else
+		upload_pack();
 	return 0;
 }
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 05/16] remote.h: Change get_remote_heads return to void

[Stefan Beller]

No function uses the return value of get_remote_heads, so we don't want
to confuse readers by it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 connect.c | 10 ++++------
 remote.h  |  8 ++++----
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index 4295ba1..a2c777e 100644
--- a/connect.c
+++ b/connect.c
@@ -108,10 +108,10 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 /*
  * Read all the refs from the other end
  */
-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,
-			      struct sha1_array *shallow_points)
+void get_remote_heads(int in, char *src_buf, size_t src_len,
+		      struct ref **list, unsigned int flags,
+		      struct sha1_array *extra_have,
+		      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
 	int got_at_least_one_head = 0;
@@ -172,8 +172,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-
-	return list;
 }
 
 static const char *parse_feature_value(struct string_list *feature_list, const char *feature, int *lenp)
diff --git a/remote.h b/remote.h
index 02d66ce..d5242b0 100644
--- a/remote.h
+++ b/remote.h
@@ -144,10 +144,10 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 struct sha1_array;
-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,
-				     struct sha1_array *shallow);
+extern void get_remote_heads(int in, char *src_buf, size_t src_len,
+			     struct ref **list, unsigned int flags,
+			     struct sha1_array *extra_have,
+			     struct sha1_array *shallow);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 06/16] remote.h: add new struct for options

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 remote.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/remote.h b/remote.h
index d5242b0..16cacfe 100644
--- a/remote.h
+++ b/remote.h
@@ -56,6 +56,20 @@ struct remote {
 	char *http_proxy;
 };
 
+/* todo: discuss if this should be merged with
+ * git_transport_options in transport.c */
+struct transport_options {
+	unsigned multi_ack : 2;
+	unsigned no_done : 1;
+	unsigned use_thin_pack : 1;
+	unsigned no_progress : 1;
+	unsigned include_tag : 1;
+	unsigned prefer_ofs_delta : 1;
+	unsigned agent_supported : 1;
+	unsigned allow_tip_sha1_in_want : 1;
+	unsigned use_sideband;
+};
+
 struct remote *remote_get(const char *name);
 struct remote *pushremote_get(const char *name);
 int remote_is_configured(const char *name);
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 07/16] transport: add infrastructure to support a protocol version number

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    This patch has been split up into 2 parts.
    This first part only introduces the infrastructure
    without exposing it to the user at all (no command line
    option nor a repository configuration option).
    
    The exposure to the user will be added in a later second
    patch. This allows us some time in between to build all
    the features we want without the requirement to be
    functional.

 remote.h           |  2 ++
 transport-helper.c |  1 +
 transport.c        | 15 +++++++++++++++
 transport.h        |  5 +++++
 4 files changed, 23 insertions(+)

diff --git a/remote.h b/remote.h
index 16cacfe..3767bed 100644
--- a/remote.h
+++ b/remote.h
@@ -50,6 +50,8 @@ struct remote {
 	const char *receivepack;
 	const char *uploadpack;
 
+	int transport_version;
+
 	/*
 	 * for curl remotes only
 	 */
diff --git a/transport-helper.c b/transport-helper.c
index 5d99a6b..ab3cd5b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -247,6 +247,7 @@ static int disconnect_helper(struct transport *transport)
 }
 
 static const char *unsupported_options[] = {
+	TRANS_OPT_TRANSPORTVERSION,
 	TRANS_OPT_UPLOADPACK,
 	TRANS_OPT_RECEIVEPACK,
 	TRANS_OPT_THIN,
diff --git a/transport.c b/transport.c
index f080e93..651f0ac 100644
--- a/transport.c
+++ b/transport.c
@@ -479,6 +479,16 @@ static int set_git_option(struct git_transport_options *opts,
 	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
 		opts->push_cert = !!value;
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_TRANSPORTVERSION)) {
+		if (!value)
+			opts->transport_version = DEFAULT_TRANSPORT_VERSION;
+		else {
+			char *end;
+			opts->transport_version = strtol(value, &end, 0);
+			if (*end)
+				die("transport: invalid transport version option '%s'", value);
+		}
+		return 0;
 	}
 	return 1;
 }
@@ -970,6 +980,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->connect = connect_git;
 		ret->disconnect = disconnect_git;
 		ret->smart_options = &(data->options);
+		ret->smart_options->transport_version =
+			DEFAULT_TRANSPORT_VERSION;
 
 		data->conn = NULL;
 		data->got_remote_heads = 0;
@@ -988,6 +1000,9 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->smart_options->receivepack = "git-receive-pack";
 		if (remote->receivepack)
 			ret->smart_options->receivepack = remote->receivepack;
+		if (remote->transport_version)
+			ret->smart_options->transport_version =
+				remote->transport_version;
 	}
 
 	return ret;
diff --git a/transport.h b/transport.h
index 18d2cf8..6095d7a 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
 	unsigned update_shallow : 1;
 	unsigned push_cert : 1;
 	int depth;
+	int transport_version;
 	const char *uploadpack;
 	const char *receivepack;
 	struct push_cas_option *cas;
@@ -162,6 +163,10 @@ struct transport *transport_get(struct remote *, const char *);
 /* Send push certificates */
 #define TRANS_OPT_PUSH_CERT "pushcert"
 
+/* Use a new version of the git protocol */
+#define TRANS_OPT_TRANSPORTVERSION "transportversion"
+#define DEFAULT_TRANSPORT_VERSION 1
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 08/16] transport: select transport version via command line or config

[Stefan Beller]

The transport version set via command line argument in
git fetch takes precedence over the configured version.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    removed -y as the short --transport-version
    
    This patch has been split up and is the second part
    carrying only the exposure to the user.

 builtin/fetch.c | 6 ++++++
 remote.c        | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7910419..a558563 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -46,6 +46,7 @@ static const char *recurse_submodules_default;
 static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
+static const char *transport_version;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -121,6 +122,9 @@ static struct option builtin_fetch_options[] = {
 		   N_("default mode for recursion"), PARSE_OPT_HIDDEN },
 	OPT_BOOL(0, "update-shallow", &update_shallow,
 		 N_("accept refs that update .git/shallow")),
+	OPT_STRING(0, "transport-version", &transport_version,
+		   N_("transport-version"),
+		   N_("specify transport version to be used")),
 	{ OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
 	  N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg },
 	OPT_END()
@@ -848,6 +852,8 @@ static struct transport *prepare_transport(struct remote *remote)
 	struct transport *transport;
 	transport = transport_get(remote, NULL);
 	transport_set_verbosity(transport, verbosity, progress);
+	if (transport_version)
+		set_option(transport, TRANS_OPT_TRANSPORTVERSION, transport_version);
 	if (upload_pack)
 		set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
 	if (keep)
diff --git a/remote.c b/remote.c
index 68901b0..2914d9d 100644
--- a/remote.c
+++ b/remote.c
@@ -476,6 +476,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
+	} else if (!strcmp(subkey, ".transportversion")) {
+		remote->transport_version = git_config_int(key, value);
 	}
 	return 0;
 }
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 09/16] remote.h: add get_remote_capabilities, request_capabilities

[Stefan Beller]

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>
---
 connect.c | 37 +++++++++++++++++++++++++++++++++++++
 remote.h  |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/connect.c b/connect.c
index a2c777e..4ebe1dc 100644
--- a/connect.c
+++ b/connect.c
@@ -105,6 +105,43 @@ 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)
+{
+	string_list_clear(&server_capabilities, 1);
+	for (;;) {
+		int len;
+		char *line = packet_buffer;
+
+		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;
+
+		string_list_append(&server_capabilities, line);
+	}
+}
+
+int request_capabilities(int out, struct transport_options *options)
+{
+	if (options->multi_ack == 2)    packet_write(out, "multi_ack_detailed");
+	if (options->multi_ack == 1)    packet_write(out, "multi_ack");
+	if (options->no_done)           packet_write(out, "no-done");
+	if (options->use_sideband == 2) packet_write(out, "side-band-64k");
+	if (options->use_sideband == 1) packet_write(out, "side-band");
+	if (options->use_thin_pack)     packet_write(out, "thin-pack");
+	if (options->no_progress)       packet_write(out, "no-progress");
+	if (options->include_tag)       packet_write(out, "include-tag");
+	if (options->prefer_ofs_delta)  packet_write(out, "ofs-delta");
+	if (options->agent_supported)   packet_write(out, "agent=%s",
+						     git_user_agent_sanitized());
+	packet_flush(out);
+}
+
 /*
  * Read all the refs from the other end
  */
diff --git a/remote.h b/remote.h
index 3767bed..61619c5 100644
--- a/remote.h
+++ b/remote.h
@@ -165,6 +165,9 @@ extern void get_remote_heads(int in, char *src_buf, size_t src_len,
 			     struct sha1_array *extra_have,
 			     struct sha1_array *shallow);
 
+void get_remote_capabilities(int in, char *src_buf, size_t src_len);
+int request_capabilities(int out, struct transport_options*);
+
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 10/16] transport: connect_setup appends protocol version number

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    name it to_free

 transport.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/transport.c b/transport.c
index 651f0ac..b49fc60 100644
--- a/transport.c
+++ b/transport.c
@@ -496,15 +496,28 @@ 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 *to_free = 0;
 
 	if (data->conn)
 		return 0;
 
+	remote_program = (for_push ? data->options.receivepack
+				   : data->options.uploadpack);
+
+	if (transport->smart_options->transport_version >= 2) {
+		to_free = xmalloc(strlen(remote_program) + 12);
+		sprintf(to_free, "%s-%d", remote_program,
+			transport->smart_options->transport_version);
+		remote_program = to_free;
+	}
+
 	data->conn = git_connect(data->fd, transport->url,
-				 for_push ? data->options.receivepack :
-				 data->options.uploadpack,
+				 remote_program,
 				 verbose ? CONNECT_VERBOSE : 0);
 
+	free(to_free);
+
 	return 0;
 }
 
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 11/16] remote: have preselect_capabilities

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 connect.c | 28 ++++++++++++++++++++++++++++
 remote.h  |  1 +
 2 files changed, 29 insertions(+)

diff --git a/connect.c b/connect.c
index 4ebe1dc..752b9a5 100644
--- a/connect.c
+++ b/connect.c
@@ -126,6 +126,34 @@ void get_remote_capabilities(int in, char *src_buf, size_t src_len)
 	}
 }
 
+/* just select all options the server advertised. */
+void preselect_capabilities(struct transport_options *options)
+{
+	if (is_repository_shallow() && !server_supports("shallow"))
+		die("Server does not support shallow clients");
+
+	if (server_supports("multi_ack"))
+		options->multi_ack = 1;
+	else if (server_supports("multi_ack_detailed"))
+		options->multi_ack = 2;
+
+	if (server_supports("side-band"))
+		options->use_sideband = 1;
+	else if (server_supports("side-band-64k"))
+		options->use_sideband = 2;
+
+	if (server_supports("no-done"))
+		options->no_done = 1;
+	if (server_supports("thin-pack"))
+		options->use_thin_pack = 1;
+	if (server_supports("no-progress"))
+		options->no_progress = 1;
+	if (server_supports("include-tag"))
+		options->include_tag = 1;
+	if (server_supports("ofs-delta"))
+		options->prefer_ofs_delta = 1;
+}
+
 int request_capabilities(int out, struct transport_options *options)
 {
 	if (options->multi_ack == 2)    packet_write(out, "multi_ack_detailed");
diff --git a/remote.h b/remote.h
index 61619c5..264a513 100644
--- a/remote.h
+++ b/remote.h
@@ -166,6 +166,7 @@ extern void get_remote_heads(int in, char *src_buf, size_t src_len,
 			     struct sha1_array *shallow);
 
 void get_remote_capabilities(int in, char *src_buf, size_t src_len);
+void preselect_capabilities(struct transport_options *options);
 int request_capabilities(int out, struct transport_options*);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs.

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    A minor issue I am unsure about here is the
    line
    	&& transport->smart_options->transport_version)
    which could be prevented if we always set the transport_version
    in make_remote to be the default remote version.
    
    The advantage of having it always set in make_remote would
    be a cleaner mind model (the version set is always accurate)
    as opposed to now (version may be 0, then the default
    applies as we don't care enough to set a version)
    
    However I think the code may be more ugly if we were to
    always set the version in make_remote as then we would need
    to move the DEFAULT_TRANSPORT_VERSION define into remote.h
    or somewhere else (transport.h is not included in remote.c,
    I guess that's on purpose?)

 transport.c | 28 ++++++++++++++++++++++++----
 transport.h |  6 ++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c
index b49fc60..ba40677 100644
--- a/transport.c
+++ b/transport.c
@@ -523,14 +523,33 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
 
 static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
 {
+	struct transport_options options;
 	struct git_transport_data *data = transport->data;
 	struct ref *refs;
+	int version = DEFAULT_TRANSPORT_VERSION;
 
+	if (transport->smart_options
+	    && transport->smart_options->transport_version)
+		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) {
+	case 2: /* first talk about capabilities, then get the heads */
+		get_remote_capabilities(data->fd[0], NULL, 0);
+		preselect_capabilities(&options);
+		if (transport->select_capabilities)
+			transport->select_capabilities(&options);
+		request_capabilities(data->fd[1], &options);
+		/* 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;
+	}
 	data->got_remote_heads = 1;
 
 	return refs;
@@ -987,6 +1006,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		struct git_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
 		ret->set_option = NULL;
+		ret->select_capabilities = NULL;
 		ret->get_refs_list = get_refs_via_connect;
 		ret->fetch = fetch_refs_via_pack;
 		ret->push_refs = git_transport_push;
diff --git a/transport.h b/transport.h
index 6095d7a..3e63efc 100644
--- a/transport.h
+++ b/transport.h
@@ -74,6 +74,12 @@ struct transport {
 	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
 
 	/**
+	 * A callback to select protocol options. Must be set if
+	 * the caller wants to change transport options.
+	 */
+	void (*select_capabilities)(struct transport_options *);
+
+	/**
 	 * Push the objects and refs. Send the necessary objects, and
 	 * then, for any refs where peer_ref is set and
 	 * peer_ref->new_sha1 is different from old_sha1, tell the
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 13/16] fetch-pack: use the configured transport protocol

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/fetch-pack.c |  22 ++++++++++-
 fetch-pack.c         | 109 +++++++++++++++++++++++++++------------------------
 fetch-pack.h         |   1 +
 3 files changed, 80 insertions(+), 52 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4a6b340..be5c43e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -48,6 +48,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct child_process *conn;
 	struct fetch_pack_args args;
 	struct sha1_array shallow = SHA1_ARRAY_INIT;
+	struct transport_options select_options;
 
 	packet_trace_identity("fetch-pack");
 
@@ -127,6 +128,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.update_shallow = 1;
 			continue;
 		}
+		if (!skip_prefix(arg, "--transport-version", &arg)) {
+			args.version = strtol(arg, NULL, 0);
+			continue;
+		}
 		usage(fetch_pack_usage);
 	}
 
@@ -175,7 +180,22 @@ 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);
+	if (args.version == 0)
+		args.version = DEFAULT_TRANSPORT_VERSION;
+
+	switch (args.version) {
+	case 2:
+
+		get_remote_capabilities(fd[0], NULL, 0);
+		select_capabilities(args, &select_options);
+		request_capabilities(fd[1], &select_options);
+		/* fall through */
+	case 1:
+		get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+		break;
+	default:
+		die("Transport version %d not supported", args.version);
+	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
 			 &shallow, pack_lockfile_ptr);
diff --git a/fetch-pack.c b/fetch-pack.c
index 48526aa..b617128 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -23,7 +23,6 @@ static int prefer_ofs_delta = 1;
 static int no_done;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
-static int agent_supported;
 static struct lock_file shallow_lock;
 static const char *alternate_shallow_file;
 
@@ -43,7 +42,8 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs;
+static struct transport_options select_options;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -257,7 +257,7 @@ static int find_common(struct fetch_pack_args *args,
 	struct strbuf req_buf = STRBUF_INIT;
 	size_t state_len = 0;
 
-	if (args->stateless_rpc && multi_ack == 1)
+	if (args->stateless_rpc && select_options.multi_ack == 1)
 		die("--stateless-rpc requires multi_ack_detailed");
 	if (marked)
 		for_each_ref(clear_marks, NULL);
@@ -290,17 +290,17 @@ static int find_common(struct fetch_pack_args *args,
 		remote_hex = sha1_to_hex(remote);
 		if (!fetching) {
 			struct strbuf c = STRBUF_INIT;
-			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
-			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
-			if (no_done)            strbuf_addstr(&c, " no-done");
-			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
-			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
-			if (args->use_thin_pack) strbuf_addstr(&c, " thin-pack");
-			if (args->no_progress)   strbuf_addstr(&c, " no-progress");
-			if (args->include_tag)   strbuf_addstr(&c, " include-tag");
-			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
-			if (agent_supported)    strbuf_addf(&c, " agent=%s",
-							    git_user_agent_sanitized());
+			if (select_options.multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
+			if (select_options.multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
+			if (select_options.no_done)            strbuf_addstr(&c, " no-done");
+			if (select_options.use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
+			if (select_options.use_sideband == 1)  strbuf_addstr(&c, " side-band");
+			if (select_options.use_thin_pack)      strbuf_addstr(&c, " thin-pack");
+			if (select_options.no_progress)        strbuf_addstr(&c, " no-progress");
+			if (select_options.include_tag)        strbuf_addstr(&c, " include-tag");
+			if (select_options.prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			if (select_options.agent_supported)    strbuf_addf(&c, " agent=%s",
+									   git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
@@ -391,7 +391,7 @@ static int find_common(struct fetch_pack_args *args,
 				switch (ack) {
 				case ACK:
 					flushes = 0;
-					multi_ack = 0;
+					select_options.multi_ack = 0;
 					retval = 0;
 					goto done;
 				case ACK_common:
@@ -440,14 +440,14 @@ done:
 	if (args->verbose)
 		fprintf(stderr, "done\n");
 	if (retval != 0) {
-		multi_ack = 0;
+		select_options.multi_ack = 0;
 		flushes++;
 	}
 	strbuf_release(&req_buf);
 
 	if (!got_ready || !no_done)
 		consume_shallow_list(args, fd[0]);
-	while (flushes || multi_ack) {
+	while (flushes || select_options.multi_ack) {
 		int ack = get_ack(fd[0], result_sha1);
 		if (ack) {
 			if (args->verbose)
@@ -455,7 +455,7 @@ done:
 					sha1_to_hex(result_sha1));
 			if (ack == ACK)
 				return 0;
-			multi_ack = 1;
+			select_options.multi_ack = 1;
 			continue;
 		}
 		flushes--;
@@ -542,7 +542,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if (select_options.allow_tip_sha1_in_want) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -671,7 +671,7 @@ static int get_pack(struct fetch_pack_args *args,
 	int ret;
 
 	memset(&demux, 0, sizeof(demux));
-	if (use_sideband) {
+	if (select_options.use_sideband) {
 		/* xd[] is talking with upload-pack; subprocess reads from
 		 * xd[0], spits out band#2 to stderr, and feeds us band#1
 		 * through demux->out.
@@ -752,7 +752,7 @@ static int get_pack(struct fetch_pack_args *args,
 		close(cmd.out);
 	}
 
-	if (!use_sideband)
+	if (!select_options.use_sideband)
 		/* Closed by start_command() */
 		xd[0] = -1;
 
@@ -763,84 +763,91 @@ static int get_pack(struct fetch_pack_args *args,
 			ret == 0;
 	else
 		die("%s failed", cmd_name);
-	if (use_sideband && finish_async(&demux))
+	if (select_options.use_sideband && finish_async(&demux))
 		die("error in sideband demultiplexer");
 	return 0;
 }
 
-static int cmp_ref_by_name(const void *a_, const void *b_)
-{
-	const struct ref *a = *((const struct ref **)a_);
-	const struct ref *b = *((const struct ref **)b_);
-	return strcmp(a->name, b->name);
-}
-
-static struct ref *do_fetch_pack(struct fetch_pack_args *args,
-				 int fd[2],
-				 const struct ref *orig_ref,
-				 struct ref **sought, int nr_sought,
-				 struct shallow_info *si,
-				 char **pack_lockfile)
+void select_capabilities(struct fetch_pack_args *args, struct transport_options *options)
 {
-	struct ref *ref = copy_ref_list(orig_ref);
-	unsigned char sha1[20];
 	const char *agent_feature;
 	int agent_len;
 
-	sort_ref_list(&ref, ref_compare_name);
-	qsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);
-
 	if (is_repository_shallow() && !server_supports("shallow"))
 		die("Server does not support shallow clients");
 	if (server_supports("multi_ack_detailed")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports multi_ack_detailed\n");
-		multi_ack = 2;
+		options->multi_ack = 2;
 		if (server_supports("no-done")) {
 			if (args->verbose)
 				fprintf(stderr, "Server supports no-done\n");
 			if (args->stateless_rpc)
-				no_done = 1;
+				options->no_done = 1;
 		}
 	}
 	else if (server_supports("multi_ack")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports multi_ack\n");
-		multi_ack = 1;
+		options->multi_ack = 1;
 	}
 	if (server_supports("side-band-64k")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports side-band-64k\n");
-		use_sideband = 2;
+		options->use_sideband = 2;
 	}
 	else if (server_supports("side-band")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports side-band\n");
-		use_sideband = 1;
+		options->use_sideband = 1;
 	}
 	if (server_supports("allow-tip-sha1-in-want")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
-		allow_tip_sha1_in_want = 1;
+		options->allow_tip_sha1_in_want = 1;
 	}
 	if (!server_supports("thin-pack"))
-		args->use_thin_pack = 0;
+		options->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
-		args->no_progress = 0;
+		options->no_progress = 0;
 	if (!server_supports("include-tag"))
-		args->include_tag = 0;
+		options->include_tag = 0;
 	if (server_supports("ofs-delta")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports ofs-delta\n");
+		options->prefer_ofs_delta = 1;
 	} else
-		prefer_ofs_delta = 0;
+		options->prefer_ofs_delta = 0;
 
 	if ((agent_feature = server_feature_value("agent", &agent_len))) {
-		agent_supported = 1;
+		options->agent_supported = 1;
 		if (args->verbose && agent_len)
 			fprintf(stderr, "Server version is %.*s\n",
 				agent_len, agent_feature);
 	}
+}
+
+static int cmp_ref_by_name(const void *a_, const void *b_)
+{
+	const struct ref *a = *((const struct ref **)a_);
+	const struct ref *b = *((const struct ref **)b_);
+	return strcmp(a->name, b->name);
+}
+
+static struct ref *do_fetch_pack(struct fetch_pack_args *args,
+				 int fd[2],
+				 const struct ref *orig_ref,
+				 struct ref **sought, int nr_sought,
+				 struct shallow_info *si,
+				 char **pack_lockfile)
+{
+	struct ref *ref = copy_ref_list(orig_ref);
+	unsigned char sha1[20];
+
+	sort_ref_list(&ref, ref_compare_name);
+	qsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);
+
+	select_capabilities(args, &select_options);
 
 	if (everything_local(args, &ref, sought, nr_sought)) {
 		packet_flush(fd[1]);
diff --git a/fetch-pack.h b/fetch-pack.h
index bb7fd76..b48b4f5 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -10,6 +10,7 @@ struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
 	int depth;
+	int version;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 14/16] t5544: add a test case for the new protocol

[Stefan Beller]

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

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

[RFCv2 15/16] Documentation/technical/pack-protocol: Mention http as possible protocol

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This may go unrelated to this series as well.

 Documentation/technical/pack-protocol.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index fc09c63..4064fc7 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -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
 file:// transports.  There exist two sets of protocols, one for pushing
 data from a client to a server and another for fetching data from a
-server to a client.  All three transports (ssh, git, file) use the same
-protocol to transfer data.
+server to a client.  The three transports (ssh, git, file) use the same
+protocol to transfer data. http is documented in http-protocol.txt.
 
 The processes invoked in the canonical Git implementation are 'upload-pack'
 on the server side and 'fetch-pack' on the client side for fetching data;
-- 
2.4.1.345.gab207b6.dirty

[RFCv2 16/16] Document protocol version 2

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/technical/pack-protocol.txt         | 79 +++++++++++++++++++++--
 Documentation/technical/protocol-capabilities.txt | 15 -----
 2 files changed, 74 insertions(+), 20 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 4064fc7..88bb30a 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -14,6 +14,12 @@ data.  The protocol functions to have a server tell a client what is
 currently on the server, then for the two to negotiate the smallest amount
 of data to send in order to fully update one or the other.
 
+"upload-pack-2" and "receive-pack-2" are the next generation of
+"upload-pack" and "receive-pack" respectively. The first two are
+referred as "version 2" in this document and pack-capabilities.txt
+while the last two are "version 1". Unless stated otherwise, "version 1"
+is implied.
+
 Transports
 ----------
 There are three transports over which the packfile protocol is
@@ -42,7 +48,8 @@ hostname parameter, terminated by a NUL byte.
 
 --
    git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]
-   request-command   = "git-upload-pack" / "git-receive-pack" /
+   request-command   = "git-upload-pack" / "git-upload-pack-2" /
+		       "git-receive-pack" / "git-receive-pack-2" /
 		       "git-upload-archive"   ; case sensitive
    pathname          = *( %x01-ff ) ; exclude NUL
    host-parameter    = "host=" hostname [ ":" port ]
@@ -124,6 +131,49 @@ has, the first can 'fetch' from the second.  This operation determines
 what data the server has that the client does not then streams that
 data down to the client in packfile format.
 
+Capability discovery (v2)
+-------------------------
+
+In version 1, capability discovery is part of reference discovery and
+covered in the reference discovery section.
+
+In version 2, when the client initially connects, the server
+immediately sends its capabilities to the client followed by a flush.
+Then the client must send the list of server capabilities it wants to
+use to the server.
+
+   S: 00XXlang\n
+   S: 00XXthin-pack\n
+   S: 00XXofs-delta\n
+   S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\n
+   S: 0000
+
+   C: 00XXthin-pack\n
+   C: 00XXofs-delta\n
+   C: 00XXlang=en\n
+   C: 00XXagent:agent=git/custom_string\n
+   C: 0000
+
+----
+  capability-list  =  *(capability) flush-pkt
+  capability       =  PKT-LINE(keyvaluepair LF)
+  keyvaluepair     = key ["=" value]
+  key              =  1*(LC_ALPHA / DIGIT / "-" / "_")
+  LC_ALPHA         =  %x61-7A
+  value            = any octet
+
+----
+
+The client MUST ignore any data on pkt-lines with unknown keys.
+
+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.
+
+See protocol-capabilities.txt for a list of allowed server and client
+capabilities and descriptions.
 
 Reference Discovery
 -------------------
@@ -154,10 +204,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.
 
 ----
   advertised-refs  =  (no-refs / list-of-refs)
@@ -185,6 +239,21 @@ MUST peel the ref if it's an annotated tag.
 Server and client MUST use lowercase for obj-id, both MUST treat obj-id
 as case-insensitive.
 
+On the very first line of the initial server response of either
+receive-pack and upload-pack the first reference is followed by
+a NUL byte and then a list of space delimited server capabilities.
+These allow the server to declare what it can and cannot support
+to the client.
+
+Client will then send a space separated list of capabilities it wants
+to be in effect. The client MUST NOT ask for capabilities the server
+did not say it supports.
+
+Server MUST diagnose and abort if capabilities it does not understand
+was sent.  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.
+
 See protocol-capabilities.txt for a list of allowed server capabilities
 and descriptions.
 
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 4f8a7bf..a6241d8 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -3,21 +3,6 @@ Git Protocol Capabilities
 
 Servers SHOULD support all capabilities defined in this document.
 
-On the very first line of the initial server response of either
-receive-pack and upload-pack the first reference is followed by
-a NUL byte and then a list of space delimited server capabilities.
-These allow the server to declare what it can and cannot support
-to the client.
-
-Client will then send a space separated list of capabilities it wants
-to be in effect. The client MUST NOT ask for capabilities the server
-did not say it supports.
-
-Server MUST diagnose and abort if capabilities it does not understand
-was sent.  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.
-
 The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
 capabilities are sent and recognized by the receive-pack (push to server)
 process.
-- 
2.4.1.345.gab207b6.dirty

Re: [RFCv2 01/16] stringlist: add from_space_separated_string

[Duy Nguyen]

On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
> diff --git a/string-list.h b/string-list.h
> index d3809a1..88c18e9 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -19,6 +19,7 @@ struct string_list {
>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>
>  void string_list_init(struct string_list *list, int strdup_strings);
> +void from_space_separated_string(struct string_list *list, char *line);

The name feels out of place. All functions in here have "string_list"
somewhere in their names. The implementation looks very close to
string_list_split() but that name's already taken.. Maybe
string_list_split_by_space()?
-- 
Duy

Re: [RFCv2 13/16] fetch-pack: use the configured transport protocol

[Duy Nguyen]

On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
> @@ -127,6 +128,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>                         args.update_shallow = 1;
>                         continue;
>                 }
> +               if (!skip_prefix(arg, "--transport-version", &arg)) {

I think this line should be "if (skip_prefix(arg,
"--transport-version=", &arg)) {".
-- 
Duy

Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Duy Nguyen]

On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     name it to_free
>
>  transport.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 651f0ac..b49fc60 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,28 @@ 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 *to_free = 0;
>
>         if (data->conn)
>                 return 0;
>
> +       remote_program = (for_push ? data->options.receivepack
> +                                  : data->options.uploadpack);
> +
> +       if (transport->smart_options->transport_version >= 2) {
> +               to_free = xmalloc(strlen(remote_program) + 12);
> +               sprintf(to_free, "%s-%d", remote_program,
> +                       transport->smart_options->transport_version);
> +               remote_program = to_free;
> +       }
> +

It looks to me that the caller should pass "upload-pack-2" here in
data->options.uploadpack already. We should not need to manipulate the
uploadpack's program name. Not sure how complicated it would be
though.

>         data->conn = git_connect(data->fd, transport->url,
> -                                for_push ? data->options.receivepack :
> -                                data->options.uploadpack,
> +                                remote_program,
>                                  verbose ? CONNECT_VERBOSE : 0);
>
> +       free(to_free);
> +
>         return 0;
>  }
>
> --
> 2.4.1.345.gab207b6.dirty
>



-- 
Duy

Re: [RFCv2 13/16] fetch-pack: use the configured transport protocol

[Duy Nguyen]

On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
>  builtin/fetch-pack.c |  22 ++++++++++-
>  fetch-pack.c         | 109 +++++++++++++++++++++++++++------------------------
>  fetch-pack.h         |   1 +
>  3 files changed, 80 insertions(+), 52 deletions(-)

And the companion changes in transport-helper.c should be in this
patch as well to support smart http. I don't think there is any
problem with how you store the "version" (or "transport_version", you
should probably stick to one name) though.
-- 
Duy

Re: [RFCv2 13/16] fetch-pack: use the configured transport protocol

[Ilari Liusvaara]

On Tue, Jun 02, 2015 at 05:02:45PM +0700, Duy Nguyen wrote:
> On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
> >  builtin/fetch-pack.c |  22 ++++++++++-
> >  fetch-pack.c         | 109 +++++++++++++++++++++++++++------------------------
> >  fetch-pack.h         |   1 +
> >  3 files changed, 80 insertions(+), 52 deletions(-)
> 
> And the companion changes in transport-helper.c should be in this
> patch as well to support smart http. I don't think there is any
> problem with how you store the "version" (or "transport_version", you
> should probably stick to one name) though.

Looking at transport-helper.c, process_connect() looks to need patching,
it handles smart transport establishment via remote helpers.

Looking at the routine it calls, both the name and exec look to need
patching.

I think that if process_connect() succeeds, then connect_setup() will
hit the "if (data->conn) return 0;" case and exit early.

transport-helper.c doesn't look to have anything smart-http specific.


-Ilari

Re: [RFCv2 01/16] stringlist: add from_space_separated_string

[Eric Sunshine]

On Tue, Jun 2, 2015 at 5:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
>> diff --git a/string-list.h b/string-list.h
>> index d3809a1..88c18e9 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -19,6 +19,7 @@ struct string_list {
>>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>>
>>  void string_list_init(struct string_list *list, int strdup_strings);
>> +void from_space_separated_string(struct string_list *list, char *line);
>
> The name feels out of place. All functions in here have "string_list"
> somewhere in their names. The implementation looks very close to
> string_list_split() but that name's already taken.. Maybe
> string_list_split_by_space()?

Indeed. If you really want to go the specialized route, splitting only
on whitespace, then Duy's suggestion makes sense. Alternately,
string_list_split_ws() might be easily understood while still
remaining somewhat terse.

However, why make this so specialized? A more generalized function
could be more widely useful. For instance, you could introduce a
function very similar to string_list_split() to which you supply
multiple delimiter characters (as a 'const char *') rather than the
single delimiter character accepted by string_list_split(). The
function could be named string_list_split_any() or
string_list_tokenize().

Also, it's ugly and inconvenient to require the incoming string be
non-const, and feels as if you're letting the interface be dictated by
an implementation detail (underlying use of strtok_r).

Re: [RFCv2 01/16] stringlist: add from_space_separated_string

[Stefan Beller]

On Tue, Jun 2, 2015 at 8:10 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 2, 2015 at 5:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
>>> diff --git a/string-list.h b/string-list.h
>>> index d3809a1..88c18e9 100644
>>> --- a/string-list.h
>>> +++ b/string-list.h
>>> @@ -19,6 +19,7 @@ struct string_list {
>>>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>>>
>>>  void string_list_init(struct string_list *list, int strdup_strings);
>>> +void from_space_separated_string(struct string_list *list, char *line);
>>
>> The name feels out of place. All functions in here have "string_list"
>> somewhere in their names. The implementation looks very close to
>> string_list_split() but that name's already taken.. Maybe
>> string_list_split_by_space()?
>
> Indeed. If you really want to go the specialized route, splitting only
> on whitespace, then Duy's suggestion makes sense. Alternately,
> string_list_split_ws() might be easily understood while still
> remaining somewhat terse.
>
> However, why make this so specialized? A more generalized function
> could be more widely useful. For instance, you could introduce a
> function very similar to string_list_split() to which you supply
> multiple delimiter characters (as a 'const char *') rather than the
> single delimiter character accepted by string_list_split(). The
> function could be named string_list_split_any() or
> string_list_tokenize().
>
> Also, it's ugly and inconvenient to require the incoming string be
> non-const, and feels as if you're letting the interface be dictated by
> an implementation detail (underlying use of strtok_r).

I see. I think I can even use string_list_split here, and drop this patch.

Thanks for pointing out the flaws!

Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Stefan Beller]

On Tue, Jun 2, 2015 at 2:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     name it to_free
>>
>>  transport.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/transport.c b/transport.c
>> index 651f0ac..b49fc60 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -496,15 +496,28 @@ 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 *to_free = 0;
>>
>>         if (data->conn)
>>                 return 0;
>>
>> +       remote_program = (for_push ? data->options.receivepack
>> +                                  : data->options.uploadpack);
>> +
>> +       if (transport->smart_options->transport_version >= 2) {
>> +               to_free = xmalloc(strlen(remote_program) + 12);
>> +               sprintf(to_free, "%s-%d", remote_program,
>> +                       transport->smart_options->transport_version);
>> +               remote_program = to_free;
>> +       }
>> +
>
> It looks to me that the caller should pass "upload-pack-2" here in
> data->options.uploadpack already. We should not need to manipulate the
> uploadpack's program name. Not sure how complicated it would be
> though.
>

I tried that before as it seemed to be the better approach to me. But
there are multiple
occasions where you can overwrite the "upload-pack" string. It can be
a repository
option or globally configured or coming from a command line argument.

And keeping track of all these places and both passing around the
version number as
well as the binary name seemed cumbersome to me when seeing my implementation.
This way we only have a very small change and you can tell the version
number from
the name, which is an advantage (the version is fixed and will not be
negotiable, so you
need some way to tell the protocol version and the name seems to be
the obvious choice).

So if there are no strong arguments for doing it the other way, I'd
like to keep it this way.

Re: [RFCv2 03/16] connect: rewrite feature parsing to work on string_list

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

The change in 2/16 to extract something into a helper was sort-of
guessable without any comment (i.e. perhaps it will gain new callers
in later patches), but a change like this needs to hint why this is
a good thing to do, I would think.

I think you'll be rerolling this anyway due to droppage of 1/16, so
hopefully we'll see the next version a bit better explained than
this round ;-)

Thanks.

>  builtin/receive-pack.c | 13 +++++----
>  connect.c              | 79 ++++++++++++++++++++++----------------------------
>  connect.h              |  2 +-
>  upload-pack.c          | 11 +++++--
>  4 files changed, 51 insertions(+), 54 deletions(-)

Re: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Subject: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

Nit; s/I/i/, to match others in the series, I think.

> In upload-pack-2 we send each capability in its own packet buffer.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     Moved the capabilities into a struct containing all the capabilities,
>     and then we selectively cancel out unwanted capabilities.

> 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
> @@ -0,0 +1 @@
> +upload-pack.c
> \ No newline at end of file

Yuck.

Can't we do an equivalent without this symbolic link, i.e. a new
Makefile rule to compile upload-pack.c in two different ways to two
different object files?

The way this patch is organized makes it unclear which part is what
was added for v2 and which part is shared with v1 (and changes can
be possible breakage to the existing code), leading to a patch that
is hard to review.

> diff --git a/upload-pack.c b/upload-pack.c
> index 2493964..7477ca3 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -716,33 +716,69 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
>  		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
>  }
>  
> +static int advertise_capabilities = 1;
> +const char *all_capabilities[] = {
> +	"multi_ack",
> +	"thin-pack",
> +	"side-band",
> +	"side-band-64k",
> +	"ofs-delta",
> +	"shallow",
> +	"no-progress",
> +	"include-tag",
> +	"multi_ack_detailed",
> +	"allow-tip-sha1-in-want",
> +	"no-done",
> +};
> +
> +static void send_capabilities(void)

This looks like send-capabilities-v2.  I am OK to share code between
v1 and v2 by having two implementations in the same file and some
(or major part of) helper functions or overall structure code shared
between the two, but if you are taking that route, a version specific
helper like this needs to be made clear which one is which.

> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
> +		const char *cap = all_capabilities[i];
> +		if (!strcmp(cap, "allow-tip-sha1-in-want") && !allow_tip_sha1_in_want)
> +			continue;
> +		if (!strcmp(cap, "no-done") && !stateless_rpc)
> +			continue;
> +		packet_write(1,"%s", cap);

s/1,/1, /;

>  static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
>  {
> -	static const char *capabilities = "multi_ack thin-pack side-band"
> -		" side-band-64k ofs-delta shallow no-progress"
> -		" include-tag multi_ack_detailed";
> +

And is this one only for v1?

>  	const char *refname_nons = strip_namespace(refname);
>  	unsigned char peeled[20];
>  
>  	if (mark_our_ref(refname, sha1))
>  		return 0;
>  
> -	if (capabilities) {
> -		struct strbuf symref_info = STRBUF_INIT;
> -
> -		format_symref_info(&symref_info, cb_data);
> -		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
> -			     sha1_to_hex(sha1), refname_nons,
> -			     0, capabilities,
> -			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
> -			     stateless_rpc ? " no-done" : "",
> -			     symref_info.buf,
> -			     git_user_agent_sanitized());
> -		strbuf_release(&symref_info);
> +	if (advertise_capabilities) {
> +		int i;
> +		struct strbuf capabilities = STRBUF_INIT;
> +
> +		for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
> +			const char *cap = all_capabilities[i];
> +			if (!strcmp(cap, "allow-tip-sha1-in-want") && !allow_tip_sha1_in_want)
> +				continue;
> +			if (!strcmp(cap, "no-done") && !stateless_rpc)
> +				continue;
> +			strbuf_addf(&capabilities, " %s", cap);
> +		}
> +
> +		format_symref_info(&capabilities, cb_data);
> +		strbuf_addf(&capabilities, " agent=%s", git_user_agent_sanitized());
> +
> +		packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname_nons,
> +			     0, capabilities.buf);
> +		strbuf_release(&capabilities);
> +		advertise_capabilities = 0;

The change itself may be going in the right direction, but I'd
suggest doing this in two steps:

 * refactor existing v1 without adding anything v2 specific
   (e.g. send-capabilities above).  A new file-scope global
   all_capabilities[] array, use of it in this new implementation of
   send_ref(), and introduction of the separate
   advertise_capabilities bool, are good examples of refactoring of
   v1.

 * then on top of that solid foundation, add v2 specific stuff.

Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> No function uses the return value of get_remote_heads, so we don't want
> to confuse readers by it.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

This is somewhat a sad change, as the returned value is designed to
be useful if caller wants to continue appending to the list.

Now such a caller has to tangle the list (the variable it gave the
function as the fourth argument) itself to find its tail.

Does it really "confuse" readers enough that it hurts to have a
return value?

>  connect.c | 10 ++++------
>  remote.h  |  8 ++++----
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 4295ba1..a2c777e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -108,10 +108,10 @@ static void annotate_refs_with_symref_info(struct ref *ref)
>  /*
>   * Read all the refs from the other end
>   */
> -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,
> -			      struct sha1_array *shallow_points)
> +void get_remote_heads(int in, char *src_buf, size_t src_len,
> +		      struct ref **list, unsigned int flags,
> +		      struct sha1_array *extra_have,
> +		      struct sha1_array *shallow_points)
>  {
>  	struct ref **orig_list = list;
>  	int got_at_least_one_head = 0;
> @@ -172,8 +172,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  	}
>  
>  	annotate_refs_with_symref_info(*orig_list);
> -
> -	return list;
>  }
>  
>  static const char *parse_feature_value(struct string_list *feature_list, const char *feature, int *lenp)
> diff --git a/remote.h b/remote.h
> index 02d66ce..d5242b0 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -144,10 +144,10 @@ int check_ref_type(const struct ref *ref, int flags);
>  void free_refs(struct ref *ref);
>  
>  struct sha1_array;
> -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,
> -				     struct sha1_array *shallow);
> +extern void get_remote_heads(int in, char *src_buf, size_t src_len,
> +			     struct ref **list, unsigned int flags,
> +			     struct sha1_array *extra_have,
> +			     struct sha1_array *shallow);
>  
>  int resolve_remote_symref(struct ref *ref, struct ref *list);
>  int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);

Re: [RFCv2 06/16] remote.h: add new struct for options

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Why?

>  remote.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/remote.h b/remote.h
> index d5242b0..16cacfe 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -56,6 +56,20 @@ struct remote {
>  	char *http_proxy;
>  };
>  
> +/* todo: discuss if this should be merged with
> + * git_transport_options in transport.c */
> +struct transport_options {
> +	unsigned multi_ack : 2;
> +	unsigned no_done : 1;
> +	unsigned use_thin_pack : 1;
> +	unsigned no_progress : 1;
> +	unsigned include_tag : 1;
> +	unsigned prefer_ofs_delta : 1;
> +	unsigned agent_supported : 1;
> +	unsigned allow_tip_sha1_in_want : 1;
> +	unsigned use_sideband;
> +};
> +
>  struct remote *remote_get(const char *name);
>  struct remote *pushremote_get(const char *name);
>  int remote_is_configured(const char *name);

Re: [RFCv2 09/16] remote.h: add get_remote_capabilities, request_capabilities

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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>
> ---
>  connect.c | 37 +++++++++++++++++++++++++++++++++++++
>  remote.h  |  3 +++
>  2 files changed, 40 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index a2c777e..4ebe1dc 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -105,6 +105,43 @@ 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)

We need to clarify that this is for v2 only in its name, no?

> +{
> +	string_list_clear(&server_capabilities, 1);
> +	for (;;) {
> +		int len;
> +		char *line = packet_buffer;
> +
> +		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;

OK, so we get sequence of capabilities, one per packet, until
packet_flush().  I wonder if we want to hint that with:

		if (!len)
			break; /* flush */

or something like that.

> +		string_list_append(&server_capabilities, line);
> +	}
> +}

> +int request_capabilities(int out, struct transport_options *options)
> +{
> +	if (options->multi_ack == 2)    packet_write(out, "multi_ack_detailed");

Spell these just like you would do any other functions, i.e.

	if (options->multi_ack == 2)
        	packet_write(out, "multi_ack_detailed");


I think this step is sensible, and anticipate that a new method that
corresponds to this will be introduced to the transport layer.

Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

[Stefan Beller]

On Tue, Jun 2, 2015 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> No function uses the return value of get_remote_heads, so we don't want
>> to confuse readers by it.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> This is somewhat a sad change, as the returned value is designed to
> be useful if caller wants to continue appending to the list.

But there are no callers since like 2005. ;)
(I did not exactly track it down, but even the last caller went away
rather fast)

>
> Does it really "confuse" readers enough that it hurts to have a
> return value?
>

Probably no.
But I think this is just carrying around cruft we could avoid?

>
> Now such a caller has to tangle the list (the variable it gave the
> function as the fourth argument) itself to find its tail.

So you're saying if someone in the future really wants to append
to that list, they don't find out to just return it again but rather
do an O(n) operation?

Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     name it to_free
>
>  transport.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 651f0ac..b49fc60 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,28 @@ 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 *to_free = 0;

	char *to_free = NULL;

> +	remote_program = (for_push ? data->options.receivepack
> +				   : data->options.uploadpack);
> +
> +	if (transport->smart_options->transport_version >= 2) {
> +		to_free = xmalloc(strlen(remote_program) + 12);
> +		sprintf(to_free, "%s-%d", remote_program,
> +			transport->smart_options->transport_version);
> +		remote_program = to_free;
> +	}

Hmph, so everybody else thinks it is interacting with 'upload-pack',
and this is the only function that knows it is actually talking with
'upload-pack-2'?

I am wondering why there isn't a separate helper function that
munges data->options.{uploadpack,receivepack} fields based on
the value of transport_version that is called _before_ this function
is called.

Also, how does this interact with the name of the program the end
user can specify via "fetch --upload-pack=<program name>" option?

>  	data->conn = git_connect(data->fd, transport->url,
> -				 for_push ? data->options.receivepack :
> -				 data->options.uploadpack,
> +				 remote_program,
>  				 verbose ? CONNECT_VERBOSE : 0);
>  
> +	free(to_free);
> +
>  	return 0;
>  }

Re: [RFCv2 06/16] remote.h: add new struct for options

[Stefan Beller]

On Tue, Jun 2, 2015 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Why?
>

To have all options required for selecting the capabilities
together in one struct.

Currently there are independent variables used in a few places for this
(fetchpack.c: lines 296 - 309, which is where I also got the formatting from)

As soon as I realized I need to touch many places I thought about introducing
this struct.  But I guess I'll drop this as well as the patches
building on top of this
because it's actually only fetch-pack.c using it and it would have communicated
with the transport layer with this struct.

Maybe it is better to let the caller handle each option anyway, so that the
transport layer just invokes a callback at the caller (fetchpack or later on
sendpack) which immediately decides if it knows the capability and needs to
act on it.

Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jun 2, 2015 at 2:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> No function uses the return value of get_remote_heads, so we don't want
>>> to confuse readers by it.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>
>> This is somewhat a sad change, as the returned value is designed to
>> be useful if caller wants to continue appending to the list.
>
> But there are no callers since like 2005. ;)

That is not a good excuse when it comes to the API design to deal
with linked lists in a bog-standard way, though.  Many callers of
commit_list_insert() may ignore its return values, but even when the
last caller stopped caring it, that does not necessarily mean the
feature can be removed.

Re: [RFCv2 06/16] remote.h: add new struct for options

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jun 2, 2015 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Why?
>>
>
> To have all options required for selecting the capabilities
> together in one struct.

No need to explain in the e-mail, as that won't be kept in our
history, unless you write it in your reroll.  Just keep them in mind
and try not to forget when you reroll ;-).

Thanks.

Re: [RFCv2 11/16] remote: have preselect_capabilities

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Why?

When adding a new thing that nobody uses yet, please explain what it
is used for and how it would help the callers in what way to help
reviewers.

>  connect.c | 28 ++++++++++++++++++++++++++++
>  remote.h  |  1 +
>  2 files changed, 29 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index 4ebe1dc..752b9a5 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -126,6 +126,34 @@ void get_remote_capabilities(int in, char *src_buf, size_t src_len)
>  	}
>  }
>  
> +/* just select all options the server advertised. */
> +void preselect_capabilities(struct transport_options *options)
> +{
> +	if (is_repository_shallow() && !server_supports("shallow"))
> +		die("Server does not support shallow clients");
> +
> +	if (server_supports("multi_ack"))
> +		options->multi_ack = 1;
> +	else if (server_supports("multi_ack_detailed"))
> +		options->multi_ack = 2;
> +
> +	if (server_supports("side-band"))
> +		options->use_sideband = 1;
> +	else if (server_supports("side-band-64k"))
> +		options->use_sideband = 2;
> +
> +	if (server_supports("no-done"))
> +		options->no_done = 1;
> +	if (server_supports("thin-pack"))
> +		options->use_thin_pack = 1;
> +	if (server_supports("no-progress"))
> +		options->no_progress = 1;
> +	if (server_supports("include-tag"))
> +		options->include_tag = 1;
> +	if (server_supports("ofs-delta"))
> +		options->prefer_ofs_delta = 1;
> +}
> +
>  int request_capabilities(int out, struct transport_options *options)
>  {
>  	if (options->multi_ack == 2)    packet_write(out, "multi_ack_detailed");
> diff --git a/remote.h b/remote.h
> index 61619c5..264a513 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -166,6 +166,7 @@ extern void get_remote_heads(int in, char *src_buf, size_t src_len,
>  			     struct sha1_array *shallow);
>  
>  void get_remote_capabilities(int in, char *src_buf, size_t src_len);
> +void preselect_capabilities(struct transport_options *options);
>  int request_capabilities(int out, struct transport_options*);
>  
>  int resolve_remote_symref(struct ref *ref, struct ref *list);

Re: [RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs.

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     A minor issue I am unsure about here is the
>     line
>     	&& transport->smart_options->transport_version)
>     which could be prevented if we always set the transport_version
>     in make_remote to be the default remote version.
>     
>     The advantage of having it always set in make_remote would
>     be a cleaner mind model (the version set is always accurate)
>     as opposed to now (version may be 0, then the default
>     applies as we don't care enough to set a version)
>     
>     However I think the code may be more ugly if we were to
>     always set the version in make_remote as then we would need
>     to move the DEFAULT_TRANSPORT_VERSION define into remote.h
>     or somewhere else (transport.h is not included in remote.c,
>     I guess that's on purpose?)

Interesting.  After reading 9/16, I was somehow expecting to see
that a new method get_capability() would be added to the transport
layer, and a function get_capability_via_connect() that calls
get_remote_capabilities() would be its implementation for the
"connect" transport.

The capability thing however is limited to the Git-aware aka smart
transports and it is unlikely that other transports would benefit
from such an organization, so I think the way this step integrates
the new get_remote_capabilities() to the system would be not just
sufficient but is more appropriate.

If you do not like the switching based on version in this function,
however, it is probably an option to add the new method and define
connect_v1 and connect_v2 as two separate transports.  The latter
would have get_capability() method, while the former (and all the
other transports) does not define it.  And the overall transport
layer would call get_capability() method when defined, and then
get_refs() method next, or something like that.

>  transport.c | 28 ++++++++++++++++++++++++----
>  transport.h |  6 ++++++
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index b49fc60..ba40677 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -523,14 +523,33 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
>  
>  static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
>  {
> +	struct transport_options options;
>  	struct git_transport_data *data = transport->data;
>  	struct ref *refs;
> +	int version = DEFAULT_TRANSPORT_VERSION;
>  
> +	if (transport->smart_options
> +	    && transport->smart_options->transport_version)
> +		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) {
> +	case 2: /* first talk about capabilities, then get the heads */
> +		get_remote_capabilities(data->fd[0], NULL, 0);
> +		preselect_capabilities(&options);
> +		if (transport->select_capabilities)
> +			transport->select_capabilities(&options);
> +		request_capabilities(data->fd[1], &options);
> +		/* 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;
> +	}
>  	data->got_remote_heads = 1;
>  
>  	return refs;
> @@ -987,6 +1006,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  		struct git_transport_data *data = xcalloc(1, sizeof(*data));
>  		ret->data = data;
>  		ret->set_option = NULL;
> +		ret->select_capabilities = NULL;
>  		ret->get_refs_list = get_refs_via_connect;
>  		ret->fetch = fetch_refs_via_pack;
>  		ret->push_refs = git_transport_push;
> diff --git a/transport.h b/transport.h
> index 6095d7a..3e63efc 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -74,6 +74,12 @@ struct transport {
>  	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
>  
>  	/**
> +	 * A callback to select protocol options. Must be set if
> +	 * the caller wants to change transport options.
> +	 */
> +	void (*select_capabilities)(struct transport_options *);
> +
> +	/**
>  	 * Push the objects and refs. Send the necessary objects, and
>  	 * then, for any refs where peer_ref is set and
>  	 * peer_ref->new_sha1 is different from old_sha1, tell the

Re: [RFCv2 15/16] Documentation/technical/pack-protocol: Mention http as possible protocol

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This may go unrelated to this series as well.

Yeah, this can come before this series as a good independent
clean-up.

>
>  Documentation/technical/pack-protocol.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt
> b/Documentation/technical/pack-protocol.txt
> index fc09c63..4064fc7 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -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
>  file:// transports.  There exist two sets of protocols, one for pushing
>  data from a client to a server and another for fetching data from a
> -server to a client.  All three transports (ssh, git, file) use the same
> -protocol to transfer data.
> +server to a client.  The three transports (ssh, git, file) use the same
> +protocol to transfer data. http is documented in http-protocol.txt.
>  
>  The processes invoked in the canonical Git implementation are 'upload-pack'
>  on the server side and 'fetch-pack' on the client side for fetching data;

Re: [RFCv2 15/16] Documentation/technical/pack-protocol: Mention http as possible protocol

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> This may go unrelated to this series as well.
>
> Yeah, this can come before this series as a good independent
> clean-up.

I'll pick this up and queue (with s/: Mention/: mention/) separately
for 'next'.

Thanks.

Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Stefan Beller]

On Tue, Jun 2, 2015 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     name it to_free
>>
>>  transport.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/transport.c b/transport.c
>> index 651f0ac..b49fc60 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -496,15 +496,28 @@ 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 *to_free = 0;
>
>         char *to_free = NULL;
>
>> +     remote_program = (for_push ? data->options.receivepack
>> +                                : data->options.uploadpack);
>> +
>> +     if (transport->smart_options->transport_version >= 2) {
>> +             to_free = xmalloc(strlen(remote_program) + 12);
>> +             sprintf(to_free, "%s-%d", remote_program,
>> +                     transport->smart_options->transport_version);
>> +             remote_program = to_free;
>> +     }
>
> Hmph, so everybody else thinks it is interacting with 'upload-pack',
> and this is the only function that knows it is actually talking with
> 'upload-pack-2'?

Yes.

>
> I am wondering why there isn't a separate helper function that
> munges data->options.{uploadpack,receivepack} fields based on
> the value of transport_version that is called _before_ this function
> is called.

That makes sense.

>
> Also, how does this interact with the name of the program the end
> user can specify via "fetch --upload-pack=<program name>" option?

You'd specify --upload-pack=foo-frotz and --transport-version=2
and it would look for foo-frotz-2 instead.

The problem IMHO is we have quite a few places where the
upload-pack binary path can be configured. Either as a command line
option or as a repository configuration.

And the way we're currently architecting the next protocol, the version
is encoded in the file name, which makes sense (an old binary will not
accept a new protocol), so what should happen when

* there is a repository configuration "upload-pack-custom" for upload-pack
   for historic reasons. When just switching to a new version, you would need
   to add a "upload-pack-custom-2" binary on the server side anyway

* additionally to the configured value you want to play around with the new
  protocol, so would you rather just say "--transport-version=2" or also need
  to have some sort of "--upload-pack=upload-pack-custom-another-path"
  involved? It's easy to forget the second option I believe.

* the user specifies
"--upload-pack=custom-upload-pack-which-talks-version1" and
 "--transport-version=2" together. This will fail, but at which stage do we
  want to fail?

All these questions lead me to think it's maybe better to make the rest of Git
unaware of the added "-${version}" string and pretend we would be talking to
upload-pack instead.

Re: [RFCv2 12/16] transport: get_refs_via_connect exchanges capabilities before refs.

[Stefan Beller]

On Tue, Jun 2, 2015 at 2:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     A minor issue I am unsure about here is the
>>     line
>>       && transport->smart_options->transport_version)
>>     which could be prevented if we always set the transport_version
>>     in make_remote to be the default remote version.
>>
>>     The advantage of having it always set in make_remote would
>>     be a cleaner mind model (the version set is always accurate)
>>     as opposed to now (version may be 0, then the default
>>     applies as we don't care enough to set a version)
>>
>>     However I think the code may be more ugly if we were to
>>     always set the version in make_remote as then we would need
>>     to move the DEFAULT_TRANSPORT_VERSION define into remote.h
>>     or somewhere else (transport.h is not included in remote.c,
>>     I guess that's on purpose?)
>
> Interesting.  After reading 9/16, I was somehow expecting to see
> that a new method get_capability() would be added to the transport
> layer, and a function get_capability_via_connect() that calls
> get_remote_capabilities() would be its implementation for the
> "connect" transport.

This suggests that we collect the capabilities at first and then handover
a (possibly huge) list to the upper calling layer with all our capabilities.

Peff pointed out last Friday (I re-read the email on Monday unfortunately),
that we don't want to have buffers of capabilities at all for future extension.

So the design we need to actually have here is to have a callback given to the
transport layer, which calls this callback for each capability received.
Then the upper layer must decide for each capability if it knows it and wants
to store it or drop it or just set a flag for it.

>
> The capability thing however is limited to the Git-aware aka smart
> transports and it is unlikely that other transports would benefit
> from such an organization, so I think the way this step integrates
> the new get_remote_capabilities() to the system would be not just
> sufficient but is more appropriate.
>
> If you do not like the switching based on version in this function,
> however, it is probably an option to add the new method and define
> connect_v1 and connect_v2 as two separate transports.  The latter
> would have get_capability() method, while the former (and all the
> other transports) does not define it.  And the overall transport
> layer would call get_capability() method when defined, and then
> get_refs() method next, or something like that.

The way you quoted my previous email, it seems to me as if you specially
want to address my concerns from the notes.
However my actual concern was only focused on the one line if we do

+       if (transport->smart_options
+           && transport->smart_options->transport_version) // <-- this line
+               version = transport->smart_options->transport_version;

or rather want to have it a bit cleaner with

+       if (transport->smart_options)
+               version = transport->smart_options->transport_version;

This second approach assumes transport->smart_options->transport_version;
to be not NULL, i.e. when setting up the smart options we must make the
decision which protocol version to use. At this point in time this would be
just initializing the transport_version correctly in remote.h. But that's more
lines than just this one.


>
>>  transport.c | 28 ++++++++++++++++++++++++----
>>  transport.h |  6 ++++++
>>  2 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/transport.c b/transport.c
>> index b49fc60..ba40677 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -523,14 +523,33 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
>>
>>  static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
>>  {
>> +     struct transport_options options;
>>       struct git_transport_data *data = transport->data;
>>       struct ref *refs;
>> +     int version = DEFAULT_TRANSPORT_VERSION;
>>
>> +     if (transport->smart_options
>> +         && transport->smart_options->transport_version)
>> +             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) {
>> +     case 2: /* first talk about capabilities, then get the heads */
>> +             get_remote_capabilities(data->fd[0], NULL, 0);
>> +             preselect_capabilities(&options);
>> +             if (transport->select_capabilities)
>> +                     transport->select_capabilities(&options);
>> +             request_capabilities(data->fd[1], &options);
>> +             /* 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;
>> +     }
>>       data->got_remote_heads = 1;
>>
>>       return refs;
>> @@ -987,6 +1006,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>>               struct git_transport_data *data = xcalloc(1, sizeof(*data));
>>               ret->data = data;
>>               ret->set_option = NULL;
>> +             ret->select_capabilities = NULL;
>>               ret->get_refs_list = get_refs_via_connect;
>>               ret->fetch = fetch_refs_via_pack;
>>               ret->push_refs = git_transport_push;
>> diff --git a/transport.h b/transport.h
>> index 6095d7a..3e63efc 100644
>> --- a/transport.h
>> +++ b/transport.h
>> @@ -74,6 +74,12 @@ struct transport {
>>       int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
>>
>>       /**
>> +      * A callback to select protocol options. Must be set if
>> +      * the caller wants to change transport options.
>> +      */
>> +     void (*select_capabilities)(struct transport_options *);
>> +
>> +     /**
>>        * Push the objects and refs. Send the necessary objects, and
>>        * then, for any refs where peer_ref is set and
>>        * peer_ref->new_sha1 is different from old_sha1, tell the

Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Hmph, so everybody else thinks it is interacting with 'upload-pack',
>> and this is the only function that knows it is actually talking with
>> 'upload-pack-2'?
>
> Yes.
>>
>> I am wondering why there isn't a separate helper function that
>> munges data->options.{uploadpack,receivepack} fields based on
>> the value of transport_version that is called _before_ this function
>> is called.
>
> That makes sense.

Hmph, but then everybody would know that it is now interacting with
upload-pack-2; I think it probably is a better way to go.

In any case, all codepaths other than what actually runs exec()
should not be basing their decision on the program names---that is
what you added transport_version field for, and they should look
at that field if they want to switch behaviour between protocol
versions anyway, so I think we can live with either way.

>> Also, how does this interact with the name of the program the end
>> user can specify via "fetch --upload-pack=<program name>" option?
>
> You'd specify --upload-pack=foo-frotz and --transport-version=2
> and it would look for foo-frotz-2 instead.

Hmm, that's an unfortunate interaction.  If you wrote a new protocol
driver that talks v2, it may be natural to say

	git fetch --upload-pack=a.out --transport-version=2

when you want to test it, and we do not want to invoke a.out-2 in
that case.

> The problem IMHO is we have quite a few places where the
> upload-pack binary path can be configured. Either as a command line
> option or as a repository configuration.

At least shouldn't you be able to tell if we are using compiled-in
default or user specified value (whether configuration/command line)?

Does the code that initialise data->options.{upload,receive}pack
fields with the compiled-in default values know what protocol
version is going to be used at that point?  I think that is where
the appending of "-2" should be done; in other words, I think
addition of "-2" should be done _ONLY_ for compiled-in default
values, and it should be done not just for exec() but should be
visible to everybody, including those who are debugging and inspect
data->options.uploadpack field.  What we spawn must match what is
stored there.

Re: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

[Stefan Beller]

On Tue, Jun 2, 2015 at 11:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Subject: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack
>
> Nit; s/I/i/, to match others in the series, I think.
>
>> In upload-pack-2 we send each capability in its own packet buffer.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     Moved the capabilities into a struct containing all the capabilities,
>>     and then we selectively cancel out unwanted capabilities.
>
>> 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
>> @@ -0,0 +1 @@
>> +upload-pack.c
>> \ No newline at end of file
>
> Yuck.
>
> Can't we do an equivalent without this symbolic link, i.e. a new
> Makefile rule to compile upload-pack.c in two different ways to two
> different object files?

Ok I changed that and it works now (only one upload-pack.c file no
upload-pack-2.c and no corresponding object either.)

However we don't want to have the version used in upload pack depending
on the file name at run time, which is why I am reverting to this state
and depending on the file name at compile time. Instead of a symlink we
could use an option passed into the compiler as well, but I am not sure if
that is as easy to add to the Makefile as this way.

>
> The way this patch is organized makes it unclear which part is what
> was added for v2 and which part is shared with v1 (and changes can
> be possible breakage to the existing code), leading to a patch that
> is hard to review.

ok :(

Changed in a reroll.

Re: [RFCv2 14/16] t5544: add a test case for the new protocol

[Eric Sunshine]

On Mon, Jun 1, 2015 at 8:02 PM, Stefan Beller <sbeller@google.com> wrote:
> 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

Broken &&-chain (still[1]).

[1]: http://article.gmane.org/gmane.comp.version-control.git/270014

> +               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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html