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
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 18, 2015, 19:26 UTC
Message-ID
<CAPig+cR-87-dWXUKoKvphrdb_Y=7268G5uerAiPYDsj-UQiuRw@mail.gmail.com>
In-Reply-To
<1426662276-8728-4-git-send-email-pyokagan@gmail.com>
On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 23 quoted lines
> t0302 now tests git-credential-store's support for the XDG user-specific
> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:
> ---
>
> The previous version can be found at [1].
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
>
> * Merge related, but previously separate, tests together in order to
>   make the test suite easier to understand.
>
> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set
>   it, and unset it immediately before and after "helper_test store" is
>   called in order to make it localized to only the command that it
>   should affect.
>
> * Add test, previously missing, to check that only the home credentials
>   file is written to if both the xdg and home files exist.
>
> * Correct mislabelling of "home-user"/"home-pass" to the proper
>   "xdg-user"/"xdg-pass".
>
> * Use "rm -f" instead of "test_might_fail rm".
This round looks much better. Thanks.

Most of the comments below are just nit-picky, with one or two genuine (minor) issues.

Show 11 quoted lines
> Thanks Eric for the code review.
>
> 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.
Show 5 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".
Terse version: "setup: -.git-redentials +xdg"
Key: "-" file removed; "+" file created.
Show 8 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"
Show 5 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/
Terse: "setup custom-xdg"
> +       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?

    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.

> +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.
> +       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.
Show 16 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"
Show 16 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?

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

Show 13 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/
Show 30 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
> --
> 2.1.4
Previous: Paul TanNext: Eric Sunshine
Message 11 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.