Re: [PATCH v2 4/4] teach config parsing to read from strbuf
- From
Heiko Voigt <hvoigt@hvoigt.net>
- Date
- Mar 12, 2013, 16:42 UTC
- Message-ID
- <20130312164254.GB4752@sandbox-ub.fritz.box>
- In-Reply-To
- <20130312111806.GF11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 07:18:06AM -0400, Jeff King wrote:
Show 10 quoted lines
> 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.
Show 18 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.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.
Show 10 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.
I mainly did this because I needed some test for the config part while developing the "fetch renamed submodules" series.
Show 5 quoted lines
> 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