Re: [PATCH v2] Ensure that SSH runs in non-interactive mode

From: Fredrik Tolf <fredrik@dolda2000.com>
Date: Jul 21, 2008, 01:44 UTC
Message-ID: <1216604693.3673.20.camel@pc7.dolda2000.com>
In-Reply-To: <alpine.DEB.1.00.0807210310330.3305@eeepc-johanness>

On Mon, 2008-07-21 at 03:15 +0200, Johannes Schindelin wrote:

Show 11 quoted lines

> Hi,
> 
> On Mon, 21 Jul 2008, Fredrik Tolf wrote:
> 
> > I'm following my previous SSH patch up with this one, which should at
> > least solve the problems discussed, and probably some more. If anything,
> > it might be considered a bit overkill for the problem at hand.
> 
> I am not assuming it is overkill, but since you do not reuse functions 
> such as strbuf_expand() and split_cmdline(), your patch ends up pretty 
> large.

Thanks! I didn't know that there was a split_cmdline. I will use it and resubmit.

> And since you use very short and undescriptive variable names, with ugly 
> assignments inside arithmetic expressions, I will be less likely 
> reviewing it in detail.

I actually use those assignments for clarity. IMO, it is more clear to have one line which clearly initializes a buffer (and from which the exact details of the buffer initialization can still be read), than to litter an algorithm with lots of auxiliary lines that obfuscate what the code really does.

Seeing how you disagree, though, I'll change that too when I'm at it.

Show 5 quoted lines

> > I assume it might have to be documented as well, if people approve of it.
> 
> Catch 22.  Since you have not documented what %P should be useful for, 
> people might not approve of the patch, because they do not understand what
> it is supposed to do.

Yes, that would be a problem. Here's some makeshift documentation:

The string specified in core.sshcommand is first checked if it matches any of the built-in templates, in which case it is expanded (I've added the templates "openssh" and "plink" by default). When used, the string is split into words, each of which is processed as follows:

* If a word is %p, it is replaced by the port number, if specified.
  If the port number is not specified, the word is deleted.
* If a word is %h, it is replaced by the remote host name.
* If a word begins with %P, it is deleted if no port number is
  specified. This is to allow for specifying different port number
  flags for different SSH implementations. The syntax is a bit ugly,
  but I cannot really think of anything that would look better.
  If a port number has been specified, the leading %P is simply deleted.

The result is used, along with the command to run on the remote side, as the SSH command line.

Fredrik Tolf