Re: [PATCH 2/2] tests: test credential-store XDG support

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

Paul Tan <pyokagan@gmail.com> writes:

> * "helper_test store" is run for when $XDG_CONFIG_HOME/git/credentials
>   exists and ~/.git-credentials does not and the other way around.
> * Test that credentials are stored in XDG file if both XDG and HOME
>   files exist.
> * Test that credentials from XDG file are used if matching credentials
>   are found in both XDG and HOME files.
> * Test that credentials from HOME file are used if a matching credential
>   could not be found in the XDG file.
> * Test that when erasing credentials, matching credentials in both the
>   XDG and HOME files are erase.d
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---

Again, I agree with everything Matthieu said ;-)

> +test_expect_success 'XDG credentials will not be created if it does not exist' '
> +	test ! -e "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +'

You repeat this ${XDG_CONFIG_HOME:-$HOME/.config} all over, but is
that necessary?  The test environment is under your control, so if
you know you have XDG_CONFIG_HOME at this point in the test, don't
use the fallback.  If you want to test _both_, on the other hand,
then test both in separate tests.

> +# Tests for when both $XDG_CONFIG_HOME/git/credentials and
> +# ~/.git-credentials exists.
> +echo '' > "$HOME/.git-credentials"
> +echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"

Do you need one empty line in each of these file, or is existence of
these two files what you care?  If the latter, then just redirect
into it without any command, like this:

	>"$HOME/.git-credentials"

> +test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '
> +	check approve store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +	test ! -z "$contents"

In an earlier part of this patch, you used "test -s FILE".  Do you
have to use "read $it && test [!] -z $it" here?

> +	read contents < "$HOME/.git-credentials"
> +	test -z "$contents"

Missing && everywhere (not just this test).

	check approve store <<-\EOF &&
        ...
        EOF
        read ... &&
        ...

If "check approve" exited with a non-zero status, that won't be
noticed as long as .../git/credentials file happens to have the
contents that passes the later part of the test, which is not what
we want.