Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
Torsten, The relevant part of the path in GIT_SSH was ‘/uplink_deploy/‘. I did begin to use GIT_SSH_COMMAND as a workaround, but regardless this still feels like an overly broad way to determine the value of the putty flag. I was kind of surprised to find it being inferred from the value of GIT_SSH instead of being explicitly set by some additional variable. GIT_USE_PUTTY or some such, though I can understand there may be some reluctance to put the onus of that on the end user. Part of the issue stemmed from not being able to find any documentation on this. After I discovered what was happening I found plenty of instructions that indicated to enable putty support set GIT_SSH to /path/to/plink.exe, but I didn’t find it stated anywhere that if ‘plink’ was found in GIT_SSH, then git will add the -batch option to the command args. In other words, I was able to find instructions on what to do if we had been using putty, but not instructions on unexpected behavior if using GIT_SHH while NOT using putty. > On Apr 23, 2015, at 12:08 AM, Torsten Bögershausen wrote: > > On 04/22/2015 09:12 PM, Patrick Sharp wrote: >> Johannes, >> >> You’re correct, looking back over it, I was pretty vague. >> >> In truth, we are not using Windows OR putty at all. Running git on an >> Ubuntu system, but we are setting the GIT_SSH environment variable to point >> to a shell script to use. >> >> Upon attempting to run git ls-remote, the system was spitting out >> getaddrinfo errors for ‘atch’ . >> >> Setting GIT_TRACE=2 showed that -batch was being added to the git command. >> >> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 >> and 2.3.5 >> >> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH >> variable that was linked to the extra -batch flag. >> >> Finally, after searching the git source, we narrowed it down to the ‘plink’ >> portion of the string. >> >> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7 > Brian, I got your patch, > but can't see it in the list yet > 1/2 looks good, thanks. > (And add msygit) > > My feeling is that patch 2/2 may break things for an unknown > amount of users which e.g. use "myplink". > > Patrick, > did you ever tell us, what you put into $GIT_SSH ? > > Can your use case be covered by using $GIT_SSH_COMMAND ? > > > > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On 04/22/2015 09:12 PM, Patrick Sharp wrote: Johannes, You’re correct, looking back over it, I was pretty vague. In truth, we are not using Windows OR putty at all. Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use. Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ . Setting GIT_TRACE=2 showed that -batch was being added to the git command. This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5 After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag. Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string. https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7 Brian, I got your patch, but can't see it in the list yet 1/2 looks good, thanks. (And add msygit) My feeling is that patch 2/2 may break things for an unknown amount of users which e.g. use "myplink". Patrick, did you ever tell us, what you put into $GIT_SSH ? Can your use case be covered by using $GIT_SSH_COMMAND ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On Wed, Apr 22, 2015 at 10:24:55PM +, brian m. carlson wrote: > 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. Thanks. > > 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: > [...] > > I can drop this in as a preparatory patch if I can have your sign-off. Definitely, thanks. Signed-off-by: Jeff King -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
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
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On Wed, Apr 22, 2015 at 09:44:45PM +, 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
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
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]));
}
argv_array_push(&conn->args, ssh);
--
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
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On Wed, Apr 22, 2015 at 09:19:15PM +, brian m. carlson wrote: > > Note that I don't think just switching the strcasestr to look for > > "plink.exe" is right. For one thing, it just punts on the problem (it > > can still happen, it's just less likely to trigger). But for another, > > you can have plink (without ".exe") on Linux systems. > > 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). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On Wed, Apr 22, 2015 at 04:29:10PM -0400, Jeff King wrote:
> I think you want something like:
>
> diff --git a/connect.c b/connect.c
> index 9ae991a..58aad56 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char
> *url_orig,
> conn->argv = arg = xcalloc(7, sizeof(*arg));
> if (protocol == PROTO_SSH) {
> const char *ssh = getenv("GIT_SSH");
> - int putty = ssh && strcasestr(ssh, "plink");
> + int putty = ssh && (ends_with(ssh, "plink") ||
> + ends_with("plink.exe"));
> if (!ssh) ssh = "ssh";
>
> *arg++ = ssh;
>
> though that is not quite enough (we do not have a case-insensitive
> version of "ends_with"). I'm also not sure if matching just "plink" and
> "plink.exe" at the end of the string is enough (I'm just guessing that
> was the original reason for using strstr in the first place).
>
> Note that I don't think just switching the strcasestr to look for
> "plink.exe" is right. For one thing, it just punts on the problem (it
> can still happen, it's just less likely to trigger). But for another,
> you can have plink (without ".exe") on Linux systems.
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.
--
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
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
On Wed, Apr 22, 2015 at 02:12:53PM -0500, Patrick Sharp wrote:
> Johannes,
>
> You’re correct, looking back over it, I was pretty vague.
>
> In truth, we are not using Windows OR putty at all. Running git on an Ubuntu
> system, but we are setting the GIT_SSH environment variable to point to a
> shell script to use.
>
> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo
> errors for ‘atch’ .
>
> Setting GIT_TRACE=2 showed that -batch was being added to the git command.
>
> This was seen on several different servers with git versions 1.8.5.2, 1.9.1
> and 2.3.5
>
> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH
> variable that was linked to the extra -batch flag.
>
> Finally, after searching the git source, we narrowed it down to the ‘plink’
> portion of the string.
>
> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756
I think you want something like:
diff --git a/connect.c b/connect.c
index 9ae991a..58aad56 100644
--- a/connect.c
+++ b/connect.c
@@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
conn->argv = arg = xcalloc(7, sizeof(*arg));
if (protocol == PROTO_SSH) {
const char *ssh = getenv("GIT_SSH");
- int putty = ssh && strcasestr(ssh, "plink");
+ int putty = ssh && (ends_with(ssh, "plink") ||
+ ends_with("plink.exe"));
if (!ssh) ssh = "ssh";
*arg++ = ssh;
though that is not quite enough (we do not have a case-insensitive
version of "ends_with"). I'm also not sure if matching just "plink" and
"plink.exe" at the end of the string is enough (I'm just guessing that
was the original reason for using strstr in the first place).
Note that I don't think just switching the strcasestr to look for
"plink.exe" is right. For one thing, it just punts on the problem (it
can still happen, it's just less likely to trigger). But for another,
you can have plink (without ".exe") on Linux systems.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
Johannes, You’re correct, looking back over it, I was pretty vague. In truth, we are not using Windows OR putty at all. Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use. Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ . Setting GIT_TRACE=2 showed that -batch was being added to the git command. This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5 After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag. Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string. https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756 > On Apr 22, 2015, at 12:46 PM, Johannes Schindelin > wrote: > > Hi Patrick, > > On 2015-04-22 16:36, Patrick Sharp wrote: >> The plink string detection in GIT_SSH for setting putty to true is very >> broad. > > Wow. You probably wanted to state that you are using Windows, downloaded Git > from [link here], that you are using [version] and that you use PLink > [version] (from the Putty package downloaded [link here]) to do your ssh > business. > > Without that information, you leave readers who have no idea about Putty > *quite* puzzled. > >> If plink is anywhere in the path to the shell file then putty gets set >> to true and ssh will fail trying to parse -batch as the hostname. > > This is cryptic even for me. > >> Wouldn’t searching for plink.exe be better?-- > > I invite you to try your hand at improving anything you find flawed. For > example, if you want to improve the PLink detection in Git for Windows 1.x, > this would be the correct place to start: > https://github.com/msysgit/msysgit/blob/70f24b4b0f5f86a5e85f7264a4ee2c0fec2d4391/share/WinGit/install.iss#L232-L253 > (yes, you would have to download the development environment from > https://msysgit.github.com/#download-msysgit and rebuild your own installer > using `/share/msysGit/WinGit/release.sh 1.9.5-patrick` after editing the > installer script). > > Ciao, > Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
Hi Patrick, On 2015-04-22 16:36, Patrick Sharp wrote: > The plink string detection in GIT_SSH for setting putty to true is very broad. Wow. You probably wanted to state that you are using Windows, downloaded Git from [link here], that you are using [version] and that you use PLink [version] (from the Putty package downloaded [link here]) to do your ssh business. Without that information, you leave readers who have no idea about Putty *quite* puzzled. > If plink is anywhere in the path to the shell file then putty gets set > to true and ssh will fail trying to parse -batch as the hostname. This is cryptic even for me. > Wouldn’t searching for plink.exe be better?-- I invite you to try your hand at improving anything you find flawed. For example, if you want to improve the PLink detection in Git for Windows 1.x, this would be the correct place to start: https://github.com/msysgit/msysgit/blob/70f24b4b0f5f86a5e85f7264a4ee2c0fec2d4391/share/WinGit/install.iss#L232-L253 (yes, you would have to download the development environment from https://msysgit.github.com/#download-msysgit and rebuild your own installer using `/share/msysGit/WinGit/release.sh 1.9.5-patrick` after editing the installer script). Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
