Re: [PATCH 2/2] connect: improve check for plink to reduce false positives
Hi Peff, On 2015-04-23 17:53, Jeff King wrote: What about plink-0.83 that was mentioned earlier in the thread? I was working a lot with Java projects where the base name is often considered to be the part before a version number that is encoded into the file name, so I think it would be a good idea here, too. Essentially, it would be the regex ^(.*[/\\])?plink(-.*|\.exe)$ ... and maybe we should actually do exactly this, use a regex? I know that many developers try to stay away from them, but if you can read them, they are an efficient encoding of expectations. 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 2/2] connect: improve check for plink to reduce false positives
Hi Brian, On 2015-04-24 01:14, brian m. carlson wrote: On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote: If I were writing from scratch, I would probably keep things as tight as possible, like: const char *base = basename(ssh); plink = !strcasecmp(base, plink) || !strcasecmp(base, plink.exe); tplink = !strcasecmp(base, tortoiseplink) || !strcasecmp(base, tortoiseplink.exe)); but maybe that is too tight at this point in time; we don't really know what's out there and working (or maybe _we_ do, but _I_ do not :) ). At any rate, brian's patch only looks for a dir-separator anywhere, not the actual basename. So: /path/to/plink/ssh would match, and I'm not sure if that's a good thing or not. This is true. In hindsight, I think it's probably better to be a bit stricter, so I'll reroll to use the stricter format. Thank you so much! 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 2/2] connect: improve check for plink to reduce false positives
On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote: + tortoiseplink = tplink == ssh || + (tplink is_dir_sep(tplink[-1])); Maybe have a helper function here? Something like `basename_matches(const char *path, const char *basename, int ignore_case)`? That would be easier to read (I have to admit that I had to wrap my head around the logic to ensure that tplink[-1] is valid; It is, but it requires more brain cycles to verify than I would like). Yeah, I had a similar thought when reading the patch. Also, I am really hesitant to just test the start of the basename; I would rather have an entire basename match so that something like PLinKoeln would not match. (And of course, for Windows I would want that hypothetical `basename_matches()` function to allow basenames to end in `.exe` automatically). What about plink-0.83 that was mentioned earlier in the thread? I think that is the reason brian's patch stuck to matching the start and not the end. But I have no idea if that is actually a real thing, or just a hypothetical. If I were writing from scratch, I would probably keep things as tight as possible, like: const char *base = basename(ssh); plink = !strcasecmp(base, plink) || !strcasecmp(base, plink.exe); tplink = !strcasecmp(base, tortoiseplink) || !strcasecmp(base, tortoiseplink.exe)); but maybe that is too tight at this point in time; we don't really know what's out there and working (or maybe _we_ do, but _I_ do not :) ). At any rate, brian's patch only looks for a dir-separator anywhere, not the actual basename. So: /path/to/plink/ssh would match, and I'm not sure if that's a good thing or not. So yet another variant is to use basename(), and then just check that the basename starts with plink (to catch plink.exe, plink-0.83, etc). That avoids cruft in the intermediate path, and unless your actual binary is named PlinKoeln, it will not false positive on the example you gave. -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 2/2] connect: improve check for plink to reduce false positives
Hi Brian, On 2015-04-23 02:06, brian m. carlson wrote: + tortoiseplink = tplink == ssh || + (tplink is_dir_sep(tplink[-1])); Maybe have a helper function here? Something like `basename_matches(const char *path, const char *basename, int ignore_case)`? That would be easier to read (I have to admit that I had to wrap my head around the logic to ensure that tplink[-1] is valid; It is, but it requires more brain cycles to verify than I would like). Also, I am really hesitant to just test the start of the basename; I would rather have an entire basename match so that something like PLinKoeln would not match. (And of course, for Windows I would want that hypothetical `basename_matches()` function to allow basenames to end in `.exe` automatically). In contrast to Torsten, I am not so concerned about `myplink` scripts: that only affects power users who can easily add the `-batch` into the script, where it actually belongs. 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 2/2] connect: improve check for plink to reduce false positives
On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote: On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote: + tortoiseplink = tplink == ssh || + (tplink is_dir_sep(tplink[-1])); Maybe have a helper function here? Something like `basename_matches(const char *path, const char *basename, int ignore_case)`? That would be easier to read (I have to admit that I had to wrap my head around the logic to ensure that tplink[-1] is valid; It is, but it requires more brain cycles to verify than I would like). Yeah, I had a similar thought when reading the patch. I was questioning whether a comment would have been helpful. Apparently the answer was yes. If I were writing from scratch, I would probably keep things as tight as possible, like: const char *base = basename(ssh); plink = !strcasecmp(base, plink) || !strcasecmp(base, plink.exe); tplink = !strcasecmp(base, tortoiseplink) || !strcasecmp(base, tortoiseplink.exe)); but maybe that is too tight at this point in time; we don't really know what's out there and working (or maybe _we_ do, but _I_ do not :) ). At any rate, brian's patch only looks for a dir-separator anywhere, not the actual basename. So: /path/to/plink/ssh would match, and I'm not sure if that's a good thing or not. This is true. In hindsight, I think it's probably better to be a bit stricter, so I'll reroll to use the stricter format. -- 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
