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
signature.asc
Description: Digital signature
