Re: [PATCH v8 03/12] Move lower case functions into wrapper.c

[Jeff King <peff@peff.net>]

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. 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).

-Peff