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

From: Jeff King <peff@peff.net>
Date: Mar 27, 2014, 22:34 UTC
Message-ID: <20140327223406.GA32434@sigill.intra.peff.net>
In-Reply-To: <xmqqlhvvcmnj.fsf@gitster.dls.corp.google.com>

On Thu, Mar 27, 2014 at 03:16:48PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

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