{"thread":{"id":"33762","subject":"[PATCH v3 2/5] config: drop cf validity check in get_next_char()","startedAt":"2013-05-09T16:17:53Z","lastAt":"2013-05-11T10:30:21Z","messageCount":14,"participants":["Heiko Voigt","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":5},"messages":[{"id":"216776","messageId":"20130509161753.GC3526@book-mint","threadId":"33762","inReplyTo":"20130509154020.GA26423@book-mint","subject":"[PATCH v3 1/5] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-09T16:17:53Z","receivedAt":"2013-05-09T16:17: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 | 42 ++++++++++++++++++++++++++++--------------\n 1 file changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex aefd80b..f0494f3 100644\n--- a/config.c\n+++ b/config.c\n@@ -896,6 +896,32 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n+/*\n+ * The fields f and name of top need to be initialized before calling\n+ * this function.\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 +931,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.3.rc1.40.gba374ae\n"},{"id":"216775","messageId":"20130509161841.GD3526@book-mint","threadId":"33762","inReplyTo":"20130509154020.GA26423@book-mint","subject":"[PATCH v3 2/5] config: drop cf validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-09T16:18:42Z","receivedAt":"2013-05-09T16:18:42Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The global variable cf is set with an initialized value in all codepaths before\ncalling this function.\n\nThe complete call graph looks like this:\n\n  git_config_from_file\n    -> do_config_from\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\nThe variable is initialized in do_config_from.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n config.c | 29 +++++++++++++----------------\n 1 file changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f0494f3..046642b 100644\n--- a/config.c\n+++ b/config.c\n@@ -169,26 +169,23 @@ 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+\tFILE *f = cf->f;\n \n-\tc = '\\n';\n-\tif (cf && ((f = cf->f) != NULL)) {\n+\tc = fgetc(f);\n+\tif (c == '\\r') {\n+\t\t/* DOS like systems */\n \t\tc = fgetc(f);\n-\t\tif (c == '\\r') {\n-\t\t\t/* DOS like systems */\n-\t\t\tc = fgetc(f);\n-\t\t\tif (c != '\\n') {\n-\t\t\t\tungetc(c, f);\n-\t\t\t\tc = '\\r';\n-\t\t\t}\n-\t\t}\n-\t\tif (c == '\\n')\n-\t\t\tcf->linenr++;\n-\t\tif (c == EOF) {\n-\t\t\tcf->eof = 1;\n-\t\t\tc = '\\n';\n+\t\tif (c != '\\n') {\n+\t\t\tungetc(c, f);\n+\t\t\tc = '\\r';\n \t\t}\n \t}\n+\tif (c == '\\n')\n+\t\tcf->linenr++;\n+\tif (c == EOF) {\n+\t\tcf->eof = 1;\n+\t\tc = '\\n';\n+\t}\n \treturn c;\n }\n \n-- \n1.8.3.rc1.40.gba374ae\n"},{"id":"216777","messageId":"20130509161932.GE3526@book-mint","threadId":"33762","inReplyTo":"20130509154020.GA26423@book-mint","subject":"[PATCH v3 3/5] config: make parsing stack struct independent from actual data source","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-09T16:19:32Z","receivedAt":"2013-05-09T16:19:32Z","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 | 66 ++++++++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 44 insertions(+), 22 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 046642b..2390458 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,20 +10,41 @@\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+\tunion {\n+\t\tFILE *file;\n+\t};\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+\treturn fgetc(conf->file);\n+}\n+\n+static int config_file_ungetc(int c, struct config_source *conf)\n+{\n+\treturn ungetc(c, conf->file);\n+}\n+\n+static long config_file_ftell(struct config_source *conf)\n+{\n+\treturn ftell(conf->file);\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@@ -168,15 +189,13 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \n static int get_next_char(void)\n {\n-\tint c;\n-\tFILE *f = cf->f;\n+\tint c = cf->fgetc(cf);\n \n-\tc = fgetc(f);\n \tif (c == '\\r') {\n \t\t/* DOS like systems */\n-\t\tc = fgetc(f);\n+\t\tc = cf->fgetc(cf);\n \t\tif (c != '\\n') {\n-\t\t\tungetc(c, f);\n+\t\t\tcf->ungetc(c, cf);\n \t\t\tc = '\\r';\n \t\t}\n \t}\n@@ -336,7 +355,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@@ -894,10 +913,11 @@ int git_default_config(const char *var, const char *value, void *dummy)\n }\n \n /*\n- * The fields f and name of top need to be initialized before calling\n+ * All source specific fields in the union, name and the callbacks\n+ * fgetc, ungetc, ftell of top need to be initialized before calling\n  * this function.\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@@ -909,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@@ -926,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.file = 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@@ -1064,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@@ -1076,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@@ -1103,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.3.rc1.40.gba374ae\n"},{"id":"216780","messageId":"20130509162018.GF3526@book-mint","threadId":"33762","inReplyTo":"20130509154020.GA26423@book-mint","subject":"[PATCH v3 4/5] teach config --blob option to parse config from database","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-09T16:20:18Z","receivedAt":"2013-05-09T16:20:18Z","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. For example it is useful for reading to be checked out\n.gitmodules files directly from the database.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n builtin/config.c       | 31 +++++++++++++++---\n cache.h                |  6 +++-\n config.c               | 85 ++++++++++++++++++++++++++++++++++++++++++++++++--\n t/t1307-config-blob.sh | 70 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 185 insertions(+), 7 deletions(-)\n create mode 100755 t/t1307-config-blob.sh\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 char term = '\\n';\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 94ca1ac..be48c4b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1142,11 +1142,15 @@ 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_buf(config_fn_t fn, const char *name,\n+\t\t\t       const char *buf, size_t len, 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 *);\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 2390458..50c762a 100644\n--- a/config.c\n+++ b/config.c\n@@ -14,6 +14,11 @@ struct config_source {\n \tstruct config_source *prev;\n \tunion {\n \t\tFILE *file;\n+\t\tstruct config_buf {\n+\t\t\tconst char *buf;\n+\t\t\tsize_t len;\n+\t\t\tsize_t pos;\n+\t\t} buf;\n \t};\n \tconst char *name;\n \tint linenr;\n@@ -45,6 +50,28 @@ static long config_file_ftell(struct config_source *conf)\n \treturn ftell(conf->file);\n }\n \n+\n+static int config_buf_fgetc(struct config_source *conf)\n+{\n+\tif (conf->buf.pos < conf->buf.len && conf->buf.buf[conf->buf.pos])\n+\t\treturn conf->buf.buf[conf->buf.pos++];\n+\n+\treturn EOF;\n+}\n+\n+static int config_buf_ungetc(int c, struct config_source *conf)\n+{\n+\tif (conf->buf.pos > 0)\n+\t\treturn conf->buf.buf[--conf->buf.pos];\n+\n+\treturn EOF;\n+}\n+\n+static long config_buf_ftell(struct config_source *conf)\n+{\n+\treturn conf->buf.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 +988,56 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \treturn ret;\n }\n \n+int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n+\t\t\tsize_t len, void *data)\n+{\n+\tstruct config_source top;\n+\n+\ttop.buf.buf = buf;\n+\ttop.buf.pos = 0;\n+\ttop.name = name;\n+\ttop.fgetc = config_buf_fgetc;\n+\ttop.ungetc = config_buf_ungetc;\n+\ttop.ftell = config_buf_ftell;\n+\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+\tenum object_type type;\n+\tchar *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+\tret = git_config_from_buf(fn, name, buf, size, data);\n+\tfree(buf);\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@@ -1026,7 +1103,9 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\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@@ -1045,6 +1124,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@@ -1055,7 +1136,7 @@ int git_config_with_options(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\nnew file mode 100755\nindex 0000000..fdc257e\n--- /dev/null\n+++ b/t/t1307-config-blob.sh\n@@ -0,0 +1,70 @@\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 non-existing value from blob is an error' '\n+\ttest_must_fail git config --blob=HEAD:config non.existing\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-- \n1.8.3.rc1.40.gba374ae\n"},{"id":"216779","messageId":"20130509162102.GG3526@book-mint","threadId":"33762","inReplyTo":"20130509154020.GA26423@book-mint","subject":"[PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-09T16:21:02Z","receivedAt":"2013-05-09T16:21:02Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"If a config parsing error in a file occurs we can die and let the user\nfix the issue. This is different for the buf parsing function since it\ncan be used to parse blobs of .gitmodules files. If a parsing error\noccurs here we should proceed since otherwise a database containing such\nan error in a single revision could be rendered unuseable.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n builtin/config.c       | 10 +++++++---\n config.c               | 10 ++++++++--\n t/t1307-config-blob.sh |  9 +++++++--\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8d01b7a..de32977 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -219,9 +219,11 @@ static int get_value(const char *key_, const char *regex_)\n \t\t}\n \t}\n \n-\tgit_config_with_options(collect_config, &values,\n+\tret = git_config_with_options(collect_config, &values,\n \t\t\t\tgiven_config_file, given_config_blob,\n \t\t\t\trespect_includes);\n+\tif (ret < 0)\n+\t\tgoto free_values;\n \n \tret = !values.nr;\n \n@@ -231,6 +233,7 @@ static int get_value(const char *key_, const char *regex_)\n \t\t\tfwrite(buf->buf, 1, buf->len, stdout);\n \t\tstrbuf_release(buf);\n \t}\n+free_values:\n \tfree(values.items);\n \n free_strings:\n@@ -333,9 +336,10 @@ static int get_colorbool(int print)\n {\n \tget_colorbool_found = -1;\n \tget_diff_color_found = -1;\n-\tgit_config_with_options(git_get_colorbool_config, NULL,\n+\tif (git_config_with_options(git_get_colorbool_config, NULL,\n \t\t\t\tgiven_config_file, given_config_blob,\n-\t\t\t\trespect_includes);\n+\t\t\t\trespect_includes) < 0)\n+\t\treturn -1;\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\ndiff --git a/config.c b/config.c\nindex 50c762a..8a8408d 100644\n--- a/config.c\n+++ b/config.c\n@@ -21,6 +21,7 @@ struct config_source {\n \t\t} buf;\n \t};\n \tconst char *name;\n+\tint die_on_error;\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n@@ -442,7 +443,10 @@ static int git_parse_source(config_fn_t fn, void *data)\n \t\tif (get_value(fn, data, var) < 0)\n \t\t\tbreak;\n \t}\n-\tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n+\tif (cf->die_on_error)\n+\t\tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n+\telse\n+\t\treturn error(\"bad config file line %d in %s\", cf->linenr, cf->name);\n }\n \n static int parse_unit_factor(const char *end, uintmax_t *val)\n@@ -940,7 +944,7 @@ int git_default_config(const char *var, const char *value, void *dummy)\n }\n \n /*\n- * All source specific fields in the union, name and the callbacks\n+ * All source specific fields in the union, die_on_error, name and the callbacks\n  * fgetc, ungetc, ftell of top need to be initialized before calling\n  * this function.\n  */\n@@ -977,6 +981,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n \t\ttop.file = f;\n \t\ttop.name = filename;\n+\t\ttop.die_on_error = 1;\n \t\ttop.fgetc = config_file_fgetc;\n \t\ttop.ungetc = config_file_ungetc;\n \t\ttop.ftell = config_file_ftell;\n@@ -996,6 +1001,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n \ttop.buf.buf = buf;\n \ttop.buf.pos = 0;\n \ttop.name = name;\n+\ttop.die_on_error = 0;\n \ttop.fgetc = config_buf_fgetc;\n \ttop.ungetc = config_buf_ungetc;\n \ttop.ftell = config_buf_ftell;\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex fdc257e..a4929eb 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '\n test_expect_success 'parse errors in blobs are properly attributed' '\n \tcat >config <<-\\EOF &&\n \t[some]\n-\t\tvalue = \"\n+\t\tvalue = 1\n+\t\terror = \"\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+\ttest_must_fail git config --blob=HEAD:config some.value 1>out 2>err &&\n+\n+\t# Make sure there is no output of values on stdout\n+\ttouch out.expect &&\n+\ttest_cmp out.expect out &&\n \n \t# just grep for our token as the exact error message is likely to\n \t# change or be internationalized\n-- \n1.8.3.rc1.40.gba374ae\n"},{"id":"216801","messageId":"CAPig+cTxcQ4o+6RrEHU3iaCvuscX2CoB9hiQsA+0cRM_OD8AUA@mail.gmail.com","threadId":"33762","inReplyTo":"20130509162102.GG3526@book-mint","subject":"Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-09T18:23:08Z","receivedAt":"2013-05-09T18:23:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 9, 2013 at 12:21 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> If a config parsing error in a file occurs we can die and let the user\n> fix the issue. This is different for the buf parsing function since it\n> can be used to parse blobs of .gitmodules files. If a parsing error\n> occurs here we should proceed since otherwise a database containing such\n> an error in a single revision could be rendered unuseable.\n\ns/unuseable/unusable/\n\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n"},{"id":"216802","messageId":"CAPig+cQDz9Pzao+9LJ1jW_=x-iL7e_7GE3=6uBfCY6Ezq48mYg@mail.gmail.com","threadId":"33762","inReplyTo":"20130509162018.GF3526@book-mint","subject":"Re: [PATCH v3 4/5] teach config --blob option to parse config from database","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-09T18:23:45Z","receivedAt":"2013-05-09T18:23:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 9, 2013 at 12:20 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> This can be used to read configuration values directly from gits\n\ns/gits/git's/\n\n> database. For example it is useful for reading to be checked out\n> .gitmodules files directly from the database.\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n"},{"id":"216823","messageId":"20130509222158.GA30774@sigill.intra.peff.net","threadId":"33762","inReplyTo":"20130509161932.GE3526@book-mint","subject":"Re: [PATCH v3 3/5] config: make parsing stack struct independent from actual data source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-09T22:21:58Z","receivedAt":"2013-05-09T22:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 09, 2013 at 06:19:32PM +0200, Heiko Voigt wrote:\n\n> diff --git a/config.c b/config.c\n> index 046642b..2390458 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -10,20 +10,41 @@\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> +\tunion {\n> +\t\tFILE *file;\n> +\t};\n\nAnonymous unions like this are a C11-ism (I don't know when gcc learned\nabout them, but I am sure that many of the older compilers we support\nwould not be happy). You have to do:\n\n  union {\n          FILE *file;\n  } u;\n\nand access the file as \"cf->u.file\".\n\n-Peff\n"},{"id":"216824","messageId":"20130509223038.GB30774@sigill.intra.peff.net","threadId":"33762","inReplyTo":"20130509162018.GF3526@book-mint","subject":"Re: [PATCH v3 4/5] teach config --blob option to parse config from database","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-09T22:30:39Z","receivedAt":"2013-05-09T22:30:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 09, 2013 at 06:20:18PM +0200, Heiko Voigt wrote:\n\n> +static int config_buf_fgetc(struct config_source *conf)\n> +{\n> +\tif (conf->buf.pos < conf->buf.len && conf->buf.buf[conf->buf.pos])\n> +\t\treturn conf->buf.buf[conf->buf.pos++];\n> +\n> +\treturn EOF;\n> +}\n\nIt probably would not matter for config reading, but is there any reason\nnot to make your fgetc replacement 8-bit clean (i.e., to check only len,\nand not worry about NUL characters)?\n\n-Peff\n"},{"id":"216825","messageId":"20130509223936.GC30774@sigill.intra.peff.net","threadId":"33762","inReplyTo":"20130509162102.GG3526@book-mint","subject":"Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-09T22:39:37Z","receivedAt":"2013-05-09T22:39:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 09, 2013 at 06:21:02PM +0200, Heiko Voigt wrote:\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 8d01b7a..de32977 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -219,9 +219,11 @@ static int get_value(const char *key_, const char *regex_)\n>  \t\t}\n>  \t}\n>  \n> -\tgit_config_with_options(collect_config, &values,\n> +\tret = git_config_with_options(collect_config, &values,\n>  \t\t\t\tgiven_config_file, given_config_blob,\n>  \t\t\t\trespect_includes);\n> +\tif (ret < 0)\n> +\t\tgoto free_values;\n>  \n>  \tret = !values.nr;\n>  \n> @@ -231,6 +233,7 @@ static int get_value(const char *key_, const char *regex_)\n>  \t\t\tfwrite(buf->buf, 1, buf->len, stdout);\n>  \t\tstrbuf_release(buf);\n>  \t}\n> +free_values:\n>  \tfree(values.items);\n>  \n>  free_strings:\n\nHmm. \"values\" is a strbuf_list. Don't we need to strbuf_release() its\nindividual members before freeing it? The writing loop directly above\nyour label frees each one after writing. It would probably make sense to\njust split it into two loops, one for writing, and then one for\nfree-ing. This is not a critical performance path that we can't iterate\nover the (probably handful of) entries twice.\n\n> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\n> index fdc257e..a4929eb 100755\n> --- a/t/t1307-config-blob.sh\n> +++ b/t/t1307-config-blob.sh\n> @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '\n>  test_expect_success 'parse errors in blobs are properly attributed' '\n>  \tcat >config <<-\\EOF &&\n>  \t[some]\n> -\t\tvalue = \"\n> +\t\tvalue = 1\n> +\t\terror = \"\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> +\ttest_must_fail git config --blob=HEAD:config some.value 1>out 2>err &&\n> +\n> +\t# Make sure there is no output of values on stdout\n> +\ttouch out.expect &&\n> +\ttest_cmp out.expect out &&\n\nI'm not clear on what is being tested here. Were we outputting to stdout\nbefore this patch (I would have thought our die() meant we did not.\n\n-Peff\n"},{"id":"216908","messageId":"20130510154735.GA3850@book-mint","threadId":"33762","inReplyTo":"20130509223038.GB30774@sigill.intra.peff.net","subject":"Re: Re: [PATCH v3 4/5] teach config --blob option to parse config from database","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-10T15:47:35Z","receivedAt":"2013-05-10T15:47:35Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, May 10, 2013 at 12:30:39AM +0200, Jeff King wrote:\n> On Thu, May 09, 2013 at 06:20:18PM +0200, Heiko Voigt wrote:\n> \n> > +static int config_buf_fgetc(struct config_source *conf)\n> > +{\n> > +\tif (conf->buf.pos < conf->buf.len && conf->buf.buf[conf->buf.pos])\n> > +\t\treturn conf->buf.buf[conf->buf.pos++];\n> > +\n> > +\treturn EOF;\n> > +}\n> \n> It probably would not matter for config reading, but is there any reason\n> not to make your fgetc replacement 8-bit clean (i.e., to check only len,\n> and not worry about NUL characters)?\n\nI got a NULL character here and by mistake thought that somehow the len\nwould include the ending character. But that was a bug introduced by me\nwhen I refactored to buffer and length instead of strbuf. I missed to\ntransport the len properly. So I will removed this check again.\n\nCheers Heiko\n"},{"id":"216978","messageId":"20130511085531.GA3670@book-mint","threadId":"33762","inReplyTo":"20130509223936.GC30774@sigill.intra.peff.net","subject":"Re: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-11T08:55:31Z","receivedAt":"2013-05-11T08:55:31Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, May 10, 2013 at 12:39:37AM +0200, Jeff King wrote:\n> On Thu, May 09, 2013 at 06:21:02PM +0200, Heiko Voigt wrote:\n> \n> > diff --git a/builtin/config.c b/builtin/config.c\n> > index 8d01b7a..de32977 100644\n> > --- a/builtin/config.c\n> > +++ b/builtin/config.c\n> > @@ -219,9 +219,11 @@ static int get_value(const char *key_, const char *regex_)\n> >  \t\t}\n> >  \t}\n> >  \n> > -\tgit_config_with_options(collect_config, &values,\n> > +\tret = git_config_with_options(collect_config, &values,\n> >  \t\t\t\tgiven_config_file, given_config_blob,\n> >  \t\t\t\trespect_includes);\n> > +\tif (ret < 0)\n> > +\t\tgoto free_values;\n> >  \n> >  \tret = !values.nr;\n> >  \n> > @@ -231,6 +233,7 @@ static int get_value(const char *key_, const char *regex_)\n> >  \t\t\tfwrite(buf->buf, 1, buf->len, stdout);\n> >  \t\tstrbuf_release(buf);\n> >  \t}\n> > +free_values:\n> >  \tfree(values.items);\n> >  \n> >  free_strings:\n> \n> Hmm. \"values\" is a strbuf_list. Don't we need to strbuf_release() its\n> individual members before freeing it? The writing loop directly above\n> your label frees each one after writing. It would probably make sense to\n> just split it into two loops, one for writing, and then one for\n> free-ing. This is not a critical performance path that we can't iterate\n> over the (probably handful of) entries twice.\n\nThanks for catching that. I missed the strbuf_release in the loop\nsomehow. Will fix in the next iteration.\n\n> > diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\n> > index fdc257e..a4929eb 100755\n> > --- a/t/t1307-config-blob.sh\n> > +++ b/t/t1307-config-blob.sh\n> > @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '\n> >  test_expect_success 'parse errors in blobs are properly attributed' '\n> >  \tcat >config <<-\\EOF &&\n> >  \t[some]\n> > -\t\tvalue = \"\n> > +\t\tvalue = 1\n> > +\t\terror = \"\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> > +\ttest_must_fail git config --blob=HEAD:config some.value 1>out 2>err &&\n> > +\n> > +\t# Make sure there is no output of values on stdout\n> > +\ttouch out.expect &&\n> > +\ttest_cmp out.expect out &&\n> \n> I'm not clear on what is being tested here. Were we outputting to stdout\n> before this patch (I would have thought our die() meant we did not.\n\nWe where not outputting to stdout before so the test is correct there as\nwell. I extended the test when implementing the non-dying version of\ngit_config_with_options() so I could see the test fail. If just\nreturning the error it would still output the values read until then. So\nif you think that it belongs into the initial version of this test\n(maybe including some comment why we need an extra parseable value) I\nam happy to move it there.\n\nCheers Heiko\n"},{"id":"216984","messageId":"20130511095936.GB17326@sigill.intra.peff.net","threadId":"33762","inReplyTo":"20130511085531.GA3670@book-mint","subject":"Re: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T09:59:36Z","receivedAt":"2013-05-11T09:59:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2013 at 10:55:31AM +0200, Heiko Voigt wrote:\n\n> > > diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\n> > > index fdc257e..a4929eb 100755\n> > > --- a/t/t1307-config-blob.sh\n> > > +++ b/t/t1307-config-blob.sh\n> > > @@ -55,12 +55,17 @@ test_expect_success 'editing a blob is an error' '\n> > >  test_expect_success 'parse errors in blobs are properly attributed' '\n> > >  \tcat >config <<-\\EOF &&\n> > >  \t[some]\n> > > -\t\tvalue = \"\n> > > +\t\tvalue = 1\n> > > +\t\terror = \"\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> > > +\ttest_must_fail git config --blob=HEAD:config some.value 1>out 2>err &&\n> > > +\n> > > +\t# Make sure there is no output of values on stdout\n> > > +\ttouch out.expect &&\n> > > +\ttest_cmp out.expect out &&\n> > \n> > I'm not clear on what is being tested here. Were we outputting to stdout\n> > before this patch (I would have thought our die() meant we did not.\n> \n> We where not outputting to stdout before so the test is correct there as\n> well. I extended the test when implementing the non-dying version of\n> git_config_with_options() so I could see the test fail. If just\n> returning the error it would still output the values read until then. So\n> if you think that it belongs into the initial version of this test\n> (maybe including some comment why we need an extra parseable value) I\n> am happy to move it there.\n\n>From what you wrote above, it sounds like we would expect git-config to\nwrite out some.value before failing on some.error. But that isn't what\nthe test is checking, is it? It is checking that nothing is output.\n\nI do not think the output matters much either way when git-config has\nfailed; if it returns a non-zero exit code, then the results of stdout\ncannot be trusted (after all, it may have been killed by signal after it\nhad written out half of the output).\n\nStill slightly puzzled...\n\n-Peff\n"},{"id":"216987","messageId":"20130511103020.GA6329@book-mint","threadId":"33762","inReplyTo":"20130511095936.GB17326@sigill.intra.peff.net","subject":"Re: Re: [PATCH v3 5/5] do not die when error in config parsing of buf occurs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-05-11T10:30:21Z","receivedAt":"2013-05-11T10:30:21Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Sat, May 11, 2013 at 11:59:36AM +0200, Jeff King wrote:\n> On Sat, May 11, 2013 at 10:55:31AM +0200, Heiko Voigt wrote:\n> > We where not outputting to stdout before so the test is correct there as\n> > well. I extended the test when implementing the non-dying version of\n> > git_config_with_options() so I could see the test fail. If just\n> > returning the error it would still output the values read until then. So\n> > if you think that it belongs into the initial version of this test\n> > (maybe including some comment why we need an extra parseable value) I\n> > am happy to move it there.\n> \n> From what you wrote above, it sounds like we would expect git-config to\n> write out some.value before failing on some.error. But that isn't what\n> the test is checking, is it? It is checking that nothing is output.\n> \n> I do not think the output matters much either way when git-config has\n> failed; if it returns a non-zero exit code, then the results of stdout\n> cannot be trusted (after all, it may have been killed by signal after it\n> had written out half of the output).\n> \n> Still slightly puzzled...\n\nWell before my \"do not die patch\" there was no output to stdout since git\nalready died during reading.\nAfterwards there would be output of values to stdout until the error\noccurred and then the error code would be returned. So my intend was to\nkeep the output the same. Since the blob reading is new I thinks its\nfine either way. So currently I am thinking about just removing the\nwhole \"stop to write anything on error\" part of this patch and just\nreturn the error. What do you think?\n\nCheers Heiko\n"}]}