{"thread":{"id":"41407","subject":"[PATCH v3 0/3] config: add '--sources' option to print the source of a config value","startedAt":"2016-02-13T14:24:13Z","lastAt":"2016-02-15T09:41:11Z","messageCount":25,"participants":["larsxschneider@gmail.com","Mike Rappazzo","Jeff King","Sebastian Schuberth","Junio C Hamano","Lars Schneider","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"278068","messageId":"1455373456-64691-1-git-send-email-larsxschneider@gmail.com","threadId":"41407","inReplyTo":null,"subject":"[PATCH v3 0/3] config: add '--sources' option to print the source of a config value","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-13T14:24:13Z","receivedAt":"2016-02-13T14:24:13Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\ndiff to v2:\n* rename cmd to cmdline\n* rename function current_config_name to current_config_type_name\n* add 'type' to config_source struct that identifies config type\n* fix config stdin error output and add test case \"invalid stdin config\"\n* change delimiter between type and name from tab to colon\n* remove is_query_action variable\n* rename \"--sources\" to \"--show-origin\"\n* add pathological test case \"--show-origin stdin with file include\"\n* enumerate all valid commandline cases for \"--show-origin\"\n* removed TODOs as there are no config include bugs\n* describe '--includes' default behavior in git-config.txt\n* split test cases\n* use non-interpolated here-docs where possible\n* improve readablity of 'show_config_origin' function by removing duality\n\nI renamed the flag from \"--source\" to \"--show-origin\" as I got the impression\nthat Sebastian, Peff and Ramsay seem to be all OK with \"--show-origin\".\n\nThanks Eric for the hint to simplify the 'show_config_origin' function.\nI took your suggestion but modfied one line. Instead of \"fputs\" I used\n\"fwrite\" to write the content. This was necssary as the last char of the\nstring could be \\0 due to the \"--null\" flag. \"fputs\" would ignore that.\n\nIn 959b545 Heiko introduced a config API to lookup .gitmodules values and\nin \"submodule-config.c\" he uses the \"git_config_from_buf\" function [1]. I\nwonder if my modifications to this function could trigger any unwanted side\neffects in his implementation? (I can't imagine any but I want to make you\naware of this connection)\n\n\nThanks a lot Peff, Sebastian, Ramsey, and Eric for the review.\n\n\n[1] https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/submodule-config.c#L430-L431\n\n\nLars Schneider (3):\n git-config.txt: describe '--includes' default behavior\n config: add 'type' to config_source struct that identifies config type\n config: add '--show-origin' option to print the origin of a config\n value\n\n Documentation/git-config.txt | 19 ++++--\n builtin/config.c | 35 +++++++++++\n cache.h | 1 +\n config.c | 23 +++++---\n t/t1300-repo-config.sh | 136 ++++++++++++++++++++++++++++++++++++++++++-\n t/t1308-config-set.sh | 4 +-\n 6 files changed, 202 insertions(+), 16 deletions(-)\n\n--\n2.5.1\n"},{"id":"278069","messageId":"1455373456-64691-2-git-send-email-larsxschneider@gmail.com","threadId":"41407","inReplyTo":"1455373456-64691-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-13T14:24:14Z","receivedAt":"2016-02-13T14:24:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nHelped-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n Documentation/git-config.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 2608ca7..59b1c95 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -219,7 +219,9 @@ See also <>.\n \n --[no-]includes::\n \tRespect `include.*` directives in config files when looking up\n-\tvalues. Defaults to on.\n+\tvalues. Defaults to off when reading a specific config file\n+\t(e.g. via `--file` or via `--local` etc.), and to on when\n+\tgenerically reading all config.\n \n [[FILES]]\n FILES\n-- \n2.5.1\n"},{"id":"278071","messageId":"1455373456-64691-3-git-send-email-larsxschneider@gmail.com","threadId":"41407","inReplyTo":"1455373456-64691-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-13T14:24:15Z","receivedAt":"2016-02-13T14:24:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse the config type to print more detailed error messages that inform\nthe user about the origin of a config error (file, stdin, blob).\n\nSigned-off-by: Lars Schneider \n---\n config.c | 17 ++++++++++-------\n t/t1300-repo-config.sh | 8 +++++++-\n t/t1308-config-set.sh | 4 ++--\n 3 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 86a5eb2..6a5942f 100644\n--- a/config.c\n+++ b/config.c\n@@ -24,6 +24,7 @@ struct config_source {\n \t\t\tsize_t pos;\n \t\t} buf;\n \t} u;\n+\tconst char *type;\n \tconst char *name;\n \tconst char *path;\n \tint die_on_error;\n@@ -471,9 +472,9 @@ static int git_parse_source(config_fn_t fn, void *data)\n \t\t\tbreak;\n \t}\n \tif (cf->die_on_error)\n-\t\tdie(_(\"bad config file line %d in %s\"), cf->linenr, cf->name);\n+\t\tdie(_(\"bad config line %d in %s %s\"), cf->linenr, cf->type, cf->name);\n \telse\n-\t\treturn error(_(\"bad config file line %d in %s\"), cf->linenr, cf->name);\n+\t\treturn error(_(\"bad config line %d in %s %s\"), cf->linenr, cf->type, cf->name);\n }\n \n static int parse_unit_factor(const char *end, uintmax_t *val)\n@@ -588,9 +589,9 @@ static void die_bad_number(const char *name, const char *value)\n \tif (!value)\n \t\tvalue = \"\";\n \n-\tif (cf && cf->name)\n-\t\tdie(_(\"bad numeric config value '%s' for '%s' in %s: %s\"),\n-\t\t value, name, cf->name, reason);\n+\tif (cf && cf->type && cf->name)\n+\t\tdie(_(\"bad numeric config value '%s' for '%s' in %s %s: %s\"),\n+\t\t value, name, cf->type, cf->name, reason);\n \tdie(_(\"bad numeric config value '%s' for '%s': %s\"), value, name, reason);\n }\n \n@@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,\n \tstruct config_source top;\n \n \ttop.u.file = f;\n-\ttop.name = name;\n+\ttop.type = path ? \"file\" : \"stdin\";\n+\ttop.name = name ? name : \"\";\n \ttop.path = path;\n \ttop.die_on_error = 1;\n \ttop.do_fgetc = config_file_fgetc;\n@@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,\n \n static int git_config_from_stdin(config_fn_t fn, void *data)\n {\n-\treturn do_config_from_file(fn, \"\", NULL, stdin, data);\n+\treturn do_config_from_file(fn, NULL, NULL, stdin, data);\n }\n \n int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n@@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n \ttop.u.buf.buf = buf;\n \ttop.u.buf.len = len;\n \ttop.u.buf.pos = 0;\n+\ttop.type = \"blob\";\n \ttop.name = name;\n \ttop.path = NULL;\n \ttop.die_on_error = 0;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 52678e7..7abdfcb 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -700,12 +700,18 @@ test_expect_success 'invalid unit' '\n \tgit config aninvalid.unit >actual &&\n \ttest_cmp expect actual &&\n \tcat >expect <<-\\EOF &&\n-\tfatal: bad numeric config value '\\''1auto'\\'' for '\\''aninvalid.unit'\\'' in .git/config: invalid unit\n+\tfatal: bad numeric config value '\\''1auto'\\'' for '\\''aninvalid.unit'\\'' in file .git/config: invalid unit\n \tEOF\n \ttest_must_fail git config --int --get aninvalid.unit 2>actual &&\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'invalid stdin config' '\n+\techo \"fatal: bad config line 1 in stdin \" >expect &&\n+\techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n+\ttest_cmp expect output\n+'\n+\n cat > expect << EOF\n true\n false\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 91235b7..82f82a1 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -195,14 +195,14 @@ test_expect_success 'proper error on error in default config files' '\n \tcp .git/config .git/config.old &&\n \ttest_when_finished \"mv .git/config.old .git/config\" &&\n \techo \"[\" >>.git/config &&\n-\techo \"fatal: bad config file line 34 in .git/config\" >expect &&\n+\techo \"fatal: bad config line 34 in file .git/config\" >expect &&\n \ttest_expect_code 128 test-config get_value foo.bar 2>actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'proper error on error in custom config files' '\n \techo \"[\" >>syntax-error &&\n-\techo \"fatal: bad config file line 1 in syntax-error\" >expect &&\n+\techo \"fatal: bad config line 1 in file syntax-error\" >expect &&\n \ttest_expect_code 128 test-config configset_get_value foo.bar syntax-error 2>actual &&\n \ttest_cmp expect actual\n '\n-- \n2.5.1\n"},{"id":"278070","messageId":"1455373456-64691-4-git-send-email-larsxschneider@gmail.com","threadId":"41407","inReplyTo":"1455373456-64691-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-13T14:24:16Z","receivedAt":"2016-02-13T14:24:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nIf config values are queried using 'git config' (e.g. via --get,\n--get-all, --get-regexp, or --list flag) then it is sometimes hard to\nfind the configuration file where the values were defined.\n\nTeach 'git config' the '--show-origin' option to print the source\nconfiguration file for every printed value.\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n Documentation/git-config.txt | 15 +++--\n builtin/config.c | 35 ++++++++++++\n cache.h | 1 +\n config.c | 6 ++\n t/t1300-repo-config.sh | 128 +++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 180 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 59b1c95..2bf4d4e 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,18 +9,18 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [] [type] [-z|--null] name [value [value_regex]]\n+'git config' [] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n 'git config' [] [type] --add name value\n 'git config' [] [type] --replace-all name value [value_regex]\n-'git config' [] [type] [-z|--null] --get name [value_regex]\n-'git config' [] [type] [-z|--null] --get-all name [value_regex]\n-'git config' [] [type] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]\n+'git config' [] [type] [--show-origin] [-z|--null] --get name [value_regex]\n+'git config' [] [type] [--show-origin] [-z|--null] --get-all name [value_regex]\n+'git config' [] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]\n 'git config' [] [type] [-z|--null] --get-urlmatch name URL\n 'git config' [] --unset name [value_regex]\n 'git config' [] --unset-all name [value_regex]\n 'git config' [] --rename-section old_name new_name\n 'git config' [] --remove-section name\n-'git config' [] [-z|--null] [--name-only] -l | --list\n+'git config' [] [--show-origin] [-z|--null] [--name-only] -l | --list\n 'git config' [] --get-color name [default]\n 'git config' [] --get-colorbool name [stdout-is-tty]\n 'git config' [] -e | --edit\n@@ -194,6 +194,11 @@ See also <>.\n \tOutput only the names of config variables for `--list` or\n \t`--get-regexp`.\n \n+--show-origin::\n+\tAugment the output of all queried config options with the\n+\torigin type (file, stdin, blob, cmdline) and the actual origin\n+\t(config file path, ref, or blob id if applicable).\n+\n --get-colorbool name [stdout-is-tty]::\n \n \tFind the color setting for `name` (e.g. `color.diff`) and output\ndiff --git a/builtin/config.c b/builtin/config.c\nindex adc7727..302b265 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -3,6 +3,7 @@\n #include \"color.h\"\n #include \"parse-options.h\"\n #include \"urlmatch.h\"\n+#include \"quote.h\"\n \n static const char *const builtin_config_usage[] = {\n \tN_(\"git config []\"),\n@@ -27,6 +28,7 @@ static int actions, types;\n static const char *get_color_slot, *get_colorbool_slot;\n static int end_null;\n static int respect_includes = -1;\n+static int show_origin;\n \n #define ACTION_GET (1<<0)\n #define ACTION_GET_ALL (1<<1)\n@@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n+\tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, stdin, blob, cmdline)\")),\n \tOPT_END(),\n };\n \n@@ -91,8 +94,30 @@ static void check_argc(int argc, int min, int max) {\n \tusage_with_options(builtin_config_usage, builtin_config_options);\n }\n \n+static void show_config_origin(struct strbuf *buf)\n+{\n+\tconst char term = end_null ? '\\0' : '\\t';\n+\tconst char *type;\n+\tconst char *name;\n+\n+\tcurrent_config_type_name(&type, &name);\n+\tstrbuf_addstr(buf, type);\n+\tstrbuf_addch(buf, ':');\n+\tif (end_null)\n+\t\tstrbuf_addstr(buf, name);\n+\telse\n+\t\tquote_c_style(name, buf, NULL, 0);\n+\tstrbuf_addch(buf, term);\n+}\n+\n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n+\tif (show_origin) {\n+ struct strbuf buf = STRBUF_INIT;\n+ show_config_origin(&buf);\n+ fwrite(buf.buf, 1, buf.len, stdout);\n+ strbuf_release(&buf);\n+\t}\n \tif (!omit_values && value_)\n \t\tprintf(\"%s%c%s%c\", key_, delim, value_, term);\n \telse\n@@ -108,6 +133,8 @@ struct strbuf_list {\n \n static int format_config(struct strbuf *buf, const char *key_, const char *value_)\n {\n+\tif (show_origin)\n+\t\tshow_config_origin(buf);\n \tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n \tif (!omit_values) {\n@@ -538,6 +565,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\terror(\"--name-only is only applicable to --list or --get-regexp\");\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n+\n+\tif (show_origin && !(actions &\n+\t\t(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) {\n+\t\terror(\"--show-origin is only applicable to --get, --get-all, \"\n+\t\t\t \"--get-regexp, and --list.\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n+\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\ndiff --git a/cache.h b/cache.h\nindex c63fcc1..3d1eb86 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1525,6 +1525,7 @@ extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \n extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n+extern void current_config_type_name(const char **type, const char **name);\n \n struct config_include_data {\n \tint depth;\ndiff --git a/config.c b/config.c\nindex 6a5942f..0603d4b 100644\n--- a/config.c\n+++ b/config.c\n@@ -2388,3 +2388,9 @@ int parse_config_key(const char *var,\n \n \treturn 0;\n }\n+\n+void current_config_type_name(const char **type, const char **name)\n+{\n+\t*type = cf && cf->type ? cf->type : \"cmdline\";\n+\t*name = cf && cf->name ? cf->name : \"\";\n+}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 7abdfcb..112f7c8 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1207,4 +1207,132 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '\n \t \"die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600\"\n '\n \n+test_expect_success '--show-origin' '\n+\t>.git/config &&\n+\t>\"$HOME\"/.gitconfig &&\n+\tINCLUDE_DIR=\"$HOME/include\" &&\n+\tmkdir -p \"$INCLUDE_DIR\" &&\n+\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n+\t\t[user]\n+\t\t\tabsolute = include\n+\tEOF\n+\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n+\t\t[user]\n+\t\t\trelative = include\n+\tEOF\n+\ttest_config_global user.global \"true\" &&\n+\ttest_config_global user.override \"global\" &&\n+\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n+\ttest_config include.path ../include/relative.include &&\n+\ttest_config user.local \"true\" &&\n+\ttest_config user.override \"local\" &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile:$HOME/.gitconfig\tuser.global=true\n+\t\tfile:$HOME/.gitconfig\tuser.override=global\n+\t\tfile:$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/absolute.include\n+\t\tfile:$INCLUDE_DIR/absolute.include\tuser.absolute=include\n+\t\tfile:.git/config\tinclude.path=../include/relative.include\n+\t\tfile:.git/../include/relative.include\tuser.relative=include\n+\t\tfile:.git/config\tuser.local=true\n+\t\tfile:.git/config\tuser.override=local\n+\t\tcmdline:\tuser.cmdline=true\n+\tEOF\n+\tgit -c user.cmdline=true config --list --show-origin >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile:$HOME/.gitconfigQuser.global\n+\t\ttrueQfile:$HOME/.gitconfigQuser.override\n+\t\tglobalQfile:$HOME/.gitconfigQinclude.path\n+\t\t$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute\n+\t\tincludeQfile:.git/configQinclude.path\n+\t\t../include/relative.includeQfile:.git/../include/relative.includeQuser.relative\n+\t\tincludeQfile:.git/configQuser.local\n+\t\ttrueQfile:.git/configQuser.override\n+\t\tlocalQcmdline:Quser.cmdline\n+\t\ttrueQ\n+\tEOF\n+\tgit -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&\n+\techo >>output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t\tfile:.git/config\tinclude.path=../include/relative.include\n+\t\tfile:.git/config\tuser.local=true\n+\t\tfile:.git/config\tuser.override=local\n+\tEOF\n+\tgit config --local --list --show-origin >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile:$HOME/.gitconfig\tuser.global true\n+\t\tfile:.git/config\tuser.local true\n+\tEOF\n+\tgit config --show-origin --get-regexp \"user\\.[g|l].*\" >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t\tfile:.git/config\tlocal\n+\tEOF\n+\tgit config --show-origin user.override >output &&\n+\ttest_cmp expect output\n+'\n+\n+CUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n+cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF &&\n+\t[user]\n+\t\tcustom = true\n+EOF\n+\n+test_expect_success '--show-origin escape special file name characters' '\n+\tcat >expect <<-\\EOF &&\n+\t\tfile:\"file\\\\twith\\\\ttabs.conf\"\tuser.custom=true\n+\tEOF\n+\tgit config --file \"$CUSTOM_CONFIG_FILE\" --show-origin --list >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--show-origin stdin' '\n+\tcat >expect <<-\\EOF &&\n+\t\tstdin:\tuser.custom=true\n+\tEOF\n+\tgit config --file - --show-origin --list <\"$CUSTOM_CONFIG_FILE\" >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--show-origin stdin with file include' '\n+\tINCLUDE_DIR=\"$HOME/include\" &&\n+\tmkdir -p \"$INCLUDE_DIR\" &&\n+\tcat >\"$INCLUDE_DIR\"/stdin.include <<-EOF &&\n+\t\t[user]\n+\t\t\tstdin = include\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tfile:$INCLUDE_DIR/stdin.include\tinclude\n+\tEOF\n+\techo \"[include]path=\\\"$INCLUDE_DIR\\\"/stdin.include\" \\\n+\t\t| git config --show-origin --includes --file - user.stdin >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--show-origin blob' '\n+\tcat >expect <<-\\EOF &&\n+\t\tblob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08\tuser.custom=true\n+\tEOF\n+\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n+\tgit config --blob=$blob --show-origin --list >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--show-origin blob ref' '\n+\tcat >expect <<-\\EOF &&\n+\t\tblob:\"master:file\\\\twith\\\\ttabs.conf\"\tuser.custom=true\n+\tEOF\n+\tgit add \"$CUSTOM_CONFIG_FILE\" &&\n+\tgit commit -m \"new config file\" &&\n+\tgit config --blob=master:\"$CUSTOM_CONFIG_FILE\" --show-origin --list >output &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n-- \n2.5.1\n"},{"id":"278072","messageId":"CANoM8SWj08qKvDXhyfRXLV2iuYJ7Vjdutzjt0Q2ZNOcProqy=Q@mail.gmail.com","threadId":"41407","inReplyTo":"1455373456-64691-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-02-13T14:43:34Z","receivedAt":"2016-02-13T14:43:34Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Sat, Feb 13, 2016 at 9:24 AM, wrote:\n> From: Lars Schneider \n>\n> diff to v2:\n> * rename cmd to cmdline\n> * rename function current_config_name to current_config_type_name\n> * add 'type' to config_source struct that identifies config type\n> * fix config stdin error output and add test case \"invalid stdin config\"\n> * change delimiter between type and name from tab to colon\n> * remove is_query_action variable\n> * rename \"--sources\" to \"--show-origin\"\n> * add pathological test case \"--show-origin stdin with file include\"\n> * enumerate all valid commandline cases for \"--show-origin\"\n> * removed TODOs as there are no config include bugs\n> * describe '--includes' default behavior in git-config.txt\n> * split test cases\n> * use non-interpolated here-docs where possible\n> * improve readablity of 'show_config_origin' function by removing duality\n>\n> I renamed the flag from \"--source\" to \"--show-origin\" as I got the impression\n> that Sebastian, Peff and Ramsay seem to be all OK with \"--show-origin\".\n\nI know that I am late to the party here, but why not make the option\n`--verbose` or `-v`? `git config` doesn't have that now, and this\nseems like a logical thing to include as verbose data. I would\nventure to guess that `--verbose` is more likely to be the first thing\nthat someone who is looking for this information will guess at before\nthey run `git config --help`.\n\n>\n> Thanks Eric for the hint to simplify the 'show_config_origin' function.\n> I took your suggestion but modfied one line. Instead of \"fputs\" I used\n> \"fwrite\" to write the content. This was necssary as the last char of the\n> string could be \\0 due to the \"--null\" flag. \"fputs\" would ignore that.\n>\n> In 959b545 Heiko introduced a config API to lookup .gitmodules values and\n> in \"submodule-config.c\" he uses the \"git_config_from_buf\" function [1]. I\n> wonder if my modifications to this function could trigger any unwanted side\n> effects in his implementation? (I can't imagine any but I want to make you\n> aware of this connection)\n>\n>\n> Thanks a lot Peff, Sebastian, Ramsey, and Eric for the review.\n>\n>\n> [1] https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/submodule-config.c#L430-L431\n>\n>\n> Lars Schneider (3):\n> git-config.txt: describe '--includes' default behavior\n> config: add 'type' to config_source struct that identifies config type\n> config: add '--show-origin' option to print the origin of a config\n> value\n>\n> Documentation/git-config.txt | 19 ++++--\n> builtin/config.c | 35 +++++++++++\n> cache.h | 1 +\n> config.c | 23 +++++---\n> t/t1300-repo-config.sh | 136 ++++++++++++++++++++++++++++++++++++++++++-\n> t/t1308-config-set.sh | 4 +-\n> 6 files changed, 202 insertions(+), 16 deletions(-)\n>\n> --\n> 2.5.1\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"278076","messageId":"20160213171704.GF30144@sigill.intra.peff.net","threadId":"41407","inReplyTo":"1455373456-64691-2-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T17:17:04Z","receivedAt":"2016-02-13T17:17:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 03:24:14PM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> Helped-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> Documentation/git-config.txt | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 2608ca7..59b1c95 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -219,7 +219,9 @@ See also <>.\n> \n> --[no-]includes::\n> \tRespect `include.*` directives in config files when looking up\n> -\tvalues. Defaults to on.\n> +\tvalues. Defaults to off when reading a specific config file\n> +\t(e.g. via `--file` or via `--local` etc.), and to on when\n> +\tgenerically reading all config.\n\nHmph. I wondered why you were confused about this the other day, when I\nchecked my Documentation/git-config.txt and found that yes, we do indeed\ndocument this behavior. I did not realize then that my patch from:\n\n http://article.gmane.org/gmane.comp.version-control.git/262641\n\nwas never picked up (but of course I've been carrying it in my tree for\na year). I guess maybe you found it, given the similarity of the\nwording. I mildly prefer the wording and formatting of my original, but\nI am OK either way. :)\n\n-Peff\n"},{"id":"278077","messageId":"20160213172435.GG30144@sigill.intra.peff.net","threadId":"41407","inReplyTo":"1455373456-64691-3-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T17:24:35Z","receivedAt":"2016-02-13T17:24:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> Use the config type to print more detailed error messages that inform\n> the user about the origin of a config error (file, stdin, blob).\n\nThis looks OK overall. A few minor nits...\n\n> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n> \ttop.u.buf.buf = buf;\n> \ttop.u.buf.len = len;\n> \ttop.u.buf.pos = 0;\n> +\ttop.type = \"blob\";\n> \ttop.name = name;\n> \ttop.path = NULL;\n> \ttop.die_on_error = 0;\n\nThis function is about reading config from a memory buffer. The only reason\nwe do so _now_ is when reading from a blob, but I think it is laying a\ntrap for somebody who wants to reuse the function later.\n\nShould git_config_from_buf() take a \"type\" parameter, and\ngit_config_from_blob_sha1() pass in \"blob\"?\n\n> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,\n> \tstruct config_source top;\n> \n> \ttop.u.file = f;\n> -\ttop.name = name;\n> +\ttop.type = path ? \"file\" : \"stdin\";\n> +\ttop.name = name ? name : \"\";\n> \ttop.path = path;\n> \ttop.die_on_error = 1;\n> \ttop.do_fgetc = config_file_fgetc;\n> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,\n> \n> static int git_config_from_stdin(config_fn_t fn, void *data)\n> {\n> -\treturn do_config_from_file(fn, \"\", NULL, stdin, data);\n> +\treturn do_config_from_file(fn, NULL, NULL, stdin, data);\n> }\n\nLikewise here, we make assumptions in do_config_from_file() about what\nthe NULL path means. I think this is less likely to be a problem than\nthe other case, but it seems like it would be cleaner for \"file\" or\n\"stdin\" to come from the caller, which knows for sure what we are doing.\n\nSimilarly, I think git_config_from_stdin() can simply pass in an empty\nname rather than NULL to avoid do_config_from_file() having to fix it\nup.\n\n> +test_expect_success 'invalid stdin config' '\n> +\techo \"fatal: bad config line 1 in stdin \" >expect &&\n> +\techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n> +\ttest_cmp expect output\n> +'\n\nThe original would have been \"bad config file line 1 in \"; I\nthink this is an improvement to drop the \"file\" here.\n\n-Peff\n"},{"id":"278078","messageId":"CAHGBnuOu4wWYLkYZdgHuaM4D0Ay8J3xNb1towtTj7FcLK1zVdw@mail.gmail.com","threadId":"41407","inReplyTo":"CANoM8SWj08qKvDXhyfRXLV2iuYJ7Vjdutzjt0Q2ZNOcProqy=Q@mail.gmail.com","subject":"Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-13T17:26:51Z","receivedAt":"2016-02-13T17:26:51Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Sat, Feb 13, 2016 at 3:43 PM, Mike Rappazzo wrote:\n\n>> I renamed the flag from \"--source\" to \"--show-origin\" as I got the impression\n>> that Sebastian, Peff and Ramsay seem to be all OK with \"--show-origin\".\n>\n> I know that I am late to the party here, but why not make the option\n> `--verbose` or `-v`? `git config` doesn't have that now, and this\n> seems like a logical thing to include as verbose data. I would\n\n`--verbose` would be fine with me.\n\n-- \nSebastian Schuberth\n"},{"id":"278079","messageId":"20160213174449.GH30144@sigill.intra.peff.net","threadId":"41407","inReplyTo":"1455373456-64691-4-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T17:44:50Z","receivedAt":"2016-02-13T17:44:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> If config values are queried using 'git config' (e.g. via --get,\n> --get-all, --get-regexp, or --list flag) then it is sometimes hard to\n> find the configuration file where the values were defined.\n> \n> Teach 'git config' the '--show-origin' option to print the source\n> configuration file for every printed value.\n\nThanks, I think this version fixes the correctness issues I mentioned\nearlier. I do still have nits to pick (of course :) ), that we may or\nmay not want to deal with.\n\n> +static void show_config_origin(struct strbuf *buf)\n> +{\n> +\tconst char term = end_null ? '\\0' : '\\t';\n> +\tconst char *type;\n> +\tconst char *name;\n> +\n> +\tcurrent_config_type_name(&type, &name);\n\nThis double out-parameter feels like a clunky interface.\n\nI was tempted to suggest that we simply make the \"struct config_source\"\navailable to builtin/config.c (which is already pretty intimate with the\nrest of the config code), and then it can pick out what it wants. But\nthere _is_ some logic in the function to convert the NULL \"cf\" into\n\"cmdline\".\n\nPerhaps it would be simpler to just have two accessor functions, and do:\n\n strbuf_addstr(buf, current_config_type());\n ...\n strbuf_addstr(buf, current_config_name());\n\nI admit it is a pretty minor point, though.\n\n> static int show_all_config(const char *key_, const char *value_, void *cb)\n> {\n> +\tif (show_origin) {\n> + struct strbuf buf = STRBUF_INIT;\n> + show_config_origin(&buf);\n> + fwrite(buf.buf, 1, buf.len, stdout);\n> + strbuf_release(&buf);\n> +\t}\n\nThe indentation is funky here.\n\nThe use of fwrite() to catch the embedded NULs is subtle enough that it\nmight be worth a comment.\n\nIt also made me wonder how format_config() handles this. It looks like\nwe send the result eventually to fwrite() there, so it all works (and it\ndoes _not_ have the comment I mentioned :) ).\n\n> +test_expect_success '--show-origin' '\n> +\t>.git/config &&\n> +\t>\"$HOME\"/.gitconfig &&\n> +\tINCLUDE_DIR=\"$HOME/include\" &&\n> +\tmkdir -p \"$INCLUDE_DIR\" &&\n> +\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n> +\t\t[user]\n> +\t\t\tabsolute = include\n> +\tEOF\n> +\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n> +\t\t[user]\n> +\t\t\trelative = include\n> +\tEOF\n> +\ttest_config_global user.global \"true\" &&\n> +\ttest_config_global user.override \"global\" &&\n> +\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n> +\ttest_config include.path ../include/relative.include &&\n> +\ttest_config user.local \"true\" &&\n> +\ttest_config user.override \"local\" &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\t\tfile:$HOME/.gitconfig\tuser.global=true\n> +\t\tfile:$HOME/.gitconfig\tuser.override=global\n> +\t\tfile:$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/absolute.include\n> +\t\tfile:$INCLUDE_DIR/absolute.include\tuser.absolute=include\n> +\t\tfile:.git/config\tinclude.path=../include/relative.include\n> +\t\tfile:.git/../include/relative.include\tuser.relative=include\n> +\t\tfile:.git/config\tuser.local=true\n> +\t\tfile:.git/config\tuser.override=local\n> +\t\tcmdline:\tuser.cmdline=true\n> +\tEOF\n> +\tgit -c user.cmdline=true config --list --show-origin >output &&\n> +\ttest_cmp expect output &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\t\tfile:$HOME/.gitconfigQuser.global\n> +\t\ttrueQfile:$HOME/.gitconfigQuser.override\n> +\t\tglobalQfile:$HOME/.gitconfigQinclude.path\n> +\t\t$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute\n> +\t\tincludeQfile:.git/configQinclude.path\n> +\t\t../include/relative.includeQfile:.git/../include/relative.includeQuser.relative\n> +\t\tincludeQfile:.git/configQuser.local\n> +\t\ttrueQfile:.git/configQuser.override\n> +\t\tlocalQcmdline:Quser.cmdline\n> +\t\ttrueQ\n> +\tEOF\n\nI see you split this up more, but there's still quite a bit going on in\nthis one block. IMHO, it would be more customary in our tests to put the\nsetup into one test_expect_success block, then each of these\nexpect-run-cmp blocks into their own test_expect_success.\n\nIt does mean that the setup mutates the global test state for further\ntests (and you should stop using test_config_*, which clean up at the\nend of the block), but I think that's the right thing here. The point of\ntest_config is \"flip on this switch just for a moment, so we can test\nits effect without hurting further tests\". But these are config tests in\nthe first place, and it is OK for them to show a progression of\nmutations of the config (you'll note that like the other tests in this\nscript, you are clearing out .git/config in the first place).\n\n-Peff\n"},{"id":"278080","messageId":"20160213180412.GA9516@sigill.intra.peff.net","threadId":"41407","inReplyTo":"20160213174449.GH30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T18:04:13Z","receivedAt":"2016-02-13T18:04:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:\n\n> > +test_expect_success '--show-origin' '\n> [...]\n> I see you split this up more, but there's still quite a bit going on in\n> this one block. IMHO, it would be more customary in our tests to put the\n> setup into one test_expect_success block, then each of these\n> expect-run-cmp blocks into their own test_expect_success.\n\nHere's a squashable patch that shows what I mean.\n\n---\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 112f7c8..c7496aa 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1207,26 +1207,34 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '\n \t \"die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600\"\n '\n \n-test_expect_success '--show-origin' '\n-\t>.git/config &&\n-\t>\"$HOME\"/.gitconfig &&\n+test_expect_success 'set up --show-origin tests' '\n \tINCLUDE_DIR=\"$HOME/include\" &&\n \tmkdir -p \"$INCLUDE_DIR\" &&\n-\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n+\tcat >\"$INCLUDE_DIR\"/absolute.include <<-\\EOF &&\n \t\t[user]\n \t\t\tabsolute = include\n \tEOF\n-\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n+\tcat >\"$INCLUDE_DIR\"/relative.include <<-\\EOF &&\n \t\t[user]\n \t\t\trelative = include\n \tEOF\n-\ttest_config_global user.global \"true\" &&\n-\ttest_config_global user.override \"global\" &&\n-\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n-\ttest_config include.path ../include/relative.include &&\n-\ttest_config user.local \"true\" &&\n-\ttest_config user.override \"local\" &&\n+\tcat >\"$HOME\"/.gitconfig <<-EOF &&\n+\t\t[user]\n+\t\t\tglobal = true\n+\t\t\toverride = global\n+\t\t[include]\n+\t\t\tpath = \"$INCLUDE_DIR/absolute.include\"\n+\tEOF\n+\tcat >.git/config <<-\\EOF\n+\t\t[include]\n+\t\t\tpath = ../include/relative.include\n+\t\t[user]\n+\t\t\tlocal = true\n+\t\t\toverride = local\n+\tEOF\n+'\n \n+test_expect_success '--show-origin with --list' '\n \tcat >expect <<-EOF &&\n \t\tfile:$HOME/.gitconfig\tuser.global=true\n \t\tfile:$HOME/.gitconfig\tuser.override=global\n@@ -1239,8 +1247,10 @@ test_expect_success '--show-origin' '\n \t\tcmdline:\tuser.cmdline=true\n \tEOF\n \tgit -c user.cmdline=true config --list --show-origin >output &&\n-\ttest_cmp expect output &&\n+\ttest_cmp expect output\n+'\n \n+test_expect_success '--show-origin with --list --null' '\n \tcat >expect <<-EOF &&\n \t\tfile:$HOME/.gitconfigQuser.global\n \t\ttrueQfile:$HOME/.gitconfigQuser.override\n@@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '\n \t\tlocalQcmdline:Quser.cmdline\n \t\ttrueQ\n \tEOF\n-\tgit -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&\n+\tgit -c user.cmdline=true config --null --list --show-origin >output.raw &&\n+\tnul_to_q output &&\n \techo >>output &&\n-\ttest_cmp expect output &&\n+\ttest_cmp expect output\n+'\n \n+test_expect_success '--show-origin with single file' '\n \tcat >expect <<-\\EOF &&\n \t\tfile:.git/config\tinclude.path=../include/relative.include\n \t\tfile:.git/config\tuser.local=true\n \t\tfile:.git/config\tuser.override=local\n \tEOF\n \tgit config --local --list --show-origin >output &&\n-\ttest_cmp expect output &&\n+\ttest_cmp expect output\n+'\n \n+test_expect_success '--show-origin with --get-regexp' '\n \tcat >expect <<-EOF &&\n \t\tfile:$HOME/.gitconfig\tuser.global true\n \t\tfile:.git/config\tuser.local true\n \tEOF\n \tgit config --show-origin --get-regexp \"user\\.[g|l].*\" >output &&\n-\ttest_cmp expect output &&\n+\ttest_cmp expect output\n+'\n \n+test_expect_success '--show-origin getting a single key' '\n \tcat >expect <<-\\EOF &&\n \t\tfile:.git/config\tlocal\n \tEOF\n@@ -1279,11 +1296,13 @@ test_expect_success '--show-origin' '\n \ttest_cmp expect output\n '\n \n-CUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n-cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF &&\n-\t[user]\n-\t\tcustom = true\n-EOF\n+test_expect_success 'set up custom config file' '\n+\tCUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n+\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n+\t\t[user]\n+\t\t\tcustom = true\n+\tEOF\n+'\n \n test_expect_success '--show-origin escape special file name characters' '\n \tcat >expect <<-\\EOF &&\n@@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '\n '\n \n test_expect_success '--show-origin stdin with file include' '\n-\tINCLUDE_DIR=\"$HOME/include\" &&\n-\tmkdir -p \"$INCLUDE_DIR\" &&\n \tcat >\"$INCLUDE_DIR\"/stdin.include <<-EOF &&\n \t\t[user]\n \t\t\tstdin = include\n"},{"id":"278081","messageId":"20160213181529.GB9516@sigill.intra.peff.net","threadId":"41407","inReplyTo":"20160213180412.GA9516@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T18:15:29Z","receivedAt":"2016-02-13T18:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:\n\n> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:\n> \n> > > +test_expect_success '--show-origin' '\n> > [...]\n> > I see you split this up more, but there's still quite a bit going on in\n> > this one block. IMHO, it would be more customary in our tests to put the\n> > setup into one test_expect_success block, then each of these\n> > expect-run-cmp blocks into their own test_expect_success.\n> \n> Here's a squashable patch that shows what I mean.\n\nAnd here are a few comments on the changes...\n\n> -test_expect_success '--show-origin' '\n> -\t>.git/config &&\n> -\t>\"$HOME\"/.gitconfig &&\n> +test_expect_success 'set up --show-origin tests' '\n> \tINCLUDE_DIR=\"$HOME/include\" &&\n> \tmkdir -p \"$INCLUDE_DIR\" &&\n> -\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n> +\tcat >\"$INCLUDE_DIR\"/absolute.include <<-\\EOF &&\n> \t\t[user]\n> \t\t\tabsolute = include\n> \tEOF\n> -\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n> +\tcat >\"$INCLUDE_DIR\"/relative.include <<-\\EOF &&\n> \t\t[user]\n> \t\t\trelative = include\n> \tEOF\n> -\ttest_config_global user.global \"true\" &&\n> -\ttest_config_global user.override \"global\" &&\n> -\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n> -\ttest_config include.path ../include/relative.include &&\n> -\ttest_config user.local \"true\" &&\n> -\ttest_config user.override \"local\" &&\n> +\tcat >\"$HOME\"/.gitconfig <<-EOF &&\n> +\t\t[user]\n> +\t\t\tglobal = true\n> +\t\t\toverride = global\n> +\t\t[include]\n> +\t\t\tpath = \"$INCLUDE_DIR/absolute.include\"\n> +\tEOF\n> +\tcat >.git/config <<-\\EOF\n> +\t\t[include]\n> +\t\t\tpath = ../include/relative.include\n> +\t\t[user]\n> +\t\t\tlocal = true\n> +\t\t\toverride = local\n> +\tEOF\n\nI preserved your ordering here (as the later \"--list\" tests care). But\nit might be worth ordering both files the same way, so that a reader\ndoes not wonder if it is significant (and just update the --list\noutput expectation later).\n\n> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '\n> \t\tlocalQcmdline:Quser.cmdline\n> \t\ttrueQ\n> \tEOF\n> -\tgit -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&\n> +\tgit -c user.cmdline=true config --null --list --show-origin >output.raw &&\n> +\tnul_to_q output &&\n\nWe usually try to avoid putting git on the left-hand side of a pipe,\nbecause it hides the exit code, and we want to make sure git does not\nfail. I won't be surprised if you copied the style from elsewhere in the\nscript, though, as this is an old one and we were not always consistent.\n\n> \techo >>output &&\n> -\ttest_cmp expect output &&\n> +\ttest_cmp expect output\n\nThis \"echo\" might be worth a comment. I think we are just adding the\nextra newline that the here-doc insists on, but that --null output would\nnot include.\n\nOverall, I find the \"--show-origin --null\" output pretty weird to read.\nWe use a newline to split the config key/value, a NUL between config\nentries, but now also a NUL between the filename and the rest of the\nconfig entry.\n\nThat makes parsing pretty weird, as you cannot just use something like\n\n git config --show-origin --list --null | perl -0ne ...\n\nto process entries; every other entry you get will be a filename. But at\nthe same time, we do not go all out and say \"there is a NUL between each\nfield\", because the key/value separator is a newline in this case, and\nthe reader has to parse that separately after splitting on NULs.\n\nBut I think it's too late to do anything about it now. The weirdness is\nreally the mixed NUL/newline thing, and you are not introducing that.\n\n> -CUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n> -cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF &&\n> -\t[user]\n> -\t\tcustom = true\n> -EOF\n> +test_expect_success 'set up custom config file' '\n> +\tCUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n> +\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> +\t\t[user]\n> +\t\t\tcustom = true\n> +\tEOF\n> +'\n\nEverything, even mundane setup, should generally go in a test_expect\nblock. That means we'll notice unexpected failures, and any output will\nfollow the usual \"--verbose\" rules.\n\nArguably this setup could just go into the initial setup block.\n\nAlso, you may not that the filename does _not_ actually have tabs in it,\nbecause the shell does not expand \"\\t\". It does have backslashes in it,\nthough, which is enough to trigger our C-style quoting.\n\nSo the test isn't wrong, but the filename is misleading. If you really\nwant tabs, you'd have to do:\n\n CUSTOM_CONFIG_FILE=\"$(printf \"file\\twith\\ttabs.conf\")\n\nor similar.\n\n> test_expect_success '--show-origin escape special file name characters' '\n> \tcat >expect <<-\\EOF &&\n> @@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '\n> '\n> \n> test_expect_success '--show-origin stdin with file include' '\n> -\tINCLUDE_DIR=\"$HOME/include\" &&\n> -\tmkdir -p \"$INCLUDE_DIR\" &&\n> \tcat >\"$INCLUDE_DIR\"/stdin.include <<-EOF &&\n> \t\t[user]\n> \t\t\tstdin = include\n\nHere we can assume that the setup block succeeded (if it didn't, all of\nthe tests are screwed anyway, so you'd want to fix that first).\n\n-Peff\n"},{"id":"278082","messageId":"20160213181901.GC9516@sigill.intra.peff.net","threadId":"41407","inReplyTo":"CANoM8SWj08qKvDXhyfRXLV2iuYJ7Vjdutzjt0Q2ZNOcProqy=Q@mail.gmail.com","subject":"Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-13T18:19:01Z","receivedAt":"2016-02-13T18:19:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 09:43:34AM -0500, Mike Rappazzo wrote:\n\n> > I renamed the flag from \"--source\" to \"--show-origin\" as I got the impression\n> > that Sebastian, Peff and Ramsay seem to be all OK with \"--show-origin\".\n> \n> I know that I am late to the party here, but why not make the option\n> `--verbose` or `-v`? `git config` doesn't have that now, and this\n> seems like a logical thing to include as verbose data. I would\n> venture to guess that `--verbose` is more likely to be the first thing\n> that someone who is looking for this information will guess at before\n> they run `git config --help`.\n\nI'm actually opposed to \"--verbose\" because it is too vague. That is\nfine for a human, but this is also an interface that we expect scripts\nto parse, and which we therefore cannot later change without breaking\nthem. I am not prepared to set \"--verbose\" in stone as showing the\nsource file and nothing else, for all time.\n\nWe _could_ have \"--show-origin\", and then \"--verbose\" implies\n\"--show-origin\", and we don't promise that it might not show more later.\nBut I don't think that is worth the added complexity (not in code, but\njust in what we have to explain to the user).\n\n-Peff\n"},{"id":"278085","messageId":"xmqqpow0s4ux.fsf@gitster.mtv.corp.google.com","threadId":"41407","inReplyTo":"20160213171704.GF30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-13T21:00:22Z","receivedAt":"2016-02-13T21:00:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I did not realize then that my patch from:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/262641\n>\n> was never picked up (but of course I've been carrying it in my tree for\n> a year).\n\nHmm, I do not see how it happened, either. It is not like I was\noffline (I can see I was involved in other threads from the same\ntimeframe). Perhaps it was lost in the noise.\n\nThanks for following up.\n"},{"id":"278086","messageId":"xmqqio1ss4on.fsf@gitster.mtv.corp.google.com","threadId":"41407","inReplyTo":"20160213172435.GG30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-13T21:04:08Z","receivedAt":"2016-02-13T21:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n>> \ttop.u.buf.buf = buf;\n>> \ttop.u.buf.len = len;\n>> \ttop.u.buf.pos = 0;\n>> +\ttop.type = \"blob\";\n>> \ttop.name = name;\n>> \ttop.path = NULL;\n>> \ttop.die_on_error = 0;\n>\n> This function is about reading config from a memory buffer. The only reason\n> we do so _now_ is when reading from a blob, but I think it is laying a\n> trap for somebody who wants to reuse the function later.\n>\n> Should git_config_from_buf() take a \"type\" parameter, and\n> git_config_from_blob_sha1() pass in \"blob\"?\n\nI think that is sensible. I think s/from_buf/from_mem/ may also be\nsensible (it would match the naming used in the index_{fd,mem,...}\nfunctions in he hashing code).\n\n>> static int git_config_from_stdin(config_fn_t fn, void *data)\n>> {\n>> -\treturn do_config_from_file(fn, \"\", NULL, stdin, data);\n>> +\treturn do_config_from_file(fn, NULL, NULL, stdin, data);\n>> }\n>\n> Likewise here, we make assumptions in do_config_from_file() about what\n> the NULL path means. I think this is less likely to be a problem than\n> the other case, but it seems like it would be cleaner for \"file\" or\n> \"stdin\" to come from the caller, which knows for sure what we are doing.\n\nMakes sense.\n\n> Similarly, I think git_config_from_stdin() can simply pass in an empty\n> name rather than NULL to avoid do_config_from_file() having to fix it\n> up.\n\nThis too.\n\n>> +test_expect_success 'invalid stdin config' '\n>> +\techo \"fatal: bad config line 1 in stdin \" >expect &&\n>> +\techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n>> +\ttest_cmp expect output\n>> +'\n>\n> The original would have been \"bad config file line 1 in \"; I\n> think this is an improvement to drop the \"file\" here.\n>\n> -Peff\n"},{"id":"278087","messageId":"xmqqegcgs4lq.fsf@gitster.mtv.corp.google.com","threadId":"41407","inReplyTo":"20160213181901.GC9516@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-13T21:05:53Z","receivedAt":"2016-02-13T21:05:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm actually opposed to \"--verbose\" because it is too vague. That is\n> fine for a human, but this is also an interface that we expect scripts\n> to parse, and which we therefore cannot later change without breaking\n> them. I am not prepared to set \"--verbose\" in stone as showing the\n> source file and nothing else, for all time.\n\nMe neither. Thanks for clearly spelling out why using --verbose for\nthis is not such a good idea.\n"},{"id":"278089","messageId":"20160214003448.GA11268@sigill.intra.peff.net","threadId":"41407","inReplyTo":"xmqqpow0s4ux.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-14T00:34:48Z","receivedAt":"2016-02-14T00:34:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2016 at 01:00:22PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... I did not realize then that my patch from:\n> >\n> > http://article.gmane.org/gmane.comp.version-control.git/262641\n> >\n> > was never picked up (but of course I've been carrying it in my tree for\n> > a year).\n> \n> Hmm, I do not see how it happened, either. It is not like I was\n> offline (I can see I was involved in other threads from the same\n> timeframe). Perhaps it was lost in the noise.\n> \n> Thanks for following up.\n\n>From even my limited time as acting maintainer, I know it is easy for\nthings to occasionally fall through the cracks for no real reason. We\nrely on submitters to be invested enough in their patch series to give a\ngentle prod in such cases. However, I sometimes churn out little patches\nlike this at such a rate that _I_ forget about them, too. :)\n\nI do keep them in my tree, but I have so many half-finished and failed\nexperiments that they get lost in the noise. I should do a better job of\nperiodically reviewing and reposting or discarding my personal topics (I\nhave 65 right now!).\n\n-Peff\n"},{"id":"278107","messageId":"0FE601E5-B926-4E23-9FA5-2F898C12BBB9@gmail.com","threadId":"41407","inReplyTo":"20160213171704.GF30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-14T12:17:43Z","receivedAt":"2016-02-14T12:17:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 13 Feb 2016, at 18:17, Jeff King wrote:\n\n> On Sat, Feb 13, 2016 at 03:24:14PM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> Helped-by: Jeff King \n>> Signed-off-by: Lars Schneider \n>> ---\n>> Documentation/git-config.txt | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n>> index 2608ca7..59b1c95 100644\n>> --- a/Documentation/git-config.txt\n>> +++ b/Documentation/git-config.txt\n>> @@ -219,7 +219,9 @@ See also <>.\n>> \n>> --[no-]includes::\n>> \tRespect `include.*` directives in config files when looking up\n>> -\tvalues. Defaults to on.\n>> +\tvalues. Defaults to off when reading a specific config file\n>> +\t(e.g. via `--file` or via `--local` etc.), and to on when\n>> +\tgenerically reading all config.\n> \n> Hmph. I wondered why you were confused about this the other day, when I\n> checked my Documentation/git-config.txt and found that yes, we do indeed\n> document this behavior. I did not realize then that my patch from:\n> \n> http://article.gmane.org/gmane.comp.version-control.git/262641\n> \n> was never picked up (but of course I've been carrying it in my tree for\n> a year). I guess maybe you found it, given the similarity of the\n> wording. I mildly prefer the wording and formatting of my original, but\n> I am OK either way. :)\n\nOh. Believe it or not but the similarity is coincidental. I referenced you \n(\"Helped-by\") because you explained the expected \"includes\" behavior to\nme in your v2 review of my series. If I would have found your patch, I \nwould have said so.\n\nI am happy to use your wording. How should I proceed? Should I just drop \nmy \"git-config.txt\" patch from my series or should I integrate your patch \ninto my series? If the latter, then does the patch require a \"Signed-off-by:\"\nby me?\n\nThanks,\nLars\n"},{"id":"278109","messageId":"394CD8BC-49CF-4D07-A9C2-CC8C60373A2B@gmail.com","threadId":"41407","inReplyTo":"20160213172435.GG30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-14T12:24:55Z","receivedAt":"2016-02-14T12:24:55Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 13 Feb 2016, at 18:24, Jeff King wrote:\n\n> On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> Use the config type to print more detailed error messages that inform\n>> the user about the origin of a config error (file, stdin, blob).\n> \n> This looks OK overall. A few minor nits...\n> \n>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n>> \ttop.u.buf.buf = buf;\n>> \ttop.u.buf.len = len;\n>> \ttop.u.buf.pos = 0;\n>> +\ttop.type = \"blob\";\n>> \ttop.name = name;\n>> \ttop.path = NULL;\n>> \ttop.die_on_error = 0;\n> \n> This function is about reading config from a memory buffer. The only reason\n> we do so _now_ is when reading from a blob, but I think it is laying a\n> trap for somebody who wants to reuse the function later.\n> \n> Should git_config_from_buf() take a \"type\" parameter, and\n> git_config_from_blob_sha1() pass in \"blob\"?\nHaha, fun fact: this was how I implemented it initially. Because of that\nI noticed that \"submodule-config.c\" also uses \"git_config_from_buf\" :-)\n\nHowever, then I thought the list wouldn't like it if I change the\ninterfaces. I will add the type parameter, again.\n\n\n> \n>> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,\n>> \tstruct config_source top;\n>> \n>> \ttop.u.file = f;\n>> -\ttop.name = name;\n>> +\ttop.type = path ? \"file\" : \"stdin\";\n>> +\ttop.name = name ? name : \"\";\n>> \ttop.path = path;\n>> \ttop.die_on_error = 1;\n>> \ttop.do_fgetc = config_file_fgetc;\n>> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,\n>> \n>> static int git_config_from_stdin(config_fn_t fn, void *data)\n>> {\n>> -\treturn do_config_from_file(fn, \"\", NULL, stdin, data);\n>> +\treturn do_config_from_file(fn, NULL, NULL, stdin, data);\n>> }\n> \n> Likewise here, we make assumptions in do_config_from_file() about what\n> the NULL path means. I think this is less likely to be a problem than\n> the other case, but it seems like it would be cleaner for \"file\" or\n> \"stdin\" to come from the caller, which knows for sure what we are doing.\n> \n> Similarly, I think git_config_from_stdin() can simply pass in an empty\n> name rather than NULL to avoid do_config_from_file() having to fix it\n> up.\nOK\n\n\n> \n>> +test_expect_success 'invalid stdin config' '\n>> +\techo \"fatal: bad config line 1 in stdin \" >expect &&\n>> +\techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n>> +\ttest_cmp expect output\n>> +'\n> \n> The original would have been \"bad config file line 1 in \"; I\n> think this is an improvement to drop the \"file\" here.\n\n\nThanks,\nLars\n"},{"id":"278110","messageId":"A74A1FE2-7C03-448F-8147-BBF276322E62@gmail.com","threadId":"41407","inReplyTo":"xmqqio1ss4on.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-14T12:26:27Z","receivedAt":"2016-02-14T12:26:27Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 13 Feb 2016, at 22:04, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n>>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,\n>>> \ttop.u.buf.buf = buf;\n>>> \ttop.u.buf.len = len;\n>>> \ttop.u.buf.pos = 0;\n>>> +\ttop.type = \"blob\";\n>>> \ttop.name = name;\n>>> \ttop.path = NULL;\n>>> \ttop.die_on_error = 0;\n>> \n>> This function is about reading config from a memory buffer. The only reason\n>> we do so _now_ is when reading from a blob, but I think it is laying a\n>> trap for somebody who wants to reuse the function later.\n>> \n>> Should git_config_from_buf() take a \"type\" parameter, and\n>> git_config_from_blob_sha1() pass in \"blob\"?\n> \n> I think that is sensible. I think s/from_buf/from_mem/ may also be\n> sensible (it would match the naming used in the index_{fd,mem,...}\n> functions in he hashing code).\nOK, I will change that, too!\n\n\n> \n>>> static int git_config_from_stdin(config_fn_t fn, void *data)\n>>> {\n>>> -\treturn do_config_from_file(fn, \"\", NULL, stdin, data);\n>>> +\treturn do_config_from_file(fn, NULL, NULL, stdin, data);\n>>> }\n>> \n>> Likewise here, we make assumptions in do_config_from_file() about what\n>> the NULL path means. I think this is less likely to be a problem than\n>> the other case, but it seems like it would be cleaner for \"file\" or\n>> \"stdin\" to come from the caller, which knows for sure what we are doing.\n> \n> Makes sense.\n> \n>> Similarly, I think git_config_from_stdin() can simply pass in an empty\n>> name rather than NULL to avoid do_config_from_file() having to fix it\n>> up.\n> \n> This too.\n> \n>>> +test_expect_success 'invalid stdin config' '\n>>> +\techo \"fatal: bad config line 1 in stdin \" >expect &&\n>>> +\techo \"[broken\" | test_must_fail git config --list --file - >output 2>&1 &&\n>>> +\ttest_cmp expect output\n>>> +'\n>> \n>> The original would have been \"bad config file line 1 in \"; I\n>> think this is an improvement to drop the \"file\" here.\n>> \n>> -Peff\n\nThanks,\nLars\n"},{"id":"278111","messageId":"CF48D0B6-66BD-4C5D-A93B-AB0BBC00615D@gmail.com","threadId":"41407","inReplyTo":"20160213174449.GH30144@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-14T12:48:59Z","receivedAt":"2016-02-14T12:48:59Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 13 Feb 2016, at 18:44, Jeff King wrote:\n\n> On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> If config values are queried using 'git config' (e.g. via --get,\n>> --get-all, --get-regexp, or --list flag) then it is sometimes hard to\n>> find the configuration file where the values were defined.\n>> \n>> Teach 'git config' the '--show-origin' option to print the source\n>> configuration file for every printed value.\n> \n> Thanks, I think this version fixes the correctness issues I mentioned\n> earlier. I do still have nits to pick (of course :) ), that we may or\n> may not want to deal with.\n> \n>> +static void show_config_origin(struct strbuf *buf)\n>> +{\n>> +\tconst char term = end_null ? '\\0' : '\\t';\n>> +\tconst char *type;\n>> +\tconst char *name;\n>> +\n>> +\tcurrent_config_type_name(&type, &name);\n> \n> This double out-parameter feels like a clunky interface.\n> \n> I was tempted to suggest that we simply make the \"struct config_source\"\n> available to builtin/config.c (which is already pretty intimate with the\n> rest of the config code), and then it can pick out what it wants. But\n> there _is_ some logic in the function to convert the NULL \"cf\" into\n> \"cmdline\".\n> \n> Perhaps it would be simpler to just have two accessor functions, and do:\n> \n> strbuf_addstr(buf, current_config_type());\n> ...\n> strbuf_addstr(buf, current_config_name());\n> \n> I admit it is a pretty minor point, though.\nAgreed, this looks nicer.\n\n\n> \n>> static int show_all_config(const char *key_, const char *value_, void *cb)\n>> {\n>> +\tif (show_origin) {\n>> + struct strbuf buf = STRBUF_INIT;\n>> + show_config_origin(&buf);\n>> + fwrite(buf.buf, 1, buf.len, stdout);\n>> + strbuf_release(&buf);\n>> +\t}\n> \n> The indentation is funky here.\nTrue! Indentation without intention :-) \n\n> \n> The use of fwrite() to catch the embedded NULs is subtle enough that it\n> might be worth a comment.\n> \n> It also made me wonder how format_config() handles this. It looks like\n> we send the result eventually to fwrite() there, so it all works (and it\n> does _not_ have the comment I mentioned :) ).\nI will add a comment in both places :-)\n\n\n> \n>> +test_expect_success '--show-origin' '\n>> +\t>.git/config &&\n>> +\t>\"$HOME\"/.gitconfig &&\n>> +\tINCLUDE_DIR=\"$HOME/include\" &&\n>> +\tmkdir -p \"$INCLUDE_DIR\" &&\n>> +\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n>> +\t\t[user]\n>> +\t\t\tabsolute = include\n>> +\tEOF\n>> +\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n>> +\t\t[user]\n>> +\t\t\trelative = include\n>> +\tEOF\n>> +\ttest_config_global user.global \"true\" &&\n>> +\ttest_config_global user.override \"global\" &&\n>> +\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n>> +\ttest_config include.path ../include/relative.include &&\n>> +\ttest_config user.local \"true\" &&\n>> +\ttest_config user.override \"local\" &&\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\t\tfile:$HOME/.gitconfig\tuser.global=true\n>> +\t\tfile:$HOME/.gitconfig\tuser.override=global\n>> +\t\tfile:$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/absolute.include\n>> +\t\tfile:$INCLUDE_DIR/absolute.include\tuser.absolute=include\n>> +\t\tfile:.git/config\tinclude.path=../include/relative.include\n>> +\t\tfile:.git/../include/relative.include\tuser.relative=include\n>> +\t\tfile:.git/config\tuser.local=true\n>> +\t\tfile:.git/config\tuser.override=local\n>> +\t\tcmdline:\tuser.cmdline=true\n>> +\tEOF\n>> +\tgit -c user.cmdline=true config --list --show-origin >output &&\n>> +\ttest_cmp expect output &&\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\t\tfile:$HOME/.gitconfigQuser.global\n>> +\t\ttrueQfile:$HOME/.gitconfigQuser.override\n>> +\t\tglobalQfile:$HOME/.gitconfigQinclude.path\n>> +\t\t$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute\n>> +\t\tincludeQfile:.git/configQinclude.path\n>> +\t\t../include/relative.includeQfile:.git/../include/relative.includeQuser.relative\n>> +\t\tincludeQfile:.git/configQuser.local\n>> +\t\ttrueQfile:.git/configQuser.override\n>> +\t\tlocalQcmdline:Quser.cmdline\n>> +\t\ttrueQ\n>> +\tEOF\n> \n> I see you split this up more, but there's still quite a bit going on in\n> this one block. IMHO, it would be more customary in our tests to put the\n> setup into one test_expect_success block, then each of these\n> expect-run-cmp blocks into their own test_expect_success.\n> \n> It does mean that the setup mutates the global test state for further\n> tests (and you should stop using test_config_*, which clean up at the\n> end of the block), but I think that's the right thing here. The point of\n> test_config is \"flip on this switch just for a moment, so we can test\n> its effect without hurting further tests\". But these are config tests in\n> the first place, and it is OK for them to show a progression of\n> mutations of the config (you'll note that like the other tests in this\n> script, you are clearing out .git/config in the first place).\n> \nTBH I am always a little annoyed if Git tests depend on each other. It makes\nit harder to just disable all uninteresting tests and only focus on the one that \nI am working with. However, I agree with your point that the test block does too\nmany things. Would it be OK if I write a bash function that performs the test\nsetup? Then I would call this function in the beginning of every individual \ntest. Or do you prefer the global state strategy?\n\nThanks,\nLars"},{"id":"278112","messageId":"20160214160546.GA5875@sigill.intra.peff.net","threadId":"41407","inReplyTo":"0FE601E5-B926-4E23-9FA5-2F898C12BBB9@gmail.com","subject":"Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-14T16:05:47Z","receivedAt":"2016-02-14T16:05:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2016 at 01:17:43PM +0100, Lars Schneider wrote:\n\n> Oh. Believe it or not but the similarity is coincidental. I referenced you \n> (\"Helped-by\") because you explained the expected \"includes\" behavior to\n> me in your v2 review of my series. If I would have found your patch, I \n> would have said so.\n\nHeh. Well, then we can take it as an endorsement, since we both\nindependently wrote almost the same thing. :)\n\n> I am happy to use your wording. How should I proceed? Should I just drop \n> my \"git-config.txt\" patch from my series or should I integrate your patch \n> into my series? If the latter, then does the patch require a \"Signed-off-by:\"\n> by me?\n\nIt looks like Junio picked up my original, so I think we can just treat\nit independently of this topic.\n\n-Peff\n"},{"id":"278113","messageId":"20160214160734.GB5875@sigill.intra.peff.net","threadId":"41407","inReplyTo":"394CD8BC-49CF-4D07-A9C2-CC8C60373A2B@gmail.com","subject":"Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-14T16:07:34Z","receivedAt":"2016-02-14T16:07:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2016 at 01:24:55PM +0100, Lars Schneider wrote:\n\n> > Should git_config_from_buf() take a \"type\" parameter, and\n> > git_config_from_blob_sha1() pass in \"blob\"?\n> Haha, fun fact: this was how I implemented it initially. Because of that\n> I noticed that \"submodule-config.c\" also uses \"git_config_from_buf\" :-)\n> \n> However, then I thought the list wouldn't like it if I change the\n> interfaces. I will add the type parameter, again.\n\nChanging internal interfaces is preferable to carrying a sub-optimal\ninterface forever, I think.\n\nThe thing we really have to watch out for is changing the assumptions or\noutput of a function without changing its signature. The interface\nchange breaks loudly on a merge, and is easy to fix. Changing the\ninternals creates quiet and subtle bugs.\n\n-Peff\n"},{"id":"278114","messageId":"20160214161816.GC5875@sigill.intra.peff.net","threadId":"41407","inReplyTo":"CF48D0B6-66BD-4C5D-A93B-AB0BBC00615D@gmail.com","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-14T16:18:16Z","receivedAt":"2016-02-14T16:18:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2016 at 01:48:59PM +0100, Lars Schneider wrote:\n\n> > I see you split this up more, but there's still quite a bit going on in\n> > this one block. IMHO, it would be more customary in our tests to put the\n> > setup into one test_expect_success block, then each of these\n> > expect-run-cmp blocks into their own test_expect_success.\n> > \n> > It does mean that the setup mutates the global test state for further\n> > tests (and you should stop using test_config_*, which clean up at the\n> > end of the block), but I think that's the right thing here. The point of\n> > test_config is \"flip on this switch just for a moment, so we can test\n> > its effect without hurting further tests\". But these are config tests in\n> > the first place, and it is OK for them to show a progression of\n> > mutations of the config (you'll note that like the other tests in this\n> > script, you are clearing out .git/config in the first place).\n> > \n> TBH I am always a little annoyed if Git tests depend on each other. It makes\n> it harder to just disable all uninteresting tests and only focus on the one that \n> I am working with. However, I agree with your point that the test block does too\n> many things. Would it be OK if I write a bash function that performs the test\n> setup? Then I would call this function in the beginning of every individual \n> test. Or do you prefer the global state strategy?\n\nIn general, my opinion is that skipping arbitrary leading tests is a\nlosing strategy. It's just too easy to introduce hidden dependencies,\nand not worth the programmer time to make sure each test runs in\nisolation. But others on the list may disagree.\n\nThat being said, I think what I am proposing is a much milder form of\nthat. With what I am proposing, you can skip everything _except_ tests\nwhich match /set.?up/ in their description. We do not perfectly adhere\nto that in our tests, but I suspect it works a majority of the time.\n\nIf it is taking too long to get to a particular test in a test script,\nmaybe that is a sign we need to break up the script. There are also a\nfew tricks you can use to still _run_ the earlier blocks, but not have\nthem interfere with debugging a particular test:\n\n 1. Use --verbose-only=123 to get verbose output only from a single\n test.\n\n 2. Use \"-i\" to stop running tests at the first failure. Usually it is\n worth fixing that one, and then seeing if other tests fail, too, or\n were simply dependent.\n\n 3. If you are using --valgrind, the tests run very slowly (normally\n t1300 takes 400ms to run on my machine, so I don't mind waiting\n that long to get to a new test at the end. With valgrind it is more\n like 90 seconds). You can use --valgrind-only=123 to test only the\n block you are debugging, and run the rest quickly.\n\nWe do use shell functions in some places to do repeated setup. In\ngeneral, I prefer setting up the global state. It's more efficient\n(which does add up when running the whole suite), and I find it easier\nto debug failing tests (it's just one less thing the failing block is\ndoing that you have to look at; and you can generally \"cd\" into the\nleftover trash directory to investigate the global state).\n\n-Peff\n"},{"id":"278134","messageId":"alpine.DEB.2.20.1602150844110.2964@virtualbox","threadId":"41407","inReplyTo":"20160214161816.GC5875@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-15T07:53:17Z","receivedAt":"2016-02-15T07:53:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\ntl;dr let's keep an eye on adding only test cases that do not depend on\nearlier test cases' output ('setup' excluded, of course).\n\nOn Sun, 14 Feb 2016, Jeff King wrote:\n\n> In general, my opinion is that skipping arbitrary leading tests is a\n> losing strategy. It's just too easy to introduce hidden dependencies,\n> and not worth the programmer time to make sure each test runs in\n> isolation. But others on the list may disagree.\n\nYes, I disagree. And you would, too, if you had to run as many tests as I\nhad to do by way of js/mingw-tests. I did not keep precise track of time,\nbut I am certain that I had to run one of those bloody tests (forgot which\none) around 100 times, each taking roughly 3 minutes to complete, and of\ncourse, it was the *last* test case failing, and *of course* it depended\non earlier tests to run.\n\nIt gets even worse when you think about those test cases that depend on\nsome prereq such as SYMLINKS or POSIXPERM. In *most* cases does the\ndeveloper who adds them not even notice that these prerequisites are\nrequired. And subsequent test cases that do *not* share those\nprerequisites depend (of course!) on the previous test cases' output.\n\nDon't get me wrong, I think it would be too much pain for little gain to\nclean up our act now. But I think that we have ample evidence that it\nwould be a plenty good idea to try pretty hard to avoid adding to the\npile.\n\nCiao,\nDscho\n"},{"id":"278145","messageId":"5DE14BA4-6F90-4337-A5F6-345775E9734A@gmail.com","threadId":"41407","inReplyTo":"20160213181529.GB9516@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-15T09:41:11Z","receivedAt":"2016-02-15T09:41:11Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 13 Feb 2016, at 19:15, Jeff King wrote:\n\n> On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:\n> \n>> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:\n>> \n>>>> +test_expect_success '--show-origin' '\n>>> [...]\n>>> I see you split this up more, but there's still quite a bit going on in\n>>> this one block. IMHO, it would be more customary in our tests to put the\n>>> setup into one test_expect_success block, then each of these\n>>> expect-run-cmp blocks into their own test_expect_success.\n>> \n>> Here's a squashable patch that shows what I mean.\n> \n> And here are a few comments on the changes...\n> \n>> -test_expect_success '--show-origin' '\n>> -\t>.git/config &&\n>> -\t>\"$HOME\"/.gitconfig &&\n>> +test_expect_success 'set up --show-origin tests' '\n>> \tINCLUDE_DIR=\"$HOME/include\" &&\n>> \tmkdir -p \"$INCLUDE_DIR\" &&\n>> -\tcat >\"$INCLUDE_DIR\"/absolute.include <<-EOF &&\n>> +\tcat >\"$INCLUDE_DIR\"/absolute.include <<-\\EOF &&\n>> \t\t[user]\n>> \t\t\tabsolute = include\n>> \tEOF\n>> -\tcat >\"$INCLUDE_DIR\"/relative.include <<-EOF &&\n>> +\tcat >\"$INCLUDE_DIR\"/relative.include <<-\\EOF &&\n>> \t\t[user]\n>> \t\t\trelative = include\n>> \tEOF\n>> -\ttest_config_global user.global \"true\" &&\n>> -\ttest_config_global user.override \"global\" &&\n>> -\ttest_config_global include.path \"$INCLUDE_DIR\"/absolute.include &&\n>> -\ttest_config include.path ../include/relative.include &&\n>> -\ttest_config user.local \"true\" &&\n>> -\ttest_config user.override \"local\" &&\n>> +\tcat >\"$HOME\"/.gitconfig <<-EOF &&\n>> +\t\t[user]\n>> +\t\t\tglobal = true\n>> +\t\t\toverride = global\n>> +\t\t[include]\n>> +\t\t\tpath = \"$INCLUDE_DIR/absolute.include\"\n>> +\tEOF\n>> +\tcat >.git/config <<-\\EOF\n>> +\t\t[include]\n>> +\t\t\tpath = ../include/relative.include\n>> +\t\t[user]\n>> +\t\t\tlocal = true\n>> +\t\t\toverride = local\n>> +\tEOF\n> \n> I preserved your ordering here (as the later \"--list\" tests care). But\n> it might be worth ordering both files the same way, so that a reader\n> does not wonder if it is significant (and just update the --list\n> output expectation later).\nOK, fixed!\n\n\n> \n>> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '\n>> \t\tlocalQcmdline:Quser.cmdline\n>> \t\ttrueQ\n>> \tEOF\n>> -\tgit -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&\n>> +\tgit -c user.cmdline=true config --null --list --show-origin >output.raw &&\n>> +\tnul_to_q output &&\n> \n> We usually try to avoid putting git on the left-hand side of a pipe,\n> because it hides the exit code, and we want to make sure git does not\n> fail. I won't be surprised if you copied the style from elsewhere in the\n> script, though, as this is an old one and we were not always consistent.\nMake sense, fixed!\n\n\n> \n>> \techo >>output &&\n>> -\ttest_cmp expect output &&\n>> +\ttest_cmp expect output\n> \n> This \"echo\" might be worth a comment. I think we are just adding the\n> extra newline that the here-doc insists on, but that --null output would\n> not include.\nDone.\n\n> \n> Overall, I find the \"--show-origin --null\" output pretty weird to read.\n> We use a newline to split the config key/value, a NUL between config\n> entries, but now also a NUL between the filename and the rest of the\n> config entry.\n> \n> That makes parsing pretty weird, as you cannot just use something like\n> \n> git config --show-origin --list --null | perl -0ne ...\n> \n> to process entries; every other entry you get will be a filename. But at\n> the same time, we do not go all out and say \"there is a NUL between each\n> field\", because the key/value separator is a newline in this case, and\n> the reader has to parse that separately after splitting on NULs.\n> \n> But I think it's too late to do anything about it now. The weirdness is\n> really the mixed NUL/newline thing, and you are not introducing that.\n> \n>> -CUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n>> -cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF &&\n>> -\t[user]\n>> -\t\tcustom = true\n>> -EOF\n>> +test_expect_success 'set up custom config file' '\n>> +\tCUSTOM_CONFIG_FILE=\"file\\twith\\ttabs.conf\" &&\n>> +\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n>> +\t\t[user]\n>> +\t\t\tcustom = true\n>> +\tEOF\n>> +'\n> \n> Everything, even mundane setup, should generally go in a test_expect\n> block. That means we'll notice unexpected failures, and any output will\n> follow the usual \"--verbose\" rules.\n> \n> Arguably this setup could just go into the initial setup block.\n> \n> Also, you may not that the filename does _not_ actually have tabs in it,\n> because the shell does not expand \"\\t\". It does have backslashes in it,\n> though, which is enough to trigger our C-style quoting.\nOh, thanks for the explanation. I was already wondering about the double\nbackslash earlier...\n\n> \n> So the test isn't wrong, but the filename is misleading. If you really\n> want tabs, you'd have to do:\n> \n> CUSTOM_CONFIG_FILE=\"$(printf \"file\\twith\\ttabs.conf\")\n> \n> or similar.\n> \n>> test_expect_success '--show-origin escape special file name characters' '\n>> \tcat >expect <<-\\EOF &&\n>> @@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '\n>> '\n>> \n>> test_expect_success '--show-origin stdin with file include' '\n>> -\tINCLUDE_DIR=\"$HOME/include\" &&\n>> -\tmkdir -p \"$INCLUDE_DIR\" &&\n>> \tcat >\"$INCLUDE_DIR\"/stdin.include <<-EOF &&\n>> \t\t[user]\n>> \t\t\tstdin = include\n> \n> Here we can assume that the setup block succeeded (if it didn't, all of\n> the tests are screwed anyway, so you'd want to fix that first).\n\nThanks,\nLars"}]}