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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2015, 23:11 UTC
Message-ID: <xmqqa8ztad7q.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1425414299-24000-3-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 13 quoted lines

> * "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"

Show 9 quoted lines

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