[PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[Jonathan Nieder]

Previously: [1].

This version should be essentially identical to v2.  Changes:
- patch 1 is new and should fix the test failure on Windows
- patch 2 is new, discussed at [2]
- patch 5 split off from patch 6 as suggested at [3]
- patch 6 commit message got two new notes to address the worries
  from [3]

Thanks for the helpful reviews, and sorry to take so long to get this
out.  Thoughts of all kinds welcome, as always.

Sincerely,
Jonathan Nieder (8):
  ssh test: make copy_ssh_wrapper_as clean up after itself
  connect: move no_fork fallback to git_tcp_connect
  connect: split git:// setup into a separate function
  connect: split ssh command line options into separate function
  connect: split ssh option computation to its own function
  ssh: 'auto' variant to select between 'ssh' and 'simple'
  ssh: 'simple' variant does not support -4/-6
  ssh: 'simple' variant does not support --port

 Documentation/config.txt |  24 ++--
 connect.c                | 322 +++++++++++++++++++++++++++++------------------
 t/t5601-clone.sh         |  69 ++++++----
 t/t5603-clone-dirname.sh |   2 +
 4 files changed, 265 insertions(+), 152 deletions(-)

[1] https://public-inbox.org/git/20171023231625.6mhcyqti7vdg6yot@aiede.mtv.corp.google.com/
[2] https://public-inbox.org/git/20171115202516.hduhzsgeoff5a22b@aiede.mtv.corp.google.com/
[3] https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/

[PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Jonathan Nieder]

Simplify by not allowing the copied ssh wrapper to persist between
tests.  This way, tests can be safely reordered, added, and removed
with less fear of hidden side effects.

This also avoids having to call setup_ssh_wrapper to restore the value
of GIT_SSH after this battery of tests, since it means each test will
restore it individually.

Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash
will overwrite that when redirecting via `>uplink`.  A proposed test
wrote a script to 'uplink' after a previous test created uplink.exe
using copy_ssh_wrapper_as, so the script written with '>uplink' had
the wrong filename and failed.

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks to Dscho for tracking this subtle issue down.

 t/t5601-clone.sh | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 86811a0c35..9d007c0f8d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
-setup_ssh_wrapper () {
-	test_expect_success 'setup ssh wrapper' '
-		cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
-			"$TRASH_DIRECTORY/ssh$X" &&
-		GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
-		export GIT_SSH &&
-		export TRASH_DIRECTORY &&
-		>"$TRASH_DIRECTORY"/ssh-output
-	'
-}
+test_expect_success 'set up ssh wrapper' '
+	cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
+		"$TRASH_DIRECTORY/ssh$X" &&
+	GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
+	export GIT_SSH &&
+	export TRASH_DIRECTORY &&
+	>"$TRASH_DIRECTORY"/ssh-output
+'
 
 copy_ssh_wrapper_as () {
 	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
+	test_when_finished "rm -f ${1%$X}$X" &&
 	GIT_SSH="${1%$X}$X" &&
-	export GIT_SSH
+	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
 }
 
 expect_ssh () {
@@ -344,8 +343,6 @@ expect_ssh () {
 	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-setup_ssh_wrapper
-
 test_expect_success 'clone myhost:src uses ssh' '
 	git clone myhost:src ssh-clone &&
 	expect_ssh myhost src
@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 	GIT_SSH_VARIANT=plink \
 	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
 	expect_ssh "-P 123" myhost src
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 	GIT_SSH_VARIANT=tortoiseplink \
 	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
 	expect_ssh "-batch -P 123" myhost src
@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '
 		git clone "[myhost:123]:src" sq-failure
 '
 
-# Reset the GIT_SSH environment variable for clone tests.
-setup_ssh_wrapper
-
 counter=0
 # $1 url
 # $2 none|host
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> Simplify by not allowing the copied ssh wrapper to persist between
> tests.  This way, tests can be safely reordered, added, and removed
> with less fear of hidden side effects.
> 
> This also avoids having to call setup_ssh_wrapper to restore the value
> of GIT_SSH after this battery of tests, since it means each test will
> restore it individually.
> 
> Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash
> will overwrite that when redirecting via `>uplink`.  A proposed test
> wrote a script to 'uplink' after a previous test created uplink.exe
> using copy_ssh_wrapper_as, so the script written with '>uplink' had
> the wrong filename and failed.
> 
> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thanks to Dscho for tracking this subtle issue down.
> 
>  t/t5601-clone.sh | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 86811a0c35..9d007c0f8d 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
>  	test_cmp fetch.expected fetch.actual
>  '
>  
> -setup_ssh_wrapper () {
> -	test_expect_success 'setup ssh wrapper' '
> -		cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
> -			"$TRASH_DIRECTORY/ssh$X" &&
> -		GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
> -		export GIT_SSH &&
> -		export TRASH_DIRECTORY &&
> -		>"$TRASH_DIRECTORY"/ssh-output
> -	'
> -}
> +test_expect_success 'set up ssh wrapper' '
> +	cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
> +		"$TRASH_DIRECTORY/ssh$X" &&
> +	GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
> +	export GIT_SSH &&
> +	export TRASH_DIRECTORY &&
> +	>"$TRASH_DIRECTORY"/ssh-output
> +'
>  
>  copy_ssh_wrapper_as () {
>  	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
> +	test_when_finished "rm -f ${1%$X}$X" &&
>  	GIT_SSH="${1%$X}$X" &&
> -	export GIT_SSH
> +	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""

All the escaping!

Patch looks good.

>  }
>  
>  expect_ssh () {
> @@ -344,8 +343,6 @@ expect_ssh () {
>  	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
>  }
>  
> -setup_ssh_wrapper
> -
>  test_expect_success 'clone myhost:src uses ssh' '
>  	git clone myhost:src ssh-clone &&
>  	expect_ssh myhost src
> @@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '
>  '
>  
>  test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	GIT_SSH_VARIANT=plink \
>  	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
>  	expect_ssh "-P 123" myhost src
>  '
>  
>  test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	GIT_SSH_VARIANT=tortoiseplink \
>  	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
>  	expect_ssh "-batch -P 123" myhost src
> @@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '
>  		git clone "[myhost:123]:src" sq-failure
>  '
>  
> -# Reset the GIT_SSH environment variable for clone tests.
> -setup_ssh_wrapper
> -
>  counter=0
>  # $1 url
>  # $2 none|host
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams

Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +test_expect_success 'set up ssh wrapper' '
> +	cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
> +		"$TRASH_DIRECTORY/ssh$X" &&
> +	GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
> +	export GIT_SSH &&
> +	export TRASH_DIRECTORY &&
> +	>"$TRASH_DIRECTORY"/ssh-output
> +'
>  
>  copy_ssh_wrapper_as () {
>  	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
> +	test_when_finished "rm -f ${1%$X}$X" &&
>  	GIT_SSH="${1%$X}$X" &&

As we can clearly see in the context, this is not a new issue, but I
find the users of this helper that benefit from the "${1%$X}$X"
magic somewhat iffy.

There are callers of this helper that pass "somedir/plink" and
"somedir/plink.exe", but behind these callers that _think_ they are
testing the variant with and without the trailing ".exe", the helper
always add ".exe" (after stripping an existing one) on $X=.exe
platforms, ending up in testing the same thing twice.  On platforms
with $X='', testing two different command names may have "some"
value, but I wonder if it is cleaner to use a much less magical
"$1$X" here, and skip the test with a caller that gives ".exe"
variant using a test prerequisite on $X=.exe platforms to avoid
redundant tests?

This is totally outside the scope of this series; I mention this
only because this may be a possible #leftoverbits.

Thanks.

[PATCH 1/8 v2] ssh test: make copy_ssh_wrapper_as clean up after itself

[Jonathan Nieder]

Simplify by not allowing the copied ssh wrapper to persist between
tests.  This way, tests can be safely reordered, added, and removed
with less fear of hidden side effects.

This also avoids having to call setup_ssh_wrapper to restore the value
of GIT_SSH after this battery of tests, since it means each test will
restore it individually.

Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash
will overwrite that when redirecting via `>uplink`.  A proposed test
wrote a script to 'uplink' after a previous test created uplink.exe
using copy_ssh_wrapper_as, so the script written with '>uplink' had
the wrong filename and failed.

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Brandon Williams <bmwill@google.com>
---
Jonathan Nieder wrote:

> Thanks to Dscho for tracking this subtle issue down.
>
>  t/t5601-clone.sh | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
[...]
> +++ b/t/t5601-clone.sh
> @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
[...]
>  copy_ssh_wrapper_as () {
>  	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
> +	test_when_finished "rm -f ${1%$X}$X" &&

I noticed while looking this over again that it is not exactly right.
$1 is likely to include spaces, causing the "rm" command not to delete
anything.  Removing the "-f" confirms the bug.

A small tweak fixes it:

 diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
 index 9d007c0f8d..4a16a0b7dd 100755
 --- i/t/t5601-clone.sh
 +++ w/t/t5601-clone.sh
 @@ -317,7 +317,7 @@ test_expect_success 'set up ssh wrapper' '
  
  copy_ssh_wrapper_as () {
  	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
 -	test_when_finished "rm -f ${1%$X}$X" &&
 +	test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" &&
  	GIT_SSH="${1%$X}$X" &&
  	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
  }

 t/t5601-clone.sh | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 86811a0c35..4a16a0b7dd 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
-setup_ssh_wrapper () {
-	test_expect_success 'setup ssh wrapper' '
-		cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
-			"$TRASH_DIRECTORY/ssh$X" &&
-		GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
-		export GIT_SSH &&
-		export TRASH_DIRECTORY &&
-		>"$TRASH_DIRECTORY"/ssh-output
-	'
-}
+test_expect_success 'set up ssh wrapper' '
+	cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
+		"$TRASH_DIRECTORY/ssh$X" &&
+	GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
+	export GIT_SSH &&
+	export TRASH_DIRECTORY &&
+	>"$TRASH_DIRECTORY"/ssh-output
+'
 
 copy_ssh_wrapper_as () {
 	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
+	test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" &&
 	GIT_SSH="${1%$X}$X" &&
-	export GIT_SSH
+	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""
 }
 
 expect_ssh () {
@@ -344,8 +343,6 @@ expect_ssh () {
 	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-setup_ssh_wrapper
-
 test_expect_success 'clone myhost:src uses ssh' '
 	git clone myhost:src ssh-clone &&
 	expect_ssh myhost src
@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 	GIT_SSH_VARIANT=plink \
 	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
 	expect_ssh "-P 123" myhost src
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 	GIT_SSH_VARIANT=tortoiseplink \
 	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
 	expect_ssh "-batch -P 123" myhost src
@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '
 		git clone "[myhost:123]:src" sq-failure
 '
 
-# Reset the GIT_SSH environment variable for clone tests.
-setup_ssh_wrapper
-
 counter=0
 # $1 url
 # $2 none|host
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 1/8 v2] ssh test: make copy_ssh_wrapper_as clean up after itself

[Stefan Beller]

On Mon, Nov 20, 2017 at 5:49 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>  @@ -317,7 +317,7 @@ test_expect_success 'set up ssh wrapper' '
>
>   copy_ssh_wrapper_as () {
>         cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
>  -      test_when_finished "rm -f ${1%$X}$X" &&
>  +      test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" &&

I wondered why the line above doesn't need the same treatment, but there
the argument is quoted, in this line we cannot use quotation as we are already
using it for bundle up the argument for test_when_finished.

The patch looks good.
Stefan

[PATCH 2/8] connect: move no_fork fallback to git_tcp_connect

[Jonathan Nieder]

git_connect has the structure

	struct child_process *conn = &no_fork;

	...
	switch (protocol) {
	case PROTO_GIT:
		if (git_use_proxy(hostandport))
			conn = git_proxy_connect(fd, hostandport);
		else
			git_tcp_connect(fd, hostandport, flags);
		...
		break;
	case PROTO_SSH:
		conn = xmalloc(sizeof(*conn));
		child_process_init(conn);
		argv_array_push(&conn->args, ssh);
		...
		break;
	...
	return conn;

In all cases except the git_tcp_connect case, conn is explicitly
assigned a value. Make the code clearer by explicitly assigning
'conn = &no_fork' in the tcp case and eliminating the default so the
compiler can ensure conn is always correctly assigned.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 connect.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/connect.c b/connect.c
index 7fbd396b35..aa994d1518 100644
--- a/connect.c
+++ b/connect.c
@@ -582,12 +582,25 @@ static int git_tcp_connect_sock(char *host, int flags)
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host, int flags)
+/*
+ * Dummy child_process returned by git_connect() if the transport protocol
+ * does not need fork(2).
+ */
+static struct child_process no_fork = CHILD_PROCESS_INIT;
+
+int git_connection_is_socket(struct child_process *conn)
+{
+	return conn == &no_fork;
+}
+
+static struct child_process *git_tcp_connect(int fd[2], char *host, int flags)
 {
 	int sockfd = git_tcp_connect_sock(host, flags);
 
 	fd[0] = sockfd;
 	fd[1] = dup(sockfd);
+
+	return &no_fork;
 }
 
 
@@ -761,8 +774,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	return protocol;
 }
 
-static struct child_process no_fork = CHILD_PROCESS_INIT;
-
 static const char *get_ssh_command(void)
 {
 	const char *ssh;
@@ -851,11 +862,11 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
 }
 
 /*
- * This returns a dummy child_process if the transport protocol does not
- * need fork(2), or a struct child_process object if it does.  Once done,
- * finish the connection with finish_connect() with the value returned from
- * this function (it is safe to call finish_connect() with NULL to support
- * the former case).
+ * This returns the dummy child_process `no_fork` if the transport protocol
+ * does not need fork(2), or a struct child_process object if it does.  Once
+ * done, finish the connection with finish_connect() with the value returned
+ * from this function (it is safe to call finish_connect() with NULL to
+ * support the former case).
  *
  * If it returns, the connect is successful; it just dies on errors (this
  * will hopefully be changed in a libification effort, to return NULL when
@@ -865,7 +876,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
-	struct child_process *conn = &no_fork;
+	struct child_process *conn;
 	enum protocol protocol;
 
 	/* Without this we cannot rely on waitpid() to tell
@@ -901,7 +912,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		if (git_use_proxy(hostandport))
 			conn = git_proxy_connect(fd, hostandport);
 		else
-			git_tcp_connect(fd, hostandport, flags);
+			conn = git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
@@ -1041,11 +1052,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 	return conn;
 }
 
-int git_connection_is_socket(struct child_process *conn)
-{
-	return conn == &no_fork;
-}
-
 int finish_connect(struct child_process *conn)
 {
 	int code;
-- 
2.15.0.448.gf294e3d99a

[PATCH 3/8] connect: split git:// setup into a separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the
PROTO_GIT-specific portion to a separate function to make it easier to
read.

No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
As before.

 connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 59 insertions(+), 44 deletions(-)

diff --git a/connect.c b/connect.c
index aa994d1518..9425229206 100644
--- a/connect.c
+++ b/connect.c
@@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
 	return ssh_variant;
 }
 
+/*
+ * Open a connection using Git's native protocol.
+ *
+ * The caller is responsible for freeing hostandport, but this function may
+ * modify it (for example, to truncate it to remove the port part).
+ */
+static struct child_process *git_connect_git(int fd[2], char *hostandport,
+					     const char *path, const char *prog,
+					     int flags)
+{
+	struct child_process *conn;
+	struct strbuf request = STRBUF_INIT;
+	/*
+	 * Set up virtual host information based on where we will
+	 * connect, unless the user has overridden us in
+	 * the environment.
+	 */
+	char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+	if (target_host)
+		target_host = xstrdup(target_host);
+	else
+		target_host = xstrdup(hostandport);
+
+	transport_check_allowed("git");
+
+	/* These underlying connection commands die() if they
+	 * cannot connect.
+	 */
+	if (git_use_proxy(hostandport))
+		conn = git_proxy_connect(fd, hostandport);
+	else
+		conn = git_tcp_connect(fd, hostandport, flags);
+	/*
+	 * Separate original protocol components prog and path
+	 * from extended host header with a NUL byte.
+	 *
+	 * Note: Do not add any other headers here!  Doing so
+	 * will cause older git-daemon servers to crash.
+	 */
+	strbuf_addf(&request,
+		    "%s %s%chost=%s%c",
+		    prog, path, 0,
+		    target_host, 0);
+
+	/* If using a new version put that stuff here after a second null byte */
+	if (get_protocol_version_config() > 0) {
+		strbuf_addch(&request, '\0');
+		strbuf_addf(&request, "version=%d%c",
+			    get_protocol_version_config(), '\0');
+	}
+
+	packet_write(fd[1], request.buf, request.len);
+
+	free(target_host);
+	strbuf_release(&request);
+	return conn;
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
-		struct strbuf request = STRBUF_INIT;
-		/*
-		 * Set up virtual host information based on where we will
-		 * connect, unless the user has overridden us in
-		 * the environment.
-		 */
-		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
-		if (target_host)
-			target_host = xstrdup(target_host);
-		else
-			target_host = xstrdup(hostandport);
-
-		transport_check_allowed("git");
-
-		/* These underlying connection commands die() if they
-		 * cannot connect.
-		 */
-		if (git_use_proxy(hostandport))
-			conn = git_proxy_connect(fd, hostandport);
-		else
-			conn = git_tcp_connect(fd, hostandport, flags);
-		/*
-		 * Separate original protocol components prog and path
-		 * from extended host header with a NUL byte.
-		 *
-		 * Note: Do not add any other headers here!  Doing so
-		 * will cause older git-daemon servers to crash.
-		 */
-		strbuf_addf(&request,
-			    "%s %s%chost=%s%c",
-			    prog, path, 0,
-			    target_host, 0);
-
-		/* If using a new version put that stuff here after a second null byte */
-		if (get_protocol_version_config() > 0) {
-			strbuf_addch(&request, '\0');
-			strbuf_addf(&request, "version=%d%c",
-				    get_protocol_version_config(), '\0');
-		}
-
-		packet_write(fd[1], request.buf, request.len);
-
-		free(target_host);
-		strbuf_release(&request);
+		conn = git_connect_git(fd, hostandport, path, prog, flags);
 	} else {
 		struct strbuf cmd = STRBUF_INIT;
 		const char *const *var;
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 3/8] connect: split git:// setup into a separate function

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> The git_connect function is growing long.  Split the
> PROTO_GIT-specific portion to a separate function to make it easier to
> read.
> 
> No functional change intended.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
> As before.
> 
>  connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 59 insertions(+), 44 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index aa994d1518..9425229206 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
>  	return ssh_variant;
>  }
>  
> +/*
> + * Open a connection using Git's native protocol.
> + *
> + * The caller is responsible for freeing hostandport, but this function may
> + * modify it (for example, to truncate it to remove the port part).
> + */
> +static struct child_process *git_connect_git(int fd[2], char *hostandport,
> +					     const char *path, const char *prog,
> +					     int flags)
> +{
> +	struct child_process *conn;
> +	struct strbuf request = STRBUF_INIT;
> +	/*
> +	 * Set up virtual host information based on where we will
> +	 * connect, unless the user has overridden us in
> +	 * the environment.
> +	 */
> +	char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> +	if (target_host)
> +		target_host = xstrdup(target_host);
> +	else
> +		target_host = xstrdup(hostandport);
> +
> +	transport_check_allowed("git");
> +
> +	/* These underlying connection commands die() if they
> +	 * cannot connect.
> +	 */

I know this is really just code motion but maybe we can fix the style of
the comment here?

> +	if (git_use_proxy(hostandport))
> +		conn = git_proxy_connect(fd, hostandport);
> +	else
> +		conn = git_tcp_connect(fd, hostandport, flags);
> +	/*
> +	 * Separate original protocol components prog and path
> +	 * from extended host header with a NUL byte.
> +	 *
> +	 * Note: Do not add any other headers here!  Doing so
> +	 * will cause older git-daemon servers to crash.
> +	 */
> +	strbuf_addf(&request,
> +		    "%s %s%chost=%s%c",
> +		    prog, path, 0,
> +		    target_host, 0);
> +
> +	/* If using a new version put that stuff here after a second null byte */
> +	if (get_protocol_version_config() > 0) {
> +		strbuf_addch(&request, '\0');
> +		strbuf_addf(&request, "version=%d%c",
> +			    get_protocol_version_config(), '\0');
> +	}
> +
> +	packet_write(fd[1], request.buf, request.len);
> +
> +	free(target_host);
> +	strbuf_release(&request);
> +	return conn;
> +}
> +
>  /*
>   * This returns the dummy child_process `no_fork` if the transport protocol
>   * does not need fork(2), or a struct child_process object if it does.  Once
> @@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		printf("Diag: path=%s\n", path ? path : "NULL");
>  		conn = NULL;
>  	} else if (protocol == PROTO_GIT) {
> -		struct strbuf request = STRBUF_INIT;
> -		/*
> -		 * Set up virtual host information based on where we will
> -		 * connect, unless the user has overridden us in
> -		 * the environment.
> -		 */
> -		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> -		if (target_host)
> -			target_host = xstrdup(target_host);
> -		else
> -			target_host = xstrdup(hostandport);
> -
> -		transport_check_allowed("git");
> -
> -		/* These underlying connection commands die() if they
> -		 * cannot connect.
> -		 */
> -		if (git_use_proxy(hostandport))
> -			conn = git_proxy_connect(fd, hostandport);
> -		else
> -			conn = git_tcp_connect(fd, hostandport, flags);
> -		/*
> -		 * Separate original protocol components prog and path
> -		 * from extended host header with a NUL byte.
> -		 *
> -		 * Note: Do not add any other headers here!  Doing so
> -		 * will cause older git-daemon servers to crash.
> -		 */
> -		strbuf_addf(&request,
> -			    "%s %s%chost=%s%c",
> -			    prog, path, 0,
> -			    target_host, 0);
> -
> -		/* If using a new version put that stuff here after a second null byte */
> -		if (get_protocol_version_config() > 0) {
> -			strbuf_addch(&request, '\0');
> -			strbuf_addf(&request, "version=%d%c",
> -				    get_protocol_version_config(), '\0');
> -		}
> -
> -		packet_write(fd[1], request.buf, request.len);
> -
> -		free(target_host);
> -		strbuf_release(&request);
> +		conn = git_connect_git(fd, hostandport, path, prog, flags);
>  	} else {
>  		struct strbuf cmd = STRBUF_INIT;
>  		const char *const *var;
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams

Re: [PATCH 3/8] connect: split git:// setup into a separate function

[Jonathan Nieder]

Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:

>> +	/* These underlying connection commands die() if they
>> +	 * cannot connect.
>> +	 */
>
> I know this is really just code motion but maybe we can fix the style of
> the comment here?

How about doing that as a separate commit?

-- >8 --
Subject: connect: correct style of C-style comment

Documentation/CodingGuidelines explains:

 - Multi-line comments include their delimiters on separate lines from
   the text.  E.g.

	/*
	 * A very long
	 * multi-line comment.
	 */

Reported-by: Brandon Williams <bmwill@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git i/connect.c w/connect.c
index 20ed1d9574..e544a5e1dd 100644
--- i/connect.c
+++ w/connect.c
@@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 
 	transport_check_allowed("git");
 
-	/* These underlying connection commands die() if they
+	/*
+	 * These underlying connection commands die() if they
 	 * cannot connect.
 	 */
 	if (git_use_proxy(hostandport))

Re: [PATCH 3/8] connect: split git:// setup into a separate function

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> Brandon Williams wrote:
> > On 11/20, Jonathan Nieder wrote:
> 
> >> +	/* These underlying connection commands die() if they
> >> +	 * cannot connect.
> >> +	 */
> >
> > I know this is really just code motion but maybe we can fix the style of
> > the comment here?
> 
> How about doing that as a separate commit?

Looks good!

> 
> -- >8 --
> Subject: connect: correct style of C-style comment
> 
> Documentation/CodingGuidelines explains:
> 
>  - Multi-line comments include their delimiters on separate lines from
>    the text.  E.g.
> 
> 	/*
> 	 * A very long
> 	 * multi-line comment.
> 	 */
> 
> Reported-by: Brandon Williams <bmwill@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  connect.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git i/connect.c w/connect.c
> index 20ed1d9574..e544a5e1dd 100644
> --- i/connect.c
> +++ w/connect.c
> @@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
>  
>  	transport_check_allowed("git");
>  
> -	/* These underlying connection commands die() if they
> +	/*
> +	 * These underlying connection commands die() if they
>  	 * cannot connect.
>  	 */
>  	if (git_use_proxy(hostandport))

-- 
Brandon Williams

[PATCH 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the portion that
discovers an ssh command and options it accepts before the service
name and path to a separate function to make it easier to read.

No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
As before.

 connect.c | 116 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/connect.c b/connect.c
index 9425229206..a9dc493db2 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 	return conn;
 }
 
+/* Prepare a child_process for use by Git's SSH-tunneled transport. */
+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
+			  const char *port, int flags)
+{
+	const char *ssh;
+	enum ssh_variant variant;
+
+	if (looks_like_command_line_option(ssh_host))
+		die("strange hostname '%s' blocked", ssh_host);
+
+	ssh = get_ssh_command();
+	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
+		 * historical compatibility).
+		 */
+		conn->use_shell = 0;
+
+		ssh = getenv("GIT_SSH");
+		if (!ssh)
+			ssh = "ssh";
+		variant = determine_ssh_variant(ssh, 0);
+	}
+
+	argv_array_push(&conn->args, ssh);
+
+	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 (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 && 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);
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh;
-			enum ssh_variant variant;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
+
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
-
 			if (!port)
 				port = get_port(ssh_host);
-
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
@@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 				strbuf_release(&cmd);
 				return NULL;
 			}
-
-			if (looks_like_command_line_option(ssh_host))
-				die("strange hostname '%s' blocked", ssh_host);
-
-			ssh = get_ssh_command();
-			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
-				 * historical compatibility).
-				 */
-				conn->use_shell = 0;
-
-				ssh = getenv("GIT_SSH");
-				if (!ssh)
-					ssh = "ssh";
-				variant = determine_ssh_variant(ssh, 0);
-			}
-
-			argv_array_push(&conn->args, ssh);
-
-			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 (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 && 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);
+			fill_ssh_args(conn, ssh_host, port, flags);
 		} else {
 			transport_check_allowed("file");
 			if (get_protocol_version_config() > 0) {
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 4/8] connect: split ssh command line options into separate function

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> The git_connect function is growing long.  Split the portion that
> discovers an ssh command and options it accepts before the service
> name and path to a separate function to make it easier to read.
> 
> No functional change intended.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
> As before.
> 
>  connect.c | 116 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 9425229206..a9dc493db2 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
>  	return conn;
>  }
>  
> +/* Prepare a child_process for use by Git's SSH-tunneled transport. */
> +static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
> +			  const char *port, int flags)
> +{
> +	const char *ssh;
> +	enum ssh_variant variant;
> +
> +	if (looks_like_command_line_option(ssh_host))
> +		die("strange hostname '%s' blocked", ssh_host);
> +
> +	ssh = get_ssh_command();
> +	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
> +		 * historical compatibility).
> +		 */
> +		conn->use_shell = 0;
> +
> +		ssh = getenv("GIT_SSH");
> +		if (!ssh)
> +			ssh = "ssh";
> +		variant = determine_ssh_variant(ssh, 0);
> +	}
> +
> +	argv_array_push(&conn->args, ssh);
> +
> +	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 (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 && 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);
> +}
> +
>  /*
>   * This returns the dummy child_process `no_fork` if the transport protocol
>   * does not need fork(2), or a struct child_process object if it does.  Once
> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		conn->use_shell = 1;
>  		conn->in = conn->out = -1;
>  		if (protocol == PROTO_SSH) {
> -			const char *ssh;
> -			enum ssh_variant variant;
>  			char *ssh_host = hostandport;
>  			const char *port = NULL;
> +
>  			transport_check_allowed("ssh");
>  			get_host_and_port(&ssh_host, &port);
> -
>  			if (!port)
>  				port = get_port(ssh_host);
> -

Are these random additions and deletions intentional?

>  			if (flags & CONNECT_DIAG_URL) {
>  				printf("Diag: url=%s\n", url ? url : "NULL");
>  				printf("Diag: protocol=%s\n", prot_name(protocol));
> @@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  				strbuf_release(&cmd);
>  				return NULL;
>  			}
> -
> -			if (looks_like_command_line_option(ssh_host))
> -				die("strange hostname '%s' blocked", ssh_host);
> -
> -			ssh = get_ssh_command();
> -			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
> -				 * historical compatibility).
> -				 */
> -				conn->use_shell = 0;
> -
> -				ssh = getenv("GIT_SSH");
> -				if (!ssh)
> -					ssh = "ssh";
> -				variant = determine_ssh_variant(ssh, 0);
> -			}
> -
> -			argv_array_push(&conn->args, ssh);
> -
> -			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 (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 && 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);
> +			fill_ssh_args(conn, ssh_host, port, flags);
>  		} else {
>  			transport_check_allowed("file");
>  			if (get_protocol_version_config() > 0) {
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams

Re: [PATCH 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

Hi,

Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:
[long stream of quoted context snipped; please cut down the quoted
 text to what you are replying to in the future]
>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
>>  		conn->use_shell = 1;
>>  		conn->in = conn->out = -1;
>>  		if (protocol == PROTO_SSH) {
>> -			const char *ssh;
>> -			enum ssh_variant variant;
>>  			char *ssh_host = hostandport;
>>  			const char *port = NULL;
>> +
>>  			transport_check_allowed("ssh");
>>  			get_host_and_port(&ssh_host, &port);
>> -
>>  			if (!port)
>>  				port = get_port(ssh_host);
>> -
>
> Are these random additions and deletions intentional?

Sorry about that.  It was to make the code easier to read, but I can
see how it's jarring during review.  I can resend without the removed
blank lines if you like.

For context, here's the code in question after the current patch:

	if (protocol == PROTO_SSH) {
		char *ssh_host = hostandport;
		const char *port = NULL;

		transport_check_allowed("ssh");
		get_host_and_port(&ssh_host, &port);
		if (!port)
			port = get_port(ssh_host);
		if (flags & CONNECT_DIAG_URL) {
			printf("Diag: url=%s\n", url ? url : "NULL");
			printf("Diag: protocol=%s\n", prot_name(protocol));
			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
			printf("Diag: port=%s\n", port ? port : "NONE");
			printf("Diag: path=%s\n", path ? path : "NULL");

			free(hostandport);
			free(path);
			free(conn);
			strbuf_release(&cmd);
			return NULL;
		}
		fill_ssh_args(conn, ssh_host, port, flags);
	} else {
		transport_check_allowed("file");
		if (get_protocol_version_config() > 0) {
			argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
					 get_protocol_version_config());
		}
	}

Jonathan

Re: [PATCH 4/8] connect: split ssh command line options into separate function

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> > On 11/20, Jonathan Nieder wrote:
> [long stream of quoted context snipped; please cut down the quoted
>  text to what you are replying to in the future]
> >> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
> >>  		conn->use_shell = 1;
> >>  		conn->in = conn->out = -1;
> >>  		if (protocol == PROTO_SSH) {
> >> -			const char *ssh;
> >> -			enum ssh_variant variant;
> >>  			char *ssh_host = hostandport;
> >>  			const char *port = NULL;
> >> +
> >>  			transport_check_allowed("ssh");
> >>  			get_host_and_port(&ssh_host, &port);
> >> -
> >>  			if (!port)
> >>  				port = get_port(ssh_host);
> >> -
> >
> > Are these random additions and deletions intentional?
> 
> Sorry about that.  It was to make the code easier to read, but I can
> see how it's jarring during review.  I can resend without the removed
> blank lines if you like.

If its for better code readability I'm all for the additions and
deletions.  I just wanted to make sure they were intentional :)

So I'm fine with either the original or the modified patch.

> 
> For context, here's the code in question after the current patch:
> 
> 	if (protocol == PROTO_SSH) {
> 		char *ssh_host = hostandport;
> 		const char *port = NULL;
> 
> 		transport_check_allowed("ssh");
> 		get_host_and_port(&ssh_host, &port);
> 		if (!port)
> 			port = get_port(ssh_host);
> 		if (flags & CONNECT_DIAG_URL) {
> 			printf("Diag: url=%s\n", url ? url : "NULL");
> 			printf("Diag: protocol=%s\n", prot_name(protocol));
> 			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
> 			printf("Diag: port=%s\n", port ? port : "NONE");
> 			printf("Diag: path=%s\n", path ? path : "NULL");
> 
> 			free(hostandport);
> 			free(path);
> 			free(conn);
> 			strbuf_release(&cmd);
> 			return NULL;
> 		}
> 		fill_ssh_args(conn, ssh_host, port, flags);
> 	} else {
> 		transport_check_allowed("file");
> 		if (get_protocol_version_config() > 0) {
> 			argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> 					 get_protocol_version_config());
> 		}
> 	}
> 
> Jonathan

-- 
Brandon Williams

[PATCH v4 4/8] connect: split ssh command line options into separate function

[Jonathan Nieder]

The git_connect function is growing long.  Split the portion that
discovers an ssh command and options it accepts before the service
name and path to a separate function to make it easier to read.

No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
Brandon Williams wrote:
> On 11/20, Jonathan Nieder wrote:

>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
>>  		conn->use_shell = 1;
>>  		conn->in = conn->out = -1;
>>  		if (protocol == PROTO_SSH) {
>> -			const char *ssh;
>> -			enum ssh_variant variant;
>>  			char *ssh_host = hostandport;
>>  			const char *port = NULL;
>> +
>>  			transport_check_allowed("ssh");
>>  			get_host_and_port(&ssh_host, &port);
>> -
>>  			if (!port)
>>  				port = get_port(ssh_host);
>> -
>
> Are these random additions and deletions intentional?

Thanks again for noticing this.  After looking more closely, I don't
see any reason for these whitespace changes.  Here's a corrected
patch.

 connect.c | 113 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 60 insertions(+), 53 deletions(-)

diff --git a/connect.c b/connect.c
index 9425229206..2113feb4f8 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 	return conn;
 }
 
+/* Prepare a child_process for use by Git's SSH-tunneled transport. */
+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
+			  const char *port, int flags)
+{
+	const char *ssh;
+	enum ssh_variant variant;
+
+	if (looks_like_command_line_option(ssh_host))
+		die("strange hostname '%s' blocked", ssh_host);
+
+	ssh = get_ssh_command();
+	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
+		 * historical compatibility).
+		 */
+		conn->use_shell = 0;
+
+		ssh = getenv("GIT_SSH");
+		if (!ssh)
+			ssh = "ssh";
+		variant = determine_ssh_variant(ssh, 0);
+	}
+
+	argv_array_push(&conn->args, ssh);
+
+	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 (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 && 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);
+}
+
 /*
  * This returns the dummy child_process `no_fork` if the transport protocol
  * does not need fork(2), or a struct child_process object if it does.  Once
@@ -972,8 +1031,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh;
-			enum ssh_variant variant;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
@@ -995,57 +1052,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 				strbuf_release(&cmd);
 				return NULL;
 			}
-
-			if (looks_like_command_line_option(ssh_host))
-				die("strange hostname '%s' blocked", ssh_host);
-
-			ssh = get_ssh_command();
-			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
-				 * historical compatibility).
-				 */
-				conn->use_shell = 0;
-
-				ssh = getenv("GIT_SSH");
-				if (!ssh)
-					ssh = "ssh";
-				variant = determine_ssh_variant(ssh, 0);
-			}
-
-			argv_array_push(&conn->args, ssh);
-
-			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 (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 && 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);
+			fill_ssh_args(conn, ssh_host, port, flags);
 		} else {
 			transport_check_allowed("file");
 			if (get_protocol_version_config() > 0) {
-- 
2.15.0.448.gf294e3d99a

[PATCH 5/8] connect: split ssh option computation to its own function

[Jonathan Nieder]

This puts the determination of options to pass to each ssh variant
(see ssh.variant in git-config(1)) in one place.

A follow-up patch will use this in an initial dry run to detect which
variant to use when the ssh command is ambiguous.

No functional change intended yet.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Split out to make patch 6 easier to read, as suggested at
https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/.

Added a function comment to make the purpose and API of this internal
helper clearer.

 connect.c | 65 ++++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/connect.c b/connect.c
index a9dc493db2..d2fbb15cc5 100644
--- a/connect.c
+++ b/connect.c
@@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 	return conn;
 }
 
+/*
+ * Append the appropriate environment variables to `env` and options to
+ * `args` for running ssh in Git's SSH-tunneled transport.
+ */
+static void push_ssh_options(struct argv_array *args, struct argv_array *env,
+			     enum ssh_variant variant, const char *port,
+			     int flags)
+{
+	if (variant == VARIANT_SSH &&
+	    get_protocol_version_config() > 0) {
+		argv_array_push(args, "-o");
+		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
+		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
+				 get_protocol_version_config());
+	}
+
+	if (variant != VARIANT_SIMPLE) {
+		if (flags & CONNECT_IPV4)
+			argv_array_push(args, "-4");
+		else if (flags & CONNECT_IPV6)
+			argv_array_push(args, "-6");
+	}
+
+	if (variant == VARIANT_TORTOISEPLINK)
+		argv_array_push(args, "-batch");
+
+	if (port && variant != VARIANT_SIMPLE) {
+		if (variant == VARIANT_SSH)
+			argv_array_push(args, "-p");
+		else
+			argv_array_push(args, "-P");
+
+		argv_array_push(args, port);
+	}
+}
+
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
 			  const char *port, int flags)
@@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
 	}
 
 	argv_array_push(&conn->args, ssh);
-
-	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 (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 && 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);
-	}
-
+	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
 	argv_array_push(&conn->args, ssh_host);
 }
 
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 5/8] connect: split ssh option computation to its own function

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> This puts the determination of options to pass to each ssh variant
> (see ssh.variant in git-config(1)) in one place.
>
> A follow-up patch will use this in an initial dry run to detect which
> variant to use when the ssh command is ambiguous.
>
> No functional change intended yet.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Split out to make patch 6 easier to read, as suggested at
> https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/.
>
> Added a function comment to make the purpose and API of this internal
> helper clearer.

The resulting fill-ssh-args reads a lot nicer.  Good.

>
>  connect.c | 65 ++++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index a9dc493db2..d2fbb15cc5 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
>  	return conn;
>  }
>  
> +/*
> + * Append the appropriate environment variables to `env` and options to
> + * `args` for running ssh in Git's SSH-tunneled transport.
> + */
> +static void push_ssh_options(struct argv_array *args, struct argv_array *env,
> +			     enum ssh_variant variant, const char *port,
> +			     int flags)
> +{
> +	if (variant == VARIANT_SSH &&
> +	    get_protocol_version_config() > 0) {
> +		argv_array_push(args, "-o");
> +		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> +		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> +				 get_protocol_version_config());
> +	}
> +
> +	if (variant != VARIANT_SIMPLE) {
> +		if (flags & CONNECT_IPV4)
> +			argv_array_push(args, "-4");
> +		else if (flags & CONNECT_IPV6)
> +			argv_array_push(args, "-6");
> +	}
> +
> +	if (variant == VARIANT_TORTOISEPLINK)
> +		argv_array_push(args, "-batch");
> +
> +	if (port && variant != VARIANT_SIMPLE) {
> +		if (variant == VARIANT_SSH)
> +			argv_array_push(args, "-p");
> +		else
> +			argv_array_push(args, "-P");
> +
> +		argv_array_push(args, port);
> +	}
> +}
> +
>  /* Prepare a child_process for use by Git's SSH-tunneled transport. */
>  static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
>  			  const char *port, int flags)
> @@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
>  	}
>  
>  	argv_array_push(&conn->args, ssh);
> -
> -	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 (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 && 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);
> -	}
> -
> +	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
>  	argv_array_push(&conn->args, ssh_host);
>  }

[PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Jonathan Nieder]

Android's "repo" tool is a tool for managing a large codebase
consisting of multiple smaller repositories, similar to Git's
submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
'simple' ssh variant, 2017-10-16), users noticed that it stopped
handling the port in ssh:// URLs.

The cause: when it encounters ssh:// URLs, repo pre-connects to the
server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses
that connection.  Before 94b8ae5a, the helper was assumed to support
OpenSSH options for lack of a better guess and got passed a -p option
to set the port.  After that patch, it uses the new default of a
simple helper that does not accept an option to set the port.

The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid
that.  But users of old versions and of other similar GIT_SSH
implementations would not get the benefit of that fix.

So update the default to use OpenSSH options again, with a twist.  As
observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles
OpenSSH options: common helpers such as travis-ci's dpl[*] are
configured using GIT_SSH and do not accept OpenSSH options.  So make
the default a new variant "auto", with the following behavior:

 1. First, check for a recognized basename, like today.

 2. If the basename is not recognized, check whether $GIT_SSH supports
    OpenSSH options by running

	$GIT_SSH -G <options> <host>

    This returns status 0 and prints configuration in OpenSSH if it
    recognizes all <options> and returns status 255 if it encounters
    an unrecognized option.  A wrapper script like

	exec ssh -- "$@"

    would fail with

	ssh: Could not resolve hostname -g: Name or service not known

    , correctly reflecting that it does not support OpenSSH options.
    The command is run with stdin, stdout, and stderr redirected to
    /dev/null so even a command that expects a terminal would exit
    immediately.

 3. Based on the result from step (2), behave like "ssh" (if it
    succeeded) or "simple" (if it failed).

This way, the default ssh variant for unrecognized commands can handle
both the repo and dpl cases as intended.

This autodetection has been running on Google workstations since
2017-10-23 with no reported negative effects.

[*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215

Reported-by: William Yan <wyan@google.com>
Improved-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Added two notes to the commit message:
 - describing the real-world testing this patch has undergone
 - stdin, stdout, and stderr go to /dev/null, preventing a
   hypothetical ssh variant that *ignores* -G from hanging waiting for
   input from the terminal.

This is to address the worries at
https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/
and https://public-inbox.org/git/CAGZ79kZTjUvcq_hKHCqTDoaBxt2x+9XcqYc6ao1bhcET2SM-PQ@mail.gmail.com/
about hanging.

No change to the code from last time.

 Documentation/config.txt | 24 +++++++++++++++---------
 connect.c                | 32 +++++++++++++++++++++++++-------
 t/t5601-clone.sh         | 21 +++++++++++++++++++++
 3 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0460af37e2..0c371ad786 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands.  This means any URLs
 visited as a result of a redirection do not participate in matching.
 
 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 ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
-	(simple).
+	By default, Git determines the command line arguments to use
+	based on the basename of the configured SSH command (configured
+	using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
+	the config setting `core.sshCommand`). If the basename is
+	unrecognized, Git will attempt to detect support of OpenSSH
+	options by first invoking the configured SSH command with the
+	`-G` (print configuration) option and will subsequently use
+	OpenSSH options (if that is successful) or no options besides
+	the host and remote command (if it fails).
 +
-The config variable `ssh.variant` can be set to override this auto-detection;
-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 config variable `ssh.variant` can be set to override this detection.
+Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
+`tortoiseplink`, `simple` (no options except the host and remote command).
+The default auto-detection can be explicitly requested using the value
+`auto`.  Any other value is treated as `ssh`.  This setting can also be
+overridden via the environment variable `GIT_SSH_VARIANT`.
 +
 The current command-line parameters used for each variant are as
 follows:
diff --git a/connect.c b/connect.c
index d2fbb15cc5..455c54a2ec 100644
--- a/connect.c
+++ b/connect.c
@@ -788,6 +788,7 @@ static const char *get_ssh_command(void)
 }
 
 enum ssh_variant {
+	VARIANT_AUTO,
 	VARIANT_SIMPLE,
 	VARIANT_SSH,
 	VARIANT_PLINK,
@@ -795,14 +796,16 @@ enum ssh_variant {
 	VARIANT_TORTOISEPLINK,
 };
 
-static int override_ssh_variant(enum ssh_variant *ssh_variant)
+static void override_ssh_variant(enum ssh_variant *ssh_variant)
 {
 	const char *variant = getenv("GIT_SSH_VARIANT");
 
 	if (!variant && git_config_get_string_const("ssh.variant", &variant))
-		return 0;
+		return;
 
-	if (!strcmp(variant, "plink"))
+	if (!strcmp(variant, "auto"))
+		*ssh_variant = VARIANT_AUTO;
+	else if (!strcmp(variant, "plink"))
 		*ssh_variant = VARIANT_PLINK;
 	else if (!strcmp(variant, "putty"))
 		*ssh_variant = VARIANT_PUTTY;
@@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)
 		*ssh_variant = VARIANT_SIMPLE;
 	else
 		*ssh_variant = VARIANT_SSH;
-
-	return 1;
 }
 
 static enum ssh_variant determine_ssh_variant(const char *ssh_command,
 					      int is_cmdline)
 {
-	enum ssh_variant ssh_variant = VARIANT_SIMPLE;
+	enum ssh_variant ssh_variant = VARIANT_AUTO;
 	const char *variant;
 	char *p = NULL;
 
-	if (override_ssh_variant(&ssh_variant))
+	override_ssh_variant(&ssh_variant);
+
+	if (ssh_variant != VARIANT_AUTO)
 		return ssh_variant;
 
 	if (!is_cmdline) {
@@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
 		variant = determine_ssh_variant(ssh, 0);
 	}
 
+	if (variant == VARIANT_AUTO) {
+		struct child_process detect = CHILD_PROCESS_INIT;
+
+		detect.use_shell = conn->use_shell;
+		detect.no_stdin = detect.no_stdout = detect.no_stderr = 1;
+
+		argv_array_push(&detect.args, ssh);
+		argv_array_push(&detect.args, "-G");
+		push_ssh_options(&detect.args, &detect.env_array,
+				 VARIANT_SSH, port, flags);
+		argv_array_push(&detect.args, ssh_host);
+
+		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
+	}
+
 	argv_array_push(&conn->args, ssh);
 	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
 	argv_array_push(&conn->args, ssh_host);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9d007c0f8d..209e2d5604 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' '
 	expect_ssh myhost src
 '
 
+test_expect_success 'variant=auto picks based on basename' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	git -c ssh.variant=auto clone -4 "[myhost:123]:src" ssh-auto-clone &&
+	expect_ssh "-4 -P 123" 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 &&
@@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' '
 	expect_ssh myhost src
 '
 
+test_expect_success 'OpenSSH-like uplink is treated as ssh' '
+	write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
+	if test "\$1" = "-G"
+	then
+		exit 0
+	fi &&
+	exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
+	EOF
+	test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" &&
+	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
+	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
+	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
+	expect_ssh "-p 123" myhost src
+'
+
 test_expect_success 'plink is treated specially (as putty)' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 	git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> Android's "repo" tool is a tool for managing a large codebase
> consisting of multiple smaller repositories, similar to Git's
> submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
> handling the port in ssh:// URLs.
> 
> The cause: when it encounters ssh:// URLs, repo pre-connects to the
> server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses
> that connection.  Before 94b8ae5a, the helper was assumed to support
> OpenSSH options for lack of a better guess and got passed a -p option
> to set the port.  After that patch, it uses the new default of a
> simple helper that does not accept an option to set the port.
> 
> The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid
> that.  But users of old versions and of other similar GIT_SSH
> implementations would not get the benefit of that fix.
> 
> So update the default to use OpenSSH options again, with a twist.  As
> observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles
> OpenSSH options: common helpers such as travis-ci's dpl[*] are
> configured using GIT_SSH and do not accept OpenSSH options.  So make
> the default a new variant "auto", with the following behavior:
> 
>  1. First, check for a recognized basename, like today.
> 
>  2. If the basename is not recognized, check whether $GIT_SSH supports
>     OpenSSH options by running
> 
> 	$GIT_SSH -G <options> <host>
> 
>     This returns status 0 and prints configuration in OpenSSH if it
>     recognizes all <options> and returns status 255 if it encounters
>     an unrecognized option.  A wrapper script like
> 
> 	exec ssh -- "$@"
> 
>     would fail with
> 
> 	ssh: Could not resolve hostname -g: Name or service not known
> 
>     , correctly reflecting that it does not support OpenSSH options.
>     The command is run with stdin, stdout, and stderr redirected to
>     /dev/null so even a command that expects a terminal would exit
>     immediately.
> 
>  3. Based on the result from step (2), behave like "ssh" (if it
>     succeeded) or "simple" (if it failed).
> 
> This way, the default ssh variant for unrecognized commands can handle
> both the repo and dpl cases as intended.
> 
> This autodetection has been running on Google workstations since
> 2017-10-23 with no reported negative effects.
> 
> [*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215
> 
> Reported-by: William Yan <wyan@google.com>
> Improved-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Added two notes to the commit message:
>  - describing the real-world testing this patch has undergone
>  - stdin, stdout, and stderr go to /dev/null, preventing a
>    hypothetical ssh variant that *ignores* -G from hanging waiting for
>    input from the terminal.
> 
> This is to address the worries at
> https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/
> and https://public-inbox.org/git/CAGZ79kZTjUvcq_hKHCqTDoaBxt2x+9XcqYc6ao1bhcET2SM-PQ@mail.gmail.com/
> about hanging.
> 
> No change to the code from last time.
> 

Thanks a lot for getting this patch out. It's a much more robust
solution than I had originally and should hopefully avoid any more issues
with detecting different ssh programs.  My only concern is there may be
some program out there that uses -G in a way different from OpenSSH.
Though realistically i don't think that is an issue because if that ends
up being a problem we can just have the authors of the offending ssh
client send a patch to us to fix it! :)

>  Documentation/config.txt | 24 +++++++++++++++---------
>  connect.c                | 32 +++++++++++++++++++++++++-------
>  t/t5601-clone.sh         | 21 +++++++++++++++++++++
>  3 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0460af37e2..0c371ad786 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands.  This means any URLs
>  visited as a result of a redirection do not participate in matching.
>  
>  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 ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
> -	(simple).
> +	By default, Git determines the command line arguments to use
> +	based on the basename of the configured SSH command (configured
> +	using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
> +	the config setting `core.sshCommand`). If the basename is
> +	unrecognized, Git will attempt to detect support of OpenSSH
> +	options by first invoking the configured SSH command with the
> +	`-G` (print configuration) option and will subsequently use
> +	OpenSSH options (if that is successful) or no options besides
> +	the host and remote command (if it fails).
>  +
> -The config variable `ssh.variant` can be set to override this auto-detection;
> -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 config variable `ssh.variant` can be set to override this detection.
> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
> +`tortoiseplink`, `simple` (no options except the host and remote command).
> +The default auto-detection can be explicitly requested using the value
> +`auto`.  Any other value is treated as `ssh`.  This setting can also be
> +overridden via the environment variable `GIT_SSH_VARIANT`.
>  +
>  The current command-line parameters used for each variant are as
>  follows:
> diff --git a/connect.c b/connect.c
> index d2fbb15cc5..455c54a2ec 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -788,6 +788,7 @@ static const char *get_ssh_command(void)
>  }
>  
>  enum ssh_variant {
> +	VARIANT_AUTO,
>  	VARIANT_SIMPLE,
>  	VARIANT_SSH,
>  	VARIANT_PLINK,
> @@ -795,14 +796,16 @@ enum ssh_variant {
>  	VARIANT_TORTOISEPLINK,
>  };
>  
> -static int override_ssh_variant(enum ssh_variant *ssh_variant)
> +static void override_ssh_variant(enum ssh_variant *ssh_variant)
>  {
>  	const char *variant = getenv("GIT_SSH_VARIANT");
>  
>  	if (!variant && git_config_get_string_const("ssh.variant", &variant))
> -		return 0;
> +		return;
>  
> -	if (!strcmp(variant, "plink"))
> +	if (!strcmp(variant, "auto"))
> +		*ssh_variant = VARIANT_AUTO;
> +	else if (!strcmp(variant, "plink"))
>  		*ssh_variant = VARIANT_PLINK;
>  	else if (!strcmp(variant, "putty"))
>  		*ssh_variant = VARIANT_PUTTY;
> @@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)
>  		*ssh_variant = VARIANT_SIMPLE;
>  	else
>  		*ssh_variant = VARIANT_SSH;
> -
> -	return 1;
>  }
>  
>  static enum ssh_variant determine_ssh_variant(const char *ssh_command,
>  					      int is_cmdline)
>  {
> -	enum ssh_variant ssh_variant = VARIANT_SIMPLE;
> +	enum ssh_variant ssh_variant = VARIANT_AUTO;
>  	const char *variant;
>  	char *p = NULL;
>  
> -	if (override_ssh_variant(&ssh_variant))
> +	override_ssh_variant(&ssh_variant);
> +
> +	if (ssh_variant != VARIANT_AUTO)
>  		return ssh_variant;
>  
>  	if (!is_cmdline) {
> @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
>  		variant = determine_ssh_variant(ssh, 0);
>  	}
>  
> +	if (variant == VARIANT_AUTO) {
> +		struct child_process detect = CHILD_PROCESS_INIT;
> +
> +		detect.use_shell = conn->use_shell;
> +		detect.no_stdin = detect.no_stdout = detect.no_stderr = 1;
> +
> +		argv_array_push(&detect.args, ssh);
> +		argv_array_push(&detect.args, "-G");
> +		push_ssh_options(&detect.args, &detect.env_array,
> +				 VARIANT_SSH, port, flags);
> +		argv_array_push(&detect.args, ssh_host);
> +
> +		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
> +	}
> +
>  	argv_array_push(&conn->args, ssh);
>  	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
>  	argv_array_push(&conn->args, ssh_host);
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 9d007c0f8d..209e2d5604 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' '
>  	expect_ssh myhost src
>  '
>  
> +test_expect_success 'variant=auto picks based on basename' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> +	git -c ssh.variant=auto clone -4 "[myhost:123]:src" ssh-auto-clone &&
> +	expect_ssh "-4 -P 123" 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 &&
> @@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' '
>  	expect_ssh myhost src
>  '
>  
> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> +	write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
> +	if test "\$1" = "-G"
> +	then
> +		exit 0
> +	fi &&
> +	exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
> +	EOF
> +	test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" &&
> +	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> +	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> +	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> +	expect_ssh "-p 123" myhost src
> +'
> +
>  test_expect_success 'plink is treated specially (as putty)' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams

Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Android's "repo" tool is a tool for managing a large codebase
> consisting of multiple smaller repositories, similar to Git's
> submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
> handling the port in ssh:// URLs.
>
> ...
> Reported-by: William Yan <wyan@google.com>
> Improved-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

Not a big issue, but the above made me wonder, due to lack of any
signed-off-by before improved-by, what "base" was improved by JTan.
If you were writing a change before formally passing it around with
your sign-off and somebody had a valuable input to improve it, it
seems that people say helped-by around here.

>  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 ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
> -	(simple).
> +	By default, Git determines the command line arguments to use
> +	based on the basename of the configured SSH command (configured
> +	using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
> +	the config setting `core.sshCommand`). If the basename is
> +	unrecognized, Git will attempt to detect support of OpenSSH
> +	options by first invoking the configured SSH command with the
> +	`-G` (print configuration) option and will subsequently use
> +	OpenSSH options (if that is successful) or no options besides
> +	the host and remote command (if it fails).
>  +
> -The config variable `ssh.variant` can be set to override this auto-detection;
> -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 config variable `ssh.variant` can be set to override this detection.
> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
> +`tortoiseplink`, `simple` (no options except the host and remote command).
> +The default auto-detection can be explicitly requested using the value
> +`auto`.  Any other value is treated as `ssh`.  This setting can also be
> +overridden via the environment variable `GIT_SSH_VARIANT`.
>  +

Cleanly written and easily read.  Good.

> @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
>  		variant = determine_ssh_variant(ssh, 0);
>  	}
>  
> +	if (variant == VARIANT_AUTO) {
> +		struct child_process detect = CHILD_PROCESS_INIT;
> +
> +		detect.use_shell = conn->use_shell;
> +		detect.no_stdin = detect.no_stdout = detect.no_stderr = 1;
> +
> +		argv_array_push(&detect.args, ssh);
> +		argv_array_push(&detect.args, "-G");
> +		push_ssh_options(&detect.args, &detect.env_array,
> +				 VARIANT_SSH, port, flags);
> +		argv_array_push(&detect.args, ssh_host);
> +
> +		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
> +	}

I briefly wondered if it safe to simply append "-G" in both cases,
i.e. where "ssh" came from GIT_SSH and it came from GIT_SSH_COMMAND,
but in the real invocation we see in the post-context of this hunk,
we'd push options the same way regardless, so it should be fine.

>  	argv_array_push(&conn->args, ssh);
>  	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
>  	argv_array_push(&conn->args, ssh_host);

> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> +	write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
> +	if test "\$1" = "-G"
> +	then
> +		exit 0
> +	fi &&
> +	exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
> +	EOF

OK.

> +	test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" &&
> +	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> +	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> +	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> +	expect_ssh "-p 123" myhost src
> +'
> +
>  test_expect_success 'plink is treated specially (as putty)' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&

Thanks.

Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Android's "repo" tool is a tool for managing a large codebase
>> consisting of multiple smaller repositories, similar to Git's
>> submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
>> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
>> handling the port in ssh:// URLs.
>>
>> ...
>> Reported-by: William Yan <wyan@google.com>
>> Improved-by: Jonathan Tan <jonathantanmy@google.com>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>
> Not a big issue, but the above made me wonder, due to lack of any
> signed-off-by before improved-by, what "base" was improved by JTan.
> If you were writing a change before formally passing it around with
> your sign-off and somebody had a valuable input to improve it, it
> seems that people say helped-by around here.

Yep, I should have put the Improved-by after my sign-off.

Jonathan Tan's contribution was at
https://public-inbox.org/git/20171023151929.67165aea67353e5c24a15229@google.com/

[...]
>> +The config variable `ssh.variant` can be set to override this detection.
>> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
>> +`tortoiseplink`, `simple` (no options except the host and remote command).
>> +The default auto-detection can be explicitly requested using the value
>> +`auto`.  Any other value is treated as `ssh`.  This setting can also be
>> +overridden via the environment variable `GIT_SSH_VARIANT`.
>>  +
>
> Cleanly written and easily read.  Good.

i.e. that part is all thanks to him. :)

Jonathan

[PATCH 7/8] ssh: 'simple' variant does not support -4/-6

[Jonathan Nieder]

If the user passes -4/--ipv4 or -6/--ipv6 to "git fetch" or "git push"
and the ssh command configured with GIT_SSH does not support such a
setting, error out instead of ignoring the option and continuing.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Stefan Beller <sbeller@google.com>
---
As before.

 connect.c        | 25 ++++++++++++++++++++++---
 t/t5601-clone.sh | 12 ++++++------
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/connect.c b/connect.c
index 455c54a2ec..be106d1868 100644
--- a/connect.c
+++ b/connect.c
@@ -938,11 +938,30 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 				 get_protocol_version_config());
 	}
 
-	if (variant != VARIANT_SIMPLE) {
-		if (flags & CONNECT_IPV4)
+	if (flags & CONNECT_IPV4) {
+		switch (variant) {
+		case VARIANT_AUTO:
+			BUG("VARIANT_AUTO passed to push_ssh_options");
+		case VARIANT_SIMPLE:
+			die("ssh variant 'simple' does not support -4");
+		case VARIANT_SSH:
+		case VARIANT_PLINK:
+		case VARIANT_PUTTY:
+		case VARIANT_TORTOISEPLINK:
 			argv_array_push(args, "-4");
-		else if (flags & CONNECT_IPV6)
+		}
+	} else if (flags & CONNECT_IPV6) {
+		switch (variant) {
+		case VARIANT_AUTO:
+			BUG("VARIANT_AUTO passed to push_ssh_options");
+		case VARIANT_SIMPLE:
+			die("ssh variant 'simple' does not support -6");
+		case VARIANT_SSH:
+		case VARIANT_PLINK:
+		case VARIANT_PUTTY:
+		case VARIANT_TORTOISEPLINK:
 			argv_array_push(args, "-6");
+		}
 	}
 
 	if (variant == VARIANT_TORTOISEPLINK)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 209e2d5604..ad910ae9fa 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -364,9 +364,10 @@ test_expect_success 'OpenSSH variant passes -4' '
 	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 'variant can be overridden' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/putty" &&
+	git -c ssh.variant=putty clone -4 "[myhost:123]:src" ssh-putty-clone &&
+	expect_ssh "-4 -P 123" myhost src
 '
 
 test_expect_success 'variant=auto picks based on basename' '
@@ -375,10 +376,9 @@ test_expect_success 'variant=auto picks based on basename' '
 	expect_ssh "-4 -P 123" myhost src
 '
 
-test_expect_success 'simple is treated as simple' '
+test_expect_success 'simple does not support -4/-6' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
-	git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple &&
-	expect_ssh myhost src
+	test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple
 '
 
 test_expect_success 'uplink is treated as simple' '
-- 
2.15.0.448.gf294e3d99a

[PATCH 8/8] ssh: 'simple' variant does not support --port

[Jonathan Nieder]

When trying to connect to an ssh:// URL with port explicitly specified
and the ssh command configured with GIT_SSH does not support such a
setting, it is less confusing to error out than to silently suppress
the port setting and continue.

This requires updating the GIT_SSH setting in t5603-clone-dirname.sh.
That test is about the directory name produced when cloning various
URLs.  It uses an ssh wrapper that ignores all its arguments but does
not declare that it supports a port argument; update it to set
GIT_SSH_VARIANT=ssh to do so.  (Real-life ssh wrappers that pass a
port argument to OpenSSH would also support -G and would not require
such an update.)

Reported-by: William Yan <wyan@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Stefan Beller <sbeller@google.com>
---
As before.

That's the end of the series.  Thanks for reading.

 connect.c                | 15 ++++++++++++---
 t/t5601-clone.sh         | 10 ++++++++--
 t/t5603-clone-dirname.sh |  2 ++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/connect.c b/connect.c
index be106d1868..20ed1d9574 100644
--- a/connect.c
+++ b/connect.c
@@ -967,11 +967,20 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 	if (variant == VARIANT_TORTOISEPLINK)
 		argv_array_push(args, "-batch");
 
-	if (port && variant != VARIANT_SIMPLE) {
-		if (variant == VARIANT_SSH)
+	if (port) {
+		switch (variant) {
+		case VARIANT_AUTO:
+			BUG("VARIANT_AUTO passed to push_ssh_options");
+		case VARIANT_SIMPLE:
+			die("ssh variant 'simple' does not support setting port");
+		case VARIANT_SSH:
 			argv_array_push(args, "-p");
-		else
+			break;
+		case VARIANT_PLINK:
+		case VARIANT_PUTTY:
+		case VARIANT_TORTOISEPLINK:
 			argv_array_push(args, "-P");
+		}
 
 		argv_array_push(args, port);
 	}
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index ad910ae9fa..78dab81d87 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -378,12 +378,18 @@ test_expect_success 'variant=auto picks based on basename' '
 
 test_expect_success 'simple does not support -4/-6' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
-	test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple
+	test_must_fail git clone -4 "myhost:src" ssh-4-clone-simple
+'
+
+test_expect_success 'simple does not support port' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
+	test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-simple
 '
 
 test_expect_success 'uplink is treated as simple' '
 	copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
-	git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
+	test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
+	git clone "myhost:src" ssh-clone-uplink &&
 	expect_ssh myhost src
 '
 
diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index d5af758129..13b5e5eb9b 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh
@@ -11,7 +11,9 @@ test_expect_success 'setup ssh wrapper' '
 	git upload-pack "$TRASH_DIRECTORY"
 	EOF
 	GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
+	GIT_SSH_VARIANT=ssh &&
 	export GIT_SSH &&
+	export GIT_SSH_VARIANT &&
 	export TRASH_DIRECTORY
 '
 
-- 
2.15.0.448.gf294e3d99a

Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> Previously: [1].
> 
> This version should be essentially identical to v2.  Changes:
> - patch 1 is new and should fix the test failure on Windows
> - patch 2 is new, discussed at [2]
> - patch 5 split off from patch 6 as suggested at [3]
> - patch 6 commit message got two new notes to address the worries
>   from [3]
> 
> Thanks for the helpful reviews, and sorry to take so long to get this
> out.  Thoughts of all kinds welcome, as always.

Just finished looking through the series.  Looks good overall!

Thanks again for getting this out!

> 
> Sincerely,
> Jonathan Nieder (8):
>   ssh test: make copy_ssh_wrapper_as clean up after itself
>   connect: move no_fork fallback to git_tcp_connect
>   connect: split git:// setup into a separate function
>   connect: split ssh command line options into separate function
>   connect: split ssh option computation to its own function
>   ssh: 'auto' variant to select between 'ssh' and 'simple'
>   ssh: 'simple' variant does not support -4/-6
>   ssh: 'simple' variant does not support --port
> 
>  Documentation/config.txt |  24 ++--
>  connect.c                | 322 +++++++++++++++++++++++++++++------------------
>  t/t5601-clone.sh         |  69 ++++++----
>  t/t5603-clone-dirname.sh |   2 +
>  4 files changed, 265 insertions(+), 152 deletions(-)
> 
> [1] https://public-inbox.org/git/20171023231625.6mhcyqti7vdg6yot@aiede.mtv.corp.google.com/
> [2] https://public-inbox.org/git/20171115202516.hduhzsgeoff5a22b@aiede.mtv.corp.google.com/
> [3] https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/

-- 
Brandon Williams

Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[Stefan Beller]

On Mon, Nov 20, 2017 at 2:32 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/20, Jonathan Nieder wrote:
>> Previously: [1].
>>
>> This version should be essentially identical to v2.  Changes:
>> - patch 1 is new and should fix the test failure on Windows
>> - patch 2 is new, discussed at [2]
>> - patch 5 split off from patch 6 as suggested at [3]
>> - patch 6 commit message got two new notes to address the worries
>>   from [3]
>>
>> Thanks for the helpful reviews, and sorry to take so long to get this
>> out.  Thoughts of all kinds welcome, as always.
>
> Just finished looking through the series.  Looks good overall!
>
> Thanks again for getting this out!

Same here,

Thanks,
Stefan

Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder (8):
>   ssh test: make copy_ssh_wrapper_as clean up after itself
>   connect: move no_fork fallback to git_tcp_connect
>   connect: split git:// setup into a separate function
>   connect: split ssh command line options into separate function
>   connect: split ssh option computation to its own function
>   ssh: 'auto' variant to select between 'ssh' and 'simple'
>   ssh: 'simple' variant does not support -4/-6
>   ssh: 'simple' variant does not support --port

Thanks.  All looked sensible.  With this, we can unblock both topics
;-)