Jeff King <peff@xxxxxxxx> writes:
> 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.
> 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);
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html