[PATCH v1] config: add '--sources' option to print the source of a config value

[PATCH v1] config: add '--sources' option to print the source of a config value

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

If config values are queried using 'git config' (e.g. via '--list' flag
or the '--get*' flags) then it is sometimes hard to find the
configuration file where the values were defined.

Teach 'git config' the '--sources' option to print the source
configuration file for every printed value.

Based-on-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 builtin/config.c       | 42 ++++++++++++++++++++++++++++++++
 cache.h                |  1 +
 config.c               |  7 ++++++
 t/t1300-repo-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index adc7727..f5dc79c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -3,6 +3,7 @@
 #include "color.h"
 #include "parse-options.h"
 #include "urlmatch.h"
+#include "quote.h"
 
 static const char *const builtin_config_usage[] = {
 	N_("git config [<options>]"),
@@ -27,6 +28,7 @@ static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 static int respect_includes = -1;
+static int show_sources;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")),
+	OPT_BOOL(0, "sources", &show_sources, N_("show source filenames of config")),
 	OPT_END(),
 };
 
@@ -91,8 +94,34 @@ static void check_argc(int argc, int min, int max) {
 	usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
+/* output to either fp or buf; only one should be non-NULL */
+static void show_config_source(struct strbuf *buf, FILE *fp)
+{
+	const char *fn = current_config_filename();
+	if (!fn)
+		return;
+
+	char term = '\t';
+	if (!end_null)
+		quote_c_style(fn, buf, fp, 0);
+	else {
+		term = '\0';
+		if (fp)
+			fprintf(fp, "%s", fn);
+		else
+			strbuf_addstr(buf, fn);
+	}
+
+	if (fp)
+		fputc(term, fp);
+	else
+		strbuf_addch(buf, term);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+	if (show_sources)
+		show_config_source(NULL, stdout);
 	if (!omit_values && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
@@ -108,6 +137,8 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char *value_)
 {
+	if (show_sources)
+		show_config_source(buf, NULL);
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
@@ -538,6 +569,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		error("--name-only is only applicable to --list or --get-regexp");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
+
+	const int is_query_action = actions & (
+		ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|
+		ACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH
+	);
+
+	if (show_sources && !is_query_action) {
+		error("--sources is only applicable to --list or --get-* actions");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
diff --git a/cache.h b/cache.h
index c63fcc1..c5111ea 100644
--- a/cache.h
+++ b/cache.h
@@ -1525,6 +1525,7 @@ extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern const char *current_config_filename(void);
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index 86a5eb2..b437002 100644
--- a/config.c
+++ b/config.c
@@ -2385,3 +2385,10 @@ int parse_config_key(const char *var,
 
 	return 0;
 }
+
+const char *current_config_filename(void)
+{
+	if (cf && cf->name)
+		return cf->name;
+	return NULL;
+}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 52678e7..2444d8a 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1201,4 +1201,69 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success '--sources' '
+	>.git/config &&
+	>"$HOME"/.gitconfig &&
+	INCLUDE_DIR="$HOME/include" &&
+	mkdir -p "$INCLUDE_DIR" &&
+	cat >"$INCLUDE_DIR"/include.conf <<-EOF &&
+		[user]
+			include = true
+	EOF
+	cat >"$HOME"/file.conf <<-EOF &&
+		[user]
+			custom = true
+	EOF
+	test_config_global user.global "true" &&
+	test_config_global user.override "global" &&
+	test_config_global include.path "$INCLUDE_DIR"/include.conf &&
+	test_config user.local "true" &&
+	test_config user.override "local" &&
+
+	cat >expect <<-EOF &&
+		$HOME/.gitconfig	user.global=true
+		$HOME/.gitconfig	user.override=global
+		$HOME/.gitconfig	include.path=$INCLUDE_DIR/include.conf
+		$INCLUDE_DIR/include.conf	user.include=true
+		.git/config	user.local=true
+		.git/config	user.override=local
+		user.cmdline=true
+	EOF
+	git -c user.cmdline=true config --list --sources >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		$HOME/.gitconfigQuser.global
+		trueQ$HOME/.gitconfigQuser.override
+		globalQ$HOME/.gitconfigQinclude.path
+		$INCLUDE_DIR/include.confQ$INCLUDE_DIR/include.confQuser.include
+		trueQ.git/configQuser.local
+		trueQ.git/configQuser.override
+		localQuser.cmdline
+		trueQ
+	EOF
+	git -c user.cmdline=true config --null --list --sources | nul_to_q >output &&
+	echo >>output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		.git/config	local
+	EOF
+	git config --sources user.override >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		$HOME/file.conf	user.custom=true
+	EOF
+	git config --file "$HOME"/file.conf --sources --list >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
+	EOF
+	blob=$(git hash-object -w "$HOME"/file.conf) &&
+	git config --blob=$blob --sources --list >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
2.5.1

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Sebastian Schuberth]

On 2/5/2016 9:42, larsxschneider@gmail.com wrote:

> Teach 'git config' the '--sources' option to print the source
> configuration file for every printed value.

Yay, not being able to see where a config setting originates from has 
bothered me in the past, too. So thanks for working on this.

However, the naming of the '--sources' option sounds a bit misleading to 
me. It has nothing to do with source code. So maybe better name it 
'--origin', or even more verbose '--show-origin' or '--show-filename'?

Regards,
Sebastian

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:

> @@ -538,6 +569,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		error("--name-only is only applicable to --list or --get-regexp");
>  		usage_with_options(builtin_config_usage, builtin_config_options);
>  	}
> +
> +	const int is_query_action = actions & (
> +		ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|
> +		ACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH
> +	);
> +
> +	if (show_sources && !is_query_action) {
> +		error("--sources is only applicable to --list or --get-* actions");
> +		usage_with_options(builtin_config_usage, builtin_config_options);
> +	}

Hmm. I had originally envisioned this only being used with "--list", but
I guess it makes sense to say "--sources --get" to show where the value
for a particular option is coming from.

The plural "sources" is a little funny there, though, as we list only
the "last one wins" final value, not all of them (for that, you can use
"--sources --get-all", which seems handy). I think it would be
sufficient for the documentation to make this clear (speaking of which,
this patch needs documentation...).

Also, I don't think the feature works with --get-color, --get-colorbool,
or --get-urlmatch (which don't do their output in quite the same way).
I think it would be fine to simply disallow --sources with those options
rather than worry about making it work.

> +/* output to either fp or buf; only one should be non-NULL */
> +static void show_config_source(struct strbuf *buf, FILE *fp)
> +{
> +	const char *fn = current_config_filename();
> +	if (!fn)
> +		return;

I'm not sure returning here is the best idea. We won't have a config
filename if we are reading from "-c", but if we return early from this
function, it parses differently than every other line. E.g., with your
patch, if I do:

  git config -c foo.bar=true config --sources --list

I'll get:

  /home/peff/.gitconfig <tab> user.name=Jeff King
  /home/peff/.gitconfig <tab> user.email=peff@peff.net
  ...etc...
  foo.bar=true

If somebody is parsing this as a tab-delimited list, then instead of the
source field for that line being empty, it is missing (and it looks like
"foo.bar=true" is the source file). I think it would be more friendly to
consumers of the output to have a blank (i.e., set "fn" to the empty
string and continue in the function).

> +
> +	char term = '\t';

This declaration comes after the "if" above, but git style doesn't allow
declaration-after-statement (to support ancient compilers).

> +test_expect_success '--sources' '
> +	>.git/config &&
> +	>"$HOME"/.gitconfig &&
> +	INCLUDE_DIR="$HOME/include" &&
> +	mkdir -p "$INCLUDE_DIR" &&
> +	cat >"$INCLUDE_DIR"/include.conf <<-EOF &&
> +		[user]
> +			include = true
> +	EOF
> +	cat >"$HOME"/file.conf <<-EOF &&
> +		[user]
> +			custom = true
> +	EOF
> +	test_config_global user.global "true" &&
> +	test_config_global user.override "global" &&
> +	test_config_global include.path "$INCLUDE_DIR"/include.conf &&

Here you include the file by its absolute path. I wondered what would
happen if we used a relative path. E.g.:

  git config include.path=foo
  git config -f .git/foo included.config=true
  git config --sources --list

which shows it as ".git/foo", because we resolved it by manipulating the
relative path ".git/config". Whereas including it from ~/.gitconfig will
show the absolute path, because we use the absolute path to get to
~/.gitconfig in the first place.

I think that's all sane. I don't know if it's worth noting it in the
documentation or not.

> +	cat >expect <<-EOF &&
> +		$HOME/.gitconfig	user.global=true
> +		$HOME/.gitconfig	user.override=global
> +		$HOME/.gitconfig	include.path=$INCLUDE_DIR/include.conf
> +		$INCLUDE_DIR/include.conf	user.include=true
> +		.git/config	user.local=true
> +		.git/config	user.override=local
> +		user.cmdline=true
> +	EOF

If the filename has funny characters (e.g., a literal tab), it will be
quoted here (but not in the --null output below). Worth including in the
test?

> +	cat >expect <<-EOF &&
> +		.git/config	local
> +	EOF
> +	git config --sources user.override >output &&
> +	test_cmp expect output &&

Good thoroughness in checking the override case.

> +	cat >expect <<-EOF &&
> +		a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
> +	EOF
> +	blob=$(git hash-object -w "$HOME"/file.conf) &&
> +	git config --blob=$blob --sources --list >output &&
> +	test_cmp expect output

This one was unexpected to me, but I think it makes sense. The option is
"--sources" and not "--source-filenames", after all. It's probably worth
mentioning in the documentation.

I think we also use the original name given, so if there was ref
resolution, you would see the ref name. Might be worth testing that.

-Peff

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Fri, Feb 05, 2016 at 12:13:04PM +0100, Sebastian Schuberth wrote:

> On 2/5/2016 9:42, larsxschneider@gmail.com wrote:
> 
> >Teach 'git config' the '--sources' option to print the source
> >configuration file for every printed value.
> 
> Yay, not being able to see where a config setting originates from has
> bothered me in the past, too. So thanks for working on this.
> 
> However, the naming of the '--sources' option sounds a bit misleading to me.
> It has nothing to do with source code. So maybe better name it '--origin',
> or even more verbose '--show-origin' or '--show-filename'?

I think he inherited the "--sources" name from me. :) I agree it could
be better. I think "--show-filename" is not right as there are
non-filename cases.  Just "--origin" sounds funny to me, perhaps because
of git's normal use of the word "origin".

I like "--show-origin" the best of the ones suggested.

-Peff

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Sebastian Schuberth]

On 2/5/2016 12:20, Jeff King wrote:

> Hmm. I had originally envisioned this only being used with "--list", but
> I guess it makes sense to say "--sources --get" to show where the value
> for a particular option is coming from.

Being able to use "--sources --get" is a feature that I'd definitely 
like to see, too.

> I'm not sure returning here is the best idea. We won't have a config
> filename if we are reading from "-c", but if we return early from this
> function, it parses differently than every other line. E.g., with your
> patch, if I do:
>
>    git config -c foo.bar=true config --sources --list
>
> I'll get:
>
>    /home/peff/.gitconfig <tab> user.name=Jeff King
>    /home/peff/.gitconfig <tab> user.email=peff@peff.net
>    ...etc...
>    foo.bar=true
>
> If somebody is parsing this as a tab-delimited list, then instead of the
> source field for that line being empty, it is missing (and it looks like
> "foo.bar=true" is the source file). I think it would be more friendly to
> consumers of the output to have a blank (i.e., set "fn" to the empty
> string and continue in the function).

Or to come up with a special string to denote config values specified on 
the command line. Maybe somehting like

     <command line> <tab> foo.bar=true

I acknowledge that "<command line>" would be a valid filename on some 
filesystems, but I think the risk is rather low that someone would 
actually be using that name for a Git config file.

Regards,
Sebastian

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:

> >I'm not sure returning here is the best idea. We won't have a config
> >filename if we are reading from "-c", but if we return early from this
> >function, it parses differently than every other line. E.g., with your
> >patch, if I do:
> >
> >   git config -c foo.bar=true config --sources --list
> >
> >I'll get:
> >
> >   /home/peff/.gitconfig <tab> user.name=Jeff King
> >   /home/peff/.gitconfig <tab> user.email=peff@peff.net
> >   ...etc...
> >   foo.bar=true
> >
> >If somebody is parsing this as a tab-delimited list, then instead of the
> >source field for that line being empty, it is missing (and it looks like
> >"foo.bar=true" is the source file). I think it would be more friendly to
> >consumers of the output to have a blank (i.e., set "fn" to the empty
> >string and continue in the function).
> 
> Or to come up with a special string to denote config values specified on the
> command line. Maybe somehting like
> 
>     <command line> <tab> foo.bar=true
> 
> I acknowledge that "<command line>" would be a valid filename on some
> filesystems, but I think the risk is rather low that someone would actually
> be using that name for a Git config file.

Yeah, I agree it's unlikely. And the output is already ambiguous, as the
first field could be a blob (though I guess the caller knows if they
passed "--blob" or not). If we really wanted an unambiguous output, we
could have something like "file:...", "blob:...", etc. But that's a bit
less readable for humans, and I don't think solves any real-world
problems.

So I think it would be OK to use "<command line>" here, as long as the
token is documented.

Are there any other reasons why current_config_filename() would return
NULL, besides command-line config? I don't think so. It looks like we
can read config from stdin, but we use the token "<stdin>" there for the
name field (another ambiguity!).

-Peff

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 12:20, Jeff King <peff@peff.net> wrote:

> On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:
> 
>> @@ -538,6 +569,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>> 		error("--name-only is only applicable to --list or --get-regexp");
>> 		usage_with_options(builtin_config_usage, builtin_config_options);
>> 	}
>> +
>> +	const int is_query_action = actions & (
>> +		ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|
>> +		ACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH
>> +	);
>> +
>> +	if (show_sources && !is_query_action) {
>> +		error("--sources is only applicable to --list or --get-* actions");
>> +		usage_with_options(builtin_config_usage, builtin_config_options);
>> +	}
> 
> Hmm. I had originally envisioned this only being used with "--list", but
> I guess it makes sense to say "--sources --get" to show where the value
> for a particular option is coming from.
> 
> The plural "sources" is a little funny there, though, as we list only
> the "last one wins" final value, not all of them (for that, you can use
> "--sources --get-all", which seems handy). I think it would be
> sufficient for the documentation to make this clear (speaking of which,
> this patch needs documentation...).
Oops. I will add documentation.

> 
> Also, I don't think the feature works with --get-color, --get-colorbool,
> or --get-urlmatch (which don't do their output in quite the same way).
> I think it would be fine to simply disallow --sources with those options
> rather than worry about making it work.
OK, I'll remove them. I don't have experience with these options as I 
have never really used them, yet.


>> +/* output to either fp or buf; only one should be non-NULL */
>> +static void show_config_source(struct strbuf *buf, FILE *fp)
>> +{
>> +	const char *fn = current_config_filename();
>> +	if (!fn)
>> +		return;
> 
> I'm not sure returning here is the best idea. We won't have a config
> filename if we are reading from "-c", but if we return early from this
> function, it parses differently than every other line. E.g., with your
> patch, if I do:
> 
>  git config -c foo.bar=true config --sources --list
> 
> I'll get:
> 
>  /home/peff/.gitconfig <tab> user.name=Jeff King
>  /home/peff/.gitconfig <tab> user.email=peff@peff.net
>  ...etc...
>  foo.bar=true
> 
> If somebody is parsing this as a tab-delimited list, then instead of the
> source field for that line being empty, it is missing (and it looks like
> "foo.bar=true" is the source file). I think it would be more friendly to
> consumers of the output to have a blank (i.e., set "fn" to the empty
> string and continue in the function).
I actually wondered about that exact point in your original patch but
"parsing" did not come to my mind. Now I understand your reasoning and I
agree.


> 
>> +
>> +	char term = '\t';
> 
> This declaration comes after the "if" above, but git style doesn't allow
> declaration-after-statement (to support ancient compilers).
Interesting, I noticed the style and wondered about it! Should we add 
"-Werror=declaration-after-statement" to the TravisCI [1] build to catch these
kind of cases automatically?

After enabling this flag the compiler showed me that I did the same
error a few lines above in "const int is_query_action ...".

[1] https://travis-ci.org/larsxschneider/git/jobs/107610347


> 
>> +test_expect_success '--sources' '
>> +	>.git/config &&
>> +	>"$HOME"/.gitconfig &&
>> +	INCLUDE_DIR="$HOME/include" &&
>> +	mkdir -p "$INCLUDE_DIR" &&
>> +	cat >"$INCLUDE_DIR"/include.conf <<-EOF &&
>> +		[user]
>> +			include = true
>> +	EOF
>> +	cat >"$HOME"/file.conf <<-EOF &&
>> +		[user]
>> +			custom = true
>> +	EOF
>> +	test_config_global user.global "true" &&
>> +	test_config_global user.override "global" &&
>> +	test_config_global include.path "$INCLUDE_DIR"/include.conf &&
> 
> Here you include the file by its absolute path. I wondered what would
> happen if we used a relative path. E.g.:
> 
>  git config include.path=foo
>  git config -f .git/foo included.config=true
>  git config --sources --list
> 
> which shows it as ".git/foo", because we resolved it by manipulating the
> relative path ".git/config". Whereas including it from ~/.gitconfig will
> show the absolute path, because we use the absolute path to get to
> ~/.gitconfig in the first place.
> 
> I think that's all sane. I don't know if it's worth noting it in the
> documentation or not.
I agree, this is the behavior I would expect and therefore I don't think
any additional documentation is necessary. The relative include is a good
idea! I added it to the test case.

> 
>> +	cat >expect <<-EOF &&
>> +		$HOME/.gitconfig	user.global=true
>> +		$HOME/.gitconfig	user.override=global
>> +		$HOME/.gitconfig	include.path=$INCLUDE_DIR/include.conf
>> +		$INCLUDE_DIR/include.conf	user.include=true
>> +		.git/config	user.local=true
>> +		.git/config	user.override=local
>> +		user.cmdline=true
>> +	EOF
> 
> If the filename has funny characters (e.g., a literal tab), it will be
> quoted here (but not in the --null output below). Worth including in the
> test?
Yes! Added!

> 
>> +	cat >expect <<-EOF &&
>> +		.git/config	local
>> +	EOF
>> +	git config --sources user.override >output &&
>> +	test_cmp expect output &&
> 
> Good thoroughness in checking the override case.
Thanks :)

> 
>> +	cat >expect <<-EOF &&
>> +		a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
>> +	EOF
>> +	blob=$(git hash-object -w "$HOME"/file.conf) &&
>> +	git config --blob=$blob --sources --list >output &&
>> +	test_cmp expect output
> 
> This one was unexpected to me, but I think it makes sense. The option is
> "--sources" and not "--source-filenames", after all. It's probably worth
> mentioning in the documentation.
OK

> 
> I think we also use the original name given, so if there was ref
> resolution, you would see the ref name. Might be worth testing that.
Good idea! Added!


Thanks for the review,
Lars

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 12:22, Jeff King <peff@peff.net> wrote:

> On Fri, Feb 05, 2016 at 12:13:04PM +0100, Sebastian Schuberth wrote:
> 
>> On 2/5/2016 9:42, larsxschneider@gmail.com wrote:
>> 
>>> Teach 'git config' the '--sources' option to print the source
>>> configuration file for every printed value.
>> 
>> Yay, not being able to see where a config setting originates from has
>> bothered me in the past, too. So thanks for working on this.
>> 
>> However, the naming of the '--sources' option sounds a bit misleading to me.
>> It has nothing to do with source code. So maybe better name it '--origin',
>> or even more verbose '--show-origin' or '--show-filename'?
> 
> I think he inherited the "--sources" name from me. :) I agree it could
> be better. I think "--show-filename" is not right as there are
> non-filename cases.  Just "--origin" sounds funny to me, perhaps because
> of git's normal use of the word "origin".
> 
> I like "--show-origin" the best of the ones suggested.

I understand your reasoning and I agree that "--show-origin" is better than
"--sources". However, I think just the word "origin" could be misleading in 
this context because people associate it with Git remotes. How about
"--show-config-origin" then? Or would that be too verbose?

Thanks,
Lars

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 14:58, Jeff King <peff@peff.net> wrote:

> On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:
> 
>>> I'm not sure returning here is the best idea. We won't have a config
>>> filename if we are reading from "-c", but if we return early from this
>>> function, it parses differently than every other line. E.g., with your
>>> patch, if I do:
>>> 
>>>  git config -c foo.bar=true config --sources --list
>>> 
>>> I'll get:
>>> 
>>>  /home/peff/.gitconfig <tab> user.name=Jeff King
>>>  /home/peff/.gitconfig <tab> user.email=peff@peff.net
>>>  ...etc...
>>>  foo.bar=true
>>> 
>>> If somebody is parsing this as a tab-delimited list, then instead of the
>>> source field for that line being empty, it is missing (and it looks like
>>> "foo.bar=true" is the source file). I think it would be more friendly to
>>> consumers of the output to have a blank (i.e., set "fn" to the empty
>>> string and continue in the function).
>> 
>> Or to come up with a special string to denote config values specified on the
>> command line. Maybe somehting like
>> 
>>    <command line> <tab> foo.bar=true
>> 
>> I acknowledge that "<command line>" would be a valid filename on some
>> filesystems, but I think the risk is rather low that someone would actually
>> be using that name for a Git config file.
> 
> Yeah, I agree it's unlikely. And the output is already ambiguous, as the
> first field could be a blob (though I guess the caller knows if they
> passed "--blob" or not). If we really wanted an unambiguous output, we
> could have something like "file:...", "blob:...", etc. But that's a bit
> less readable for humans, and I don't think solves any real-world
> problems.
> 
> So I think it would be OK to use "<command line>" here, as long as the
> token is documented.
Sounds good to me. I'll add it!

> Are there any other reasons why current_config_filename() would return
> NULL, besides command-line config? I don't think so. It looks like we
> can read config from stdin, but we use the token "<stdin>" there for the
> name field (another ambiguity!).
During my testing I haven't found any other case either. To be honest
I didn't know the "stdin" way to set the config! I added a test case for 
that, too!

Thanks,
Lars

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Sebastian Schuberth]

On Sun, Feb 7, 2016 at 8:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

>>> However, the naming of the '--sources' option sounds a bit misleading to me.
>>> It has nothing to do with source code. So maybe better name it '--origin',
>>> or even more verbose '--show-origin' or '--show-filename'?
>>
>> I think he inherited the "--sources" name from me. :) I agree it could
>> be better. I think "--show-filename" is not right as there are
>> non-filename cases.  Just "--origin" sounds funny to me, perhaps because
>> of git's normal use of the word "origin".
>>
>> I like "--show-origin" the best of the ones suggested.
>
> I understand your reasoning and I agree that "--show-origin" is better than
> "--sources". However, I think just the word "origin" could be misleading in
> this context because people associate it with Git remotes. How about
> "--show-config-origin" then? Or would that be too verbose?

Well, "origin" just happens to be the name of the default remote.
AFAIK all options that deal with remotes have "remote" and not
"origin" in their name, so I think the risk of confusion is rather
low. But I'd be fine with "--show-config-origin", too. Although it's
verbose, it's probably not used very often, so personally I could live
with typing the extra character. Esp. if you add Bash completion
support for it :-)

-- 
Sebastian Schuberth

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Sebastian Schuberth]

On 2/5/2016 14:58, Jeff King wrote:

> Yeah, I agree it's unlikely. And the output is already ambiguous, as the
> first field could be a blob (though I guess the caller knows if they
> passed "--blob" or not). If we really wanted an unambiguous output, we
> could have something like "file:...", "blob:...", etc. But that's a bit
> less readable for humans, and I don't think solves any real-world
> problems.
>
> So I think it would be OK to use "<command line>" here, as long as the
> token is documented.

Thinking about it again, I actually do like Peff's prefix solution 
better. It would solve the real-world problem that my proposed "<command 
line>" marker could in fact be a file name.

Regards,
Sebastian

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Mon, Feb 08, 2016 at 12:25:58PM +0100, Sebastian Schuberth wrote:

> On 2/5/2016 14:58, Jeff King wrote:
> 
> >Yeah, I agree it's unlikely. And the output is already ambiguous, as the
> >first field could be a blob (though I guess the caller knows if they
> >passed "--blob" or not). If we really wanted an unambiguous output, we
> >could have something like "file:...", "blob:...", etc. But that's a bit
> >less readable for humans, and I don't think solves any real-world
> >problems.
> >
> >So I think it would be OK to use "<command line>" here, as long as the
> >token is documented.
> 
> Thinking about it again, I actually do like Peff's prefix solution better.
> It would solve the real-world problem that my proposed "<command line>"
> marker could in fact be a file name.

I'm OK with that direction, but I think it will need some more
infrastructure in the config code (right now we just set "name" to some
string without recording its type). It probably would not be too hard to
add, though.

-Peff

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Sun, Feb 07, 2016 at 08:28:26PM +0100, Lars Schneider wrote:

> > I think he inherited the "--sources" name from me. :) I agree it could
> > be better. I think "--show-filename" is not right as there are
> > non-filename cases.  Just "--origin" sounds funny to me, perhaps because
> > of git's normal use of the word "origin".
> > 
> > I like "--show-origin" the best of the ones suggested.
> 
> I understand your reasoning and I agree that "--show-origin" is better than
> "--sources". However, I think just the word "origin" could be misleading in 
> this context because people associate it with Git remotes. How about
> "--show-config-origin" then? Or would that be too verbose?

I think "-config" is redundant here, as we are running "git config". We
have "--add" and not "--add-config", after all.

I dunno. TBH, I do not find --sources to be all that bad. I suspect we
could nitpick any name, and at least it is short and unique.

-Peff

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Jeff King]

On Sun, Feb 07, 2016 at 08:44:50PM +0100, Lars Schneider wrote:

> > Are there any other reasons why current_config_filename() would return
> > NULL, besides command-line config? I don't think so. It looks like we
> > can read config from stdin, but we use the token "<stdin>" there for the
> > name field (another ambiguity!).
> During my testing I haven't found any other case either. To be honest
> I didn't know the "stdin" way to set the config! I added a test case for 
> that, too!

I didn't remember it either, until I peeked at the code trying to answer my
own question. Ironically, I appear to have been involved in reviewing
the patches that added it. :-/

-Peff