On Wed, Apr 22, 2015 at 06:00:54PM -0400, Jeff King wrote:
> Yeah, that looks right to me. You might want to represent the "are we
> tortoise" check as a separate flag, though, and reuse it a few lines
> later.

Sounds like a good idea.  I'll send a more formal patch a bit later
today.

> Also, not related to your patch, but I notice the "putty" declaration is
> in a different scope than I would have expected, which made me wonder if
> it gets initialized in all code paths. I think is from the recent
> addition of CONNECT_DIAG_URL, which pushes the bulk of the code into its
> own else clause, even though the first part of the "if" always returns
> early.  I wonder if it would be simpler to read like:
> 
> diff --git a/connect.c b/connect.c
> index 391d211..749a07b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,
>  				free(path);
>  				free(conn);
>  				return NULL;
> -			} else {
> -				ssh = getenv("GIT_SSH_COMMAND");
> -				if (ssh) {
> -					conn->use_shell = 1;
> -					putty = 0;
> -				} else {
> -					ssh = getenv("GIT_SSH");
> -					if (!ssh)
> -						ssh = "ssh";
> -					putty = !!strcasestr(ssh, "plink");
> -				}
> -
> -				argv_array_push(&conn->args, ssh);
> -				if (putty && !strcasestr(ssh, "tortoiseplink"))
> -					argv_array_push(&conn->args, "-batch");
> -				if (port) {
> -					/* P is for PuTTY, p is for OpenSSH */
> -					argv_array_push(&conn->args, putty ? "-P" : "-p");
> -					argv_array_push(&conn->args, port);
> -				}
> -				argv_array_push(&conn->args, ssh_host);
>  			}
> +
> +			ssh = getenv("GIT_SSH_COMMAND");
> +			if (ssh) {
> +				conn->use_shell = 1;
> +				putty = 0;
> +			} else {
> +				ssh = getenv("GIT_SSH");
> +				if (!ssh)
> +					ssh = "ssh";
> +				putty = !!strcasestr(ssh, "plink");
> +			}
> +
> +			argv_array_push(&conn->args, ssh);
> +			if (putty && !strcasestr(ssh, "tortoiseplink"))
> +				argv_array_push(&conn->args, "-batch");
> +			if (port) {
> +				/* P is for PuTTY, p is for OpenSSH */
> +				argv_array_push(&conn->args, putty ? "-P" : "-p");
> +				argv_array_push(&conn->args, port);
> +			}
> +			argv_array_push(&conn->args, ssh_host);
>  		} else {
>  			/* remove repo-local variables from the environment */
>  			conn->env = local_repo_env;

I can drop this in as a preparatory patch if I can have your sign-off.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187