[PATCH v3 00/10] protocol transition

[Brandon Williams <bmwill@google.com>]

Changes in v3:
 * added a new ssh variant 'simple' and update documentation to better reflect
   the command-line parameters passed to the ssh command.
 * updated various commit messages based on feedback.
 * tighten the wording for 'GIT_PROTOCOL' to indicate that both unknown keys
   and values must be ignored.
 * added API comments for functions in protocol.h
 * updated various tests in t5700 based on reviewer feedback

Brandon Williams (9):
  pkt-line: add packet_write function
  protocol: introduce protocol extention mechanisms
  daemon: recognize hidden request arguments
  upload-pack, receive-pack: introduce protocol version 1
  connect: teach client to recognize v1 server response
  connect: tell server that the client understands v1
  http: tell server that the client understands v1
  i5700: add interop test for protocol transition
  ssh: introduce a 'simple' ssh variant

Jonathan Tan (1):
  connect: in ref advertisement, shallows are last

 Documentation/config.txt               |  44 +++-
 Documentation/git.txt                  |  15 +-
 Makefile                               |   1 +
 builtin/receive-pack.c                 |  15 ++
 cache.h                                |  10 +
 connect.c                              | 353 ++++++++++++++++++++++-----------
 daemon.c                               |  68 ++++++-
 http.c                                 |  18 ++
 pkt-line.c                             |   6 +
 pkt-line.h                             |   1 +
 protocol.c                             |  79 ++++++++
 protocol.h                             |  33 +++
 t/interop/i5700-protocol-transition.sh |  68 +++++++
 t/lib-httpd/apache.conf                |   7 +
 t/t5601-clone.sh                       |   9 +-
 t/t5700-protocol-v1.sh                 | 294 +++++++++++++++++++++++++++
 upload-pack.c                          |  18 +-
 17 files changed, 900 insertions(+), 139 deletions(-)
 create mode 100644 protocol.c
 create mode 100644 protocol.h
 create mode 100755 t/interop/i5700-protocol-transition.sh
 create mode 100755 t/t5700-protocol-v1.sh

--- interdiff with 'origin/bw/protocol-v1'

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b78747abc..0460af37e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2084,12 +2084,31 @@ ssh.variant::
 	Depending on the value of the environment variables `GIT_SSH` or
 	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
 	auto-detects whether to adjust its command-line parameters for use
-	with plink or tortoiseplink, as opposed to the default (OpenSSH).
+	with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
+	(simple).
 +
 The config variable `ssh.variant` can be set to override this auto-detection;
-valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
-will be treated as normal ssh. This setting can be overridden via the
-environment variable `GIT_SSH_VARIANT`.
+valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
+other value will be treated as normal ssh. This setting can be overridden via
+the environment variable `GIT_SSH_VARIANT`.
++
+The current command-line parameters used for each variant are as
+follows:
++
+--
+
+* `ssh` - [-p port] [-4] [-6] [-o option] [username@]host command
+
+* `simple` - [username@]host command
+
+* `plink` or `putty` - [-P port] [-4] [-6] [username@]host command
+
+* `tortoiseplink` - [-P port] [-4] [-6] -batch [username@]host command
+
+--
++
+Except for the `simple` variant, command-line parameters are likely to
+change as git gains new features.
 
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 299f75c7b..8bc3f2147 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -518,11 +518,10 @@ other
 	If either of these environment variables is set then 'git fetch'
 	and 'git push' will use the specified command instead of 'ssh'
 	when they need to connect to a remote system.
-	The command will be given exactly two or four arguments: the
-	'username@host' (or just 'host') from the URL and the shell
-	command to execute on that remote system, optionally preceded by
-	`-p` (literally) and the 'port' from the URL when it specifies
-	something other than the default SSH port.
+	The command-line parameters passed to the configured command are
+	determined by the ssh variant.  See `ssh.variant` option in
+	linkgit:git-config[1] for details.
+
 +
 `$GIT_SSH_COMMAND` takes precedence over `$GIT_SSH`, and is interpreted
 by the shell, which allows additional arguments to be included.
@@ -700,7 +699,8 @@ of clones and fetches.
 `GIT_PROTOCOL`::
 	For internal use only.  Used in handshaking the wire protocol.
 	Contains a colon ':' separated list of keys with optional values
-	'key[=value]'.  Presence of unknown keys must be tolerated.
+	'key[=value]'.  Presence of unknown keys and values must be
+	ignored.
 
 Discussion[[Discussion]]
 ------------------------
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cb179367b..94b7d29ea 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1973,8 +1973,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		 * so just fall through after writing the version string.
 		 */
 	case protocol_v0:
-	default:
 		break;
+	default:
+		BUG("unknown protocol version");
 	}
 
 	if (advertise_refs || !stateless_rpc) {
diff --git a/cache.h b/cache.h
index aaadac1f0..3a6b869c2 100644
--- a/cache.h
+++ b/cache.h
@@ -448,7 +448,8 @@ static inline enum object_type object_type(unsigned int mode)
 /*
  * Environment variable used in handshaking the wire protocol.
  * Contains a colon ':' separated list of keys with optional values
- * 'key[=value]'.  Presence of unknown keys must be tolerated.
+ * 'key[=value]'.  Presence of unknown keys and values must be
+ * ignored.
  */
 #define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
 /* HTTP header used to handshake the wire protocol */
diff --git a/connect.c b/connect.c
index 12ebab724..65cee49b6 100644
--- a/connect.c
+++ b/connect.c
@@ -139,12 +139,12 @@ static int read_remote_ref(int in, char **src_buf, size_t *src_len,
 static int process_protocol_version(void)
 {
 	switch (determine_protocol_version_client(packet_buffer)) {
-		case protocol_v1:
-			return 1;
-		case protocol_v0:
-			return 0;
-		default:
-			die("server is speaking an unknown protocol");
+	case protocol_v1:
+		return 1;
+	case protocol_v0:
+		return 0;
+	default:
+		die("server is speaking an unknown protocol");
 	}
 }
 
@@ -776,37 +776,44 @@ static const char *get_ssh_command(void)
 	return NULL;
 }
 
-static int override_ssh_variant(int *port_option, int *needs_batch)
+enum ssh_variant {
+	VARIANT_SIMPLE,
+	VARIANT_SSH,
+	VARIANT_PLINK,
+	VARIANT_PUTTY,
+	VARIANT_TORTOISEPLINK,
+};
+
+static int override_ssh_variant(enum ssh_variant *ssh_variant)
 {
-	char *variant;
+	const char *variant = getenv("GIT_SSH_VARIANT");
 
-	variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
-	if (!variant &&
-	    git_config_get_string("ssh.variant", &variant))
+	if (!variant && git_config_get_string_const("ssh.variant", &variant))
 		return 0;
 
-	if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
-		*port_option = 'P';
-		*needs_batch = 0;
-	} else if (!strcmp(variant, "tortoiseplink")) {
-		*port_option = 'P';
-		*needs_batch = 1;
-	} else {
-		*port_option = 'p';
-		*needs_batch = 0;
-	}
-	free(variant);
+	if (!strcmp(variant, "plink"))
+		*ssh_variant = VARIANT_PLINK;
+	else if (!strcmp(variant, "putty"))
+		*ssh_variant = VARIANT_PUTTY;
+	else if (!strcmp(variant, "tortoiseplink"))
+		*ssh_variant = VARIANT_TORTOISEPLINK;
+	else if (!strcmp(variant, "simple"))
+		*ssh_variant = VARIANT_SIMPLE;
+	else
+		*ssh_variant = VARIANT_SSH;
+
 	return 1;
 }
 
-static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
-			       int *port_option, int *needs_batch)
+static enum ssh_variant determine_ssh_variant(const char *ssh_command,
+					      int is_cmdline)
 {
+	enum ssh_variant ssh_variant = VARIANT_SIMPLE;
 	const char *variant;
 	char *p = NULL;
 
-	if (override_ssh_variant(port_option, needs_batch))
-		return;
+	if (override_ssh_variant(&ssh_variant))
+		return ssh_variant;
 
 	if (!is_cmdline) {
 		p = xstrdup(ssh_command);
@@ -825,19 +832,21 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 			free(ssh_argv);
 		} else {
 			free(p);
-			return;
+			return ssh_variant;
 		}
 	}
 
-	if (!strcasecmp(variant, "plink") ||
-	    !strcasecmp(variant, "plink.exe"))
-		*port_option = 'P';
+	if (!strcasecmp(variant, "ssh"))
+		ssh_variant = VARIANT_SSH;
+	else if (!strcasecmp(variant, "plink") ||
+		 !strcasecmp(variant, "plink.exe"))
+		ssh_variant = VARIANT_PLINK;
 	else if (!strcasecmp(variant, "tortoiseplink") ||
-		 !strcasecmp(variant, "tortoiseplink.exe")) {
-		*port_option = 'P';
-		*needs_batch = 1;
-	}
+		 !strcasecmp(variant, "tortoiseplink.exe"))
+		ssh_variant = VARIANT_TORTOISEPLINK;
+
 	free(p);
+	return ssh_variant;
 }
 
 /*
@@ -937,8 +946,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
-			int needs_batch = 0;
-			int port_option = 'p';
+			enum ssh_variant variant;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
@@ -965,10 +973,9 @@ struct child_process *git_connect(int fd[2], const char *url,
 				die("strange hostname '%s' blocked", ssh_host);
 
 			ssh = get_ssh_command();
-			if (ssh)
-				handle_ssh_variant(ssh, 1, &port_option,
-						   &needs_batch);
-			else {
+			if (ssh) {
+				variant = determine_ssh_variant(ssh, 1);
+			} else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
@@ -979,32 +986,38 @@ struct child_process *git_connect(int fd[2], const char *url,
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
-				else
-					handle_ssh_variant(ssh, 0,
-							   &port_option,
-							   &needs_batch);
+				variant = determine_ssh_variant(ssh, 0);
 			}
 
 			argv_array_push(&conn->args, ssh);
 
-			if (get_protocol_version_config() > 0) {
+			if (variant == VARIANT_SSH &&
+			    get_protocol_version_config() > 0) {
 				argv_array_push(&conn->args, "-o");
 				argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
 				argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
 						 get_protocol_version_config());
 			}
 
-			if (flags & CONNECT_IPV4)
-				argv_array_push(&conn->args, "-4");
-			else if (flags & CONNECT_IPV6)
-				argv_array_push(&conn->args, "-6");
-			if (needs_batch)
+			if (variant != VARIANT_SIMPLE) {
+				if (flags & CONNECT_IPV4)
+					argv_array_push(&conn->args, "-4");
+				else if (flags & CONNECT_IPV6)
+					argv_array_push(&conn->args, "-6");
+			}
+
+			if (variant == VARIANT_TORTOISEPLINK)
 				argv_array_push(&conn->args, "-batch");
-			if (port) {
-				argv_array_pushf(&conn->args,
-						 "-%c", port_option);
+
+			if (port && variant != VARIANT_SIMPLE) {
+				if (variant == VARIANT_SSH)
+					argv_array_push(&conn->args, "-p");
+				else
+					argv_array_push(&conn->args, "-P");
+
 				argv_array_push(&conn->args, port);
 			}
+
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			transport_check_allowed("file");
diff --git a/protocol.c b/protocol.c
index 369503065..43012b7eb 100644
--- a/protocol.c
+++ b/protocol.c
@@ -33,6 +33,13 @@ enum protocol_version determine_protocol_version_server(void)
 	const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
 	enum protocol_version version = protocol_v0;
 
+	/*
+	 * Determine which protocol version the client has requested.  Since
+	 * multiple 'version' keys can be sent by the client, indicating that
+	 * the client is okay to speak any of them, select the greatest version
+	 * that the client has requested.  This is due to the assumption that
+	 * the most recent protocol version will be the most state-of-the-art.
+	 */
 	if (git_protocol) {
 		struct string_list list = STRING_LIST_INIT_DUP;
 		const struct string_list_item *item;
diff --git a/protocol.h b/protocol.h
index 18f9a5235..1b2bc94a8 100644
--- a/protocol.h
+++ b/protocol.h
@@ -7,8 +7,27 @@ enum protocol_version {
 	protocol_v1 = 1,
 };
 
+/*
+ * Used by a client to determine which protocol version to request be used when
+ * communicating with a server, reflecting the configured value of the
+ * 'protocol.version' config.  If unconfigured, a value of 'protocol_v0' is
+ * returned.
+ */
 extern enum protocol_version get_protocol_version_config(void);
+
+/*
+ * Used by a server to determine which protocol version should be used based on
+ * a client's request, communicated via the 'GIT_PROTOCOL' environment variable
+ * by setting appropriate values for the key 'version'.  If a client doesn't
+ * request a particular protocol version, a default of 'protocol_v0' will be
+ * used.
+ */
 extern enum protocol_version determine_protocol_version_server(void);
+
+/*
+ * Used by a client to determine which protocol version the server is speaking
+ * based on the server's initial response.
+ */
 extern enum protocol_version determine_protocol_version_client(const char *server_response);
 
 #endif /* PROTOCOL_H */
diff --git a/t/interop/i5700-protocol-transition.sh b/t/interop/i5700-protocol-transition.sh
index 9e83428a8..97e8e580e 100755
--- a/t/interop/i5700-protocol-transition.sh
+++ b/t/interop/i5700-protocol-transition.sh
@@ -3,7 +3,7 @@
 VERSION_A=.
 VERSION_B=v2.0.0
 
-: ${LIB_GIT_DAEMON_PORT:=5600}
+: ${LIB_GIT_DAEMON_PORT:=5700}
 LIB_GIT_DAEMON_COMMAND='git.b daemon'
 
 test_description='clone and fetch by client who is trying to use a new protocol'
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9c56f771b..ee1a24c5b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -312,6 +312,8 @@ setup_ssh_wrapper () {
 			"$TRASH_DIRECTORY/ssh-wrapper$X" &&
 		GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper$X" &&
 		export GIT_SSH &&
+		GIT_SSH_VARIANT=ssh &&
+		export GIT_SSH_VARIANT &&
 		export TRASH_DIRECTORY &&
 		>"$TRASH_DIRECTORY"/ssh-output
 	'
@@ -320,7 +322,8 @@ setup_ssh_wrapper () {
 copy_ssh_wrapper_as () {
 	cp "$TRASH_DIRECTORY/ssh-wrapper$X" "${1%$X}$X" &&
 	GIT_SSH="${1%$X}$X" &&
-	export GIT_SSH
+	export GIT_SSH &&
+	unset GIT_SSH_VARIANT
 }
 
 expect_ssh () {
@@ -362,10 +365,10 @@ test_expect_success 'bracketed hostnames are still ssh' '
 	expect_ssh "-p 123" myhost src
 '
 
-test_expect_success 'uplink is not treated as putty' '
+test_expect_success 'uplink is treated as simple' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
 	git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
-	expect_ssh "-p 123" myhost src
+	expect_ssh myhost src
 '
 
 test_expect_success 'plink is treated specially (as putty)' '
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 222265127..ba86a44eb 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -26,7 +26,7 @@ test_expect_success 'clone with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "version=1" log &&
+	grep "clone> .*\\\0\\\0version=1\\\0$" log &&
 	# Server responded using protocol v1
 	grep "clone< version 1" log
 '
@@ -37,12 +37,12 @@ test_expect_success 'fetch with git:// using protocol v1' '
 	GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
 		fetch 2>log &&
 
-	git -C daemon_child log -1 --format=%s FETCH_HEAD >actual &&
+	git -C daemon_child log -1 --format=%s origin/master >actual &&
 	git -C "$daemon_parent" log -1 --format=%s >expect &&
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "version=1" log &&
+	grep "fetch> .*\\\0\\\0version=1\\\0$" log &&
 	# Server responded using protocol v1
 	grep "fetch< version 1" log
 '
@@ -56,7 +56,7 @@ test_expect_success 'pull with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "version=1" log &&
+	grep "fetch> .*\\\0\\\0version=1\\\0$" log &&
 	# Server responded using protocol v1
 	grep "fetch< version 1" log
 '
@@ -64,8 +64,8 @@ test_expect_success 'pull with git:// using protocol v1' '
 test_expect_success 'push with git:// using protocol v1' '
 	test_commit -C daemon_child three &&
 
-	# Since the repository being served isnt bare we need to push to
-	# another branch explicitly to avoid mangling the master branch
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
 	GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
 		push origin HEAD:client_branch 2>log &&
 
@@ -74,7 +74,7 @@ test_expect_success 'push with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "version=1" log &&
+	grep "push> .*\\\0\\\0version=1\\\0$" log &&
 	# Server responded using protocol v1
 	grep "push< version 1" log
 '
@@ -106,7 +106,7 @@ test_expect_success 'fetch with file:// using protocol v1' '
 	GIT_TRACE_PACKET=1 git -C file_child -c protocol.version=1 \
 		fetch 2>log &&
 
-	git -C file_child log -1 --format=%s FETCH_HEAD >actual &&
+	git -C file_child log -1 --format=%s origin/master >actual &&
 	git -C file_parent log -1 --format=%s >expect &&
 	test_cmp expect actual &&
 
@@ -129,8 +129,8 @@ test_expect_success 'pull with file:// using protocol v1' '
 test_expect_success 'push with file:// using protocol v1' '
 	test_commit -C file_child three &&
 
-	# Since the repository being served isnt bare we need to push to
-	# another branch explicitly to avoid mangling the master branch
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
 	GIT_TRACE_PACKET=1 git -C file_child -c protocol.version=1 \
 		push origin HEAD:client_branch 2>log &&
 
@@ -145,8 +145,10 @@ test_expect_success 'push with file:// using protocol v1' '
 # Test protocol v1 with 'ssh://' transport
 #
 test_expect_success 'setup ssh wrapper' '
-	GIT_SSH="$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" &&
+	GIT_SSH="$GIT_BUILD_DIR/t/helper/test-fake-ssh" &&
 	export GIT_SSH &&
+	GIT_SSH_VARIANT=ssh &&
+	export GIT_SSH_VARIANT &&
 	export TRASH_DIRECTORY &&
 	>"$TRASH_DIRECTORY"/ssh-output
 '
@@ -182,7 +184,7 @@ test_expect_success 'fetch with ssh:// using protocol v1' '
 		fetch 2>log &&
 	expect_ssh git-upload-pack &&
 
-	git -C ssh_child log -1 --format=%s FETCH_HEAD >actual &&
+	git -C ssh_child log -1 --format=%s origin/master >actual &&
 	git -C ssh_parent log -1 --format=%s >expect &&
 	test_cmp expect actual &&
 
@@ -206,8 +208,8 @@ test_expect_success 'pull with ssh:// using protocol v1' '
 test_expect_success 'push with ssh:// using protocol v1' '
 	test_commit -C ssh_child three &&
 
-	# Since the repository being served isnt bare we need to push to
-	# another branch explicitly to avoid mangling the master branch
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
 	GIT_TRACE_PACKET=1 git -C ssh_child -c protocol.version=1 \
 		push origin HEAD:client_branch 2>log &&
 	expect_ssh git-receive-pack &&
@@ -251,7 +253,7 @@ test_expect_success 'fetch with http:// using protocol v1' '
 	GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
 		fetch 2>log &&
 
-	git -C http_child log -1 --format=%s FETCH_HEAD >actual &&
+	git -C http_child log -1 --format=%s origin/master >actual &&
 	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s >expect &&
 	test_cmp expect actual &&
 
@@ -274,8 +276,8 @@ test_expect_success 'pull with http:// using protocol v1' '
 test_expect_success 'push with http:// using protocol v1' '
 	test_commit -C http_child three &&
 
-	# Since the repository being served isnt bare we need to push to
-	# another branch explicitly to avoid mangling the master branch
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
 	GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
 		push origin HEAD:client_branch && #2>log &&
 
diff --git a/upload-pack.c b/upload-pack.c
index 5cab39819..ef438e9c2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1078,9 +1078,10 @@ int cmd_main(int argc, const char **argv)
 		 * so just fall through after writing the version string.
 		 */
 	case protocol_v0:
-	default:
 		upload_pack();
 		break;
+	default:
+		BUG("unknown protocol version");
 	}
 
 	return 0;


-- 
2.14.2.920.gcf0c67979c-goog