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

[PATCH v2] 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>
---

diff to v1:

* add documention
* produce a consistent (tab-delimited) format that can be parsed
* adhere declaration-after-statement style
* prefix every source line with the source type (file, stdin, blob, cmd)
* add relative path test case
* add blob ref test case
* add "git config --local" test case (Note: I think I found a bug there that I
  plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)
* add a test case to check funny character escapes (file name with tabs)

Sebastian suggested "--show-origin" as a better option name over "--sources".
I still believe "--sources" might be slightly better as I fear that users could
somehow related "origin" to "remote" kind of configs. However, I am happy to
change that if a majority prefers "--show-origin".

Thanks to Peff and Sebastian for the review,
Lars

 Documentation/git-config.txt |  15 ++++--
 builtin/config.c             |  66 ++++++++++++++++++++++++++
 cache.h                      |   1 +
 config.c                     |   9 +++-
 t/t1300-repo-config.sh       | 109 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 194 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2608ca7..8c4969f 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,18 +9,18 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [type] [--sources] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [type] --add name value
 'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [type] [--sources] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [type] [--sources] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [type] [--sources] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
-'git config' [<file-option>] [-z|--null] [--name-only] -l | --list
+'git config' [<file-option>] [--sources] [-z|--null] [--name-only] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -194,6 +194,11 @@ See also <<FILES>>.
 	Output only the names of config variables for `--list` or
 	`--get-regexp`.

+--sources::
+	Augment the output of all queried config options with the
+	source type (file, stdin, blob, cmd) and the actual source
+	(config file path, ref, or blob id if applicable).
+
 --get-colorbool name [stdout-is-tty]::

 	Find the color setting for `name` (e.g. `color.diff`) and output
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..78f3344 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,58 @@ 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)
+{
+	char term = '\t';
+	char *prefix;
+	const char *fn = current_config_filename();
+
+	if (end_null)
+		term = '\0';
+
+	if (fn) {
+		if (given_config_source.use_stdin)
+			prefix = "stdin";
+		else if (given_config_source.blob)
+			prefix = "blob";
+		else
+			prefix = "file";
+	} else {
+		fn = "";
+		prefix = "cmd";
+	}
+
+	if (fp)
+		fprintf(fp, "%s", prefix);
+	else {
+		strbuf_addstr(buf, prefix);
+	}
+
+	if (fp)
+		fputc(term, fp);
+	else
+		strbuf_addch(buf, term);
+
+	if (!end_null)
+		quote_c_style(fn, buf, fp, 0);
+	else {
+		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 +161,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) {
@@ -451,6 +506,7 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
+	int is_query_action = 0;
 	char *value;

 	given_config_source.file = getenv(CONFIG_ENVIRONMENT);
@@ -538,6 +594,16 @@ 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);
 	}
+
+	is_query_action = actions & (
+		ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST
+	);
+
+	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..204e6b7 100644
--- a/config.c
+++ b/config.c
@@ -1078,7 +1078,7 @@ static int do_config_from_file(config_fn_t fn,

 static int git_config_from_stdin(config_fn_t fn, void *data)
 {
-	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
+	return do_config_from_file(fn, "", NULL, stdin, data);
 }

 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
@@ -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..d69974e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1201,4 +1201,113 @@ 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"/absolute.include <<-EOF &&
+		[user]
+			absolute = include
+	EOF
+	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
+		[user]
+			relative = include
+	EOF
+	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
+	cat >"$CUSTOM_CONFIG_FILE" <<-EOF &&
+		[user]
+			custom = true
+	EOF
+	test_config_global user.global "true" &&
+	test_config_global user.override "global" &&
+	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
+	test_config include.path ../include/relative.include &&
+	test_config user.local "true" &&
+	test_config user.override "local" &&
+
+	cat >expect <<-EOF &&
+		file	$HOME/.gitconfig	user.global=true
+		file	$HOME/.gitconfig	user.override=global
+		file	$HOME/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
+		file	$INCLUDE_DIR/absolute.include	user.absolute=include
+		file	.git/config	include.path=../include/relative.include
+		file	.git/../include/relative.include	user.relative=include
+		file	.git/config	user.local=true
+		file	.git/config	user.override=local
+		cmd		user.cmdline=true
+	EOF
+	git -c user.cmdline=true config --list --sources >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		fileQ$HOME/.gitconfigQuser.global
+		trueQfileQ$HOME/.gitconfigQuser.override
+		globalQfileQ$HOME/.gitconfigQinclude.path
+		$INCLUDE_DIR/absolute.includeQfileQ$INCLUDE_DIR/absolute.includeQuser.absolute
+		includeQfileQ.git/configQinclude.path
+		../include/relative.includeQfileQ.git/../include/relative.includeQuser.relative
+		includeQfileQ.git/configQuser.local
+		trueQfileQ.git/configQuser.override
+		localQcmdQQuser.cmdline
+		trueQ
+	EOF
+	git -c user.cmdline=true config --null --list --sources | nul_to_q >output &&
+	echo >>output &&
+	test_cmp expect output &&
+
+	# TODO:
+	# The locally included config value is not printed below. I think
+	# this is a bug.
+	# "file	.git/../include/relative.include	user.relative=include"
+	#
+	cat >expect <<-EOF &&
+		file	.git/config	include.path=../include/relative.include
+		file	.git/config	user.local=true
+		file	.git/config	user.override=local
+		EOF
+	git config --local --list --sources >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file	$HOME/.gitconfig	user.global true
+		file	.git/config	user.local true
+	EOF
+	git config --sources --get-regexp "user\.[g|l].*" >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file	.git/config	local
+	EOF
+	git config --sources user.override >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file	"file\\\twith\\\ttabs.conf"	user.custom=true
+	EOF
+	git config --file "$CUSTOM_CONFIG_FILE" --sources --list >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		stdin		user.custom=true
+	EOF
+	git config --file - --sources --list <"$CUSTOM_CONFIG_FILE" >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		blob	a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
+	EOF
+	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+	git config --blob=$blob --sources --list >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		blob	"master:file\\\twith\\\ttabs.conf"	user.custom=true
+	EOF
+	git add "$CUSTOM_CONFIG_FILE" &&
+	git commit -m "blubb" &&
+	git config --blob=master:"$CUSTOM_CONFIG_FILE" --sources --list >output &&
+	test_cmp expect output
+'
+
 test_done
--
2.5.1

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

[Ramsay Jones]

On 10/02/16 10:13, larsxschneider@gmail.com wrote:
> 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>
> ---
> 
> diff to v1:
> 
> * add documention
> * produce a consistent (tab-delimited) format that can be parsed
> * adhere declaration-after-statement style
> * prefix every source line with the source type (file, stdin, blob, cmd)
> * add relative path test case
> * add blob ref test case
> * add "git config --local" test case (Note: I think I found a bug there that I
>   plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)
> * add a test case to check funny character escapes (file name with tabs)
> 
> Sebastian suggested "--show-origin" as a better option name over "--sources".
> I still believe "--sources" might be slightly better as I fear that users could
> somehow related "origin" to "remote" kind of configs. However, I am happy to
> change that if a majority prefers "--show-origin".

<bikeshed>
As I have said before, I'm not very good at naming things, but ...

Of the two, I *slightly* prefer --show-origin, since I don't think
there will be any confusion. However, I think --source may be OK too
(for some reason it sounds better than the plural). Another idea
may be --show-source. ;-)

</bikeshed>

ATB,
Ramsay Jones

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

[Jeff King]

On Wed, Feb 10, 2016 at 11:13:18AM +0100, larsxschneider@gmail.com wrote:

> diff to v1:
> 
> * add documention
> * produce a consistent (tab-delimited) format that can be parsed
> * adhere declaration-after-statement style
> * prefix every source line with the source type (file, stdin, blob, cmd)
> * add relative path test case
> * add blob ref test case
> * add "git config --local" test case (Note: I think I found a bug there that I
>   plan to fix/investigate in a seperate patch. Is it ok to leave the TODO?)
> * add a test case to check funny character escapes (file name with tabs)
> 
> Sebastian suggested "--show-origin" as a better option name over "--sources".
> I still believe "--sources" might be slightly better as I fear that users could
> somehow related "origin" to "remote" kind of configs. However, I am happy to
> change that if a majority prefers "--show-origin".

Thanks, this is getting closer, but I still have a few comments.

> @@ -194,6 +194,11 @@ See also <<FILES>>.
>  	Output only the names of config variables for `--list` or
>  	`--get-regexp`.
> 
> +--sources::
> +	Augment the output of all queried config options with the
> +	source type (file, stdin, blob, cmd) and the actual source
> +	(config file path, ref, or blob id if applicable).

I think something like "cmdline" might be more descriptive than "cmd".
Technically such options could also come from the environment (as "-c"
is really just a shorthand for modifying the environment), but I don't
think we actually advertise that.

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

A minor nit, but I think our discussion has shown that this function
does not strictly return filenames. We might just want s/filename/name/.

But moreover...

> +	if (end_null)
> +		term = '\0';
> +
> +	if (fn) {
> +		if (given_config_source.use_stdin)
> +			prefix = "stdin";
> +		else if (given_config_source.blob)
> +			prefix = "blob";
> +		else
> +			prefix = "file";
> +	} else {
> +		fn = "";
> +		prefix = "cmd";
> +	}

I don't think this is quite right. "fn" represents the current file we
happen to be parsing, but given_config_source is where we _started_. So
here's a fairly pathological example that shows the distinction:

  echo "[include]path=/home/peff/.gitconfig" |
  git config --sources --includes --file - user.name

which produces:

  stdin   /home/peff/.gitconfig   Jeff King

So I think we really need to record this source information in the
config_source of config.c and feed it back via current_config_filename(),
whose name grows even more inaccurate. :)

> +	if (fp)
> +		fprintf(fp, "%s", prefix);
> +	else {
> +		strbuf_addstr(buf, prefix);
> +	}
> +
> +	if (fp)
> +		fputc(term, fp);
> +	else
> +		strbuf_addch(buf, term);

So the format here is like:

  file\t<filename>\t<value...>
  blob\t<blob>\t<value...>
  stdin\t\t<value...>
  cmd\t\t<value...>

where two of the prefixes have nothing in the second slot. I expected
something more like:

  file:<filename>\t</value...>
  blob:<blob>\t<value...>
  stdin\t<value...>
  cmd\t<value...>

with a single delimited slot for the source, which can then be broken
down further if desired.  I can't think of any reason to prefer one over
the other rather than personal preference, though. They can both be
parsed unambiguously.

> +
> +	is_query_action = actions & (
> +		ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST
> +	);
> +
> +	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);
> +	}

This means that "git config --sources --get-colorbool" will tell the
user that --sources is only applicable to --get-* actions, which is what
they said. I wonder if we should simply enumerate the cases where it
works.

I also wonder if "is_query_action" is worth the separate variable, since
it is no longer quite accurate (it is really "query actions that support
--sources", and I would not want somebody coming along later to treat it
as "any query action").

> @@ -1078,7 +1078,7 @@ static int do_config_from_file(config_fn_t fn,
> 
>  static int git_config_from_stdin(config_fn_t fn, void *data)
>  {
> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
> +	return do_config_from_file(fn, "", NULL, stdin, data);
>  }

I think this is a regression for the other uses of the name "name"
field. Before your patch:

  $ echo '[broken' | git config --list --file -
  fatal: bad config file line 1 in <stdin>

And after it just says:

  fatal: bad config file line 1 in 

This should be fixed for free along with the source stuff I mentioned
above, though.

> +	# TODO:
> +	# The locally included config value is not printed below. I think
> +	# this is a bug.
> +	# "file	.git/../include/relative.include	user.relative=include"
> +	#
> +	cat >expect <<-EOF &&
> +		file	.git/config	include.path=../include/relative.include
> +		file	.git/config	user.local=true
> +		file	.git/config	user.override=local
> +		EOF
> +	git config --local --list --sources >output &&
> +	test_cmp expect output &&

This is behaving as expected. By default "--include" is off when reading
a specific config file (triggered by your "--local"), and on when
generically reading all config.

> +	cat >expect <<-EOF &&
> +		file	$HOME/.gitconfig	user.global true
> +		file	.git/config	user.local true
> +	EOF
> +	git config --sources --get-regexp "user\.[g|l].*" >output &&
> +	test_cmp expect output &&
> +
> +	cat >expect <<-EOF &&
> +		file	.git/config	local
> +	EOF
> +	git config --sources user.override >output &&
> +	test_cmp expect output &&

There are quite a lot of separate things tested here all in a single
test_expect_success. If you break each of these out into their own
test_expect_success blocks, it is much easier for somebody to diagnose a
failing test. They immediately know _which_ block failed, and if you
have written a description, then they know what the test was trying to
do.

> +	cat >expect <<-EOF &&
> +		file	"file\\\twith\\\ttabs.conf"	user.custom=true
> +	EOF

If you escape the here-doc marker, it turns off interpolation, and you
can avoid the extra layer of backslash quoting. Like:

        cat >expect <<-\EOF
		file	"file\twith\ttabs.conf"	user.custom = true
	EOF

In fact, we generally prefer to use non-interpolated here-docs unless
you actually intend to expand anything. It's one less thing for a reader
of the code to have to worry about.

-Peff

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

[Sebastian Schuberth]

On Wed, Feb 10, 2016 at 1:47 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:

>> Sebastian suggested "--show-origin" as a better option name over "--sources".
>> I still believe "--sources" might be slightly better as I fear that users could
>> somehow related "origin" to "remote" kind of configs. However, I am happy to
>> change that if a majority prefers "--show-origin".
>
> <bikeshed>
> As I have said before, I'm not very good at naming things, but ...
>
> Of the two, I *slightly* prefer --show-origin, since I don't think
> there will be any confusion. However, I think --source may be OK too
> (for some reason it sounds better than the plural). Another idea
> may be --show-source. ;-)
>
> </bikeshed>

I agree that using --source sounds better than --sources, as the
latter sounds even more like "source code".

Here's another idea: How about --declaration or --show-declaration?

-- 
Sebastian Schuberth

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

[Sebastian Schuberth]

On Wed, Feb 10, 2016 at 1:54 PM, Jeff King <peff@peff.net> wrote:

>> +--sources::
>> +     Augment the output of all queried config options with the
>> +     source type (file, stdin, blob, cmd) and the actual source
>> +     (config file path, ref, or blob id if applicable).
>
> I think something like "cmdline" might be more descriptive than "cmd".

I agree "cmdline" is better than "cmd".

> So the format here is like:
>
>   file\t<filename>\t<value...>
>   blob\t<blob>\t<value...>
>   stdin\t\t<value...>
>   cmd\t\t<value...>
>
> where two of the prefixes have nothing in the second slot. I expected
> something more like:
>
>   file:<filename>\t</value...>
>   blob:<blob>\t<value...>
>   stdin\t<value...>
>   cmd\t<value...>
>
> with a single delimited slot for the source, which can then be broken
> down further if desired.  I can't think of any reason to prefer one over
> the other rather than personal preference, though. They can both be
> parsed unambiguously.

I also would have expected sopme like the latter, except that I'd also
expect a colon after "stdin" and "cmd" (or "cmdline", as said above).
I.e. the colon should be part of the prefix to mark it as such.

-- 
Sebastian Schuberth

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

[Jeff King]

On Wed, Feb 10, 2016 at 04:33:11PM +0100, Sebastian Schuberth wrote:

> > where two of the prefixes have nothing in the second slot. I expected
> > something more like:
> >
> >   file:<filename>\t</value...>
> >   blob:<blob>\t<value...>
> >   stdin\t<value...>
> >   cmd\t<value...>
> >
> > with a single delimited slot for the source, which can then be broken
> > down further if desired.  I can't think of any reason to prefer one over
> > the other rather than personal preference, though. They can both be
> > parsed unambiguously.
> 
> I also would have expected sopme like the latter, except that I'd also
> expect a colon after "stdin" and "cmd" (or "cmdline", as said above).
> I.e. the colon should be part of the prefix to mark it as such.

Yeah, I waffled on that. Having a colon means you can definitely parse
to the first ":" without looking at what the prefix is. But if you don't
know what the prefix is, I don't know what good that does you. IOW, I'd
expect it to be parsed like:

  if (/^file:(.*)/) {
    # source is file $1
  } elsif (/^blob:(.*)/) {
    # source is blob $1
  } elsif (/^stdin/) {
    # source is stdin
  } elsif (/^cmdline/) {
    # source is cmdline
  } else {
    die "eh? I don't know about $_ at all!"
  }

That's perl, but I think most languages make prefix-parsing like that
easy. I dunno. I doubt it matters all that much, and we are deep into
personal preference. There's already plenty to bikeshed on the option
name :)

-Peff

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

[Sebastian Schuberth]

On Wed, Feb 10, 2016 at 4:40 PM, Jeff King <peff@peff.net> wrote:

>> >   file:<filename>\t</value...>
>> >   blob:<blob>\t<value...>
>> >   stdin\t<value...>
>> >   cmd\t<value...>
>> >
>> > with a single delimited slot for the source, which can then be broken
>> > down further if desired.  I can't think of any reason to prefer one over
>> > the other rather than personal preference, though. They can both be
>> > parsed unambiguously.
>>
>> I also would have expected sopme like the latter, except that I'd also
>> expect a colon after "stdin" and "cmd" (or "cmdline", as said above).
>> I.e. the colon should be part of the prefix to mark it as such.
>
> Yeah, I waffled on that. Having a colon means you can definitely parse
> to the first ":" without looking at what the prefix is. But if you don't
> know what the prefix is, I don't know what good that does you. IOW, I'd

IMO that's asking the wrong question. The question should not be "what
good does it do if we add the colons also there", but "what
justification do we have to introduce an inconsistency by not adding
them".

> That's perl, but I think most languages make prefix-parsing like that
> easy. I dunno. I doubt it matters all that much, and we are deep into
> personal preference. There's already plenty to bikeshed on the option
> name :)

I agree the option wording mostly is personal preference. On the other
hand, I find discussions like these often prematurely waved aside as
bikeshedding, just because e.g. Perl can parse the one as good as the
other. But it's not about Perl, it's about humans. IMO Git has
historically made many mistakes by not caring enough about consistency
in docs, command and command line option naming, so I don't see it as
wasted time to discuss about things like this.

-- 
Sebastian Schuberth

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

[Ramsay Jones]

On 10/02/16 15:28, Sebastian Schuberth wrote:
> On Wed, Feb 10, 2016 at 1:47 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
> 
>>> Sebastian suggested "--show-origin" as a better option name over "--sources".
>>> I still believe "--sources" might be slightly better as I fear that users could
>>> somehow related "origin" to "remote" kind of configs. However, I am happy to
>>> change that if a majority prefers "--show-origin".
>>
>> <bikeshed>
>> As I have said before, I'm not very good at naming things, but ...
>>
>> Of the two, I *slightly* prefer --show-origin, since I don't think
>> there will be any confusion. However, I think --source may be OK too
>> (for some reason it sounds better than the plural). Another idea
>> may be --show-source. ;-)
>>
>> </bikeshed>
> 
> I agree that using --source sounds better than --sources, as the
> latter sounds even more like "source code".
> 
> Here's another idea: How about --declaration or --show-declaration?
> 

Hmm, I think its more like a definition! :-D

[Sorry, I just couldn't resist. I promise not
to say any more on this. ;-) ]

ATB,
Ramsay Jones

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

[Jeff King]

On Wed, Feb 10, 2016 at 04:57:00PM +0100, Sebastian Schuberth wrote:

> >> I also would have expected sopme like the latter, except that I'd also
> >> expect a colon after "stdin" and "cmd" (or "cmdline", as said above).
> >> I.e. the colon should be part of the prefix to mark it as such.
> >
> > Yeah, I waffled on that. Having a colon means you can definitely parse
> > to the first ":" without looking at what the prefix is. But if you don't
> > know what the prefix is, I don't know what good that does you. IOW, I'd
> 
> IMO that's asking the wrong question. The question should not be "what
> good does it do if we add the colons also there", but "what
> justification do we have to introduce an inconsistency by not adding
> them".

But I think it is only an inconsistency if your mental model of the
format is "the source field is a prefix, a colon, and then an argument;
sometimes we omit the colon if there is no argument". If your mental
model is "the source field contains a type, followed by type-specific
data", then it is not an inconsistency. It is simply context-sensitive.

Now you can argue that context-sensitive things are bad, and I might
agree with that. :)

> I agree the option wording mostly is personal preference. On the other
> hand, I find discussions like these often prematurely waved aside as
> bikeshedding, just because e.g. Perl can parse the one as good as the
> other. But it's not about Perl, it's about humans. IMO Git has
> historically made many mistakes by not caring enough about consistency
> in docs, command and command line option naming, so I don't see it as
> wasted time to discuss about things like this.

Oh, I agree that formats are worth discussing. It's just that I do not
think either of the mental models above is better than the other.

My point in mentioning perl is not that perl can do it, but rather the
opposite: that _any_ language can do it. And I would even go so far as
to say that humans parse context-sensitive things better than machines,
because we simultaneously apply the semantic and syntactic parsing (so
you see "stdin" and know because of its meaning that there is nothing
left to parse; you don't need a trailing colon to tell you that). Or
perhaps you mean "humans write the programs that machines use to parse,
and we must make things regular and easy for them". I can somewhat buy
that argument, though again, I think either is easy; it is mostly a
matter of documenting the format's mental model.

-Peff

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

[Eric Sunshine]

On Wed, Feb 10, 2016 at 11:13:18AM +0100, larsxschneider@gmail.com wrote:
> 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>
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -91,8 +94,58 @@ static void check_argc(int argc, int min, int max) {
> +/* output to either fp or buf; only one should be non-NULL */
> +static void show_config_source(struct strbuf *buf, FILE *fp)
> +{
> +	char term = '\t';
> +	char *prefix;
> +	const char *fn = current_config_filename();
> +
> +	if (end_null)
> +		term = '\0';
> +
> +	if (fn) {
> +		if (given_config_source.use_stdin)
> +			prefix = "stdin";
> +		else if (given_config_source.blob)
> +			prefix = "blob";
> +		else
> +			prefix = "file";
> +	} else {
> +		fn = "";
> +		prefix = "cmd";
> +	}
> +
> +	if (fp)
> +		fprintf(fp, "%s", prefix);
> +	else {
> +		strbuf_addstr(buf, prefix);
> +	}

Style: drop unnecessary braces

> +
> +	if (fp)
> +		fputc(term, fp);
> +	else
> +		strbuf_addch(buf, term);

Why not combine this 'if' with the preceding one?

> +
> +	if (!end_null)
> +		quote_c_style(fn, buf, fp, 0);
> +	else {
> +		if (fp)
> +			fprintf(fp, "%s", fn);
> +		else
> +			strbuf_addstr(buf, fn);
> +	}
> +
> +	if (fp)
> +		fputc(term, fp);
> +	else
> +		strbuf_addch(buf, term);
> +}

Overall, due to its duality (outputting to FILE* or strbuf), this
function is more difficult to read than it probably ought to be. Have
you considered simplifying it by making it single-purpose. Something
like this, for instance:

    static void get_config_source(const char **name, const char **prefix)
    {
        *name = current_config_filename();
        if (!*name) {
            *name = "";
            *prefix = "cmd";
        } else if (given_config_source.use_stdin)
            *prefix = "stdin";
        else if (given_config_source.blob)
            *prefix = "blob";
        else
            *prefix = "file";
    }

    static void show_config_source(struct strbuf *buf)
    {
        char term = end_null ? '\0' : '\t';
        char *prefix;
        const char *name;

        get_config_source(&name, &prefix);
        strbuf_addstr(buf, prefix);
        strbuf_addch(buf, term);
        if (end_null)
            strbuf_addstr(buf, name);
        else
            quote_c_style(name, buf, NULL, 0);
        strbuf_addch(buf, term);
    }

    static int show_all_config(...)
    {
        if (show_sources) {
            struct strbuf buf = STRBUF_INIT;
            show_config_source(&buf);
            fputs(buf.buf, stdout);
            strbuf_release(&buf);
        }
        ...
    }

    static int format_config(...)
    {
        if (show_sources)
            show_config_source(buf);
        ...
    }