I think that is a very sensible approach, but just like we have a few
"built-in" function-header regexps with customization possibilities for
the user, we might want to:

 * Have that "-x", "-T" in the command line we generate for OpenSSH;

 * Allow users to specify OpenSSH substitute via a configuration and/or
   environment variable, and have them use your script; and

 * Have a built-in logic for selected and common "OpenSSH substitute",
   e.g. plink.

There is no reason to make users suffer an extra redirection for common
enough alternatives.

Here is to get it started...

 connect.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 574f42f..c72dd9e 100644
--- a/connect.c
+++ b/connect.c
@@ -599,12 +599,36 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	conn->argv = arg = xcalloc(6, sizeof(*arg));
 	if (protocol == PROTO_SSH) {
 		const char *ssh = getenv("GIT_SSH");
+		const char *ssh_basename;
 		if (!ssh) ssh = "ssh";
 
+		ssh_basename = strrchr(ssh, '/');
+		ssh_basename = ssh_basename ? (ssh_basename + 1) : ssh;
+
 		*arg++ = ssh;
-		if (port) {
-			*arg++ = "-p";
-			*arg++ = port;
+		/*
+		 * Make sure to enlarge conn->argv if you add more
+		 * paremeters here.
+		 *
+		 * We know how to invoke a few ssh implementations
+		 * ourselves.
+		 */
+		if (!strcmp(ssh_basename, "plink")) {
+			if (port) {
+				*arg++ = "-P";
+				*arg++ = port;
+			}
+		} else {
+			/*
+			 * This is for stock OpenSSH, but you can have
+			 * your custom wrapper script to parse this
+			 * and invoke other ssh implementations after
+			 * rearranging parameters as well.
+			 */
+			if (port) {
+				*arg++ = "-p";
+				*arg++ = port;
+			}
 		}
 		*arg++ = host;
 	}

--

[ message continues ]

Hi,


How about this instead?

-- snipsnap --
diff --git a/connect.c b/connect.c
index 574f42f..7e7f4d3 100644
--- a/connect.c
+++ b/connect.c
@@ -603,7 +603,8 @@ struct child_process *git_connect(int fd[2], const char *url
 
 		*arg++ = ssh;
 		if (port) {
-			*arg++ = "-p";
+			const char *opt = getenv("GIT_SSH_PORT_OPTION");
+			*arg++ = opt ? opt : "-p";
 			*arg++ = port;
 		}
 		*arg++ = host;

--

[ message continues ]

If you only care only about the ones we currently want to support, I do
not htink it makes any difference either way, but if we are shooting for
having a minimum-but-reasonable framework to make it easy to support other
ones that we haven't seen, it feels very much like an inadequate hack to
waste an envirnoment variable for such a narrow special case.  With this,
what you really mean is "Plink uses -P instead of -p", right?

I do not know if "plink" is used widely enough to be special cased, but if
so, I think we would better have an explicit support for it.  Will we add
GIT_SSH_FORBID_X11_FORWARDING_OPTION environment variable and friends,
too?

The extra environment would not help dealing with an implementation that
wants --port=90222 (i.e. not as two separate arguments but a single one),
for example.  You would need the extra wrapper support for that kind of
thing anyway.  That extra environment _solution_ will need to make an
assuption that any reasonable implementation would have an option string
to specify port which may not be "-p" and that is to be followed by a
separate argument that is a decimal port number, which probably is
reasonable for this particular "port" thing, but as a general design
principle I do not think it is a good direction to go.

--

[ message continues ]

Hi,


Yeah.  My first attempt was to allow "GIT_SSH='plink.exe -P %p %h'" to 
work, and for that matter, "git config --global transport.ssh 'plink.exe 
-P %p %h'", but I decided that it would be easier to do the patch I 
posted.

Anyway, I think that this issue wasted enough of my time, as I will never 
use plink anyway.  As long as the patch does not have an adverse effect on 
my use case, which happens to be the default case, I will just not bother 
anymore, even if I think that GIT_SSH=wrapper would be better than special 
case rarely exercized ssh programs in the source code.

Ciao,
Dscho

--

[ message continues ]

Our installer on Windows explicitly supports plink as an alternative to
OpenSSH.  Putty has a GUI for managing your ssh keys (Pageant).  You  
need
to type your password only once to unlock a key and make it available to
all connections that you start afterwards.

I think it should be special cased.  I use plink myself.

	Steffen
--

[ message continues ]

I am slightly negative on this, because we are setting OpenSSH
preferences behind the user's back that they would not normally expect
git to be tampering with.

I think the expectation for this is that it impacts only the ssh session
used by git.  But because OpenSSH supports the concept of "master" and
"slave" sessions (i.e., it can multiplex many sessions over a single ssh
session, avoiding authentication and thus reducing latency until the
start of the session), what you do in one session can impact other
sessions. In particular, if the 'master' does not have x11 forwarding
(because it happens to be started by git), then slave connections do not
get it. So a user with X11Forwarding and ControlMaster set in his config
would usually have everything work, but bad timing with the
git-initiated session as the master would unexpectedly break his
X11Forwarding for other sessions.

I don't know how commonly the ControlMaster option for openssh is used.
I also don't know if this should simply be considered a bug in openssh,
since it silently ignores the request for X forwarding.  Personally, I
will not be affected because I don't do X forwarding by default, anyway.
But I thought I would raise the point.

-Peff
--

[ message continues ]

I'm not sure the ControlMaster option is still followed when using -T. 
Also, IIRC, ControlMaster doesn't exit until slave connections are
done, so git ssh sessions granted the master control would stall until
then if they happen to have slaves launched. i.e. It can *already* have
bad side effects.

Adding '-S none' would ensure ControlMaster would not take effect; on
the other hand, it would not allow git's ssh connection to be a slave
either. '-o ControlMaster no' could be a solution.

All these need to be tested, obviously.

Mike
--

[ message continues ]

Yes, that is a problem (and IMHO a weakness in the implementation, but

I think that is definitely a mistake; git is one of the main reasons I

That is actually quite sensible, and would make this a non-issue, as

I tested, and doing "ssh -Tx -o 'ControlMaster no'" does the right thing
(reuse existing session if possible, create a new one with -Tx
otherwise, and never create a control socket for slaves).

-Peff
--

[ message continues ]