{"thread":{"id":"38900","subject":"[PATCH v5 1/3] git-credential-store: support multiple credential files","startedAt":"2015-03-24T05:20:27Z","lastAt":"2015-03-26T13:27:38Z","messageCount":11,"participants":["Paul Tan","Matthieu Moy","Eric Sunshine","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"258397","messageId":"1427174429-5876-1-git-send-email-pyokagan@gmail.com","threadId":"38900","inReplyTo":null,"subject":"[PATCH v5 1/3] git-credential-store: support multiple credential files","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-24T05:20:27Z","receivedAt":"2015-03-24T05:20:27Z","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/265683\n\nThe changes are as follows:\n\n* Fix comment formatting error. Thanks Matthieu.\n\n credential-store.c | 81 +++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 56 insertions(+), 25 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 925d3f4..d700a93 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,37 @@ 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/*\n+\t * Write credential to the filename specified by fns->items[0], thus\n+\t * creating it\n+\t */\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 +126,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 +150,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 +167,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":"258398","messageId":"1427174429-5876-2-git-send-email-pyokagan@gmail.com","threadId":"38900","inReplyTo":"1427174429-5876-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v5 2/3] git-credential-store: support XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-24T05:20:28Z","receivedAt":"2015-03-24T05:20:28Z","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\nTo make this precedence explicit in docs/git-credential-store, add a new\nsection FILES that lists out the credential file paths in their order of\nprecedence, and explain how the ordering affects the lookup, storage and\nerase operations.\n\nAlso, update the documentation for --file to briefly explain the\noperations on multiple files if the --file option is not provided.\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 \nHelped-by: Eric Sunshine \nSigned-off-by: Paul Tan \n---\n\nThis patch is a combination of 2 patches [1][2].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265683/focus=265686\n\n[2] http://thread.gmane.org/gmane.comp.version-control.git/265683/focus=265685\n\nThe changes are as follows:\n\n* s/per default/by default/ in the documentation. Thanks Matthieu.\n\n Documentation/git-credential-store.txt | 35 ++++++++++++++++++++++++++++++++--\n credential-store.c | 13 +++++++++----\n 2 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex bc97071..e3c8f27 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 by 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 --------\ndiff --git a/credential-store.c b/credential-store.c\nindex d700a93..8b22251 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -165,11 +165,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":"258399","messageId":"1427174429-5876-3-git-send-email-pyokagan@gmail.com","threadId":"38900","inReplyTo":"1427174429-5876-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-24T05:20:29Z","receivedAt":"2015-03-24T05:20:29Z","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/265683/focus=265684\n\nThe changes are as follows:\n\n* Fix broken && chains\n\n* Rewrite test descriptions to be shorter\n\n* Code formatting\n\n* Fixed test \"get: return credentials from home file if xdg files are\nunreadable\". It should actually be the reverse to follow the actual\nprecedence order.\n\nThanks Eric and Matthieu for the code review.\n\nMatthieu and Eric: I know I said I will try to re-order the patches to\nput the tests before the implementation, but after thinking and trying\nto rewrite the commit messages I realised it seems really weird to me.\nIn this patch series, the implementation is split across the first two\npatches. The first patch should use the old tests, and ideally, the new\ntests should be squashed with the second patch because it seems more\nlogical to me to implement the tests at the same time as the new\nfeature. However, since the tests patch is very long, to make it easier\nto review it is split into a separate patch which is applied after the\nimplementation patches.\n\n t/t0302-credential-store.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 114 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex f61b40c..4e1f8ec 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -6,4 +6,118 @@ test_description='credential-store tests'\n \n helper_test store\n \n+test_expect_success 'when xdg file does not exist, xdg file not created' '\n+\ttest_path_is_missing \"$HOME/.config/git/credentials\" &&\n+\ttest -s \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'setup xdg 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 file exists, home file not created' '\n+\ttest -s \"$HOME/.config/git/credentials\" &&\n+\ttest_path_is_missing \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'setup custom xdg file' '\n+\trm -f \"$HOME/.git-credentials\" &&\n+\trm -f \"$HOME/.config/git/credentials\" &&\n+\tmkdir -p \"$HOME/xdg/git\" &&\n+\t>\"$HOME/xdg/git/credentials\"\n+'\n+\n+XDG_CONFIG_HOME=\"$HOME/xdg\"\n+export XDG_CONFIG_HOME\n+helper_test store\n+unset XDG_CONFIG_HOME\n+\n+test_expect_success 'if custom xdg file exists, home and xdg files not created' '\n+\ttest_when_finished \"rm -f $HOME/xdg/git/credentials\" &&\n+\ttest -s \"$HOME/xdg/git/credentials\" &&\n+\ttest_path_is_missing \"$HOME/.git-credentials\" &&\n+\ttest_path_is_missing \"$HOME/.config/git/credentials\"\n+'\n+\n+test_expect_success 'get: use home file if both home and xdg files have matches' '\n+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\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=home-user\n+\tpassword=home-pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: use xdg file if home file has no matches' '\n+\t>\"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\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: use xdg file if home file is unreadable' '\n+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tchmod -r \"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\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 'store: if both xdg and home files exist, only store in home file' '\n+\t>\"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\t>\"$HOME/.config/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+\techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tmkdir -p \"$HOME/.config/git\" &&\n+\techo \"https://xdg-user:xdg-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n+\tcheck reject store <<-\\EOF &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_must_be_empty \"$HOME/.git-credentials\" &&\n+\ttest_must_be_empty \"$HOME/.config/git/credentials\"\n+'\n+\n test_done\n-- \n2.1.4\n"},{"id":"258403","messageId":"vpqbnjivi2n.fsf@anie.imag.fr","threadId":"38900","inReplyTo":"1427174429-5876-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-24T09:52:00Z","receivedAt":"2015-03-24T09:52:00Z","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> Matthieu and Eric: I know I said I will try to re-order the patches to\n> put the tests before the implementation, but after thinking and trying\n> to rewrite the commit messages I realised it seems really weird to me.\n> In this patch series, the implementation is split across the first two\n> patches. The first patch should use the old tests, and ideally, the new\n> tests should be squashed with the second patch because it seems more\n> logical to me to implement the tests at the same time as the new\n> feature. However, since the tests patch is very long, to make it easier\n> to review it is split into a separate patch which is applied after the\n> implementation patches.\n\nNo problem, your version is very good. I was pointing out alternatives,\nbut not requesting a change, and your reasoning makes perfect sense.\n\nI had reviewed v4 in details, and checked the diff between v4 and v5.\nThe whole series is now\n\nReviewed-by: Matthieu Moy \n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"258487","messageId":"CAPig+cTWnLYU8wU+WjEVpDBuWS7of=wxrWnq7b9W-r=wQh5S1A@mail.gmail.com","threadId":"38900","inReplyTo":"1427174429-5876-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-25T06:42:21Z","receivedAt":"2015-03-25T06:42:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 24, 2015 at 1:20 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> * 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>\n> Helped-by: Matthieu Moy \n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> Signed-off-by: Paul Tan \n> ---\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index f61b40c..4e1f8ec 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -6,4 +6,118 @@ test_description='credential-store tests'\n>\n> helper_test store\n>\n> +test_expect_success 'get: use xdg file if home file is unreadable' '\n\nI meant to mention this earlier. Does this test need to be protected\nby the POSIXPERM prerequisite since it's using chmod?\n\n test_expect_success POSIXPERM 'get: ... unreadable' '\n\nOtherwise, the test will likely fail on Windows.\n\n> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n> + chmod -r \"$HOME/.git-credentials\" &&\n> + mkdir -p \"$HOME/.config/git\" &&\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"},{"id":"258489","messageId":"CAPig+cQ-yLnjrsB1E-7=UXfGzuJHat6YtfS8EVRNP2dcjj_6TA@mail.gmail.com","threadId":"38900","inReplyTo":"vpqbnjivi2n.fsf@anie.imag.fr","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-25T06:54:42Z","receivedAt":"2015-03-25T06:54:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 24, 2015 at 5:52 AM, Matthieu Moy\n wrote:\n> Paul Tan writes:\n>\n>> Matthieu and Eric: I know I said I will try to re-order the patches to\n>> put the tests before the implementation, but after thinking and trying\n>> to rewrite the commit messages I realised it seems really weird to me.\n>> In this patch series, the implementation is split across the first two\n>> patches. The first patch should use the old tests, and ideally, the new\n>> tests should be squashed with the second patch because it seems more\n>> logical to me to implement the tests at the same time as the new\n>> feature. However, since the tests patch is very long, to make it easier\n>> to review it is split into a separate patch which is applied after the\n>> implementation patches.\n>\n> No problem, your version is very good. I was pointing out alternatives,\n> but not requesting a change, and your reasoning makes perfect sense.\n>\n> I had reviewed v4 in details, and checked the diff between v4 and v5.\n> The whole series is now\n>\n> Reviewed-by: Matthieu Moy \n\nWith the POSIXPERM issue[1] addressed (if necessary), patch 3/3 is also:\n\nReviewed-by: Eric Sunshine \n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/266265\n"},{"id":"258504","messageId":"xmqqwq252grl.fsf@gitster.dls.corp.google.com","threadId":"38900","inReplyTo":"CAPig+cQ-yLnjrsB1E-7=UXfGzuJHat6YtfS8EVRNP2dcjj_6TA@mail.gmail.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-25T16:17:34Z","receivedAt":"2015-03-25T16:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Mar 24, 2015 at 5:52 AM, Matthieu Moy\n> ...\n>> Reviewed-by: Matthieu Moy \n>\n> With the POSIXPERM issue[1] addressed (if necessary), patch 3/3 is also:\n>\n> Reviewed-by: Eric Sunshine \n\nTHanks for a review. Paul, the 3-patch series is already in 'next',\nso please fix these up with a follow-up patch.\n\nThanks.\n"},{"id":"258513","messageId":"55130693.20607@kdbg.org","threadId":"38900","inReplyTo":"CAPig+cTWnLYU8wU+WjEVpDBuWS7of=wxrWnq7b9W-r=wQh5S1A@mail.gmail.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-03-25T19:03:47Z","receivedAt":"2015-03-25T19:03:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.03.2015 um 07:42 schrieb Eric Sunshine:\n> On Tue, Mar 24, 2015 at 1:20 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>> * 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>>\n>> Helped-by: Matthieu Moy \n>> Helped-by: Junio C Hamano \n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Paul Tan \n>> ---\n>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n>> index f61b40c..4e1f8ec 100755\n>> --- a/t/t0302-credential-store.sh\n>> +++ b/t/t0302-credential-store.sh\n>> @@ -6,4 +6,118 @@ test_description='credential-store tests'\n>>\n>> helper_test store\n>>\n>> +test_expect_success 'get: use xdg file if home file is unreadable' '\n>\n> I meant to mention this earlier. Does this test need to be protected\n> by the POSIXPERM prerequisite since it's using chmod?\n>\n> test_expect_success POSIXPERM 'get: ... unreadable' '\n>\n> Otherwise, the test will likely fail on Windows.\n\nWell spotted! The test indeed fails on Windows. POSIXPERM is required.\n\n>> + echo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n>> + chmod -r \"$HOME/.git-credentials\" &&\n>> + mkdir -p \"$HOME/.config/git\" &&\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-- Hannes\n"},{"id":"258520","messageId":"xmqqr3sc25b0.fsf@gitster.dls.corp.google.com","threadId":"38900","inReplyTo":"xmqqwq252grl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-25T20:25:07Z","receivedAt":"2015-03-25T20:25:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> On Tue, Mar 24, 2015 at 5:52 AM, Matthieu Moy\n>> ...\n>>> Reviewed-by: Matthieu Moy \n>>\n>> With the POSIXPERM issue[1] addressed (if necessary), patch 3/3 is also:\n>>\n>> Reviewed-by: Eric Sunshine \n>\n> THanks for a review. Paul, the 3-patch series is already in 'next',\n> so please fix these up with a follow-up patch.\n>\n> Thanks.\n\nI've already queued the following and merged it to 'next'.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Wed, 25 Mar 2015 13:23:21 -0700\nSubject: [PATCH] t0302: \"unreadable\" test needs POSIXPERM\n\nNoticed and fixed by Eric Sunshine, confirmed by Johannes Sixt.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0302-credential-store.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 4e1f8ec..0979df9 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -75,7 +75,7 @@ test_expect_success 'get: use xdg file if home file has no matches' '\n \tEOF\n '\n \n-test_expect_success 'get: use xdg file if home file is unreadable' '\n+test_expect_success POSIXPERM 'get: use xdg file if home file is unreadable' '\n \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n \tchmod -r \"$HOME/.git-credentials\" &&\n \tmkdir -p \"$HOME/.config/git\" &&\n-- \n2.3.4-462-gc8eeafd\n"},{"id":"258534","messageId":"20150326052042.GA20689@yoshi.chippynet.com","threadId":"38900","inReplyTo":"xmqqr3sc25b0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-26T05:20:42Z","receivedAt":"2015-03-26T05:20:42Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Mar 25, 2015 at 01:25:07PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> I've already queued the following and merged it to 'next'.\n\nThanks Matthieu and Eric for your reviews, and Johannes for following up\non this.\n\nWill keep in view XDG support for ~/.git-credential-cache next because I\ndon't like to leave things unfinished, unless we want to keep it around\nas a microproject. Perhaps home_config_paths() can also be\nsimplified/removed because it hardcodes '~/.gitconfig'.\n\nRegards,\nPaul\n"},{"id":"258547","messageId":"vpq384r7ut1.fsf@anie.imag.fr","threadId":"38900","inReplyTo":"20150326052042.GA20689@yoshi.chippynet.com","subject":"Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-26T13:27:38Z","receivedAt":"2015-03-26T13:27:38Z","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> On Wed, Mar 25, 2015 at 01:25:07PM -0700, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> I've already queued the following and merged it to 'next'.\n>\n> Thanks Matthieu and Eric for your reviews, and Johannes for following up\n> on this.\n>\n> Will keep in view XDG support for ~/.git-credential-cache next because I\n> don't like to leave things unfinished, unless we want to keep it around\n> as a microproject. Perhaps home_config_paths() can also be\n> simplified/removed because it hardcodes '~/.gitconfig'.\n\nI may be able to get some students to work on it in May-June (I teach in\nEnsimag, a french computer-science school and offer \"contribute to\nopen-source\" as an end-of-year project), but no guarantee. If you want\nto do it before, just do it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}