Re: [PATCH v2 4/4] teach config parsing to read from strbuf

From: Jeff King <peff@peff.net>
Date: Mar 12, 2013, 11:18 UTC
Message-ID: <20130312111806.GF11340@sigill.intra.peff.net>
In-Reply-To: <20130310170052.GE1136@sandbox-ub.fritz.box>

On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:

> This can be used to read configuration values directly from gits
> database.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

This is lacking motivation. IIRC, the rest of the story is something like "...so we can read .gitmodules directly from the repo" or something like that?

Show 8 quoted lines

> +struct config_strbuf {
> +	struct strbuf *strbuf;
> +	int pos;
> +};
>
> +static int config_strbuf_fgetc(struct config_source *conf)
> +{
> +	struct config_strbuf *str = conf->data;

Yuck. If you used a union in the previous patch, then this could just go inline into the "struct config_source".

> +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)

Should this be a "const struct strbuf *strbuf"? For that matter, is there any reason not to take a bare pointer/len combination? It seems likely that callers would get the data from read_sha1_file, which means they have to stuff it into a strbuf for no good reason.

Show 6 quoted lines

> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 0000000..c650837
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,40 @@

I'm slightly "meh" on this test-config program. Having to add a C test harness like this is a good indication that we are short-changing users of the shell API in favor of builtin C code.

Your series does not actually add any callers of the new function. The obvious "patch 5/4" would be to plumb it into "git config --blob", and then we can just directly test it there (there could be other callers besides reading from a blob, of course, but I think the point of the series is to head in that direction).

-Peff