Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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