[PATCH v2 0/4] allow more sources for config values

[PATCH v2 0/4] allow more sources for config values

[Heiko Voigt]

The following issues still exist:

 * Error handling: If this should be useful to interrogate configs from
   the database during git operations we need a way to recover from
   parsing errors instead of dying.

 * More tests ?

This is an update with the comments of the first iteration[1]
incorporated.

[1] http://thread.gmane.org/gmane.comp.version-control.git/217018/

Heiko Voigt (4):
  config: factor out config file stack management
  config: drop file pointer validity check in get_next_char()
  config: make parsing stack struct independent from actual data source
  teach config parsing to read from strbuf

 .gitignore             |   1 +
 Makefile               |   1 +
 cache.h                |   2 +
 config.c               | 143 ++++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh |  24 +++++++++
 test-config.c          |  40 ++++++++++++++
 6 files changed, 180 insertions(+), 31 deletions(-)
 create mode 100644 test-config.c

-- 
1.8.2.rc0.26.gf7384c5

[PATCH v2 1/4] config: factor out config file stack management

[Heiko Voigt]

Because a config callback may start parsing a new file, the
global context regarding the current config file is stored
as a stack. Currently we only need to manage that stack from
git_config_from_file. Let's factor it out to allow new
sources of config data.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 config.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/config.c b/config.c
index aefd80b..2c299dc 100644
--- a/config.c
+++ b/config.c
@@ -896,6 +896,28 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	return 0;
 }
 
+static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
+{
+	int ret;
+
+	/* push config-file parsing state stack */
+	top->prev = cf;
+	top->linenr = 1;
+	top->eof = 0;
+	strbuf_init(&top->value, 1024);
+	strbuf_init(&top->var, 1024);
+	cf = top;
+
+	ret = git_parse_file(fn, data);
+
+	/* pop config-file parsing state stack */
+	strbuf_release(&top->value);
+	strbuf_release(&top->var);
+	cf = top->prev;
+
+	return ret;
+}
+
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
 	int ret;
@@ -905,22 +927,10 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	if (f) {
 		config_file top;
 
-		/* push config-file parsing state stack */
-		top.prev = cf;
 		top.f = f;
 		top.name = filename;
-		top.linenr = 1;
-		top.eof = 0;
-		strbuf_init(&top.value, 1024);
-		strbuf_init(&top.var, 1024);
-		cf = &top;
-
-		ret = git_parse_file(fn, data);
-
-		/* pop config-file parsing state stack */
-		strbuf_release(&top.value);
-		strbuf_release(&top.var);
-		cf = top.prev;
+
+		ret = do_config_from(&top, fn, data);
 
 		fclose(f);
 	}
-- 
1.8.2.rc0.26.gf7384c5

[PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

The only location where cf is set in this file is in do_config_from().
This function has only one callsite which is config_from_file(). In
config_from_file() its ensured that the f member is set to non-zero.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 2c299dc..f55c43d 100644
--- a/config.c
+++ b/config.c
@@ -169,10 +169,10 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 static int get_next_char(void)
 {
 	int c;
-	FILE *f;
 
 	c = '\n';
-	if (cf && ((f = cf->f) != NULL)) {
+	if (cf) {
+		FILE *f = cf->f;
 		c = fgetc(f);
 		if (c == '\r') {
 			/* DOS like systems */
-- 
1.8.2.rc0.26.gf7384c5

[PATCH v2 3/4] config: make parsing stack struct independent from actual data source

[Heiko Voigt]

To simplify adding other sources we extract all functions needed for
parsing into a list of callbacks. We implement those callbacks for the
current file parsing. A new source can implement its own set of callbacks.

Instead of storing the concrete FILE pointer for parsing we store a void
pointer. A new source can use this to store its custom data.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 config.c | 63 +++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/config.c b/config.c
index f55c43d..fe1c0e5 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,42 @@
 #include "strbuf.h"
 #include "quote.h"
 
-typedef struct config_file {
-	struct config_file *prev;
-	FILE *f;
+struct config_source {
+	struct config_source *prev;
+	void *data;
 	const char *name;
 	int linenr;
 	int eof;
 	struct strbuf value;
 	struct strbuf var;
-} config_file;
 
-static config_file *cf;
+	int (*fgetc)(struct config_source *c);
+	int (*ungetc)(int c, struct config_source *conf);
+	long (*ftell)(struct config_source *c);
+};
+
+static struct config_source *cf;
 
 static int zlib_compression_seen;
 
+static int config_file_fgetc(struct config_source *conf)
+{
+	FILE *f = conf->data;
+	return fgetc(f);
+}
+
+static int config_file_ungetc(int c, struct config_source *conf)
+{
+	FILE *f = conf->data;
+	return ungetc(c, f);
+}
+
+static long config_file_ftell(struct config_source *conf)
+{
+	FILE *f = conf->data;
+	return ftell(f);
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -172,13 +194,12 @@ static int get_next_char(void)
 
 	c = '\n';
 	if (cf) {
-		FILE *f = cf->f;
-		c = fgetc(f);
+		c = cf->fgetc(cf);
 		if (c == '\r') {
 			/* DOS like systems */
-			c = fgetc(f);
+			c = cf->fgetc(cf);
 			if (c != '\n') {
-				ungetc(c, f);
+				cf->ungetc(c, cf);
 				c = '\r';
 			}
 		}
@@ -339,7 +360,7 @@ static int get_base_var(struct strbuf *name)
 	}
 }
 
-static int git_parse_file(config_fn_t fn, void *data)
+static int git_parse_source(config_fn_t fn, void *data)
 {
 	int comment = 0;
 	int baselen = 0;
@@ -896,7 +917,7 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	return 0;
 }
 
-static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
+static int do_config_from_source(struct config_source *top, config_fn_t fn, void *data)
 {
 	int ret;
 
@@ -908,7 +929,7 @@ static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
 	strbuf_init(&top->var, 1024);
 	cf = top;
 
-	ret = git_parse_file(fn, data);
+	ret = git_parse_source(fn, data);
 
 	/* pop config-file parsing state stack */
 	strbuf_release(&top->value);
@@ -925,12 +946,15 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 	ret = -1;
 	if (f) {
-		config_file top;
+		struct config_source top;
 
-		top.f = f;
+		top.data = f;
 		top.name = filename;
+		top.fgetc = config_file_fgetc;
+		top.ungetc = config_file_ungetc;
+		top.ftell = config_file_ftell;
 
-		ret = do_config_from(&top, fn, data);
+		ret = do_config_from_source(&top, fn, data);
 
 		fclose(f);
 	}
@@ -1063,7 +1087,6 @@ static int store_aux(const char *key, const char *value, void *cb)
 {
 	const char *ep;
 	size_t section_len;
-	FILE *f = cf->f;
 
 	switch (store.state) {
 	case KEY_SEEN:
@@ -1075,7 +1098,7 @@ static int store_aux(const char *key, const char *value, void *cb)
 				return 1;
 			}
 
-			store.offset[store.seen] = ftell(f);
+			store.offset[store.seen] = cf->ftell(cf);
 			store.seen++;
 		}
 		break;
@@ -1102,19 +1125,19 @@ static int store_aux(const char *key, const char *value, void *cb)
 		 * Do not increment matches: this is no match, but we
 		 * just made sure we are in the desired section.
 		 */
-		store.offset[store.seen] = ftell(f);
+		store.offset[store.seen] = cf->ftell(cf);
 		/* fallthru */
 	case SECTION_END_SEEN:
 	case START:
 		if (matches(key, value)) {
-			store.offset[store.seen] = ftell(f);
+			store.offset[store.seen] = cf->ftell(cf);
 			store.state = KEY_SEEN;
 			store.seen++;
 		} else {
 			if (strrchr(key, '.') - key == store.baselen &&
 			      !strncmp(key, store.key, store.baselen)) {
 					store.state = SECTION_SEEN;
-					store.offset[store.seen] = ftell(f);
+					store.offset[store.seen] = cf->ftell(cf);
 			}
 		}
 	}
-- 
1.8.2.rc0.26.gf7384c5

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

[Heiko Voigt]

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

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 .gitignore             |  1 +
 Makefile               |  1 +
 cache.h                |  2 ++
 config.c               | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t1300-repo-config.sh | 24 ++++++++++++++++++++++++
 test-config.c          | 40 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 116 insertions(+)
 create mode 100644 test-config.c

diff --git a/.gitignore b/.gitignore
index 6669bf0..386b7f2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -178,6 +178,7 @@
 /gitweb/static/gitweb.min.*
 /test-chmtime
 /test-ctype
+/test-config
 /test-date
 /test-delta
 /test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 26d3332..1a9ea10 100644
--- a/Makefile
+++ b/Makefile
@@ -541,6 +541,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/cache.h b/cache.h
index e493563..a2621fa 100644
--- a/cache.h
+++ b/cache.h
@@ -1128,6 +1128,8 @@ extern int update_server_info(int);
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern int git_config_from_strbuf(config_fn_t fn, const char *name,
+				  struct strbuf *strbuf, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
diff --git a/config.c b/config.c
index fe1c0e5..b8c8640 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,37 @@ static long config_file_ftell(struct config_source *conf)
 	return ftell(f);
 }
 
+struct config_strbuf {
+	struct strbuf *strbuf;
+	int pos;
+};
+
+static int config_strbuf_fgetc(struct config_source *conf)
+{
+	struct config_strbuf *str = conf->data;
+
+	if (str->pos < str->strbuf->len)
+		return str->strbuf->buf[str->pos++];
+
+	return EOF;
+}
+
+static int config_strbuf_ungetc(int c, struct config_source *conf)
+{
+	struct config_strbuf *str = conf->data;
+
+	if (str->pos > 0)
+		return str->strbuf->buf[--str->pos];
+
+	return EOF;
+}
+
+static long config_strbuf_ftell(struct config_source *conf)
+{
+	struct config_strbuf *str = conf->data;
+	return str->pos;
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -961,6 +992,23 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	return ret;
 }
 
+int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)
+{
+	struct config_source top;
+	struct config_strbuf str;
+
+	str.strbuf = strbuf;
+	str.pos = 0;
+
+	top.data = &str;
+	top.name = name;
+	top.fgetc = config_strbuf_fgetc;
+	top.ungetc = config_strbuf_ungetc;
+	top.ftell = config_strbuf_ftell;
+
+	return do_config_from_source(&top, fn, data);
+}
+
 const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c96fda..5103f66 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1087,4 +1087,28 @@ test_expect_success 'barf on incomplete string' '
 	grep " line 3 " error
 '
 
+test_expect_success 'reading config from strbuf' '
+	cat >expect <<-\EOF &&
+	var: some.value, value: content
+	EOF
+	test-config strbuf 12345:.gitmodules >actual <<-\EOF &&
+	[some]
+		value = content
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'reading config from strbuf with error' '
+	touch expect.out &&
+	cat >expect.err <<-\EOF &&
+	fatal: bad config file line 2 in 12345:.gitmodules
+	EOF
+	test_must_fail test-config strbuf 12345:.gitmodules >actual.out 2>actual.err <<-\EOF &&
+	[some]
+		value = "
+	EOF
+	test_cmp expect.out actual.out &&
+	test_cmp expect.err actual.err
+'
+
 test_done
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 @@
+#include "cache.h"
+
+static int config_strbuf(const char *var, const char *value, void *data)
+{
+	printf("var: %s, value: %s\n", var, value);
+
+	return 1;
+}
+
+static void die_usage(int argc, char **argv)
+{
+	fprintf(stderr, "Usage: %s strbuf <name>\n", argv[0]);
+	exit(1);
+}
+
+int main(int argc, char **argv)
+{
+	if (argc < 3)
+		die_usage(argc, argv);
+
+	if (!strcmp(argv[1], "strbuf")) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *name = argv[2];
+
+		while (strbuf_fread(&buf, 1024, stdin) == 1024)
+			;
+
+		if (ferror(stdin))
+			die("An error occurred while reading from stdin");
+
+		git_config_from_strbuf(config_strbuf, name, &buf, NULL);
+		strbuf_release(&buf);
+
+		return 0;
+	}
+
+	die_usage(argc, argv);
+
+	return 1;
+}
-- 
1.8.2.rc0.26.gf7384c5

Re: [PATCH v2 1/4] config: factor out config file stack management

[Jeff King]

On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:

> Because a config callback may start parsing a new file, the
> global context regarding the current config file is stored
> as a stack. Currently we only need to manage that stack from
> git_config_from_file. Let's factor it out to allow new
> sources of config data.
>
> [...]
>
> +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
> +{
> +	int ret;
> +
> +	/* push config-file parsing state stack */
> +	top->prev = cf;
> +	top->linenr = 1;
> +	top->eof = 0;
> +	strbuf_init(&top->value, 1024);
> +	strbuf_init(&top->var, 1024);
> +	cf = top;
> +
> +	ret = git_parse_file(fn, data);
> +
> +	/* pop config-file parsing state stack */
> +	strbuf_release(&top->value);
> +	strbuf_release(&top->var);
> +	cf = top->prev;
> +
> +	return ret;
> +}

Can we throw in a comment at the top here with the expected usage? In
particular, do_config_from is expecting the caller to have filled in
certain fields (at this point, top->f and top->name), but there is
nothing to make that clear.

-Peff

Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Jeff King]

On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:

> The only location where cf is set in this file is in do_config_from().
> This function has only one callsite which is config_from_file(). In
> config_from_file() its ensured that the f member is set to non-zero.
> [...]
> -	if (cf && ((f = cf->f) != NULL)) {
> +	if (cf) {

I still think we can drop this conditional entirely. The complete call
graph looks like:

  git_config_from_file
    -> git_parse_file
      -> get_next_char
      -> get_value
          -> get_next_char
          -> parse_value
              -> get_next_char
      -> get_base_var
          -> get_next_char
          -> get_extended_base_var
              -> get_next_char

That is, every path to get_next_char happens while we are in
git_config_from_file, and that function guarantees that cf = &top, and
that top.f != NULL.  We do not have to even do any analysis of the
conditions for each call, because we never change "cf" nor "top.f"
except when we set them in git_config_from_file.

-Peff

Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source

[Jeff King]

On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:

> diff --git a/config.c b/config.c
> index f55c43d..fe1c0e5 100644
> --- a/config.c
> +++ b/config.c
> @@ -10,20 +10,42 @@
>  #include "strbuf.h"
>  #include "quote.h"
>  
> -typedef struct config_file {
> -	struct config_file *prev;
> -	FILE *f;
> +struct config_source {
> +	struct config_source *prev;
> +	void *data;

Would a union be more appropriate here? We do not ever have to pass it
directly as a parameter, since we pass the "struct config_source" to the
method functions.

It's still possible to screw up using a union, but it's slightly harder
than screwing up using a void pointer. And I do not think we need the
run-time flexibility offered by the void pointer in this case.

> +static int config_file_fgetc(struct config_source *conf)
> +{
> +	FILE *f = conf->data;
> +	return fgetc(f);
> +}

This could become just:

  return fgetc(conf->u.f);

and so forth (might it make sense to give "f" a more descriptive name,
as we are adding other sources?).

-Peff

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

[Jeff King]

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?

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

Re: [PATCH v2 1/4] config: factor out config file stack management

[Heiko Voigt]

On Tue, Mar 12, 2013 at 06:52:00AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:
> 
> > Because a config callback may start parsing a new file, the
> > global context regarding the current config file is stored
> > as a stack. Currently we only need to manage that stack from
> > git_config_from_file. Let's factor it out to allow new
> > sources of config data.
> >
> > [...]
> >
> > +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
> > +{
> > +	int ret;
> > +
> > +	/* push config-file parsing state stack */
> > +	top->prev = cf;
> > +	top->linenr = 1;
> > +	top->eof = 0;
> > +	strbuf_init(&top->value, 1024);
> > +	strbuf_init(&top->var, 1024);
> > +	cf = top;
> > +
> > +	ret = git_parse_file(fn, data);
> > +
> > +	/* pop config-file parsing state stack */
> > +	strbuf_release(&top->value);
> > +	strbuf_release(&top->var);
> > +	cf = top->prev;
> > +
> > +	return ret;
> > +}
> 
> Can we throw in a comment at the top here with the expected usage? In
> particular, do_config_from is expecting the caller to have filled in
> certain fields (at this point, top->f and top->name), but there is
> nothing to make that clear.

Of course. Will do that in the next iteration. How about I squash this in:

diff --git a/config.c b/config.c
index b8c8640..b7632c9 100644
--- a/config.c
+++ b/config.c
@@ -948,6 +954,9 @@ int git_default_config(const char *var, const char *value, v
        return 0;
 }
 
+/* The fields data, name and the source specific callbacks of top need
+ * to be initialized before calling this function.
+ */
 static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
 {
        int ret;


I would add that to the third patch:

	config: make parsing stack struct independent from actual data source

because that contains the final modification to config_file/config_source.

Cheers Heiko

Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> 
> > The only location where cf is set in this file is in do_config_from().
> > This function has only one callsite which is config_from_file(). In
> > config_from_file() its ensured that the f member is set to non-zero.
> > [...]
> > -	if (cf && ((f = cf->f) != NULL)) {
> > +	if (cf) {
> 
> I still think we can drop this conditional entirely. The complete call
> graph looks like:
> 
>   git_config_from_file
>     -> git_parse_file
>       -> get_next_char
>       -> get_value
>           -> get_next_char
>           -> parse_value
>               -> get_next_char
>       -> get_base_var
>           -> get_next_char
>           -> get_extended_base_var
>               -> get_next_char
> 
> That is, every path to get_next_char happens while we are in
> git_config_from_file, and that function guarantees that cf = &top, and
> that top.f != NULL.  We do not have to even do any analysis of the
> conditions for each call, because we never change "cf" nor "top.f"
> except when we set them in git_config_from_file.

Ok if you say so I will do that :-). I was thinking about adding a patch
that would remove cf as a global variable and explicitely pass it down
to get_next_char. That makes it more obvious that it actually is != NULL.
Looking at your callgraph I do not think its that much work. What do you
think?

BTW, how did you generate this callgraph? Do you have a nice tool for that?

Cheers Heiko

Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:
> On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> > On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> > 
> > > The only location where cf is set in this file is in do_config_from().
> > > This function has only one callsite which is config_from_file(). In
> > > config_from_file() its ensured that the f member is set to non-zero.
> > > [...]
> > > -	if (cf && ((f = cf->f) != NULL)) {
> > > +	if (cf) {
> > 
> > I still think we can drop this conditional entirely. The complete call
> > graph looks like:
> > 
> >   git_config_from_file
> >     -> git_parse_file
> >       -> get_next_char
> >       -> get_value
> >           -> get_next_char
> >           -> parse_value
> >               -> get_next_char
> >       -> get_base_var
> >           -> get_next_char
> >           -> get_extended_base_var
> >               -> get_next_char
> > 
> > That is, every path to get_next_char happens while we are in
> > git_config_from_file, and that function guarantees that cf = &top, and
> > that top.f != NULL.  We do not have to even do any analysis of the
> > conditions for each call, because we never change "cf" nor "top.f"
> > except when we set them in git_config_from_file.
> 
> Ok if you say so I will do that :-). I was thinking about adding a patch
> that would remove cf as a global variable and explicitely pass it down
> to get_next_char. That makes it more obvious that it actually is != NULL.
> Looking at your callgraph I do not think its that much work. What do you
> think?

I just had a look and unfortunately there are other functions that use
this variable (namely handle_path_include) for which its not that easy
to pass this in. So I will just drop the check here.

Cheers Heiko

Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source

[Heiko Voigt]

On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:
> 
> > diff --git a/config.c b/config.c
> > index f55c43d..fe1c0e5 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -10,20 +10,42 @@
> >  #include "strbuf.h"
> >  #include "quote.h"
> >  
> > -typedef struct config_file {
> > -	struct config_file *prev;
> > -	FILE *f;
> > +struct config_source {
> > +	struct config_source *prev;
> > +	void *data;
> 
> Would a union be more appropriate here? We do not ever have to pass it
> directly as a parameter, since we pass the "struct config_source" to the
> method functions.
> 
> It's still possible to screw up using a union, but it's slightly harder
> than screwing up using a void pointer. And I do not think we need the
> run-time flexibility offered by the void pointer in this case.

No we do not need the void pointer flexibility. But that means every new
source would need to add to this union. Junio complained about that fact
when I first added the extra members directly to the struct. A union
does not waste that much space and we get some seperation using the
union members. Since this struct is local only I think that should be
ok.

> > +static int config_file_fgetc(struct config_source *conf)
> > +{
> > +	FILE *f = conf->data;
> > +	return fgetc(f);
> > +}
> 
> This could become just:
> 
>   return fgetc(conf->u.f);
> 
> and so forth (might it make sense to give "f" a more descriptive name,
> as we are adding other sources?).

Will change that.

Cheers Heiko

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

[Heiko Voigt]

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

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

Re: [PATCH v2 1/4] config: factor out config file stack management

[Jeff King]

On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:

> > Can we throw in a comment at the top here with the expected usage? In
> > particular, do_config_from is expecting the caller to have filled in
> > certain fields (at this point, top->f and top->name), but there is
> > nothing to make that clear.
> 
> Of course. Will do that in the next iteration. How about I squash this in:
> [...]
> +/* The fields data, name and the source specific callbacks of top need
> + * to be initialized before calling this function.
> + */
>  static int do_config_from_source(struct config_source *top, config_fn_t fn, voi

I think that is OK, but it may be even better to list the fields by
name. Also, our multi-line comment style is:

  /*
   * Multi-line comment.
   */


> I would add that to the third patch:
> 
> 	config: make parsing stack struct independent from actual data source
> 
> because that contains the final modification to config_file/config_source.

It does not matter to the end result, but I find it helps with reviewing
when the comment is added along with the function, and then expanded as
the function is changed. It helps to understand the effects of later
patches if they need to tweak comments.

I do not care that much in this instance, since we have already
discussed it, and I know what is going on, though.

-Peff

Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Jeff King]

On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:

> > That is, every path to get_next_char happens while we are in
> > git_config_from_file, and that function guarantees that cf = &top, and
> > that top.f != NULL.  We do not have to even do any analysis of the
> > conditions for each call, because we never change "cf" nor "top.f"
> > except when we set them in git_config_from_file.
> 
> Ok if you say so I will do that :-). I was thinking about adding a patch
> that would remove cf as a global variable and explicitely pass it down
> to get_next_char. That makes it more obvious that it actually is != NULL.
> Looking at your callgraph I do not think its that much work. What do you
> think?

Yeah, I think that makes it more obvious what is going on, but you will
run into trouble if you ever want that information to cross the "void *"
boundary of a config callback, as adding a new parameter there is hard.

The only place we do that now is for git_config_include, and I think you
could get by with stuffing it into the config_include_data struct (which
is already there to deal with this problem).

It would also make something like this patch hard or impossible:

  http://article.gmane.org/gmane.comp.version-control.git/190267

as it wants to access "cf" from arbitrary callbacks. That is not a patch
that is actively under consideration, but I had considered polishing it
up at some point.

> BTW, how did you generate this callgraph? Do you have a nice tool for that?

I just did it manually in vi, working backwards from every instance of
get_next_char, as it is not that big a graph. I suspect something like
cscope could make it easier, but I don't know of any tool that will
build the graph automatically (it would not be that hard from the data
cscope has, though, so I wouldn't be surprised if such a tool exists).

-Peff

Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

[Jeff King]

On Tue, Mar 12, 2013 at 05:16:20PM +0100, Heiko Voigt wrote:

> > Ok if you say so I will do that :-). I was thinking about adding a patch
> > that would remove cf as a global variable and explicitely pass it down
> > to get_next_char. That makes it more obvious that it actually is != NULL.
> > Looking at your callgraph I do not think its that much work. What do you
> > think?
> 
> I just had a look and unfortunately there are other functions that use
> this variable (namely handle_path_include) for which its not that easy
> to pass this in. So I will just drop the check here.

Ah, I should have read all your responses before answering the last
email. I think handle_path_include is surmountable, and I started to do
a proof-of-concept patch, but there's a harder one: die_bad_config,
which is called from git_config_int and friends. Getting the "cf"
pointer there is hard, because you would have to update every config
callback that uses git_config_int. Not worth it, I think.

Thanks for looking, though.

-Peff

Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source

[Jeff King]

On Tue, Mar 12, 2013 at 05:27:35PM +0100, Heiko Voigt wrote:

> > Would a union be more appropriate here? We do not ever have to pass it
> > directly as a parameter, since we pass the "struct config_source" to the
> > method functions.
> > 
> > It's still possible to screw up using a union, but it's slightly harder
> > than screwing up using a void pointer. And I do not think we need the
> > run-time flexibility offered by the void pointer in this case.
> 
> No we do not need the void pointer flexibility. But that means every new
> source would need to add to this union. Junio complained about that fact
> when I first added the extra members directly to the struct. A union
> does not waste that much space and we get some seperation using the
> union members. Since this struct is local only I think that should be
> ok.

Right. I think that is not a big deal, as we are not exposing this
struct outside of the config.c; any additions would need to add a new
git_config_from_foo function, anyway.

-Peff

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

[Jeff King]

On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:

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

I am on the fence. I do not want to create more work for you, but I do
think it may come in handy, if only for doing submodule things from
shell. And it is hopefully not a very large patch. I'd say try it, and
if starts looking like it will be very ugly, the right thing may be to
leave it until somebody really wants it.

-Peff

Re: [PATCH v2 1/4] config: factor out config file stack management

[Heiko Voigt]

On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote:
> On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:
> 
> > > Can we throw in a comment at the top here with the expected usage? In
> > > particular, do_config_from is expecting the caller to have filled in
> > > certain fields (at this point, top->f and top->name), but there is
> > > nothing to make that clear.
> > 
> > Of course. Will do that in the next iteration. How about I squash this in:
> > [...]
> > +/* The fields data, name and the source specific callbacks of top need
> > + * to be initialized before calling this function.
> > + */
> >  static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
> 
> I think that is OK, but it may be even better to list the fields by
> name. Also, our multi-line comment style is:
> 
>   /*
>    * Multi-line comment.
>    */

Ok will do both.

> > I would add that to the third patch:
> > 
> > 	config: make parsing stack struct independent from actual data source
> > 
> > because that contains the final modification to config_file/config_source.
> 
> It does not matter to the end result, but I find it helps with reviewing
> when the comment is added along with the function, and then expanded as
> the function is changed. It helps to understand the effects of later
> patches if they need to tweak comments.

To make the series more clear to others who read it later, I will add
the comment from the beginning.

Cheers Heiko

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

[Heiko Voigt]

On Tue, Mar 12, 2013 at 03:29:59PM -0400, Jeff King wrote:
> On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:
> 
> > > 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.
> 
> I am on the fence. I do not want to create more work for you, but I do
> think it may come in handy, if only for doing submodule things from
> shell. And it is hopefully not a very large patch. I'd say try it, and
> if starts looking like it will be very ugly, the right thing may be to
> leave it until somebody really wants it.

Thats what I will do: Try it and see where I get.

Cheers Heiko

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

[Jeff King]

On Thu, Mar 14, 2013 at 07:39:33AM +0100, Heiko Voigt wrote:

> > I am on the fence. I do not want to create more work for you, but I do
> > think it may come in handy, if only for doing submodule things from
> > shell. And it is hopefully not a very large patch. I'd say try it, and
> > if starts looking like it will be very ugly, the right thing may be to
> > leave it until somebody really wants it.
> 
> Thats what I will do: Try it and see where I get.

I looked into this a little. The first sticking point is that
git_config_with_options needs to support the alternate source. Here's a
sketch of what I think the solution should look like, on top of your
patches.

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
 
 static int use_global_config, use_system_config, use_local_config;
 static const char *given_config_file;
+static const char *given_config_blob;
 static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config file")),
 	OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config file")),
 	OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given config file")),
+	OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read config from given blob object")),
 	OPT_GROUP(N_("Action")),
 	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
 	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
 	}
 
 	git_config_with_options(collect_config, &values,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	ret = !values.nr;
 
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config_with_options(git_get_color_config, NULL,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	if (!get_color_found && def_color)
 		color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	if (get_colorbool_found < 0) {
 		if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
 		return get_colorbool_found ? 0 : 1;
 }
 
+static void check_blob_write(void)
+{
+	if (given_config_blob)
+		die("writing config blobs is not supported");
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			     builtin_config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {
+	if (use_global_config + use_system_config + use_local_config +
+	    !!given_config_file + !!given_config_blob > 1) {
 		error("only one config file at a time.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
 					    given_config_file,
+					    given_config_blob,
 					    respect_includes) < 0) {
 			if (given_config_file)
 				die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (!given_config_file && nongit)
 			die("not in a git directory");
+		if (given_config_blob)
+			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
 		launch_editor(given_config_file ?
 			      given_config_file : git_path("config"),
@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_SET) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		ret = git_config_set_in_file(given_config_file, argv[0], value);
@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
+		check_blob_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, argv[2], 0);
 	}
 	else if (actions == ACTION_ADD) {
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, "^$", 0);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
+		check_blob_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_value(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
+		check_blob_write();
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file(given_config_file,
@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 						      argv[0], NULL);
 	}
 	else if (actions == ACTION_UNSET_ALL) {
+		check_blob_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], NULL, argv[1], 1);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], argv[1]);
@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], NULL);
diff --git a/cache.h b/cache.h
index a2621fa..9db5f74 100644
--- a/cache.h
+++ b/cache.h
@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
-				   const char *filename, int respect_includes);
+				   const char *filename,
+				   const char *blob_ref,
+				   int respect_includes);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index b8c8640..35aa8e2 100644
--- a/config.c
+++ b/config.c
@@ -1073,8 +1073,34 @@ int git_config_with_options(config_fn_t fn, void *data,
 	return ret == 0 ? found : ret;
 }
 
+static int read_blob_reference(const char *ref,
+			       struct strbuf *sb)
+{
+	unsigned char sha1[20];
+	enum object_type type;
+	void *buf;
+	unsigned long size;
+
+	if (get_sha1(ref, sha1) < 0)
+		return error("unable to resolve config blob '%s'", ref);
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to load config blob object '%s'", ref);
+	if (type != OBJ_BLOB) {
+		free(buf);
+		return error("reference '%s' does not point to a blob", ref);
+	}
+
+	/* read_sha1_file always NUL-terminates for us, so size+1 is OK */
+	strbuf_attach(sb, buf, size, size+1);
+	return 0;
+}
+
 int git_config_with_options(config_fn_t fn, void *data,
-			    const char *filename, int respect_includes)
+			    const char *filename,
+			    const char *blob_ref,
+			    int respect_includes)
 {
 	char *repo_config = NULL;
 	int ret;
@@ -1093,6 +1119,15 @@ int git_config_with_options(config_fn_t fn, void *data,
 	 */
 	if (filename)
 		return git_config_from_file(fn, filename, data);
+	else if (blob_ref) {
+		struct strbuf buf = STRBUF_INIT;
+		if (read_blob_reference(blob_ref, &buf) < 0)
+			return -1;
+
+		ret = git_config_from_strbuf(fn, blob_ref, &buf, data);
+		strbuf_release(&buf);
+		return ret;
+	}
 
 	repo_config = git_pathdup("config");
 	ret = git_config_early(fn, data, repo_config);
@@ -1103,7 +1138,7 @@ int git_config(config_fn_t fn, void *data)
 
 int git_config(config_fn_t fn, void *data)
 {
-	return git_config_with_options(fn, data, NULL, 1);
+	return git_config_with_options(fn, data, NULL, NULL, 1);
 }
 
 /*

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

[Jeff King]

On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:

> I looked into this a little. The first sticking point is that
> git_config_with_options needs to support the alternate source. Here's a
> sketch of what I think the solution should look like, on top of your
> patches.

Here it is again, with two changes:

  1. Rather than handling the blob lookup inline in
     git_config_with_options, it adds direct functions for reading
     config from blob sha1s and blob references. I think this should
     make it easier to reuse when you are trying to read .gitmodules
     from C code.

  2. It adds some basic tests.

I'll leave it here for tonight. The next step would be to rebase it on
your modified series (in particular, I think git_config_from_strbuf
should become git_config_from_buf, which will impact this).

Feel free to use, pick apart, rewrite, or discard as you see fit for
your series.

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
 
 static int use_global_config, use_system_config, use_local_config;
 static const char *given_config_file;
+static const char *given_config_blob;
 static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config file")),
 	OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config file")),
 	OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given config file")),
+	OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read config from given blob object")),
 	OPT_GROUP(N_("Action")),
 	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
 	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
 	}
 
 	git_config_with_options(collect_config, &values,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	ret = !values.nr;
 
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config_with_options(git_get_color_config, NULL,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	if (!get_color_found && def_color)
 		color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
-				given_config_file, respect_includes);
+				given_config_file, given_config_blob,
+				respect_includes);
 
 	if (get_colorbool_found < 0) {
 		if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
 		return get_colorbool_found ? 0 : 1;
 }
 
+static void check_blob_write(void)
+{
+	if (given_config_blob)
+		die("writing config blobs is not supported");
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			     builtin_config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {
+	if (use_global_config + use_system_config + use_local_config +
+	    !!given_config_file + !!given_config_blob > 1) {
 		error("only one config file at a time.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
 					    given_config_file,
+					    given_config_blob,
 					    respect_includes) < 0) {
 			if (given_config_file)
 				die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (!given_config_file && nongit)
 			die("not in a git directory");
+		if (given_config_blob)
+			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
 		launch_editor(given_config_file ?
 			      given_config_file : git_path("config"),
@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_SET) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		ret = git_config_set_in_file(given_config_file, argv[0], value);
@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
+		check_blob_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, argv[2], 0);
 	}
 	else if (actions == ACTION_ADD) {
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, "^$", 0);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
+		check_blob_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_value(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
+		check_blob_write();
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file(given_config_file,
@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 						      argv[0], NULL);
 	}
 	else if (actions == ACTION_UNSET_ALL) {
+		check_blob_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], NULL, argv[1], 1);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], argv[1]);
@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
 		int ret;
+		check_blob_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], NULL);
diff --git a/cache.h b/cache.h
index a2621fa..9db5f74 100644
--- a/cache.h
+++ b/cache.h
@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
-				   const char *filename, int respect_includes);
+				   const char *filename,
+				   const char *blob_ref,
+				   int respect_includes);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index b8c8640..5c492a8 100644
--- a/config.c
+++ b/config.c
@@ -1009,6 +1009,47 @@ int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strb
 	return do_config_from_source(&top, fn, data);
 }
 
+static int git_config_from_blob_sha1(config_fn_t fn,
+				     const char *name,
+				     const unsigned char *sha1,
+				     void *data)
+{
+	struct strbuf sb = STRBUF_INIT;
+	enum object_type type;
+	void *buf;
+	unsigned long size;
+	int ret;
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to load config blob object '%s'", name);
+	if (type != OBJ_BLOB) {
+		free(buf);
+		return error("reference '%s' does not point to a blob", name);
+	}
+
+	/*
+	 * read_sha1_file always NUL-terminates for us, so size+1 is OK;
+	 * we could skip this step if we had git_config_from_buf
+	 */
+	strbuf_attach(&sb, buf, size, size+1);
+	ret = git_config_from_strbuf(fn, name, &sb, data);
+	strbuf_release(&sb);
+
+	return ret;
+}
+
+static int git_config_from_blob_ref(config_fn_t fn,
+				    const char *name,
+				    void *data)
+{
+	unsigned char sha1[20];
+
+	if (get_sha1(name, sha1) < 0)
+		return error("unable to resolve config blob '%s'", name);
+	return git_config_from_blob_sha1(fn, name, sha1, data);
+}
+
 const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;
@@ -1074,7 +1115,9 @@ int git_config_with_options(config_fn_t fn, void *data,
 }
 
 int git_config_with_options(config_fn_t fn, void *data,
-			    const char *filename, int respect_includes)
+			    const char *filename,
+			    const char *blob_ref,
+			    int respect_includes)
 {
 	char *repo_config = NULL;
 	int ret;
@@ -1093,6 +1136,8 @@ int git_config_with_options(config_fn_t fn, void *data,
 	 */
 	if (filename)
 		return git_config_from_file(fn, filename, data);
+	else if (blob_ref)
+		return git_config_from_blob_ref(fn, blob_ref, data);
 
 	repo_config = git_pathdup("config");
 	ret = git_config_early(fn, data, repo_config);
@@ -1103,7 +1148,7 @@ int git_config(config_fn_t fn, void *data)
 
 int git_config(config_fn_t fn, void *data)
 {
-	return git_config_with_options(fn, data, NULL, 1);
+	return git_config_with_options(fn, data, NULL, NULL, 1);
 }
 
 /*
diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index e69de29..141ca21 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+
+test_description='support for reading config from a blob'
+. ./test-lib.sh
+
+test_expect_success 'create config blob' '
+	cat >config <<-\EOF &&
+	[some]
+		value = 1
+	EOF
+	git add config &&
+	git commit -m foo
+'
+
+test_expect_success 'list config blob contents' '
+	echo some.value=1 >expect &&
+	git config --blob=HEAD:config --list >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fetch value from blob' '
+	echo true >expect &&
+	git config --blob=HEAD:config --bool some.value >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'reading from blob and file is an error' '
+	test_must_fail git config --blob=HEAD:config --system --list
+'
+
+test_expect_success 'reading from missing ref is an error' '
+	test_must_fail git config --blob=HEAD:doesnotexist --list
+'
+
+test_expect_success 'reading from non-blob is an error' '
+	test_must_fail git config --blob=HEAD --list
+'
+
+test_expect_success 'setting a value in a blob is an error' '
+	test_must_fail git config --blob=HEAD:config some.value foo
+'
+
+test_expect_success 'deleting a value in a blob is an error' '
+	test_must_fail git config --blob=HEAD:config --unset some.value
+'
+
+test_expect_success 'editing a blob is an error' '
+	test_must_fail git config --blob=HEAD:config --edit
+'
+
+test_expect_success 'parse errors in blobs are properly attributed' '
+	cat >config <<-\EOF &&
+	[some]
+		value = "
+	EOF
+	git add config &&
+	git commit -m broken &&
+
+	test_must_fail git config --blob=HEAD:config some.value 2>err &&
+
+	# just grep for our token as the exact error message is likely to
+	# change or be internationalized
+	grep "HEAD:config" err
+'
+
+test_done

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

[Heiko Voigt]

Hi Peff,

On Thu, Mar 14, 2013 at 03:39:14AM -0400, Jeff King wrote:
> On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:
>
> > I looked into this a little. The first sticking point is that
> > git_config_with_options needs to support the alternate source. Here's a
> > sketch of what I think the solution should look like, on top of your
> > patches.
>
> Here it is again, with two changes:
>
>   1. Rather than handling the blob lookup inline in
>      git_config_with_options, it adds direct functions for reading
>      config from blob sha1s and blob references. I think this should
>      make it easier to reuse when you are trying to read .gitmodules
>      from C code.
>
>   2. It adds some basic tests.
>
> I'll leave it here for tonight. The next step would be to rebase it on
> your modified series (in particular, I think git_config_from_strbuf
> should become git_config_from_buf, which will impact this).
>
> Feel free to use, pick apart, rewrite, or discard as you see fit for
> your series.

Sorry for the late reply, I was not online during the last days. Thanks 
a lot for this. I will use it in the next iteration of the series.

Cheers Heiko