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

[Jeff King <peff@peff.net>]

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?

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

> 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