Re: [PATCH] archive-tar: fix sanity check in config parsing

From: Jeff King <peff@peff.net>
Date: Jan 14, 2013, 14:58 UTC
Message-ID: <20130114145845.GA16497@sigill.intra.peff.net>
In-Reply-To: <20130114124424.GA14129@sigill.intra.peff.net>

On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:

Show 5 quoted lines

> > Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> > at least a comment?
> [...]
> We could also potentially encapsulate it in a function. I think the diff
> code has a very similar block.

Here's a series that does that, with a few other cleanups on top. The diffstat actually ends up a few lines longer, but that is mostly because of comments and function declarations. More importantly, though, the call-sites are much easier to read.

Having written this series, though, I can't help but wonder if the world would be a better place if config_fn_t looked more like:

  typedef int (*config_fn_t)(const char *full_var,
                             const char *section,
                             const char *subsection,
                             const char *key,
                             const char *value,
                             void *data);

It's just as easy for the config parser to do this ahead of time, and by handing off real C-strings (instead of ending up with a ptr/len pair for the subsection), it makes the lives of the callbacks much easier (e.g., the final patch below contorts a bit to use string_list with the subsection).

I can look into that, but here is the less invasive cleanup:

  [1/6]: config: add helper function for parsing key names
  [2/6]: archive-tar: use match_config_key when parsing config
  [3/6]: convert some config callbacks to match_config_key
  [4/6]: userdiff: drop parse_driver function
  [5/6]: submodule: use match_config_key when parsing config
  [6/6]: submodule: simplify memory handling in config parsing

-Peff