threads / patch / 41407

v3, 3 partsconfig: add '--sources' option to print the source of a config value

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

## tl;dr

25 messages between Feb 13, 2016 and Feb 15, 2016. Diffs are folded; open one to read it.

replies: 24people: 6as markdown or json

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

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

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

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

Thanks a lot Peff, Sebastian, Ramsey, and Eric for the review.
[1] https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/submodule-config.c#L430-L431
Lars Schneider (3):
  git-config.txt: describe '--includes' default behavior
  config: add 'type' to config_source struct that identifies config type
  config: add '--show-origin' option to print the origin of a config
    value
 Documentation/git-config.txt |  19 ++++--
 builtin/config.c             |  35 +++++++++++
 cache.h                      |   1 +
 config.c                     |  23 +++++---
 t/t1300-repo-config.sh       | 136 ++++++++++++++++++++++++++++++++++++++++++-
 t/t1308-config-set.sh        |   4 +-
 6 files changed, 202 insertions(+), 16 deletions(-)

-- 2.5.1

larsxschneider@gmail.com· Feb 13, 2016, 14:24 UTC · re: larsxschneider@gmail.com · lore

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

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

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

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

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

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

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

-Peff
Junio C Hamano· Feb 13, 2016, 21:00 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> ... I did not realize then that my patch from:
>
>   http://article.gmane.org/gmane.comp.version-control.git/262641
>
> was never picked up (but of course I've been carrying it in my tree for
> a year).

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

Thanks for following up.
Jeff King· Feb 14, 2016, 00:34 UTC · re: Junio C Hamano · lore

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

On Sat, Feb 13, 2016 at 01:00:22PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > ... I did not realize then that my patch from:
> >
> >   http://article.gmane.org/gmane.comp.version-control.git/262641
> >
> > was never picked up (but of course I've been carrying it in my tree for
> > a year).
> 
> Hmm, I do not see how it happened, either.  It is not like I was
> offline (I can see I was involved in other threads from the same
> timeframe).  Perhaps it was lost in the noise.
> 
> Thanks for following up.
>From even my limited time as acting maintainer, I know it is easy for

things to occasionally fall through the cracks for no real reason. We rely on submitters to be invested enough in their patch series to give a gentle prod in such cases. However, I sometimes churn out little patches like this at such a rate that _I_ forget about them, too. :)

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

-Peff
Lars Schneider· Feb 14, 2016, 12:17 UTC · re: Jeff King · lore

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

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

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

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

Thanks, Lars

Jeff King· Feb 14, 2016, 16:05 UTC · re: Lars Schneider · lore

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

On Sun, Feb 14, 2016 at 01:17:43PM +0100, Lars Schneider wrote:
> Oh. Believe it or not but the similarity is coincidental. I referenced you 
> ("Helped-by") because you explained the expected "includes" behavior to
> me in your v2 review of my series. If I would have found your patch, I 
> would have said so.

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

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

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

-Peff
larsxschneider@gmail.com· Feb 13, 2016, 14:24 UTC · re: larsxschneider@gmail.com · lore

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

From: Lars Schneider <larsxschneider@gmail.com>

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

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

config.c, t/t1300-repo-config.sh, t/t1308-config-set.sh

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

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

On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Use the config type to print more detailed error messages that inform
> the user about the origin of a config error (file, stdin, blob).
This looks OK overall. A few minor nits...
Show 8 quoted lines
> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>  	top.u.buf.buf = buf;
>  	top.u.buf.len = len;
>  	top.u.buf.pos = 0;
> +	top.type = "blob";
>  	top.name = name;
>  	top.path = NULL;
>  	top.die_on_error = 0;

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

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

Show 17 quoted lines
> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
>  	struct config_source top;
>  
>  	top.u.file = f;
> -	top.name = name;
> +	top.type = path ? "file" : "stdin";
> +	top.name = name ? name : "";
>  	top.path = path;
>  	top.die_on_error = 1;
>  	top.do_fgetc = config_file_fgetc;
> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,
>  
>  static int git_config_from_stdin(config_fn_t fn, void *data)
>  {
> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
> +	return do_config_from_file(fn, NULL, NULL, stdin, data);
>  }

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

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

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

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

-Peff
Junio C Hamano· Feb 13, 2016, 21:04 UTC · re: Jeff King · lore

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

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

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

Show 10 quoted lines
>>  static int git_config_from_stdin(config_fn_t fn, void *data)
>>  {
>> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
>> +	return do_config_from_file(fn, NULL, NULL, stdin, data);
>>  }
>
> Likewise here, we make assumptions in do_config_from_file() about what
> the NULL path means. I think this is less likely to be a problem than
> the other case, but it seems like it would be cleaner for "file" or
> "stdin" to come from the caller, which knows for sure what we are doing.
Makes sense.
> Similarly, I think git_config_from_stdin() can simply pass in an empty
> name rather than NULL to avoid do_config_from_file() having to fix it
> up.
This too.
Show 10 quoted lines
>> +test_expect_success 'invalid stdin config' '
>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> +	test_cmp expect output
>> +'
>
> The original would have been "bad config file line 1 in <stdin>"; I
> think this is an improvement to drop the "file" here.
>
> -Peff
Lars Schneider· Feb 14, 2016, 12:26 UTC · re: Junio C Hamano · lore

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

On 13 Feb 2016, at 22:04, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>>> 	top.u.buf.buf = buf;
>>> 	top.u.buf.len = len;
>>> 	top.u.buf.pos = 0;
>>> +	top.type = "blob";
>>> 	top.name = name;
>>> 	top.path = NULL;
>>> 	top.die_on_error = 0;
>> 
>> This function is about reading config from a memory buffer. The only reason
>> we do so _now_ is when reading from a blob, but I think it is laying a
>> trap for somebody who wants to reuse the function later.
>> 
>> Should git_config_from_buf() take a "type" parameter, and
>> git_config_from_blob_sha1() pass in "blob"?
> 
> I think that is sensible.  I think s/from_buf/from_mem/ may also be
> sensible (it would match the naming used in the index_{fd,mem,...}
> functions in he hashing code).
OK, I will change that, too!
Show 30 quoted lines
> 
>>> static int git_config_from_stdin(config_fn_t fn, void *data)
>>> {
>>> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
>>> +	return do_config_from_file(fn, NULL, NULL, stdin, data);
>>> }
>> 
>> Likewise here, we make assumptions in do_config_from_file() about what
>> the NULL path means. I think this is less likely to be a problem than
>> the other case, but it seems like it would be cleaner for "file" or
>> "stdin" to come from the caller, which knows for sure what we are doing.
> 
> Makes sense.
> 
>> Similarly, I think git_config_from_stdin() can simply pass in an empty
>> name rather than NULL to avoid do_config_from_file() having to fix it
>> up.
> 
> This too.
> 
>>> +test_expect_success 'invalid stdin config' '
>>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>>> +	test_cmp expect output
>>> +'
>> 
>> The original would have been "bad config file line 1 in <stdin>"; I
>> think this is an improvement to drop the "file" here.
>> 
>> -Peff

Thanks, Lars

Lars Schneider· Feb 14, 2016, 12:24 UTC · re: Jeff King · lore

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

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

Haha, fun fact: this was how I implemented it initially. Because of that I noticed that "submodule-config.c" also uses "git_config_from_buf" :-)

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

Show 27 quoted lines
> 
>> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
>> 	struct config_source top;
>> 
>> 	top.u.file = f;
>> -	top.name = name;
>> +	top.type = path ? "file" : "stdin";
>> +	top.name = name ? name : "";
>> 	top.path = path;
>> 	top.die_on_error = 1;
>> 	top.do_fgetc = config_file_fgetc;
>> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,
>> 
>> static int git_config_from_stdin(config_fn_t fn, void *data)
>> {
>> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
>> +	return do_config_from_file(fn, NULL, NULL, stdin, data);
>> }
> 
> Likewise here, we make assumptions in do_config_from_file() about what
> the NULL path means. I think this is less likely to be a problem than
> the other case, but it seems like it would be cleaner for "file" or
> "stdin" to come from the caller, which knows for sure what we are doing.
> 
> Similarly, I think git_config_from_stdin() can simply pass in an empty
> name rather than NULL to avoid do_config_from_file() having to fix it
> up.
OK
Show 9 quoted lines
> 
>> +test_expect_success 'invalid stdin config' '
>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> +	test_cmp expect output
>> +'
> 
> The original would have been "bad config file line 1 in <stdin>"; I
> think this is an improvement to drop the "file" here.

Thanks, Lars

Jeff King· Feb 14, 2016, 16:07 UTC · re: Lars Schneider · lore

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

On Sun, Feb 14, 2016 at 01:24:55PM +0100, Lars Schneider wrote:
Show 7 quoted lines
> > Should git_config_from_buf() take a "type" parameter, and
> > git_config_from_blob_sha1() pass in "blob"?
> Haha, fun fact: this was how I implemented it initially. Because of that
> I noticed that "submodule-config.c" also uses "git_config_from_buf" :-)
> 
> However, then I thought the list wouldn't like it if I change the
> interfaces. I will add the type parameter, again.

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

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

-Peff
larsxschneider@gmail.com· Feb 13, 2016, 14:24 UTC · re: larsxschneider@gmail.com · lore

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

From: Lars Schneider <larsxschneider@gmail.com>

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

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

Based-on-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/git-config.txt |  15 +++--
 builtin/config.c             |  35 ++++++++++++
 cache.h                      |   1 +
 config.c                     |   6 ++
 t/t1300-repo-config.sh       | 128 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 5 deletions(-)
Show changes to 5 files +180 −5

Documentation/git-config.txt, builtin/config.c, cache.h, config.c, t/t1300-repo-config.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 59b1c95..2bf4d4e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,18 +9,18 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [type] --add name value
 'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
-'git config' [<file-option>] [-z|--null] [--name-only] -l | --list
+'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -194,6 +194,11 @@ See also <<FILES>>.
 	Output only the names of config variables for `--list` or
 	`--get-regexp`.
 
+--show-origin::
+	Augment the output of all queried config options with the
+	origin type (file, stdin, blob, cmdline) and the actual origin
+	(config file path, ref, or blob id if applicable).
+
 --get-colorbool name [stdout-is-tty]::
 
 	Find the color setting for `name` (e.g. `color.diff`) and output
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..302b265 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -3,6 +3,7 @@
 #include "color.h"
 #include "parse-options.h"
 #include "urlmatch.h"
+#include "quote.h"
 
 static const char *const builtin_config_usage[] = {
 	N_("git config [<options>]"),
@@ -27,6 +28,7 @@ static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 static int respect_includes = -1;
+static int show_origin;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")),
+	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, stdin, blob, cmdline)")),
 	OPT_END(),
 };
 
@@ -91,8 +94,30 @@ static void check_argc(int argc, int min, int max) {
 	usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
+static void show_config_origin(struct strbuf *buf)
+{
+	const char term = end_null ? '\0' : '\t';
+	const char *type;
+	const char *name;
+
+	current_config_type_name(&type, &name);
+	strbuf_addstr(buf, type);
+	strbuf_addch(buf, ':');
+	if (end_null)
+		strbuf_addstr(buf, name);
+	else
+		quote_c_style(name, buf, NULL, 0);
+	strbuf_addch(buf, term);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+	if (show_origin) {
+       struct strbuf buf = STRBUF_INIT;
+       show_config_origin(&buf);
+       fwrite(buf.buf, 1, buf.len, stdout);
+       strbuf_release(&buf);
+	}
 	if (!omit_values && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
@@ -108,6 +133,8 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char *value_)
 {
+	if (show_origin)
+		show_config_origin(buf);
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
@@ -538,6 +565,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		error("--name-only is only applicable to --list or --get-regexp");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
+
+	if (show_origin && !(actions &
+		(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) {
+		error("--show-origin is only applicable to --get, --get-all, "
+			  "--get-regexp, and --list.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
diff --git a/cache.h b/cache.h
index c63fcc1..3d1eb86 100644
--- a/cache.h
+++ b/cache.h
@@ -1525,6 +1525,7 @@ extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern void current_config_type_name(const char **type, const char **name);
 
 struct config_include_data {
 	int depth;
diff --git a/config.c b/config.c
index 6a5942f..0603d4b 100644
--- a/config.c
+++ b/config.c
@@ -2388,3 +2388,9 @@ int parse_config_key(const char *var,
 
 	return 0;
 }
+
+void current_config_type_name(const char **type, const char **name)
+{
+	*type = cf && cf->type ? cf->type : "cmdline";
+	*name = cf && cf->name ? cf->name : "";
+}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7abdfcb..112f7c8 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1207,4 +1207,132 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+test_expect_success '--show-origin' '
+	>.git/config &&
+	>"$HOME"/.gitconfig &&
+	INCLUDE_DIR="$HOME/include" &&
+	mkdir -p "$INCLUDE_DIR" &&
+	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
+		[user]
+			absolute = include
+	EOF
+	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
+		[user]
+			relative = include
+	EOF
+	test_config_global user.global "true" &&
+	test_config_global user.override "global" &&
+	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
+	test_config include.path ../include/relative.include &&
+	test_config user.local "true" &&
+	test_config user.override "local" &&
+
+	cat >expect <<-EOF &&
+		file:$HOME/.gitconfig	user.global=true
+		file:$HOME/.gitconfig	user.override=global
+		file:$HOME/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
+		file:$INCLUDE_DIR/absolute.include	user.absolute=include
+		file:.git/config	include.path=../include/relative.include
+		file:.git/../include/relative.include	user.relative=include
+		file:.git/config	user.local=true
+		file:.git/config	user.override=local
+		cmdline:	user.cmdline=true
+	EOF
+	git -c user.cmdline=true config --list --show-origin >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file:$HOME/.gitconfigQuser.global
+		trueQfile:$HOME/.gitconfigQuser.override
+		globalQfile:$HOME/.gitconfigQinclude.path
+		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
+		includeQfile:.git/configQinclude.path
+		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
+		includeQfile:.git/configQuser.local
+		trueQfile:.git/configQuser.override
+		localQcmdline:Quser.cmdline
+		trueQ
+	EOF
+	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
+	echo >>output &&
+	test_cmp expect output &&
+
+	cat >expect <<-\EOF &&
+		file:.git/config	include.path=../include/relative.include
+		file:.git/config	user.local=true
+		file:.git/config	user.override=local
+	EOF
+	git config --local --list --show-origin >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+		file:$HOME/.gitconfig	user.global true
+		file:.git/config	user.local true
+	EOF
+	git config --show-origin --get-regexp "user\.[g|l].*" >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-\EOF &&
+		file:.git/config	local
+	EOF
+	git config --show-origin user.override >output &&
+	test_cmp expect output
+'
+
+CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
+cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
+	[user]
+		custom = true
+EOF
+
+test_expect_success '--show-origin escape special file name characters' '
+	cat >expect <<-\EOF &&
+		file:"file\\twith\\ttabs.conf"	user.custom=true
+	EOF
+	git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin stdin' '
+	cat >expect <<-\EOF &&
+		stdin:	user.custom=true
+	EOF
+	git config --file - --show-origin --list <"$CUSTOM_CONFIG_FILE" >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin stdin with file include' '
+	INCLUDE_DIR="$HOME/include" &&
+	mkdir -p "$INCLUDE_DIR" &&
+	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
+		[user]
+			stdin = include
+	EOF
+	cat >expect <<-EOF &&
+		file:$INCLUDE_DIR/stdin.include	include
+	EOF
+	echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
+		| git config --show-origin --includes --file - user.stdin >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin blob' '
+	cat >expect <<-\EOF &&
+		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
+	EOF
+	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+	git config --blob=$blob --show-origin --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--show-origin blob ref' '
+	cat >expect <<-\EOF &&
+		blob:"master:file\\twith\\ttabs.conf"	user.custom=true
+	EOF
+	git add "$CUSTOM_CONFIG_FILE" &&
+	git commit -m "new config file" &&
+	git config --blob=master:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
2.5.1
Jeff King· Feb 13, 2016, 17:44 UTC · re: larsxschneider@gmail.com · lore

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

On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:
Show 8 quoted lines
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.

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

Show 7 quoted lines
> +static void show_config_origin(struct strbuf *buf)
> +{
> +	const char term = end_null ? '\0' : '\t';
> +	const char *type;
> +	const char *name;
> +
> +	current_config_type_name(&type, &name);
This double out-parameter feels like a clunky interface.

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

Perhaps it would be simpler to just have two accessor functions, and do:
  strbuf_addstr(buf, current_config_type());
  ...
  strbuf_addstr(buf, current_config_name());
I admit it is a pretty minor point, though.
Show 8 quoted lines
>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> +	if (show_origin) {
> +       struct strbuf buf = STRBUF_INIT;
> +       show_config_origin(&buf);
> +       fwrite(buf.buf, 1, buf.len, stdout);
> +       strbuf_release(&buf);
> +	}
The indentation is funky here.

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

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

Show 46 quoted lines
> +test_expect_success '--show-origin' '
> +	>.git/config &&
> +	>"$HOME"/.gitconfig &&
> +	INCLUDE_DIR="$HOME/include" &&
> +	mkdir -p "$INCLUDE_DIR" &&
> +	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
> +		[user]
> +			absolute = include
> +	EOF
> +	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
> +		[user]
> +			relative = include
> +	EOF
> +	test_config_global user.global "true" &&
> +	test_config_global user.override "global" &&
> +	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
> +	test_config include.path ../include/relative.include &&
> +	test_config user.local "true" &&
> +	test_config user.override "local" &&
> +
> +	cat >expect <<-EOF &&
> +		file:$HOME/.gitconfig	user.global=true
> +		file:$HOME/.gitconfig	user.override=global
> +		file:$HOME/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
> +		file:$INCLUDE_DIR/absolute.include	user.absolute=include
> +		file:.git/config	include.path=../include/relative.include
> +		file:.git/../include/relative.include	user.relative=include
> +		file:.git/config	user.local=true
> +		file:.git/config	user.override=local
> +		cmdline:	user.cmdline=true
> +	EOF
> +	git -c user.cmdline=true config --list --show-origin >output &&
> +	test_cmp expect output &&
> +
> +	cat >expect <<-EOF &&
> +		file:$HOME/.gitconfigQuser.global
> +		trueQfile:$HOME/.gitconfigQuser.override
> +		globalQfile:$HOME/.gitconfigQinclude.path
> +		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
> +		includeQfile:.git/configQinclude.path
> +		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
> +		includeQfile:.git/configQuser.local
> +		trueQfile:.git/configQuser.override
> +		localQcmdline:Quser.cmdline
> +		trueQ
> +	EOF

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

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

-Peff
Jeff King· Feb 13, 2016, 18:04 UTC · re: Jeff King · lore

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

On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
Show 6 quoted lines
> > +test_expect_success '--show-origin' '
> [...]
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.
Here's a squashable patch that shows what I mean.
---
Show changes to t/t1300-repo-config.sh +40 −23
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 112f7c8..c7496aa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1207,26 +1207,34 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
-test_expect_success '--show-origin' '
-	>.git/config &&
-	>"$HOME"/.gitconfig &&
+test_expect_success 'set up --show-origin tests' '
 	INCLUDE_DIR="$HOME/include" &&
 	mkdir -p "$INCLUDE_DIR" &&
-	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
+	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
 		[user]
 			absolute = include
 	EOF
-	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
+	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
 		[user]
 			relative = include
 	EOF
-	test_config_global user.global "true" &&
-	test_config_global user.override "global" &&
-	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
-	test_config include.path ../include/relative.include &&
-	test_config user.local "true" &&
-	test_config user.override "local" &&
+	cat >"$HOME"/.gitconfig <<-EOF &&
+		[user]
+			global = true
+			override = global
+		[include]
+			path = "$INCLUDE_DIR/absolute.include"
+	EOF
+	cat >.git/config <<-\EOF
+		[include]
+			path = ../include/relative.include
+		[user]
+			local = true
+			override = local
+	EOF
+'
 
+test_expect_success '--show-origin with --list' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global=true
 		file:$HOME/.gitconfig	user.override=global
@@ -1239,8 +1247,10 @@ test_expect_success '--show-origin' '
 		cmdline:	user.cmdline=true
 	EOF
 	git -c user.cmdline=true config --list --show-origin >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with --list --null' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfigQuser.global
 		trueQfile:$HOME/.gitconfigQuser.override
@@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
 		localQcmdline:Quser.cmdline
 		trueQ
 	EOF
-	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
+	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
+	nul_to_q <output.raw >output &&
 	echo >>output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with single file' '
 	cat >expect <<-\EOF &&
 		file:.git/config	include.path=../include/relative.include
 		file:.git/config	user.local=true
 		file:.git/config	user.override=local
 	EOF
 	git config --local --list --show-origin >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin with --get-regexp' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global true
 		file:.git/config	user.local true
 	EOF
 	git config --show-origin --get-regexp "user\.[g|l].*" >output &&
-	test_cmp expect output &&
+	test_cmp expect output
+'
 
+test_expect_success '--show-origin getting a single key' '
 	cat >expect <<-\EOF &&
 		file:.git/config	local
 	EOF
@@ -1279,11 +1296,13 @@ test_expect_success '--show-origin' '
 	test_cmp expect output
 '
 
-CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
-cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
-	[user]
-		custom = true
-EOF
+test_expect_success 'set up custom config file' '
+	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
+	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+		[user]
+			custom = true
+	EOF
+'
 
 test_expect_success '--show-origin escape special file name characters' '
 	cat >expect <<-\EOF &&
@@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
 '
 
 test_expect_success '--show-origin stdin with file include' '
-	INCLUDE_DIR="$HOME/include" &&
-	mkdir -p "$INCLUDE_DIR" &&
 	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
 		[user]
 			stdin = include
Jeff King· Feb 13, 2016, 18:15 UTC · re: Jeff King · lore

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

On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:
Show 10 quoted lines
> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
> 
> > > +test_expect_success '--show-origin' '
> > [...]
> > I see you split this up more, but there's still quite a bit going on in
> > this one block. IMHO, it would be more customary in our tests to put the
> > setup into one test_expect_success block, then each of these
> > expect-run-cmp blocks into their own test_expect_success.
> 
> Here's a squashable patch that shows what I mean.
And here are a few comments on the changes...
Show 36 quoted lines
> -test_expect_success '--show-origin' '
> -	>.git/config &&
> -	>"$HOME"/.gitconfig &&
> +test_expect_success 'set up --show-origin tests' '
>  	INCLUDE_DIR="$HOME/include" &&
>  	mkdir -p "$INCLUDE_DIR" &&
> -	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
> +	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>  		[user]
>  			absolute = include
>  	EOF
> -	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
> +	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>  		[user]
>  			relative = include
>  	EOF
> -	test_config_global user.global "true" &&
> -	test_config_global user.override "global" &&
> -	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
> -	test_config include.path ../include/relative.include &&
> -	test_config user.local "true" &&
> -	test_config user.override "local" &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +		[user]
> +			global = true
> +			override = global
> +		[include]
> +			path = "$INCLUDE_DIR/absolute.include"
> +	EOF
> +	cat >.git/config <<-\EOF
> +		[include]
> +			path = ../include/relative.include
> +		[user]
> +			local = true
> +			override = local
> +	EOF

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

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

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

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

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

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

That makes parsing pretty weird, as you cannot just use something like
  git config --show-origin --list --null | perl -0ne ...

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

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

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

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

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

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

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

  CUSTOM_CONFIG_FILE="$(printf "file\twith\ttabs.conf")
or similar.
Show 11 quoted lines
>  test_expect_success '--show-origin escape special file name characters' '
>  	cat >expect <<-\EOF &&
> @@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
>  '
>  
>  test_expect_success '--show-origin stdin with file include' '
> -	INCLUDE_DIR="$HOME/include" &&
> -	mkdir -p "$INCLUDE_DIR" &&
>  	cat >"$INCLUDE_DIR"/stdin.include <<-EOF &&
>  		[user]
>  			stdin = include

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

-Peff
Lars Schneider· Feb 15, 2016, 09:41 UTC · re: Jeff King · lore

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

On 13 Feb 2016, at 19:15, Jeff King <peff@peff.net> wrote:
Show 56 quoted lines
> On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:
> 
>> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
>> 
>>>> +test_expect_success '--show-origin' '
>>> [...]
>>> I see you split this up more, but there's still quite a bit going on in
>>> this one block. IMHO, it would be more customary in our tests to put the
>>> setup into one test_expect_success block, then each of these
>>> expect-run-cmp blocks into their own test_expect_success.
>> 
>> Here's a squashable patch that shows what I mean.
> 
> And here are a few comments on the changes...
> 
>> -test_expect_success '--show-origin' '
>> -	>.git/config &&
>> -	>"$HOME"/.gitconfig &&
>> +test_expect_success 'set up --show-origin tests' '
>> 	INCLUDE_DIR="$HOME/include" &&
>> 	mkdir -p "$INCLUDE_DIR" &&
>> -	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>> 		[user]
>> 			absolute = include
>> 	EOF
>> -	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +	cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>> 		[user]
>> 			relative = include
>> 	EOF
>> -	test_config_global user.global "true" &&
>> -	test_config_global user.override "global" &&
>> -	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> -	test_config include.path ../include/relative.include &&
>> -	test_config user.local "true" &&
>> -	test_config user.override "local" &&
>> +	cat >"$HOME"/.gitconfig <<-EOF &&
>> +		[user]
>> +			global = true
>> +			override = global
>> +		[include]
>> +			path = "$INCLUDE_DIR/absolute.include"
>> +	EOF
>> +	cat >.git/config <<-\EOF
>> +		[include]
>> +			path = ../include/relative.include
>> +		[user]
>> +			local = true
>> +			override = local
>> +	EOF
> 
> I preserved your ordering here (as the later "--list" tests care). But
> it might be worth ordering both files the same way, so that a reader
> does not wonder if it is significant (and just update the --list
> output expectation later).
OK, fixed!
Show 13 quoted lines
> 
>> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>> 		localQcmdline:Quser.cmdline
>> 		trueQ
>> 	EOF
>> -	git -c user.cmdline=true config --null --list --show-origin | nul_to_q >output &&
>> +	git -c user.cmdline=true config --null --list --show-origin >output.raw &&
>> +	nul_to_q <output.raw >output &&
> 
> We usually try to avoid putting git on the left-hand side of a pipe,
> because it hides the exit code, and we want to make sure git does not
> fail. I won't be surprised if you copied the style from elsewhere in the
> script, though, as this is an old one and we were not always consistent.
Make sense, fixed!
Show 8 quoted lines
> 
>> 	echo >>output &&
>> -	test_cmp expect output &&
>> +	test_cmp expect output
> 
> This "echo" might be worth a comment. I think we are just adding the
> extra newline that the here-doc insists on, but that --null output would
> not include.
Done.
Show 40 quoted lines
> 
> Overall, I find the "--show-origin --null" output pretty weird to read.
> We use a newline to split the config key/value, a NUL between config
> entries, but now also a NUL between the filename and the rest of the
> config entry.
> 
> That makes parsing pretty weird, as you cannot just use something like
> 
>  git config --show-origin --list --null | perl -0ne ...
> 
> to process entries; every other entry you get will be a filename. But at
> the same time, we do not go all out and say "there is a NUL between each
> field", because the key/value separator is a newline in this case, and
> the reader has to parse that separately after splitting on NULs.
> 
> But I think it's too late to do anything about it now. The weirdness is
> really the mixed NUL/newline thing, and you are not introducing that.
> 
>> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
>> -	[user]
>> -		custom = true
>> -EOF
>> +test_expect_success 'set up custom config file' '
>> +	CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> +	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
>> +		[user]
>> +			custom = true
>> +	EOF
>> +'
> 
> Everything, even mundane setup, should generally go in a test_expect
> block. That means we'll notice unexpected failures, and any output will
> follow the usual "--verbose" rules.
> 
> Arguably this setup could just go into the initial setup block.
> 
> Also, you may not that the filename does _not_ actually have tabs in it,
> because the shell does not expand "\t". It does have backslashes in it,
> though, which is enough to trigger our C-style quoting.

Oh, thanks for the explanation. I was already wondering about the double backslash earlier...

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

Thanks, Lars

Lars Schneider· Feb 14, 2016, 12:48 UTC · re: Jeff King · lore

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

On 13 Feb 2016, at 18:44, Jeff King <peff@peff.net> wrote:
Show 38 quoted lines
> On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> If config values are queried using 'git config' (e.g. via --get,
>> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
>> find the configuration file where the values were defined.
>> 
>> Teach 'git config' the '--show-origin' option to print the source
>> configuration file for every printed value.
> 
> Thanks, I think this version fixes the correctness issues I mentioned
> earlier. I do still have nits to pick (of course :) ), that we may or
> may not want to deal with.
> 
>> +static void show_config_origin(struct strbuf *buf)
>> +{
>> +	const char term = end_null ? '\0' : '\t';
>> +	const char *type;
>> +	const char *name;
>> +
>> +	current_config_type_name(&type, &name);
> 
> This double out-parameter feels like a clunky interface.
> 
> I was tempted to suggest that we simply make the "struct config_source"
> available to builtin/config.c (which is already pretty intimate with the
> rest of the config code), and then it can pick out what it wants. But
> there _is_ some logic in the function to convert the NULL "cf" into
> "cmdline".
> 
> Perhaps it would be simpler to just have two accessor functions, and do:
> 
>  strbuf_addstr(buf, current_config_type());
>  ...
>  strbuf_addstr(buf, current_config_name());
> 
> I admit it is a pretty minor point, though.
Agreed, this looks nicer.
Show 11 quoted lines
> 
>> static int show_all_config(const char *key_, const char *value_, void *cb)
>> {
>> +	if (show_origin) {
>> +       struct strbuf buf = STRBUF_INIT;
>> +       show_config_origin(&buf);
>> +       fwrite(buf.buf, 1, buf.len, stdout);
>> +       strbuf_release(&buf);
>> +	}
> 
> The indentation is funky here.
True! Indentation without intention :-) 
Show 7 quoted lines
> 
> The use of fwrite() to catch the embedded NULs is subtle enough that it
> might be worth a comment.
> 
> It also made me wonder how format_config() handles this. It looks like
> we send the result eventually to fwrite() there, so it all works (and it
> does _not_ have the comment I mentioned :) ).
I will add a comment in both places :-)
Show 62 quoted lines
> 
>> +test_expect_success '--show-origin' '
>> +	>.git/config &&
>> +	>"$HOME"/.gitconfig &&
>> +	INCLUDE_DIR="$HOME/include" &&
>> +	mkdir -p "$INCLUDE_DIR" &&
>> +	cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +		[user]
>> +			absolute = include
>> +	EOF
>> +	cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +		[user]
>> +			relative = include
>> +	EOF
>> +	test_config_global user.global "true" &&
>> +	test_config_global user.override "global" &&
>> +	test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> +	test_config include.path ../include/relative.include &&
>> +	test_config user.local "true" &&
>> +	test_config user.override "local" &&
>> +
>> +	cat >expect <<-EOF &&
>> +		file:$HOME/.gitconfig	user.global=true
>> +		file:$HOME/.gitconfig	user.override=global
>> +		file:$HOME/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
>> +		file:$INCLUDE_DIR/absolute.include	user.absolute=include
>> +		file:.git/config	include.path=../include/relative.include
>> +		file:.git/../include/relative.include	user.relative=include
>> +		file:.git/config	user.local=true
>> +		file:.git/config	user.override=local
>> +		cmdline:	user.cmdline=true
>> +	EOF
>> +	git -c user.cmdline=true config --list --show-origin >output &&
>> +	test_cmp expect output &&
>> +
>> +	cat >expect <<-EOF &&
>> +		file:$HOME/.gitconfigQuser.global
>> +		trueQfile:$HOME/.gitconfigQuser.override
>> +		globalQfile:$HOME/.gitconfigQinclude.path
>> +		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
>> +		includeQfile:.git/configQinclude.path
>> +		../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
>> +		includeQfile:.git/configQuser.local
>> +		trueQfile:.git/configQuser.override
>> +		localQcmdline:Quser.cmdline
>> +		trueQ
>> +	EOF
> 
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.
> 
> It does mean that the setup mutates the global test state for further
> tests (and you should stop using test_config_*, which clean up at the
> end of the block), but I think that's the right thing here. The point of
> test_config is "flip on this switch just for a moment, so we can test
> its effect without hurting further tests". But these are config tests in
> the first place, and it is OK for them to show a progression of
> mutations of the config (you'll note that like the other tests in this
> script, you are clearing out .git/config in the first place).
> 

TBH I am always a little annoyed if Git tests depend on each other. It makes it harder to just disable all uninteresting tests and only focus on the one that I am working with. However, I agree with your point that the test block does too many things. Would it be OK if I write a bash function that performs the test setup? Then I would call this function in the beginning of every individual test. Or do you prefer the global state strategy?

Thanks, Lars

Jeff King· Feb 14, 2016, 16:18 UTC · re: Lars Schneider · lore

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

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

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

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

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

  1. Use --verbose-only=123 to get verbose output only from a single
     test.
  2. Use "-i" to stop running tests at the first failure. Usually it is
     worth fixing that one, and then seeing if other tests fail, too, or
     were simply dependent.
  3. If you are using --valgrind, the tests run very slowly (normally
     t1300 takes 400ms to run on my machine, so I don't mind waiting
     that long to get to a new test at the end. With valgrind it is more
     like 90 seconds). You can use --valgrind-only=123 to test only the
     block you are debugging, and run the rest quickly.

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

-Peff
Johannes Schindelin· Feb 15, 2016, 07:53 UTC · re: Jeff King · lore

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

Hi Peff,

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

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

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

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

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

Ciao, Dscho

Mike Rappazzo· Feb 13, 2016, 14:43 UTC · re: larsxschneider@gmail.com · lore

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

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

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

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

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

On Sat, Feb 13, 2016 at 3:43 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
Show 6 quoted lines
>> I renamed the flag from "--source" to "--show-origin" as I got the impression
>> that Sebastian, Peff and Ramsay seem to be all OK with "--show-origin".
>
> I know that I am late to the party here, but why not make the option
> `--verbose` or `-v`?  `git config` doesn't have that now, and this
> seems like a logical thing to include as verbose data.  I would
`--verbose` would be fine with me.
-- 
Sebastian Schuberth
Jeff King· Feb 13, 2016, 18:19 UTC · re: Mike Rappazzo · lore

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

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

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

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

-Peff
Junio C Hamano· Feb 13, 2016, 21:05 UTC · re: Jeff King · lore

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

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

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

← back to recent threads