Re: [PATCH/RFC 2/4] config: test home and xdg files in `list --global`

[Junio C Hamano <gitster@pobox.com>]

"Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Delilah Ashley Wu <delilahwu@microsoft.com>
>
> The `git config list --global` output includes `$HOME/.gitconfig` (home
> config), but ignores `$XDG_CONFIG_HOME/git/config` (XDG config). It
> should include both files.

Please be gentle to future readers of "git log" and help them with a
bit more explanation on the "should" here.  E.g., 

    should include both files, to be consistent with the output from
    `git config list` (not limited to `--global`) that lists entries
    from both files (in addition to system-wide and repository-specific
    entries, of course).

or something.

> Modify tests to check the following and expect a failure:
>   - `git config list --global` should include contents from both the
>      home and XDG config locations (assuming they are readable), not
>      just the former.
>
>   - `--show-origin` should print correct paths to both config files,
>     assuming they exist.

Testing these two combinations is a good thing, but "expect a
failure"?  There doesn't seem to be any test that is marked as
"test_expect_failure" in this patch.  Confused?

    Side note: we generally do not want test_expect_failure tests in
    one patch, followed by a code fix with changes to tests that
    flip s/test_expect_failure/test_expect_success/' in another
    patch, though.  The reason is primarily that such a two-patch
    series makes it harder to review the step that has the fix, by
    hiding the body of the test whose earlier failure gets fixed by
    the code change.

> Also, add tests to ensure subsequent patches do not introduce
> regressions to `git config list`. Specifically, check that:
>   - The home config should take precedence over the XDG config.
>
>   - Without `--global`, it should not bail on unreadable/non-existent
>     global config files.
>
>   - With `--global`, it should bail when both `$HOME/.gitconfig` and
>     `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail if
>     at least one of them is readable.

Good.

> The next patch, config: read global scope via config_sequence, will
> implement a fix to include both config files when `--global` is
> specified.
>
> Reported-by: Jade Lovelace <lists@jade.fyi>
> Helped-by: Derrick Stolee <stolee@gmail.com>
> Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t1300-config.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++++
>  t/t1306-xdg-files.sh |  5 ++--
>  2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index f856821839..5fa0111bd9 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2367,6 +2367,71 @@ test_expect_success '--show-scope with --default' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'list with nonexistent global config' '
> +	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
> +	git config ${mode_prefix}list --show-scope
> +'

Do we expect an empty output, or are we happy as long as "git
config" does not segfault, even if it spews anything?  I guess that
at this late point in the test we have per-repository or system-wide
configuration files with something in them to test, so there would
be some output but we do not care?  If that is the case, not
checking the output, like this patch does, is the right thing.

> +test_expect_success 'list --global with nonexistent global config' '
> +	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
> +	test_must_fail git config ${mode_prefix}list --global --show-scope
> +'

OK.  Do we require --show-scope to fail this, or do we fail with and
without --show-scope as long as --global is in effect?  If the latter,
test both ...

	rm -f "$HOME/.gitconfig" "$HOME/.config/git/config" &&
	test_must_fail git config ${mode_prefix}list --global &&
	test_must_fail git config ${mode_prefix}list --global --show-scope

... like this, perhaps?  Also, don't overuse '-r' with 'rm' (applies
other tests in this patch) when you know what you are removing
should not be a directory.

> +test_expect_success 'list --global with only home' '
> +	rm -rf "$HOME"/.config/git/config &&

Lose "r" from "-rf" or lose "/config".

> +	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +	[home]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	home.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope >output &&
> +	test_cmp expect output
> +'

OK.

> +test_expect_success 'list --global with only xdg' '
> +	rm -f "$HOME"/.gitconfig &&
> +
> +	test_when_finished rm -rf \"\$HOME\"/.config/git &&
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF &&
> +	[xdg]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	xdg.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope >output &&
> +	test_cmp expect output
> +'

OK.

> +test_expect_success 'list --global with both home and xdg' '
> +	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +	[home]
> +		config = true
> +	EOF
> +
> +	test_when_finished rm -rf \"\$HOME\"/.config/git &&
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF &&
> +	[xdg]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	file:$HOME/.config/git/config	xdg.config=true
> +	global	file:$HOME/.gitconfig	home.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope --show-origin >output &&
> +	! test_cmp expect output
> +'

Do not write a test this way.  If you want to document an existing
and unfixed breakage, instead of saying "we do want to see what is
in this expect file, but we know output does not unfortunately match
it", which is how the above test expresses it, start the whole thing
with "test_expect_failure" (instead of "test_expect_success"), and
have the body of the test express what you really want to see.  I.e.
the last steps should say

	git config ${mode_prefix}list --global --show-scope --show-origin >actual &&
	test_cmp expect actual

But an earier side note applies.  If "git config list --global" gets
corrected, this test will see update to turn "! test_cmp" into
"test_cmp" (or "test_expect_success" to "test_expect_failure"), and
such a patch that comes with the code fix will not show what is
being tested and forcing the reviewer to go back to the previous
step to see what the change is really about.  A test that
demonstrates and protects the behaviour corrected by the code change
is best added in the same patch as the code change.

> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> index 40d3c42618..0318755799 100755
> --- a/t/t1306-xdg-files.sh
> +++ b/t/t1306-xdg-files.sh
> @@ -68,9 +68,10 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
>  	>.gitconfig &&
>  	echo "[user]" >.gitconfig &&
>  	echo "	name = read_gitconfig" >>.gitconfig &&
> -	echo user.name=read_gitconfig >expected &&
> +	echo user.name=read_config >expected &&
> +	echo user.name=read_gitconfig >>expected &&
>  	git config --global --list >actual &&
> -	test_cmp expected actual
> +	! test_cmp expected actual
>  '

I cannot quite tell from only half the test, but I suspect that this
shares exactly the same problem with the last one in the other file
I commented above?

Thanks.