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

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

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v4:
* improve wording of "exit code" cleanup commit message (Thanks Peff)
* remove t1300 "--show-origin" test teardown (Thanks Peff)
* fix no parameter function prototype (Thanks Ramsay)
* use doublequotes instead of tabs to test quoted paths (Thanks Junio)

I though a while about Ramsay's suggestion to restore the old error messages
for config reads via stdin. Since reading the config via stdin is not that
common I don't think the increased complexity is worth the effort. Unless
Ramsay or someone else feels strongly about it, I would like to proceed and
keep the new error messages

Thanks for the review,
Lars


Lars Schneider (3):
  t: do not hide Git's exit code in tests using 'nul_to_q'
  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       | 161 ++++++++++++++++++++++++++++++++++++++++++-
 t/t1308-config-set.sh        |   4 +-
 t/t7008-grep-binary.sh       |   3 +-
 8 files changed, 236 insertions(+), 26 deletions(-)

--
2.5.1

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

[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.

Fix all invocations of 'nul_to_q' (defined in /t/test-lib-functions.sh) using
this pattern. There is one more occurrence of the pattern in t9010-svn-fe.sh
which is too evolved to change it easily.

All remaining test code that does not adhere to the pattern can be found with
the following command:
git grep -E 'git.*[^|]\|($|[^|])'

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 v5 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).

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
 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..791283c 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(void);
+extern const char *current_config_name(void);
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index 86a5eb2..55f3400 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(void)
+{
+	return cf && cf->type ? cf->type : "cmdline";
+}
+
+const char *current_config_name(void)
+{
+	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 v5 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       | 147 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 190 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..e54f6d5 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1209,4 +1209,151 @@ 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="file\" (dq) and spaces.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\" (dq) and spaces.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\" (dq) and spaces.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 v5 2/3] config: add 'type' to config_source struct that identifies config type

[Junio C Hamano]

larsxschneider@gmail.com writes:

> 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).

"type" is too broad a word in the context of configuration file, and
does not help readers as a variable or a field name in a structure.
You are not talking about "this is a binary typed variable", for
example.

If showing the origin is useful to the user, that origin should be
called origin, not type, I would think.

> 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 &&

This definitely is a great improvement ;-)

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> larsxschneider@gmail.com writes:
>
>> 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).
>
> "type" is too broad a word in the context of configuration file, and
> does not help readers as a variable or a field name in a structure.
> You are not talking about "this is a binary typed variable", for
> example.
>
> If showing the origin is useful to the user, that origin should be
> called origin, not type, I would think.

You seem to use in 3/3 "origin type", and I think that is a sensible
phrasing.  Renaming 'type' to 'origin' in this patch would not be a
good idea, but renaming it to 'origin_type' would be a great thing
to do.

In the context of configuration parsing, 'origin' would be like
"file called .git/config", "standard input", "command line's 3rd
argument", etc., i.e. for some origin types, it would consist of
<origin_type=file, origin_value=.git/config> pair, while some other
origin type (e.g. "standard input") there is no need for an
"origin_value" that further qualifies the origin.

Thanks.

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

[Lars Schneider]

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

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> larsxschneider@gmail.com writes:
>> 
>>> 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).
>> 
>> "type" is too broad a word in the context of configuration file, and
>> does not help readers as a variable or a field name in a structure.
>> You are not talking about "this is a binary typed variable", for
>> example.
>> 
>> If showing the origin is useful to the user, that origin should be
>> called origin, not type, I would think.
> 
> You seem to use in 3/3 "origin type", and I think that is a sensible
> phrasing.  Renaming 'type' to 'origin' in this patch would not be a
> good idea, but renaming it to 'origin_type' would be a great thing
> to do.

OK, I agree. I will wait one more day for other comments and make a
reroll with the change tomorrow.

Thanks,
Lars


> 
> In the context of configuration parsing, 'origin' would be like
> "file called .git/config", "standard input", "command line's 3rd
> argument", etc., i.e. for some origin types, it would consist of
> <origin_type=file, origin_value=.git/config> pair, while some other
> origin type (e.g. "standard input") there is no need for an
> "origin_value" that further qualifies the origin.
> 
> Thanks.