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

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 11, 2015, 08:40 UTC
Message-ID
<CAPig+cSZpYX0BZFN3Yx=PTiE3VVVVPH9X3yTwKe7p5J=5Dsngg@mail.gmail.com>
In-Reply-To
<1426056553-9364-5-git-send-email-pyokagan@gmail.com>
On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 12 quoted lines
> 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.
Show 25 quoted lines
> * 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@grenoble-inp.fr>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> 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?
Show 12 quoted lines
> +       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.
Show 15 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.

Show 7 quoted lines
> +
> +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.
Show 22 quoted lines
> +'
> +
> +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@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' '
> +       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.

Show 44 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 'get: return credentials from home file if xdg files are 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" &&
> +       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@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: Paul Tan
Message 16 of 17 in “git-credential-store: XDG user-specific config file support”
  1. 0/4 git-credential-store: XDG user-specific config file supportPaul Tan, Mar 11, 2015
  2. 1/4 git-credential-store: support multiple credential filesPaul Tan, Mar 11, 2015
  3. Jeff KingMar 13, 2015
  4. Paul TanMar 14, 2015
  5. Jeff KingMar 14, 2015
  6. Jeff KingMar 14, 2015
  7. Junio C HamanoMar 14, 2015
  8. Matthieu MoyMar 15, 2015
  9. Junio C HamanoMar 15, 2015
  10. Paul TanMar 18, 2015
  11. 2/4 git-credential-store: support XDG_CONFIG_HOMEPaul Tan, Mar 11, 2015
  12. 3/4 docs/git-credential-store: document XDG file and precedencePaul Tan, Mar 11, 2015
  13. Eric SunshineMar 11, 2015
  14. Paul TanMar 12, 2015
  15. 4/4 t0302: test credential-store support for XDG_CONFIG_HOMEPaul Tan, Mar 11, 2015
  16. Eric SunshineMar 11, 2015
  17. Paul TanMar 12, 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.