Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano <hidden>
Date: 2017-02-01 19:46:49
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2017-02-01 · Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config · Johannes Schindelin <hidden>
- 2017-02-01 · Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config · Junio C Hamano <hidden>
- 2017-02-01 · Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config · Johannes Schindelin <hidden>
- 2017-02-01 · Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config · Junio C Hamano <hidden>
- 2017-02-01 · Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] 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; } /*