From: Jeff King <peff@xxxxxxxx> Subject: Re: [PATCH v8 03/12] Move lower case functions into wrapper.c Date: Thu, 27 Mar 2014 18:34:06 -0400 > On Thu, Mar 27, 2014 at 03:16:48PM -0700, Junio C Hamano wrote: > >> > I wasn't looking at the caller (and I haven't). I agree that, if >> > you have to compare case-insensitive user input against known set of >> > tokens, using strcasecmp() would be saner than making a downcased >> > copy and the set of known tokens. I do not know however you want to >> > compare in a case-insensitive way in your application, though. >> >> It appears that one place this "lowercase" is used is to allow >> rAnDOm casing in the configuration file, e.g. >> >> [trailer "Signed-off-by"] >> where = AfTEr >> >> which I find is totally unnecessary. Do we churn code to accept >> such a nonsense input in other places? > > I think we are very inconsistent. > > All bool config values allow "tRuE". Ones that take "auto" often use > strcasecmp (e.g., diff.*.binary). "blame.date" and "help.format" choose > from a fixed set of tokens, but use strcmp. > > Command line parameters are of course case-sensitive, and tokens used by > them usually are, too (e.g., the date formats for "blame.date" or also > the same ones taken by "--date="). > > In general I do not see any reason _not_ to use strcasecmp for config > values that are matching a fixed set. It's friendlier to the user, the > extra CPU time is negligible, and the code is no harder to read than a > strcmp. I agree with this. I think it would be better to just use strcasecmp() for all the config values matching a fixed set. It is just much easier to explain to users how things work this way. Even if no one ever complained about this on the mailing list, many users complain that Git is very inconsistent. > Just looking at the callers in patch 04/12, I think it would be > better just used strcasecmp instead of making a lowercase copy. Not > because the copy is wasteful (it is, but it almost certainly doesn't > matter here), but because avoiding the copy is shorter and easier to > follow (you don't have to wonder about memory ownership). Yeah, that's also why I am not very happy to have to change things in this area. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html