{"thread":{"id":"33146","subject":"[PATCH v2 0/4] allow more sources for config values","startedAt":"2013-03-10T16:56:42Z","lastAt":"2013-03-18T14:18:28Z","messageCount":24,"participants":["Heiko Voigt","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"210980","messageId":"20130310165642.GA1136@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":null,"subject":"[PATCH v2 0/4] allow more sources for config values","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T16:56:42Z","receivedAt":"2013-03-10T16:56:42Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The following issues still exist:\n\n * Error handling: If this should be useful to interrogate configs from\n   the database during git operations we need a way to recover from\n   parsing errors instead of dying.\n\n * More tests ?\n\nThis is an update with the comments of the first iteration[1]\nincorporated.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/217018/\n\nHeiko Voigt (4):\n  config: factor out config file stack management\n  config: drop file pointer validity check in get_next_char()\n  config: make parsing stack struct independent from actual data source\n  teach config parsing to read from strbuf\n\n .gitignore             |   1 +\n Makefile               |   1 +\n cache.h                |   2 +\n config.c               | 143 ++++++++++++++++++++++++++++++++++++++-----------\n t/t1300-repo-config.sh |  24 +++++++++\n test-config.c          |  40 ++++++++++++++\n 6 files changed, 180 insertions(+), 31 deletions(-)\n create mode 100644 test-config.c\n\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210981","messageId":"20130310165752.GB1136@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130310165642.GA1136@sandbox-ub.fritz.box","subject":"[PATCH v2 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T16:57:53Z","receivedAt":"2013-03-10T16:57:53Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Because a config callback may start parsing a new file, the\nglobal context regarding the current config file is stored\nas a stack. Currently we only need to manage that stack from\ngit_config_from_file. Let's factor it out to allow new\nsources of config data.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n config.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex aefd80b..2c299dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -896,6 +896,28 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n+static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n+{\n+\tint ret;\n+\n+\t/* push config-file parsing state stack */\n+\ttop->prev = cf;\n+\ttop->linenr = 1;\n+\ttop->eof = 0;\n+\tstrbuf_init(&top->value, 1024);\n+\tstrbuf_init(&top->var, 1024);\n+\tcf = top;\n+\n+\tret = git_parse_file(fn, data);\n+\n+\t/* pop config-file parsing state stack */\n+\tstrbuf_release(&top->value);\n+\tstrbuf_release(&top->var);\n+\tcf = top->prev;\n+\n+\treturn ret;\n+}\n+\n int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n {\n \tint ret;\n@@ -905,22 +927,10 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tif (f) {\n \t\tconfig_file top;\n \n-\t\t/* push config-file parsing state stack */\n-\t\ttop.prev = cf;\n \t\ttop.f = f;\n \t\ttop.name = filename;\n-\t\ttop.linenr = 1;\n-\t\ttop.eof = 0;\n-\t\tstrbuf_init(&top.value, 1024);\n-\t\tstrbuf_init(&top.var, 1024);\n-\t\tcf = &top;\n-\n-\t\tret = git_parse_file(fn, data);\n-\n-\t\t/* pop config-file parsing state stack */\n-\t\tstrbuf_release(&top.value);\n-\t\tstrbuf_release(&top.var);\n-\t\tcf = top.prev;\n+\n+\t\tret = do_config_from(&top, fn, data);\n \n \t\tfclose(f);\n \t}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210982","messageId":"20130310165857.GC1136@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130310165642.GA1136@sandbox-ub.fritz.box","subject":"[PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T16:58:57Z","receivedAt":"2013-03-10T16:58:57Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The only location where cf is set in this file is in do_config_from().\nThis function has only one callsite which is config_from_file(). In\nconfig_from_file() its ensured that the f member is set to non-zero.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2c299dc..f55c43d 100644\n--- a/config.c\n+++ b/config.c\n@@ -169,10 +169,10 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n static int get_next_char(void)\n {\n \tint c;\n-\tFILE *f;\n \n \tc = '\\n';\n-\tif (cf && ((f = cf->f) != NULL)) {\n+\tif (cf) {\n+\t\tFILE *f = cf->f;\n \t\tc = fgetc(f);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210983","messageId":"20130310165940.GD1136@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130310165642.GA1136@sandbox-ub.fritz.box","subject":"[PATCH v2 3/4] config: make parsing stack struct independent from actual data source","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T16:59:40Z","receivedAt":"2013-03-10T16:59:40Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To simplify adding other sources we extract all functions needed for\nparsing into a list of callbacks. We implement those callbacks for the\ncurrent file parsing. A new source can implement its own set of callbacks.\n\nInstead of storing the concrete FILE pointer for parsing we store a void\npointer. A new source can use this to store its custom data.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n config.c | 63 +++++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 43 insertions(+), 20 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f55c43d..fe1c0e5 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,20 +10,42 @@\n #include \"strbuf.h\"\n #include \"quote.h\"\n \n-typedef struct config_file {\n-\tstruct config_file *prev;\n-\tFILE *f;\n+struct config_source {\n+\tstruct config_source *prev;\n+\tvoid *data;\n \tconst char *name;\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n \tstruct strbuf var;\n-} config_file;\n \n-static config_file *cf;\n+\tint (*fgetc)(struct config_source *c);\n+\tint (*ungetc)(int c, struct config_source *conf);\n+\tlong (*ftell)(struct config_source *c);\n+};\n+\n+static struct config_source *cf;\n \n static int zlib_compression_seen;\n \n+static int config_file_fgetc(struct config_source *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn fgetc(f);\n+}\n+\n+static int config_file_ungetc(int c, struct config_source *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn ungetc(c, f);\n+}\n+\n+static long config_file_ftell(struct config_source *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn ftell(f);\n+}\n+\n #define MAX_INCLUDE_DEPTH 10\n static const char include_depth_advice[] =\n \"exceeded maximum include depth (%d) while including\\n\"\n@@ -172,13 +194,12 @@ static int get_next_char(void)\n \n \tc = '\\n';\n \tif (cf) {\n-\t\tFILE *f = cf->f;\n-\t\tc = fgetc(f);\n+\t\tc = cf->fgetc(cf);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n-\t\t\tc = fgetc(f);\n+\t\t\tc = cf->fgetc(cf);\n \t\t\tif (c != '\\n') {\n-\t\t\t\tungetc(c, f);\n+\t\t\t\tcf->ungetc(c, cf);\n \t\t\t\tc = '\\r';\n \t\t\t}\n \t\t}\n@@ -339,7 +360,7 @@ static int get_base_var(struct strbuf *name)\n \t}\n }\n \n-static int git_parse_file(config_fn_t fn, void *data)\n+static int git_parse_source(config_fn_t fn, void *data)\n {\n \tint comment = 0;\n \tint baselen = 0;\n@@ -896,7 +917,7 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n-static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n+static int do_config_from_source(struct config_source *top, config_fn_t fn, void *data)\n {\n \tint ret;\n \n@@ -908,7 +929,7 @@ static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n \tstrbuf_init(&top->var, 1024);\n \tcf = top;\n \n-\tret = git_parse_file(fn, data);\n+\tret = git_parse_source(fn, data);\n \n \t/* pop config-file parsing state stack */\n \tstrbuf_release(&top->value);\n@@ -925,12 +946,15 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n \tret = -1;\n \tif (f) {\n-\t\tconfig_file top;\n+\t\tstruct config_source top;\n \n-\t\ttop.f = f;\n+\t\ttop.data = f;\n \t\ttop.name = filename;\n+\t\ttop.fgetc = config_file_fgetc;\n+\t\ttop.ungetc = config_file_ungetc;\n+\t\ttop.ftell = config_file_ftell;\n \n-\t\tret = do_config_from(&top, fn, data);\n+\t\tret = do_config_from_source(&top, fn, data);\n \n \t\tfclose(f);\n \t}\n@@ -1063,7 +1087,6 @@ static int store_aux(const char *key, const char *value, void *cb)\n {\n \tconst char *ep;\n \tsize_t section_len;\n-\tFILE *f = cf->f;\n \n \tswitch (store.state) {\n \tcase KEY_SEEN:\n@@ -1075,7 +1098,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\tstore.seen++;\n \t\t}\n \t\tbreak;\n@@ -1102,19 +1125,19 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n-\t\tstore.offset[store.seen] = ftell(f);\n+\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n \t\t} else {\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t      !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n-\t\t\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\t}\n \t\t}\n \t}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210984","messageId":"20130310170052.GE1136@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130310165642.GA1136@sandbox-ub.fritz.box","subject":"[PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T17:00:52Z","receivedAt":"2013-03-10T17:00:52Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"This can be used to read configuration values directly from gits\ndatabase.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n .gitignore             |  1 +\n Makefile               |  1 +\n cache.h                |  2 ++\n config.c               | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t1300-repo-config.sh | 24 ++++++++++++++++++++++++\n test-config.c          | 40 ++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 116 insertions(+)\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 6669bf0..386b7f2 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -178,6 +178,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex 26d3332..1a9ea10 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -541,6 +541,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/cache.h b/cache.h\nindex e493563..a2621fa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1128,6 +1128,8 @@ extern int update_server_info(int);\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n+extern int git_config_from_strbuf(config_fn_t fn, const char *name,\n+\t\t\t\t  struct strbuf *strbuf, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\ndiff --git a/config.c b/config.c\nindex fe1c0e5..b8c8640 100644\n--- a/config.c\n+++ b/config.c\n@@ -46,6 +46,37 @@ static long config_file_ftell(struct config_source *conf)\n \treturn ftell(f);\n }\n \n+struct config_strbuf {\n+\tstruct strbuf *strbuf;\n+\tint pos;\n+};\n+\n+static int config_strbuf_fgetc(struct config_source *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\n+\tif (str->pos < str->strbuf->len)\n+\t\treturn str->strbuf->buf[str->pos++];\n+\n+\treturn EOF;\n+}\n+\n+static int config_strbuf_ungetc(int c, struct config_source *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\n+\tif (str->pos > 0)\n+\t\treturn str->strbuf->buf[--str->pos];\n+\n+\treturn EOF;\n+}\n+\n+static long config_strbuf_ftell(struct config_source *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\treturn str->pos;\n+}\n+\n #define MAX_INCLUDE_DEPTH 10\n static const char include_depth_advice[] =\n \"exceeded maximum include depth (%d) while including\\n\"\n@@ -961,6 +992,23 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \treturn ret;\n }\n \n+int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)\n+{\n+\tstruct config_source top;\n+\tstruct config_strbuf str;\n+\n+\tstr.strbuf = strbuf;\n+\tstr.pos = 0;\n+\n+\ttop.data = &str;\n+\ttop.name = name;\n+\ttop.fgetc = config_strbuf_fgetc;\n+\ttop.ungetc = config_strbuf_ungetc;\n+\ttop.ftell = config_strbuf_ftell;\n+\n+\treturn do_config_from_source(&top, fn, data);\n+}\n+\n const char *git_etc_gitconfig(void)\n {\n \tstatic const char *system_wide;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c96fda..5103f66 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1087,4 +1087,28 @@ test_expect_success 'barf on incomplete string' '\n \tgrep \" line 3 \" error\n '\n \n+test_expect_success 'reading config from strbuf' '\n+\tcat >expect <<-\\EOF &&\n+\tvar: some.value, value: content\n+\tEOF\n+\ttest-config strbuf 12345:.gitmodules >actual <<-\\EOF &&\n+\t[some]\n+\t\tvalue = content\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading config from strbuf with error' '\n+\ttouch expect.out &&\n+\tcat >expect.err <<-\\EOF &&\n+\tfatal: bad config file line 2 in 12345:.gitmodules\n+\tEOF\n+\ttest_must_fail test-config strbuf 12345:.gitmodules >actual.out 2>actual.err <<-\\EOF &&\n+\t[some]\n+\t\tvalue = \"\n+\tEOF\n+\ttest_cmp expect.out actual.out &&\n+\ttest_cmp expect.err actual.err\n+'\n+\n test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..c650837\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,40 @@\n+#include \"cache.h\"\n+\n+static int config_strbuf(const char *var, const char *value, void *data)\n+{\n+\tprintf(\"var: %s, value: %s\\n\", var, value);\n+\n+\treturn 1;\n+}\n+\n+static void die_usage(int argc, char **argv)\n+{\n+\tfprintf(stderr, \"Usage: %s strbuf <name>\\n\", argv[0]);\n+\texit(1);\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tif (argc < 3)\n+\t\tdie_usage(argc, argv);\n+\n+\tif (!strcmp(argv[1], \"strbuf\")) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tconst char *name = argv[2];\n+\n+\t\twhile (strbuf_fread(&buf, 1024, stdin) == 1024)\n+\t\t\t;\n+\n+\t\tif (ferror(stdin))\n+\t\t\tdie(\"An error occurred while reading from stdin\");\n+\n+\t\tgit_config_from_strbuf(config_strbuf, name, &buf, NULL);\n+\t\tstrbuf_release(&buf);\n+\n+\t\treturn 0;\n+\t}\n+\n+\tdie_usage(argc, argv);\n+\n+\treturn 1;\n+}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"211111","messageId":"20130312105200.GC11340@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130310165752.GB1136@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 1/4] config: factor out config file stack management","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T10:52:00Z","receivedAt":"2013-03-12T10:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:\n\n> Because a config callback may start parsing a new file, the\n> global context regarding the current config file is stored\n> as a stack. Currently we only need to manage that stack from\n> git_config_from_file. Let's factor it out to allow new\n> sources of config data.\n>\n> [...]\n>\n> +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n> +{\n> +\tint ret;\n> +\n> +\t/* push config-file parsing state stack */\n> +\ttop->prev = cf;\n> +\ttop->linenr = 1;\n> +\ttop->eof = 0;\n> +\tstrbuf_init(&top->value, 1024);\n> +\tstrbuf_init(&top->var, 1024);\n> +\tcf = top;\n> +\n> +\tret = git_parse_file(fn, data);\n> +\n> +\t/* pop config-file parsing state stack */\n> +\tstrbuf_release(&top->value);\n> +\tstrbuf_release(&top->var);\n> +\tcf = top->prev;\n> +\n> +\treturn ret;\n> +}\n\nCan we throw in a comment at the top here with the expected usage? In\nparticular, do_config_from is expecting the caller to have filled in\ncertain fields (at this point, top->f and top->name), but there is\nnothing to make that clear.\n\n-Peff\n"},{"id":"211112","messageId":"20130312110003.GD11340@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130310165857.GC1136@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T11:00:03Z","receivedAt":"2013-03-12T11:00:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:\n\n> The only location where cf is set in this file is in do_config_from().\n> This function has only one callsite which is config_from_file(). In\n> config_from_file() its ensured that the f member is set to non-zero.\n> [...]\n> -\tif (cf && ((f = cf->f) != NULL)) {\n> +\tif (cf) {\n\nI still think we can drop this conditional entirely. The complete call\ngraph looks like:\n\n  git_config_from_file\n    -> git_parse_file\n      -> get_next_char\n      -> get_value\n          -> get_next_char\n          -> parse_value\n              -> get_next_char\n      -> get_base_var\n          -> get_next_char\n          -> get_extended_base_var\n              -> get_next_char\n\nThat is, every path to get_next_char happens while we are in\ngit_config_from_file, and that function guarantees that cf = &top, and\nthat top.f != NULL.  We do not have to even do any analysis of the\nconditions for each call, because we never change \"cf\" nor \"top.f\"\nexcept when we set them in git_config_from_file.\n\n-Peff\n"},{"id":"211113","messageId":"20130312110355.GE11340@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130310165940.GD1136@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T11:03:55Z","receivedAt":"2013-03-12T11:03:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:\n\n> diff --git a/config.c b/config.c\n> index f55c43d..fe1c0e5 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -10,20 +10,42 @@\n>  #include \"strbuf.h\"\n>  #include \"quote.h\"\n>  \n> -typedef struct config_file {\n> -\tstruct config_file *prev;\n> -\tFILE *f;\n> +struct config_source {\n> +\tstruct config_source *prev;\n> +\tvoid *data;\n\nWould a union be more appropriate here? We do not ever have to pass it\ndirectly as a parameter, since we pass the \"struct config_source\" to the\nmethod functions.\n\nIt's still possible to screw up using a union, but it's slightly harder\nthan screwing up using a void pointer. And I do not think we need the\nrun-time flexibility offered by the void pointer in this case.\n\n> +static int config_file_fgetc(struct config_source *conf)\n> +{\n> +\tFILE *f = conf->data;\n> +\treturn fgetc(f);\n> +}\n\nThis could become just:\n\n  return fgetc(conf->u.f);\n\nand so forth (might it make sense to give \"f\" a more descriptive name,\nas we are adding other sources?).\n\n-Peff\n"},{"id":"211114","messageId":"20130312111806.GF11340@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130310170052.GE1136@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T11:18:06Z","receivedAt":"2013-03-12T11:18:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:\n\n> This can be used to read configuration values directly from gits\n> database.\n> \n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n\nThis is lacking motivation. IIRC, the rest of the story is something\nlike \"...so we can read .gitmodules directly from the repo\" or something\nlike that?\n\n> +struct config_strbuf {\n> +\tstruct strbuf *strbuf;\n> +\tint pos;\n> +};\n>\n> +static int config_strbuf_fgetc(struct config_source *conf)\n> +{\n> +\tstruct config_strbuf *str = conf->data;\n\nYuck. If you used a union in the previous patch, then this could just go\ninline into the \"struct config_source\".\n\n> +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)\n\nShould this be a \"const struct strbuf *strbuf\"? For that matter, is\nthere any reason not to take a bare pointer/len combination? It seems\nlikely that callers would get the data from read_sha1_file, which means\nthey have to stuff it into a strbuf for no good reason.\n\n> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..c650837\n> --- /dev/null\n> +++ b/test-config.c\n> @@ -0,0 +1,40 @@\n\nI'm slightly \"meh\" on this test-config program.  Having to add a C test\nharness like this is a good indication that we are short-changing users\nof the shell API in favor of builtin C code.\n\nYour series does not actually add any callers of the new function. The\nobvious \"patch 5/4\" would be to plumb it into \"git config --blob\", and\nthen we can just directly test it there (there could be other callers\nbesides reading from a blob, of course, but I think the point of the\nseries is to head in that direction).\n\n-Peff\n"},{"id":"211135","messageId":"20130312154434.GB3021@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312105200.GC11340@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-12T15:44:35Z","receivedAt":"2013-03-12T15:44:35Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 06:52:00AM -0400, Jeff King wrote:\n> On Sun, Mar 10, 2013 at 05:57:53PM +0100, Heiko Voigt wrote:\n> \n> > Because a config callback may start parsing a new file, the\n> > global context regarding the current config file is stored\n> > as a stack. Currently we only need to manage that stack from\n> > git_config_from_file. Let's factor it out to allow new\n> > sources of config data.\n> >\n> > [...]\n> >\n> > +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/* push config-file parsing state stack */\n> > +\ttop->prev = cf;\n> > +\ttop->linenr = 1;\n> > +\ttop->eof = 0;\n> > +\tstrbuf_init(&top->value, 1024);\n> > +\tstrbuf_init(&top->var, 1024);\n> > +\tcf = top;\n> > +\n> > +\tret = git_parse_file(fn, data);\n> > +\n> > +\t/* pop config-file parsing state stack */\n> > +\tstrbuf_release(&top->value);\n> > +\tstrbuf_release(&top->var);\n> > +\tcf = top->prev;\n> > +\n> > +\treturn ret;\n> > +}\n> \n> Can we throw in a comment at the top here with the expected usage? In\n> particular, do_config_from is expecting the caller to have filled in\n> certain fields (at this point, top->f and top->name), but there is\n> nothing to make that clear.\n\nOf course. Will do that in the next iteration. How about I squash this in:\n\ndiff --git a/config.c b/config.c\nindex b8c8640..b7632c9 100644\n--- a/config.c\n+++ b/config.c\n@@ -948,6 +954,9 @@ int git_default_config(const char *var, const char *value, v\n        return 0;\n }\n \n+/* The fields data, name and the source specific callbacks of top need\n+ * to be initialized before calling this function.\n+ */\n static int do_config_from_source(struct config_source *top, config_fn_t fn, voi\n {\n        int ret;\n\n\nI would add that to the third patch:\n\n\tconfig: make parsing stack struct independent from actual data source\n\nbecause that contains the final modification to config_file/config_source.\n\nCheers Heiko\n"},{"id":"211136","messageId":"20130312160056.GB4472@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312110003.GD11340@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-12T16:00:56Z","receivedAt":"2013-03-12T16:00:56Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:\n> On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:\n> \n> > The only location where cf is set in this file is in do_config_from().\n> > This function has only one callsite which is config_from_file(). In\n> > config_from_file() its ensured that the f member is set to non-zero.\n> > [...]\n> > -\tif (cf && ((f = cf->f) != NULL)) {\n> > +\tif (cf) {\n> \n> I still think we can drop this conditional entirely. The complete call\n> graph looks like:\n> \n>   git_config_from_file\n>     -> git_parse_file\n>       -> get_next_char\n>       -> get_value\n>           -> get_next_char\n>           -> parse_value\n>               -> get_next_char\n>       -> get_base_var\n>           -> get_next_char\n>           -> get_extended_base_var\n>               -> get_next_char\n> \n> That is, every path to get_next_char happens while we are in\n> git_config_from_file, and that function guarantees that cf = &top, and\n> that top.f != NULL.  We do not have to even do any analysis of the\n> conditions for each call, because we never change \"cf\" nor \"top.f\"\n> except when we set them in git_config_from_file.\n\nOk if you say so I will do that :-). I was thinking about adding a patch\nthat would remove cf as a global variable and explicitely pass it down\nto get_next_char. That makes it more obvious that it actually is != NULL.\nLooking at your callgraph I do not think its that much work. What do you\nthink?\n\nBTW, how did you generate this callgraph? Do you have a nice tool for that?\n\nCheers Heiko\n"},{"id":"211139","messageId":"20130312161620.GA4551@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312160056.GB4472@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-12T16:16:20Z","receivedAt":"2013-03-12T16:16:20Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:\n> On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:\n> > On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:\n> > \n> > > The only location where cf is set in this file is in do_config_from().\n> > > This function has only one callsite which is config_from_file(). In\n> > > config_from_file() its ensured that the f member is set to non-zero.\n> > > [...]\n> > > -\tif (cf && ((f = cf->f) != NULL)) {\n> > > +\tif (cf) {\n> > \n> > I still think we can drop this conditional entirely. The complete call\n> > graph looks like:\n> > \n> >   git_config_from_file\n> >     -> git_parse_file\n> >       -> get_next_char\n> >       -> get_value\n> >           -> get_next_char\n> >           -> parse_value\n> >               -> get_next_char\n> >       -> get_base_var\n> >           -> get_next_char\n> >           -> get_extended_base_var\n> >               -> get_next_char\n> > \n> > That is, every path to get_next_char happens while we are in\n> > git_config_from_file, and that function guarantees that cf = &top, and\n> > that top.f != NULL.  We do not have to even do any analysis of the\n> > conditions for each call, because we never change \"cf\" nor \"top.f\"\n> > except when we set them in git_config_from_file.\n> \n> Ok if you say so I will do that :-). I was thinking about adding a patch\n> that would remove cf as a global variable and explicitely pass it down\n> to get_next_char. That makes it more obvious that it actually is != NULL.\n> Looking at your callgraph I do not think its that much work. What do you\n> think?\n\nI just had a look and unfortunately there are other functions that use\nthis variable (namely handle_path_include) for which its not that easy\nto pass this in. So I will just drop the check here.\n\nCheers Heiko\n"},{"id":"211141","messageId":"20130312162734.GB4704@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312110355.GE11340@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-12T16:27:35Z","receivedAt":"2013-03-12T16:27:35Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:\n> On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:\n> \n> > diff --git a/config.c b/config.c\n> > index f55c43d..fe1c0e5 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -10,20 +10,42 @@\n> >  #include \"strbuf.h\"\n> >  #include \"quote.h\"\n> >  \n> > -typedef struct config_file {\n> > -\tstruct config_file *prev;\n> > -\tFILE *f;\n> > +struct config_source {\n> > +\tstruct config_source *prev;\n> > +\tvoid *data;\n> \n> Would a union be more appropriate here? We do not ever have to pass it\n> directly as a parameter, since we pass the \"struct config_source\" to the\n> method functions.\n> \n> It's still possible to screw up using a union, but it's slightly harder\n> than screwing up using a void pointer. And I do not think we need the\n> run-time flexibility offered by the void pointer in this case.\n\nNo we do not need the void pointer flexibility. But that means every new\nsource would need to add to this union. Junio complained about that fact\nwhen I first added the extra members directly to the struct. A union\ndoes not waste that much space and we get some seperation using the\nunion members. Since this struct is local only I think that should be\nok.\n\n> > +static int config_file_fgetc(struct config_source *conf)\n> > +{\n> > +\tFILE *f = conf->data;\n> > +\treturn fgetc(f);\n> > +}\n> \n> This could become just:\n> \n>   return fgetc(conf->u.f);\n> \n> and so forth (might it make sense to give \"f\" a more descriptive name,\n> as we are adding other sources?).\n\nWill change that.\n\nCheers Heiko\n"},{"id":"211142","messageId":"20130312164254.GB4752@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312111806.GF11340@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-12T16:42:54Z","receivedAt":"2013-03-12T16:42:54Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 07:18:06AM -0400, Jeff King wrote:\n> On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:\n> \n> > This can be used to read configuration values directly from gits\n> > database.\n> > \n> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> \n> This is lacking motivation. IIRC, the rest of the story is something\n> like \"...so we can read .gitmodules directly from the repo\" or something\n> like that?\n\nWill add some more here.\n\n> > +struct config_strbuf {\n> > +\tstruct strbuf *strbuf;\n> > +\tint pos;\n> > +};\n> >\n> > +static int config_strbuf_fgetc(struct config_source *conf)\n> > +{\n> > +\tstruct config_strbuf *str = conf->data;\n> \n> Yuck. If you used a union in the previous patch, then this could just go\n> inline into the \"struct config_source\".\n> \n> > +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data)\n> \n> Should this be a \"const struct strbuf *strbuf\"? For that matter, is\n> there any reason not to take a bare pointer/len combination? It seems\n> likely that callers would get the data from read_sha1_file, which means\n> they have to stuff it into a strbuf for no good reason.\n\npointer/len should be fine too. I just used strbuf since when you find\nout later that you need to modify the string its easier to handle. A\nconfig parser should not need to do that so I will change that.\n\n> > diff --git a/test-config.c b/test-config.c\n> > new file mode 100644\n> > index 0000000..c650837\n> > --- /dev/null\n> > +++ b/test-config.c\n> > @@ -0,0 +1,40 @@\n> \n> I'm slightly \"meh\" on this test-config program.  Having to add a C test\n> harness like this is a good indication that we are short-changing users\n> of the shell API in favor of builtin C code.\n\nI mainly did this because I needed some test for the config part while\ndeveloping the \"fetch renamed submodules\" series.\n\n> Your series does not actually add any callers of the new function. The\n> obvious \"patch 5/4\" would be to plumb it into \"git config --blob\", and\n> then we can just directly test it there (there could be other callers\n> besides reading from a blob, of course, but I think the point of the\n> series is to head in that direction).\n\nSince this is a split of the series mentioned above there are no real\ncallers yet. The main reason for the split was that I wanted to reduce\nthe review burden of one big series into multiple reviews of smaller\nchunks. If you think it is useful to add the --blob option I can also\ntest from there. It could actually be useful to look at certain\n.gitmodules options from the submodule script.\n\nCheers Heiko\n"},{"id":"211147","messageId":"20130312190456.GC17099@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130312154434.GB3021@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 1/4] config: factor out config file stack management","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T19:04:56Z","receivedAt":"2013-03-12T19:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:\n\n> > Can we throw in a comment at the top here with the expected usage? In\n> > particular, do_config_from is expecting the caller to have filled in\n> > certain fields (at this point, top->f and top->name), but there is\n> > nothing to make that clear.\n> \n> Of course. Will do that in the next iteration. How about I squash this in:\n> [...]\n> +/* The fields data, name and the source specific callbacks of top need\n> + * to be initialized before calling this function.\n> + */\n>  static int do_config_from_source(struct config_source *top, config_fn_t fn, voi\n\nI think that is OK, but it may be even better to list the fields by\nname. Also, our multi-line comment style is:\n\n  /*\n   * Multi-line comment.\n   */\n\n\n> I would add that to the third patch:\n> \n> \tconfig: make parsing stack struct independent from actual data source\n> \n> because that contains the final modification to config_file/config_source.\n\nIt does not matter to the end result, but I find it helps with reviewing\nwhen the comment is added along with the function, and then expanded as\nthe function is changed. It helps to understand the effects of later\npatches if they need to tweak comments.\n\nI do not care that much in this instance, since we have already\ndiscussed it, and I know what is going on, though.\n\n-Peff\n"},{"id":"211149","messageId":"20130312191809.GD17099@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130312160056.GB4472@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T19:18:09Z","receivedAt":"2013-03-12T19:18:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:\n\n> > That is, every path to get_next_char happens while we are in\n> > git_config_from_file, and that function guarantees that cf = &top, and\n> > that top.f != NULL.  We do not have to even do any analysis of the\n> > conditions for each call, because we never change \"cf\" nor \"top.f\"\n> > except when we set them in git_config_from_file.\n> \n> Ok if you say so I will do that :-). I was thinking about adding a patch\n> that would remove cf as a global variable and explicitely pass it down\n> to get_next_char. That makes it more obvious that it actually is != NULL.\n> Looking at your callgraph I do not think its that much work. What do you\n> think?\n\nYeah, I think that makes it more obvious what is going on, but you will\nrun into trouble if you ever want that information to cross the \"void *\"\nboundary of a config callback, as adding a new parameter there is hard.\n\nThe only place we do that now is for git_config_include, and I think you\ncould get by with stuffing it into the config_include_data struct (which\nis already there to deal with this problem).\n\nIt would also make something like this patch hard or impossible:\n\n  http://article.gmane.org/gmane.comp.version-control.git/190267\n\nas it wants to access \"cf\" from arbitrary callbacks. That is not a patch\nthat is actively under consideration, but I had considered polishing it\nup at some point.\n\n> BTW, how did you generate this callgraph? Do you have a nice tool for that?\n\nI just did it manually in vi, working backwards from every instance of\nget_next_char, as it is not that big a graph. I suspect something like\ncscope could make it easier, but I don't know of any tool that will\nbuild the graph automatically (it would not be that hard from the data\ncscope has, though, so I wouldn't be surprised if such a tool exists).\n\n-Peff\n"},{"id":"211152","messageId":"20130312192636.GE17099@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130312161620.GA4551@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T19:26:36Z","receivedAt":"2013-03-12T19:26:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2013 at 05:16:20PM +0100, Heiko Voigt wrote:\n\n> > Ok if you say so I will do that :-). I was thinking about adding a patch\n> > that would remove cf as a global variable and explicitely pass it down\n> > to get_next_char. That makes it more obvious that it actually is != NULL.\n> > Looking at your callgraph I do not think its that much work. What do you\n> > think?\n> \n> I just had a look and unfortunately there are other functions that use\n> this variable (namely handle_path_include) for which its not that easy\n> to pass this in. So I will just drop the check here.\n\nAh, I should have read all your responses before answering the last\nemail. I think handle_path_include is surmountable, and I started to do\na proof-of-concept patch, but there's a harder one: die_bad_config,\nwhich is called from git_config_int and friends. Getting the \"cf\"\npointer there is hard, because you would have to update every config\ncallback that uses git_config_int. Not worth it, I think.\n\nThanks for looking, though.\n\n-Peff\n"},{"id":"211153","messageId":"20130312192737.GF17099@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130312162734.GB4704@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T19:27:37Z","receivedAt":"2013-03-12T19:27:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2013 at 05:27:35PM +0100, Heiko Voigt wrote:\n\n> > Would a union be more appropriate here? We do not ever have to pass it\n> > directly as a parameter, since we pass the \"struct config_source\" to the\n> > method functions.\n> > \n> > It's still possible to screw up using a union, but it's slightly harder\n> > than screwing up using a void pointer. And I do not think we need the\n> > run-time flexibility offered by the void pointer in this case.\n> \n> No we do not need the void pointer flexibility. But that means every new\n> source would need to add to this union. Junio complained about that fact\n> when I first added the extra members directly to the struct. A union\n> does not waste that much space and we get some seperation using the\n> union members. Since this struct is local only I think that should be\n> ok.\n\nRight. I think that is not a big deal, as we are not exposing this\nstruct outside of the config.c; any additions would need to add a new\ngit_config_from_foo function, anyway.\n\n-Peff\n"},{"id":"211154","messageId":"20130312192959.GG17099@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130312164254.GB4752@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-12T19:29:59Z","receivedAt":"2013-03-12T19:29:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:\n\n> > Your series does not actually add any callers of the new function. The\n> > obvious \"patch 5/4\" would be to plumb it into \"git config --blob\", and\n> > then we can just directly test it there (there could be other callers\n> > besides reading from a blob, of course, but I think the point of the\n> > series is to head in that direction).\n> \n> Since this is a split of the series mentioned above there are no real\n> callers yet. The main reason for the split was that I wanted to reduce\n> the review burden of one big series into multiple reviews of smaller\n> chunks. If you think it is useful to add the --blob option I can also\n> test from there. It could actually be useful to look at certain\n> .gitmodules options from the submodule script.\n\nI am on the fence. I do not want to create more work for you, but I do\nthink it may come in handy, if only for doing submodule things from\nshell. And it is hopefully not a very large patch. I'd say try it, and\nif starts looking like it will be very ugly, the right thing may be to\nleave it until somebody really wants it.\n\n-Peff\n"},{"id":"211276","messageId":"20130314063629.GA4062@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312190456.GC17099@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-14T06:36:30Z","receivedAt":"2013-03-14T06:36:30Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote:\n> On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:\n> \n> > > Can we throw in a comment at the top here with the expected usage? In\n> > > particular, do_config_from is expecting the caller to have filled in\n> > > certain fields (at this point, top->f and top->name), but there is\n> > > nothing to make that clear.\n> > \n> > Of course. Will do that in the next iteration. How about I squash this in:\n> > [...]\n> > +/* The fields data, name and the source specific callbacks of top need\n> > + * to be initialized before calling this function.\n> > + */\n> >  static int do_config_from_source(struct config_source *top, config_fn_t fn, voi\n> \n> I think that is OK, but it may be even better to list the fields by\n> name. Also, our multi-line comment style is:\n> \n>   /*\n>    * Multi-line comment.\n>    */\n\nOk will do both.\n\n> > I would add that to the third patch:\n> > \n> > \tconfig: make parsing stack struct independent from actual data source\n> > \n> > because that contains the final modification to config_file/config_source.\n> \n> It does not matter to the end result, but I find it helps with reviewing\n> when the comment is added along with the function, and then expanded as\n> the function is changed. It helps to understand the effects of later\n> patches if they need to tweak comments.\n\nTo make the series more clear to others who read it later, I will add\nthe comment from the beginning.\n\nCheers Heiko\n"},{"id":"211277","messageId":"20130314063933.GB4062@sandbox-ub.fritz.box","threadId":"33146","inReplyTo":"20130312192959.GG17099@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-14T06:39:33Z","receivedAt":"2013-03-14T06:39:33Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 12, 2013 at 03:29:59PM -0400, Jeff King wrote:\n> On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:\n> \n> > > Your series does not actually add any callers of the new function. The\n> > > obvious \"patch 5/4\" would be to plumb it into \"git config --blob\", and\n> > > then we can just directly test it there (there could be other callers\n> > > besides reading from a blob, of course, but I think the point of the\n> > > series is to head in that direction).\n> > \n> > Since this is a split of the series mentioned above there are no real\n> > callers yet. The main reason for the split was that I wanted to reduce\n> > the review burden of one big series into multiple reviews of smaller\n> > chunks. If you think it is useful to add the --blob option I can also\n> > test from there. It could actually be useful to look at certain\n> > .gitmodules options from the submodule script.\n> \n> I am on the fence. I do not want to create more work for you, but I do\n> think it may come in handy, if only for doing submodule things from\n> shell. And it is hopefully not a very large patch. I'd say try it, and\n> if starts looking like it will be very ugly, the right thing may be to\n> leave it until somebody really wants it.\n\nThats what I will do: Try it and see where I get.\n\nCheers Heiko\n"},{"id":"211280","messageId":"20130314071046.GB6103@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130314063933.GB4062@sandbox-ub.fritz.box","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-14T07:10:46Z","receivedAt":"2013-03-14T07:10:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2013 at 07:39:33AM +0100, Heiko Voigt wrote:\n\n> > I am on the fence. I do not want to create more work for you, but I do\n> > think it may come in handy, if only for doing submodule things from\n> > shell. And it is hopefully not a very large patch. I'd say try it, and\n> > if starts looking like it will be very ugly, the right thing may be to\n> > leave it until somebody really wants it.\n> \n> Thats what I will do: Try it and see where I get.\n\nI looked into this a little. The first sticking point is that\ngit_config_with_options needs to support the alternate source. Here's a\nsketch of what I think the solution should look like, on top of your\npatches.\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 33c9bf9..8d01b7a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -21,6 +21,7 @@ static const char *given_config_file;\n \n static int use_global_config, use_system_config, use_local_config;\n static const char *given_config_file;\n+static const char *given_config_blob;\n static int actions, types;\n static const char *get_color_slot, *get_colorbool_slot;\n static int end_null;\n@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOLEAN(0, \"system\", &use_system_config, N_(\"use system config file\")),\n \tOPT_BOOLEAN(0, \"local\", &use_local_config, N_(\"use repository config file\")),\n \tOPT_STRING('f', \"file\", &given_config_file, N_(\"file\"), N_(\"use given config file\")),\n+\tOPT_STRING(0, \"blob\", &given_config_blob, N_(\"blob-id\"), N_(\"read config from given blob object\")),\n \tOPT_GROUP(N_(\"Action\")),\n \tOPT_BIT(0, \"get\", &actions, N_(\"get value: name [value-regex]\"), ACTION_GET),\n \tOPT_BIT(0, \"get-all\", &actions, N_(\"get all values: key [value-regex]\"), ACTION_GET_ALL),\n@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)\n \t}\n \n \tgit_config_with_options(collect_config, &values,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tret = !values.nr;\n \n@@ -302,7 +305,8 @@ static void get_color(const char *def_color)\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tgit_config_with_options(git_get_color_config, NULL,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tif (!get_color_found && def_color)\n \t\tcolor_parse(def_color, \"command line\", parsed_color);\n@@ -330,7 +334,8 @@ static int get_colorbool(int print)\n \tget_colorbool_found = -1;\n \tget_diff_color_found = -1;\n \tgit_config_with_options(git_get_colorbool_config, NULL,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n@@ -348,6 +353,12 @@ static int get_colorbool(int print)\n \t\treturn get_colorbool_found ? 0 : 1;\n }\n \n+static void check_blob_write(void)\n+{\n+\tif (given_config_blob)\n+\t\tdie(\"writing config blobs is not supported\");\n+}\n+\n int cmd_config(int argc, const char **argv, const char *prefix)\n {\n \tint nongit = !startup_info->have_repository;\n@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_config_usage,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \n-\tif (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {\n+\tif (use_global_config + use_system_config + use_local_config +\n+\t    !!given_config_file + !!given_config_blob > 1) {\n \t\terror(\"only one config file at a time.\");\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\n \t\t\t\t\t    given_config_file,\n+\t\t\t\t\t    given_config_blob,\n \t\t\t\t\t    respect_includes) < 0) {\n \t\t\tif (given_config_file)\n \t\t\t\tdie_errno(\"unable to read config file '%s'\",\n@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (!given_config_file && nongit)\n \t\t\tdie(\"not in a git directory\");\n+\t\tif (given_config_blob)\n+\t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n \t\tlaunch_editor(given_config_file ?\n \t\t\t      given_config_file : git_path(\"config\"),\n@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (actions == ACTION_SET) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\tret = git_config_set_in_file(given_config_file, argv[0], value);\n@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], value, argv[2], 0);\n \t}\n \telse if (actions == ACTION_ADD) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], value, \"^$\", 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn get_value(argv[0], argv[1]);\n \t}\n \telse if (actions == ACTION_UNSET) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\tif (argc == 2)\n \t\t\treturn git_config_set_multivar_in_file(given_config_file,\n@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t      argv[0], NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], NULL, argv[1], 1);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tret = git_config_rename_section_in_file(given_config_file,\n \t\t\t\t\t\t\targv[0], argv[1]);\n@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (actions == ACTION_REMOVE_SECTION) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 1);\n \t\tret = git_config_rename_section_in_file(given_config_file,\n \t\t\t\t\t\t\targv[0], NULL);\ndiff --git a/cache.h b/cache.h\nindex a2621fa..9db5f74 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n-\t\t\t\t   const char *filename, int respect_includes);\n+\t\t\t\t   const char *filename,\n+\t\t\t\t   const char *blob_ref,\n+\t\t\t\t   int respect_includes);\n extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex b8c8640..35aa8e2 100644\n--- a/config.c\n+++ b/config.c\n@@ -1073,8 +1073,34 @@ int git_config_with_options(config_fn_t fn, void *data,\n \treturn ret == 0 ? found : ret;\n }\n \n+static int read_blob_reference(const char *ref,\n+\t\t\t       struct strbuf *sb)\n+{\n+\tunsigned char sha1[20];\n+\tenum object_type type;\n+\tvoid *buf;\n+\tunsigned long size;\n+\n+\tif (get_sha1(ref, sha1) < 0)\n+\t\treturn error(\"unable to resolve config blob '%s'\", ref);\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"unable to load config blob object '%s'\", ref);\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn error(\"reference '%s' does not point to a blob\", ref);\n+\t}\n+\n+\t/* read_sha1_file always NUL-terminates for us, so size+1 is OK */\n+\tstrbuf_attach(sb, buf, size, size+1);\n+\treturn 0;\n+}\n+\n int git_config_with_options(config_fn_t fn, void *data,\n-\t\t\t    const char *filename, int respect_includes)\n+\t\t\t    const char *filename,\n+\t\t\t    const char *blob_ref,\n+\t\t\t    int respect_includes)\n {\n \tchar *repo_config = NULL;\n \tint ret;\n@@ -1093,6 +1119,15 @@ int git_config_with_options(config_fn_t fn, void *data,\n \t */\n \tif (filename)\n \t\treturn git_config_from_file(fn, filename, data);\n+\telse if (blob_ref) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tif (read_blob_reference(blob_ref, &buf) < 0)\n+\t\t\treturn -1;\n+\n+\t\tret = git_config_from_strbuf(fn, blob_ref, &buf, data);\n+\t\tstrbuf_release(&buf);\n+\t\treturn ret;\n+\t}\n \n \trepo_config = git_pathdup(\"config\");\n \tret = git_config_early(fn, data, repo_config);\n@@ -1103,7 +1138,7 @@ int git_config(config_fn_t fn, void *data)\n \n int git_config(config_fn_t fn, void *data)\n {\n-\treturn git_config_with_options(fn, data, NULL, 1);\n+\treturn git_config_with_options(fn, data, NULL, NULL, 1);\n }\n \n /*\n"},{"id":"211281","messageId":"20130314073913.GA7024@sigill.intra.peff.net","threadId":"33146","inReplyTo":"20130314071046.GB6103@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-14T07:39:14Z","receivedAt":"2013-03-14T07:39:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:\n\n> I looked into this a little. The first sticking point is that\n> git_config_with_options needs to support the alternate source. Here's a\n> sketch of what I think the solution should look like, on top of your\n> patches.\n\nHere it is again, with two changes:\n\n  1. Rather than handling the blob lookup inline in\n     git_config_with_options, it adds direct functions for reading\n     config from blob sha1s and blob references. I think this should\n     make it easier to reuse when you are trying to read .gitmodules\n     from C code.\n\n  2. It adds some basic tests.\n\nI'll leave it here for tonight. The next step would be to rebase it on\nyour modified series (in particular, I think git_config_from_strbuf\nshould become git_config_from_buf, which will impact this).\n\nFeel free to use, pick apart, rewrite, or discard as you see fit for\nyour series.\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 33c9bf9..8d01b7a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -21,6 +21,7 @@ static const char *given_config_file;\n \n static int use_global_config, use_system_config, use_local_config;\n static const char *given_config_file;\n+static const char *given_config_blob;\n static int actions, types;\n static const char *get_color_slot, *get_colorbool_slot;\n static int end_null;\n@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOLEAN(0, \"system\", &use_system_config, N_(\"use system config file\")),\n \tOPT_BOOLEAN(0, \"local\", &use_local_config, N_(\"use repository config file\")),\n \tOPT_STRING('f', \"file\", &given_config_file, N_(\"file\"), N_(\"use given config file\")),\n+\tOPT_STRING(0, \"blob\", &given_config_blob, N_(\"blob-id\"), N_(\"read config from given blob object\")),\n \tOPT_GROUP(N_(\"Action\")),\n \tOPT_BIT(0, \"get\", &actions, N_(\"get value: name [value-regex]\"), ACTION_GET),\n \tOPT_BIT(0, \"get-all\", &actions, N_(\"get all values: key [value-regex]\"), ACTION_GET_ALL),\n@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)\n \t}\n \n \tgit_config_with_options(collect_config, &values,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tret = !values.nr;\n \n@@ -302,7 +305,8 @@ static void get_color(const char *def_color)\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tgit_config_with_options(git_get_color_config, NULL,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tif (!get_color_found && def_color)\n \t\tcolor_parse(def_color, \"command line\", parsed_color);\n@@ -330,7 +334,8 @@ static int get_colorbool(int print)\n \tget_colorbool_found = -1;\n \tget_diff_color_found = -1;\n \tgit_config_with_options(git_get_colorbool_config, NULL,\n-\t\t\t\tgiven_config_file, respect_includes);\n+\t\t\t\tgiven_config_file, given_config_blob,\n+\t\t\t\trespect_includes);\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n@@ -348,6 +353,12 @@ static int get_colorbool(int print)\n \t\treturn get_colorbool_found ? 0 : 1;\n }\n \n+static void check_blob_write(void)\n+{\n+\tif (given_config_blob)\n+\t\tdie(\"writing config blobs is not supported\");\n+}\n+\n int cmd_config(int argc, const char **argv, const char *prefix)\n {\n \tint nongit = !startup_info->have_repository;\n@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_config_usage,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \n-\tif (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {\n+\tif (use_global_config + use_system_config + use_local_config +\n+\t    !!given_config_file + !!given_config_blob > 1) {\n \t\terror(\"only one config file at a time.\");\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\n \t\t\t\t\t    given_config_file,\n+\t\t\t\t\t    given_config_blob,\n \t\t\t\t\t    respect_includes) < 0) {\n \t\t\tif (given_config_file)\n \t\t\t\tdie_errno(\"unable to read config file '%s'\",\n@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (!given_config_file && nongit)\n \t\t\tdie(\"not in a git directory\");\n+\t\tif (given_config_blob)\n+\t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n \t\tlaunch_editor(given_config_file ?\n \t\t\t      given_config_file : git_path(\"config\"),\n@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (actions == ACTION_SET) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\tret = git_config_set_in_file(given_config_file, argv[0], value);\n@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], value, argv[2], 0);\n \t}\n \telse if (actions == ACTION_ADD) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], value, \"^$\", 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn get_value(argv[0], argv[1]);\n \t}\n \telse if (actions == ACTION_UNSET) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\tif (argc == 2)\n \t\t\treturn git_config_set_multivar_in_file(given_config_file,\n@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t      argv[0], NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\treturn git_config_set_multivar_in_file(given_config_file,\n \t\t\t\t\t\t       argv[0], NULL, argv[1], 1);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tret = git_config_rename_section_in_file(given_config_file,\n \t\t\t\t\t\t\targv[0], argv[1]);\n@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (actions == ACTION_REMOVE_SECTION) {\n \t\tint ret;\n+\t\tcheck_blob_write();\n \t\tcheck_argc(argc, 1, 1);\n \t\tret = git_config_rename_section_in_file(given_config_file,\n \t\t\t\t\t\t\targv[0], NULL);\ndiff --git a/cache.h b/cache.h\nindex a2621fa..9db5f74 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n-\t\t\t\t   const char *filename, int respect_includes);\n+\t\t\t\t   const char *filename,\n+\t\t\t\t   const char *blob_ref,\n+\t\t\t\t   int respect_includes);\n extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex b8c8640..5c492a8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1009,6 +1009,47 @@ int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strb\n \treturn do_config_from_source(&top, fn, data);\n }\n \n+static int git_config_from_blob_sha1(config_fn_t fn,\n+\t\t\t\t     const char *name,\n+\t\t\t\t     const unsigned char *sha1,\n+\t\t\t\t     void *data)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tenum object_type type;\n+\tvoid *buf;\n+\tunsigned long size;\n+\tint ret;\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"unable to load config blob object '%s'\", name);\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn error(\"reference '%s' does not point to a blob\", name);\n+\t}\n+\n+\t/*\n+\t * read_sha1_file always NUL-terminates for us, so size+1 is OK;\n+\t * we could skip this step if we had git_config_from_buf\n+\t */\n+\tstrbuf_attach(&sb, buf, size, size+1);\n+\tret = git_config_from_strbuf(fn, name, &sb, data);\n+\tstrbuf_release(&sb);\n+\n+\treturn ret;\n+}\n+\n+static int git_config_from_blob_ref(config_fn_t fn,\n+\t\t\t\t    const char *name,\n+\t\t\t\t    void *data)\n+{\n+\tunsigned char sha1[20];\n+\n+\tif (get_sha1(name, sha1) < 0)\n+\t\treturn error(\"unable to resolve config blob '%s'\", name);\n+\treturn git_config_from_blob_sha1(fn, name, sha1, data);\n+}\n+\n const char *git_etc_gitconfig(void)\n {\n \tstatic const char *system_wide;\n@@ -1074,7 +1115,9 @@ int git_config_with_options(config_fn_t fn, void *data,\n }\n \n int git_config_with_options(config_fn_t fn, void *data,\n-\t\t\t    const char *filename, int respect_includes)\n+\t\t\t    const char *filename,\n+\t\t\t    const char *blob_ref,\n+\t\t\t    int respect_includes)\n {\n \tchar *repo_config = NULL;\n \tint ret;\n@@ -1093,6 +1136,8 @@ int git_config_with_options(config_fn_t fn, void *data,\n \t */\n \tif (filename)\n \t\treturn git_config_from_file(fn, filename, data);\n+\telse if (blob_ref)\n+\t\treturn git_config_from_blob_ref(fn, blob_ref, data);\n \n \trepo_config = git_pathdup(\"config\");\n \tret = git_config_early(fn, data, repo_config);\n@@ -1103,7 +1148,7 @@ int git_config(config_fn_t fn, void *data)\n \n int git_config(config_fn_t fn, void *data)\n {\n-\treturn git_config_with_options(fn, data, NULL, 1);\n+\treturn git_config_with_options(fn, data, NULL, NULL, 1);\n }\n \n /*\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex e69de29..141ca21 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -0,0 +1,66 @@\n+#!/bin/sh\n+\n+test_description='support for reading config from a blob'\n+. ./test-lib.sh\n+\n+test_expect_success 'create config blob' '\n+\tcat >config <<-\\EOF &&\n+\t[some]\n+\t\tvalue = 1\n+\tEOF\n+\tgit add config &&\n+\tgit commit -m foo\n+'\n+\n+test_expect_success 'list config blob contents' '\n+\techo some.value=1 >expect &&\n+\tgit config --blob=HEAD:config --list >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fetch value from blob' '\n+\techo true >expect &&\n+\tgit config --blob=HEAD:config --bool some.value >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading from blob and file is an error' '\n+\ttest_must_fail git config --blob=HEAD:config --system --list\n+'\n+\n+test_expect_success 'reading from missing ref is an error' '\n+\ttest_must_fail git config --blob=HEAD:doesnotexist --list\n+'\n+\n+test_expect_success 'reading from non-blob is an error' '\n+\ttest_must_fail git config --blob=HEAD --list\n+'\n+\n+test_expect_success 'setting a value in a blob is an error' '\n+\ttest_must_fail git config --blob=HEAD:config some.value foo\n+'\n+\n+test_expect_success 'deleting a value in a blob is an error' '\n+\ttest_must_fail git config --blob=HEAD:config --unset some.value\n+'\n+\n+test_expect_success 'editing a blob is an error' '\n+\ttest_must_fail git config --blob=HEAD:config --edit\n+'\n+\n+test_expect_success 'parse errors in blobs are properly attributed' '\n+\tcat >config <<-\\EOF &&\n+\t[some]\n+\t\tvalue = \"\n+\tEOF\n+\tgit add config &&\n+\tgit commit -m broken &&\n+\n+\ttest_must_fail git config --blob=HEAD:config some.value 2>err &&\n+\n+\t# just grep for our token as the exact error message is likely to\n+\t# change or be internationalized\n+\tgrep \"HEAD:config\" err\n+'\n+\n+test_done\n"},{"id":"211570","messageId":"51472234.2060207@hvoigt.net","threadId":"33146","inReplyTo":"20130314073913.GA7024@sigill.intra.peff.net","subject":"Re: Re: [PATCH v2 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-18T14:18:28Z","receivedAt":"2013-03-18T14:18:28Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Peff,\n\nOn Thu, Mar 14, 2013 at 03:39:14AM -0400, Jeff King wrote:\n> On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:\n>\n> > I looked into this a little. The first sticking point is that\n> > git_config_with_options needs to support the alternate source. Here's a\n> > sketch of what I think the solution should look like, on top of your\n> > patches.\n>\n> Here it is again, with two changes:\n>\n>   1. Rather than handling the blob lookup inline in\n>      git_config_with_options, it adds direct functions for reading\n>      config from blob sha1s and blob references. I think this should\n>      make it easier to reuse when you are trying to read .gitmodules\n>      from C code.\n>\n>   2. It adds some basic tests.\n>\n> I'll leave it here for tonight. The next step would be to rebase it on\n> your modified series (in particular, I think git_config_from_strbuf\n> should become git_config_from_buf, which will impact this).\n>\n> Feel free to use, pick apart, rewrite, or discard as you see fit for\n> your series.\n\nSorry for the late reply, I was not online during the last days. Thanks \na lot for this. I will use it in the next iteration of the series.\n\nCheers Heiko\n"}]}