Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
Jeff King p...@peff.net writes: On Sun, Apr 26, 2015 at 08:30:12PM +, brian m. carlson wrote: 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... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote: 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. :) 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
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: 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. :) 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
On Fri, Jun 26, 2015 at 07:13:15PM +0200, Johannes Schindelin wrote: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
On Fri, Jun 26, 2015 at 09:15:24AM -0400, Jeff King wrote: On Sun, Apr 26, 2015 at 08:30:12PM +, brian m. carlson wrote: 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). 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. Thanks for the data point. While we don't use plink at $DAYJOB, this is the kind of test we might well perform. I expect it's most likely to hit people in test setups like this, but if it turns out to be a problem, we can certainly loosen it or if necessary, revert it. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
On Sun, Apr 26, 2015 at 08:30:12PM +, brian m. carlson wrote: The git_connect function has code to handle plink and tortoiseplink specially, as they require different command line arguments from OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires -batch). However, the match was done by checking for plink anywhere in the string, which led to a GIT_SSH value containing uplink being treated as an invocation of putty's plink. 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). 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. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
Hi, On 2015-04-26 22:30, brian m. carlson wrote: The git_connect function has code to handle plink and tortoiseplink specially, as they require different command line arguments from OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires -batch). However, the match was done by checking for plink anywhere in the string, which led to a GIT_SSH value containing uplink being treated as an invocation of putty's plink. 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. Signed-off-by: brian m. carlson sand...@crustytoothpaste.net I like it! Acked-by: Johannes Schindelin johannes.schinde...@gmx.de Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives
On Sun, Apr 26, 2015 at 08:30:12PM +, brian m. carlson wrote: The git_connect function has code to handle plink and tortoiseplink specially, as they require different command line arguments from OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires -batch). However, the match was done by checking for plink anywhere in the string, which led to a GIT_SSH value containing uplink being treated as an invocation of putty's plink. 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. Thanks, I think this version looks good. It's possible that it's too tight, but I think it represents our best guess at what is reasonable, and we can only know more by getting it into the hands of users (hopefully while it is cooking during this release cycle, but I am not incredibly optimistic about such things). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
