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 <hvo...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
