[PATCH v4 0/5] allow more sources for config values

[PATCH v4 0/5] allow more sources for config values

[Heiko Voigt]

Hi,

all the comments to the last iteration[1] incorporated.

You can also find this on my github[2]. This is only for review. I will
resubmit this once 1.8.3 is out.

Cheers Heiko

[1] http://article.gmane.org/gmane.comp.version-control.git/223743
[2] https://github.com/hvoigt/git/commits/hv/strbuf_config_parsing-series4

Heiko Voigt (5):
  config: factor out config file stack management
  config: drop cf validity check in get_next_char()
  config: make parsing stack struct independent from actual data source
  teach config --blob option to parse config from database
  do not die when error in config parsing of buf occurs

 builtin/config.c       |  31 ++++++-
 cache.h                |   6 +-
 config.c               | 217 ++++++++++++++++++++++++++++++++++++++-----------
 t/t1307-config-blob.sh |  70 ++++++++++++++++
 4 files changed, 271 insertions(+), 53 deletions(-)
 create mode 100755 t/t1307-config-blob.sh

-- 
1.8.3.rc1.53.g0126843

[PATCH v4 1/5] 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 | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/config.c b/config.c
index aefd80b..f0494f3 100644
--- a/config.c
+++ b/config.c
@@ -896,6 +896,32 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	return 0;
 }
 
+/*
+ * The fields f and name of top need to be initialized before calling
+ * this function.
+ */
+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 +931,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.3.rc1.53.g0126843

[PATCH v4 2/5] config: drop cf validity check in get_next_char()

[Heiko Voigt]

The global variable cf is set with an initialized value in all codepaths before
calling this function.

The complete call graph looks like this:

  git_config_from_file
    -> do_config_from
      -> 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

The variable is initialized in do_config_from.

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

diff --git a/config.c b/config.c
index f0494f3..046642b 100644
--- a/config.c
+++ b/config.c
@@ -169,26 +169,23 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 static int get_next_char(void)
 {
 	int c;
-	FILE *f;
+	FILE *f = cf->f;
 
-	c = '\n';
-	if (cf && ((f = cf->f) != NULL)) {
+	c = fgetc(f);
+	if (c == '\r') {
+		/* DOS like systems */
 		c = fgetc(f);
-		if (c == '\r') {
-			/* DOS like systems */
-			c = fgetc(f);
-			if (c != '\n') {
-				ungetc(c, f);
-				c = '\r';
-			}
-		}
-		if (c == '\n')
-			cf->linenr++;
-		if (c == EOF) {
-			cf->eof = 1;
-			c = '\n';
+		if (c != '\n') {
+			ungetc(c, f);
+			c = '\r';
 		}
 	}
+	if (c == '\n')
+		cf->linenr++;
+	if (c == EOF) {
+		cf->eof = 1;
+		c = '\n';
+	}
 	return c;
 }
 
-- 
1.8.3.rc1.53.g0126843

[PATCH v4 3/5] 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 | 66 ++++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 22 deletions(-)

diff --git a/config.c b/config.c
index 046642b..e21c24c 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,41 @@
 #include "strbuf.h"
 #include "quote.h"
 
-typedef struct config_file {
-	struct config_file *prev;
-	FILE *f;
+struct config_source {
+	struct config_source *prev;
+	union {
+		FILE *file;
+	} u;
 	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)
+{
+	return fgetc(conf->u.file);
+}
+
+static int config_file_ungetc(int c, struct config_source *conf)
+{
+	return ungetc(c, conf->u.file);
+}
+
+static long config_file_ftell(struct config_source *conf)
+{
+	return ftell(conf->u.file);
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -168,15 +189,13 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 
 static int get_next_char(void)
 {
-	int c;
-	FILE *f = cf->f;
+	int c = cf->fgetc(cf);
 
-	c = fgetc(f);
 	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';
 		}
 	}
@@ -336,7 +355,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;
@@ -894,10 +913,11 @@ int git_default_config(const char *var, const char *value, void *dummy)
 }
 
 /*
- * The fields f and name of top need to be initialized before calling
+ * All source specific fields in the union, name and the callbacks
+ * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
-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;
 
@@ -909,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);
@@ -926,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.u.file = 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);
 	}
@@ -1064,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:
@@ -1076,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;
@@ -1103,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.3.rc1.53.g0126843

[PATCH v4 4/5] teach config --blob option to parse config from database

[Heiko Voigt]

This can be used to read configuration values directly from git's
database. For example it is useful for reading to be checked out
.gitmodules files directly from the database.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/config.c       | 31 +++++++++++++++---
 cache.h                |  6 +++-
 config.c               | 86 ++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t1307-config-blob.sh | 70 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 7 deletions(-)
 create mode 100755 t/t1307-config-blob.sh

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 char term = '\n';
 
 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 94ca1ac..be48c4b 100644
--- a/cache.h
+++ b/cache.h
@@ -1142,11 +1142,15 @@ 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_buf(config_fn_t fn, const char *name,
+			       const char *buf, size_t len, 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 *);
 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 e21c24c..23b14f4 100644
--- a/config.c
+++ b/config.c
@@ -14,6 +14,11 @@ struct config_source {
 	struct config_source *prev;
 	union {
 		FILE *file;
+		struct config_buf {
+			const char *buf;
+			size_t len;
+			size_t pos;
+		} buf;
 	} u;
 	const char *name;
 	int linenr;
@@ -45,6 +50,28 @@ static long config_file_ftell(struct config_source *conf)
 	return ftell(conf->u.file);
 }
 
+
+static int config_buf_fgetc(struct config_source *conf)
+{
+	if (conf->u.buf.pos < conf->u.buf.len)
+		return conf->u.buf.buf[conf->u.buf.pos++];
+
+	return EOF;
+}
+
+static int config_buf_ungetc(int c, struct config_source *conf)
+{
+	if (conf->u.buf.pos > 0)
+		return conf->u.buf.buf[--conf->u.buf.pos];
+
+	return EOF;
+}
+
+static long config_buf_ftell(struct config_source *conf)
+{
+	return conf->u.buf.pos;
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -961,6 +988,57 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	return ret;
 }
 
+int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
+			size_t len, void *data)
+{
+	struct config_source top;
+
+	top.u.buf.buf = buf;
+	top.u.buf.len = len;
+	top.u.buf.pos = 0;
+	top.name = name;
+	top.fgetc = config_buf_fgetc;
+	top.ungetc = config_buf_ungetc;
+	top.ftell = config_buf_ftell;
+
+	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)
+{
+	enum object_type type;
+	char *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);
+	}
+
+	ret = git_config_from_buf(fn, name, buf, size, data);
+	free(buf);
+
+	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;
@@ -1026,7 +1104,9 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 }
 
 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;
@@ -1045,6 +1125,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);
@@ -1055,7 +1137,7 @@ int git_config_with_options(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
new file mode 100755
index 0000000..fdc257e
--- /dev/null
+++ b/t/t1307-config-blob.sh
@@ -0,0 +1,70 @@
+#!/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 non-existing value from blob is an error' '
+	test_must_fail git config --blob=HEAD:config non.existing
+'
+
+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
-- 
1.8.3.rc1.53.g0126843

[PATCH v4 5/5] do not die when error in config parsing of buf occurs

[Heiko Voigt]

If a config parsing error in a file occurs we can die and let the user
fix the issue. This is different for the buf parsing function since it
can be used to parse blobs of .gitmodules files. If a parsing error
occurs here we should proceed since otherwise a database containing such
an error in a single revision could be rendered unusable.

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

diff --git a/config.c b/config.c
index 23b14f4..a8d3dcf 100644
--- a/config.c
+++ b/config.c
@@ -21,6 +21,7 @@ struct config_source {
 		} buf;
 	} u;
 	const char *name;
+	int die_on_error;
 	int linenr;
 	int eof;
 	struct strbuf value;
@@ -442,7 +443,10 @@ static int git_parse_source(config_fn_t fn, void *data)
 		if (get_value(fn, data, var) < 0)
 			break;
 	}
-	die("bad config file line %d in %s", cf->linenr, cf->name);
+	if (cf->die_on_error)
+		die("bad config file line %d in %s", cf->linenr, cf->name);
+	else
+		return error("bad config file line %d in %s", cf->linenr, cf->name);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -940,7 +944,7 @@ int git_default_config(const char *var, const char *value, void *dummy)
 }
 
 /*
- * All source specific fields in the union, name and the callbacks
+ * All source specific fields in the union, die_on_error, name and the callbacks
  * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
@@ -977,6 +981,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 		top.u.file = f;
 		top.name = filename;
+		top.die_on_error = 1;
 		top.fgetc = config_file_fgetc;
 		top.ungetc = config_file_ungetc;
 		top.ftell = config_file_ftell;
@@ -997,6 +1002,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
 	top.u.buf.len = len;
 	top.u.buf.pos = 0;
 	top.name = name;
+	top.die_on_error = 0;
 	top.fgetc = config_buf_fgetc;
 	top.ungetc = config_buf_ungetc;
 	top.ftell = config_buf_ftell;
-- 
1.8.3.rc1.53.g0126843

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

>  /*
> - * The fields f and name of top need to be initialized before calling
> + * All source specific fields in the union, name and the callbacks
> + * fgetc, ungetc, ftell of top need to be initialized before calling
>   * this function.
>   */
> -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)

This renaming may have made sense if we were to have many different
do_config_from_$type functions for different types of source, but as
this patch introduces a nice "config_source" abstraction, I do not
think it is unnecessary. Shortening do_config_from() to do_config()
may make more sense, if anything.

But that is a very minor point, as this is entirely internal with a
single caller.

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

[Heiko Voigt]

On Sun, May 12, 2013 at 09:56:39PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> >  /*
> > - * The fields f and name of top need to be initialized before calling
> > + * All source specific fields in the union, name and the callbacks
> > + * fgetc, ungetc, ftell of top need to be initialized before calling
> >   * this function.
> >   */
> > -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)
> 
> This renaming may have made sense if we were to have many different
> do_config_from_$type functions for different types of source, but as
> this patch introduces a nice "config_source" abstraction, I do not
> think it is unnecessary. Shortening do_config_from() to do_config()
> may make more sense, if anything.
> 
> But that is a very minor point, as this is entirely internal with a
> single caller.

Did you really intent this double negation: "..., I do not think it
is unnecessary." ? The rest of the paragraph sounds like you would
think the rename is actually "not necessary". I thought I recalled that
Jeff asked me to change the name but I can not find the email, so maybe
its just my wrong memory. I am happy to drop the rename here, if thats
what you meant.

Cheers Heiko

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> On Sun, May 12, 2013 at 09:56:39PM -0700, Junio C Hamano wrote:
>> Heiko Voigt <hvoigt@hvoigt.net> writes:
>> 
>> >  /*
>> > - * The fields f and name of top need to be initialized before calling
>> > + * All source specific fields in the union, name and the callbacks
>> > + * fgetc, ungetc, ftell of top need to be initialized before calling
>> >   * this function.
>> >   */
>> > -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)
>> 
>> This renaming may have made sense if we were to have many different
>> do_config_from_$type functions for different types of source, but as
>> this patch introduces a nice "config_source" abstraction, I do not
>> think it is unnecessary. Shortening do_config_from() to do_config()
>> may make more sense, if anything.
>> 
>> But that is a very minor point, as this is entirely internal with a
>> single caller.
>
> Did you really intent this double negation: "..., I do not think it
> is unnecessary."

Eh, rather "I think it is unnecessary" or "I do not think it is
necessary".

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

[Jeff King]

On Mon, May 13, 2013 at 04:04:35PM +0200, Heiko Voigt wrote:

> > > -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)
> [...]
> I thought I recalled that Jeff asked me to change the name but I can
> not find the email, so maybe its just my wrong memory. I am happy to
> drop the rename here, if thats what you meant.

I think you are thinking of:

  http://thread.gmane.org/gmane.comp.version-control.git/217018/focus=217170

where I criticize the name. But I think the comment you added makes its
purpose much more clear, so the name is less important (and for the
record, I think "do_config" or "config_source_parse" would be fine,
too).

-Peff

Re: [PATCH v4 0/5] allow more sources for config values

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> This is only for review. I will resubmit this once 1.8.3 is out.

Ping?

No need to hurry, but just to make sure this didn't disappear from
everybody's radar.

Thanks.