On Wed, Apr 22, 2015 at 09:44:45PM +0000, brian m. carlson wrote:
> On Wed, Apr 22, 2015 at 05:29:04PM -0400, Jeff King wrote:
> > > Perhaps it would be worthwhile to check instead if the text "plink" is
> > > the beginning of string or is preceded by a path separator. That would
> > > give us a bit more confidence that the user is looking for plink, but
> > > would still allow people to use "plink-0.63" if they like.
> >
> > Yeah, I think that is a reasonable approach. Note that it needs to
> > handle the "tortoiseplink" case from below, too (you can still use your
> > strategy, you just need to look for either string).
>
> So maybe something like this?
>
> diff --git a/connect.c b/connect.c
> index 391d211..ba3ab34 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -749,10 +749,15 @@ struct child_process *git_connect(int fd[2], const char *url,
> conn->use_shell = 1;
> putty = 0;
> } else {
> + char *plink, *tplink;
> +
> ssh = getenv("GIT_SSH");
> if (!ssh)
> ssh = "ssh";
> - putty = !!strcasestr(ssh, "plink");
> + plink = strcasestr(ssh, "plink");
> + tplink = strcasestr(ssh, "tortoiseplink");
> + putty = plink == ssh || (plink && is_dir_sep(plink[-1])) ||
> + tplink == ssh || (tplink && is_dir_sep(tplink[-1]));
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.
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;
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html