* [PATCH v2 1/3] connect: handle putty/plink also in GIT_SSH_COMMAND
[not found] ` <cover.1485442231.git.johannes.schindelin@gmx.de>
@ 2017-01-26 14:51 ` Johannes Schindelin
2017-01-26 14:51 ` [PATCH v2 2/3] connect: rename tortoiseplink and putty variables Johannes Schindelin
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-01-26 14:51 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
From: Segev Finer <segev208@gmail.com>
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 <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
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(&ssh_host, &port);
@@ -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, &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(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 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 GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 2/3] connect: rename tortoiseplink and putty variables
[not found] ` <cover.1485442231.git.johannes.schindelin@gmx.de>
2017-01-26 14:51 ` [PATCH v2 1/3] connect: handle putty/plink also " Johannes Schindelin
@ 2017-01-26 14:51 ` Johannes Schindelin
2017-01-26 14:52 ` [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
3 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-01-26 14:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Junio C Hamano, Jeff King
From: Junio C Hamano <gitster@pobox.com>
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 <port>" when OpenSSH would use "-p <port>",
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.
[jes: wrapped overly-long line]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 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,11 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
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_pushf(&conn->args,
+ "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
[not found] ` <cover.1485442231.git.johannes.schindelin@gmx.de>
2017-01-26 14:51 ` [PATCH v2 1/3] connect: handle putty/plink also " Johannes Schindelin
2017-01-26 14:51 ` [PATCH v2 2/3] connect: rename tortoiseplink and putty variables Johannes Schindelin
@ 2017-01-26 14:52 ` Johannes Schindelin
2017-01-26 19:27 ` Junio C Hamano
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
3 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2017-01-26 14:52 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
From: Segev Finer <segev208@gmail.com>
This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.
[jes: wrapped overly-long lines, changed get_ssh_variant() to
handle_ssh_variant() to accomodate the change from the
putty/tortoiseplink variables to port_option/needs_batch.]
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/config.txt | 7 +++++++
Documentation/git.txt | 7 +++++++
connect.c | 24 ++++++++++++++++++++++--
t/t5601-clone.sh | 26 ++++++++++++++++++++++++++
4 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..f2c210f0a0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are
matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
+ssh.variant::
+ Override the autodetection of plink/tortoiseplink in the SSH
+ command that 'git fetch' and 'git push' use. It can be set to
+ either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
+ value will be treated as normal ssh. This is useful in case
+ that Git gets this wrong.
+
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..c322558aa7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
personal `.ssh/config` file. Please consult your ssh documentation
for further details.
+`GIT_SSH_VARIANT`::
+ If this environment variable is set, it overrides the autodetection
+ of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
+ push' use. It can be set to either `ssh`, `plink`, `putty` or
+ `tortoiseplink`. Any other value will be treated as normal ssh. This
+ is useful in case that Git gets this wrong.
+
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 9f750eacb6..7b4437578b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
return NULL;
}
+static int handle_ssh_variant(int *port_option, int *needs_batch)
+{
+ const char *variant;
+
+ if (!(variant = getenv("GIT_SSH_VARIANT")) &&
+ git_config_get_string_const("ssh.variant", &variant))
+ return 0;
+
+ if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+ *port_option = 'P';
+ else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ }
+
+ return 1;
+}
+
/*
* 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,
@@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh_argv0 = xstrdup(ssh);
}
- if (ssh_argv0) {
+ if (!handle_ssh_variant(&port_option, &needs_batch) &&
+ ssh_argv0) {
const char *base = basename(ssh_argv0);
if (!strcasecmp(base, "tortoiseplink") ||
@@ -828,9 +847,10 @@ struct child_process *git_connect(int fd[2], const char *url,
!strcasecmp(base, "plink.exe")) {
port_option = 'P';
}
- free(ssh_argv0);
}
+ free(ssh_argv0);
+
argv_array_push(&conn->args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(&conn->args, "-4");
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
'
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ GIT_SSH_VARIANT=ssh \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ git -c ssh.variant=ssh \
+ clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+ GIT_SSH_VARIANT=plink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+ expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+ GIT_SSH_VARIANT=tortoiseplink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+ expect_ssh "-batch -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-01-26 14:52 ` [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
@ 2017-01-26 19:27 ` Junio C Hamano
2017-01-27 10:35 ` Johannes Schindelin
2017-01-27 18:17 ` Junio C Hamano
0 siblings, 2 replies; 38+ messages in thread
From: Junio C Hamano @ 2017-01-26 19:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4cc02..f2c210f0a0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are
> matched against are those given directly to Git commands. This means any URLs
> visited as a result of a redirection do not participate in matching.
>
> +ssh.variant::
> + Override the autodetection of plink/tortoiseplink in the SSH
> + command that 'git fetch' and 'git push' use. It can be set to
> + either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
> + value will be treated as normal ssh. This is useful in case
> + that Git gets this wrong.
> +
I do like the fact that this now sits under ssh.* hierarchy (not core.*).
It should mention core.sshCommand, GIT_SSH_COMMAND, etc., because
reading this alone would mislead users to set only this one and expect
that their plink will be used without setting core.sshCommand etc.
It also should say that GIT_SSH_VARIANT environment variable will
override this variable.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 4f208fab92..c322558aa7 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
> personal `.ssh/config` file. Please consult your ssh documentation
> for further details.
>
> +`GIT_SSH_VARIANT`::
> + If this environment variable is set, it overrides the autodetection
> + of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
> + push' use. It can be set to either `ssh`, `plink`, `putty` or
> + `tortoiseplink`. Any other value will be treated as normal ssh. This
> + is useful in case that Git gets this wrong.
Similarly this should mention GIT_SSH_COMMAND at least. It is crazy
to set something that will cause misdetection to core.sshCommand and
use this environment variable to fix it (instead of using ssh.variant),
so I think it is OK not to mention core.sshCommand (but it would not
hurt to do so).
> diff --git a/connect.c b/connect.c
> index 9f750eacb6..7b4437578b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
> return NULL;
> }
>
> +static int handle_ssh_variant(int *port_option, int *needs_batch)
> +{
> + const char *variant;
> +
> + if (!(variant = getenv("GIT_SSH_VARIANT")) &&
> + git_config_get_string_const("ssh.variant", &variant))
> + return 0;
> +
> + if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
> + *port_option = 'P';
> + else if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + }
> +
> + return 1;
> +}
Between handle and get I do not think there is strong reason to
favor one over the other. Unlike the one I sent yesterday, this is
not overriding but is getting, so we should keep the original name
Segev gave it, which is shorter, I would think, rather than renaming
it to "handle".
The string that came from the configuration must be freed, no? That
is what I recall in Peff's comment yesterday.
> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
> ssh_argv0 = xstrdup(ssh);
> }
>
> - if (ssh_argv0) {
> + if (!handle_ssh_variant(&port_option, &needs_batch) &&
> + ssh_argv0) {
I like the placement of this "if the user explicitly told us" much
better.
My patch yesterday was deliberately being lazy, because the next
thought that will come to us after comparing the two is "this way,
we avoid having to do the auto-detection altogether, which is way
better than wasting the effort to auto-detect, only to override".
And that reasoning will lead to a realization "there is no reason to
even do the split_cmdline() if the user explicitly told us". While
that reasoning is correct and we _should_ further refactor, I didn't
want to spend braincycles on that myself.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-01-26 19:27 ` Junio C Hamano
@ 2017-01-27 10:35 ` Johannes Schindelin
2017-01-27 18:17 ` Junio C Hamano
1 sibling, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-01-27 10:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
Hi Junio,
On Thu, 26 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/connect.c b/connect.c
> > index 9f750eacb6..7b4437578b 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
> > return NULL;
> > }
> >
> > +static int handle_ssh_variant(int *port_option, int *needs_batch)
> > +{
> > + const char *variant;
> > +
> > + if (!(variant = getenv("GIT_SSH_VARIANT")) &&
> > + git_config_get_string_const("ssh.variant", &variant))
> > + return 0;
> > +
> > + if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
> > + *port_option = 'P';
> > + else if (!strcmp(variant, "tortoiseplink")) {
> > + *port_option = 'P';
> > + *needs_batch = 1;
> > + }
> > +
> > + return 1;
> > +}
>
> Between handle and get I do not think there is strong reason to
> favor one over the other.
That is correct. "handle" and "get" are two very beautiful words, and none
of them deserves to take a back seat behind the other.
In this case, "get" is inappropriate, though, as the function does not
return the ssh variant, nor does it assign the ssh variant to any variable
to which any of its argument points.
Will try to find the time to address the other issues soon,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-01-26 19:27 ` Junio C Hamano
2017-01-27 10:35 ` Johannes Schindelin
@ 2017-01-27 18:17 ` Junio C Hamano
2017-02-01 12:01 ` Johannes Schindelin
1 sibling, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2017-01-27 18:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Just to save us extra round-trip.
Junio C Hamano <gitster@pobox.com> writes:
>> +`GIT_SSH_VARIANT`::
>> + If this environment variable is set, it overrides the autodetection
>> + of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
>> + push' use. It can be set to either `ssh`, `plink`, `putty` or
>> + `tortoiseplink`. Any other value will be treated as normal ssh. This
>> + is useful in case that Git gets this wrong.
>
> Similarly this should mention GIT_SSH_COMMAND at least. It is crazy
> to set something that will cause misdetection to core.sshCommand and
> use this environment variable to fix it (instead of using ssh.variant),
> so I think it is OK not to mention core.sshCommand (but it would not
> hurt to do so).
>
>> diff --git a/connect.c b/connect.c
>> index 9f750eacb6..7b4437578b 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
>> return NULL;
>> }
>>
>> +static int handle_ssh_variant(int *port_option, int *needs_batch)
>> +{
>> ...
>> +}
> ...
>
> The string that came from the configuration must be freed, no? That
> is what I recall in Peff's comment yesterday.
The leak needs plugging in some way.
Because this thing is merely an escape hatch that somebody who needs
it needs to use it always (as opposed to one-shot basis), we do not
need to support the environment variable and go with only the
configuration, which may make it easier to plug the leak.
>> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>> ssh_argv0 = xstrdup(ssh);
>> }
>>
>> - if (ssh_argv0) {
>> + if (!handle_ssh_variant(&port_option, &needs_batch) &&
>> + ssh_argv0) {
>
> I like the placement of this "if the user explicitly told us" much
> better.
> ...
> And that reasoning will lead to a realization "there is no reason to
> even do the split_cmdline() if the user explicitly told us". While
> that reasoning is correct and we _should_ further refactor, I didn't
> want to spend braincycles on that myself.
This _should_ above is a lot weaker than _must_.
IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
tokens before we realize that the user used the escape hatch and the
splitting was a wasted effort. This is exactly because this thing
is an escape hatch that is expected to be rarely used. Of course,
if the "wasted effort" can be eliminated without sacrificing the
simplicity of the code, that is fine as well.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-01-27 18:17 ` Junio C Hamano
@ 2017-02-01 12:01 ` Johannes Schindelin
2017-02-01 16:53 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
Hi Junio,
On Fri, 27 Jan 2017, Junio C Hamano wrote:
> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
> tokens before we realize that the user used the escape hatch and the
> splitting was a wasted effort. This is exactly because this thing
> is an escape hatch that is expected to be rarely used. Of course,
> if the "wasted effort" can be eliminated without sacrificing the
> simplicity of the code, that is fine as well.
Simplicity is retained. Battle-readiness was sacrificed on the way: the
new code is not tested well enough, and `next` will not help one bit.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 12:01 ` Johannes Schindelin
@ 2017-02-01 16:53 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Fri, 27 Jan 2017, Junio C Hamano wrote:
>
>> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
>> tokens before we realize that the user used the escape hatch and the
>> splitting was a wasted effort. This is exactly because this thing
>> is an escape hatch that is expected to be rarely used. Of course,
>> if the "wasted effort" can be eliminated without sacrificing the
>> simplicity of the code, that is fine as well.
>
> Simplicity is retained. Battle-readiness was sacrificed on the way: the
> new code is not tested well enough, and `next` will not help one bit.
Let me make it clear that there is no burning desire to sacrifice
battle-readiness in the above. If we expect that auto-detection
would be minority, then it makes sense to get the configured value
first and then spend cycles to split and guess only when detection
is needed.
In this case, because we expect that auto-detection will be used
most of the time, it is good enough to always split first, get the
configured value, and spend cycles to guess, or for that matter it
is perfectly fine to always split and guess first and then override
with the configured value.
If your attempt to optimize for a wrong case ended up causing new
unnecessary bugs, don't blame me.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
[not found] ` <cover.1485442231.git.johannes.schindelin@gmx.de>
` (2 preceding siblings ...)
2017-01-26 14:52 ` [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
@ 2017-02-01 11:57 ` Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 1/4] connect: handle putty/plink also " Johannes Schindelin
` (4 more replies)
3 siblings, 5 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 11:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.
For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.
This is error-prone and a bad user experience.
To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.
Changes relative to v2:
- touched up the documentation for ssh.variant to make it even easier to
understand
- free()d the config variable
- completely refactored the code to fulfil Junio's burning desire to
avoid split_cmdline() when unnecessary
It is quite preposterous to call this an "iteration" of the patch
series, because the code is so different now. I say this because I want
to caution that this code has not been tested as thoroughly, by far, as
the first iteration.
The primary purpose of code review is correctness, everything else is
either a consequence of it, or a means to make reviewing easier.
That means that I highly encourage those who pushed for these extensive
changes that make the patch series a lot less robust to balance things
out by at least *rudimentary* testing.
Johannes Schindelin (1):
git_connect(): factor out SSH variant handling
Junio C Hamano (1):
connect: rename tortoiseplink and putty variables
Segev Finer (2):
connect: handle putty/plink also in GIT_SSH_COMMAND
connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
Documentation/config.txt | 11 +++++++
Documentation/git.txt | 6 ++++
connect.c | 75 ++++++++++++++++++++++++++++++++++++------------
t/t5601-clone.sh | 41 ++++++++++++++++++++++++++
4 files changed, 114 insertions(+), 19 deletions(-)
base-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3
Interdiff vs v2:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f2c210f0a0..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
ssh.variant::
- Override the autodetection of plink/tortoiseplink in the SSH
- command that 'git fetch' and 'git push' use. It can be set to
- either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
- value will be treated as normal ssh. This is useful in case
- that Git gets this wrong.
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c322558aa7..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation
for further details.
`GIT_SSH_VARIANT`::
- If this environment variable is set, it overrides the autodetection
- of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
- push' use. It can be set to either `ssh`, `plink`, `putty` or
- `tortoiseplink`. Any other value will be treated as normal ssh. This
- is useful in case that Git gets this wrong.
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
diff --git a/connect.c b/connect.c
index 7b4437578b..7f1f802396 100644
--- a/connect.c
+++ b/connect.c
@@ -691,20 +691,45 @@ static const char *get_ssh_command(void)
return NULL;
}
-static int handle_ssh_variant(int *port_option, int *needs_batch)
+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
{
- const char *variant;
+ const char *variant = getenv("GIT_SSH_VARIANT");
+ char *p = NULL;
+
+ if (variant)
+ ; /* okay, fall through */
+ else if (!git_config_get_string("ssh.variant", &p))
+ variant = p;
+ else if (!is_cmdline) {
+ p = xstrdup(ssh_command);
+ variant = basename(p);
+ } else {
+ const char **ssh_argv;
- if (!(variant = getenv("GIT_SSH_VARIANT")) &&
- git_config_get_string_const("ssh.variant", &variant))
- return 0;
+ p = xstrdup(ssh_command);
+ if (split_cmdline(p, &ssh_argv)) {
+ variant = basename((char *)ssh_argv[0]);
+ /*
+ * At this point, variant points into the buffer
+ * referenced by p, hence we do not need ssh_argv
+ * any longer.
+ */
+ free(ssh_argv);
+ } else
+ return 0;
+ }
- if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+ if (!strcasecmp(variant, "plink") ||
+ !strcasecmp(variant, "plink.exe") ||
+ !strcasecmp(variant, "putty"))
*port_option = 'P';
- else if (!strcmp(variant, "tortoiseplink")) {
+ else if (!strcasecmp(variant, "tortoiseplink") ||
+ !strcasecmp(variant, "tortoiseplink.exe")) {
*port_option = 'P';
*needs_batch = 1;
}
+ free(p);
return 1;
}
@@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url,
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
- char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (ssh) {
- char *split_ssh = xstrdup(ssh);
- const char **ssh_argv;
-
- if (split_cmdline(split_ssh, &ssh_argv))
- ssh_argv0 = xstrdup(ssh_argv[0]);
- free(split_ssh);
- free((void *)ssh_argv);
- } else {
+ if (ssh)
+ handle_ssh_variant(ssh, 1, &port_option,
+ &needs_batch);
+ else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh = getenv("GIT_SSH");
if (!ssh)
ssh = "ssh";
-
- ssh_argv0 = xstrdup(ssh);
+ else
+ handle_ssh_variant(ssh, 0,
+ &port_option,
+ &needs_batch);
}
- if (!handle_ssh_variant(&port_option, &needs_batch) &&
- ssh_argv0) {
- const char *base = basename(ssh_argv0);
-
- 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);
-
argv_array_push(&conn->args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(&conn->args, "-4");
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/4] connect: handle putty/plink also in GIT_SSH_COMMAND
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
@ 2017-02-01 12:01 ` Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 2/4] connect: rename tortoiseplink and putty variables Johannes Schindelin
` (3 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
From: Segev Finer <segev208@gmail.com>
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 <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
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(&ssh_host, &port);
@@ -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, &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(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 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 GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/4] connect: rename tortoiseplink and putty variables
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 1/4] connect: handle putty/plink also " Johannes Schindelin
@ 2017-02-01 12:01 ` Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 3/4] git_connect(): factor out SSH variant handling Johannes Schindelin
` (2 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Junio C Hamano, Jeff King
From: Junio C Hamano <gitster@pobox.com>
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 <port>" when OpenSSH would use "-p <port>",
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.
[jes: wrapped overly-long line]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 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,11 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
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_pushf(&conn->args,
+ "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] git_connect(): factor out SSH variant handling
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 1/4] connect: handle putty/plink also " Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 2/4] connect: rename tortoiseplink and putty variables Johannes Schindelin
@ 2017-02-01 12:01 ` Johannes Schindelin
2017-02-01 12:01 ` [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
2017-02-01 20:07 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Junio C Hamano
4 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
We handle plink and tortoiseplink as OpenSSH replacements, by passing
the correct command-line options when detecting that they are used.
To let users override that auto-detection (in case Git gets it wrong),
we need to introduce new code to that end.
In preparation for this code, let's factor out the SSH variant handling
into its own function, handle_ssh_variant().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/connect.c b/connect.c
index 9f750eacb6..2734b9a1ca 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,44 @@ static const char *get_ssh_command(void)
return NULL;
}
+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
+{
+ const char *variant;
+ char *p = NULL;
+
+ if (!is_cmdline) {
+ p = xstrdup(ssh_command);
+ variant = basename(p);
+ } else {
+ const char **ssh_argv;
+
+ p = xstrdup(ssh_command);
+ if (split_cmdline(p, &ssh_argv)) {
+ variant = basename((char *)ssh_argv[0]);
+ /*
+ * At this point, variant points into the buffer
+ * referenced by p, hence we do not need ssh_argv
+ * any longer.
+ */
+ free(ssh_argv);
+ } else
+ return 0;
+ }
+
+ if (!strcasecmp(variant, "plink") ||
+ !strcasecmp(variant, "plink.exe"))
+ *port_option = 'P';
+ else if (!strcasecmp(variant, "tortoiseplink") ||
+ !strcasecmp(variant, "tortoiseplink.exe")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ }
+ free(p);
+
+ return 1;
+}
+
/*
* 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,
@@ -773,7 +811,6 @@ struct child_process *git_connect(int fd[2], const char *url,
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
- char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -794,15 +831,10 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (ssh) {
- char *split_ssh = xstrdup(ssh);
- const char **ssh_argv;
-
- if (split_cmdline(split_ssh, &ssh_argv))
- ssh_argv0 = xstrdup(ssh_argv[0]);
- free(split_ssh);
- free((void *)ssh_argv);
- } else {
+ if (ssh)
+ handle_ssh_variant(ssh, 1, &port_option,
+ &needs_batch);
+ else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -813,22 +845,10 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh = getenv("GIT_SSH");
if (!ssh)
ssh = "ssh";
-
- ssh_argv0 = xstrdup(ssh);
- }
-
- if (ssh_argv0) {
- const char *base = basename(ssh_argv0);
-
- 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);
+ else
+ handle_ssh_variant(ssh, 0,
+ &port_option,
+ &needs_batch);
}
argv_array_push(&conn->args, ssh);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
` (2 preceding siblings ...)
2017-02-01 12:01 ` [PATCH v3 3/4] git_connect(): factor out SSH variant handling Johannes Schindelin
@ 2017-02-01 12:01 ` Johannes Schindelin
2017-02-01 19:19 ` Junio C Hamano
2017-02-01 20:07 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Junio C Hamano
4 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
From: Segev Finer <segev208@gmail.com>
This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.
[jes: wrapped overly-long lines, factored out and changed
get_ssh_variant() to handle_ssh_variant() to accomodate the
change from the putty/tortoiseplink variables to
port_option/needs_batch, adjusted the documentation, free()d
value obtained from the config.]
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/config.txt | 11 +++++++++++
Documentation/git.txt | 6 ++++++
connect.c | 11 ++++++++---
t/t5601-clone.sh | 26 ++++++++++++++++++++++++++
4 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,17 @@ Environment variable settings always override any matches. The URLs that are
matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
+ssh.variant::
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
+
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,12 @@ Usually it is easier to configure any desired options through your
personal `.ssh/config` file. Please consult your ssh documentation
for further details.
+`GIT_SSH_VARIANT`::
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
+
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 2734b9a1ca..7f1f802396 100644
--- a/connect.c
+++ b/connect.c
@@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
int *port_option, int *needs_batch)
{
- const char *variant;
+ const char *variant = getenv("GIT_SSH_VARIANT");
char *p = NULL;
- if (!is_cmdline) {
+ if (variant)
+ ; /* okay, fall through */
+ else if (!git_config_get_string("ssh.variant", &p))
+ variant = p;
+ else if (!is_cmdline) {
p = xstrdup(ssh_command);
variant = basename(p);
} else {
@@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
}
if (!strcasecmp(variant, "plink") ||
- !strcasecmp(variant, "plink.exe"))
+ !strcasecmp(variant, "plink.exe") ||
+ !strcasecmp(variant, "putty"))
*port_option = 'P';
else if (!strcasecmp(variant, "tortoiseplink") ||
!strcasecmp(variant, "tortoiseplink.exe")) {
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
'
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ GIT_SSH_VARIANT=ssh \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ git -c ssh.variant=ssh \
+ clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+ GIT_SSH_VARIANT=plink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+ expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+ GIT_SSH_VARIANT=tortoiseplink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+ expect_ssh "-batch -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 12:01 ` [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
@ 2017-02-01 19:19 ` Junio C Hamano
2017-02-01 19:46 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 19:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> index 2734b9a1ca..7f1f802396 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
> static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> int *port_option, int *needs_batch)
> {
> - const char *variant;
> + const char *variant = getenv("GIT_SSH_VARIANT");
> char *p = NULL;
>
> - if (!is_cmdline) {
> + if (variant)
> + ; /* okay, fall through */
> + else if (!git_config_get_string("ssh.variant", &p))
> + variant = p;
> + else if (!is_cmdline) {
> p = xstrdup(ssh_command);
> variant = basename(p);
> } else {
> @@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> }
>
> if (!strcasecmp(variant, "plink") ||
> - !strcasecmp(variant, "plink.exe"))
> + !strcasecmp(variant, "plink.exe") ||
> + !strcasecmp(variant, "putty"))
This means that "putty" that appear as the first word of the command
line, not the value configured for ssh.variant or GIT_SSH_VARIANT,
will also trigger the option for "plink", no?
Worse, "plink.exe" configured as the value of "ssh.variant", is
anything other than 'ssh', 'plink', 'putty', or 'tortoiseplink', but
it is not treated as normal ssh, contrary to the documentation.
> +ssh.variant::
> + Depending on the value of the environment variables `GIT_SSH` or
> + `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
> + auto-detects whether to adjust its command-line parameters for use
> + with plink or tortoiseplink, as opposed to the default (OpenSSH).
> ++
> +The config variable `ssh.variant` can be set to override this auto-detection;
> +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
> +will be treated as normal ssh. This setting can be overridden via the
> +environment variable `GIT_SSH_VARIANT`.
I was hoping that the "rewrite that retains simplicity" was also
correct, but let's not go in this direction. The more lines you
touch in a hurry, the worse the code will get, and that is to be
expected.
I'd suggest taking the documentation updates from this series, and
then minimally plug the leak introduced by the previous round,
perhaps like the attached SQUASH. As I said, GIT_SSH_VARIANT and
ssh.variant are expected to be rare cases, and the case in which
they are set does not have to be optimized if it makes the necessary
change smaller and more likely to be correct.
I think restructuring along the line of 3/4 of this round is very
sensible in the longer term (if anything, handle_ssh_variant() now
really handles ssh variants in all cases, which makes it worthy of
its name, as opposed to the one in the previous round that only
reacts to the overrides). But it seems that it will take longer to
get such a rewrite right, and my priority is seeing this topic to
add autodetection to GIT_SSH_COMMAND and other smaller topics in the
upcoming -rc0 in a serviceable and correct shape.
The restructuring done by 3/4 can come later after the dust settles,
if somebody cares deeply enough about performance in the rare cases.
Documentation/config.txt | 14 +++++++++-----
Documentation/git.txt | 9 ++++-----
connect.c | 9 +++++----
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f2c210f0a0..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
ssh.variant::
- Override the autodetection of plink/tortoiseplink in the SSH
- command that 'git fetch' and 'git push' use. It can be set to
- either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
- value will be treated as normal ssh. This is useful in case
- that Git gets this wrong.
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c322558aa7..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation
for further details.
`GIT_SSH_VARIANT`::
- If this environment variable is set, it overrides the autodetection
- of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
- push' use. It can be set to either `ssh`, `plink`, `putty` or
- `tortoiseplink`. Any other value will be treated as normal ssh. This
- is useful in case that Git gets this wrong.
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
diff --git a/connect.c b/connect.c
index 7b4437578b..da51fef9ee 100644
--- a/connect.c
+++ b/connect.c
@@ -693,10 +693,11 @@ static const char *get_ssh_command(void)
static int handle_ssh_variant(int *port_option, int *needs_batch)
{
- const char *variant;
+ char *variant;
- if (!(variant = getenv("GIT_SSH_VARIANT")) &&
- git_config_get_string_const("ssh.variant", &variant))
+ variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+ if (!variant &&
+ git_config_get_string("ssh.variant", &variant))
return 0;
if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
@@ -705,7 +706,7 @@ static int handle_ssh_variant(int *port_option, int *needs_batch)
*port_option = 'P';
*needs_batch = 1;
}
-
+ free(variant);
return 1;
}
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 19:19 ` Junio C Hamano
@ 2017-02-01 19:46 ` Junio C Hamano
2017-02-01 22:24 ` Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 19:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Junio C Hamano <gitster@pobox.com> writes:
> I think restructuring along the line of 3/4 of this round is very
> sensible in the longer term (if anything, handle_ssh_variant() now
> really handles ssh variants in all cases, which makes it worthy of
> its name, as opposed to the one in the previous round that only
> reacts to the overrides). But it seems that it will take longer to
> get such a rewrite right, and my priority is seeing this topic to
> add autodetection to GIT_SSH_COMMAND and other smaller topics in the
> upcoming -rc0 in a serviceable and correct shape.
>
> The restructuring done by 3/4 can come later after the dust settles,
> if somebody cares deeply enough about performance in the rare cases.
Having said all that, because I like the patch 3/4 so much, here is
another way to fix this, and I think (of course I am biased, but...)
the result is easier to grok. Not only it makes it clear that the
namespace for the actual command names on the command line and the
namespace for the supported values of the configuration are distinct,
it makes it clear that we do not do anything extra when the user
explicitly tells us which variant to use.
This is designed to be squashed into [4/4] of this latest round (as
opposed to the one in the message I am responding to, which is to be
squashed into the previous round).
connect.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/connect.c b/connect.c
index 7f1f802396..12ad8d4c8b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
return NULL;
}
-static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
- int *port_option, int *needs_batch)
+static int override_ssh_variant(int *port_option, int *needs_batch)
{
- const char *variant = getenv("GIT_SSH_VARIANT");
+ char *variant;
+
+ variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+ if (!variant &&
+ git_config_get_string("ssh.variant", &variant))
+ return 0;
+
+ if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
+ *port_option = 'P';
+ *needs_batch = 0;
+ } else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ } else {
+ *port_option = 'p';
+ *needs_batch = 0;
+ }
+ free(variant);
+ return 1;
+}
+
+static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
+{
+ const char *variant;
char *p = NULL;
- if (variant)
- ; /* okay, fall through */
- else if (!git_config_get_string("ssh.variant", &p))
- variant = p;
- else if (!is_cmdline) {
+ if (override_ssh_variant(port_option, needs_batch))
+ return;
+
+ if (!is_cmdline) {
p = xstrdup(ssh_command);
variant = basename(p);
} else {
@@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*/
free(ssh_argv);
} else
- return 0;
+ return;
}
if (!strcasecmp(variant, "plink") ||
@@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*needs_batch = 1;
}
free(p);
-
- return 1;
}
/*
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 19:46 ` Junio C Hamano
@ 2017-02-01 22:24 ` Johannes Schindelin
2017-02-01 22:33 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 22:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I think restructuring along the line of 3/4 of this round is very
> > sensible in the longer term (if anything, handle_ssh_variant() now
> > really handles ssh variants in all cases, which makes it worthy of
> > its name, as opposed to the one in the previous round that only
> > reacts to the overrides). But it seems that it will take longer to
> > get such a rewrite right, and my priority is seeing this topic to
> > add autodetection to GIT_SSH_COMMAND and other smaller topics in the
> > upcoming -rc0 in a serviceable and correct shape.
> >
> > The restructuring done by 3/4 can come later after the dust settles,
> > if somebody cares deeply enough about performance in the rare cases.
>
> Having said all that, because I like the patch 3/4 so much, here is
> another way to fix this, and I think (of course I am biased, but...)
> the result is easier to grok. Not only it makes it clear that the
> namespace for the actual command names on the command line and the
> namespace for the supported values of the configuration are distinct,
> it makes it clear that we do not do anything extra when the user
> explicitly tells us which variant to use.
>
> This is designed to be squashed into [4/4] of this latest round (as
> opposed to the one in the message I am responding to, which is to be
> squashed into the previous round).
>
> connect.c | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 7f1f802396..12ad8d4c8b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
> return NULL;
> }
>
> -static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> - int *port_option, int *needs_batch)
> +static int override_ssh_variant(int *port_option, int *needs_batch)
> {
> - const char *variant = getenv("GIT_SSH_VARIANT");
> + char *variant;
> +
> + variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
> + if (!variant &&
> + git_config_get_string("ssh.variant", &variant))
> + return 0;
> +
> + if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
> + *port_option = 'P';
> + *needs_batch = 0;
> + } else if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + } else {
> + *port_option = 'p';
> + *needs_batch = 0;
> + }
> + free(variant);
> + return 1;
> +}
> +
> +static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
> + int *port_option, int *needs_batch)
> +{
> + const char *variant;
> char *p = NULL;
>
> - if (variant)
> - ; /* okay, fall through */
> - else if (!git_config_get_string("ssh.variant", &p))
> - variant = p;
> - else if (!is_cmdline) {
> + if (override_ssh_variant(port_option, needs_batch))
> + return;
> +
> + if (!is_cmdline) {
> p = xstrdup(ssh_command);
> variant = basename(p);
> } else {
> @@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> */
> free(ssh_argv);
> } else
> - return 0;
> + return;
> }
>
> if (!strcasecmp(variant, "plink") ||
> @@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> *needs_batch = 1;
> }
> free(p);
> -
> - return 1;
> }
>
> /*
That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
not your specific objection that that is the case?
No worries, I will let this simmer for a while. Your fixup has a lot of
duplicated code (so much for maintainability as an important goal... ;-))
and I will have to think about it. My immediate thinking is to *not*
duplicate code, strip the .exe extension directly after calling basename()
in the cases where we handle commands, and handle "putty" in the other
cases by replacing it with the string "plink".
But as I said, this will simmer for a while.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 22:24 ` Johannes Schindelin
@ 2017-02-01 22:33 ` Junio C Hamano
2017-02-01 22:42 ` Johannes Schindelin
2017-02-01 22:43 ` Junio C Hamano
0 siblings, 2 replies; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 22:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> not your specific objection that that is the case?
Yup, you can remove that while you reroll.
> No worries, I will let this simmer for a while. Your fixup has a lot of
> duplicated code (so much for maintainability as an important goal... ;-))
> and I will have to think about it. My immediate thinking is to *not*
> duplicate code,...
You need to realize that the namespaces of the configuration and the
command names are distinct. There is no code duplication.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 22:33 ` Junio C Hamano
@ 2017-02-01 22:42 ` Johannes Schindelin
2017-02-01 22:43 ` Junio C Hamano
1 sibling, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 22:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> > not your specific objection that that is the case?
>
> Yup, you can remove that while you reroll.
>
> > No worries, I will let this simmer for a while. Your fixup has a lot of
> > duplicated code (so much for maintainability as an important goal... ;-))
> > and I will have to think about it. My immediate thinking is to *not*
> > duplicate code,...
>
> You need to realize that the namespaces of the configuration and the
> command names are distinct. There is no code duplication.
Since your 2/4 did away with the "plink" and "tortoiseplink" values in
favor of "port_option" and "batch_option", there is a duplication of logic
which I tried to undo in v3 and which you reintroduce in your fixup.
Maybe that refactoring was not so smart to begin with, and we should have
instead modified the code to use an enum instead and keep the original
conditionals instead of setting a port_option and a batch_option
explicitly.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 22:33 ` Junio C Hamano
2017-02-01 22:42 ` Johannes Schindelin
@ 2017-02-01 22:43 ` Junio C Hamano
2017-02-01 23:07 ` Johannes Schindelin
1 sibling, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 22:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
>> not your specific objection that that is the case?
>
> Yup, you can remove that while you reroll.
>
>> No worries, I will let this simmer for a while. Your fixup has a lot of
>> duplicated code (so much for maintainability as an important goal... ;-))
>> and I will have to think about it. My immediate thinking is to *not*
>> duplicate code,...
>
> You need to realize that the namespaces of the configuration and the
> command names are distinct. There is no code duplication.
To explain this a bit, there is no reason why allowed values for
SSH_VARIANT must be "putty" and "tortoiseplink". An alternative
design could be "port_option=-p,needs_batch=yes" and it may be more
logical and futureproof if a variant of tortoiseplink decides to use
"-p" instead of "-P" and still require "-batch".
Prematurely attempting to share code, only because the current
vocabularies for two distinct concepts happen to overlap, is not
de-duplicating the code for maintainability. It is adding
unnecessary work other people need to do in the future when they
want to extend the system.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
2017-02-01 22:43 ` Junio C Hamano
@ 2017-02-01 23:07 ` Johannes Schindelin
0 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> >> not your specific objection that that is the case?
> >
> > Yup, you can remove that while you reroll.
> >
> >> No worries, I will let this simmer for a while. Your fixup has a lot of
> >> duplicated code (so much for maintainability as an important goal... ;-))
> >> and I will have to think about it. My immediate thinking is to *not*
> >> duplicate code,...
> >
> > You need to realize that the namespaces of the configuration and the
> > command names are distinct. There is no code duplication.
>
> To explain this a bit, there is no reason why allowed values for
> SSH_VARIANT must be "putty" and "tortoiseplink". An alternative
> design could be "port_option=-p,needs_batch=yes" and it may be more
> logical and futureproof if a variant of tortoiseplink decides to use
> "-p" instead of "-P" and still require "-batch".
>
> Prematurely attempting to share code, only because the current
> vocabularies for two distinct concepts happen to overlap, is not
> de-duplicating the code for maintainability. It is adding
> unnecessary work other people need to do in the future when they
> want to extend the system.
Except, of course, that your hypothetical port_option and needs_batch
settings would be handled at a different point altogether.
I sense very strongly that this discussion has taken a very emotional
turn, which is detrimental to the quality. So let's take a break here.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
2017-02-01 11:57 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Johannes Schindelin
` (3 preceding siblings ...)
2017-02-01 12:01 ` [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Johannes Schindelin
@ 2017-02-01 20:07 ` Junio C Hamano
2017-02-01 22:17 ` Johannes Schindelin
4 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 20:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> It is quite preposterous to call this an "iteration" of the patch
> series, because the code is so different now. I say this because I want
> to caution that this code has not been tested as thoroughly, by far, as
> the first iteration.
>
> The primary purpose of code review is correctness, everything else is
> either a consequence of it, or a means to make reviewing easier.
You are utterly wrong here.
The primary purpose of code review is to spot and correct the
problems the submitter has missed. The problems can span from
stupid bugs that affect correctness to maintainability, to design
mistakes, to clarity of explanation for both end users and future
developers.
Among them, correctness problems are, as long as the problem to be
solved is specified clearly enough, the easiest to spot by the
submitter before the patch is sent out. The submitter is focused on
solving one problem, and if the updated code does not even work as
the submitter advertises it would, that can be caught by the
submitter before the patch is even sent out.
Of course, humans are not perfect, and catching correctness problems
is important, but that is not the only (let alone primary) thing.
When a submitter has been focusing on solving one problem, it is
easy to lose the larger picture and to end up adding something that
may be "correct" (in the sense of "works as advertised by the
submitter") but does not fit well with the rest of the system, or
covers some use cases but misses other important and related use
cases. If the "does not fit well" surfaces to the end user level,
that would become a design problem. If it affects the future
developers, that would become a maintainability problem.
Compared to the correctness issue, these are much harder to spot by
the submitter alone, who focused so intensely to get his code
"correct". The review process is of greater value to spot these
issues.
I've already read and commented on the series; as I said, I think
the rewrite in 3/4 makes the resulting code much easier to read, and
with the fix-up I just sent out, I think the end result is correct,
works as advertised, the way it is advertised is clear to end users,
and is maintainable.
But I do share your uneasiness about the "new-ness" of the code.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
2017-02-01 20:07 ` [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND Junio C Hamano
@ 2017-02-01 22:17 ` Johannes Schindelin
2017-02-01 22:55 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2017-02-01 22:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > It is quite preposterous to call this an "iteration" of the patch
> > series, because the code is so different now. I say this because I want
> > to caution that this code has not been tested as thoroughly, by far, as
> > the first iteration.
> >
> > The primary purpose of code review is correctness, everything else is
> > either a consequence of it, or a means to make reviewing easier.
>
> You are utterly wrong here.
>
> The primary purpose of code review is to spot and correct the
> problems the submitter has missed. The problems can span from
> stupid bugs that affect correctness to maintainability, to design
> mistakes, to clarity of explanation for both end users and future
> developers.
>
> Among them, correctness problems are, as long as the problem to be
> solved is specified clearly enough, the easiest to spot by the
> submitter before the patch is sent out. The submitter is focused on
> solving one problem, and if the updated code does not even work as
> the submitter advertises it would, that can be caught by the
> submitter before the patch is even sent out.
>
> Of course, humans are not perfect, and catching correctness problems
> is important, but that is not the only (let alone primary) thing.
>
> When a submitter has been focusing on solving one problem, it is
> easy to lose the larger picture and to end up adding something that
> may be "correct" (in the sense of "works as advertised by the
> submitter") but does not fit well with the rest of the system, or
> covers some use cases but misses other important and related use
> cases. If the "does not fit well" surfaces to the end user level,
> that would become a design problem. If it affects the future
> developers, that would become a maintainability problem.
>
> Compared to the correctness issue, these are much harder to spot by
> the submitter alone, who focused so intensely to get his code
> "correct". The review process is of greater value to spot these
> issues.
We will never agree on this.
From my perspective, design, explanation and maintainability are a
consequence of making it easy for reviewers to spot where the code is
incorrect.
And correctness is not covered by "the submitter tested this". Correctness
includes all the corner cases, where the "many eyes make bugs shallow"
really shines.
I'd rather have reviewers find bugs than users.
I will *never* be a fan of a review process that pushes correctness to a
back seat (yes, it is much harder than spotting typos or lines longer than
80 columns per row, but the ultimate goal is to deliver value to the end
user, not to make life easy for the maintainer).
Ciao,
Johannes
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
2017-02-01 22:17 ` Johannes Schindelin
@ 2017-02-01 22:55 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2017-02-01 22:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Compared to the correctness issue, these are much harder to spot by
>> the submitter alone, who focused so intensely to get his code
>> "correct". The review process is of greater value to spot these
>> issues.
>
> We will never agree on this.
That's too bad.
> From my perspective, design, explanation and maintainability are a
> consequence of making it easy for reviewers to spot where the code is
> incorrect.
>
> And correctness is not covered by "the submitter tested this". Correctness
> includes all the corner cases, where the "many eyes make bugs shallow"
> really shines.
>
> I'd rather have reviewers find bugs than users.
I'd rather have submitters find bugs than reviewers.
> I will *never* be a fan of a review process that pushes correctness to a
> back seat (yes, it is much harder than spotting typos or lines longer than
> 80 columns per row, but the ultimate goal is to deliver value to the end
> user, not to make life easy for the maintainer).
Did I ever say correctness is pushed to a back seat?
I said that it is easier to spot correctness issues for you as a
submitter than other kinds of issues without outside help (and
implied that if you are a diligent contributor, you should aim for,
and you should be able to achieve, a patch series where correctness
issues do not need to be pointed out). But other higher level
issues are harder for any submitter to spot (regardless of
experience and competence of the submitter), because one gets so
married to one's own code, design and worldview. And that is why
"review is primarily to spot bugs" can never be a correct viewpoint.
A reviewee needs to be prepared to accept review comments on higher
level issues, even more readily than comments on correctness issues,
because it is too easy to be constrained by early decisions one has
already made while preparing a patch series and become blind to
bigger picture after staring one's own new code for number of hours.
Higher level issues can be more easily spotted by reviewers, whose
eyes are still fresh to the series.
^ permalink raw reply [flat|nested] 38+ messages in thread