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
