[PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v2:
* rename cmd to cmdline
* rename function current_config_name to current_config_type_name
* add 'type' to config_source struct that identifies config type
* fix config stdin error output and add test case "invalid stdin config"
* change delimiter between type and name from tab to colon
* remove is_query_action variable
* rename "--sources" to "--show-origin"
* add pathological test case "--show-origin stdin with file include"
* enumerate all valid commandline cases for "--show-origin"
* removed TODOs as there are no config include bugs
* describe '--includes' default behavior in git-config.txt
* split test cases
* use non-interpolated here-docs where possible
* improve readablity of 'show_config_origin' function by removing duality

I renamed the flag from "--source" to "--show-origin" as I got the impression
that Sebastian, Peff and Ramsay seem to be all OK with "--show-origin".

Thanks Eric for the hint to simplify the 'show_config_origin' function.
I took your suggestion but modfied one line. Instead of "fputs" I used
"fwrite" to write the content. This was necssary as the last char of the
string could be \0 due to the "--null" flag. "fputs" would ignore that.

In 959b545 Heiko introduced a config API to lookup .gitmodules values and
in "submodule-config.c" he uses the "git_config_from_buf" function [1]. I
wonder if my modifications to this function could trigger any unwanted side
effects in his implementation? (I can't imagine any but I want to make you
aware of this connection)


Thanks a lot Peff, Sebastian, Ramsey, and Eric for the review.


[1] https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/submodule-config.c#L430-L431


Lars Schneider (3):
  git-config.txt: describe '--includes' default behavior
  config: add 'type' to config_source struct that identifies config type
  config: add '--show-origin' option to print the origin of a config
    value

 Documentation/git-config.txt |  19 ++++--
 builtin/config.c             |  35 +++++++++++
 cache.h                      |   1 +
 config.c                     |  23 +++++---
 t/t1300-repo-config.sh       | 136 ++++++++++++++++++++++++++++++++++++++++++-
 t/t1308-config-set.sh        |   4 +-
 6 files changed, 202 insertions(+), 16 deletions(-)

--
2.5.1

[PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/git-config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2608ca7..59b1c95 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -219,7 +219,9 @@ See also <<FILES>>.
 
 --[no-]includes::
 	Respect `include.*` directives in config files when looking up
-	values. Defaults to on.
+	values. Defaults to off when reading a specific config file
+	(e.g. via `--file` or via `--local` etc.), and to on when
+	generically reading all config.
 
 [[FILES]]
 FILES
-- 
2.5.1

[PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Use the config type to print more detailed error messages that inform
the user about the origin of a config error (file, stdin, blob).

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 config.c               | 17 ++++++++++-------
 t/t1300-repo-config.sh |  8 +++++++-
 t/t1308-config-set.sh  |  4 ++--
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/config.c b/config.c
index 86a5eb2..6a5942f 100644
--- a/config.c
+++ b/config.c
@@ -24,6 +24,7 @@ struct config_source {
 			size_t pos;
 		} buf;
 	} u;
+	const char *type;
 	const char *name;
 	const char *path;
 	int die_on_error;
@@ -471,9 +472,9 @@ static int git_parse_source(config_fn_t fn, void *data)
 			break;
 	}
 	if (cf->die_on_error)
-		die(_("bad config file line %d in %s"), cf->linenr, cf->name);
+		die(_("bad config line %d in %s %s"), cf->linenr, cf->type, cf->name);
 	else
-		return error(_("bad config file line %d in %s"), cf->linenr, cf->name);
+		return error(_("bad config line %d in %s %s"), cf->linenr, cf->type, cf->name);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -588,9 +589,9 @@ static void die_bad_number(const char *name, const char *value)
 	if (!value)
 		value = "";
 
-	if (cf && cf->name)
-		die(_("bad numeric config value '%s' for '%s' in %s: %s"),
-		    value, name, cf->name, reason);
+	if (cf && cf->type && cf->name)
+		die(_("bad numeric config value '%s' for '%s' in %s %s: %s"),
+		    value, name, cf->type, cf->name, reason);
 	die(_("bad numeric config value '%s' for '%s': %s"), value, name, reason);
 }
 
@@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
 	struct config_source top;
 
 	top.u.file = f;
-	top.name = name;
+	top.type = path ? "file" : "stdin";
+	top.name = name ? name : "";
 	top.path = path;
 	top.die_on_error = 1;
 	top.do_fgetc = config_file_fgetc;
@@ -1078,7 +1080,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, NULL, stdin, data);
 }
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
@@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
 	top.u.buf.buf = buf;
 	top.u.buf.len = len;
 	top.u.buf.pos = 0;
+	top.type = "blob";
 	top.name = name;
 	top.path = NULL;
 	top.die_on_error = 0;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 52678e7..7abdfcb 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -700,12 +700,18 @@ test_expect_success 'invalid unit' '
 	git config aninvalid.unit >actual &&
 	test_cmp expect actual &&
 	cat >expect <<-\EOF &&
-	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in .git/config: invalid unit
+	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
 	EOF
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'invalid stdin config' '
+	echo "fatal: bad config line 1 in stdin " >expect &&
+	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
+	test_cmp expect output
+'
+
 cat > expect << EOF
 true
 false
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 91235b7..82f82a1 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -195,14 +195,14 @@ test_expect_success 'proper error on error in default config files' '
 	cp .git/config .git/config.old &&
 	test_when_finished "mv .git/config.old .git/config" &&
 	echo "[" >>.git/config &&
-	echo "fatal: bad config file line 34 in .git/config" >expect &&
+	echo "fatal: bad config line 34 in file .git/config" >expect &&
 	test_expect_code 128 test-config get_value foo.bar 2>actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'proper error on error in custom config files' '
 	echo "[" >>syntax-error &&
-	echo "fatal: bad config file line 1 in syntax-error" >expect &&
+	echo "fatal: bad config line 1 in file syntax-error" >expect &&
 	test_expect_code 128 test-config configset_get_value foo.bar syntax-error 2>actual &&
 	test_cmp expect actual
 '
-- 
2.5.1

[PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

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

Teach 'git config' the '--show-origin' 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>
---
 Documentation/git-config.txt |  15 +++--
 builtin/config.c             |  35 ++++++++++++
 cache.h                      |   1 +
 config.c                     |   6 ++
 t/t1300-repo-config.sh       | 128 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 59b1c95..2bf4d4e 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] [--show-origin] [-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] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [type] [--show-origin] [-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>] [--show-origin] [-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`.
 
+--show-origin::
+	Augment the output of all queried config options with the
+	origin type (file, stdin, blob, cmdline) and the actual origin
+	(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..302b265 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_origin;
 
 #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, "show-origin", &show_origin, N_("show origin of config (file, stdin, blob, cmdline)")),
 	OPT_END(),
 };
 
@@ -91,8 +94,30 @@ static void check_argc(int argc, int min, int max) {
 	usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
+static void show_config_origin(struct strbuf *buf)
+{
+	const char term = end_null ? '\0' : '\t';
+	const char *type;
+	const char *name;
+
+	current_config_type_name(&type, &name);
+	strbuf_addstr(buf, type);
+	strbuf_addch(buf, ':');
+	if (end_null)
+		strbuf_addstr(buf, name);
+	else
+		quote_c_style(name, buf, NULL, 0);
+	strbuf_addch(buf, term);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+	if (show_origin) {
+       struct strbuf buf = STRBUF_INIT;
+       show_config_origin(&buf);
+       fwrite(buf.buf, 1, buf.len, stdout);
+       strbuf_release(&buf);
+	}
 	if (!omit_values && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
@@ -108,6 +133,8 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char *value_)
 {
+	if (show_origin)
+		show_config_origin(buf);
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
@@ -538,6 +565,14 @@ 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);
 	}
+
+	if (show_origin && !(actions &
+		(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) {
+		error("--show-origin is only applicable to --get, --get-all, "
+			  "--get-regexp, and --list.");
+		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..3d1eb86 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 void current_config_type_name(const char **type, const char **name);
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index 6a5942f..0603d4b 100644
--- a/config.c
+++ b/config.c
@@ -2388,3 +2388,9 @@ int parse_config_key(const char *var,
 
 	return 0;
 }
+
+void current_config_type_name(const char **type, const char **name)
+{
+	*type = cf && cf->type ? cf->type : "cmdline";
+	*name = cf && cf->name ? cf->name : "";
+}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7abdfcb..112f7c8 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1207,4 +1207,132 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success '--show-origin' '
+	>.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
+	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
+		cmdline:	user.cmdline=true
+	EOF
+	git -c user.cmdline=true config --list --show-origin >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file:$HOME/.gitconfigQuser.global
+		trueQfile:$HOME/.gitconfigQuser.override
+		globalQfile:$HOME/.gitconfigQinclude.path
+		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
+		includeQfile:.git/configQinclude.path
+		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
+		includeQfile:.git/configQuser.local
+		trueQfile:.git/configQuser.override
+		localQcmdline:Quser.cmdline
+		trueQ
+	EOF
+	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
+	echo >>output &&
+	test_cmp expect output &&
+
+	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 --show-origin >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file:$HOME/.gitconfig	user.global true
+		file:.git/config	user.local true
+	EOF
+	git config --show-origin --get-regexp "user\.[g|l].*" >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-\EOF &&
+		file:.git/config	local
+	EOF
+	git config --show-origin user.override >output &&
+	test_cmp expect output
+'
+
+CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
+cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
+	[user]
+		custom = true
+EOF
+
+test_expect_success '--show-origin escape special file name characters' '
+	cat >expect <<-\EOF &&
+		file:"file\\twith\\ttabs.conf"	user.custom=true
+	EOF
+	git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin stdin' '
+	cat >expect <<-\EOF &&
+		stdin:	user.custom=true
+	EOF
+	git config --file - --show-origin --list <"$CUSTOM_CONFIG_FILE" >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin stdin with file include' '
+	INCLUDE_DIR="$HOME/include" &&
+	mkdir -p "$INCLUDE_DIR" &&
+	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
+		[user]
+			stdin = include
+	EOF
+	cat >expect <<-EOF &&
+		file:$INCLUDE_DIR/stdin.include	include
+	EOF
+	echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
+		| git config --show-origin --includes --file - user.stdin >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin blob' '
+	cat >expect <<-\EOF &&
+		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
+	EOF
+	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+	git config --blob=$blob --show-origin --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin blob ref' '
+	cat >expect <<-\EOF &&
+		blob:"master:file\\twith\\ttabs.conf"	user.custom=true
+	EOF
+	git add "$CUSTOM_CONFIG_FILE" &&
+	git commit -m "new config file" &&
+	git config --blob=master:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
2.5.1

Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[Mike Rappazzo]

On Sat, Feb 13, 2016 at 9:24 AM,  <larsxschneider@gmail.com> wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> diff to v2:
> * rename cmd to cmdline
> * rename function current_config_name to current_config_type_name
> * add 'type' to config_source struct that identifies config type
> * fix config stdin error output and add test case "invalid stdin config"
> * change delimiter between type and name from tab to colon
> * remove is_query_action variable
> * rename "--sources" to "--show-origin"
> * add pathological test case "--show-origin stdin with file include"
> * enumerate all valid commandline cases for "--show-origin"
> * removed TODOs as there are no config include bugs
> * describe '--includes' default behavior in git-config.txt
> * split test cases
> * use non-interpolated here-docs where possible
> * improve readablity of 'show_config_origin' function by removing duality
>
> I renamed the flag from "--source" to "--show-origin" as I got the impression
> that Sebastian, Peff and Ramsay seem to be all OK with "--show-origin".

I know that I am late to the party here, but why not make the option
`--verbose` or `-v`?  `git config` doesn't have that now, and this
seems like a logical thing to include as verbose data.  I would
venture to guess that `--verbose` is more likely to be the first thing
that someone who is looking for this information will guess at before
they run `git config --help`.

>
> Thanks Eric for the hint to simplify the 'show_config_origin' function.
> I took your suggestion but modfied one line. Instead of "fputs" I used
> "fwrite" to write the content. This was necssary as the last char of the
> string could be \0 due to the "--null" flag. "fputs" would ignore that.
>
> In 959b545 Heiko introduced a config API to lookup .gitmodules values and
> in "submodule-config.c" he uses the "git_config_from_buf" function [1]. I
> wonder if my modifications to this function could trigger any unwanted side
> effects in his implementation? (I can't imagine any but I want to make you
> aware of this connection)
>
>
> Thanks a lot Peff, Sebastian, Ramsey, and Eric for the review.
>
>
> [1] https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/submodule-config.c#L430-L431
>
>
> Lars Schneider (3):
>   git-config.txt: describe '--includes' default behavior
>   config: add 'type' to config_source struct that identifies config type
>   config: add '--show-origin' option to print the origin of a config
>     value
>
>  Documentation/git-config.txt |  19 ++++--
>  builtin/config.c             |  35 +++++++++++
>  cache.h                      |   1 +
>  config.c                     |  23 +++++---
>  t/t1300-repo-config.sh       | 136 ++++++++++++++++++++++++++++++++++++++++++-
>  t/t1308-config-set.sh        |   4 +-
>  6 files changed, 202 insertions(+), 16 deletions(-)
>
> --
> 2.5.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[Jeff King]

On Sat, Feb 13, 2016 at 03:24:14PM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  Documentation/git-config.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 2608ca7..59b1c95 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -219,7 +219,9 @@ See also <<FILES>>.
>  
>  --[no-]includes::
>  	Respect `include.*` directives in config files when looking up
> -	values. Defaults to on.
> +	values. Defaults to off when reading a specific config file
> +	(e.g. via `--file` or via `--local` etc.), and to on when
> +	generically reading all config.

Hmph. I wondered why you were confused about this the other day, when I
checked my Documentation/git-config.txt and found that yes, we do indeed
document this behavior. I did not realize then that my patch from:

  http://article.gmane.org/gmane.comp.version-control.git/262641

was never picked up (but of course I've been carrying it in my tree for
a year). I guess maybe you found it, given the similarity of the
wording. I mildly prefer the wording and formatting of my original, but
I am OK either way. :)

-Peff

Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Jeff King]

On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Use the config type to print more detailed error messages that inform
> the user about the origin of a config error (file, stdin, blob).

This looks OK overall. A few minor nits...

> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>  	top.u.buf.buf = buf;
>  	top.u.buf.len = len;
>  	top.u.buf.pos = 0;
> +	top.type = "blob";
>  	top.name = name;
>  	top.path = NULL;
>  	top.die_on_error = 0;

This function is about reading config from a memory buffer. The only reason
we do so _now_ is when reading from a blob, but I think it is laying a
trap for somebody who wants to reuse the function later.

Should git_config_from_buf() take a "type" parameter, and
git_config_from_blob_sha1() pass in "blob"?

> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
>  	struct config_source top;
>  
>  	top.u.file = f;
> -	top.name = name;
> +	top.type = path ? "file" : "stdin";
> +	top.name = name ? name : "";
>  	top.path = path;
>  	top.die_on_error = 1;
>  	top.do_fgetc = config_file_fgetc;
> @@ -1078,7 +1080,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, NULL, stdin, data);
>  }

Likewise here, we make assumptions in do_config_from_file() about what
the NULL path means. I think this is less likely to be a problem than
the other case, but it seems like it would be cleaner for "file" or
"stdin" to come from the caller, which knows for sure what we are doing.

Similarly, I think git_config_from_stdin() can simply pass in an empty
name rather than NULL to avoid do_config_from_file() having to fix it
up.

> +test_expect_success 'invalid stdin config' '
> +	echo "fatal: bad config line 1 in stdin " >expect &&
> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
> +	test_cmp expect output
> +'

The original would have been "bad config file line 1 in <stdin>"; I
think this is an improvement to drop the "file" here.

-Peff

Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[Sebastian Schuberth]

On Sat, Feb 13, 2016 at 3:43 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:

>> I renamed the flag from "--source" to "--show-origin" as I got the impression
>> that Sebastian, Peff and Ramsay seem to be all OK with "--show-origin".
>
> I know that I am late to the party here, but why not make the option
> `--verbose` or `-v`?  `git config` doesn't have that now, and this
> seems like a logical thing to include as verbose data.  I would

`--verbose` would be fine with me.

-- 
Sebastian Schuberth

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.

Thanks, I think this version fixes the correctness issues I mentioned
earlier. I do still have nits to pick (of course :) ), that we may or
may not want to deal with.

> +static void show_config_origin(struct strbuf *buf)
> +{
> +	const char term = end_null ? '\0' : '\t';
> +	const char *type;
> +	const char *name;
> +
> +	current_config_type_name(&type, &name);

This double out-parameter feels like a clunky interface.

I was tempted to suggest that we simply make the "struct config_source"
available to builtin/config.c (which is already pretty intimate with the
rest of the config code), and then it can pick out what it wants. But
there _is_ some logic in the function to convert the NULL "cf" into
"cmdline".

Perhaps it would be simpler to just have two accessor functions, and do:

  strbuf_addstr(buf, current_config_type());
  ...
  strbuf_addstr(buf, current_config_name());

I admit it is a pretty minor point, though.

>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> +	if (show_origin) {
> +       struct strbuf buf = STRBUF_INIT;
> +       show_config_origin(&buf);
> +       fwrite(buf.buf, 1, buf.len, stdout);
> +       strbuf_release(&buf);
> +	}

The indentation is funky here.

The use of fwrite() to catch the embedded NULs is subtle enough that it
might be worth a comment.

It also made me wonder how format_config() handles this. It looks like
we send the result eventually to fwrite() there, so it all works (and it
does _not_ have the comment I mentioned :) ).

> +test_expect_success '--show-origin' '
> +	>.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
> +	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
> +		cmdline:	user.cmdline=true
> +	EOF
> +	git -c user.cmdline=true config --list --show-origin >output &&
> +	test_cmp expect output &&
> +
> +	cat >expect <<-EOF &&
> +		file:$HOME/.gitconfigQuser.global
> +		trueQfile:$HOME/.gitconfigQuser.override
> +		globalQfile:$HOME/.gitconfigQinclude.path
> +		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
> +		includeQfile:.git/configQinclude.path
> +		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
> +		includeQfile:.git/configQuser.local
> +		trueQfile:.git/configQuser.override
> +		localQcmdline:Quser.cmdline
> +		trueQ
> +	EOF

I see you split this up more, but there's still quite a bit going on in
this one block. IMHO, it would be more customary in our tests to put the
setup into one test_expect_success block, then each of these
expect-run-cmp blocks into their own test_expect_success.

It does mean that the setup mutates the global test state for further
tests (and you should stop using test_config_*, which clean up at the
end of the block), but I think that's the right thing here. The point of
test_config is "flip on this switch just for a moment, so we can test
its effect without hurting further tests". But these are config tests in
the first place, and it is OK for them to show a progression of
mutations of the config (you'll note that like the other tests in this
script, you are clearing out .git/config in the first place).

-Peff

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:

> > +test_expect_success '--show-origin' '
> [...]
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.

Here's a squashable patch that shows what I mean.

---
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 112f7c8..c7496aa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1207,26 +1207,34 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
-test_expect_success '--show-origin' '
-	>.git/config &&
-	>"$HOME"/.gitconfig &&
+test_expect_success 'set up --show-origin tests' '
 	INCLUDE_DIR="$HOME/include" &&
 	mkdir -p "$INCLUDE_DIR" &&
-	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
+	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
 		[user]
 			absolute = include
 	EOF
-	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
+	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
 		[user]
 			relative = include
 	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 >"$HOME"/.gitconfig <<-EOF &&
+		[user]
+			global = true
+			override = global
+		[include]
+			path = "$INCLUDE_DIR/absolute.include"
+	EOF
+	cat >.git/config <<-\EOF
+		[include]
+			path = ../include/relative.include
+		[user]
+			local = true
+			override = local
+	EOF
+'
 
+test_expect_success '--show-origin with --list' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global=true
 		file:$HOME/.gitconfig	user.override=global
@@ -1239,8 +1247,10 @@ test_expect_success '--show-origin' '
 		cmdline:	user.cmdline=true
 	EOF
 	git -c user.cmdline=true config --list --show-origin >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with --list --null' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfigQuser.global
 		trueQfile:$HOME/.gitconfigQuser.override
@@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
 		localQcmdline:Quser.cmdline
 		trueQ
 	EOF
-	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
+	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
+	nul_to_q <output.raw >output &&
 	echo >>output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with single file' '
 	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 --show-origin >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with --get-regexp' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global true
 		file:.git/config	user.local true
 	EOF
 	git config --show-origin --get-regexp "user\.[g|l].*" >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin getting a single key' '
 	cat >expect <<-\EOF &&
 		file:.git/config	local
 	EOF
@@ -1279,11 +1296,13 @@ test_expect_success '--show-origin' '
 	test_cmp expect output
 '
 
-CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
-cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
-	[user]
-		custom = true
-EOF
+test_expect_success 'set up custom config file' '
+	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
+	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+		[user]
+			custom = true
+	EOF
+'
 
 test_expect_success '--show-origin escape special file name characters' '
 	cat >expect <<-\EOF &&
@@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
 '
 
 test_expect_success '--show-origin stdin with file include' '
-	INCLUDE_DIR="$HOME/include" &&
-	mkdir -p "$INCLUDE_DIR" &&
 	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
 		[user]
 			stdin = include

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:

> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
> 
> > > +test_expect_success '--show-origin' '
> > [...]
> > I see you split this up more, but there's still quite a bit going on in
> > this one block. IMHO, it would be more customary in our tests to put the
> > setup into one test_expect_success block, then each of these
> > expect-run-cmp blocks into their own test_expect_success.
> 
> Here's a squashable patch that shows what I mean.

And here are a few comments on the changes...

> -test_expect_success '--show-origin' '
> -	>.git/config &&
> -	>"$HOME"/.gitconfig &&
> +test_expect_success 'set up --show-origin tests' '
>  	INCLUDE_DIR="$HOME/include" &&
>  	mkdir -p "$INCLUDE_DIR" &&
> -	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
> +	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>  		[user]
>  			absolute = include
>  	EOF
> -	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
> +	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>  		[user]
>  			relative = include
>  	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 >"$HOME"/.gitconfig <<-EOF &&
> +		[user]
> +			global = true
> +			override = global
> +		[include]
> +			path = "$INCLUDE_DIR/absolute.include"
> +	EOF
> +	cat >.git/config <<-\EOF
> +		[include]
> +			path = ../include/relative.include
> +		[user]
> +			local = true
> +			override = local
> +	EOF

I preserved your ordering here (as the later "--list" tests care). But
it might be worth ordering both files the same way, so that a reader
does not wonder if it is significant (and just update the --list
output expectation later).

> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>  		localQcmdline:Quser.cmdline
>  		trueQ
>  	EOF
> -	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
> +	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
> +	nul_to_q <output.raw >output &&

We usually try to avoid putting git on the left-hand side of a pipe,
because it hides the exit code, and we want to make sure git does not
fail. I won't be surprised if you copied the style from elsewhere in the
script, though, as this is an old one and we were not always consistent.

>  	echo >>output &&
> -	test_cmp expect output &&
> +	test_cmp expect output

This "echo" might be worth a comment. I think we are just adding the
extra newline that the here-doc insists on, but that --null output would
not include.

Overall, I find the "--show-origin --null" output pretty weird to read.
We use a newline to split the config key/value, a NUL between config
entries, but now also a NUL between the filename and the rest of the
config entry.

That makes parsing pretty weird, as you cannot just use something like

  git config --show-origin --list --null | perl -0ne ...

to process entries; every other entry you get will be a filename. But at
the same time, we do not go all out and say "there is a NUL between each
field", because the key/value separator is a newline in this case, and
the reader has to parse that separately after splitting on NULs.

But I think it's too late to do anything about it now. The weirdness is
really the mixed NUL/newline thing, and you are not introducing that.

> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
> -	[user]
> -		custom = true
> -EOF
> +test_expect_success 'set up custom config file' '
> +	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
> +	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> +		[user]
> +			custom = true
> +	EOF
> +'

Everything, even mundane setup, should generally go in a test_expect
block. That means we'll notice unexpected failures, and any output will
follow the usual "--verbose" rules.

Arguably this setup could just go into the initial setup block.

Also, you may not that the filename does _not_ actually have tabs in it,
because the shell does not expand "\t". It does have backslashes in it,
though, which is enough to trigger our C-style quoting.

So the test isn't wrong, but the filename is misleading. If you really
want tabs, you'd have to do:

  CUSTOM_CONFIG_FILE="$(printf "file\twith\ttabs.conf")

or similar.

>  test_expect_success '--show-origin escape special file name characters' '
>  	cat >expect <<-\EOF &&
> @@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
>  '
>  
>  test_expect_success '--show-origin stdin with file include' '
> -	INCLUDE_DIR="$HOME/include" &&
> -	mkdir -p "$INCLUDE_DIR" &&
>  	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
>  		[user]
>  			stdin = include

Here we can assume that the setup block succeeded (if it didn't, all of
the tests are screwed anyway, so you'd want to fix that first).

-Peff

Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[Jeff King]

On Sat, Feb 13, 2016 at 09:43:34AM -0500, Mike Rappazzo wrote:

> > I renamed the flag from "--source" to "--show-origin" as I got the impression
> > that Sebastian, Peff and Ramsay seem to be all OK with "--show-origin".
> 
> I know that I am late to the party here, but why not make the option
> `--verbose` or `-v`?  `git config` doesn't have that now, and this
> seems like a logical thing to include as verbose data.  I would
> venture to guess that `--verbose` is more likely to be the first thing
> that someone who is looking for this information will guess at before
> they run `git config --help`.

I'm actually opposed to "--verbose" because it is too vague. That is
fine for a human, but this is also an interface that we expect scripts
to parse, and which we therefore cannot later change without breaking
them. I am not prepared to set "--verbose" in stone as showing the
source file and nothing else, for all time.

We _could_ have "--show-origin", and then "--verbose" implies
"--show-origin", and we don't promise that it might not show more later.
But I don't think that is worth the added complexity (not in code, but
just in what we have to explain to the user).

-Peff

Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... I did not realize then that my patch from:
>
>   http://article.gmane.org/gmane.comp.version-control.git/262641
>
> was never picked up (but of course I've been carrying it in my tree for
> a year).

Hmm, I do not see how it happened, either.  It is not like I was
offline (I can see I was involved in other threads from the same
timeframe).  Perhaps it was lost in the noise.

Thanks for following up.

Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>>  	top.u.buf.buf = buf;
>>  	top.u.buf.len = len;
>>  	top.u.buf.pos = 0;
>> +	top.type = "blob";
>>  	top.name = name;
>>  	top.path = NULL;
>>  	top.die_on_error = 0;
>
> This function is about reading config from a memory buffer. The only reason
> we do so _now_ is when reading from a blob, but I think it is laying a
> trap for somebody who wants to reuse the function later.
>
> Should git_config_from_buf() take a "type" parameter, and
> git_config_from_blob_sha1() pass in "blob"?

I think that is sensible.  I think s/from_buf/from_mem/ may also be
sensible (it would match the naming used in the index_{fd,mem,...}
functions in he hashing code).

>>  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, NULL, stdin, data);
>>  }
>
> Likewise here, we make assumptions in do_config_from_file() about what
> the NULL path means. I think this is less likely to be a problem than
> the other case, but it seems like it would be cleaner for "file" or
> "stdin" to come from the caller, which knows for sure what we are doing.

Makes sense.

> Similarly, I think git_config_from_stdin() can simply pass in an empty
> name rather than NULL to avoid do_config_from_file() having to fix it
> up.

This too.

>> +test_expect_success 'invalid stdin config' '
>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> +	test_cmp expect output
>> +'
>
> The original would have been "bad config file line 1 in <stdin>"; I
> think this is an improvement to drop the "file" here.
>
> -Peff

Re: [PATCH v3 0/3] config: add '--sources' option to print the source of a config value

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm actually opposed to "--verbose" because it is too vague. That is
> fine for a human, but this is also an interface that we expect scripts
> to parse, and which we therefore cannot later change without breaking
> them. I am not prepared to set "--verbose" in stone as showing the
> source file and nothing else, for all time.

Me neither.  Thanks for clearly spelling out why using --verbose for
this is not such a good idea.

Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[Jeff King]

On Sat, Feb 13, 2016 at 01:00:22PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... I did not realize then that my patch from:
> >
> >   http://article.gmane.org/gmane.comp.version-control.git/262641
> >
> > was never picked up (but of course I've been carrying it in my tree for
> > a year).
> 
> Hmm, I do not see how it happened, either.  It is not like I was
> offline (I can see I was involved in other threads from the same
> timeframe).  Perhaps it was lost in the noise.
> 
> Thanks for following up.

>From even my limited time as acting maintainer, I know it is easy for
things to occasionally fall through the cracks for no real reason. We
rely on submitters to be invested enough in their patch series to give a
gentle prod in such cases. However, I sometimes churn out little patches
like this at such a rate that _I_ forget about them, too. :)

I do keep them in my tree, but I have so many half-finished and failed
experiments that they get lost in the noise. I should do a better job of
periodically reviewing and reposting or discarding my personal topics (I
have 65 right now!).

-Peff

Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[Lars Schneider]

On 13 Feb 2016, at 18:17, Jeff King <peff@peff.net> wrote:

> On Sat, Feb 13, 2016 at 03:24:14PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/git-config.txt | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>> index 2608ca7..59b1c95 100644
>> --- a/Documentation/git-config.txt
>> +++ b/Documentation/git-config.txt
>> @@ -219,7 +219,9 @@ See also <<FILES>>.
>> 
>> --[no-]includes::
>> 	Respect `include.*` directives in config files when looking up
>> -	values. Defaults to on.
>> +	values. Defaults to off when reading a specific config file
>> +	(e.g. via `--file` or via `--local` etc.), and to on when
>> +	generically reading all config.
> 
> Hmph. I wondered why you were confused about this the other day, when I
> checked my Documentation/git-config.txt and found that yes, we do indeed
> document this behavior. I did not realize then that my patch from:
> 
>  http://article.gmane.org/gmane.comp.version-control.git/262641
> 
> was never picked up (but of course I've been carrying it in my tree for
> a year). I guess maybe you found it, given the similarity of the
> wording. I mildly prefer the wording and formatting of my original, but
> I am OK either way. :)

Oh. Believe it or not but the similarity is coincidental. I referenced you 
("Helped-by") because you explained the expected "includes" behavior to
me in your v2 review of my series. If I would have found your patch, I 
would have said so.

I am happy to use your wording. How should I proceed? Should I just drop 
my "git-config.txt" patch from my series or should I integrate your patch 
into my series? If the latter, then does the patch require a "Signed-off-by:"
by me?

Thanks,
Lars

Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Lars Schneider]

On 13 Feb 2016, at 18:24, Jeff King <peff@peff.net> wrote:

> On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Use the config type to print more detailed error messages that inform
>> the user about the origin of a config error (file, stdin, blob).
> 
> This looks OK overall. A few minor nits...
> 
>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>> 	top.u.buf.buf = buf;
>> 	top.u.buf.len = len;
>> 	top.u.buf.pos = 0;
>> +	top.type = "blob";
>> 	top.name = name;
>> 	top.path = NULL;
>> 	top.die_on_error = 0;
> 
> This function is about reading config from a memory buffer. The only reason
> we do so _now_ is when reading from a blob, but I think it is laying a
> trap for somebody who wants to reuse the function later.
> 
> Should git_config_from_buf() take a "type" parameter, and
> git_config_from_blob_sha1() pass in "blob"?
Haha, fun fact: this was how I implemented it initially. Because of that
I noticed that "submodule-config.c" also uses "git_config_from_buf" :-)

However, then I thought the list wouldn't like it if I change the
interfaces. I will add the type parameter, again.


> 
>> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
>> 	struct config_source top;
>> 
>> 	top.u.file = f;
>> -	top.name = name;
>> +	top.type = path ? "file" : "stdin";
>> +	top.name = name ? name : "";
>> 	top.path = path;
>> 	top.die_on_error = 1;
>> 	top.do_fgetc = config_file_fgetc;
>> @@ -1078,7 +1080,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, NULL, stdin, data);
>> }
> 
> Likewise here, we make assumptions in do_config_from_file() about what
> the NULL path means. I think this is less likely to be a problem than
> the other case, but it seems like it would be cleaner for "file" or
> "stdin" to come from the caller, which knows for sure what we are doing.
> 
> Similarly, I think git_config_from_stdin() can simply pass in an empty
> name rather than NULL to avoid do_config_from_file() having to fix it
> up.
OK


> 
>> +test_expect_success 'invalid stdin config' '
>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> +	test_cmp expect output
>> +'
> 
> The original would have been "bad config file line 1 in <stdin>"; I
> think this is an improvement to drop the "file" here.


Thanks,
Lars

Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Lars Schneider]

On 13 Feb 2016, at 22:04, Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
> 
>>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>>> 	top.u.buf.buf = buf;
>>> 	top.u.buf.len = len;
>>> 	top.u.buf.pos = 0;
>>> +	top.type = "blob";
>>> 	top.name = name;
>>> 	top.path = NULL;
>>> 	top.die_on_error = 0;
>> 
>> This function is about reading config from a memory buffer. The only reason
>> we do so _now_ is when reading from a blob, but I think it is laying a
>> trap for somebody who wants to reuse the function later.
>> 
>> Should git_config_from_buf() take a "type" parameter, and
>> git_config_from_blob_sha1() pass in "blob"?
> 
> I think that is sensible.  I think s/from_buf/from_mem/ may also be
> sensible (it would match the naming used in the index_{fd,mem,...}
> functions in he hashing code).
OK, I will change that, too!


> 
>>> 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, NULL, stdin, data);
>>> }
>> 
>> Likewise here, we make assumptions in do_config_from_file() about what
>> the NULL path means. I think this is less likely to be a problem than
>> the other case, but it seems like it would be cleaner for "file" or
>> "stdin" to come from the caller, which knows for sure what we are doing.
> 
> Makes sense.
> 
>> Similarly, I think git_config_from_stdin() can simply pass in an empty
>> name rather than NULL to avoid do_config_from_file() having to fix it
>> up.
> 
> This too.
> 
>>> +test_expect_success 'invalid stdin config' '
>>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>>> +	test_cmp expect output
>>> +'
>> 
>> The original would have been "bad config file line 1 in <stdin>"; I
>> think this is an improvement to drop the "file" here.
>> 
>> -Peff

Thanks,
Lars

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 13 Feb 2016, at 18:44, Jeff King <peff@peff.net> wrote:

> On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> If config values are queried using 'git config' (e.g. via --get,
>> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
>> find the configuration file where the values were defined.
>> 
>> Teach 'git config' the '--show-origin' option to print the source
>> configuration file for every printed value.
> 
> Thanks, I think this version fixes the correctness issues I mentioned
> earlier. I do still have nits to pick (of course :) ), that we may or
> may not want to deal with.
> 
>> +static void show_config_origin(struct strbuf *buf)
>> +{
>> +	const char term = end_null ? '\0' : '\t';
>> +	const char *type;
>> +	const char *name;
>> +
>> +	current_config_type_name(&type, &name);
> 
> This double out-parameter feels like a clunky interface.
> 
> I was tempted to suggest that we simply make the "struct config_source"
> available to builtin/config.c (which is already pretty intimate with the
> rest of the config code), and then it can pick out what it wants. But
> there _is_ some logic in the function to convert the NULL "cf" into
> "cmdline".
> 
> Perhaps it would be simpler to just have two accessor functions, and do:
> 
>  strbuf_addstr(buf, current_config_type());
>  ...
>  strbuf_addstr(buf, current_config_name());
> 
> I admit it is a pretty minor point, though.
Agreed, this looks nicer.


> 
>> static int show_all_config(const char *key_, const char *value_, void *cb)
>> {
>> +	if (show_origin) {
>> +       struct strbuf buf = STRBUF_INIT;
>> +       show_config_origin(&buf);
>> +       fwrite(buf.buf, 1, buf.len, stdout);
>> +       strbuf_release(&buf);
>> +	}
> 
> The indentation is funky here.
True! Indentation without intention :-) 

> 
> The use of fwrite() to catch the embedded NULs is subtle enough that it
> might be worth a comment.
> 
> It also made me wonder how format_config() handles this. It looks like
> we send the result eventually to fwrite() there, so it all works (and it
> does _not_ have the comment I mentioned :) ).
I will add a comment in both places :-)


> 
>> +test_expect_success '--show-origin' '
>> +	>.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
>> +	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
>> +		cmdline:	user.cmdline=true
>> +	EOF
>> +	git -c user.cmdline=true config --list --show-origin >output &&
>> +	test_cmp expect output &&
>> +
>> +	cat >expect <<-EOF &&
>> +		file:$HOME/.gitconfigQuser.global
>> +		trueQfile:$HOME/.gitconfigQuser.override
>> +		globalQfile:$HOME/.gitconfigQinclude.path
>> +		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
>> +		includeQfile:.git/configQinclude.path
>> +		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
>> +		includeQfile:.git/configQuser.local
>> +		trueQfile:.git/configQuser.override
>> +		localQcmdline:Quser.cmdline
>> +		trueQ
>> +	EOF
> 
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.
> 
> It does mean that the setup mutates the global test state for further
> tests (and you should stop using test_config_*, which clean up at the
> end of the block), but I think that's the right thing here. The point of
> test_config is "flip on this switch just for a moment, so we can test
> its effect without hurting further tests". But these are config tests in
> the first place, and it is OK for them to show a progression of
> mutations of the config (you'll note that like the other tests in this
> script, you are clearing out .git/config in the first place).
> 
TBH I am always a little annoyed if Git tests depend on each other. It makes
it harder to just disable all uninteresting tests and only focus on the one that 
I am working with. However, I agree with your point that the test block does too
many things. Would it be OK if I write a bash function that performs the test
setup? Then I would call this function in the beginning of every individual 
test. Or do you prefer the global state strategy?

Thanks,
Lars

Re: [PATCH v3 1/3] git-config.txt: describe '--includes' default behavior

[Jeff King]

On Sun, Feb 14, 2016 at 01:17:43PM +0100, Lars Schneider wrote:

> Oh. Believe it or not but the similarity is coincidental. I referenced you 
> ("Helped-by") because you explained the expected "includes" behavior to
> me in your v2 review of my series. If I would have found your patch, I 
> would have said so.

Heh. Well, then we can take it as an endorsement, since we both
independently wrote almost the same thing. :)

> I am happy to use your wording. How should I proceed? Should I just drop 
> my "git-config.txt" patch from my series or should I integrate your patch 
> into my series? If the latter, then does the patch require a "Signed-off-by:"
> by me?

It looks like Junio picked up my original, so I think we can just treat
it independently of this topic.

-Peff

Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Jeff King]

On Sun, Feb 14, 2016 at 01:24:55PM +0100, Lars Schneider wrote:

> > Should git_config_from_buf() take a "type" parameter, and
> > git_config_from_blob_sha1() pass in "blob"?
> Haha, fun fact: this was how I implemented it initially. Because of that
> I noticed that "submodule-config.c" also uses "git_config_from_buf" :-)
> 
> However, then I thought the list wouldn't like it if I change the
> interfaces. I will add the type parameter, again.

Changing internal interfaces is preferable to carrying a sub-optimal
interface forever, I think.

The thing we really have to watch out for is changing the assumptions or
output of a function without changing its signature. The interface
change breaks loudly on a merge, and is easy to fix. Changing the
internals creates quiet and subtle bugs.

-Peff

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Sun, Feb 14, 2016 at 01:48:59PM +0100, Lars Schneider wrote:

> > I see you split this up more, but there's still quite a bit going on in
> > this one block. IMHO, it would be more customary in our tests to put the
> > setup into one test_expect_success block, then each of these
> > expect-run-cmp blocks into their own test_expect_success.
> > 
> > It does mean that the setup mutates the global test state for further
> > tests (and you should stop using test_config_*, which clean up at the
> > end of the block), but I think that's the right thing here. The point of
> > test_config is "flip on this switch just for a moment, so we can test
> > its effect without hurting further tests". But these are config tests in
> > the first place, and it is OK for them to show a progression of
> > mutations of the config (you'll note that like the other tests in this
> > script, you are clearing out .git/config in the first place).
> > 
> TBH I am always a little annoyed if Git tests depend on each other. It makes
> it harder to just disable all uninteresting tests and only focus on the one that 
> I am working with. However, I agree with your point that the test block does too
> many things. Would it be OK if I write a bash function that performs the test
> setup? Then I would call this function in the beginning of every individual 
> test. Or do you prefer the global state strategy?

In general, my opinion is that skipping arbitrary leading tests is a
losing strategy. It's just too easy to introduce hidden dependencies,
and not worth the programmer time to make sure each test runs in
isolation. But others on the list may disagree.

That being said, I think what I am proposing is a much milder form of
that. With what I am proposing, you can skip everything _except_ tests
which match /set.?up/ in their description. We do not perfectly adhere
to that in our tests, but I suspect it works a majority of the time.

If it is taking too long to get to a particular test in a test script,
maybe that is a sign we need to break up the script. There are also a
few tricks you can use to still _run_ the earlier blocks, but not have
them interfere with debugging a particular test:

  1. Use --verbose-only=123 to get verbose output only from a single
     test.

  2. Use "-i" to stop running tests at the first failure. Usually it is
     worth fixing that one, and then seeing if other tests fail, too, or
     were simply dependent.

  3. If you are using --valgrind, the tests run very slowly (normally
     t1300 takes 400ms to run on my machine, so I don't mind waiting
     that long to get to a new test at the end. With valgrind it is more
     like 90 seconds). You can use --valgrind-only=123 to test only the
     block you are debugging, and run the rest quickly.

We do use shell functions in some places to do repeated setup. In
general, I prefer setting up the global state. It's more efficient
(which does add up when running the whole suite), and I find it easier
to debug failing tests (it's just one less thing the failing block is
doing that you have to look at; and you can generally "cd" into the
leftover trash directory to investigate the global state).

-Peff

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Johannes Schindelin]

Hi Peff,

tl;dr let's keep an eye on adding only test cases that do not depend on
earlier test cases' output ('setup' excluded, of course).

On Sun, 14 Feb 2016, Jeff King wrote:

> In general, my opinion is that skipping arbitrary leading tests is a
> losing strategy. It's just too easy to introduce hidden dependencies,
> and not worth the programmer time to make sure each test runs in
> isolation. But others on the list may disagree.

Yes, I disagree. And you would, too, if you had to run as many tests as I
had to do by way of js/mingw-tests. I did not keep precise track of time,
but I am certain that I had to run one of those bloody tests (forgot which
one) around 100 times, each taking roughly 3 minutes to complete, and of
course, it was the *last* test case failing, and *of course* it depended
on earlier tests to run.

It gets even worse when you think about those test cases that depend on
some prereq such as SYMLINKS or POSIXPERM. In *most* cases does the
developer who adds them not even notice that these prerequisites are
required. And subsequent test cases that do *not* share those
prerequisites depend (of course!) on the previous test cases' output.

Don't get me wrong, I think it would be too much pain for little gain to
clean up our act now. But I think that we have ample evidence that it
would be a plenty good idea to try pretty hard to avoid adding to the
pile.

Ciao,
Dscho

Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 13 Feb 2016, at 19:15, Jeff King <peff@peff.net> wrote:

> On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:
> 
>> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
>> 
>>>> +test_expect_success '--show-origin' '
>>> [...]
>>> I see you split this up more, but there's still quite a bit going on in
>>> this one block. IMHO, it would be more customary in our tests to put the
>>> setup into one test_expect_success block, then each of these
>>> expect-run-cmp blocks into their own test_expect_success.
>> 
>> Here's a squashable patch that shows what I mean.
> 
> And here are a few comments on the changes...
> 
>> -test_expect_success '--show-origin' '
>> -	>.git/config &&
>> -	>"$HOME"/.gitconfig &&
>> +test_expect_success 'set up --show-origin tests' '
>> 	INCLUDE_DIR="$HOME/include" &&
>> 	mkdir -p "$INCLUDE_DIR" &&
>> -	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>> 		[user]
>> 			absolute = include
>> 	EOF
>> -	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>> 		[user]
>> 			relative = include
>> 	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 >"$HOME"/.gitconfig <<-EOF &&
>> +		[user]
>> +			global = true
>> +			override = global
>> +		[include]
>> +			path = "$INCLUDE_DIR/absolute.include"
>> +	EOF
>> +	cat >.git/config <<-\EOF
>> +		[include]
>> +			path = ../include/relative.include
>> +		[user]
>> +			local = true
>> +			override = local
>> +	EOF
> 
> I preserved your ordering here (as the later "--list" tests care). But
> it might be worth ordering both files the same way, so that a reader
> does not wonder if it is significant (and just update the --list
> output expectation later).
OK, fixed!


> 
>> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>> 		localQcmdline:Quser.cmdline
>> 		trueQ
>> 	EOF
>> -	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
>> +	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
>> +	nul_to_q <output.raw >output &&
> 
> We usually try to avoid putting git on the left-hand side of a pipe,
> because it hides the exit code, and we want to make sure git does not
> fail. I won't be surprised if you copied the style from elsewhere in the
> script, though, as this is an old one and we were not always consistent.
Make sense, fixed!


> 
>> 	echo >>output &&
>> -	test_cmp expect output &&
>> +	test_cmp expect output
> 
> This "echo" might be worth a comment. I think we are just adding the
> extra newline that the here-doc insists on, but that --null output would
> not include.
Done.

> 
> Overall, I find the "--show-origin --null" output pretty weird to read.
> We use a newline to split the config key/value, a NUL between config
> entries, but now also a NUL between the filename and the rest of the
> config entry.
> 
> That makes parsing pretty weird, as you cannot just use something like
> 
>  git config --show-origin --list --null | perl -0ne ...
> 
> to process entries; every other entry you get will be a filename. But at
> the same time, we do not go all out and say "there is a NUL between each
> field", because the key/value separator is a newline in this case, and
> the reader has to parse that separately after splitting on NULs.
> 
> But I think it's too late to do anything about it now. The weirdness is
> really the mixed NUL/newline thing, and you are not introducing that.
> 
>> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
>> -	[user]
>> -		custom = true
>> -EOF
>> +test_expect_success 'set up custom config file' '
>> +	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> +	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
>> +		[user]
>> +			custom = true
>> +	EOF
>> +'
> 
> Everything, even mundane setup, should generally go in a test_expect
> block. That means we'll notice unexpected failures, and any output will
> follow the usual "--verbose" rules.
> 
> Arguably this setup could just go into the initial setup block.
> 
> Also, you may not that the filename does _not_ actually have tabs in it,
> because the shell does not expand "\t". It does have backslashes in it,
> though, which is enough to trigger our C-style quoting.
Oh, thanks for the explanation. I was already wondering about the double
backslash earlier...

> 
> So the test isn't wrong, but the filename is misleading. If you really
> want tabs, you'd have to do:
> 
>  CUSTOM_CONFIG_FILE="$(printf "file\twith\ttabs.conf")
> 
> or similar.
> 
>> test_expect_success '--show-origin escape special file name characters' '
>> 	cat >expect <<-\EOF &&
>> @@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
>> '
>> 
>> test_expect_success '--show-origin stdin with file include' '
>> -	INCLUDE_DIR="$HOME/include" &&
>> -	mkdir -p "$INCLUDE_DIR" &&
>> 	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
>> 		[user]
>> 			stdin = include
> 
> Here we can assume that the setup block succeeded (if it didn't, all of
> the tests are screwed anyway, so you'd want to fix that first).

Thanks,
Lars