[PATCHv2 0/8] config key-parsing cleanups

From: Jeff King <peff@peff.net>
Date: Jan 23, 2013, 06:21 UTC
Message-ID: <20130123062132.GA2038@sigill.intra.peff.net>
In-Reply-To: <7va9s6qkkz.fsf@alter.siamese.dyndns.org>

On Fri, Jan 18, 2013 at 12:53:32PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> >> ... did you have any comment on
> >> the "struct config_key" alternative I sent as a follow-up?
> >
> > I did read it but I cannot say I did so very carefully.  My gut
> > reaction was that the "take the variable name and section name,
> > return the subsection name pointer and length, if there is any, and
> > the key" made it readable enough.  The proposed interface to make
> > and lend a copy to the caller does make it more readble, but I do
> > not know if that is worth doing.  Neutral-to-slightly-in-favor, I
> > would say.
> 
> Now I re-read that "struct config_key" thing, I would have to say
> that the idea of giving split and NUL-terminated strings to the
> callers is good, but the "cheat" looks somewhat brittle for all the
> reasons that come from using a static buffer (which you already
> mentioned).  As I do not offhand think of a better alternative, I'd
> say we leave it for another day.

OK. I had the feeling if the config parser provided it to the caller that more sites could take advantage of it (without adding too many lines to call the parsing function). But looking again, there aren't that many sites that would benefit. E.g., git_daemon_config in daemon.c could use it to avoid using a constant offset. But the current code there is not hard to read, and saving a few characters there is not worth the complexity.

So I've re-rolled the original version, taking into account the comments from you and Jonathan. I also clarified a few of the commit messages, and modified two more sites to use the new function.

  [1/8]: config: add helper function for parsing key names

Same as before, but now called parse_config_key.

  [2/8]: archive-tar: use parse_config_key when parsing config

Same (rebased for new name, of course).

  [3/8]: convert some config callbacks to parse_config_key

Tweaked confusing "ep" variable name. Fixed missing "!name" check in userdiff code (which gets removed in the next patch anyway).

  [4/8]: userdiff: drop parse_driver function
  [5/8]: submodule: use parse_config_key when parsing config
  [6/8]: submodule: simplify memory handling in config parsing

Same.

  [7/8]: help: use parse_config_key for man config
  [8/8]: reflog: use parse_config_key in config callback

Two new callsites. I split these out because unlike the ones in 3/8, they do not benefit from a reduction in lines of code. However, I think the results are still more readable. You can judge for yourself; drop them if you disagree. Or feel free to squash them into 3/8 if that makes more sense.

-Peff