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:

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

quoted

quoted

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

or similar.

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