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

[Lars Schneider <larsxschneider@gmail.com>]

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

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


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


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

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

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

Thanks,
Lars