connect: recognize [tortoise]plink in GIT_SSH_COMMAND

[segevfiner]

When users want to pass some additional arguments to [tortoise]plink via
GIT_SSH, for example, setting a private key, the user is required to either
use a shell script named plink or tortoiseplink or duplicate the logic
that's already in Git for passing the correct style of command line
arguments, which can be difficult, error prone and annoying to get right.
They could have used GIT_SSH_COMMAND but Git doesn't detect [tortoise]plink
in this environment variable, meaning that Git would pass the wrong style
of command line arguments. For example -p instead of -P.

This patch uses the logic that was used to recognize [tortoise]plink in
GIT_SSH for the GIT_SSH_COMMAND environment variable, allowing to use
GIT_SSH_COMMAND with [tortoise]plink directly.

Signed-off-by: Segev Finer [email protected]

[PhilipOakley]

Segev, Can I suggest an alternative wording to cover the implied 'why, what, how' of your commit - which, If I understand it correctly, simply allows extra command line arguments to be passed to tortoise/plink. "The Git [tortoise]plink interface passes a fixed set of parameter. When users want to pass some additional arguments to [tortoise]plink, for example, setting a private key, the user is required to use a shell script that will duplicate the logic that's already in Git which can be difficult, error prone and annoying to get right. Adapt the passed command line arguments to include the additional user supplied arguments." Or something like that. (I mainly stole your words and re-arranged them;-) You may want to adjust the title/subject line as well. When I looked at the code, All I saw was a change to the way you process the argv[] array, rather than being any change that directly asks 'is this plink' (that part is already somewhere else I think). The other think to clarify would be if there are any security implications of just taking any rubbish the user mis-typed, or a bad actor thought of. I.e. Do the extra inputs need sanitising, or are we good? Philip

…

----- Original Message ----- From: Segev Finer To: git-for-windows/git Cc: Subscribed Sent: Friday, December 23, 2016 10:19 PM Subject: [git-for-windows/git] connect: recognize [tortoise]plink in GIT_SSH_COMMAND (#1006) This allows using GIT_SSH_COMMAND with [tortoise]plink without having to use a shell script that will duplicate the logic that's already in Git that adapts the command line arguments for [tortoise]plink. Having to use such a shell script is annoying when you want to simply pass some additional arguments to [tortoise]plink, for example: setting a private key. Signed-off-by: Segev Finer [email protected]

------------------------------------------------------------------------------ You can view, comment on, or merge this pull request online at: #1006 Commit Summary a.. connect: recognize [tortoise]plink in GIT_SSH_COMMAND File Changes a.. M connect.c (26) b.. M t/t5601-clone.sh (12) Patch Links: a.. https://github.com/git-for-windows/git/pull/1006.patch b.. https://github.com/git-for-windows/git/pull/1006.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[segevfiner]

@PhilipOakley

This change is actually specific to the GIT_SSH_COMMAND environment variable which existed before. You could set it to a command line that invokes plink, but the logic that determines whether we are invoking plink or not was only used for the GIT_SSH environment variable. Meaning that Git would have passed -p instead of -P, for example, and the invocation would fail. You could write a shell script which would do the necessary conversion when using GIT_SSH_COMMAND but that will defeat the purpose, in my eyes, of GIT_SSH_COMMAND which would be to avoid a shell script.

I hope this clarifies what this change is about.

How about this as a clearer description:

connect: recognize [tortoise]plink in GIT_SSH_COMMAND

When users want to pass some additional arguments to [tortoise]plink via GIT_SSH, for example,
setting a private key, the user is required to either use a shell script named plink or tortoiseplink or
duplicate the logic that's already in Git for passing the correct style of command line arguments, which can be difficult, error prone and annoying to get right. They could have used GIT_SSH_COMMAND
but Git doesn't detect [tortoise]plink in this environment variable, meaning that Git would
pass the wrong style of command line arguments. For example -p instead of -P.

This patch uses the logic that was used to recognize [tortoise]plink in GIT_SSH for the
GIT_SSH_COMMAND environment variable, allowing to use GIT_SSH_COMMAND
with [tortoise]plink directly.

If it's OK I will rewrite the commit with it.

I don't think I caused any change in terms of security. Git used to pass GIT_SSH_COMMAND directly to the shell and that haven't change. I simply added in code to get the first argument (The command) to check if it's plink or not using the same logic that was used for the GIT_SSH environment variable before.

Thanks.

[dscho]

Thank you for your contribution!

Until next year I will only be able to see the commit on a small screen, and that does not let me assess whether my hunch is correct that there might be a shorter patch accomplishing the same goal.

Could you ping me after January 2nd so I do not forget about this PR? Thanks!

[PhilipOakley]

Segev, The extra paragraph (""This patch uses") helped clarify it for me. I think it was having the proximity of GIT_SSH and GIT_SSH_COMMAND as disrinct env variables in the same sentence that let me see what was going on, which I hadn't seen before. P. ----- Original Message ----- From: Segev Finer To: git-for-windows/git Cc: Philip Oakley ; Mention Sent: Saturday, December 24, 2016 9:25 AM Subject: Re: [git-for-windows/git] connect: recognize [tortoise]plink in GIT_SSH_COMMAND (#1006) @PhilipOakley This change is actually specific to the GIT_SSH_COMMAND environment variable which existed before. You could set it to a command line that invokes plink, but the logic that determines whether we are invoking plink or not was only used for the GIT_SSH environment variable. Meaning that Git would have passed -p instead of -P, for example, and the invocation would fail. You could write a shell script which would do the necessary conversion when using GIT_SSH_COMMAND but that will defeat the purpose, in my eyes, of GIT_SSH_COMMAND which would be to avoid a shell script. I hope this clarifies what this change is about. How about this as a clearer description: connect: recognize [tortoise]plink in GIT_SSH_COMMAND When users want to pass some additional arguments to [tortoise]plink via GIT_SSH, for example, setting a private key, the user is required to either use a shell script named plink or tortoiseplink or duplicate the logic that's already in Git for passing the correct style of command line arguments, which can be difficult, error prone and annoying to get right. They could have used GIT_SSH_COMMAND but Git doesn't detect [tortoise]plink in this environment variable, meaning that Git would pass the wrong style of command line arguments. For example -p instead of -P. This patch uses the logic that was used to recognize [tortoise]plink in GIT_SSH for the GIT_SSH_COMMAND environment variable, allowing to use GIT_SSH_COMMAND with [tortoise]plink directly. If it's OK I will rewrite the commit with it. I don't think I caused any change in terms of security. Git used to pass GIT_SSH_COMMAND directly to the shell and that haven't change. I simply added in code to get the first argument (The command) to check if it's plink or not using the same logic that was used for the GIT_SSH environment variable before. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dscho]

Thank you! This iteration is already almost what I will send upstream, it's quicker for me to do the final touch-ups myself.

Thanks again for your contribution!