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

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:

quoted

This can be used to read configuration values directly from gits
database.

Signed-off-by: Heiko Voigt <redacted>

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.

quoted

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

quoted

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

quoted

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