git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Paul Tan <pyokagan@gmail.com>
Date
Mar 21, 2015, 05:46 UTC
Message-ID
<CACRoPnTXH+HDD42H2ca7b+OPF15v2TxM+y+NRRi2CS_Xg3tGEw@mail.gmail.com>
In-Reply-To
<CAPig+cR-87-dWXUKoKvphrdb_Y=7268G5uerAiPYDsj-UQiuRw@mail.gmail.com>
Hi,
On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 24 quoted lines
> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> 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.

Show 7 quoted lines
>> +       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)

Show 10 quoted lines
>> +       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"
Show 7 quoted lines
>> +       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"

Show 9 quoted lines
>
>> +       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.
Show 13 quoted lines
>     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.

Show 9 quoted lines
>> +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.
Show 9 quoted lines
>
>> +       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.

Show 19 quoted lines
>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
>> +       echo "https://home-user:home-pass@example.com" >"$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".
Show 22 quoted lines
>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.git-credentials" &&
>> +       echo "https://xdg-user:xdg-pass@example.com" >"$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.

Show 7 quoted lines
>> +       mkdir -p "$HOME/.config/git" &&
>> +       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
>> +       echo "https://home-user:home-pass@example.com" >"$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)
Show 16 quoted lines
>
>> +       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.
Show 29 quoted lines
>
>> +       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@example.com" >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@example.com" >"$HOME/.config/git/credentials" &&
>> +       echo "https://home-user:home-pass@example.com" >"$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.
Previous: Eric SunshineNext: Eric Sunshine
Message 13 of 15 in “git-credential-store: support multiple credential files”
  1. 1/4 git-credential-store: support multiple credential filesPaul Tan, Mar 18, 2015
  2. 2/4 git-credential-store: support XDG_CONFIG_HOMEPaul Tan, Mar 18, 2015
  3. 3/4 docs/git-credential-store: document XDG file and precedencePaul Tan, Mar 18, 2015
  4. Matthieu MoyMar 18, 2015
  5. Paul TanMar 21, 2015
  6. 4/4 t0302: test credential-store support for XDG_CONFIG_HOMEPaul Tan, Mar 18, 2015
  7. Matthieu MoyMar 18, 2015
  8. Eric SunshineMar 18, 2015
  9. Matthieu MoyMar 19, 2015
  10. Paul TanMar 21, 2015
  11. Eric SunshineMar 18, 2015
  12. Eric SunshineMar 19, 2015
  13. Paul TanMar 21, 2015
  14. Eric SunshineMar 23, 2015
  15. Matthieu MoyMar 18, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.