protocol transition

Brandon Williams git 4 versions Sep 13, 2017 – Oct 16, 2017 In review

Get series with b4

b4 am [email protected]

Versions newest first · linked by author and title

v4 Latest Oct 16, 201711 patches 1 replyIn review
v3 Oct 3, 201710 patches 25 repliesSuperseded
v2 Sep 26, 20179 patches 21 repliesSuperseded

Earlier versions · 1

Version	Posted	Patches	Replies	Status
v1	Sep 13, 2017	8 patches	10 replies	Superseded

Changes in v4 from the cover letter

* Added more tests for the new handeling of ssh variants.
* Removed the 'default' case in upload_pack and receive_pack and instead ensured that all enum values were accounted for. This way when a new protocol version is introduced the compiler will throw an error if the new protocol version isn't accounted for in these switch statements.
* Added Jonathan's Documentation patch ontop of the series (with the small change I pointed out in reply to the patch itself)
* A few other small changes due to reviewer comments.

Patches in v4 No applies recorded yet

#	Subject and files	Size	Replies
1/11	connect: in ref advertisement, shallows are last`connect.c`	+123 ~~-66~~	0 replies
2/11	pkt-line: add packet_write function`pkt-line.c` `pkt-line.h`	+7 -0	0 replies
3/11	protocol: introduce protocol extension mechanisms`Documentation/config.txt` `Documentation/git.txt` `Makefile` +3 more	+144 -0	1 reply
4/11	daemon: recognize hidden request arguments`daemon.c`	+62 -9	0 replies
5/11	upload-pack, receive-pack: introduce protocol version 1`builtin/receive-pack.c` `upload-pack.c`	+36 -1	0 replies
6/11	connect: teach client to recognize v1 server response`connect.c`	+26 -4	0 replies
7/11	connect: tell server that the client understands v1`connect.c` `t/t5700-protocol-v1.sh`	+255 -5	0 replies
8/11	http: tell server that the client understands v1`cache.h` `http.c` `t/lib-httpd/apache.conf` +1 more	+96 -0	0 replies
9/11	i5700: add interop test for protocol transition`t/interop/i5700-protocol-transition.sh`	+68 -0	0 replies
10/11	ssh: introduce a 'simple' ssh variant`Documentation/config.txt` `Documentation/git.txt` `connect.c` +2 more	+111 ~~-61~~	0 replies
11/11	Documentation: document Extra Parameters`Documentation/technical/http-protocol.txt` `Documentation/technical/pack-protocol.txt`	+44 -7	0 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v4 yet.

Read the v4 thread

Cover letter [PATCH v4 00/11] protocol transition

Changes in v4:
* Added more tests for the new handeling of ssh variants.
* Removed the 'default' case in upload_pack and receive_pack and instead ensured that all enum values were accounted for. This way when a new protocol version is introduced the compiler will throw an error if the new protocol version isn't accounted for in these switch statements.
* Added Jonathan's Documentation patch ontop of the series (with the small change I pointed out in reply to the patch itself)
* A few other small changes due to reviewer comments.

Brandon Williams (9):
pkt-line: add packet_write function
protocol: introduce protocol extension 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 (2):
connect: in ref advertisement, shallows are last
Documentation: document Extra Parameters

 Documentation/config.txt                  |  44 +++-
 Documentation/git.txt                     |  15 +-
 Documentation/technical/http-protocol.txt |   8 +
 Documentation/technical/pack-protocol.txt |  43 +++-
 Makefile                                  |   1 +
 builtin/receive-pack.c                    |  17 ++
 cache.h                                   |  10 +
 connect.c                                 | 354 ++++++++++++++++++++----------
 daemon.c                                  |  71 +++++-
 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                          |  26 ++-
 t/t5700-protocol-v1.sh                    | 294 +++++++++++++++++++++++++
 upload-pack.c                             |  20 +-
 19 files changed, 967 insertions(+), 148 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'

Documentation/technical/http-protocol.txt +8 -0

index 1c561bdd9..a0e45f288 100644
@@ -219,6 +219,10 @@ smart server reply:
    S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
    S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
 
+The client may send Extra Parameters (see
+Documentation/technical/pack-protocol.txt) as a colon-separated string
+in the Git-Protocol HTTP header.
+
 Dumb Server Response
 ^^^^^^^^^^^^^^^^^^^^
 Dumb servers MUST respond with the dumb server reply format.

@@ -269,7 +273,11 @@ the C locale ordering.  The stream SHOULD include the default ref
 named `HEAD` as the first ref.  The stream MUST include capability
 declarations behind a NUL on the first ref.
 
+The returned response contains "version 1" if "version=1" was sent as an
+Extra Parameter.
+
   smart_reply     =  PKT-LINE("# service=$servicename" LF)
+		     *1("version 1")
 		     ref_list
 		     "0000"
   ref_list        =  empty_list / non_empty_list

Documentation/technical/pack-protocol.txt +36 -7

84 lines not on this page · raw message

builtin/receive-pack.c +5 -3

index 94b7d29ea..839c1462d 100644
@@ -1966,15 +1966,17 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 	switch (determine_protocol_version_server()) {
 	case protocol_v1:
-		if (advertise_refs || !stateless_rpc)
-			packet_write_fmt(1, "version 1\n");
 		/*
 		 * v1 is just the original protocol with a version string,
 		 * so just fall through after writing the version string.
 		 */
+		if (advertise_refs || !stateless_rpc)
+			packet_write_fmt(1, "version 1\n");
+
+		/* fallthrough */
 	case protocol_v0:
 		break;
-	default:
+	case protocol_unknown_version:
 		BUG("unknown protocol version");
 	}

connect.c +2 -1

index 65cee49b6..7fbd396b3 100644
@@ -836,7 +836,8 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
 		}
 	}
 
-	if (!strcasecmp(variant, "ssh"))
+	if (!strcasecmp(variant, "ssh") ||
+	    !strcasecmp(variant, "ssh.exe"))
 		ssh_variant = VARIANT_SSH;
 	else if (!strcasecmp(variant, "plink") ||
 		 !strcasecmp(variant, "plink.exe"))

daemon.c +3 -0

index 36cc794c9..e37e343d0 100644
@@ -582,6 +582,9 @@ static void canonicalize_client(struct strbuf *out, const char *in)
 
 /*
  * Read the host as supplied by the client connection.
+ *
+ * Returns a pointer to the character after the NUL byte terminating the host
+ * arguemnt, or 'extra_args' if there is no host arguemnt.
  */
 static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {

pkt-line.c +1 -1

index c025d0332..7006b3587 100644
@@ -188,7 +188,7 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 	return 0;
 }
 
-void packet_write(const int fd_out, const char *buf, size_t size)
+void packet_write(int fd_out, const char *buf, size_t size)
 {
 	if (packet_write_gently(fd_out, buf, size))
 		die_errno("packet write failed");

pkt-line.h +1 -1

index d9e9783b1..3dad583e2 100644
@@ -22,7 +22,7 @@
 void packet_flush(int fd);
 void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 void packet_buf_flush(struct strbuf *buf);
-void packet_write(const int fd_out, const char *buf, size_t size);
+void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));

t/t5601-clone.sh +20 -7

index ee1a24c5b..86811a0c3 100755
@@ -309,21 +309,18 @@ test_expect_success 'clone checking out a tag' '
 setup_ssh_wrapper () {
 	test_expect_success 'setup ssh wrapper' '
 		cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
-			"$TRASH_DIRECTORY/ssh-wrapper$X" &&
-		GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper$X" &&
+			"$TRASH_DIRECTORY/ssh$X" &&
+		GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
 		export GIT_SSH &&
-		GIT_SSH_VARIANT=ssh &&
-		export GIT_SSH_VARIANT &&
 		export TRASH_DIRECTORY &&
 		>"$TRASH_DIRECTORY"/ssh-output
 	'
 }
 
 copy_ssh_wrapper_as () {
-	cp "$TRASH_DIRECTORY/ssh-wrapper$X" "${1%$X}$X" &&
+	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
 	GIT_SSH="${1%$X}$X" &&
-	export GIT_SSH &&
-	unset GIT_SSH_VARIANT
+	export GIT_SSH
 }
 
 expect_ssh () {

@@ -365,6 +362,22 @@ test_expect_success 'bracketed hostnames are still ssh' '
 	expect_ssh "-p 123" myhost src
 '
 
+test_expect_success 'OpenSSH variant passes -4' '
+	git clone -4 "[myhost:123]:src" ssh-ipv4-clone &&
+	expect_ssh "-4 -p 123" myhost src
+'
+
+test_expect_success 'variant can be overriden' '
+	git -c ssh.variant=simple clone -4 "[myhost:123]:src" ssh-simple-clone &&
+	expect_ssh myhost src
+'
+
+test_expect_success 'simple is treated as simple' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
+	git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple &&
+	expect_ssh myhost src
+'
+
 test_expect_success 'uplink is treated as simple' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
 	git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&

upload-pack.c +5 -3

index ef438e9c2..ef99a029c 100644
@@ -1071,16 +1071,18 @@ int cmd_main(int argc, const char **argv)
 
 	switch (determine_protocol_version_server()) {
 	case protocol_v1:
-		if (advertise_refs || !stateless_rpc)
-			packet_write_fmt(1, "version 1\n");
 		/*
 		 * v1 is just the original protocol with a version string,
 		 * so just fall through after writing the version string.
 		 */
+		if (advertise_refs || !stateless_rpc)
+			packet_write_fmt(1, "version 1\n");
+
+		/* fallthrough */
 	case protocol_v0:
 		upload_pack();
 		break;
-	default:
+	case protocol_unknown_version:
 		BUG("unknown protocol version");
 	}

Signature · 1 line

2.15.0.rc0.271.g36b669edcc-goog

Read the cover letter in its thread