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