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

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

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v3:

* pass type as parameter to "git_config_from_mem" (renamed from
  "git_config_from_buf") and "do_config_from_file"
* split current_config_type_name into two functions
* explain usage of fwrite with a comment
* use tabs instead of spaces to fix indentation error
* squash Peff's commit to split the test cases
* sort all test configs key/value pairs in the same way
* add comment to explain newline introduce by here-doc
* use real tabs for test file name
* add a test teardown
* fix hiding of git exit code t1300 and t7008
* drop "git-config.txt: describe '--includes' default behavior" patch

I like the idea of a "test set up block" within a test script. In order
to clean up nicely before any subsequent tests I would like to propose
a "tear down" block. Would that work as a compromise in our "test cases
depend on earlier test cases" discussion?

In "t: do not hide Git's exit code in tests" I also fixed a few more
places where Git's exit code was hidden. Please drop this patch if you
think that this should not be part of this series.

Thanks a lot for the reviews and explainations,
Lars

Lars Schneider (3):
  t: do not hide Git's exit code in tests
  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 |  15 ++--
 builtin/config.c             |  33 +++++++++
 cache.h                      |   6 +-
 config.c                     |  36 +++++++---
 submodule-config.c           |   4 +-
 t/t1300-repo-config.sh       | 167 ++++++++++++++++++++++++++++++++++++++++++-
 t/t1308-config-set.sh        |   4 +-
 t/t7008-grep-binary.sh       |   3 +-
 8 files changed, 242 insertions(+), 26 deletions(-)

--
2.5.1

[PATCH v4 1/3] t: do not hide Git's exit code in tests

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Git should not be on the left-hand side of a pipe, because it hides the exit
code, and we want to make sure git does not fail.

There is one more occurrence of this pattern in t9010-svn-fe.sh which is to
evolved to change it easily.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t1300-repo-config.sh | 6 ++++--
 t/t7008-grep-binary.sh | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 52678e7..1782add 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -957,13 +957,15 @@ Qsection.sub=section.val4
 Qsection.sub=section.val5Q
 EOF
 test_expect_success '--null --list' '
-	git config --null --list | nul_to_q >result &&
+	git config --null --list >result.raw &&
+	nul_to_q <result.raw >result &&
 	echo >>result &&
 	test_cmp expect result
 '
 
 test_expect_success '--null --get-regexp' '
-	git config --null --get-regexp "val[0-9]" | nul_to_q >result &&
+	git config --null --get-regexp "val[0-9]" >result.raw &&
+	nul_to_q <result.raw >result &&
 	echo >>result &&
 	test_cmp expect result
 '
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index b146406..9c9c378 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -141,7 +141,8 @@ test_expect_success 'grep respects not-binary diff attribute' '
 	test_cmp expect actual &&
 	echo "b diff" >.gitattributes &&
 	echo "b:binQary" >expect &&
-	git grep bin b | nul_to_q >actual &&
+	git grep bin b >actual.raw &&
+	nul_to_q <actual.raw >actual &&
 	test_cmp expect actual
 '
 
-- 
2.5.1

[PATCH v4 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>
---
 cache.h                |  6 ++++--
 config.c               | 36 +++++++++++++++++++++++++-----------
 submodule-config.c     |  4 ++--
 t/t1300-repo-config.sh |  8 +++++++-
 t/t1308-config-set.sh  |  4 ++--
 5 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/cache.h b/cache.h
index c63fcc1..8d86e5c 100644
--- a/cache.h
+++ b/cache.h
@@ -1485,8 +1485,8 @@ struct git_config_source {
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
-extern int git_config_from_buf(config_fn_t fn, const char *name,
-			       const char *buf, size_t len, void *data);
+extern int git_config_from_mem(config_fn_t fn, const char *type,
+					const char *name, const char *buf, size_t len, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern void git_config(config_fn_t fn, void *);
@@ -1525,6 +1525,8 @@ 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_type();
+extern const char *current_config_name();
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index 86a5eb2..5cf11b5 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);
 }
 
@@ -1061,11 +1062,13 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data)
 }
 
 static int do_config_from_file(config_fn_t fn,
-		const char *name, const char *path, FILE *f, void *data)
+		const char *type, const char *name, const char *path, FILE *f,
+		void *data)
 {
 	struct config_source top;
 
 	top.u.file = f;
+	top.type = type;
 	top.name = name;
 	top.path = path;
 	top.die_on_error = 1;
@@ -1078,7 +1081,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, "stdin", "", NULL, stdin, data);
 }
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
@@ -1089,21 +1092,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	f = fopen(filename, "r");
 	if (f) {
 		flockfile(f);
-		ret = do_config_from_file(fn, filename, filename, f, data);
+		ret = do_config_from_file(fn, "file", filename, filename, f, data);
 		funlockfile(f);
 		fclose(f);
 	}
 	return ret;
 }
 
-int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
-			size_t len, void *data)
+int git_config_from_mem(config_fn_t fn, const char *type, const char *name,
+			const char *buf, size_t len, void *data)
 {
 	struct config_source top;
 
 	top.u.buf.buf = buf;
 	top.u.buf.len = len;
 	top.u.buf.pos = 0;
+	top.type = type;
 	top.name = name;
 	top.path = NULL;
 	top.die_on_error = 0;
@@ -1132,7 +1136,7 @@ static int git_config_from_blob_sha1(config_fn_t fn,
 		return error("reference '%s' does not point to a blob", name);
 	}
 
-	ret = git_config_from_buf(fn, name, buf, size, data);
+	ret = git_config_from_mem(fn, "blob", name, buf, size, data);
 	free(buf);
 
 	return ret;
@@ -2385,3 +2389,13 @@ int parse_config_key(const char *var,
 
 	return 0;
 }
+
+const char *current_config_type()
+{
+	return cf && cf->type ? cf->type : "cmdline";
+}
+
+const char *current_config_name()
+{
+	return cf && cf->name ? cf->name : "";
+}
diff --git a/submodule-config.c b/submodule-config.c
index fe8ceab..92502b5 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -427,8 +427,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	parameter.commit_sha1 = commit_sha1;
 	parameter.gitmodules_sha1 = sha1;
 	parameter.overwrite = 0;
-	git_config_from_buf(parse_config, rev.buf, config, config_size,
-			&parameter);
+	git_config_from_mem(parse_config, "submodule-blob", rev.buf,
+			config, config_size, &parameter);
 	free(config);
 
 	switch (lookup_type) {
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1782add..42ed5cc 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 v4 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             |  33 ++++++++++
 t/t1300-repo-config.sh       | 153 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2608ca7..6374997 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..7bad0c0 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,28 @@ 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';
+
+	strbuf_addstr(buf, current_config_type());
+	strbuf_addch(buf, ':');
+	if (end_null)
+		strbuf_addstr(buf, current_config_name());
+	else
+		quote_c_style(current_config_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);
+		/* Use fwrite as "buf" can contain \0's if "end_null" is set. */
+		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 +131,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 +563,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/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 42ed5cc..4abbdf9 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1209,4 +1209,157 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success 'set up --show-origin tests' '
+	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
+	cat >"$HOME"/.gitconfig <<-EOF &&
+		[user]
+			global = true
+			override = global
+		[include]
+			path = "$INCLUDE_DIR/absolute.include"
+	EOF
+	cat >.git/config <<-\EOF
+		[user]
+			local = true
+			override = local
+		[include]
+			path = ../include/relative.include
+	EOF
+'
+
+test_expect_success '--show-origin with --list' '
+	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	user.local=true
+		file:.git/config	user.override=local
+		file:.git/config	include.path=../include/relative.include
+		file:.git/../include/relative.include	user.relative=include
+		cmdline:	user.cmdline=true
+	EOF
+	git -c user.cmdline=true config --list --show-origin >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin with --list --null' '
+	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/configQuser.local
+		trueQfile:.git/configQuser.override
+		localQfile:.git/configQinclude.path
+		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
+		includeQcmdline:Quser.cmdline
+		trueQ
+	EOF
+	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
+	nul_to_q <output.raw >output &&
+	# The here-doc above adds a newline that the --null output would not
+	# include. Add it here to make the two comparable.
+	echo >>output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin with single file' '
+	cat >expect <<-\EOF &&
+		file:.git/config	user.local=true
+		file:.git/config	user.override=local
+		file:.git/config	include.path=../include/relative.include
+	EOF
+	git config --local --list --show-origin >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_expect_success '--show-origin getting a single key' '
+	cat >expect <<-\EOF &&
+		file:.git/config	local
+	EOF
+	git config --show-origin user.override >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'set up custom config file' '
+	CUSTOM_CONFIG_FILE=$(printf "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' '
+	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_expect_success 'tear down --show-origin tests' '
+	rm -rf "$INCLUDE_DIR" &&
+	>"$HOME"/.gitconfig &&
+	>.git/config
+'
+
 test_done
-- 
2.5.1

Re: [PATCH v4 1/3] t: do not hide Git's exit code in tests

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:44AM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Git should not be 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 think this is a nice cleanup.

> There is one more occurrence of this pattern in t9010-svn-fe.sh which is to
> evolved to change it easily.

The final sentence in the commit message needs s/to/too/. However, I'm
not sure this is the only remaining case. Doing:

  git grep -E 'git.*[^|]\|($|[^|])'

shows quite a few. I guess you just looked for "nul_to_q". There is
certainly no need to fix all of them, but you may want to note the
extent of your grepping in your commit message.

-Peff

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

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:45AM +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).
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  cache.h                |  6 ++++--
>  config.c               | 36 +++++++++++++++++++++++++-----------
>  submodule-config.c     |  4 ++--
>  t/t1300-repo-config.sh |  8 +++++++-
>  t/t1308-config-set.sh  |  4 ++--
>  5 files changed, 40 insertions(+), 18 deletions(-)

Looks good to me.

-Peff

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

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschneider@gmail.com wrote:

> I like the idea of a "test set up block" within a test script. In order
> to clean up nicely before any subsequent tests I would like to propose
> a "tear down" block. Would that work as a compromise in our "test cases
> depend on earlier test cases" discussion?

I don't have any real problem with what you've written in the final
patch, but I also don't think it's accomplishing much (and is more lines
of code, and more running processes).

If you want to run test N without having run all of 1..N-1, what you
really want is some known, reliable state when that test starts. But the
tests before it do not necessarily know what that state is.  The best
they can do is roughly restore the original state before they ran. But:

  1. What does the state consist of? Which files (and their contents)
     are important to the test?

     In your tear-down you get rid of $INCLUDE_DIR, and you zero-out the
     config files. But you leave expect, output, output.raw, and the
     oddly named $CUSTOM_CONFIG_FILE. Nor do you clean up the
     environment variables.

     To be clear, I think it's perfectly fine to leave those. But you
     are still making assumptions about what the next test relies on.

  2. We may create a clean slate, but that is probably not what the next
     test wants. It will want to do its own setup. I.e., it will
     probably not want a blank .git/config, and will create it itself,
     just as you did in your setup step.

So rather than tearing down, I think we are better off trying to make
tests themselves (or blocks of them) set up their own assumptions. E.g.,
by overwriting files rather than appending to them. By using unique
filenames, commit messages, etc for their tests. That's less of a big
deal here, but in many tests that create commits, "test_commit foo"
would fail a second time, because there are no changes to "foo". Doing
"test_commit subdir/check-diff-in-subdir" is less likely to clash
without another test.

Sometimes we _are_ better off with a teardown step, because subsequent
tests would not reasonably think to clear some state we've set (e.g., in
non-config tests, if we set some random config variable, we use
test_config to tear it down afterwards rather than have each test clean
out all of the config). So there's definitely a subjective judgement
call on what is "reasonable" there. But I find it unlikely that your
tear-down will help anybody in this case. Further tests will not care
about $INCLUDE_DIR unless they reference it, and any further tests would
set up their own .git/config, etc.

-Peff

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

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:46AM +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.
> 
> 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             |  33 ++++++++++
>  t/t1300-repo-config.sh       | 153 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 196 insertions(+), 5 deletions(-)

I packed all of my thoughts on test tear-down into my response to the
cover letter. :)

Other than that minor point, this version looks good to me.

-Peff

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

[Eric Sunshine]

On Mon, Feb 15, 2016 at 5:17 AM,  <larsxschneider@gmail.com> wrote:
> 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>
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -27,6 +28,7 @@ static int actions, types;
>  static const char *get_color_slot, *get_colorbool_slot;
>  static int end_null;

Not related to your changes, but I just realized that this variable
really ought to be named 'end_nul' since we're talking about the
character NUL, not a NULL pointer.

>  static int respect_includes = -1;
> +static int show_origin;
> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
>         OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),

Likewise, the long option name should be --nul rather than --null, or
the long name could be dropped altogether since some other commands
just recognize short option -z.

There is no need for this patch series to address this anomaly; it's
perhaps low-hanging fruit for someone wanting to join the project. The
only very minor wrinkle is that we'd still need to recognize --null as
a deprecated (and undocumented) alias for --nul.

>         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(),
>  };

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

[Jeff King]

On Mon, Feb 15, 2016 at 03:58:12PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -27,6 +28,7 @@ static int actions, types;
> >  static const char *get_color_slot, *get_colorbool_slot;
> >  static int end_null;
> 
> Not related to your changes, but I just realized that this variable
> really ought to be named 'end_nul' since we're talking about the
> character NUL, not a NULL pointer.

Yeah, I noticed that, too. We just went through a round of related fixes
here, and the "usual" name is now "nul_term_line". I don't especially
like that one either, but at least it is correct and consistent. :)

> >  static int respect_includes = -1;
> > +static int show_origin;
> > @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
> >         OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
> 
> Likewise, the long option name should be --nul rather than --null, or
> the long name could be dropped altogether since some other commands
> just recognize short option -z.
> 
> There is no need for this patch series to address this anomaly; it's
> perhaps low-hanging fruit for someone wanting to join the project. The
> only very minor wrinkle is that we'd still need to recognize --null as
> a deprecated (and undocumented) alias for --nul.

I think that would be OK as long as we keep the compatible option.

-Peff

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

[Ramsay Jones]

On 15/02/16 10:17, 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).
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  cache.h                |  6 ++++--
>  config.c               | 36 +++++++++++++++++++++++++-----------
>  submodule-config.c     |  4 ++--
>  t/t1300-repo-config.sh |  8 +++++++-
>  t/t1308-config-set.sh  |  4 ++--
>  5 files changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index c63fcc1..8d86e5c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1485,8 +1485,8 @@ struct git_config_source {
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  extern int git_default_config(const char *, const char *, void *);
>  extern int git_config_from_file(config_fn_t fn, const char *, void *);
> -extern int git_config_from_buf(config_fn_t fn, const char *name,
> -			       const char *buf, size_t len, void *data);
> +extern int git_config_from_mem(config_fn_t fn, const char *type,
> +					const char *name, const char *buf, size_t len, void *data);
>  extern void git_config_push_parameter(const char *text);
>  extern int git_config_from_parameters(config_fn_t fn, void *data);
>  extern void git_config(config_fn_t fn, void *);
> @@ -1525,6 +1525,8 @@ 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_type();
> +extern const char *current_config_name();
>  
>  struct config_include_data {
>  	int depth;
> diff --git a/config.c b/config.c
> index 86a5eb2..5cf11b5 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);
>  }
>  
> @@ -1061,11 +1062,13 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data)
>  }
>  
>  static int do_config_from_file(config_fn_t fn,
> -		const char *name, const char *path, FILE *f, void *data)
> +		const char *type, const char *name, const char *path, FILE *f,
> +		void *data)
>  {
>  	struct config_source top;
>  
>  	top.u.file = f;
> +	top.type = type;
>  	top.name = name;
>  	top.path = path;
>  	top.die_on_error = 1;
> @@ -1078,7 +1081,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, "stdin", "", NULL, stdin, data);

I think this should be:
	return do_config_from_file(fn, "file", "<stdin>", NULL, stdin, data);

ie. <stdin> is not a separate type, but a file that does not exist in
the filesystem and, thus, has no name. (what you use internally is a
separate issue, but <file,NULL> works for me.)

Also, I'm so used to '<stdin>' as the 'name' (token/handle) of that
file, that it looks very odd spelt otherwise. :-D

>  }
>  
>  int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> @@ -1089,21 +1092,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
>  	f = fopen(filename, "r");
>  	if (f) {
>  		flockfile(f);
> -		ret = do_config_from_file(fn, filename, filename, f, data);
> +		ret = do_config_from_file(fn, "file", filename, filename, f, data);
>  		funlockfile(f);
>  		fclose(f);
>  	}
>  	return ret;
>  }
>  
> -int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
> -			size_t len, void *data)
> +int git_config_from_mem(config_fn_t fn, const char *type, const char *name,
> +			const char *buf, size_t len, void *data)
>  {
>  	struct config_source top;
>  
>  	top.u.buf.buf = buf;
>  	top.u.buf.len = len;
>  	top.u.buf.pos = 0;
> +	top.type = type;
>  	top.name = name;
>  	top.path = NULL;
>  	top.die_on_error = 0;
> @@ -1132,7 +1136,7 @@ static int git_config_from_blob_sha1(config_fn_t fn,
>  		return error("reference '%s' does not point to a blob", name);
>  	}
>  
> -	ret = git_config_from_buf(fn, name, buf, size, data);
> +	ret = git_config_from_mem(fn, "blob", name, buf, size, data);
>  	free(buf);
>  
>  	return ret;
> @@ -2385,3 +2389,13 @@ int parse_config_key(const char *var,
>  
>  	return 0;
>  }
> +
> +const char *current_config_type()
> +{
> +	return cf && cf->type ? cf->type : "cmdline";
> +}
> +
> +const char *current_config_name()
> +{
> +	return cf && cf->name ? cf->name : "";
> +}
> diff --git a/submodule-config.c b/submodule-config.c
> index fe8ceab..92502b5 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -427,8 +427,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>  	parameter.commit_sha1 = commit_sha1;
>  	parameter.gitmodules_sha1 = sha1;
>  	parameter.overwrite = 0;
> -	git_config_from_buf(parse_config, rev.buf, config, config_size,
> -			&parameter);
> +	git_config_from_mem(parse_config, "submodule-blob", rev.buf,
> +			config, config_size, &parameter);
>  	free(config);
>  
>  	switch (lookup_type) {
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 1782add..42ed5cc 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 &&

So, this looks odd. Using the above would give:
	fatal: bad config line 1 in file <stdin>

> +	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
>  '
> 

ATB,
Ramsay Jones

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

[Ramsay Jones]

On 15/02/16 10:17, 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.
> 
> 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             |  33 ++++++++++
>  t/t1300-repo-config.sh       | 153 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 196 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 2608ca7..6374997 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

file, blob, cmdline (hmm, maybe cmd-line? ;-) )

> +	(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..7bad0c0 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,28 @@ 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';
> +
> +	strbuf_addstr(buf, current_config_type());
> +	strbuf_addch(buf, ':');
> +	if (end_null)
> +		strbuf_addstr(buf, current_config_name());
> +	else
> +		quote_c_style(current_config_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);
> +		/* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> +		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 +131,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 +563,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/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 42ed5cc..4abbdf9 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1209,4 +1209,157 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
>  	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
>  '
>  
> +test_expect_success 'set up --show-origin tests' '
> +	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
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +		[user]
> +			global = true
> +			override = global
> +		[include]
> +			path = "$INCLUDE_DIR/absolute.include"
> +	EOF
> +	cat >.git/config <<-\EOF
> +		[user]
> +			local = true
> +			override = local
> +		[include]
> +			path = ../include/relative.include
> +	EOF
> +'
> +
> +test_expect_success '--show-origin with --list' '
> +	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	user.local=true
> +		file:.git/config	user.override=local
> +		file:.git/config	include.path=../include/relative.include
> +		file:.git/../include/relative.include	user.relative=include
> +		cmdline:	user.cmdline=true
> +	EOF
> +	git -c user.cmdline=true config --list --show-origin >output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success '--show-origin with --list --null' '
> +	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/configQuser.local
> +		trueQfile:.git/configQuser.override
> +		localQfile:.git/configQinclude.path
> +		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
> +		includeQcmdline:Quser.cmdline
> +		trueQ
> +	EOF
> +	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
> +	nul_to_q <output.raw >output &&
> +	# The here-doc above adds a newline that the --null output would not
> +	# include. Add it here to make the two comparable.
> +	echo >>output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success '--show-origin with single file' '
> +	cat >expect <<-\EOF &&
> +		file:.git/config	user.local=true
> +		file:.git/config	user.override=local
> +		file:.git/config	include.path=../include/relative.include
> +	EOF
> +	git config --local --list --show-origin >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_expect_success '--show-origin getting a single key' '
> +	cat >expect <<-\EOF &&
> +		file:.git/config	local
> +	EOF
> +	git config --show-origin user.override >output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success 'set up custom config file' '
> +	CUSTOM_CONFIG_FILE=$(printf "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

So, as with the previous patch, I think this should be:
		file:<stdin>	user.custom=true

ATB,
Ramsay Jones


> +	EOF
> +	git config --file - --show-origin --list <"$CUSTOM_CONFIG_FILE" >output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success '--show-origin stdin with file include' '
> +	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_expect_success 'tear down --show-origin tests' '
> +	rm -rf "$INCLUDE_DIR" &&
> +	>"$HOME"/.gitconfig &&
> +	>.git/config
> +'
> +
>  test_done
> 

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

[Jeff King]

On Mon, Feb 15, 2016 at 09:36:23PM +0000, Ramsay Jones wrote:

> > +test_expect_success '--show-origin stdin' '
> > +	cat >expect <<-\EOF &&
> > +		stdin:	user.custom=true
> 
> So, as with the previous patch, I think this should be:
> 		file:<stdin>	user.custom=true

That's ambiguous with a file named "<stdin>", which was the point of
having the two separate prefixes in the first place.

I think in practice we _could_ get by with an ambiguous output (it's not
like "<stdin>" is a common filename), but that was discussed earlier in
the thread, and Lars decided to go for something unambiguous.

That doesn't necessarily have to bleed over into the error messages,
though (which could continue to use "<stdin>" if we want to put in a
little extra code to covering the cases separately.

-Peff

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> +--show-origin::
>> +	Augment the output of all queried config options with the
>> +	origin type (file, stdin, blob, cmdline) and the actual origin
>
> file, blob, cmdline (hmm, maybe cmd-line? ;-) )

If we are going to spell it out, "command-line" would be the way to
go.  "cmdline" is probably OK.

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

[Junio C Hamano]

larsxschneider@gmail.com writes:

> +test_expect_success 'set up custom config file' '
> +	CUSTOM_CONFIG_FILE=$(printf "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
> +'

Do we really need to use a file with such a name?

An existing test t3300 tells me that a test that uses a path with a
tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
dquote is exercised, can't we just do with a path with a SP in it or
something?

Thanks.

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

[Ramsay Jones]

On 15/02/16 21:40, Jeff King wrote:
> On Mon, Feb 15, 2016 at 09:36:23PM +0000, Ramsay Jones wrote:
> 
>>> +test_expect_success '--show-origin stdin' '
>>> +	cat >expect <<-\EOF &&
>>> +		stdin:	user.custom=true
>>
>> So, as with the previous patch, I think this should be:
>> 		file:<stdin>	user.custom=true
> 
> That's ambiguous with a file named "<stdin>", which was the point of
> having the two separate prefixes in the first place.
> 
> I think in practice we _could_ get by with an ambiguous output (it's not
> like "<stdin>" is a common filename), but that was discussed earlier in
> the thread, and Lars decided to go for something unambiguous.

sure, I just don't think it would cause a problem in practice.
How about using '-' for <stdin>? Hmm, you can actually create
such a file in the filesystem! Oh well, I guess its not a big deal.

> 
> That doesn't necessarily have to bleed over into the error messages,
> though (which could continue to use "<stdin>" if we want to put in a
> little extra code to covering the cases separately.

Yep.

ATB,
Ramsay Jones

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

[Jeff King]

On Mon, Feb 15, 2016 at 02:18:18PM -0800, Junio C Hamano wrote:

> larsxschneider@gmail.com writes:
> 
> > +test_expect_success 'set up custom config file' '
> > +	CUSTOM_CONFIG_FILE=$(printf "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
> > +'
> 
> Do we really need to use a file with such a name?
> 
> An existing test t3300 tells me that a test that uses a path with a
> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
> dquote is exercised, can't we just do with a path with a SP in it or
> something?

It has to trigger quote_c_style(). You can see the complete set of
quoted characters in quote.c:sq_lookup, but space is not one of them.
Probably double-quote or backslash is the best bet, as the rest are all
control characters.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> An existing test t3300 tells me that a test that uses a path with a
>> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
>> dquote is exercised, can't we just do with a path with a SP in it or
>> something?
>
> It has to trigger quote_c_style(). You can see the complete set of
> quoted characters in quote.c:sq_lookup, but space is not one of them.
> Probably double-quote or backslash is the best bet, as the rest are all
> control characters.

Yeah, 3300 seems to use dq for that.

Thanks for checking.

Re: [PATCH v4 1/3] t: do not hide Git's exit code in tests

[Lars Schneider]

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

> On Mon, Feb 15, 2016 at 11:17:44AM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Git should not be 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 think this is a nice cleanup.
> 
>> There is one more occurrence of this pattern in t9010-svn-fe.sh which is to
>> evolved to change it easily.
> 
> The final sentence in the commit message needs s/to/too/.
OK!

> However, I'm
> not sure this is the only remaining case. Doing:
> 
>  git grep -E 'git.*[^|]\|($|[^|])'
> 
> shows quite a few. I guess you just looked for "nul_to_q". There is
> certainly no need to fix all of them, but you may want to note the
> extent of your grepping in your commit message.
Yes, I looked only for nul_to_q... there are more than 1000 other cases. 
I thought about a clever sed script to fix the easy ones but I don't
think it is worth the effort.

Thanks,
Lars

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

[Lars Schneider]

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

> On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschneider@gmail.com wrote:
> 
>> I like the idea of a "test set up block" within a test script. In order
>> to clean up nicely before any subsequent tests I would like to propose
>> a "tear down" block. Would that work as a compromise in our "test cases
>> depend on earlier test cases" discussion?
> 
> I don't have any real problem with what you've written in the final
> patch, but I also don't think it's accomplishing much (and is more lines
> of code, and more running processes).
> 
> If you want to run test N without having run all of 1..N-1, what you
> really want is some known, reliable state when that test starts. But the
> tests before it do not necessarily know what that state is.  The best
> they can do is roughly restore the original state before they ran. But:
> 
>  1. What does the state consist of? Which files (and their contents)
>     are important to the test?
> 
>     In your tear-down you get rid of $INCLUDE_DIR, and you zero-out the
>     config files. But you leave expect, output, output.raw, and the
>     oddly named $CUSTOM_CONFIG_FILE. Nor do you clean up the
>     environment variables.
Good argument - I can't disagree.

> 
>     To be clear, I think it's perfectly fine to leave those. But you
>     are still making assumptions about what the next test relies on.
> 
>  2. We may create a clean slate, but that is probably not what the next
>     test wants. It will want to do its own setup. I.e., it will
>     probably not want a blank .git/config, and will create it itself,
>     just as you did in your setup step.
> 
> So rather than tearing down, I think we are better off trying to make
> tests themselves (or blocks of them) set up their own assumptions. E.g.,
> by overwriting files rather than appending to them. By using unique
> filenames, commit messages, etc for their tests. That's less of a big
> deal here, but in many tests that create commits, "test_commit foo"
> would fail a second time, because there are no changes to "foo". Doing
> "test_commit subdir/check-diff-in-subdir" is less likely to clash
> without another test.
> 
> Sometimes we _are_ better off with a teardown step, because subsequent
> tests would not reasonably think to clear some state we've set (e.g., in
> non-config tests, if we set some random config variable, we use
> test_config to tear it down afterwards rather than have each test clean
> out all of the config). So there's definitely a subjective judgement
> call on what is "reasonable" there. But I find it unlikely that your
> tear-down will help anybody in this case. Further tests will not care
> about $INCLUDE_DIR unless they reference it, and any further tests would
> set up their own .git/config, etc.

OK, I will remove the block in the next roll. Thanks for explaining
your thoughts on this.

- Lars

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

[Lars Schneider]

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

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>>> +--show-origin::
>>> +	Augment the output of all queried config options with the
>>> +	origin type (file, stdin, blob, cmdline) and the actual origin
>> 
>> file, blob, cmdline (hmm, maybe cmd-line? ;-) )
> 
> If we are going to spell it out, "command-line" would be the way to
> go.  "cmdline" is probably OK.

I think cmdline is OK, too. However, if the list thinks that there is a chance
that it could be incomprehensible to the user then I would prefer "command-line".

- Lars

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

[Lars Schneider]

On 15 Feb 2016, at 23:39, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

> 
> 
> On 15/02/16 21:40, Jeff King wrote:
>> On Mon, Feb 15, 2016 at 09:36:23PM +0000, Ramsay Jones wrote:
>> 
>>>> +test_expect_success '--show-origin stdin' '
>>>> +	cat >expect <<-\EOF &&
>>>> +		stdin:	user.custom=true
>>> 
>>> So, as with the previous patch, I think this should be:
>>> 		file:<stdin>	user.custom=true
>> 
>> That's ambiguous with a file named "<stdin>", which was the point of
>> having the two separate prefixes in the first place.
>> 
>> I think in practice we _could_ get by with an ambiguous output (it's not
>> like "<stdin>" is a common filename), but that was discussed earlier in
>> the thread, and Lars decided to go for something unambiguous.
> 
> sure, I just don't think it would cause a problem in practice.
> How about using '-' for <stdin>? Hmm, you can actually create
> such a file in the filesystem! Oh well, I guess its not a big deal.
> 
>> 
>> That doesn't necessarily have to bleed over into the error messages,
>> though (which could continue to use "<stdin>" if we want to put in a
>> little extra code to covering the cases separately.
> 
> Yep.
OK, I am happy to add the extra code. However, out of curiosity, can
you explain in what cases you actually use configs from stdin? I wasn't
aware of this feature before working on this patch and I still wonder
when I would use it. If it is only a seldom used feature then I am not
sure if adding the extra code to restore the existing error message
is worth the effort?

Thanks,
Lars

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

[Lars Schneider]

On 16 Feb 2016, at 00:56, Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
> 
>>> An existing test t3300 tells me that a test that uses a path with a
>>> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
>>> dquote is exercised, can't we just do with a path with a SP in it or
>>> something?
>> 
>> It has to trigger quote_c_style(). You can see the complete set of
>> quoted characters in quote.c:sq_lookup, but space is not one of them.
>> Probably double-quote or backslash is the best bet, as the rest are all
>> control characters.
> 
> Yeah, 3300 seems to use dq for that.
> 
> Thanks for checking.

Thanks for the pointer Junio. I didn't think of FAT/NTFS. I will mimic
3300 and use double quotes.

Thanks,
Lars

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

[Ramsay Jones]

On 16/02/16 09:51, Lars Schneider wrote:
> 
> On 15 Feb 2016, at 23:39, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
>>
>>
>> On 15/02/16 21:40, Jeff King wrote:
>>> On Mon, Feb 15, 2016 at 09:36:23PM +0000, Ramsay Jones wrote:
>>>
>>>>> +test_expect_success '--show-origin stdin' '
>>>>> +	cat >expect <<-\EOF &&
>>>>> +		stdin:	user.custom=true
>>>>
>>>> So, as with the previous patch, I think this should be:
>>>> 		file:<stdin>	user.custom=true
>>>
>>> That's ambiguous with a file named "<stdin>", which was the point of
>>> having the two separate prefixes in the first place.
>>>
>>> I think in practice we _could_ get by with an ambiguous output (it's not
>>> like "<stdin>" is a common filename), but that was discussed earlier in
>>> the thread, and Lars decided to go for something unambiguous.
>>
>> sure, I just don't think it would cause a problem in practice.
>> How about using '-' for <stdin>? Hmm, you can actually create
>> such a file in the filesystem! Oh well, I guess its not a big deal.
>>
>>>
>>> That doesn't necessarily have to bleed over into the error messages,
>>> though (which could continue to use "<stdin>" if we want to put in a
>>> little extra code to covering the cases separately.
>>
>> Yep.

Sorry for not replying earlier - today has been hectic, so far.

> OK, I am happy to add the extra code. 

Unless I've missed something (quite possible), this patch does not
need to change. (you have (both) convinced me that your current
solution is the best).

The only change that I would suggest is the one-liner I already
suggested to the previous patch (plus the one-liner in the test,
of course. err ... so the two-liner!). Having said that, I didn't
try it out - I was just typing into my email client, so ...

>                                       However, out of curiosity, can
> you explain in what cases you actually use configs from stdin? I wasn't
> aware of this feature before working on this patch and I still wonder
> when I would use it.

Personally, I can't imagine ever using it. (I don't have a great
imagination. ;-)

Since I couldn't recall when this feature was added, I looked for
the commit that added it and found it was merged in commit 08f36302.
In particular, commit 3caec73b ("config: teach "git config --file -"
to read from the standard input", 19-02-2014) does not seem to
include any motivation for the change. The corresponding release
notes for v2.2.0 do not seem to add anything either.

So, I'm not much help here. :(

[Ah, looking at the date on the merge explains why I didn't
notice this.]

>                  If it is only a seldom used feature then I am not
> sure if adding the extra code to restore the existing error message
> is worth the effort?

ATB,
Ramsay Jones

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

[Jeff King]

On Tue, Feb 16, 2016 at 04:46:07PM +0000, Ramsay Jones wrote:

> > OK, I am happy to add the extra code. 
> 
> Unless I've missed something (quite possible), this patch does not
> need to change. (you have (both) convinced me that your current
> solution is the best).
> 
> The only change that I would suggest is the one-liner I already
> suggested to the previous patch (plus the one-liner in the test,
> of course. err ... so the two-liner!). Having said that, I didn't
> try it out - I was just typing into my email client, so ...

I think it's more than that one-liner. This patch shows "type:name"
verbatim from what is passed into do_config_from_file, as does the error
message. If they are going to have different output formats (e.g.,
"<stdin>" versus "stdin"), there needs to be logic transforming them in
at least one of the spots.

-Peff

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

[Ramsay Jones]

On 15/02/16 17:42, Jeff King wrote:
> On Mon, Feb 15, 2016 at 11:17:45AM +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).
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>>  cache.h                |  6 ++++--
>>  config.c               | 36 +++++++++++++++++++++++++-----------
>>  submodule-config.c     |  4 ++--
>>  t/t1300-repo-config.sh |  8 +++++++-
>>  t/t1308-config-set.sh  |  4 ++--
>>  5 files changed, 40 insertions(+), 18 deletions(-)
> 
> Looks good to me.
> 
> -Peff
> 

Hi Lars,

Could you please squash this into your patch.

Thanks!

ATB,
Ramsay Jones

-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Tue, 16 Feb 2016 21:35:41 +0000
Subject: [PATCH] config: avoid declaration missing prototype warnings

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 cache.h  | 4 ++--
 config.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 3ff94dc..7f1fa65 100644
--- a/cache.h
+++ b/cache.h
@@ -1546,8 +1546,8 @@ 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_type();
-extern const char *current_config_name();
+extern const char *current_config_type(void);
+extern const char *current_config_name(void);
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index f1efba8..0a35323 100644
--- a/config.c
+++ b/config.c
@@ -2414,12 +2414,12 @@ int parse_config_key(const char *var,
 	return 0;
 }
 
-const char *current_config_type()
+const char *current_config_type(void)
 {
 	return cf && cf->type ? cf->type : "cmdline";
 }
 
-const char *current_config_name()
+const char *current_config_name(void)
 {
 	return cf && cf->name ? cf->name : "";
 }
-- 
2.7.0

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

[Ramsay Jones]

On 16/02/16 17:38, Jeff King wrote:
> On Tue, Feb 16, 2016 at 04:46:07PM +0000, Ramsay Jones wrote:
> 
>>> OK, I am happy to add the extra code. 
>>
>> Unless I've missed something (quite possible), this patch does not
>> need to change. (you have (both) convinced me that your current
>> solution is the best).
>>
>> The only change that I would suggest is the one-liner I already
>> suggested to the previous patch (plus the one-liner in the test,
>> of course. err ... so the two-liner!). Having said that, I didn't
>> try it out - I was just typing into my email client, so ...
> 
> I think it's more than that one-liner. This patch shows "type:name"
> verbatim from what is passed into do_config_from_file, as does the error
> message. If they are going to have different output formats (e.g.,
> "<stdin>" versus "stdin"), there needs to be logic transforming them in
> at least one of the spots.

Ugh, yes you are right.

Hmm, I just hacked something up (see below) and, since its a bit
ugly, I'm now in two minds! (it could be improved, of course). ;-)

So, I'll leave it to yourself and Lars to decide.

ATB,
Ramsay Jones

-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Tue, 16 Feb 2016 21:39:47 +0000
Subject: [PATCH] config: fixup '<stdin>' error messages

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 config.c               | 22 ++++++++++++++++++----
 t/t1300-repo-config.sh |  2 +-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index 0a35323..adc808c 100644
--- a/config.c
+++ b/config.c
@@ -417,6 +417,8 @@ static int git_parse_source(config_fn_t fn, void *data)
 	int comment = 0;
 	int baselen = 0;
 	struct strbuf *var = &cf->var;
+	const char *cftype = cf->type;
+	const char *cfname = cf->name;
 
 	/* U+FEFF Byte Order Mark in UTF8 */
 	const char *bomptr = utf8_bom;
@@ -471,10 +473,14 @@ static int git_parse_source(config_fn_t fn, void *data)
 		if (get_value(fn, data, var) < 0)
 			break;
 	}
+	if (!strcmp(cftype, "stdin")) {
+		cftype = "file";
+		cfname = "<stdin>";
+	}
 	if (cf->die_on_error)
-		die(_("bad config line %d in %s %s"), cf->linenr, cf->type, cf->name);
+		die(_("bad config line %d in %s %s"), cf->linenr, cftype, cfname);
 	else
-		return error(_("bad config line %d in %s %s"), cf->linenr, cf->type, cf->name);
+		return error(_("bad config line %d in %s %s"), cf->linenr, cftype, cfname);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -589,9 +595,17 @@ static void die_bad_number(const char *name, const char *value)
 	if (!value)
 		value = "";
 
-	if (cf && cf->type && cf->name)
+	if (cf && cf->type && cf->name) {
+		const char *cftype = cf->type;
+		const char *cfname = cf->name;
+
+		if (!strcmp(cftype, "stdin")) {
+			cftype = "file";
+			cfname = "<stdin>";
+		}
 		die(_("bad numeric config value '%s' for '%s' in %s %s: %s"),
-		    value, name, cf->type, cf->name, reason);
+		    value, name, cftype, cfname, reason);
+	}
 	die(_("bad numeric config value '%s' for '%s': %s"), value, name, reason);
 }
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4abbdf9..4497688 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -707,7 +707,7 @@ test_expect_success 'invalid unit' '
 '
 
 test_expect_success 'invalid stdin config' '
-	echo "fatal: bad config line 1 in stdin " >expect &&
+	echo "fatal: bad config line 1 in file <stdin>" >expect &&
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
 	test_cmp expect output
 '
-- 
2.7.0

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

[Jeff King]

On Tue, Feb 16, 2016 at 10:14:45PM +0000, Ramsay Jones wrote:

> > I think it's more than that one-liner. This patch shows "type:name"
> > verbatim from what is passed into do_config_from_file, as does the error
> > message. If they are going to have different output formats (e.g.,
> > "<stdin>" versus "stdin"), there needs to be logic transforming them in
> > at least one of the spots.
> 
> Ugh, yes you are right.
> 
> Hmm, I just hacked something up (see below) and, since its a bit
> ugly, I'm now in two minds! (it could be improved, of course). ;-)
> 
> So, I'll leave it to yourself and Lars to decide.
> [...]
> +	if (!strcmp(cftype, "stdin")) {
> +		cftype = "file";
> +		cfname = "<stdin>";
> +	}

I think if we go this route it would be cleaner to just make "type" an
enum and convert it to the appropriate string in the callers. But other
than that, I think your patch is along the correct lines.

I dunno. Personally I am fine with the change in error messages done by
Lars. I could go either way.

-Peff

PS Thanks also for your patch fixing the prototypes. I completely missed
   that.

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

[Lars Schneider]

On 15 Feb 2016, at 21:58, Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Mon, Feb 15, 2016 at 5:17 AM,  <larsxschneider@gmail.com> wrote:
>> 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>
>> ---
>> diff --git a/builtin/config.c b/builtin/config.c
>> @@ -27,6 +28,7 @@ static int actions, types;
>> static const char *get_color_slot, *get_colorbool_slot;
>> static int end_null;
> 
> Not related to your changes, but I just realized that this variable
> really ought to be named 'end_nul' since we're talking about the
> character NUL, not a NULL pointer.
> 
>> static int respect_includes = -1;
>> +static int show_origin;
>> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
>>        OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
> 
> Likewise, the long option name should be --nul rather than --null, or
> the long name could be dropped altogether since some other commands
> just recognize short option -z.
> 
> There is no need for this patch series to address this anomaly; it's
> perhaps low-hanging fruit for someone wanting to join the project. The
> only very minor wrinkle is that we'd still need to recognize --null as
> a deprecated (and undocumented) alias for --nul.

Does the list have a place to document these ideas for newbies to be found?

Thanks,
Lars

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

[Johannes Schindelin]

Hi Lars,

On Mon, 15 Feb 2016, 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).

Given that you settled on `--show-origin` as the command-line option, I
would have expected s/type/origin/g, too...

Ciao,
Dscho

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> There is no need for this patch series to address this anomaly; it's
>> perhaps low-hanging fruit for someone wanting to join the project. The
>> only very minor wrinkle is that we'd still need to recognize --null as
>> a deprecated (and undocumented) alias for --nul.
>
> Does the list have a place to document these ideas for newbies to be found?

I do not know of one offhand, but if somebody (or like-minded group
of people) starts to collect one, I am sure it would be very much
appreciated.

A search for "low hanging fruit" and "hint hint" in the list archive
used to work well as a first-order approximation; using these
keywords takes conscious effort on those who write them, though ;-).

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

[Lars Schneider]

On 17 Feb 2016, at 14:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> Hi Lars,
> 
> On Mon, 15 Feb 2016, 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).
> 
> Given that you settled on `--show-origin` as the command-line option, I
> would have expected s/type/origin/g, too...

Agreed. Junio suggested "origin_type" which I like, too.

Thanks,
Lars