Re: [PATCH v3 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

From: Paul Tan <pyokagan@gmail.com>
Date: Mar 12, 2015, 09:32 UTC
Message-ID: <CACRoPnTcrAW7BBdAM_C8mtm+bG5AKM5028L+wHCesjQ16fvHTQ@mail.gmail.com>
In-Reply-To: <CAPig+cSZpYX0BZFN3Yx=PTiE3VVVVPH9X3yTwKe7p5J=5Dsngg@mail.gmail.com>

Hi,

Thanks for taking the time to write such a detailed review and catching all of my careless mistakes.

On Wed, Mar 11, 2015 at 4:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 17 quoted lines

> On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> 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?

Whoops, the test is indeed missing.

Show 26 quoted lines

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

Indeed, it looks much cleaner. Thanks for the suggestion.

Show 6 quoted lines

>> +'
>> +
>> +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?

I picked this pattern up in t1306-xdg-files.sh so I thought it is the norm, but turns out it's not. I think I can see the rationale though. At this point in the tests the file ~/.git-credentials is expected to exist, so if it does not exist a diagnostic message should be printed to stderr (which -f will not do). Whether this actually helps is purely theoretical though, so I will change it to "rm -f" on the next iteration.

Show 33 quoted lines

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

I agree that the setting of XDG_CONFIG_HOME needs to be more localized. This is a by-product of me following the "no code outside tests" rule too strictly. A quick grep for "export" in the tests show that quite a lot of tests do use export outside of test scripts, though, so I guess I will change this to export XDG_CONFIG_HOME just before running "helper_test store". Thanks for the suggestion.

Show 11 quoted lines

>> +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@example.com" >"$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.

This is a careless mistake on my part when I was renaming all the usernames and passwords to the current form. Thanks for catching it.

Regards, Paul