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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@xxxxxxxxx> wrote:
>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
>> index f61b40c..5b0a666 100755
>> --- a/t/t0302-credential-store.sh
>> +++ b/t/t0302-credential-store.sh
>> @@ -6,4 +6,115 @@ test_description='credential-store tests'
>>
>>  helper_test store
>>
>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '
>
> These test descriptions are quite long, often mirroring in prose what
> the test body already says more concisely. I generally try to keep
> descriptions short while still being descriptive enough to give a
> general idea about what is being tested. I've rewritten a few test
> descriptions (below) to be very short, in fact probably too terse; but
> perhaps better descriptions would lie somewhere between the two
> extremes. First example, for this test:
>
>     "!xdg: >.git-credentials !xdg"
>
> Key: Space-separated items. Items before ":" are the pre-conditions,
> and items after are the post-state. "!" file does not exist; ">"
> output goes here.

I will make the test descriptions shorter. However, I don't think the
test descriptions need to be so terse such that a separate key is
required. e.g. I will shorten the above to "when xdg file does not
exist, it's not created.", or even terser: "when xdg file not exists,
it's not created.". I don't think symbols should be used, as many
other test descriptions do not use them.

>> +       test -s "$HOME/.git-credentials" &&
>> +       test_path_is_missing "$HOME/.config/git/credentials"
>> +'
>> +
>> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
>
> It's customary to call this "setup" rather than "create".

Will fix.

> Terse version: "setup: -.git-redentials +xdg"
>
> Key: "-" file removed; "+" file created.

How about just "setup xdg file" (the fact that ~/.git-credentials is
not created/deleted is implied in the next test)

>> +       rm -f "$HOME/.git-credentials" &&
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.config/git/credentials"
>> +'
>> +
>> +helper_test store
>> +
>> +test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '
>
> Terse version: "!.git-credentials xdg: !.git-credentials >xdg"

How about "when xdg file exists, home file not created"

>> +       test -s "$HOME/.config/git/credentials" &&
>> +       test_path_is_missing "$HOME/.git-credentials"
>> +'
>> +
>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '
>
> s/set up/setup/

Will fix. Thanks.

> Terse: "setup custom-xdg"

It's a matter of taste, but I personally don't see the need for
hyphenation. How about: "setup custom xdg file"

>
>> +       mkdir -p "$HOME/xdg/git" &&
>> +       rm -f "$HOME/.config/git/credentials" &&
>> +       >"$HOME/xdg/git/credentials"
>
> It would be easier to read this if you placed the two lines together
> which refer to the custom xdg file.
> Also, for completeness and to be
> self-contained, don't you want to remove ~/.git-credentials?

Ah yes, thanks for the suggestion.

>     rm -f "$HOME/.git-credentials" &&
>     rm -f "$HOME/.config/git/credentials" &&
>     mkdir -p "$HOME/xdg/git" &&
>     >"$HOME/xdg/git/credentials"
>
>> +'
>> +
>> +XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
>> +unset XDG_CONFIG_HOME
>
> It's hard to spot the "helper_test store" at the end of line. I'd
> place it on a line by itself so that it is easy to see that it is
> wrapped by the setting and unsetting of the environment variable.

Thanks, will fix. Although now it looks weird that the "export" is the
only one with a continuation on a single line, so I split all of them
so that they each have their own line.

>> +test_expect_success 'if custom XDG_CONFIG_HOME credentials file ~/xdg/git/credentials exists, it will only be written to; ~/.config/git/credentials and ~/.git-credentials will not be created' '
>
> Terse: "!.git-credentials !xdg custom-xdg: !.git-credentials !xdg >custom-xdg"
>
>> +       test_when_finished "rm -f $HOME/xdg/git/credentials" &&
>> +       test -s "$HOME/xdg/git/credentials" &&
>> +       test_path_is_missing "$HOME/.config/git/credentials"
>
> Matthieu already pointed out the broken &&-chain.

Will fix. Thanks for catching it.

>
>> +       test_path_is_missing "$HOME/.git-credentials"
>> +'
>> +
>> +test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '
>
> Terse: ".git-credentials xdg: <.git-credentials"
>
> Key: "<" taken from here.

How about "use home file if both home and xdg files have matches". I
wish to make it explicit that which files are used depends on whether
they have the matching credential, not if they exist.

>
>> +       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' '
>
> Terse: "!.git-credentials xdg: <xdg"

How about "use xdg file if home file has no matches".

>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.git-credentials" &&
>> +       echo "https://xdg-user:xdg-pass@xxxxxxxxxxx"; >"$HOME/.config/git/credentials" &&
>> +       check fill store <<-\EOF
>> +       protocol=https
>> +       host=example.com
>> +       --
>> +       protocol=https
>> +       host=example.com
>> +       username=xdg-user
>> +       password=xdg-pass
>> +       --
>> +       EOF
>> +'
>> +
>> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '
>
> An earlier test showed that the home file is preferred if both it and
> the xdg file exist, so is this test actually telling us anything new?
> Did you mean instead to reverse the case and make the home file
> unreadable?

Ah yes, this is embarrassing. Apparently this test was written with
the old precedence ordering. Thank you so much for catching this fatal
error.

>> +       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" &&
>
> It would be a bit easier to see that the 'chmod' applies to the xdg
> file if it directly followed creation of the xdg file.

Will fix (will chmod the home file instead)

>
>> +       check fill store <<-\EOF
>> +       protocol=https
>> +       host=example.com
>> +       --
>> +       protocol=https
>> +       host=example.com
>> +       username=home-user
>> +       password=home-pass
>> +       --
>> +       EOF
>> +'
>> +
>> +test_expect_success 'store: If both xdg and home files exist, only store in home file' '
>
> Inconsistent capitalization: s/If/if/

Will fix.

>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.config/git/credentials" &&
>> +       >"$HOME/.git-credentials" &&
>> +       check approve store <<-\EOF &&
>> +       protocol=https
>> +       host=example.com
>> +       username=store-user
>> +       password=store-pass
>> +       EOF
>> +       echo "https://store-user:store-pass@xxxxxxxxxxx"; >expected &&
>> +       test_cmp expected "$HOME/.git-credentials" &&
>> +       test_must_be_empty "$HOME/.config/git/credentials"
>> +'
>> +
>> +
>> +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

Thanks so much Eric and Matthieu for the review.
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]