Re: [PATCH 2/2] connect: improve check for plink to reduce false positives
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Apr 23, 2015, 23:14 UTC
- Message-ID
- <20150423231403.GC274681@vauxhall.crustytoothpaste.net>
- In-Reply-To
- <20150423155304.GA26018@peff.net>
On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote:
Show 13 quoted lines
> 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.
Show 18 quoted lines
> 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