{"thread":{"id":"41337","subject":"[PATCH v1] config: add '--sources' option to print the source of a config value","startedAt":"2016-02-05T08:42:30Z","lastAt":"2016-02-08T12:12:49Z","messageCount":14,"participants":["larsxschneider@gmail.com","Sebastian Schuberth","Jeff King","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"277519","messageId":"1454661750-85703-1-git-send-email-larsxschneider@gmail.com","threadId":"41337","inReplyTo":null,"subject":"[PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-05T08:42:30Z","receivedAt":"2016-02-05T08:42:30Z","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 builtin/config.c | 42 ++++++++++++++++++++++++++++++++\n cache.h | 1 +\n config.c | 7 ++++++\n t/t1300-repo-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 115 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex adc7727..f5dc79c 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,34 @@ 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+\tconst char *fn = current_config_filename();\n+\tif (!fn)\n+\t\treturn;\n+\n+\tchar term = '\\t';\n+\tif (!end_null)\n+\t\tquote_c_style(fn, buf, fp, 0);\n+\telse {\n+\t\tterm = '\\0';\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 +137,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@@ -538,6 +569,17 @@ 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+\tconst int is_query_action = actions & (\n+\t\tACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|\n+\t\tACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH\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..b437002 100644\n--- a/config.c\n+++ b/config.c\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..2444d8a 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1201,4 +1201,69 @@ 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\"/include.conf <<-EOF &&\n+\t\t[user]\n+\t\t\tinclude = true\n+\tEOF\n+\tcat >\"$HOME\"/file.conf <<-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\"/include.conf &&\n+\ttest_config user.local \"true\" &&\n+\ttest_config user.override \"local\" &&\n+\n+\tcat >expect <<-EOF &&\n+\t\t$HOME/.gitconfig\tuser.global=true\n+\t\t$HOME/.gitconfig\tuser.override=global\n+\t\t$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/include.conf\n+\t\t$INCLUDE_DIR/include.conf\tuser.include=true\n+\t\t.git/config\tuser.local=true\n+\t\t.git/config\tuser.override=local\n+\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\t$HOME/.gitconfigQuser.global\n+\t\ttrueQ$HOME/.gitconfigQuser.override\n+\t\tglobalQ$HOME/.gitconfigQinclude.path\n+\t\t$INCLUDE_DIR/include.confQ$INCLUDE_DIR/include.confQuser.include\n+\t\ttrueQ.git/configQuser.local\n+\t\ttrueQ.git/configQuser.override\n+\t\tlocalQuser.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+\tcat >expect <<-EOF &&\n+\t\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\t$HOME/file.conf\tuser.custom=true\n+\tEOF\n+\tgit config --file \"$HOME\"/file.conf --sources --list >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect <<-EOF &&\n+\t\ta9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08\tuser.custom=true\n+\tEOF\n+\tblob=$(git hash-object -w \"$HOME\"/file.conf) &&\n+\tgit config --blob=$blob --sources --list >output &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n-- \n2.5.1\n"},{"id":"277532","messageId":"56B483C0.6070906@gmail.com","threadId":"41337","inReplyTo":"1454661750-85703-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-05T11:13:04Z","receivedAt":"2016-02-05T11:13:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2/5/2016 9:42, larsxschneider@gmail.com wrote:\n\n> Teach 'git config' the '--sources' option to print the source\n> configuration file for every printed value.\n\nYay, not being able to see where a config setting originates from has \nbothered me in the past, too. So thanks for working on this.\n\nHowever, the naming of the '--sources' option sounds a bit misleading to \nme. It has nothing to do with source code. So maybe better name it \n'--origin', or even more verbose '--show-origin' or '--show-filename'?\n\nRegards,\nSebastian\n"},{"id":"277533","messageId":"20160205112001.GA13397@sigill.intra.peff.net","threadId":"41337","inReplyTo":"1454661750-85703-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-05T11:20:01Z","receivedAt":"2016-02-05T11:20:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:\n\n> @@ -538,6 +569,17 @@ 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> +\tconst int is_query_action = actions & (\n> +\t\tACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|\n> +\t\tACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH\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\nHmm. I had originally envisioned this only being used with \"--list\", but\nI guess it makes sense to say \"--sources --get\" to show where the value\nfor a particular option is coming from.\n\nThe plural \"sources\" is a little funny there, though, as we list only\nthe \"last one wins\" final value, not all of them (for that, you can use\n\"--sources --get-all\", which seems handy). I think it would be\nsufficient for the documentation to make this clear (speaking of which,\nthis patch needs documentation...).\n\nAlso, I don't think the feature works with --get-color, --get-colorbool,\nor --get-urlmatch (which don't do their output in quite the same way).\nI think it would be fine to simply disallow --sources with those options\nrather than worry about making it work.\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> +\tconst char *fn = current_config_filename();\n> +\tif (!fn)\n> +\t\treturn;\n\nI'm not sure returning here is the best idea. We won't have a config\nfilename if we are reading from \"-c\", but if we return early from this\nfunction, it parses differently than every other line. E.g., with your\npatch, if I do:\n\n git config -c foo.bar=true config --sources --list\n\nI'll get:\n\n /home/peff/.gitconfig user.name=Jeff King\n /home/peff/.gitconfig user.email=peff@peff.net\n ...etc...\n foo.bar=true\n\nIf somebody is parsing this as a tab-delimited list, then instead of the\nsource field for that line being empty, it is missing (and it looks like\n\"foo.bar=true\" is the source file). I think it would be more friendly to\nconsumers of the output to have a blank (i.e., set \"fn\" to the empty\nstring and continue in the function).\n\n> +\n> +\tchar term = '\\t';\n\nThis declaration comes after the \"if\" above, but git style doesn't allow\ndeclaration-after-statement (to support ancient compilers).\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\"/include.conf <<-EOF &&\n> +\t\t[user]\n> +\t\t\tinclude = true\n> +\tEOF\n> +\tcat >\"$HOME\"/file.conf <<-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\"/include.conf &&\n\nHere you include the file by its absolute path. I wondered what would\nhappen if we used a relative path. E.g.:\n\n git config include.path=foo\n git config -f .git/foo included.config=true\n git config --sources --list\n\nwhich shows it as \".git/foo\", because we resolved it by manipulating the\nrelative path \".git/config\". Whereas including it from ~/.gitconfig will\nshow the absolute path, because we use the absolute path to get to\n~/.gitconfig in the first place.\n\nI think that's all sane. I don't know if it's worth noting it in the\ndocumentation or not.\n\n> +\tcat >expect <<-EOF &&\n> +\t\t$HOME/.gitconfig\tuser.global=true\n> +\t\t$HOME/.gitconfig\tuser.override=global\n> +\t\t$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/include.conf\n> +\t\t$INCLUDE_DIR/include.conf\tuser.include=true\n> +\t\t.git/config\tuser.local=true\n> +\t\t.git/config\tuser.override=local\n> +\t\tuser.cmdline=true\n> +\tEOF\n\nIf the filename has funny characters (e.g., a literal tab), it will be\nquoted here (but not in the --null output below). Worth including in the\ntest?\n\n> +\tcat >expect <<-EOF &&\n> +\t\t.git/config\tlocal\n> +\tEOF\n> +\tgit config --sources user.override >output &&\n> +\ttest_cmp expect output &&\n\nGood thoroughness in checking the override case.\n\n> +\tcat >expect <<-EOF &&\n> +\t\ta9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08\tuser.custom=true\n> +\tEOF\n> +\tblob=$(git hash-object -w \"$HOME\"/file.conf) &&\n> +\tgit config --blob=$blob --sources --list >output &&\n> +\ttest_cmp expect output\n\nThis one was unexpected to me, but I think it makes sense. The option is\n\"--sources\" and not \"--source-filenames\", after all. It's probably worth\nmentioning in the documentation.\n\nI think we also use the original name given, so if there was ref\nresolution, you would see the ref name. Might be worth testing that.\n\n-Peff\n"},{"id":"277534","messageId":"20160205112240.GA18581@sigill.intra.peff.net","threadId":"41337","inReplyTo":"56B483C0.6070906@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-05T11:22:40Z","receivedAt":"2016-02-05T11:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 05, 2016 at 12:13:04PM +0100, Sebastian Schuberth wrote:\n\n> On 2/5/2016 9:42, larsxschneider@gmail.com wrote:\n> \n> >Teach 'git config' the '--sources' option to print the source\n> >configuration file for every printed value.\n> \n> Yay, not being able to see where a config setting originates from has\n> bothered me in the past, too. So thanks for working on this.\n> \n> However, the naming of the '--sources' option sounds a bit misleading to me.\n> It has nothing to do with source code. So maybe better name it '--origin',\n> or even more verbose '--show-origin' or '--show-filename'?\n\nI think he inherited the \"--sources\" name from me. :) I agree it could\nbe better. I think \"--show-filename\" is not right as there are\nnon-filename cases. Just \"--origin\" sounds funny to me, perhaps because\nof git's normal use of the word \"origin\".\n\nI like \"--show-origin\" the best of the ones suggested.\n\n-Peff\n"},{"id":"277536","messageId":"56B48803.9080909@gmail.com","threadId":"41337","inReplyTo":"20160205112001.GA13397@sigill.intra.peff.net","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-05T11:31:15Z","receivedAt":"2016-02-05T11:31:15Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2/5/2016 12:20, Jeff King wrote:\n\n> Hmm. I had originally envisioned this only being used with \"--list\", but\n> I guess it makes sense to say \"--sources --get\" to show where the value\n> for a particular option is coming from.\n\nBeing able to use \"--sources --get\" is a feature that I'd definitely \nlike to see, too.\n\n> I'm not sure returning here is the best idea. We won't have a config\n> filename if we are reading from \"-c\", but if we return early from this\n> function, it parses differently than every other line. E.g., with your\n> patch, if I do:\n>\n> git config -c foo.bar=true config --sources --list\n>\n> I'll get:\n>\n> /home/peff/.gitconfig user.name=Jeff King\n> /home/peff/.gitconfig user.email=peff@peff.net\n> ...etc...\n> foo.bar=true\n>\n> If somebody is parsing this as a tab-delimited list, then instead of the\n> source field for that line being empty, it is missing (and it looks like\n> \"foo.bar=true\" is the source file). I think it would be more friendly to\n> consumers of the output to have a blank (i.e., set \"fn\" to the empty\n> string and continue in the function).\n\nOr to come up with a special string to denote config values specified on \nthe command line. Maybe somehting like\n\n foo.bar=true\n\nI acknowledge that \"\" would be a valid filename on some \nfilesystems, but I think the risk is rather low that someone would \nactually be using that name for a Git config file.\n\nRegards,\nSebastian\n"},{"id":"277543","messageId":"20160205135855.GA19154@sigill.intra.peff.net","threadId":"41337","inReplyTo":"56B48803.9080909@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-05T13:58:55Z","receivedAt":"2016-02-05T13:58:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:\n\n> >I'm not sure returning here is the best idea. We won't have a config\n> >filename if we are reading from \"-c\", but if we return early from this\n> >function, it parses differently than every other line. E.g., with your\n> >patch, if I do:\n> >\n> > git config -c foo.bar=true config --sources --list\n> >\n> >I'll get:\n> >\n> > /home/peff/.gitconfig user.name=Jeff King\n> > /home/peff/.gitconfig user.email=peff@peff.net\n> > ...etc...\n> > foo.bar=true\n> >\n> >If somebody is parsing this as a tab-delimited list, then instead of the\n> >source field for that line being empty, it is missing (and it looks like\n> >\"foo.bar=true\" is the source file). I think it would be more friendly to\n> >consumers of the output to have a blank (i.e., set \"fn\" to the empty\n> >string and continue in the function).\n> \n> Or to come up with a special string to denote config values specified on the\n> command line. Maybe somehting like\n> \n> foo.bar=true\n> \n> I acknowledge that \"\" would be a valid filename on some\n> filesystems, but I think the risk is rather low that someone would actually\n> be using that name for a Git config file.\n\nYeah, I agree it's unlikely. And the output is already ambiguous, as the\nfirst field could be a blob (though I guess the caller knows if they\npassed \"--blob\" or not). If we really wanted an unambiguous output, we\ncould have something like \"file:...\", \"blob:...\", etc. But that's a bit\nless readable for humans, and I don't think solves any real-world\nproblems.\n\nSo I think it would be OK to use \"\" here, as long as the\ntoken is documented.\n\nAre there any other reasons why current_config_filename() would return\nNULL, besides command-line config? I don't think so. It looks like we\ncan read config from stdin, but we use the token \"\" there for the\nname field (another ambiguity!).\n\n-Peff\n"},{"id":"277699","messageId":"920E424B-B052-43A0-A0AA-59EB8E093573@gmail.com","threadId":"41337","inReplyTo":"20160205112001.GA13397@sigill.intra.peff.net","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-07T18:26:21Z","receivedAt":"2016-02-07T18:26:21Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 05 Feb 2016, at 12:20, Jeff King wrote:\n\n> On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:\n> \n>> @@ -538,6 +569,17 @@ 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>> +\tconst int is_query_action = actions & (\n>> +\t\tACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|\n>> +\t\tACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH\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> Hmm. I had originally envisioned this only being used with \"--list\", but\n> I guess it makes sense to say \"--sources --get\" to show where the value\n> for a particular option is coming from.\n> \n> The plural \"sources\" is a little funny there, though, as we list only\n> the \"last one wins\" final value, not all of them (for that, you can use\n> \"--sources --get-all\", which seems handy). I think it would be\n> sufficient for the documentation to make this clear (speaking of which,\n> this patch needs documentation...).\nOops. I will add documentation.\n\n> \n> Also, I don't think the feature works with --get-color, --get-colorbool,\n> or --get-urlmatch (which don't do their output in quite the same way).\n> I think it would be fine to simply disallow --sources with those options\n> rather than worry about making it work.\nOK, I'll remove them. I don't have experience with these options as I \nhave never really used them, yet.\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>> +\tconst char *fn = current_config_filename();\n>> +\tif (!fn)\n>> +\t\treturn;\n> \n> I'm not sure returning here is the best idea. We won't have a config\n> filename if we are reading from \"-c\", but if we return early from this\n> function, it parses differently than every other line. E.g., with your\n> patch, if I do:\n> \n> git config -c foo.bar=true config --sources --list\n> \n> I'll get:\n> \n> /home/peff/.gitconfig user.name=Jeff King\n> /home/peff/.gitconfig user.email=peff@peff.net\n> ...etc...\n> foo.bar=true\n> \n> If somebody is parsing this as a tab-delimited list, then instead of the\n> source field for that line being empty, it is missing (and it looks like\n> \"foo.bar=true\" is the source file). I think it would be more friendly to\n> consumers of the output to have a blank (i.e., set \"fn\" to the empty\n> string and continue in the function).\nI actually wondered about that exact point in your original patch but\n\"parsing\" did not come to my mind. Now I understand your reasoning and I\nagree.\n\n\n> \n>> +\n>> +\tchar term = '\\t';\n> \n> This declaration comes after the \"if\" above, but git style doesn't allow\n> declaration-after-statement (to support ancient compilers).\nInteresting, I noticed the style and wondered about it! Should we add \n\"-Werror=declaration-after-statement\" to the TravisCI [1] build to catch these\nkind of cases automatically?\n\nAfter enabling this flag the compiler showed me that I did the same\nerror a few lines above in \"const int is_query_action ...\".\n\n[1] https://travis-ci.org/larsxschneider/git/jobs/107610347\n\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\"/include.conf <<-EOF &&\n>> +\t\t[user]\n>> +\t\t\tinclude = true\n>> +\tEOF\n>> +\tcat >\"$HOME\"/file.conf <<-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\"/include.conf &&\n> \n> Here you include the file by its absolute path. I wondered what would\n> happen if we used a relative path. E.g.:\n> \n> git config include.path=foo\n> git config -f .git/foo included.config=true\n> git config --sources --list\n> \n> which shows it as \".git/foo\", because we resolved it by manipulating the\n> relative path \".git/config\". Whereas including it from ~/.gitconfig will\n> show the absolute path, because we use the absolute path to get to\n> ~/.gitconfig in the first place.\n> \n> I think that's all sane. I don't know if it's worth noting it in the\n> documentation or not.\nI agree, this is the behavior I would expect and therefore I don't think\nany additional documentation is necessary. The relative include is a good\nidea! I added it to the test case.\n\n> \n>> +\tcat >expect <<-EOF &&\n>> +\t\t$HOME/.gitconfig\tuser.global=true\n>> +\t\t$HOME/.gitconfig\tuser.override=global\n>> +\t\t$HOME/.gitconfig\tinclude.path=$INCLUDE_DIR/include.conf\n>> +\t\t$INCLUDE_DIR/include.conf\tuser.include=true\n>> +\t\t.git/config\tuser.local=true\n>> +\t\t.git/config\tuser.override=local\n>> +\t\tuser.cmdline=true\n>> +\tEOF\n> \n> If the filename has funny characters (e.g., a literal tab), it will be\n> quoted here (but not in the --null output below). Worth including in the\n> test?\nYes! Added!\n\n> \n>> +\tcat >expect <<-EOF &&\n>> +\t\t.git/config\tlocal\n>> +\tEOF\n>> +\tgit config --sources user.override >output &&\n>> +\ttest_cmp expect output &&\n> \n> Good thoroughness in checking the override case.\nThanks :)\n\n> \n>> +\tcat >expect <<-EOF &&\n>> +\t\ta9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08\tuser.custom=true\n>> +\tEOF\n>> +\tblob=$(git hash-object -w \"$HOME\"/file.conf) &&\n>> +\tgit config --blob=$blob --sources --list >output &&\n>> +\ttest_cmp expect output\n> \n> This one was unexpected to me, but I think it makes sense. The option is\n> \"--sources\" and not \"--source-filenames\", after all. It's probably worth\n> mentioning in the documentation.\nOK\n\n> \n> I think we also use the original name given, so if there was ref\n> resolution, you would see the ref name. Might be worth testing that.\nGood idea! Added!\n\n\nThanks for the review,\nLars\n"},{"id":"277701","messageId":"DF57DD4F-630F-421D-A379-0A451F6509DF@gmail.com","threadId":"41337","inReplyTo":"20160205112240.GA18581@sigill.intra.peff.net","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-07T19:28:26Z","receivedAt":"2016-02-07T19:28:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 05 Feb 2016, at 12:22, Jeff King wrote:\n\n> On Fri, Feb 05, 2016 at 12:13:04PM +0100, Sebastian Schuberth wrote:\n> \n>> On 2/5/2016 9:42, larsxschneider@gmail.com wrote:\n>> \n>>> Teach 'git config' the '--sources' option to print the source\n>>> configuration file for every printed value.\n>> \n>> Yay, not being able to see where a config setting originates from has\n>> bothered me in the past, too. So thanks for working on this.\n>> \n>> However, the naming of the '--sources' option sounds a bit misleading to me.\n>> It has nothing to do with source code. So maybe better name it '--origin',\n>> or even more verbose '--show-origin' or '--show-filename'?\n> \n> I think he inherited the \"--sources\" name from me. :) I agree it could\n> be better. I think \"--show-filename\" is not right as there are\n> non-filename cases. Just \"--origin\" sounds funny to me, perhaps because\n> of git's normal use of the word \"origin\".\n> \n> I like \"--show-origin\" the best of the ones suggested.\n\nI understand your reasoning and I agree that \"--show-origin\" is better than\n\"--sources\". However, I think just the word \"origin\" could be misleading in \nthis context because people associate it with Git remotes. How about\n\"--show-config-origin\" then? Or would that be too verbose?\n\nThanks,\nLars\n"},{"id":"277702","messageId":"278405D6-873C-4E73-965B-543D66A893D7@gmail.com","threadId":"41337","inReplyTo":"20160205135855.GA19154@sigill.intra.peff.net","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-07T19:44:50Z","receivedAt":"2016-02-07T19:44:50Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 05 Feb 2016, at 14:58, Jeff King wrote:\n\n> On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:\n> \n>>> I'm not sure returning here is the best idea. We won't have a config\n>>> filename if we are reading from \"-c\", but if we return early from this\n>>> function, it parses differently than every other line. E.g., with your\n>>> patch, if I do:\n>>> \n>>> git config -c foo.bar=true config --sources --list\n>>> \n>>> I'll get:\n>>> \n>>> /home/peff/.gitconfig user.name=Jeff King\n>>> /home/peff/.gitconfig user.email=peff@peff.net\n>>> ...etc...\n>>> foo.bar=true\n>>> \n>>> If somebody is parsing this as a tab-delimited list, then instead of the\n>>> source field for that line being empty, it is missing (and it looks like\n>>> \"foo.bar=true\" is the source file). I think it would be more friendly to\n>>> consumers of the output to have a blank (i.e., set \"fn\" to the empty\n>>> string and continue in the function).\n>> \n>> Or to come up with a special string to denote config values specified on the\n>> command line. Maybe somehting like\n>> \n>> foo.bar=true\n>> \n>> I acknowledge that \"\" would be a valid filename on some\n>> filesystems, but I think the risk is rather low that someone would actually\n>> be using that name for a Git config file.\n> \n> Yeah, I agree it's unlikely. And the output is already ambiguous, as the\n> first field could be a blob (though I guess the caller knows if they\n> passed \"--blob\" or not). If we really wanted an unambiguous output, we\n> could have something like \"file:...\", \"blob:...\", etc. But that's a bit\n> less readable for humans, and I don't think solves any real-world\n> problems.\n> \n> So I think it would be OK to use \"\" here, as long as the\n> token is documented.\nSounds good to me. I'll add it!\n\n> Are there any other reasons why current_config_filename() would return\n> NULL, besides command-line config? I don't think so. It looks like we\n> can read config from stdin, but we use the token \"\" there for the\n> name field (another ambiguity!).\nDuring my testing I haven't found any other case either. To be honest\nI didn't know the \"stdin\" way to set the config! I added a test case for \nthat, too!\n\nThanks,\nLars"},{"id":"277718","messageId":"CAHGBnuOcWDhxn+poXhxasFW3gSon7fv8sugZqPjGZFMxNyf7Ng@mail.gmail.com","threadId":"41337","inReplyTo":"DF57DD4F-630F-421D-A379-0A451F6509DF@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-08T11:22:01Z","receivedAt":"2016-02-08T11:22:01Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Sun, Feb 7, 2016 at 8:28 PM, Lars Schneider wrote:\n\n>>> However, the naming of the '--sources' option sounds a bit misleading to me.\n>>> It has nothing to do with source code. So maybe better name it '--origin',\n>>> or even more verbose '--show-origin' or '--show-filename'?\n>>\n>> I think he inherited the \"--sources\" name from me. :) I agree it could\n>> be better. I think \"--show-filename\" is not right as there are\n>> non-filename cases. Just \"--origin\" sounds funny to me, perhaps because\n>> of git's normal use of the word \"origin\".\n>>\n>> I like \"--show-origin\" the best of the ones suggested.\n>\n> I understand your reasoning and I agree that \"--show-origin\" is better than\n> \"--sources\". However, I think just the word \"origin\" could be misleading in\n> this context because people associate it with Git remotes. How about\n> \"--show-config-origin\" then? Or would that be too verbose?\n\nWell, \"origin\" just happens to be the name of the default remote.\nAFAIK all options that deal with remotes have \"remote\" and not\n\"origin\" in their name, so I think the risk of confusion is rather\nlow. But I'd be fine with \"--show-config-origin\", too. Although it's\nverbose, it's probably not used very often, so personally I could live\nwith typing the extra character. Esp. if you add Bash completion\nsupport for it :-)\n\n-- \nSebastian Schuberth\n"},{"id":"277719","messageId":"56B87B46.6050304@gmail.com","threadId":"41337","inReplyTo":"20160205135855.GA19154@sigill.intra.peff.net","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2016-02-08T11:25:58Z","receivedAt":"2016-02-08T11:25:58Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2/5/2016 14:58, Jeff King wrote:\n\n> Yeah, I agree it's unlikely. And the output is already ambiguous, as the\n> first field could be a blob (though I guess the caller knows if they\n> passed \"--blob\" or not). If we really wanted an unambiguous output, we\n> could have something like \"file:...\", \"blob:...\", etc. But that's a bit\n> less readable for humans, and I don't think solves any real-world\n> problems.\n>\n> So I think it would be OK to use \"\" here, as long as the\n> token is documented.\n\nThinking about it again, I actually do like Peff's prefix solution \nbetter. It would solve the real-world problem that my proposed \"\" marker could in fact be a file name.\n\nRegards,\nSebastian\n"},{"id":"277720","messageId":"20160208120838.GA24217@sigill.intra.peff.net","threadId":"41337","inReplyTo":"56B87B46.6050304@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T12:08:39Z","receivedAt":"2016-02-08T12:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 12:25:58PM +0100, Sebastian Schuberth wrote:\n\n> On 2/5/2016 14:58, Jeff King wrote:\n> \n> >Yeah, I agree it's unlikely. And the output is already ambiguous, as the\n> >first field could be a blob (though I guess the caller knows if they\n> >passed \"--blob\" or not). If we really wanted an unambiguous output, we\n> >could have something like \"file:...\", \"blob:...\", etc. But that's a bit\n> >less readable for humans, and I don't think solves any real-world\n> >problems.\n> >\n> >So I think it would be OK to use \"\" here, as long as the\n> >token is documented.\n> \n> Thinking about it again, I actually do like Peff's prefix solution better.\n> It would solve the real-world problem that my proposed \"\"\n> marker could in fact be a file name.\n\nI'm OK with that direction, but I think it will need some more\ninfrastructure in the config code (right now we just set \"name\" to some\nstring without recording its type). It probably would not be too hard to\nadd, though.\n\n-Peff\n"},{"id":"277721","messageId":"20160208121108.GB24217@sigill.intra.peff.net","threadId":"41337","inReplyTo":"DF57DD4F-630F-421D-A379-0A451F6509DF@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T12:11:09Z","receivedAt":"2016-02-08T12:11:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 07, 2016 at 08:28:26PM +0100, Lars Schneider wrote:\n\n> > I think he inherited the \"--sources\" name from me. :) I agree it could\n> > be better. I think \"--show-filename\" is not right as there are\n> > non-filename cases. Just \"--origin\" sounds funny to me, perhaps because\n> > of git's normal use of the word \"origin\".\n> > \n> > I like \"--show-origin\" the best of the ones suggested.\n> \n> I understand your reasoning and I agree that \"--show-origin\" is better than\n> \"--sources\". However, I think just the word \"origin\" could be misleading in \n> this context because people associate it with Git remotes. How about\n> \"--show-config-origin\" then? Or would that be too verbose?\n\nI think \"-config\" is redundant here, as we are running \"git config\". We\nhave \"--add\" and not \"--add-config\", after all.\n\nI dunno. TBH, I do not find --sources to be all that bad. I suspect we\ncould nitpick any name, and at least it is short and unique.\n\n-Peff\n"},{"id":"277722","messageId":"20160208121249.GC24217@sigill.intra.peff.net","threadId":"41337","inReplyTo":"278405D6-873C-4E73-965B-543D66A893D7@gmail.com","subject":"Re: [PATCH v1] config: add '--sources' option to print the source of a config value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T12:12:49Z","receivedAt":"2016-02-08T12:12:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 07, 2016 at 08:44:50PM +0100, Lars Schneider wrote:\n\n> > Are there any other reasons why current_config_filename() would return\n> > NULL, besides command-line config? I don't think so. It looks like we\n> > can read config from stdin, but we use the token \"\" there for the\n> > name field (another ambiguity!).\n> During my testing I haven't found any other case either. To be honest\n> I didn't know the \"stdin\" way to set the config! I added a test case for \n> that, too!\n\nI didn't remember it either, until I peeked at the code trying to answer my\nown question. Ironically, I appear to have been involved in reviewing\nthe patches that added it. :-/\n\n-Peff\n"}]}