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

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


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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to