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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Mar 12, 2013 at 07:18:06AM -0400, Jeff King wrote:
> 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?

Will add some more here.

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

pointer/len should be fine too. I just used strbuf since when you find
out later that you need to modify the string its easier to handle. A
config parser should not need to do that so I will change that.

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

I mainly did this because I needed some test for the config part while
developing the "fetch renamed submodules" series.

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

Since this is a split of the series mentioned above there are no real
callers yet. The main reason for the split was that I wanted to reduce
the review burden of one big series into multiple reviews of smaller
chunks. If you think it is useful to add the --blob option I can also
test from there. It could actually be useful to look at certain
.gitmodules options from the submodule script.

Cheers Heiko