{"thread":{"id":"38839","subject":"[PATCH v4 1/4] git-credential-store: support multiple credential files","startedAt":"2015-03-18T07:04:33Z","lastAt":"2015-03-23T01:56:48Z","messageCount":15,"participants":["Paul Tan","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"257907","messageId":"1426662276-8728-1-git-send-email-pyokagan@gmail.com","threadId":"38839","inReplyTo":null,"subject":"[PATCH v4 1/4] git-credential-store: support multiple credential files","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-18T07:04:33Z","receivedAt":"2015-03-18T07:04:33Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Previously, git-credential-store only supported storing credentials in a\nsingle file: ~/.git-credentials. In order to support the XDG base\ndirectory specification[1], git-credential-store needs to be able to\nlookup and erase credentials from multiple files, as well as to pick the\nappropriate file to write to so that the credentials can be found on\nsubsequent lookups.\n\n[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n\nNote that some credential storage files may not be owned, readable or\nwritable by the user, as they may be system-wide files that are meant to\napply to every user.\n\nInstead of a single file path, lookup_credential(), remove_credential()\nand store_credential() now take a precedence-ordered string_list of\nfile paths. lookup_credential() expects both user-specific and\nsystem-wide credential files to be provided to support the use case of\nsystem administrators setting default credentials for users.\nremove_credential() and store_credential() expect only the user-specific\ncredential files to be provided as usually the only config files that\nusers are allowed to edit are their own user-specific ones.\n\nlookup_credential() will read these (user-specific and system-wide) file\npaths in order until it finds the 1st matching credential and print it.\nAs some files may be private and thus unreadable, any file which cannot\nbe read will be ignored silently.\n\nremove_credential() will erase credentials from all (user-specific)\nfiles in the list. This is because if credentials are only erased from\nthe file with the highest precedence, a matching credential may still be\nfound in a file further down the list. (Note that due to the lockfile\ncode, this requires the directory to be writable, which should be so for\nuser-specific config files)\n\nstore_credential() will write the credentials to the first existing\n(user-specific) file in the list. If none of the files in the list\nexist, store_credential() will write to the filename specified by the\nfirst item of the filename list. For backwards compatibility, this\nfilename should be \"~/.git-credentials\".\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\n\nThe previous version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265309\n\nThe changes are as follows:\n\n* Remove default_fn argument from store_credential(). It now uses the\nfirst item of the filenames string_list. Thanks Jeff, Matthieu and\nJunio for your input in this discussion.\n\n* Change filename string_list fns to duplicate strings by default\n(STRING_LIST_INIT_DUP). This is to make memory ownership explicit --\nwhen the string_list receives the file paths from xdg_config_home()\nand expand_user_path(), it is responsible for freeing those strings.\nThanks Jeff for pointing this out.\n\n credential-store.c | 79 +++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 54 insertions(+), 25 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 925d3f4..8dad479 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -6,7 +6,7 @@\n \n static struct lock_file credential_lock;\n \n-static void parse_credential_file(const char *fn,\n+static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n \t\t\t\t void (*other_cb)(struct strbuf *))\n@@ -14,18 +14,20 @@ static void parse_credential_file(const char *fn,\n \tFILE *fh;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n+\tint found_credential = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n-\t\tif (errno != ENOENT)\n+\t\tif (errno != ENOENT && errno != EACCES)\n \t\t\tdie_errno(\"unable to open %s\", fn);\n-\t\treturn;\n+\t\treturn found_credential;\n \t}\n \n \twhile (strbuf_getline(&line, fh, '\\n') != EOF) {\n \t\tcredential_from_url(&entry, line.buf);\n \t\tif (entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\n+\t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n@@ -38,6 +40,7 @@ static void parse_credential_file(const char *fn,\n \tcredential_clear(&entry);\n \tstrbuf_release(&line);\n \tfclose(fh);\n+\treturn found_credential;\n }\n \n static void print_entry(struct credential *c)\n@@ -64,21 +67,10 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(\"unable to commit credential store\");\n }\n \n-static void store_credential(const char *fn, struct credential *c)\n+static void store_credential_file(const char *fn, struct credential *c)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\t/*\n-\t * Sanity check that what we are storing is actually sensible.\n-\t * In particular, we can't make a URL without a protocol field.\n-\t * Without either a host or pathname (depending on the scheme),\n-\t * we have no primary key. And without a username and password,\n-\t * we are not actually storing a credential.\n-\t */\n-\tif (!c->protocol || !(c->host || c->path) ||\n-\t !c->username || !c->password)\n-\t\treturn;\n-\n \tstrbuf_addf(&buf, \"%s://\", c->protocol);\n \tstrbuf_addstr_urlencode(&buf, c->username, 1);\n \tstrbuf_addch(&buf, ':');\n@@ -95,8 +87,35 @@ static void store_credential(const char *fn, struct credential *c)\n \tstrbuf_release(&buf);\n }\n \n-static void remove_credential(const char *fn, struct credential *c)\n+static void store_credential(const struct string_list *fns, struct credential *c)\n {\n+\tstruct string_list_item *fn;\n+\n+\t/*\n+\t * Sanity check that what we are storing is actually sensible.\n+\t * In particular, we can't make a URL without a protocol field.\n+\t * Without either a host or pathname (depending on the scheme),\n+\t * we have no primary key. And without a username and password,\n+\t * we are not actually storing a credential.\n+\t */\n+\tif (!c->protocol || !(c->host || c->path) || !c->username || !c->password)\n+\t\treturn;\n+\n+\tfor_each_string_list_item(fn, fns)\n+\t\tif (!access(fn->string, F_OK)) {\n+\t\t\tstore_credential_file(fn->string, c);\n+\t\t\treturn;\n+\t\t}\n+\t/* Write credential to the filename specified by fns->items[0], thus\n+\t * creating it */\n+\tif (fns->nr)\n+\t\tstore_credential_file(fns->items[0].string, c);\n+}\n+\n+static void remove_credential(const struct string_list *fns, struct credential *c)\n+{\n+\tstruct string_list_item *fn;\n+\n \t/*\n \t * Sanity check that we actually have something to match\n \t * against. The input we get is a restrictive pattern,\n@@ -105,14 +124,20 @@ static void remove_credential(const char *fn, struct credential *c)\n \t * to empty input. So explicitly disallow it, and require that the\n \t * pattern have some actual content to match.\n \t */\n-\tif (c->protocol || c->host || c->path || c->username)\n-\t\trewrite_credential_file(fn, c, NULL);\n+\tif (!c->protocol && !c->host && !c->path && !c->username)\n+\t\treturn;\n+\tfor_each_string_list_item(fn, fns)\n+\t\tif (!access(fn->string, F_OK))\n+\t\t\trewrite_credential_file(fn->string, c, NULL);\n }\n \n-static int lookup_credential(const char *fn, struct credential *c)\n+static void lookup_credential(const struct string_list *fns, struct credential *c)\n {\n-\tparse_credential_file(fn, c, print_entry, NULL);\n-\treturn c->username && c->password;\n+\tstruct string_list_item *fn;\n+\n+\tfor_each_string_list_item(fn, fns)\n+\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\t\treturn; /* Found credential */\n }\n \n int main(int argc, char **argv)\n@@ -123,6 +148,7 @@ int main(int argc, char **argv)\n \t};\n \tconst char *op;\n \tstruct credential c = CREDENTIAL_INIT;\n+\tstruct string_list fns = STRING_LIST_INIT_DUP;\n \tchar *file = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING(0, \"file\", &file, \"path\",\n@@ -139,20 +165,23 @@ int main(int argc, char **argv)\n \n \tif (!file)\n \t\tfile = expand_user_path(\"~/.git-credentials\");\n-\tif (!file)\n+\tif (file)\n+\t\tstring_list_append(&fns, file);\n+\telse\n \t\tdie(\"unable to set up default path; use --file\");\n \n \tif (credential_read(&c, stdin) < 0)\n \t\tdie(\"unable to read credential\");\n \n \tif (!strcmp(op, \"get\"))\n-\t\tlookup_credential(file, &c);\n+\t\tlookup_credential(&fns, &c);\n \telse if (!strcmp(op, \"erase\"))\n-\t\tremove_credential(file, &c);\n+\t\tremove_credential(&fns, &c);\n \telse if (!strcmp(op, \"store\"))\n-\t\tstore_credential(file, &c);\n+\t\tstore_credential(&fns, &c);\n \telse\n \t\t; /* Ignore unknown operation. */\n \n+\tstring_list_clear(&fns, 0);\n \treturn 0;\n }\n-- \n2.1.4\n"},{"id":"257910","messageId":"1426662276-8728-2-git-send-email-pyokagan@gmail.com","threadId":"38839","inReplyTo":"1426662276-8728-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v4 2/4] git-credential-store: support XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-18T07:04:34Z","receivedAt":"2015-03-18T07:04:34Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Add $XDG_CONFIG_HOME/git/credentials to the default credential search\npath of git-credential-store. This allows git-credential-store to\nsupport user-specific configuration files in accordance with the XDG\nbase directory specification[1].\n\n[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n\n~/.git-credentials has a higher precedence than\n$XDG_CONFIG_HOME/git/credentials when looking up credentials. This\nmeans that if any duplicate matching credentials are found in the xdg\nfile (due to ~/.git-credentials being updated by old versions of git or\noutdated tools), they will not be used at all. This is to give the user\nsome leeway in switching to old versions of git while keeping the xdg\ndirectory. This is consistent with the behavior of git-config.\n\nHowever, the higher precedence of ~/.git-credentials means that as long\nas ~/.git-credentials exist, all credentials will be written to the\n~/.git-credentials file even if the user has an xdg file as having a\n~/.git-credentials file indicates that the user wants to preserve\nbackwards-compatibility. This is also consistent with the behavior of\ngit-config.\n\nSince the xdg file will not be used unless it actually exists, to\nprevent the situation where some credentials are present in the xdg file\nwhile some are present in the home file, users are recommended to not\ncreate the xdg file if they require compatibility with old versions of\ngit or outdated tools. Note, though, that \"erase\" can be used to\nexplicitly erase matching credentials from all files.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\n\nThe previous version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308\n\nThere are no changes in this version.\n\n credential-store.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 8dad479..d805df7 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -163,11 +163,16 @@ int main(int argc, char **argv)\n \t\tusage_with_options(usage, options);\n \top = argv[0];\n \n-\tif (!file)\n-\t\tfile = expand_user_path(\"~/.git-credentials\");\n-\tif (file)\n+\tif (file) {\n \t\tstring_list_append(&fns, file);\n-\telse\n+\t} else {\n+\t\tif ((file = expand_user_path(\"~/.git-credentials\")))\n+\t\t\tstring_list_append_nodup(&fns, file);\n+\t\thome_config_paths(NULL, &file, \"credentials\");\n+\t\tif (file)\n+\t\t\tstring_list_append_nodup(&fns, file);\n+\t}\n+\tif (!fns.nr)\n \t\tdie(\"unable to set up default path; use --file\");\n \n \tif (credential_read(&c, stdin) < 0)\n-- \n2.1.4\n"},{"id":"257909","messageId":"1426662276-8728-3-git-send-email-pyokagan@gmail.com","threadId":"38839","inReplyTo":"1426662276-8728-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v4 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-18T07:04:35Z","receivedAt":"2015-03-18T07:04:35Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"git-credential-store now supports an additional default credential file\nat $XDG_CONFIG_HOME/git/credentials. However, ~/.git-credentials takes\nprecedence over it for backwards compatibility. To make the precedence\nordering explicit, add a new section FILES that lists out the credential\nfile paths in their order of precedence, and explains how the ordering\naffects the lookup, storage and erase operations.\n\nAlso update documentation for --store to briefly explain the operations\non multiple files if the --store option is not provided.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Paul Tan \n---\n\nThe previous version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308\n\nThe changes are as follows:\n\n* Remove \"support for this file was added fairly recently\" statement, as\nit will become out-dated with time. Thanks Eric for pointing this out.\n\n Documentation/git-credential-store.txt | 35 ++++++++++++++++++++++++++++++++--\n 1 file changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex bc97071..e16afd8 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -31,10 +31,41 @@ OPTIONS\n \n --file=::\n \n-\tUse `` to store credentials. The file will have its\n+\tUse `` to lookup and store credentials. The file will have its\n \tfilesystem permissions set to prevent other users on the system\n \tfrom reading it, but will not be encrypted or otherwise\n-\tprotected. Defaults to `~/.git-credentials`.\n+\tprotected. If not specified, credentials will be searched for from\n+\t`~/.git-credentials` and `$XDG_CONFIG_HOME/git/credentials`, and\n+\tcredentials will be written to `~/.git-credentials` if it exists, or\n+\t`$XDG_CONFIG_HOME/git/credentials` if it exists and the former does\n+\tnot. See also <>.\n+\n+[[FILES]]\n+FILES\n+-----\n+\n+If not set explicitly with '--file', there are two files where\n+git-credential-store will search for credentials in order of precedence:\n+\n+~/.git-credentials::\n+\tUser-specific credentials file.\n+\n+$XDG_CONFIG_HOME/git/credentials::\n+\tSecond user-specific credentials file. If '$XDG_CONFIG_HOME' is not set\n+\tor empty, `$HOME/.config/git/credentials` will be used. Any credentials\n+\tstored in this file will not be used if `~/.git-credentials` has a\n+\tmatching credential as well. It is a good idea not to create this file\n+\tif you sometimes use older versions of Git that do not support it.\n+\n+For credential lookups, the files are read in the order given above, with the\n+first matching credential found taking precedence over credentials found in\n+files further down the list.\n+\n+Credential storage will per default write to the first existing file in the\n+list. If none of these files exist, `~/.git-credentials` will be created and\n+written to.\n+\n+When erasing credentials, matching credentials will be erased from all files.\n \n EXAMPLES\n --------\n-- \n2.1.4\n"},{"id":"257908","messageId":"1426662276-8728-4-git-send-email-pyokagan@gmail.com","threadId":"38839","inReplyTo":"1426662276-8728-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-18T07:04:36Z","receivedAt":"2015-03-18T07:04:36Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"t0302 now tests git-credential-store's support for the XDG user-specific\nconfiguration file $XDG_CONFIG_HOME/git/credentials. Specifically:\n\n* Ensure that the XDG file is strictly opt-in. It should not be created\n by git at all times if it does not exist.\n\n* Conversely, if the XDG file exists, ~/.git-credentials should\n not be created at all times.\n\n* If both the XDG file and ~/.git-credentials exists, then both files\n should be used for credential lookups. However, credentials should\n only be written to ~/.git-credentials.\n\n* Credentials must be erased from both files.\n\n* $XDG_CONFIG_HOME can be a custom directory set by the user as per the\n XDG base directory specification. Test that git-credential-store\n respects that, but defaults to \"~/.config/git/credentials\" if it does\n not exist or is empty.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Paul Tan \n---\n\nThe previous version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308\n\n* Merge related, but previously separate, tests together in order to\n make the test suite easier to understand.\n\n* Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set\n it, and unset it immediately before and after \"helper_test store\" is\n called in order to make it localized to only the command that it\n should affect.\n\n* Add test, previously missing, to check that only the home credentials\n file is written to if both the xdg and home files exist.\n\n* Correct mislabelling of \"home-user\"/\"home-pass\" to the proper\n \"xdg-user\"/\"xdg-pass\".\n\n* Use \"rm -f\" instead of \"test_might_fail rm\".\n\nThanks Eric for the code review.\n\n t/t0302-credential-store.sh | 111 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 111 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex f61b40c..5b0a666 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -6,4 +6,115 @@ test_description='credential-store tests'\n \n helper_test store\n \n+test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '\n+\ttest -s \"$HOME/.git-credentials\" &&\n+\ttest_path_is_missing \"$HOME/.config/git/credentials\"\n+'\n+\n+test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n+\trm -f \"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\t>\"$HOME/.config/git/credentials\"\n+'\n+\n+helper_test store\n+\n+test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '\n+\ttest -s \"$HOME/.config/git/credentials\" &&\n+\ttest_path_is_missing \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '\n+\tmkdir -p \"$HOME/xdg/git\" &&\n+\trm -f \"$HOME/.config/git/credentials\" &&\n+\t>\"$HOME/xdg/git/credentials\"\n+'\n+\n+XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME && helper_test store\n+unset XDG_CONFIG_HOME\n+\n+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' '\n+\ttest_when_finished \"rm -f $HOME/xdg/git/credentials\" &&\n+\ttest -s \"$HOME/xdg/git/credentials\" &&\n+\ttest_path_is_missing \"$HOME/.config/git/credentials\"\n+\ttest_path_is_missing \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\techo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=home-user\n+\tpassword=home-pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: return credentials from xdg file if the home files do not have them' '\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\t>\"$HOME/.git-credentials\" &&\n+\techo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=xdg-user\n+\tpassword=xdg-pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: return credentials from home file if xdg files are unreadable' '\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\techo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tchmod -r \"$HOME/.config/git/credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=home-user\n+\tpassword=home-pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'store: If both xdg and home files exist, only store in home file' '\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\t>\"$HOME/.config/git/credentials\" &&\n+\t>\"$HOME/.git-credentials\" &&\n+\tcheck approve store <<-\\EOF &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=store-user\n+\tpassword=store-pass\n+\tEOF\n+\techo \"https://store-user:store-pass@example.com\" >expected &&\n+\ttest_cmp expected \"$HOME/.git-credentials\" &&\n+\ttest_must_be_empty \"$HOME/.config/git/credentials\"\n+'\n+\n+\n+test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\techo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcheck reject store <<-\\EOF &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_must_be_empty \"$HOME/.config/git/credentials\" &&\n+\ttest_must_be_empty \"$HOME/.git-credentials\"\n+'\n+\n test_done\n-- \n2.1.4\n"},{"id":"257962","messageId":"vpqh9tifff9.fsf@anie.imag.fr","threadId":"38839","inReplyTo":"1426662276-8728-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v4 1/4] git-credential-store: support multiple credential files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-18T16:17:46Z","receivedAt":"2015-03-18T16:17:46Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan writes:\n\n> +\t/* Write credential to the filename specified by fns->items[0], thus\n> +\t * creating it */\n\nJust to show I'm following: misformatted multi-line comment.\n\nOther than that, good job!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257963","messageId":"vpq3852ff5j.fsf@anie.imag.fr","threadId":"38839","inReplyTo":"1426662276-8728-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v4 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-18T16:23:36Z","receivedAt":"2015-03-18T16:23:36Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"I would personnally prefer to see this squashed with PATCH 2/4: pushing\nthe \"bisectable history\" principle a bit, the state between patches 2\nand 3 could be considered broken because the code does not do what the\ndocumentation says. And as a reviewer, I like having pieces of docs\nlinked to the patch they document.\n\nPaul Tan writes:\n\n> +Credential storage will per default\n\nNot a native, but \"per default\" sounds weird and \"by default\" seems far\nmore common.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257964","messageId":"vpq3852dzry.fsf@anie.imag.fr","threadId":"38839","inReplyTo":"1426662276-8728-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-18T16:41:05Z","receivedAt":"2015-03-18T16:41:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Similarly to the \"merge doc and code\", I personally prefer seeing code\nand tests in the same patch.\n\nActually, my preference goes for a first patch that introduces the tests\nwith test_expect_failure for things that are not yet implemented (and\nyou can check that tests do not pass yet before you code), and then the\npatch introducing the feature doing\n\n-test_expect_failure\n+test_expect_success\n\nwhich documents quite clearly and concisely that you just made the\nfeature work.\n\nPaul Tan writes:\n\n> +test_expect_success 'if custom XDG_CONFIG_HOME credentials file\n> + ~/xdg/git/credentials exists, it will only be written to;\n> + ~/.config/git/credentials and ~/.git-credentials will not be\n> + created' '\n> +\ttest_when_finished \"rm -f $HOME/xdg/git/credentials\" &&\n> +\ttest -s \"$HOME/xdg/git/credentials\" &&\n> +\ttest_path_is_missing \"$HOME/.config/git/credentials\"\n> +\ttest_path_is_missing \"$HOME/.git-credentials\"\n> +'\n\nBroken &&-chain. That is a real issue that must be fixed.\n\n> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '\n\n\"files are\" -> \"file is\", no?\n\n> +'\n> +\n> +\n> +test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n\nDouble blank line.\n\n\nOn overall, except the broken &&-chain above, the whole series looks\ngood. Feel free to ignore my other nitpicks if you prefer.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257974","messageId":"CAPig+cR-87-dWXUKoKvphrdb_Y=7268G5uerAiPYDsj-UQiuRw@mail.gmail.com","threadId":"38839","inReplyTo":"1426662276-8728-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-18T19:26:42Z","receivedAt":"2015-03-18T19:26:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan wrote:\n> t0302 now tests git-credential-store's support for the XDG user-specific\n> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:\n> ---\n>\n> The previous version can be found at [1].\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308\n>\n> * Merge related, but previously separate, tests together in order to\n> make the test suite easier to understand.\n>\n> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set\n> it, and unset it immediately before and after \"helper_test store\" is\n> called in order to make it localized to only the command that it\n> should affect.\n>\n> * Add test, previously missing, to check that only the home credentials\n> file is written to if both the xdg and home files exist.\n>\n> * Correct mislabelling of \"home-user\"/\"home-pass\" to the proper\n> \"xdg-user\"/\"xdg-pass\".\n>\n> * Use \"rm -f\" instead of \"test_might_fail rm\".\n\nThis round looks much better. Thanks.\n\nMost of the comments below are just nit-picky, with one or two genuine\n(minor) issues.\n\n> Thanks Eric for the code review.\n>\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index f61b40c..5b0a666 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -6,4 +6,115 @@ test_description='credential-store tests'\n>\n> helper_test store\n>\n> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '\n\nThese test descriptions are quite long, often mirroring in prose what\nthe test body already says more concisely. I generally try to keep\ndescriptions short while still being descriptive enough to give a\ngeneral idea about what is being tested. I've rewritten a few test\ndescriptions (below) to be very short, in fact probably too terse; but\nperhaps better descriptions would lie somewhere between the two\nextremes. First example, for this test:\n\n \"!xdg: >.git-credentials !xdg\"\n\nKey: Space-separated items. Items before \":\" are the pre-conditions,\nand items after are the post-state. \"!\" file does not exist; \">\"\noutput goes here.\n\n> + test -s \"$HOME/.git-credentials\" &&\n> + test_path_is_missing \"$HOME/.config/git/credentials\"\n> +'\n> +\n> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n\nIt's customary to call this \"setup\" rather than \"create\".\n\nTerse version: \"setup: -.git-redentials +xdg\"\n\nKey: \"-\" file removed; \"+\" file created.\n\n> + rm -f \"$HOME/.git-credentials\" &&\n> + mkdir -p \"$HOME/.config/git\" &&\n> + >\"$HOME/.config/git/credentials\"\n> +'\n> +\n> +helper_test store\n> +\n> +test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '\n\nTerse version: \"!.git-credentials xdg: !.git-credentials >xdg\"\n\n> + test -s \"$HOME/.config/git/credentials\" &&\n> + test_path_is_missing \"$HOME/.git-credentials\"\n> +'\n> +\n> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '\n\ns/set up/setup/\n\nTerse: \"setup custom-xdg\"\n\n> + mkdir -p \"$HOME/xdg/git\" &&\n> + rm -f \"$HOME/.config/git/credentials\" &&\n> + >\"$HOME/xdg/git/credentials\"\n\nIt would be easier to read this if you placed the two lines together\nwhich refer to the custom xdg file. Also, for completeness and to be\nself-contained, don't you want to remove ~/.git-credentials?\n\n rm -f \"$HOME/.git-credentials\" &&\n rm -f \"$HOME/.config/git/credentials\" &&\n mkdir -p \"$HOME/xdg/git\" &&\n >\"$HOME/xdg/git/credentials\"\n\n> +'\n> +\n> +XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME && helper_test store\n> +unset XDG_CONFIG_HOME\n\nIt's hard to spot the \"helper_test store\" at the end of line. I'd\nplace it on a line by itself so that it is easy to see that it is\nwrapped by the setting and unsetting of the environment variable.\n\n> +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' '\n\nTerse: \"!.git-credentials !xdg custom-xdg: !.git-credentials !xdg >custom-xdg\"\n\n> + test_when_finished \"rm -f $HOME/xdg/git/credentials\" &&\n> + test -s \"$HOME/xdg/git/credentials\" &&\n> + test_path_is_missing \"$HOME/.config/git/credentials\"\n\nMatthieu already pointed out the broken &&-chain.\n\n> + test_path_is_missing \"$HOME/.git-credentials\"\n> +'\n> +\n> +test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '\n\nTerse: \".git-credentials xdg: <.git-credentials\"\n\nKey: \"<\" taken from here.\n\n> + mkdir -p \"$HOME/.config/git\" &&\n> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n> + check fill store <<-\\EOF\n> + protocol=https\n> + host=example.com\n> + --\n> + protocol=https\n> + host=example.com\n> + username=home-user\n> + password=home-pass\n> + --\n> + EOF\n> +'\n> +\n> +test_expect_success 'get: return credentials from xdg file if the home files do not have them' '\n\nTerse: \"!.git-credentials xdg: + mkdir -p \"$HOME/.config/git\" &&\n> + >\"$HOME/.git-credentials\" &&\n> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n> + check fill store <<-\\EOF\n> + protocol=https\n> + host=example.com\n> + --\n> + protocol=https\n> + host=example.com\n> + username=xdg-user\n> + password=xdg-pass\n> + --\n> + EOF\n> +'\n> +\n> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '\n\nAn earlier test showed that the home file is preferred if both it and\nthe xdg file exist, so is this test actually telling us anything new?\nDid you mean instead to reverse the case and make the home file\nunreadable?\n\n> + mkdir -p \"$HOME/.config/git\" &&\n> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n> + chmod -r \"$HOME/.config/git/credentials\" &&\n\nIt would be a bit easier to see that the 'chmod' applies to the xdg\nfile if it directly followed creation of the xdg file.\n\n> + check fill store <<-\\EOF\n> + protocol=https\n> + host=example.com\n> + --\n> + protocol=https\n> + host=example.com\n> + username=home-user\n> + password=home-pass\n> + --\n> + EOF\n> +'\n> +\n> +test_expect_success 'store: If both xdg and home files exist, only store in home file' '\n\nInconsistent capitalization: s/If/if/\n\n> + mkdir -p \"$HOME/.config/git\" &&\n> + >\"$HOME/.config/git/credentials\" &&\n> + >\"$HOME/.git-credentials\" &&\n> + check approve store <<-\\EOF &&\n> + protocol=https\n> + host=example.com\n> + username=store-user\n> + password=store-pass\n> + EOF\n> + echo \"https://store-user:store-pass@example.com\" >expected &&\n> + test_cmp expected \"$HOME/.git-credentials\" &&\n> + test_must_be_empty \"$HOME/.config/git/credentials\"\n> +'\n> +\n> +\n> +test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n> + mkdir -p \"$HOME/.config/git\" &&\n> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n> + check reject store <<-\\EOF &&\n> + protocol=https\n> + host=example.com\n> + EOF\n> + test_must_be_empty \"$HOME/.config/git/credentials\" &&\n> + test_must_be_empty \"$HOME/.git-credentials\"\n> +'\n> +\n> test_done\n> --\n> 2.1.4\n"},{"id":"257982","messageId":"CAPig+cTaQAnWoz7=v9tOPZoeE3LTV+H6KD_tAzmOXi3Tjay_WA@mail.gmail.com","threadId":"38839","inReplyTo":"vpq3852dzry.fsf@anie.imag.fr","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-18T21:15:10Z","receivedAt":"2015-03-18T21:15:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 18, 2015 at 12:41 PM, Matthieu Moy\n wrote:\n> Similarly to the \"merge doc and code\", I personally prefer seeing code\n> and tests in the same patch.\n\nIn this case, the patch introducing the tests is already quite long\nand intricate, almost to the point of being a burden to review.\nCombining the patches would likely push it over the edge. I'd elect to\nkeep them separate.\n\n> Actually, my preference goes for a first patch that introduces the tests\n> with test_expect_failure for things that are not yet implemented (and\n> you can check that tests do not pass yet before you code), and then the\n> patch introducing the feature doing\n>\n> -test_expect_failure\n> +test_expect_success\n>\n> which documents quite clearly and concisely that you just made the\n> feature work.\n\nI also tend to favor adding \"failure\" tests which are flipped to\n\"success\" when appropriate, however, as this is an entirely new\nfeature, this approach may be unsuitable (and perhaps overkill).\nGenerally speaking, these new tests don't really make sense without\nthe feature; and, more specifically, due to their nature, several of\nthem will pass even without the feature implemented. As such, the\ncurrent patch organization seems reasonable.\n"},{"id":"258026","messageId":"vpqy4mtrtxm.fsf@anie.imag.fr","threadId":"38839","inReplyTo":"CAPig+cTaQAnWoz7=v9tOPZoeE3LTV+H6KD_tAzmOXi3Tjay_WA@mail.gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-19T13:35:49Z","receivedAt":"2015-03-19T13:35:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> I also tend to favor adding \"failure\" tests which are flipped to\n> \"success\" when appropriate, however, as this is an entirely new\n> feature, this approach may be unsuitable (and perhaps overkill).\n\nI can buy the \"overkill\", but not unsuitable. Even for new features, the\ntests should fail before and pass after. Otherwise, the tests are not\ntesting the feature. Actually, this is a strong principle in test-driven\ndevelopment: don't write code unless you have a failing test.\n\nBut I was just thinking out loudly, certainly not requesting a change.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"258067","messageId":"CAPig+cS3rZgTZRYsC2ofNAFHs9WsGO1AqW01dt734AZf6edBzQ@mail.gmail.com","threadId":"38839","inReplyTo":"CAPig+cR-87-dWXUKoKvphrdb_Y=7268G5uerAiPYDsj-UQiuRw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-19T21:53:10Z","receivedAt":"2015-03-19T21:53:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 18, 2015 at 3:26 PM, Eric Sunshine wrote:\n> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan wrote:\n>> t0302 now tests git-credential-store's support for the XDG user-specific\n>> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:\n>> ---\n>>\n>> The previous version can be found at [1].\n>>\n>> [1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308\n>>\n>> * Merge related, but previously separate, tests together in order to\n>> make the test suite easier to understand.\n>>\n>> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set\n>> it, and unset it immediately before and after \"helper_test store\" is\n>> called in order to make it localized to only the command that it\n>> should affect.\n>>\n>> * Add test, previously missing, to check that only the home credentials\n>> file is written to if both the xdg and home files exist.\n>>\n>> * Correct mislabelling of \"home-user\"/\"home-pass\" to the proper\n>> \"xdg-user\"/\"xdg-pass\".\n>>\n>> * Use \"rm -f\" instead of \"test_might_fail rm\".\n>\n> This round looks much better. Thanks.\n>\n> Most of the comments below are just nit-picky, with one or two genuine\n> (minor) issues.\n\nI should add that the nit-picky items are not necessarily actionable.\nAs the person doing the actual work, it's okay if you disagree and\nfeel that they are not worth the effort of addressing.\n\n(The genuine issues, on the other hand, ought to be addressed.)\n"},{"id":"258232","messageId":"CACRoPnTXH+HDD42H2ca7b+OPF15v2TxM+y+NRRi2CS_Xg3tGEw@mail.gmail.com","threadId":"38839","inReplyTo":"CAPig+cR-87-dWXUKoKvphrdb_Y=7268G5uerAiPYDsj-UQiuRw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T05:46:13Z","receivedAt":"2015-03-21T05:46:13Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine wrote:\n> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan wrote:\n>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n>> index f61b40c..5b0a666 100755\n>> --- a/t/t0302-credential-store.sh\n>> +++ b/t/t0302-credential-store.sh\n>> @@ -6,4 +6,115 @@ test_description='credential-store tests'\n>>\n>> helper_test store\n>>\n>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '\n>\n> These test descriptions are quite long, often mirroring in prose what\n> the test body already says more concisely. I generally try to keep\n> descriptions short while still being descriptive enough to give a\n> general idea about what is being tested. I've rewritten a few test\n> descriptions (below) to be very short, in fact probably too terse; but\n> perhaps better descriptions would lie somewhere between the two\n> extremes. First example, for this test:\n>\n> \"!xdg: >.git-credentials !xdg\"\n>\n> Key: Space-separated items. Items before \":\" are the pre-conditions,\n> and items after are the post-state. \"!\" file does not exist; \">\"\n> output goes here.\n\nI will make the test descriptions shorter. However, I don't think the\ntest descriptions need to be so terse such that a separate key is\nrequired. e.g. I will shorten the above to \"when xdg file does not\nexist, it's not created.\", or even terser: \"when xdg file not exists,\nit's not created.\". I don't think symbols should be used, as many\nother test descriptions do not use them.\n\n>> + test -s \"$HOME/.git-credentials\" &&\n>> + test_path_is_missing \"$HOME/.config/git/credentials\"\n>> +'\n>> +\n>> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n>\n> It's customary to call this \"setup\" rather than \"create\".\n\nWill fix.\n\n> Terse version: \"setup: -.git-redentials +xdg\"\n>\n> Key: \"-\" file removed; \"+\" file created.\n\nHow about just \"setup xdg file\" (the fact that ~/.git-credentials is\nnot created/deleted is implied in the next test)\n\n>> + rm -f \"$HOME/.git-credentials\" &&\n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + >\"$HOME/.config/git/credentials\"\n>> +'\n>> +\n>> +helper_test store\n>> +\n>> +test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '\n>\n> Terse version: \"!.git-credentials xdg: !.git-credentials >xdg\"\n\nHow about \"when xdg file exists, home file not created\"\n\n>> + test -s \"$HOME/.config/git/credentials\" &&\n>> + test_path_is_missing \"$HOME/.git-credentials\"\n>> +'\n>> +\n>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '\n>\n> s/set up/setup/\n\nWill fix. Thanks.\n\n> Terse: \"setup custom-xdg\"\n\nIt's a matter of taste, but I personally don't see the need for\nhyphenation. How about: \"setup custom xdg file\"\n\n>\n>> + mkdir -p \"$HOME/xdg/git\" &&\n>> + rm -f \"$HOME/.config/git/credentials\" &&\n>> + >\"$HOME/xdg/git/credentials\"\n>\n> It would be easier to read this if you placed the two lines together\n> which refer to the custom xdg file.\n> Also, for completeness and to be\n> self-contained, don't you want to remove ~/.git-credentials?\n\nAh yes, thanks for the suggestion.\n\n> rm -f \"$HOME/.git-credentials\" &&\n> rm -f \"$HOME/.config/git/credentials\" &&\n> mkdir -p \"$HOME/xdg/git\" &&\n> >\"$HOME/xdg/git/credentials\"\n>\n>> +'\n>> +\n>> +XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME && helper_test store\n>> +unset XDG_CONFIG_HOME\n>\n> It's hard to spot the \"helper_test store\" at the end of line. I'd\n> place it on a line by itself so that it is easy to see that it is\n> wrapped by the setting and unsetting of the environment variable.\n\nThanks, will fix. Although now it looks weird that the \"export\" is the\nonly one with a continuation on a single line, so I split all of them\nso that they each have their own line.\n\n>> +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' '\n>\n> Terse: \"!.git-credentials !xdg custom-xdg: !.git-credentials !xdg >custom-xdg\"\n>\n>> + test_when_finished \"rm -f $HOME/xdg/git/credentials\" &&\n>> + test -s \"$HOME/xdg/git/credentials\" &&\n>> + test_path_is_missing \"$HOME/.config/git/credentials\"\n>\n> Matthieu already pointed out the broken &&-chain.\n\nWill fix. Thanks for catching it.\n\n>\n>> + test_path_is_missing \"$HOME/.git-credentials\"\n>> +'\n>> +\n>> +test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '\n>\n> Terse: \".git-credentials xdg: <.git-credentials\"\n>\n> Key: \"<\" taken from here.\n\nHow about \"use home file if both home and xdg files have matches\". I\nwish to make it explicit that which files are used depends on whether\nthey have the matching credential, not if they exist.\n\n>\n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n>> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n>> + check fill store <<-\\EOF\n>> + protocol=https\n>> + host=example.com\n>> + --\n>> + protocol=https\n>> + host=example.com\n>> + username=home-user\n>> + password=home-pass\n>> + --\n>> + EOF\n>> +'\n>> +\n>> +test_expect_success 'get: return credentials from xdg file if the home files do not have them' '\n>\n> Terse: \"!.git-credentials xdg: \n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + >\"$HOME/.git-credentials\" &&\n>> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n>> + check fill store <<-\\EOF\n>> + protocol=https\n>> + host=example.com\n>> + --\n>> + protocol=https\n>> + host=example.com\n>> + username=xdg-user\n>> + password=xdg-pass\n>> + --\n>> + EOF\n>> +'\n>> +\n>> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '\n>\n> An earlier test showed that the home file is preferred if both it and\n> the xdg file exist, so is this test actually telling us anything new?\n> Did you mean instead to reverse the case and make the home file\n> unreadable?\n\nAh yes, this is embarrassing. Apparently this test was written with\nthe old precedence ordering. Thank you so much for catching this fatal\nerror.\n\n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n>> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n>> + chmod -r \"$HOME/.config/git/credentials\" &&\n>\n> It would be a bit easier to see that the 'chmod' applies to the xdg\n> file if it directly followed creation of the xdg file.\n\nWill fix (will chmod the home file instead)\n\n>\n>> + check fill store <<-\\EOF\n>> + protocol=https\n>> + host=example.com\n>> + --\n>> + protocol=https\n>> + host=example.com\n>> + username=home-user\n>> + password=home-pass\n>> + --\n>> + EOF\n>> +'\n>> +\n>> +test_expect_success 'store: If both xdg and home files exist, only store in home file' '\n>\n> Inconsistent capitalization: s/If/if/\n\nWill fix.\n\n>\n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + >\"$HOME/.config/git/credentials\" &&\n>> + >\"$HOME/.git-credentials\" &&\n>> + check approve store <<-\\EOF &&\n>> + protocol=https\n>> + host=example.com\n>> + username=store-user\n>> + password=store-pass\n>> + EOF\n>> + echo \"https://store-user:store-pass@example.com\" >expected &&\n>> + test_cmp expected \"$HOME/.git-credentials\" &&\n>> + test_must_be_empty \"$HOME/.config/git/credentials\"\n>> +'\n>> +\n>> +\n>> +test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n>> + mkdir -p \"$HOME/.config/git\" &&\n>> + echo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n>> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n>> + check reject store <<-\\EOF &&\n>> + protocol=https\n>> + host=example.com\n>> + EOF\n>> + test_must_be_empty \"$HOME/.config/git/credentials\" &&\n>> + test_must_be_empty \"$HOME/.git-credentials\"\n>> +'\n>> +\n>> test_done\n\nThanks so much Eric and Matthieu for the review.\n"},{"id":"258233","messageId":"CACRoPnTqSk=v6ambpsr1+3yP27oVsz4PGkfY8iPrsqgNyyv+yg@mail.gmail.com","threadId":"38839","inReplyTo":"vpq3852ff5j.fsf@anie.imag.fr","subject":"Re: [PATCH v4 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T05:59:28Z","receivedAt":"2015-03-21T05:59:28Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Thu, Mar 19, 2015 at 12:23 AM, Matthieu Moy\n wrote:\n> I would personnally prefer to see this squashed with PATCH 2/4: pushing\n> the \"bisectable history\" principle a bit, the state between patches 2\n> and 3 could be considered broken because the code does not do what the\n> documentation says. And as a reviewer, I like having pieces of docs\n> linked to the patch they document.\n\nYup, I can see what you mean. Will squash on the next version.\n\n> Paul Tan writes:\n>\n>> +Credential storage will per default\n>\n> Not a native, but \"per default\" sounds weird and \"by default\" seems far\n> more common.\n\nAh right, that definitely sounds better. Thanks.\n"},{"id":"258239","messageId":"CACRoPnRC0V6MZvfBBG9sx4vxnC4J1UdyiGoXaxbRHd=PZjC_kA@mail.gmail.com","threadId":"38839","inReplyTo":"vpqy4mtrtxm.fsf@anie.imag.fr","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T10:01:17Z","receivedAt":"2015-03-21T10:01:17Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Thu, Mar 19, 2015 at 9:35 PM, Matthieu Moy\n wrote:\n> Eric Sunshine writes:\n>\n>> I also tend to favor adding \"failure\" tests which are flipped to\n>> \"success\" when appropriate, however, as this is an entirely new\n>> feature, this approach may be unsuitable (and perhaps overkill).\n>\n> I can buy the \"overkill\", but not unsuitable. Even for new features, the\n> tests should fail before and pass after. Otherwise, the tests are not\n> testing the feature. Actually, this is a strong principle in test-driven\n> development: don't write code unless you have a failing test.\n>\n> But I was just thinking out loudly, certainly not requesting a change.\n\nI also think that the tests belong in their own patch as the patch is\nreally long compared to the rest of the patch series and it makes it\neasier for me to respond to comments as well.\n\nPutting the tests before the implementation, though, makes sense in\nthis case as it is just an implementation of a well-defined set of\nrequirements. Some tests will still pass with or without the feature,\nbut that is the requirement of the feature -- which is to not be\nenabled until the user explicitly creates the xdg file. Will re-order\nthe patch series in the next version.\n\nThanks.\n\nRegards,\nPaul\n"},{"id":"258336","messageId":"CAPig+cShTKEVovMRWWPVn7cVMUF7J+ZT_eK4OfOGgn2H4H0TdA@mail.gmail.com","threadId":"38839","inReplyTo":"CACRoPnTXH+HDD42H2ca7b+OPF15v2TxM+y+NRRi2CS_Xg3tGEw@mail.gmail.com","subject":"Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-23T01:56:48Z","receivedAt":"2015-03-23T01:56:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 21, 2015 at 1:46 AM, Paul Tan wrote:\n> On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine wrote:\n>> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan wrote:\n>>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n>>> index f61b40c..5b0a666 100755\n>>> --- a/t/t0302-credential-store.sh\n>>> +++ b/t/t0302-credential-store.sh\n>>> @@ -6,4 +6,115 @@ test_description='credential-store tests'\n>>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '\n>>\n>> These test descriptions are quite long, often mirroring in prose what\n>> the test body already says more concisely. I generally try to keep\n>> descriptions short while still being descriptive enough to give a\n>> general idea about what is being tested. I've rewritten a few test\n>> descriptions (below) to be very short, in fact probably too terse; but\n>> perhaps better descriptions would lie somewhere between the two\n>> extremes. First example, for this test:\n>>\n>> \"!xdg: >.git-credentials !xdg\"\n>>\n> I will make the test descriptions shorter. However, I don't think the\n> test descriptions need to be so terse such that a separate key is\n> required. e.g. I will shorten the above to \"when xdg file does not\n> exist, it's not created.\", or even terser: \"when xdg file not exists,\n> it's not created.\". I don't think symbols should be used, as many\n> other test descriptions do not use them.\n\nYour updated test descriptions all sound fine.\n\n>>> +XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME && helper_test store\n>>> +unset XDG_CONFIG_HOME\n>>\n>> It's hard to spot the \"helper_test store\" at the end of line. I'd\n>> place it on a line by itself so that it is easy to see that it is\n>> wrapped by the setting and unsetting of the environment variable.\n>\n> Thanks, will fix. Although now it looks weird that the \"export\" is the\n> only one with a continuation on a single line, so I split all of them\n> so that they each have their own line.\n\nAn &&-chain outside of a test is not meaningful in this case, so I\nmeant either this:\n\n XDG_CONFIG_HOME=\"$HOME/xdg\"\n export XDG_CONFIG_HOME\n helper_test store\n unset XDG_CONFIG_HOME\n\nor, slightly more compact (using && just to combine the assignment and\nexport on one line):\n\n XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME\n helper_test store\n unset XDG_CONFIG_HOME\n"}]}