Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

Jeff King [off-list ref] writes:

On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:

quoted

Improve the check by looking for "plink" or "tortoiseplink" (or those
names suffixed with ".exe") only in the final component of the path.
This has the downside that a program such as "plink-0.63" would no
longer be recognized, but the increased robustness is likely worth it.
Add tests to cover these cases to avoid regressions.

FYI, this ended up biting me today. We have some integration tests that
make sure we can clone over putty, and we wrap plink in a
"plink-wrapper.sh" script that tweaks a few extra options. That used to
match under the old scheme, but not the new. It would also match if we
looked for "plink" anywhere in the basename (but not in leading
directories).

So this was a minor regression? ;-)

I was able to work around it pretty easily by changing our test setup,
but I thought I would include it here as a data point. It's probably not
that representative of real-world users.

I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
appropriate name to install that wrapper as than "plink-wrapper.sh",
but then people would need to think how to help that wrapper find
the real plink, so...

On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:

quoted

FYI, this ended up biting me today. We have some integration tests that
make sure we can clone over putty, and we wrap plink in a
"plink-wrapper.sh" script that tweaks a few extra options. That used to
match under the old scheme, but not the new. It would also match if we
looked for "plink" anywhere in the basename (but not in leading
directories).

So this was a minor regression? ;-)

Yes. :)

quoted

I was able to work around it pretty easily by changing our test setup,
but I thought I would include it here as a data point. It's probably not
that representative of real-world users.

I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
appropriate name to install that wrapper as than "plink-wrapper.sh",
but then people would need to think how to help that wrapper find
the real plink, so...

It's the test suite for the server side of our git infrastructure, so
nothing gets installed. It's more like:

  export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
  export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
  git clone localhost:foo.git

and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
easy to swap, without any hacks to avoid recursing to ourselves in the
$PATH.

I doubt it is a problem for most people, because I don't imagine they
are writing test suites for git-related software.

-Peff

Hi Peff,

On 2015-06-26 18:27, Jeff King wrote:

On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:

quoted

quoted

FYI, this ended up biting me today. We have some integration tests that
make sure we can clone over putty, and we wrap plink in a
"plink-wrapper.sh" script that tweaks a few extra options. That used to
match under the old scheme, but not the new. It would also match if we
looked for "plink" anywhere in the basename (but not in leading
directories).

So this was a minor regression? ;-)

Yes. :)

quoted

quoted

I was able to work around it pretty easily by changing our test setup,
but I thought I would include it here as a data point. It's probably not
that representative of real-world users.

I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
appropriate name to install that wrapper as than "plink-wrapper.sh",
but then people would need to think how to help that wrapper find
the real plink, so...

It's the test suite for the server side of our git infrastructure, so
nothing gets installed. It's more like:

  export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
  export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
  git clone localhost:foo.git

and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
easy to swap, without any hacks to avoid recursing to ourselves in the
$PATH.

I doubt it is a problem for most people, because I don't imagine they
are writing test suites for git-related software.

Sorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.

Would it help you if we detected ^plink[^a-zA-Z]?

Ciao,
Dscho

Footnote *1*: took me friggin' 9 1/2 hours to figure this one out: https://github.com/Alexpux/MSYS2-packages/pull/275

On Fri, Jun 26, 2015 at 07:13:15PM +0200, Johannes Schindelin wrote:

quoted

It's the test suite for the server side of our git infrastructure, so
nothing gets installed. It's more like:

  export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
  export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
  git clone localhost:foo.git

and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
easy to swap, without any hacks to avoid recursing to ourselves in the
$PATH.

I doubt it is a problem for most people, because I don't imagine they
are writing test suites for git-related software.

Sorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.

No problem. I don't envy you. :)

Would it help you if we detected ^plink[^a-zA-Z]?

In our we would have needed "^plink[^a-zA-Z-.]". I think there's no real
"right" answer here, as you can come up with hypotheticals that work and
don't work with just about every pattern. I was less trying to advocate
for loosening, and more just providing a data point to the list.

If we were to do any loosening, I'd probably suggest "^plink.*" (in the
basename).

-Peff