Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Jeff King <hidden>
Date: 2016-06-15 22:56:22
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 <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?
+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.
quoted hunk ↗ jump to hunk
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