Re: [PATCH] git_connect: clarify conn->use_shell flag
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 8, 2015, 17:18 UTC
- Message-ID
- <xmqqwpw096dq.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20150908083314.GB2991@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 25 quoted lines
> Subject: [PATCH] git_connect: clarify conn->use_shell flag
>
> When executing user-specified programs, we generally always
> want to use a shell, for flexibility and consistency. One
> big exception is executing $GIT_SSH, which for historical
> reasons must not use a shell.
>
> Once upon a time the logic in git_connect looked like:
>
> if (protocol == PROTO_SSH) {
> ... setup ssh ...
> } else {
> ... setup local connection ...
> conn->use_shell = 1;
> }
>
> But over time the PROTO_SSH block has grown, and the "local"
> block has shrunk so that it contains only conn->use_shell;
> it's easy to miss at the end of the large block. Moreover,
> PROTO_SSH now also sometimes sets use_shell, when the new
> GIT_SSH_COMMAND is used.
>
> To make the flow easier to follow, let's just set
> conn->use_shell when we're setting up the "conn" struct, and
> unset it (with a comment) in the historical GIT_SSH case.Makes perfect sense. I think another thing that falls into the same class wrt readability is 'putty'; if the code does putty=0 at the beginning of "various flavours of SSH", and sets it only when it checks with "various flavours of plink" when GIT_SSH_COMMAND is not set, the logic would be even clearer, I suspect.
Show 5 quoted lines
> Note that for clarity we leave "use_shell" on in the case > that we fall back to our default "ssh" This looks like a > behavior change, but in practice run-command.c optimizes > shell invocations without metacharacters into a straight > execve anyway.
Hmm, interesting. I am not sure if that has to be the way, though. Wouldn't the resulting code become simpler if you do not do that?
That is, is is what I have in mind on top of your patch. Did I break something?
connect.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/connect.c b/connect.c index 6f3f85e..acd39d7 100644 --- a/connect.c +++ b/connect.c @@ -727,7 +727,7 @@ 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, tortoiseplink = 0; + int putty = 0, tortoiseplink = 0; char *ssh_host = hostandport; const char *port = NULL; get_host_and_port(&ssh_host, &port); @@ -749,9 +749,7 @@ struct child_process *git_connect(int fd[2], const char *url, } ssh = getenv("GIT_SSH_COMMAND"); - if (ssh) - putty = 0; - else { + if (!ssh) { const char *base; char *ssh_dup; @@ -760,10 +758,10 @@ struct child_process *git_connect(int fd[2], const char *url, * GIT_SSH_COMMAND (and must remain so for * historical compatibility). */ + conn->use_shell = 0; + ssh = getenv("GIT_SSH"); - if (ssh) - conn->use_shell = 0; - else + if (!ssh) ssh = "ssh"; ssh_dup = xstrdup(ssh); @@ -771,8 +769,9 @@ struct child_process *git_connect(int fd[2], const char *url, tortoiseplink = !strcasecmp(base, "tortoiseplink") || !strcasecmp(base, "tortoiseplink.exe"); - putty = !strcasecmp(base, "plink") || - !strcasecmp(base, "plink.exe") || tortoiseplink; + putty = tortoiseplink || + !strcasecmp(base, "plink") || + !strcasecmp(base, "plink.exe"); free(ssh_dup); }