{"thread":{"id":"41381","subject":"[PATCH v2] config: add '--sources' option to print the source of a config value","startedAt":"2016-02-10T10:13:18Z","lastAt":"2016-02-10T19:03:28Z","messageCount":10,"participants":["larsxschneider@gmail.com","Ramsay Jones","Jeff King","Sebastian Schuberth","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"277856","messageId":"1455099198-11515-1-git-send-email-larsxschneider@gmail.com","threadId":"41381","inReplyTo":null,"subject":"[PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-10T10:13:18Z","receivedAt":"2016-02-10T10:13:18Z","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 '--list' flag\nor the '--get*' flags) then it is sometimes hard to find the\nconfiguration file where the values were defined.\n\nTeach 'git config' the '--sources' 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\ndiff to v1:\n\n* add documention\n* produce a consistent (tab-delimited) format that can be parsed\n* adhere declaration-after-statement style\n* prefix every source line with the source type (file, stdin, blob, cmd)\n* add relative path test case\n* add blob ref test case\n* add \"git config --local\" test case (Note: I think I found a bug there that I\n plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)\n* add a test case to check funny character escapes (file name with tabs)\n\nSebastian suggested \"--show-origin\" as a better option name over \"--sources\".\nI still believe \"--sources\" might be slightly better as I fear that users could\nsomehow related \"origin\" to \"remote\" kind of configs. However, I am happy to\nchange that if a majority prefers \"--show-origin\".\n\nThanks to Peff and Sebastian for the review,\nLars\n\n Documentation/git-config.txt | 15 ++++--\n builtin/config.c | 66 ++++++++++++++++++++++++++\n cache.h | 1 +\n config.c | 9 +++-\n t/t1300-repo-config.sh | 109 +++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 194 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 2608ca7..8c4969f 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] [--sources] [-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] [--sources] [-z|--null] --get name [value_regex]\n+'git config' [] [type] [--sources] [-z|--null] --get-all name [value_regex]\n+'git config' [] [type] [--sources] [-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' [] [--sources] [-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+--sources::\n+\tAugment the output of all queried config options with the\n+\tsource type (file, stdin, blob, cmd) and the actual source\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..78f3344 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_sources;\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, \"sources\", &show_sources, N_(\"show source filenames of config\")),\n \tOPT_END(),\n };\n\n@@ -91,8 +94,58 @@ static void check_argc(int argc, int min, int max) {\n \tusage_with_options(builtin_config_usage, builtin_config_options);\n }\n\n+/* output to either fp or buf; only one should be non-NULL */\n+static void show_config_source(struct strbuf *buf, FILE *fp)\n+{\n+\tchar term = '\\t';\n+\tchar *prefix;\n+\tconst char *fn = current_config_filename();\n+\n+\tif (end_null)\n+\t\tterm = '\\0';\n+\n+\tif (fn) {\n+\t\tif (given_config_source.use_stdin)\n+\t\t\tprefix = \"stdin\";\n+\t\telse if (given_config_source.blob)\n+\t\t\tprefix = \"blob\";\n+\t\telse\n+\t\t\tprefix = \"file\";\n+\t} else {\n+\t\tfn = \"\";\n+\t\tprefix = \"cmd\";\n+\t}\n+\n+\tif (fp)\n+\t\tfprintf(fp, \"%s\", prefix);\n+\telse {\n+\t\tstrbuf_addstr(buf, prefix);\n+\t}\n+\n+\tif (fp)\n+\t\tfputc(term, fp);\n+\telse\n+\t\tstrbuf_addch(buf, term);\n+\n+\tif (!end_null)\n+\t\tquote_c_style(fn, buf, fp, 0);\n+\telse {\n+\t\tif (fp)\n+\t\t\tfprintf(fp, \"%s\", fn);\n+\t\telse\n+\t\t\tstrbuf_addstr(buf, fn);\n+\t}\n+\n+\tif (fp)\n+\t\tfputc(term, fp);\n+\telse\n+\t\tstrbuf_addch(buf, term);\n+}\n+\n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n+\tif (show_sources)\n+\t\tshow_config_source(NULL, stdout);\n \tif (!omit_values && value_)\n \t\tprintf(\"%s%c%s%c\", key_, delim, value_, term);\n \telse\n@@ -108,6 +161,8 @@ struct strbuf_list {\n\n static int format_config(struct strbuf *buf, const char *key_, const char *value_)\n {\n+\tif (show_sources)\n+\t\tshow_config_source(buf, NULL);\n \tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n \tif (!omit_values) {\n@@ -451,6 +506,7 @@ static char *default_user_config(void)\n int cmd_config(int argc, const char **argv, const char *prefix)\n {\n \tint nongit = !startup_info->have_repository;\n+\tint is_query_action = 0;\n \tchar *value;\n\n \tgiven_config_source.file = getenv(CONFIG_ENVIRONMENT);\n@@ -538,6 +594,16 @@ 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+\tis_query_action = actions & (\n+\t\tACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST\n+\t);\n+\n+\tif (show_sources && !is_query_action) {\n+\t\terror(\"--sources is only applicable to --list or --get-* actions\");\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..c5111ea 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 const char *current_config_filename(void);\n\n struct config_include_data {\n \tint depth;\ndiff --git a/config.c b/config.c\nindex 86a5eb2..204e6b7 100644\n--- a/config.c\n+++ b/config.c\n@@ -1078,7 +1078,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, stdin, data);\n }\n\n int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n@@ -2385,3 +2385,10 @@ int parse_config_key(const char *var,\n\n \treturn 0;\n }\n+\n+const char *current_config_filename(void)\n+{\n+\tif (cf && cf->name)\n+\t\treturn cf->name;\n+\treturn NULL;\n+}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 52678e7..d69974e 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1201,4 +1201,113 @@ 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 '--sources' '\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+\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+\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\t$HOME/.gitconfig\tuser.global=true\n+\t\tfile\t$HOME/.gitconfig\tuser.override=global\n+\t\tfile\t$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/absolute.include\n+\t\tfile\t$INCLUDE_DIR/absolute.include\tuser.absolute=include\n+\t\tfile\t.git/config\tinclude.path=../include/relative.include\n+\t\tfile\t.git/../include/relative.include\tuser.relative=include\n+\t\tfile\t.git/config\tuser.local=true\n+\t\tfile\t.git/config\tuser.override=local\n+\t\tcmd\t\tuser.cmdline=true\n+\tEOF\n+\tgit -c user.cmdline=true config --list --sources >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfileQ$HOME/.gitconfigQuser.global\n+\t\ttrueQfileQ$HOME/.gitconfigQuser.override\n+\t\tglobalQfileQ$HOME/.gitconfigQinclude.path\n+\t\t$INCLUDE_DIR/absolute.includeQfileQ$INCLUDE_DIR/absolute.includeQuser.absolute\n+\t\tincludeQfileQ.git/configQinclude.path\n+\t\t../include/relative.includeQfileQ.git/../include/relative.includeQuser.relative\n+\t\tincludeQfileQ.git/configQuser.local\n+\t\ttrueQfileQ.git/configQuser.override\n+\t\tlocalQcmdQQuser.cmdline\n+\t\ttrueQ\n+\tEOF\n+\tgit -c user.cmdline=true config --null --list --sources | nul_to_q >output &&\n+\techo >>output &&\n+\ttest_cmp expect output &&\n+\n+\t# TODO:\n+\t# The locally included config value is not printed below. I think\n+\t# this is a bug.\n+\t# \"file\t.git/../include/relative.include\tuser.relative=include\"\n+\t#\n+\tcat >expect <<-EOF &&\n+\t\tfile\t.git/config\tinclude.path=../include/relative.include\n+\t\tfile\t.git/config\tuser.local=true\n+\t\tfile\t.git/config\tuser.override=local\n+\t\tEOF\n+\tgit config --local --list --sources >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile\t$HOME/.gitconfig\tuser.global true\n+\t\tfile\t.git/config\tuser.local true\n+\tEOF\n+\tgit config --sources --get-regexp \"user\\.[g|l].*\" >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile\t.git/config\tlocal\n+\tEOF\n+\tgit config --sources user.override >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tfile\t\"file\\\\\\twith\\\\\\ttabs.conf\"\tuser.custom=true\n+\tEOF\n+\tgit config --file \"$CUSTOM_CONFIG_FILE\" --sources --list >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tstdin\t\tuser.custom=true\n+\tEOF\n+\tgit config --file - --sources --list <\"$CUSTOM_CONFIG_FILE\" >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tblob\ta9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08\tuser.custom=true\n+\tEOF\n+\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n+\tgit config --blob=$blob --sources --list >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\tblob\t\"master:file\\\\\\twith\\\\\\ttabs.conf\"\tuser.custom=true\n+\tEOF\n+\tgit add \"$CUSTOM_CONFIG_FILE\" &&\n+\tgit commit -m \"blubb\" &&\n+\tgit config --blob=master:\"$CUSTOM_CONFIG_FILE\" --sources --list >output &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n--\n2.5.1\n"},{"id":"277858","messageId":"56BB3157.7080307@ramsayjones.plus.com","threadId":"41381","inReplyTo":"1455099198-11515-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-02-10T12:47:19Z","receivedAt":"2016-02-10T12:47:19Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 10/02/16 10:13, larsxschneider@gmail.com wrote:\n> From: Lars Schneider \n> \n> If config values are queried using 'git config' (e.g. via '--list' flag\n> or the '--get*' flags) then it is sometimes hard to find the\n> configuration file where the values were defined.\n> \n> Teach 'git config' the '--sources' option to print the source\n> configuration file for every printed value.\n> \n> Based-on-patch-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> \n> diff to v1:\n> \n> * add documention\n> * produce a consistent (tab-delimited) format that can be parsed\n> * adhere declaration-after-statement style\n> * prefix every source line with the source type (file, stdin, blob, cmd)\n> * add relative path test case\n> * add blob ref test case\n> * add \"git config --local\" test case (Note: I think I found a bug there that I\n> plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)\n> * add a test case to check funny character escapes (file name with tabs)\n> \n> Sebastian suggested \"--show-origin\" as a better option name over \"--sources\".\n> I still believe \"--sources\" might be slightly better as I fear that users could\n> somehow related \"origin\" to \"remote\" kind of configs. However, I am happy to\n> change that if a majority prefers \"--show-origin\".\n\n\nAs I have said before, I'm not very good at naming things, but ...\n\nOf the two, I *slightly* prefer --show-origin, since I don't think\nthere will be any confusion. However, I think --source may be OK too\n(for some reason it sounds better than the plural). Another idea\nmay be --show-source. ;-)\n\n\n\nATB,\nRamsay Jones\n"},{"id":"277860","messageId":"20160210125429.GA8078@sigill.intra.peff.net","threadId":"41381","inReplyTo":"1455099198-11515-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T12:54:30Z","receivedAt":"2016-02-10T12:54:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2016 at 11:13:18AM +0100, larsxschneider@gmail.com wrote:\n\n> diff to v1:\n> \n> * add documention\n> * produce a consistent (tab-delimited) format that can be parsed\n> * adhere declaration-after-statement style\n> * prefix every source line with the source type (file, stdin, blob, cmd)\n> * add relative path test case\n> * add blob ref test case\n> * add \"git config --local\" test case (Note: I think I found a bug there that I\n> plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)\n> * add a test case to check funny character escapes (file name with tabs)\n> \n> Sebastian suggested \"--show-origin\" as a better option name over \"--sources\".\n> I still believe \"--sources\" might be slightly better as I fear that users could\n> somehow related \"origin\" to \"remote\" kind of configs. However, I am happy to\n> change that if a majority prefers \"--show-origin\".\n\nThanks, this is getting closer, but I still have a few comments.\n\n> @@ -194,6 +194,11 @@ See also <>.\n> \tOutput only the names of config variables for `--list` or\n> \t`--get-regexp`.\n> \n> +--sources::\n> +\tAugment the output of all queried config options with the\n> +\tsource type (file, stdin, blob, cmd) and the actual source\n> +\t(config file path, ref, or blob id if applicable).\n\nI think something like \"cmdline\" might be more descriptive than \"cmd\".\nTechnically such options could also come from the environment (as \"-c\"\nis really just a shorthand for modifying the environment), but I don't\nthink we actually advertise that.\n\n> +/* output to either fp or buf; only one should be non-NULL */\n> +static void show_config_source(struct strbuf *buf, FILE *fp)\n> +{\n> +\tchar term = '\\t';\n> +\tchar *prefix;\n> +\tconst char *fn = current_config_filename();\n\nA minor nit, but I think our discussion has shown that this function\ndoes not strictly return filenames. We might just want s/filename/name/.\n\nBut moreover...\n\n> +\tif (end_null)\n> +\t\tterm = '\\0';\n> +\n> +\tif (fn) {\n> +\t\tif (given_config_source.use_stdin)\n> +\t\t\tprefix = \"stdin\";\n> +\t\telse if (given_config_source.blob)\n> +\t\t\tprefix = \"blob\";\n> +\t\telse\n> +\t\t\tprefix = \"file\";\n> +\t} else {\n> +\t\tfn = \"\";\n> +\t\tprefix = \"cmd\";\n> +\t}\n\nI don't think this is quite right. \"fn\" represents the current file we\nhappen to be parsing, but given_config_source is where we _started_. So\nhere's a fairly pathological example that shows the distinction:\n\n echo \"[include]path=/home/peff/.gitconfig\" |\n git config --sources --includes --file - user.name\n\nwhich produces:\n\n stdin /home/peff/.gitconfig Jeff King\n\nSo I think we really need to record this source information in the\nconfig_source of config.c and feed it back via current_config_filename(),\nwhose name grows even more inaccurate. :)\n\n> +\tif (fp)\n> +\t\tfprintf(fp, \"%s\", prefix);\n> +\telse {\n> +\t\tstrbuf_addstr(buf, prefix);\n> +\t}\n> +\n> +\tif (fp)\n> +\t\tfputc(term, fp);\n> +\telse\n> +\t\tstrbuf_addch(buf, term);\n\nSo the format here is like:\n\n file\\t\\t\n blob\\t\\t\n stdin\\t\\t\n cmd\\t\\t\n\nwhere two of the prefixes have nothing in the second slot. I expected\nsomething more like:\n\n file:\\t\n blob:\\t\n stdin\\t\n cmd\\t\n\nwith a single delimited slot for the source, which can then be broken\ndown further if desired. I can't think of any reason to prefer one over\nthe other rather than personal preference, though. They can both be\nparsed unambiguously.\n\n> +\n> +\tis_query_action = actions & (\n> +\t\tACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST\n> +\t);\n> +\n> +\tif (show_sources && !is_query_action) {\n> +\t\terror(\"--sources is only applicable to --list or --get-* actions\");\n> +\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n> +\t}\n\nThis means that \"git config --sources --get-colorbool\" will tell the\nuser that --sources is only applicable to --get-* actions, which is what\nthey said. I wonder if we should simply enumerate the cases where it\nworks.\n\nI also wonder if \"is_query_action\" is worth the separate variable, since\nit is no longer quite accurate (it is really \"query actions that support\n--sources\", and I would not want somebody coming along later to treat it\nas \"any query action\").\n\n> @@ -1078,7 +1078,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, stdin, data);\n> }\n\nI think this is a regression for the other uses of the name \"name\"\nfield. Before your patch:\n\n $ echo '[broken' | git config --list --file -\n fatal: bad config file line 1 in \n\nAnd after it just says:\n\n fatal: bad config file line 1 in \n\nThis should be fixed for free along with the source stuff I mentioned\nabove, though.\n\n> +\t# TODO:\n> +\t# The locally included config value is not printed below. I think\n> +\t# this is a bug.\n> +\t# \"file\t.git/../include/relative.include\tuser.relative=include\"\n> +\t#\n> +\tcat >expect <<-EOF &&\n> +\t\tfile\t.git/config\tinclude.path=../include/relative.include\n> +\t\tfile\t.git/config\tuser.local=true\n> +\t\tfile\t.git/config\tuser.override=local\n> +\t\tEOF\n> +\tgit config --local --list --sources >output &&\n> +\ttest_cmp expect output &&\n\nThis is behaving as expected. By default \"--include\" is off when reading\na specific config file (triggered by your \"--local\"), and on when\ngenerically reading all config.\n\n> +\tcat >expect <<-EOF &&\n> +\t\tfile\t$HOME/.gitconfig\tuser.global true\n> +\t\tfile\t.git/config\tuser.local true\n> +\tEOF\n> +\tgit config --sources --get-regexp \"user\\.[g|l].*\" >output &&\n> +\ttest_cmp expect output &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\t\tfile\t.git/config\tlocal\n> +\tEOF\n> +\tgit config --sources user.override >output &&\n> +\ttest_cmp expect output &&\n\nThere are quite a lot of separate things tested here all in a single\ntest_expect_success. If you break each of these out into their own\ntest_expect_success blocks, it is much easier for somebody to diagnose a\nfailing test. They immediately know _which_ block failed, and if you\nhave written a description, then they know what the test was trying to\ndo.\n\n> +\tcat >expect <<-EOF &&\n> +\t\tfile\t\"file\\\\\\twith\\\\\\ttabs.conf\"\tuser.custom=true\n> +\tEOF\n\nIf you escape the here-doc marker, it turns off interpolation, and you\ncan avoid the extra layer of backslash quoting. Like:\n\n cat >expect <<-\\EOF\n\t\tfile\t\"file\\twith\\ttabs.conf\"\tuser.custom = true\n\tEOF\n\nIn fact, we generally prefer to use non-interpolated here-docs unless\nyou actually intend to expand anything. It's one less thing for a reader\nof the code to have to worry about.\n\n-Peff\n"},{"id":"277864","messageId":"CAHGBnuNGTQvBStqK6-OERye4y2+yvRkK3g+rdSNPepSqy4kyRg@mail.gmail.com","threadId":"41381","inReplyTo":"56BB3157.7080307@ramsayjones.plus.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-10T15:28:47Z","receivedAt":"2016-02-10T15:28:47Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Feb 10, 2016 at 1:47 PM, Ramsay Jones\n wrote:\n\n>> Sebastian suggested \"--show-origin\" as a better option name over \"--sources\".\n>> I still believe \"--sources\" might be slightly better as I fear that users could\n>> somehow related \"origin\" to \"remote\" kind of configs. However, I am happy to\n>> change that if a majority prefers \"--show-origin\".\n>\n> \n> As I have said before, I'm not very good at naming things, but ...\n>\n> Of the two, I *slightly* prefer --show-origin, since I don't think\n> there will be any confusion. However, I think --source may be OK too\n> (for some reason it sounds better than the plural). Another idea\n> may be --show-source. ;-)\n>\n> \n\nI agree that using --source sounds better than --sources, as the\nlatter sounds even more like \"source code\".\n\nHere's another idea: How about --declaration or --show-declaration?\n\n-- \nSebastian Schuberth\n"},{"id":"277865","messageId":"CAHGBnuMMN_xEVDJ_dqWgL6QCGK08hy=ggg=6PzkSCinHr=QeFg@mail.gmail.com","threadId":"41381","inReplyTo":"20160210125429.GA8078@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-10T15:33:11Z","receivedAt":"2016-02-10T15:33:11Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Feb 10, 2016 at 1:54 PM, Jeff King wrote:\n\n>> +--sources::\n>> + Augment the output of all queried config options with the\n>> + source type (file, stdin, blob, cmd) and the actual source\n>> + (config file path, ref, or blob id if applicable).\n>\n> I think something like \"cmdline\" might be more descriptive than \"cmd\".\n\nI agree \"cmdline\" is better than \"cmd\".\n\n> So the format here is like:\n>\n> file\\t\\t\n> blob\\t\\t\n> stdin\\t\\t\n> cmd\\t\\t\n>\n> where two of the prefixes have nothing in the second slot. I expected\n> something more like:\n>\n> file:\\t\n> blob:\\t\n> stdin\\t\n> cmd\\t\n>\n> with a single delimited slot for the source, which can then be broken\n> down further if desired. I can't think of any reason to prefer one over\n> the other rather than personal preference, though. They can both be\n> parsed unambiguously.\n\nI also would have expected sopme like the latter, except that I'd also\nexpect a colon after \"stdin\" and \"cmd\" (or \"cmdline\", as said above).\nI.e. the colon should be part of the prefix to mark it as such.\n\n-- \nSebastian Schuberth\n"},{"id":"277866","messageId":"20160210154006.GA19867@sigill.intra.peff.net","threadId":"41381","inReplyTo":"CAHGBnuMMN_xEVDJ_dqWgL6QCGK08hy=ggg=6PzkSCinHr=QeFg@mail.gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T15:40:06Z","receivedAt":"2016-02-10T15:40:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2016 at 04:33:11PM +0100, Sebastian Schuberth wrote:\n\n> > where two of the prefixes have nothing in the second slot. I expected\n> > something more like:\n> >\n> > file:\\t\n> > blob:\\t\n> > stdin\\t\n> > cmd\\t\n> >\n> > with a single delimited slot for the source, which can then be broken\n> > down further if desired. I can't think of any reason to prefer one over\n> > the other rather than personal preference, though. They can both be\n> > parsed unambiguously.\n> \n> I also would have expected sopme like the latter, except that I'd also\n> expect a colon after \"stdin\" and \"cmd\" (or \"cmdline\", as said above).\n> I.e. the colon should be part of the prefix to mark it as such.\n\nYeah, I waffled on that. Having a colon means you can definitely parse\nto the first \":\" without looking at what the prefix is. But if you don't\nknow what the prefix is, I don't know what good that does you. IOW, I'd\nexpect it to be parsed like:\n\n if (/^file:(.*)/) {\n # source is file $1\n } elsif (/^blob:(.*)/) {\n # source is blob $1\n } elsif (/^stdin/) {\n # source is stdin\n } elsif (/^cmdline/) {\n # source is cmdline\n } else {\n die \"eh? I don't know about $_ at all!\"\n }\n\nThat's perl, but I think most languages make prefix-parsing like that\neasy. I dunno. I doubt it matters all that much, and we are deep into\npersonal preference. There's already plenty to bikeshed on the option\nname :)\n\n-Peff\n"},{"id":"277868","messageId":"CAHGBnuMDCYePJyEU4yHjGjEzyZn8mRBE73WC3qsRqbb6Amhyog@mail.gmail.com","threadId":"41381","inReplyTo":"20160210154006.GA19867@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-10T15:57:00Z","receivedAt":"2016-02-10T15:57:00Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Feb 10, 2016 at 4:40 PM, Jeff King wrote:\n\n>> > file:\\t\n>> > blob:\\t\n>> > stdin\\t\n>> > cmd\\t\n>> >\n>> > with a single delimited slot for the source, which can then be broken\n>> > down further if desired. I can't think of any reason to prefer one over\n>> > the other rather than personal preference, though. They can both be\n>> > parsed unambiguously.\n>>\n>> I also would have expected sopme like the latter, except that I'd also\n>> expect a colon after \"stdin\" and \"cmd\" (or \"cmdline\", as said above).\n>> I.e. the colon should be part of the prefix to mark it as such.\n>\n> Yeah, I waffled on that. Having a colon means you can definitely parse\n> to the first \":\" without looking at what the prefix is. But if you don't\n> know what the prefix is, I don't know what good that does you. IOW, I'd\n\nIMO that's asking the wrong question. The question should not be \"what\ngood does it do if we add the colons also there\", but \"what\njustification do we have to introduce an inconsistency by not adding\nthem\".\n\n> That's perl, but I think most languages make prefix-parsing like that\n> easy. I dunno. I doubt it matters all that much, and we are deep into\n> personal preference. There's already plenty to bikeshed on the option\n> name :)\n\nI agree the option wording mostly is personal preference. On the other\nhand, I find discussions like these often prematurely waved aside as\nbikeshedding, just because e.g. Perl can parse the one as good as the\nother. But it's not about Perl, it's about humans. IMO Git has\nhistorically made many mistakes by not caring enough about consistency\nin docs, command and command line option naming, so I don't see it as\nwasted time to discuss about things like this.\n\n-- \nSebastian Schuberth\n"},{"id":"277869","messageId":"56BB5F55.9040404@ramsayjones.plus.com","threadId":"41381","inReplyTo":"CAHGBnuNGTQvBStqK6-OERye4y2+yvRkK3g+rdSNPepSqy4kyRg@mail.gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-02-10T16:03:33Z","receivedAt":"2016-02-10T16:03:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 10/02/16 15:28, Sebastian Schuberth wrote:\n> On Wed, Feb 10, 2016 at 1:47 PM, Ramsay Jones\n> wrote:\n> \n>>> Sebastian suggested \"--show-origin\" as a better option name over \"--sources\".\n>>> I still believe \"--sources\" might be slightly better as I fear that users could\n>>> somehow related \"origin\" to \"remote\" kind of configs. However, I am happy to\n>>> change that if a majority prefers \"--show-origin\".\n>>\n>> \n>> As I have said before, I'm not very good at naming things, but ...\n>>\n>> Of the two, I *slightly* prefer --show-origin, since I don't think\n>> there will be any confusion. However, I think --source may be OK too\n>> (for some reason it sounds better than the plural). Another idea\n>> may be --show-source. ;-)\n>>\n>> \n> \n> I agree that using --source sounds better than --sources, as the\n> latter sounds even more like \"source code\".\n> \n> Here's another idea: How about --declaration or --show-declaration?\n> \n\nHmm, I think its more like a definition! :-D\n\n[Sorry, I just couldn't resist. I promise not\nto say any more on this. ;-) ]\n\nATB,\nRamsay Jones\n"},{"id":"277877","messageId":"20160210162408.GD19867@sigill.intra.peff.net","threadId":"41381","inReplyTo":"CAHGBnuMDCYePJyEU4yHjGjEzyZn8mRBE73WC3qsRqbb6Amhyog@mail.gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T16:24:08Z","receivedAt":"2016-02-10T16:24:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2016 at 04:57:00PM +0100, Sebastian Schuberth wrote:\n\n> >> I also would have expected sopme like the latter, except that I'd also\n> >> expect a colon after \"stdin\" and \"cmd\" (or \"cmdline\", as said above).\n> >> I.e. the colon should be part of the prefix to mark it as such.\n> >\n> > Yeah, I waffled on that. Having a colon means you can definitely parse\n> > to the first \":\" without looking at what the prefix is. But if you don't\n> > know what the prefix is, I don't know what good that does you. IOW, I'd\n> \n> IMO that's asking the wrong question. The question should not be \"what\n> good does it do if we add the colons also there\", but \"what\n> justification do we have to introduce an inconsistency by not adding\n> them\".\n\nBut I think it is only an inconsistency if your mental model of the\nformat is \"the source field is a prefix, a colon, and then an argument;\nsometimes we omit the colon if there is no argument\". If your mental\nmodel is \"the source field contains a type, followed by type-specific\ndata\", then it is not an inconsistency. It is simply context-sensitive.\n\nNow you can argue that context-sensitive things are bad, and I might\nagree with that. :)\n\n> I agree the option wording mostly is personal preference. On the other\n> hand, I find discussions like these often prematurely waved aside as\n> bikeshedding, just because e.g. Perl can parse the one as good as the\n> other. But it's not about Perl, it's about humans. IMO Git has\n> historically made many mistakes by not caring enough about consistency\n> in docs, command and command line option naming, so I don't see it as\n> wasted time to discuss about things like this.\n\nOh, I agree that formats are worth discussing. It's just that I do not\nthink either of the mental models above is better than the other.\n\nMy point in mentioning perl is not that perl can do it, but rather the\nopposite: that _any_ language can do it. And I would even go so far as\nto say that humans parse context-sensitive things better than machines,\nbecause we simultaneously apply the semantic and syntactic parsing (so\nyou see \"stdin\" and know because of its meaning that there is nothing\nleft to parse; you don't need a trailing colon to tell you that). Or\nperhaps you mean \"humans write the programs that machines use to parse,\nand we must make things regular and easy for them\". I can somewhat buy\nthat argument, though again, I think either is easy; it is mostly a\nmatter of documenting the format's mental model.\n\n-Peff\n"},{"id":"277884","messageId":"20160210190328.GA2376@flurp.local","threadId":"41381","inReplyTo":"1455099198-11515-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] config: add '--sources' option to print the source of a config value","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-10T19:03:28Z","receivedAt":"2016-02-10T19:03:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 10, 2016 at 11:13:18AM +0100, larsxschneider@gmail.com wrote:\n> If config values are queried using 'git config' (e.g. via '--list' flag\n> or the '--get*' flags) then it is sometimes hard to find the\n> configuration file where the values were defined.\n> \n> Teach 'git config' the '--sources' option to print the source\n> configuration file for every printed value.\n> \n> Based-on-patch-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/builtin/config.c b/builtin/config.c\n> @@ -91,8 +94,58 @@ static void check_argc(int argc, int min, int max) {\n> +/* output to either fp or buf; only one should be non-NULL */\n> +static void show_config_source(struct strbuf *buf, FILE *fp)\n> +{\n> +\tchar term = '\\t';\n> +\tchar *prefix;\n> +\tconst char *fn = current_config_filename();\n> +\n> +\tif (end_null)\n> +\t\tterm = '\\0';\n> +\n> +\tif (fn) {\n> +\t\tif (given_config_source.use_stdin)\n> +\t\t\tprefix = \"stdin\";\n> +\t\telse if (given_config_source.blob)\n> +\t\t\tprefix = \"blob\";\n> +\t\telse\n> +\t\t\tprefix = \"file\";\n> +\t} else {\n> +\t\tfn = \"\";\n> +\t\tprefix = \"cmd\";\n> +\t}\n> +\n> +\tif (fp)\n> +\t\tfprintf(fp, \"%s\", prefix);\n> +\telse {\n> +\t\tstrbuf_addstr(buf, prefix);\n> +\t}\n\nStyle: drop unnecessary braces\n\n> +\n> +\tif (fp)\n> +\t\tfputc(term, fp);\n> +\telse\n> +\t\tstrbuf_addch(buf, term);\n\nWhy not combine this 'if' with the preceding one?\n\n> +\n> +\tif (!end_null)\n> +\t\tquote_c_style(fn, buf, fp, 0);\n> +\telse {\n> +\t\tif (fp)\n> +\t\t\tfprintf(fp, \"%s\", fn);\n> +\t\telse\n> +\t\t\tstrbuf_addstr(buf, fn);\n> +\t}\n> +\n> +\tif (fp)\n> +\t\tfputc(term, fp);\n> +\telse\n> +\t\tstrbuf_addch(buf, term);\n> +}\n\nOverall, due to its duality (outputting to FILE* or strbuf), this\nfunction is more difficult to read than it probably ought to be. Have\nyou considered simplifying it by making it single-purpose. Something\nlike this, for instance:\n\n static void get_config_source(const char **name, const char **prefix)\n {\n *name = current_config_filename();\n if (!*name) {\n *name = \"\";\n *prefix = \"cmd\";\n } else if (given_config_source.use_stdin)\n *prefix = \"stdin\";\n else if (given_config_source.blob)\n *prefix = \"blob\";\n else\n *prefix = \"file\";\n }\n\n static void show_config_source(struct strbuf *buf)\n {\n char term = end_null ? '\\0' : '\\t';\n char *prefix;\n const char *name;\n\n get_config_source(&name, &prefix);\n strbuf_addstr(buf, prefix);\n strbuf_addch(buf, term);\n if (end_null)\n strbuf_addstr(buf, name);\n else\n quote_c_style(name, buf, NULL, 0);\n strbuf_addch(buf, term);\n }\n\n static int show_all_config(...)\n {\n if (show_sources) {\n struct strbuf buf = STRBUF_INIT;\n show_config_source(&buf);\n fputs(buf.buf, stdout);\n strbuf_release(&buf);\n }\n ...\n }\n\n static int format_config(...)\n {\n if (show_sources)\n show_config_source(buf);\n ...\n }\n"}]}