git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Jonathan Nieder" <jrnieder@gmail.com>,
	"Joachim Schmitz" <jojo@schmitz-digital.de>,
	"René Scharfe" <rene.scharfe@lsrfire.ath.cx>,
	git@vger.kernel.org
Subject: [PATCHv2 0/8] config key-parsing cleanups
Date: Wed, 23 Jan 2013 01:21:32 -0500	[thread overview]
Message-ID: <20130123062132.GA2038@sigill.intra.peff.net> (raw)
In-Reply-To: <7va9s6qkkz.fsf@alter.siamese.dyndns.org>

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

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

  reply	other threads:[~2013-01-23  6:22 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-01-13 17:42 [PATCH] archive-tar: fix sanity check in config parsing René Scharfe
2013-01-13 20:00 ` Jeff King
2013-01-14  8:17   ` Joachim Schmitz
2013-01-14 12:44     ` Jeff King
2013-01-14 14:58       ` Jeff King
2013-01-14 15:00         ` [PATCH 1/6] config: add helper function for parsing key names Jeff King
2013-01-14 18:08           ` Junio C Hamano
2013-01-15 16:04             ` Jeff King
2013-01-15 17:07               ` Junio C Hamano
2013-01-18 20:53                 ` Junio C Hamano
2013-01-23  6:21                   ` Jeff King [this message]
2013-01-23  6:23                     ` [PATCHv2 1/8] " Jeff King
2013-01-23  6:23                     ` [PATCHv2 2/8] archive-tar: use parse_config_key when parsing config Jeff King
2013-01-23  6:24                     ` [PATCHv2 3/8] convert some config callbacks to parse_config_key Jeff King
2013-01-23  6:25                     ` [PATCHv2 4/8] userdiff: drop parse_driver function Jeff King
2013-01-23  6:25                     ` [PATCHv2 5/8] submodule: use parse_config_key when parsing config Jeff King
2013-01-23 20:45                       ` Jens Lehmann
2013-01-23  6:26                     ` [PATCHv2 6/8] submodule: simplify memory handling in config parsing Jeff King
2013-01-23 20:51                       ` Jens Lehmann
2013-01-23  6:27                     ` [PATCHv2 7/8] help: use parse_config_key for man config Jeff King
2013-01-23  6:27                     ` [PATCHv2 8/8] reflog: use parse_config_key in config callback Jeff King
2013-01-23  7:04                       ` Junio C Hamano
2013-01-23  7:27                     ` [PATCHv2 0/8] config key-parsing cleanups Jonathan Nieder
2013-01-14 15:02         ` [PATCH 2/6] archive-tar: use match_config_key when parsing config Jeff King
2013-01-14 15:03         ` [PATCH 3/6] convert some config callbacks to match_config_key Jeff King
2013-01-14 16:55           ` Jonathan Nieder
2013-01-14 17:06             ` Jeff King
2013-01-14 18:05               ` Jeff King
2013-01-14 15:04         ` [PATCH 4/6] userdiff: drop parse_driver function Jeff King
2013-01-14 15:04         ` [PATCH 5/6] submodule: use match_config_key when parsing config Jeff King
2013-01-14 15:07         ` [PATCH 6/6] submodule: simplify memory handling in config parsing Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130123062132.GA2038@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jojo@schmitz-digital.de \
    --cc=jrnieder@gmail.com \
    --cc=rene.scharfe@lsrfire.ath.cx \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).