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@xxxxxxxxxxxxxx> wrote: > On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@xxxxxxxxx> 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. >> +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. >> +' >> + >> +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. >> +' >> + >> +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. >> +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@xxxxxxxxxxx"; >"$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 -- 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