Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Subject: [PATCH] connect: rename tortoiseplink and putty variables
> 
> One of these two may have originally been named after "what exact
> SSH implementation do we have" so that we can tweak the command line
> options, but these days "putty=1" no longer means "We are using the
> plink SSH implementation that comes with PuTTY".  It is set when we
> guess that either PuTTY plink or Tortoiseplink is in use.
> 
> Rename them after what effect is desired.  The current "putty"
> option is about using "-P " when OpenSSH would use "-p ",
> so rename it to port_option whose value is either 'p' or 'P".  The
> other one is about passing an extra command line option "-batch",
> so rename it needs_batch.
> 
> Signed-off-by: Junio C Hamano 

Apart from an overly-long line, this patch looks good. It made my life a
little harder, as I had to rebase Segev's ssh.variant (why this should be
a core.* variable is not clear to me) patch and it caused conflicts. I
resolved those conflicts and made both patches part of this patch series.

Will contribute v2 as soon as the test suite passes,
Johannes

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Now, with the patch in question (without the follow-up, which I would
> > like to ask you to ignore, just like you did so far), Git would not
> > figure out that your script calls PuTTY eventually. The work-around?
> > Easy:
> >
> > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh
> 
> Think about how you would explain that to an end-user in our document?
> You'll need to explain how exactly the auto-detection works, so that the
> user can "exploit" the loophole to do that.  And what maintenance burden
> does it add when auto-detection is updated?

Fine, you do not like it. Saying so (instead of asking me questions) would
have been helpful.

> I think I know you well enough that you know well enough that it is too
> ugly to live, and I suspect that the above is a tongue-in-cheek "arguing
> for the sake of argument" and would not need a serious response, but
> just in case...

It was not tongue-in-cheek, I was being serious.

> Yes.  Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).

I'd much rather prefer
https://github.com/git-for-windows/git/pull/1030 than your patch.

Ciao,
Johannes

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: [PATCH] connect: core.sshvariant to correct misidentification
>
> I have been watching this discussion from the sidelines, and I agree
> that this direction is a big improvement.
> ...
> IIRC, the "const" in git_config_get_string_const is only about avoiding
> an annoying cast. The result is still allocated and needs freed. Since
> you are not keeping the value after the function returns, I think you
> could just use git_config_get_value().

It is too late for today's pushout (I have this topic near the tip
of 'pu', so it is possible to tweak and redo only 'pu' branch, but I
generally hate tweaking things after 15:00 my time), but I'll fix
that before the topic hits 'jch' (which is a bit more than 'next'
and that is what I use for everyday work).

Thanks.  

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Jeff King]

On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] connect: core.sshvariant to correct misidentification

I have been watching this discussion from the sidelines, and I agree
that this direction is a big improvement.

> +static void override_ssh_variant(int *port_option, int *needs_batch)
> +{
> + const char *variant;
> +
> + if (git_config_get_string_const("core.sshvariant", ))
> + return;
> + if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + } else if (!strcmp(variant, "putty")) {
> + *port_option = 'P';
> + *needs_batch = 0;
> + } else {
> + /* default */
> + if (strcmp(variant, "ssh")) {
> + warning(_("core.sshvariant: unknown value '%s'"), 
> variant);
> + warning(_("using OpenSSH compatible behaviour"));
> + }
> + *port_option = 'p';
> + *needs_batch = 0;
> + }
> +}

IIRC, the "const" in git_config_get_string_const is only about avoiding
an annoying cast. The result is still allocated and needs freed. Since
you are not keeping the value after the function returns, I think you
could just use git_config_get_value().

(Grepping around, I see a few other places that seem to make the same
mistake. I think this is a confusing interface that should probably be
fixed).

-Peff

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Junio C Hamano  writes:

> Yes.  Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).

... and this is the "obvious clean-up patch" to apply directly on
top of Segev's GIT_SSH_COMMAND support, which comes before the
previous one.

-- >8 --
Subject: [PATCH] connect: rename tortoiseplink and putty variables

One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY".  It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired.  The current "putty"
option is about using "-P " when OpenSSH would use "-p ",
so rename it to port_option whose value is either 'p' or 'P".  The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.

Signed-off-by: Junio C Hamano 
---
 connect.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index c81f77001b..aa51b33bfc 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
-   int putty = 0, tortoiseplink = 0;
+   int needs_batch = 0;
+   int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
 
-   tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
-   !strcasecmp(base, "tortoiseplink.exe");
-   putty = tortoiseplink ||
-   !strcasecmp(base, "plink") ||
-   !strcasecmp(base, "plink.exe");
-
+   if (!strcasecmp(base, "tortoiseplink") ||
+   !strcasecmp(base, "tortoiseplink.exe")) {
+   port_option = 'P';
+   needs_batch = 1;
+   } else if (!strcasecmp(base, "plink") ||
+  !strcasecmp(base, "plink.exe")) {
+   port_option = 'P';
+   }
free(ssh_argv0);
}
 
@@ -833,11 +836,10 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(>args, "-6");
-   if (tortoiseplink)
+   if (needs_batch)
argv_array_push(>args, "-batch");
if (port) {
-   /* P is for PuTTY, p is for OpenSSH */
-   argv_array_push(>args, putty ? "-P" : 
"-p");
+   argv_array_pushf(>args, "-%c", 
port_option);
argv_array_push(>args, port);
}
argv_array_push(>args, ssh_host);
-- 
2.11.0-699-ga1f1475476

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

> Now, with the patch in question (without the follow-up, which I would like
> to ask you to ignore, just like you did so far), Git would not figure out
> that your script calls PuTTY eventually. The work-around? Easy:
>
>   DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh

Think about how you would explain that to an end-user in our
document?  You'll need to explain how exactly the auto-detection
works, so that the user can "exploit" the loophole to do that.  And
what maintenance burden does it add when auto-detection is updated?

I think I know you well enough that you know well enough that it is
too ugly to live, and I suspect that the above is a tongue-in-cheek
"arguing for the sake of argument" and would not need a serious
response, but just in case...

> ...
> Of course, given our discussion I think all of this should be documented
> in the commit message as well as in the man page.

Yes.  Here is what comes on an obvious clean-up patch (which will be
sent as a follow-up to this message).

-- >8 --
Subject: [PATCH] connect: core.sshvariant to correct misidentification

While the logic we have been using to guess which variant of SSH
implementation is in use by looking at the name of the program has
been robust enough for GIT_SSH (which does not go through the shell,
hence it can only spell the name the SSH program and nothing else),
extending it to GIT_SSH_COMMAND and core.sshcommand opens it for
larger chance of misidentification, because these can specify
arbitrary shell command, and a simple "the first word on the line
must be the command name" heuristic may not even look at the command
name at all.

As any effort to improve the heuristic will give us only diminishing
returns, and especially given that most of the users are expected to
have a command line for which the heuristic would work correctly,
give an explicit escape hatch to override a misidentification, just
in case one happens.

It is arguably bad to add this new variable to the core.* set, in
the sense that you'll never see this variable if you do not interact
with other repositories over ssh, but core.sshcommand is already
there for some reason, so let's match it.

Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt | 13 +
 connect.c| 26 ++
 2 files changed, 39 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..8ea1db49c6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -455,6 +455,19 @@ core.sshCommand::
the `GIT_SSH_COMMAND` environment variable and is overridden
when the environment variable is set.
 
+core.sshVariant::
+   SSH implementations used by Git when running `git fetch`,
+   `git clone`, and `git push` use slightly different command
+   line options (e.g. PuTTY and TortoisePlink use `-P `
+   while OpenSSH uses `-p ` to specify the port number.
+   TortoisePlink in addition requires `-batch` option to be
+   passed).  Git guesses which variant is in use correctly from
+   the name of the ssh command used (see `core.sshCommand`
+   configuration variable and also `GIT_SSH_COMMAND`
+   environment variable) most of the time.  You can set this
+   variable to 'putty', 'tortoiseplink', or 'ssh' to correct it
+   when Git makes an incorrect guess.
+
 core.ignoreStat::
If true, Git will avoid using lstat() calls to detect if files have
changed by setting the "assume-unchanged" bit for those tracked files
diff --git a/connect.c b/connect.c
index aa51b33bfc..358e9c06f0 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,29 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
+static void override_ssh_variant(int *port_option, int *needs_batch)
+{
+   const char *variant;
+
+   if (git_config_get_string_const("core.sshvariant", ))
+   return;
+   if (!strcmp(variant, "tortoiseplink")) {
+   *port_option = 'P';
+   *needs_batch = 1;
+   } else if (!strcmp(variant, "putty")) {
+   *port_option = 'P';
+   *needs_batch = 0;
+   } else {
+   /* default */
+   if (strcmp(variant, "ssh")) {
+   warning(_("core.sshvariant: unknown value '%s'"), 
variant);
+   warning(_("using OpenSSH compatible behaviour"));
+   }
+   *port_option = 'p';
+   *needs_batch = 0;
+   }
+}
+
 /*
  * 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,
@@ -836,6 +859,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

> IOW, "give an escape hatch to override auto-detected tortoiseplink and
> plink variables" suggestion should be taken as an "in addition to"
> suggestion (as opposed to an "instead of" suggestion).  I was not clear
> in my very first message, and I thought in my second and my third
> message I clarified it as such, but probably I wasn't explicit enough.

While you may not have been explicit enough, I was not smart enough.

That escape-hatch exists *already*. And it is exactly the thing that you
mentioned earlier as a potential source of mis-identification.

Let's assume that you want to use a script for the GIT_SSH_COMMAND that
eventually uses PuTTY, and you call this script "junio-is-a-superstar.sh".
All plausible so far ;-)

Now, with the patch in question (without the follow-up, which I would like
to ask you to ignore, just like you did so far), Git would not figure out
that your script calls PuTTY eventually. The work-around? Easy:

DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh

In other words: the thing that we thought may be a problem is actually a
feature.

Likewise, if your GIT_SSH_COMMAND looks like this:

THIS_IS_NOT_ACTUALLY_PUTTY=/my/window/needs/putty my-ssh.sh

... you can easily change Git's mind by prefixing

DUMMY=blubber

If you want to use a script that decides itself whether to call OpenSSH or
PuTTY, Git should "stay dumb" about it, as it will not be able to tell
beforehand whether a port argument should be passed via `-p` or `-P`
anyway.

Of course, given our discussion I think all of this should be documented
in the commit message as well as in the man page.

Do you agree this is a good direction to take?

Ciao,
Johannes

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

>> > But do we really need to do that?
>> 
>> No.
>
> Exactly.

> But since you seem to convinced that a future bug report like this may
> happen (I am not, and it contradicts my conviction that one should cross a
> bridge only when reaching it, but whatever), how about this, on top:

I do not understand why you keep arguing.

With No-Exactly exchange, I thought that we established that

 * The original auto-detection for GIT_SSH is battle tested and has
   worked for us very well.  It may probably have covered 99.9% of
   the population, and we haven't heard complaints from the
   remaining 0.1%.

 * The added support for GIT_SSH_COMMAND, because the mechanism
   gives more lattitude to end-users to be creative, will have lower
   chance of correctly deciding between ssh, tortoiseplink and
   plink, but it would still be high enough, say 99%, correct
   detection rate.

 * It is foolish to add more code to refine the auto-detection to
   raise 99% to 99.5%.  We know that the approach fundamentally
   cannot make the detection rate reach 100%.

The last one can be paraphrased as "perfect is the enemy of good".

But that does not mean that it is OK to leave the system unusable
for 1% of the users.  If the auto-detection code does not work
correctly for a tiny minority of users, it is still OK, as long as
we give them an escape hatch to allow them to say "You may not be
able to guess among ssh, tortoiseplink and plink, or you may even
guess incorrectly, so I'll tell you.  I am using plink".  99% of
users do not have to know about that escape hatch, but 1% of users
will be helped.

IOW, "give an escape hatch to override auto-detected tortoiseplink
and plink variables" suggestion should be taken as an "in addition
to" suggestion (as opposed to an "instead of" suggestion).  I was
not clear in my very first message, and I thought in my second and
my third message I clarified it as such, but probably I wasn't
explicit enough.

In any case, "do this only if the first one token on the command
line does not have '='" we see below is flawed for two reasons and I
think it would not be a workable counter-proposal (besides, it goes
against the "perfect is the enemy of good" mantra).

For one thing, the command line may not be "VAR=VAL cmd", but just
"cmd" that names the path to tortoiseplink, but the leading
directory path that houses tortoiseplink may have a '=' in it, in
which case we would detect it correctly if we didn't have such a
hack, but with the hack we would punt.  More importantly, even when
"VAR=VAL cmd" form was caught with the change, it may punt and stop
guessing, but punting alone does not help users by letting them say
"I know you cannot auto-detect, so let me help you---what I have is
PuTTY plink".  If it always assumes that the user uses the plain
vanilla ssh, then the change merely changed what incorrect guess the
auto-detection makes from "tortoiseplink" to "vanilla ssh" for a
user who uses the PuTTY variant of plink.

> -- snipsnap --
> diff --git a/connect.c b/connect.c
> index c81f77001b..b990dd6190 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const
> char *url,
>   char *split_ssh = xstrdup(ssh);
>   const char **ssh_argv;
>  
> - if (split_cmdline(split_ssh, _argv))
> + if (split_cmdline(split_ssh, _argv) &&
> + !strchr(ssh_argv[0], '='))
>   ssh_argv0 = xstrdup(ssh_argv[0]);
>   free(split_ssh);
>   free((void *)ssh_argv);

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > If you feel strongly about your contrived examples possibly being
> > affected by this patch, we could easily make this conditional on
> >
> > 1) no '&&' or '||' being found on the command-line, and
> > 2) argv[0] not containing an '='
> >
> > Another approach would be to verify that argv[i] starts with '-' for
> > non-zero i.
> >
> > But do we really need to do that?
> 
> No.

Exactly.

> > That means that the user has to specify something like
> >
> > HAHAHA_IT_IS_NOT=/plink.exe ssh
> >
> > as GIT_SSH_COMMAND.
> 
> My second message was to clarify that "VAR1=VAL2 command" is NOT a
> contrived example, and this response indicates that I somehow failed
> to convey that to you.

Indeed. The quite contrived example was about a script that chooses
between plink and tortoiseplink (and fails to call anything else). And it
failed to convince me.

But since you seem to convinced that a future bug report like this may
happen (I am not, and it contradicts my conviction that one should cross a
bridge only when reaching it, but whatever), how about this, on top:

-- snipsnap --
diff --git a/connect.c b/connect.c
index c81f77001b..b990dd6190 100644
--- a/connect.c
+++ b/connect.c
@@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const
char *url,
char *split_ssh = xstrdup(ssh);
const char **ssh_argv;
 
-   if (split_cmdline(split_ssh, _argv))
+   if (split_cmdline(split_ssh, _argv) &&
+   !strchr(ssh_argv[0], '='))
ssh_argv0 = xstrdup(ssh_argv[0]);
free(split_ssh);
free((void *)ssh_argv);

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

> If you feel strongly about your contrived examples possibly being
> affected by this patch, we could easily make this conditional on
>
> 1) no '&&' or '||' being found on the command-line, and
> 2) argv[0] not containing an '='
>
> Another approach would be to verify that argv[i] starts with '-' for
> non-zero i.
>
> But do we really need to do that?

No.  An explicit way to override an incorrect guess is sufficient
and necessary.  The above two-item list you gave will be just part
of the machinery to make an incorrect guess.  The auto-detection in
the posted patch should cover many users' use case and I do not
think it needs to be extended further to make it more brittle, as by
definition its guess cannot be perfect.  Just keep it simple and
give a separate escape hatch.

> That means that the user has to specify something like
>
>   HAHAHA_IT_IS_NOT=/plink.exe ssh
>
> as GIT_SSH_COMMAND.

My second message was to clarify that "VAR1=VAL2 command" is NOT a
contrived example, and this response indicates that I somehow failed
to convey that to you.  The "if tortoiseplink exists (and the end
user can override the location with an environment), use that, and
if PuTTY plink exists (ditto), use that instead" in a "myssh"
script, and use it as core.sshcommand with the environment to
override my custom installation location to these two programs,
would be what I would do when I get two Windows machines, with these
variants of SSH on each.  So take the second message as a bug report
against the version of Git for Windows you ship with the patch in
question.

The auto-detection may work for many people and that is a great
thing.  I failed to say that in my message, as I thought that was
obvious.  But it is important to plan to cope with the case where it
does not work.  The usual practice around here is to say "the it may
not necessarily work for everybody, so lets be prepared to add an
explicit override if it turns out to be necessary".  

The second message, which you are responding to, was meant to be a
bug report from the future, telling us that an override is needed,
showing that we do not have to wait for a bug report to act on.

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Sun, 8 Jan 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > I suspect that this will break when GIT_SSH_COMMAND, which is meant
> > to be processed by the shell, hence the user can write anything,
> > begins with a one-shot environment variable assignment, e.g.
> >
> > [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
> >
> > One possible unintended side effect of this patch is when VAL1 ends
> > with /plink (or /tortoiseplink) and the command is not either of
> > these, in which case the "tortoiseplink" and "putty" variables will
> > tweak the command line for an SSH implementation that does not want
> > such a tweak to be made.  There may be other unintended fallouts.
> 
> Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)
> interface takes any shell-interpretable commands, the first "token"
> you see is not limited to a one-shot environment assignment.  It
> could even be "cmd1 && cmd2 && ..." or even:
> 
>   if test -f ${TPLINK:=//path/to/tortoiseplink}
>   then
>   exec "$TPLINK" "$@"
>   elif test -f ${PLINK:=//path/to/plink}
>   exec "$PLINK" "$@"
>   else
>   echo >&2 no usable ssh on this host
>   fi

Sure, it could be all of that. It could even blast through the environment
limits in some environments on some platforms, but not on others. It could
automatically transfer bitcoins whenever a connection to github.com is
attempted. It could start the camera and build a time-lapse of "every time
I pushed or fetched a repository", as an art project. It could shut down
the computer.

It could do all of that.

In practice, however, what users realistically do is to use
GIT_SSH_COMMAND whenever they need to override the ssh command with
options.

This is the common case, and that is what we must make less cumbersome to
use.

If you feel strongly about your contrived examples possibly being affected
by this patch, we could easily make this conditional on

1) no '&&' or '||' being found on the command-line, and
2) argv[0] not containing an '='

Another approach would be to verify that argv[i] starts with '-' for
non-zero i.

But do we really need to do that?

Let's have a very brief look at the amount of work required to come up
with a false positive (I am not so much concerned about any power user
writing elaborate shell expressions that may call plink.exe: those power
users will simply have to continue to use their own -P => -p and -batch
hacks):

The logic kicks in if the split command-line's first component has a
basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The
only way this logic can kick in by mistake is if the first component is
*not* the command to call *and* if the first component *still* has a
basename of `plink` or `tortoiseplink`.

That means that the user has to specify something like

HAHAHA_IT_IS_NOT=/plink.exe ssh

as GIT_SSH_COMMAND.

Now, let's take a *very* brief look at the question how likely it is to
have a basename of `plink` or `tortoiseplink`. Remember, there are two
ways that the basename can be a specific term: either the original string
is already equal to that term, or it ends in a slash followed by the term.
That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e.
it would be a *true* positive. Or the first component would end in
"/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be
a command*. But how likely is it to specify a non-command (such as a
per-process variable assignment) that specifically mentions plink or
tortoiseplink?

Is it even realistic to expect users to specify values for GIT_SSH_COMMAND
that contain "plink" as a substring when they do *not* want to call plink
at all?

After thinking a bit longer than I had originally planned about it, my
answer is: no. No, I do not think it is realistic. I fully expect *no*
user to have a GIT_SSH_COMMAND containing even a substring "plink"
*anywhere*, unless they do, in fact, want to call plink or tortoiseplink.

My main aim with Git for Windows is to improve the user experience, and
the patch in question does do that. Of course, I also try to avoid
breaking existing setups while improving the user experience, and
I believe that it is safe to assume that the patch in question in all
likelihood does not break any existing setup.

Ciao,
Dscho

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Junio C Hamano  writes:

> I suspect that this will break when GIT_SSH_COMMAND, which is meant
> to be processed by the shell, hence the user can write anything,
> begins with a one-shot environment variable assignment, e.g.
>
>   [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
>
> One possible unintended side effect of this patch is when VAL1 ends
> with /plink (or /tortoiseplink) and the command is not either of
> these, in which case the "tortoiseplink" and "putty" variables will
> tweak the command line for an SSH implementation that does not want
> such a tweak to be made.  There may be other unintended fallouts.

Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)
interface takes any shell-interpretable commands, the first "token"
you see is not limited to a one-shot environment assignment.  It
could even be "cmd1 && cmd2 && ..." or even:

if test -f ${TPLINK:=//path/to/tortoiseplink}
then
exec "$TPLINK" "$@"
elif test -f ${PLINK:=//path/to/plink}
exec "$PLINK" "$@"
else
echo >&2 no usable ssh on this host
fi

Worse, the above may be in "myssh" script found on user's PATH and
then core.sshcommand may be set to

TPLINK=//my/path/to/tortoiseplink PLINK=//my/path/to/plink myssh

in the configuration the user uses on multiple hosts, some have
tortoiseplink installed and some do not.  The idea the user who set
it up may be to use whatever available depending on the host.

The posted patch would be confused that we are using tortoiseplink
but the "myssh" script would correctly notice that on a particular
host it is unavailable and choose to use plink instead.

> Sorry, no concrete suggestion to get this to work comes to my mind
> offhand. 
>
> Perhaps give an explicit way to force "tortoiseplink" and "putty"
> variables without looking at and guessing from the pathname, so that
> the solution does not have to split and peek the command line?

A user with such an elaborate set-up with multiple hosts with
different configurations would likely to want to say "Just give me a
regular SSH command line, and in my 'myssh' script I'll futz with
-p/-P differences and such, as I'm writing a small script to cope
with Tortoiseplink and Puttyplink anyway".  With a separate,
explicit configuration variable to tell Git what variant of SSH you
have, even such a user can be served (she would just set ssh.variant
to "vanilla" or whatever that is not "tortoiseplink" or "plink").



.

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

> + if (ssh) {
> + char *split_ssh = xstrdup(ssh);
> + const char **ssh_argv;
> +
> + if (split_cmdline(split_ssh, _argv))
> + ssh_argv0 = xstrdup(ssh_argv[0]);
> + free(split_ssh);
> + free((void *)ssh_argv);
> + } else {
>   /*
>* GIT_SSH is the no-shell version of
>* GIT_SSH_COMMAND (and must remain so for
> @@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char 
> *url,
>   if (!ssh)
>   ssh = "ssh";
>  
> - ssh_dup = xstrdup(ssh);
> - base = basename(ssh_dup);
> + ssh_argv0 = xstrdup(ssh);
> + }
> +
> + if (ssh_argv0) {
> + const char *base = basename(ssh_argv0);
>  
>   tortoiseplink = !strcasecmp(base, 
> "tortoiseplink") ||
>   !strcasecmp(base, "tortoiseplink.exe");

I suspect that this will break when GIT_SSH_COMMAND, which is meant
to be processed by the shell, hence the user can write anything,
begins with a one-shot environment variable assignment, e.g.

[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink

One possible unintended side effect of this patch is when VAL1 ends
with /plink (or /tortoiseplink) and the command is not either of
these, in which case the "tortoiseplink" and "putty" variables will
tweak the command line for an SSH implementation that does not want
such a tweak to be made.  There may be other unintended fallouts.

Sorry, no concrete suggestion to get this to work comes to my mind
offhand. 

Perhaps give an explicit way to force "tortoiseplink" and "putty"
variables without looking at and guessing from the pathname, so that
the solution does not have to split and peek the command line?


 connect.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/connect.c b/connect.c
index 8cb93b0720..1768122456 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,23 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
+static void get_ssh_variant(int *tortoiseplink, int *putty)
+{
+   const char *variant;
+   if (!git_config_get_string_const("ssh.variant", ))
+   return;
+   if (!strcmp(variant, "tortoiseplink")) {
+   *tortoiseplink = 1;
+   *putty = 1;
+   } else if (!strcmp(variant, "putty")) {
+   *tortoiseplink = 0;
+   *putty = 1;
+   } else {
+   *tortoiseplink = 0;
+   *putty = 0;
+   }
+}
+
 /*
  * 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,
@@ -824,6 +841,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(>args, "-6");
+
+   get_ssh_variant(, );
+
if (tortoiseplink)
argv_array_push(>args, "-batch");
if (port) {

[PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

From: Segev Finer 

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer 
Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v1
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v1


Original Pull Request:
https://github.com/git-for-windows/git/pull/1006

 connect.c| 23 ---
 t/t5601-clone.sh | 15 +++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
int putty = 0, tortoiseplink = 0;
char *ssh_host = hostandport;
const char *port = NULL;
+   char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char 
*url,
}
 
ssh = get_ssh_command();
-   if (!ssh) {
-   const char *base;
-   char *ssh_dup;
-
+   if (ssh) {
+   char *split_ssh = xstrdup(ssh);
+   const char **ssh_argv;
+
+   if (split_cmdline(split_ssh, _argv))
+   ssh_argv0 = xstrdup(ssh_argv[0]);
+   free(split_ssh);
+   free((void *)ssh_argv);
+   } else {
/*
 * GIT_SSH is the no-shell version of
 * GIT_SSH_COMMAND (and must remain so for
@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (!ssh)
ssh = "ssh";
 
-   ssh_dup = xstrdup(ssh);
-   base = basename(ssh_dup);
+   ssh_argv0 = xstrdup(ssh);
+   }
+
+   if (ssh_argv0) {
+   const char *base = basename(ssh_argv0);
 
tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
!strcasecmp(base, "tortoiseplink.exe");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
!strcasecmp(base, "plink") ||
!strcasecmp(base, "plink.exe");
 
-   free(ssh_dup);
+   free(ssh_argv0);
}
 
argv_array_push(>args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a433394200..5b228e2675 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with 
extra arguments' '
expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+   GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+   git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+   expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in