On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@xxxxxxxxx> wrote:
> t0302 now tests git-credential-store's support for the XDG user-specific
> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:
>
> * Ensure that the XDG file is strictly opt-in. It should not be created
> by git at all times if it does not exist.
>
> * On the flip side, if the XDG file exists, ~/.git-credentials should
> not be created at all times.
>
> * If both the XDG file and ~/.git-credentials exists, then both files
> should be used for credential lookups. However, credentials should
> only be written to ~/.git-credentials.
Regarding the final sentence: I don't see a test corresponding to the
restriction that only ~/.git-credentials will be modified if both
files exist. Am I missing something?
More below.
> * Credentials must be erased from both files.
>
> * $XDG_CONFIG_HOME can be a custom directory set by the user as per the
> XDG base directory specification. Test that git-credential-store
> respects that, but defaults to "~/.config/git/credentials" if it does
> not exist or is empty.
>
> Helped-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
> Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
> Signed-off-by: Paul Tan <pyokagan@xxxxxxxxx>
> ---
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index f61b40c..34752ea 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -6,4 +6,111 @@ test_description='credential-store tests'
>
> helper_test store
>
> +test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '
> + test -s "$HOME/.git-credentials"
> +'
> +
> +test_expect_success 'xdg credentials file will not be created if it does not exist' '
> + test_path_is_missing "$HOME/.config/git/credentials"
Since these two tests are complementary, each checking a facet of the
same behavioral rule, it seems like they ought to be combined. For
people reading the tests, having them separate implies incorrectly
that they are unrelated, making it difficult to understand the overall
picture of how the pieces relate to one another. In prose, you would
describe the behavior as:
When XDG credentials does not exist, write only to
~/.git-credentials; do not create XDG credentials.
It's one thought; easy to read and understand. The test should reflect
the same intent:
test_expect_success '...' '
test -s "$HOME/.git-credentials" &&
test_path_is_missing "$HOME/.config/git/credentials"
'
The same observation applies to several other tests below.
> +'
> +
> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
> + test_might_fail rm "$HOME/.git-credentials" &&
Can this just be "rm -f $HOME/.git-credentials &&" instead?
> + mkdir -p "$HOME/.config/git" &&
> + >"$HOME/.config/git/credentials"
> +'
> +
> +helper_test store
> +
> +test_expect_success 'xdg credentials file will be written to if it exists' '
> + test -s "$HOME/.config/git/credentials"
> +'
> +
> +test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '
> + test_path_is_missing "$HOME/.git-credentials"
Ditto: It seems like these two tests should be combined.
> +'
> +
> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
> + XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
> + mkdir -p "$XDG_CONFIG_HOME/git" &&
> + >"$XDG_CONFIG_HOME/git/credentials" &&
> + >"$HOME/.config/git/credentials"
> +'
> +
> +helper_test store
> +
> +test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '
> + test_when_finished "unset XDG_CONFIG_HOME" &&
> + test -s "$XDG_CONFIG_HOME/git/credentials"
> +'
It feels wrong to set global state (XDG_CONFIG_HOME) in one test and
then clear it later in another test, and it's not obvious to the
casual reader that global state is being modified. An alternative
would be to set XDG_CONFIG_HOME outside of the first test to which it
applies, clear it after the final test. In reality, however, it only
needs to be defined for the "helper_test store" invocation, so it also
could be highly localized:
XDG_CONFIG_HOME="$HOME/xdg"
export XDG_CONFIG_HOME
helper_test store
unset XDG_CONFIG_HOME
A final alternative would be to wrap the block of tests needing
XDG_CONFIG_HOME within a subshell and set the variable only within the
subshell. Then, there's no need to unset it, and it's clear to the
reader that only the tests within the subshell are affected by it.
> +
> +test_expect_success '~/.config/git/credentials file will not be written to if a custom XDG_CONFIG_HOME is set' '
> + test_must_be_empty "$HOME/.config/git/credentials"
> +'
> +
> +test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '
> + test_path_is_missing "$HOME/.git-credentials"
For clarity, the three above tests probably ought to be combined.
> +'
> +
> +test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '
> + mkdir -p "$HOME/.config/git" &&
> + echo "https://xdg-user:xdg-pass@xxxxxxxxxxx"; >"$HOME/.config/git/credentials" &&
> + echo "https://home-user:home-pass@xxxxxxxxxxx"; >"$HOME/.git-credentials" &&
> + check fill store <<-\EOF
> + protocol=https
> + host=example.com
> + --
> + protocol=https
> + host=example.com
> + username=home-user
> + password=home-pass
> + --
> + EOF
> +'
> +
> +test_expect_success 'get: return credentials from xdg file if the home files do not have them' '
> + mkdir -p "$HOME/.config/git" &&
> + >"$HOME/.git-credentials" &&
> + echo "https://home-user:home-pass@xxxxxxxxxxx"; >"$HOME/.config/git/credentials" &&
In the other tests, credentials in the XDG file are
"xdg-user:xdg-pass", and credentials in ~/.git-credentials are
"home-user:home-pass". It's not at all clear why, in this test, you
instead use "home-user:home-pass" for the XDG credentials. It seems
like an arbitrary and unnecessary change from the norm, and thus
confuses the reader.
> + check fill store <<-\EOF
> + protocol=https
> + host=example.com
> + --
> + protocol=https
> + host=example.com
> + username=home-user
> + password=home-pass
> + --
> + EOF
> +'
> +
> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '
> + mkdir -p "$HOME/.config/git" &&
> + echo "https://xdg-user:xdg-pass@xxxxxxxxxxx"; >"$HOME/.config/git/credentials" &&
> + echo "https://home-user:home-pass@xxxxxxxxxxx"; >"$HOME/.git-credentials" &&
> + chmod -r "$HOME/.config/git/credentials" &&
> + check fill store <<-\EOF
> + protocol=https
> + host=example.com
> + --
> + protocol=https
> + host=example.com
> + username=home-user
> + password=home-pass
> + --
> + EOF
> +'
> +
> +test_expect_success 'erase: erase matching credentials from both xdg and home files' '
> + mkdir -p "$HOME/.config/git" &&
> + echo "https://xdg-user:xdg-pass@xxxxxxxxxxx"; >"$HOME/.config/git/credentials" &&
> + echo "https://home-user:home-pass@xxxxxxxxxxx"; >"$HOME/.git-credentials" &&
> + check reject store <<-\EOF
> + protocol=https
> + host=example.com
> + EOF
> + test_must_be_empty "$HOME/.config/git/credentials" &&
> + test_must_be_empty "$HOME/.git-credentials"
> +'
> +
> test_done
> --
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html