{"thread":{"id":"38783","subject":"[PATCH v3 0/4] git-credential-store: XDG user-specific config file support","startedAt":"2015-03-11T06:49:09Z","lastAt":"2015-03-18T06:39:39Z","messageCount":17,"participants":["Paul Tan","Eric Sunshine","Jeff King","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"257538","messageId":"1426056553-9364-1-git-send-email-pyokagan@gmail.com","threadId":"38783","inReplyTo":null,"subject":"[PATCH v3 0/4] git-credential-store: XDG user-specific config file support","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-11T06:49:09Z","receivedAt":"2015-03-11T06:49:09Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"The previous patch series can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265042\n\nThe changes for \"git-credential-store: support multiple credential files\" are\nas follows:\n\n* store_credential(), instead of taking an index to the string_list for the\ndefault filename, takes a filename string instead as it leads to a more\nflexible API.\n\nThe changes for \"t0302: test credential-store support for XDG_CONFIG_HOME\" are\nas follows:\n\n* Corrected code style violations: All tests are now separated by newlines.\n\n* After $XDG_CONFIG_HOME is set to \"$HOME/xdg\", use $XDG_CONFIG_HOME directly\nfor all paths instead of \"$HOME/xdg\".\n\nThanks Matthieu for the code review.\n\nThe diff between the previous patch series is as follows:\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 7b22a3a..b00f80f 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -88,7 +88,7 @@ static void store_credential_file(const char *fn, struct credential *c)\n }\n \n static void store_credential(const struct string_list *fns, struct credential *c,\n-\t\t\t     unsigned int default_index)\n+\t\t\t     const char *default_fn)\n {\n \tstruct string_list_item *fn;\n \n@@ -107,8 +107,9 @@ static void store_credential(const struct string_list *fns, struct credential *c\n \t\t\tstore_credential_file(fn->string, c);\n \t\t\treturn;\n \t\t}\n-\t/* Write credential to the filename at default_index, creating it */\n-\tstore_credential_file(fns->items[default_index].string, c);\n+\t/* Write credential to default_fn, thus creating it */\n+\tif (default_fn)\n+\t\tstore_credential_file(default_fn, c);\n }\n \n static void remove_credential(const struct string_list *fns, struct credential *c)\n@@ -182,7 +183,7 @@ int main(int argc, char **argv)\n \telse if (!strcmp(op, \"erase\"))\n \t\tremove_credential(&fns, &c);\n \telse if (!strcmp(op, \"store\"))\n-\t\tstore_credential(&fns, &c, 0);\n+\t\tstore_credential(&fns, &c, fns.items[0].string);\n \telse\n \t\t; /* Ignore unknown operation. */\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 7fe832d..34752ea 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -5,41 +5,53 @@ test_description='credential-store tests'\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n helper_test store\n+\n test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '\n \ttest -s \"$HOME/.git-credentials\"\n '\n+\n test_expect_success 'xdg credentials file will not be created if it does not exist' '\n \ttest_path_is_missing \"$HOME/.config/git/credentials\"\n '\n+\n test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n \ttest_might_fail rm \"$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 'xdg credentials file will be written to if it exists' '\n \ttest -s \"$HOME/.config/git/credentials\"\n '\n+\n test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\n \ttest_path_is_missing \"$HOME/.git-credentials\"\n '\n+\n test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '\n \tXDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME &&\n-\tmkdir -p \"$HOME/xdg/git\" &&\n-\t>\"$HOME/xdg/git/credentials\" &&\n+\tmkdir -p \"$XDG_CONFIG_HOME/git\" &&\n+\t>\"$XDG_CONFIG_HOME/git/credentials\" &&\n \t>\"$HOME/.config/git/credentials\"\n '\n+\n helper_test store\n+\n test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '\n-\tunset XDG_CONFIG_HOME &&\n-\ttest -s \"$HOME/xdg/git/credentials\"\n+\ttest_when_finished \"unset XDG_CONFIG_HOME\" &&\n+\ttest -s \"$XDG_CONFIG_HOME/git/credentials\"\n '\n+\n test_expect_success '~/.config/git/credentials file will not be written to if a custom XDG_CONFIG_HOME is set' '\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n+\n test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\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@@ -55,6 +67,7 @@ test_expect_success 'get: return credentials from home file if matches are found\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@@ -70,6 +83,7 @@ test_expect_success 'get: return credentials from xdg file if the home files do\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@@ -86,6 +100,7 @@ test_expect_success 'get: return credentials from home file if xdg files are unr\n \t--\n \tEOF\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\nPaul Tan (4):\n  git-credential-store: support multiple credential files\n  git-credential-store: support XDG_CONFIG_HOME\n  docs/git-credential-store: document XDG file and precedence\n  t0302: test credential-store support for XDG_CONFIG_HOME\n\n Documentation/git-credential-store.txt |  37 +++++++++++-\n credential-store.c                     |  87 ++++++++++++++++++---------\n t/t0302-credential-store.sh            | 107 +++++++++++++++++++++++++++++++++\n 3 files changed, 202 insertions(+), 29 deletions(-)\n\n-- \n2.1.4\n"},{"id":"257542","messageId":"1426056553-9364-2-git-send-email-pyokagan@gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-11T06:49:10Z","receivedAt":"2015-03-11T06:49:10Z","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\ndefault_fn, thus creating it. For backwards compatibility, default_fn\nshould be \"~/.git-credentials\".\n\nHelped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nPrevious version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265042/focus=265038\n\nThe changes are as follows:\n\n* store_credential(), instead of taking an index to the string_list for the\ndefault filename, takes a filename string instead as it leads to a more\nflexible API.\n\n credential-store.c | 78 +++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 53 insertions(+), 25 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 925d3f4..803bed2 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+\t\t\t     const char *default_fn)\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 default_fn, thus creating it */\n+\tif (default_fn)\n+\t\tstore_credential_file(default_fn, 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_NODUP;\n \tchar *file = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING(0, \"file\", &file, \"path\",\n@@ -139,18 +165,20 @@ 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_nodup(&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, fns.items[0].string);\n \telse\n \t\t; /* Ignore unknown operation. */\n \n-- \n2.1.4\n"},{"id":"257541","messageId":"1426056553-9364-3-git-send-email-pyokagan@gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 2/4] git-credential-store: support XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-11T06:49:11Z","receivedAt":"2015-03-11T06:49:11Z","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 <Matthieu.Moy@grenoble-inp.fr>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\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 803bed2..b00f80f 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_nodup(&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":"257539","messageId":"1426056553-9364-4-git-send-email-pyokagan@gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-11T06:49:12Z","receivedAt":"2015-03-11T06:49:12Z","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\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n Documentation/git-credential-store.txt | 37 ++++++++++++++++++++++++++++++++--\n 1 file changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex bc97071..451c4fa 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -31,10 +31,43 @@ OPTIONS\n \n --file=<path>::\n \n-\tUse `<path>` to store credentials. The file will have its\n+\tUse `<path>` 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 <<FILES>>.\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, as support for this file\n+\twas added fairly recently.\n+\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":"257540","messageId":"1426056553-9364-5-git-send-email-pyokagan@gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-11T06:49:13Z","receivedAt":"2015-03-11T06:49:13Z","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* On the flip side, 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 <Matthieu.Moy@grenoble-inp.fr>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nThe previous version can be found at [1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265042/focus=265041\n\nThe changes are as follows:\n\n* Corrected code style violations: All tests are now separated by newlines.\n\n* After $XDG_CONFIG_HOME is set to \"$HOME/xdg\", use $XDG_CONFIG_HOME directly\nfor all paths instead of \"$HOME/xdg\".\n\nThanks Matthieu for the code review.\n\n t/t0302-credential-store.sh | 107 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 107 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex f61b40c..34752ea 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -6,4 +6,111 @@ test_description='credential-store tests'\n \n helper_test store\n \n+test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '\n+\ttest -s \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'xdg credentials file will not be created if it does not exist' '\n+\ttest_path_is_missing \"$HOME/.config/git/credentials\"\n+'\n+\n+test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n+\ttest_might_fail rm \"$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 'xdg credentials file will be written to if it exists' '\n+\ttest -s \"$HOME/.config/git/credentials\"\n+'\n+\n+test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\n+\ttest_path_is_missing \"$HOME/.git-credentials\"\n+'\n+\n+test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '\n+\tXDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME &&\n+\tmkdir -p \"$XDG_CONFIG_HOME/git\" &&\n+\t>\"$XDG_CONFIG_HOME/git/credentials\" &&\n+\t>\"$HOME/.config/git/credentials\"\n+'\n+\n+helper_test store\n+\n+test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '\n+\ttest_when_finished \"unset XDG_CONFIG_HOME\" &&\n+\ttest -s \"$XDG_CONFIG_HOME/git/credentials\"\n+'\n+\n+test_expect_success '~/.config/git/credentials file will not be written to if a custom XDG_CONFIG_HOME is set' '\n+\ttest_must_be_empty \"$HOME/.config/git/credentials\"\n+'\n+\n+test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\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://home-user:home-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: 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 '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":"257545","messageId":"CAPig+cTvX-yZpwC8BvN2CmgkBrOKUaotvJHdwXwfU8YnqLi4nQ@mail.gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v3 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-11T07:47:02Z","receivedAt":"2015-03-11T07:47:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> git-credential-store now supports an additional default credential file\n> at $XDG_CONFIG_HOME/git/credentials. However, ~/.git-credentials takes\n> precedence over it for backwards compatibility. To make the precedence\n> ordering explicit, add a new section FILES that lists out the credential\n> file paths in their order of precedence, and explains how the ordering\n> affects the lookup, storage and erase operations.\n>\n> Also update documentation for --store to briefly explain the operations\n> on multiple files if the --store option is not provided.\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index bc97071..451c4fa 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -31,10 +31,43 @@ OPTIONS\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> +       User-specific credentials file.\n> +\n> +$XDG_CONFIG_HOME/git/credentials::\n> +       Second user-specific credentials file. If '$XDG_CONFIG_HOME' is not set\n> +       or empty, `$HOME/.config/git/credentials` will be used. Any credentials\n> +       stored in this file will not be used if `~/.git-credentials` has a\n> +       matching credential as well. It is a good idea not to create this file\n> +       if you sometimes use older versions of Git, as support for this file\n> +       was added fairly recently.\n\nThe final sentence won't age well: \"fairly recently\" is too nebulous.\nIt may be sufficient merely to advise the reader to avoid this file if\nshe also uses an older version of Git which doesn't support XDG for\ncredentials.\n\nOther than this minor point, the patch series seems well prepared and\nquite nicely done. Thanks.\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> --\n> 2.1.4\n"},{"id":"257551","messageId":"CAPig+cSZpYX0BZFN3Yx=PTiE3VVVVPH9X3yTwKe7p5J=5Dsngg@mail.gmail.com","threadId":"38783","inReplyTo":"1426056553-9364-5-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v3 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-11T08:40:49Z","receivedAt":"2015-03-11T08:40:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> 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> * On the flip side, 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\nRegarding the final sentence: I don't see a test corresponding to the\nrestriction that only ~/.git-credentials will be modified if both\nfiles exist. Am I missing something?\n\nMore below.\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 <Matthieu.Moy@grenoble-inp.fr>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index f61b40c..34752ea 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -6,4 +6,111 @@ test_description='credential-store tests'\n>\n>  helper_test store\n>\n> +test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '\n> +       test -s \"$HOME/.git-credentials\"\n> +'\n> +\n> +test_expect_success 'xdg credentials file will not be created if it does not exist' '\n> +       test_path_is_missing \"$HOME/.config/git/credentials\"\n\nSince these two tests are complementary, each checking a facet of the\nsame behavioral rule, it seems like they ought to be combined. For\npeople reading the tests, having them separate implies incorrectly\nthat they are unrelated, making it difficult to understand the overall\npicture of how the pieces relate to one another. In prose, you would\ndescribe the behavior as:\n\n    When XDG credentials does not exist, write only to\n    ~/.git-credentials; do not create XDG credentials.\n\nIt's one thought; easy to read and understand. The test should reflect\nthe same intent:\n\n    test_expect_success '...' '\n        test -s \"$HOME/.git-credentials\" &&\n        test_path_is_missing \"$HOME/.config/git/credentials\"\n    '\n\nThe same observation applies to several other tests below.\n\n> +'\n> +\n> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n> +       test_might_fail rm \"$HOME/.git-credentials\" &&\n\nCan this just be \"rm -f $HOME/.git-credentials &&\" instead?\n\n> +       mkdir -p \"$HOME/.config/git\" &&\n> +       >\"$HOME/.config/git/credentials\"\n> +'\n> +\n> +helper_test store\n> +\n> +test_expect_success 'xdg credentials file will be written to if it exists' '\n> +       test -s \"$HOME/.config/git/credentials\"\n> +'\n> +\n> +test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\n> +       test_path_is_missing \"$HOME/.git-credentials\"\n\nDitto: It seems like these two tests should be combined.\n\n> +'\n> +\n> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '\n> +       XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME &&\n> +       mkdir -p \"$XDG_CONFIG_HOME/git\" &&\n> +       >\"$XDG_CONFIG_HOME/git/credentials\" &&\n> +       >\"$HOME/.config/git/credentials\"\n> +'\n> +\n> +helper_test store\n> +\n> +test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '\n> +       test_when_finished \"unset XDG_CONFIG_HOME\" &&\n> +       test -s \"$XDG_CONFIG_HOME/git/credentials\"\n> +'\n\nIt feels wrong to set global state (XDG_CONFIG_HOME) in one test and\nthen clear it later in another test, and it's not obvious to the\ncasual reader that global state is being modified. An alternative\nwould be to set XDG_CONFIG_HOME outside of the first test to which it\napplies, clear it after the final test. In reality, however, it only\nneeds to be defined for the \"helper_test store\" invocation, so it also\ncould be highly localized:\n\n    XDG_CONFIG_HOME=\"$HOME/xdg\"\n    export XDG_CONFIG_HOME\n    helper_test store\n    unset XDG_CONFIG_HOME\n\nA final alternative would be to wrap the block of tests needing\nXDG_CONFIG_HOME within a subshell and set the variable only within the\nsubshell. Then, there's no need to unset it, and it's clear to the\nreader that only the tests within the subshell are affected by it.\n\n> +\n> +test_expect_success '~/.config/git/credentials file will not be written to if a custom XDG_CONFIG_HOME is set' '\n> +       test_must_be_empty \"$HOME/.config/git/credentials\"\n> +'\n> +\n> +test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '\n> +       test_path_is_missing \"$HOME/.git-credentials\"\n\nFor clarity, the three above tests probably ought to be combined.\n\n> +'\n> +\n> +test_expect_success 'get: return credentials from home file if matches are found in both home and xdg 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 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> +       mkdir -p \"$HOME/.config/git\" &&\n> +       >\"$HOME/.git-credentials\" &&\n> +       echo \"https://home-user:home-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n\nIn the other tests, credentials in the XDG file are\n\"xdg-user:xdg-pass\", and credentials in ~/.git-credentials are\n\"home-user:home-pass\". It's not at all clear why, in this test, you\ninstead use \"home-user:home-pass\" for the XDG credentials. It seems\nlike an arbitrary and unnecessary change from the norm, and thus\nconfuses the reader.\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 'get: return credentials from home file if xdg files are unreadable' '\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> +       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 '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":"257585","messageId":"CACRoPnTcrAW7BBdAM_C8mtm+bG5AKM5028L+wHCesjQ16fvHTQ@mail.gmail.com","threadId":"38783","inReplyTo":"CAPig+cSZpYX0BZFN3Yx=PTiE3VVVVPH9X3yTwKe7p5J=5Dsngg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] t0302: test credential-store support for XDG_CONFIG_HOME","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-12T09:32:22Z","receivedAt":"2015-03-12T09:32:22Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nThanks for taking the time to write such a detailed review and\ncatching all of my careless mistakes.\n\nOn Wed, Mar 11, 2015 at 4:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> 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>> * On the flip side, 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> Regarding the final sentence: I don't see a test corresponding to the\n> restriction that only ~/.git-credentials will be modified if both\n> files exist. Am I missing something?\n\nWhoops, the test is indeed missing.\n\n>> +test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '\n>> +       test -s \"$HOME/.git-credentials\"\n>> +'\n>> +\n>> +test_expect_success 'xdg credentials file will not be created if it does not exist' '\n>> +       test_path_is_missing \"$HOME/.config/git/credentials\"\n>\n> Since these two tests are complementary, each checking a facet of the\n> same behavioral rule, it seems like they ought to be combined. For\n> people reading the tests, having them separate implies incorrectly\n> that they are unrelated, making it difficult to understand the overall\n> picture of how the pieces relate to one another. In prose, you would\n> describe the behavior as:\n>\n>     When XDG credentials does not exist, write only to\n>     ~/.git-credentials; do not create XDG credentials.\n>\n> It's one thought; easy to read and understand. The test should reflect\n> the same intent:\n>\n>     test_expect_success '...' '\n>         test -s \"$HOME/.git-credentials\" &&\n>         test_path_is_missing \"$HOME/.config/git/credentials\"\n>     '\n>\n> The same observation applies to several other tests below.\n\nIndeed, it looks much cleaner. Thanks for the suggestion.\n\n>> +'\n>> +\n>> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '\n>> +       test_might_fail rm \"$HOME/.git-credentials\" &&\n>\n> Can this just be \"rm -f $HOME/.git-credentials &&\" instead?\n\nI picked this pattern up in t1306-xdg-files.sh so I thought it is the\nnorm, but turns out it's not. I think I can see the rationale though.\nAt this point in the tests the file ~/.git-credentials is expected to\nexist, so if it does not exist a diagnostic message should be printed\nto stderr (which -f will not do). Whether this actually helps is\npurely theoretical though, so I will change it to \"rm -f\" on the next\niteration.\n\n>> +'\n>> +\n>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '\n>> +       XDG_CONFIG_HOME=\"$HOME/xdg\" && export XDG_CONFIG_HOME &&\n>> +       mkdir -p \"$XDG_CONFIG_HOME/git\" &&\n>> +       >\"$XDG_CONFIG_HOME/git/credentials\" &&\n>> +       >\"$HOME/.config/git/credentials\"\n>> +'\n>> +\n>> +helper_test store\n>> +\n>> +test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '\n>> +       test_when_finished \"unset XDG_CONFIG_HOME\" &&\n>> +       test -s \"$XDG_CONFIG_HOME/git/credentials\"\n>> +'\n>\n> It feels wrong to set global state (XDG_CONFIG_HOME) in one test and\n> then clear it later in another test, and it's not obvious to the\n> casual reader that global state is being modified. An alternative\n> would be to set XDG_CONFIG_HOME outside of the first test to which it\n> applies, clear it after the final test. In reality, however, it only\n> needs to be defined for the \"helper_test store\" invocation, so it also\n> could be highly localized:\n>\n>     XDG_CONFIG_HOME=\"$HOME/xdg\"\n>     export XDG_CONFIG_HOME\n>     helper_test store\n>     unset XDG_CONFIG_HOME\n>\n> A final alternative would be to wrap the block of tests needing\n> XDG_CONFIG_HOME within a subshell and set the variable only within the\n> subshell. Then, there's no need to unset it, and it's clear to the\n> reader that only the tests within the subshell are affected by it.\n\nI agree that the setting of XDG_CONFIG_HOME needs to be more\nlocalized. This is a by-product of me following the \"no code outside\ntests\" rule too strictly. A quick grep for \"export\" in the tests show\nthat quite a lot of tests do use export outside of test scripts,\nthough, so I guess I will change this to export XDG_CONFIG_HOME just\nbefore running \"helper_test store\". Thanks for the suggestion.\n\n>> +test_expect_success 'get: return credentials from xdg file if the home files do not have them' '\n>> +       mkdir -p \"$HOME/.config/git\" &&\n>> +       >\"$HOME/.git-credentials\" &&\n>> +       echo \"https://home-user:home-pass@example.com\" >\"$HOME/.config/git/credentials\" &&\n>\n> In the other tests, credentials in the XDG file are\n> \"xdg-user:xdg-pass\", and credentials in ~/.git-credentials are\n> \"home-user:home-pass\". It's not at all clear why, in this test, you\n> instead use \"home-user:home-pass\" for the XDG credentials. It seems\n> like an arbitrary and unnecessary change from the norm, and thus\n> confuses the reader.\n\nThis is a careless mistake on my part when I was renaming all the\nusernames and passwords to the current form. Thanks for catching it.\n\nRegards,\nPaul\n"},{"id":"257587","messageId":"CACRoPnTM95Sbrcz+s6w9wY5toLQa1WVFDQvi_ZPdaNCXM9PDGw@mail.gmail.com","threadId":"38783","inReplyTo":"CAPig+cTvX-yZpwC8BvN2CmgkBrOKUaotvJHdwXwfU8YnqLi4nQ@mail.gmail.com","subject":"Re: [PATCH v3 3/4] docs/git-credential-store: document XDG file and precedence","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-12T09:50:50Z","receivedAt":"2015-03-12T09:50:50Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Mar 11, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> wrote:\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>> +       User-specific credentials file.\n>> +\n>> +$XDG_CONFIG_HOME/git/credentials::\n>> +       Second user-specific credentials file. If '$XDG_CONFIG_HOME' is not set\n>> +       or empty, `$HOME/.config/git/credentials` will be used. Any credentials\n>> +       stored in this file will not be used if `~/.git-credentials` has a\n>> +       matching credential as well. It is a good idea not to create this file\n>> +       if you sometimes use older versions of Git, as support for this file\n>> +       was added fairly recently.\n>\n> The final sentence won't age well: \"fairly recently\" is too nebulous.\n> It may be sufficient merely to advise the reader to avoid this file if\n> she also uses an older version of Git which doesn't support XDG for\n> credentials.\n\nI copied this part from the documentation of git-config. I couldn't\nfind the exact patch in the archives where \"fairly recently\" was\nintroduced, but I did find this patch[1] where apparently a version\nnumber was supposed to be used instead.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/198837/focus=200552\n\nSo yes, at this point in time I think the sentence should be changed\nto something like \"It is a good idea not to create this file if you\nuse older versions of git that do not support this file.\", although it\nwould be even more useful for users if the version where this feature\nwas introduced is stated as well. This patch series has not even hit\npu though ;)\n\n> Other than this minor point, the patch series seems well prepared and\n> quite nicely done. Thanks.\n\nThank you so much for the positive review. Will re-roll the documentation.\n\nRegards,\nPaul\n"},{"id":"257631","messageId":"20150313061530.GA24588@peff.net","threadId":"38783","inReplyTo":"1426056553-9364-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T06:15:30Z","receivedAt":"2015-03-13T06:15:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 11, 2015 at 02:49:10PM +0800, Paul Tan wrote:\n\n> Previously, git-credential-store only supported storing credentials in a\n> single file: ~/.git-credentials. In order to support the XDG base\n> directory specification[1], git-credential-store needs to be able to\n> lookup and erase credentials from multiple files, as well as to pick the\n> appropriate file to write to so that the credentials can be found on\n> subsequent lookups.\n\nI looked over the code here and in the second patch, and I didn't see\nany real problems. Thanks for iterating on this.\n\nTwo minor nits, that might not even be worth addressing:\n\n> +static void store_credential(const struct string_list *fns, struct credential *c,\n> +\t\t\t     const char *default_fn)\n\nI think you could even get away without passing default_fn here, and\njust use the rule \"the first file in the list is the default\". Unless\nyou are anticipating ever passing something else, but I couldn't think\nof a case where that would be useful.\n\n> +\tstruct string_list fns = STRING_LIST_INIT_NODUP;\n\nThis is declared NODUP...\n\n> -\tif (!file)\n> +\tif (file)\n> +\t\tstring_list_append_nodup(&fns, file);\n\nSo you can just call the regular string_list_append here (the idea of\ndeclaring the list as DUP or NODUP is that the callers do not have to\ncare; string_list_append does the right thing).\n\n-Peff\n"},{"id":"257668","messageId":"CACRoPnR9pTc2LC87Vf0bMAgTj-FnbsRBpjn+3RCxCP6yrzsCkw@mail.gmail.com","threadId":"38783","inReplyTo":"20150313061530.GA24588@peff.net","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-14T08:15:53Z","receivedAt":"2015-03-14T08:15:53Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Mar 13, 2015 at 2:15 PM, Jeff King <peff@peff.net> wrote:\n>> +static void store_credential(const struct string_list *fns, struct credential *c,\n>> +                          const char *default_fn)\n>\n> I think you could even get away without passing default_fn here, and\n> just use the rule \"the first file in the list is the default\". Unless\n> you are anticipating ever passing something else, but I couldn't think\n> of a case where that would be useful.\n\nEven though in this case the store_credential() function is not used\nanywhere else, from my personal API design experience I think that\ncementing the rule of \"the first file in the list is the default\" in\nthe behavior of the function is not a good thing. For example, in the\nfuture, we may wish to keep the precedence ordering the same, but if\nnone of the credential files exist, we create the XDG file by default\ninstead. It's a balance of flexibility, but in this case I think\nputting the default filename in a separate argument is a good thing.\n\n>> +     struct string_list fns = STRING_LIST_INIT_NODUP;\n>\n> This is declared NODUP...\n>\n>> -     if (!file)\n>> +     if (file)\n>> +             string_list_append_nodup(&fns, file);\n>\n> So you can just call the regular string_list_append here (the idea of\n> declaring the list as DUP or NODUP is that the callers do not have to\n> care; string_list_append does the right thing).\n\nActually, thinking about it again from a memory management\nperspective, STRING_LIST_INIT_DUP should be used instead as the\nstring_list `fns` should own the memory of the strings inside it.\nThus, in this case since `file` is pulled from the argv array,\nstring_list_append() should be used to duplicate the memory, and for\nexpand_user_path(), string_list_append_nodup() should be used because\nthe returned path is newly-allocated memory. Finally,\nstring_list_clear() should be called at the end to release memory.\n\nThanks for taking the time to review the patches, I will work on v4\nnow. (Really keen on getting this to pu)\n\nRegards,\nPaul\n"},{"id":"257679","messageId":"20150314173328.GA32599@peff.net","threadId":"38783","inReplyTo":"CACRoPnR9pTc2LC87Vf0bMAgTj-FnbsRBpjn+3RCxCP6yrzsCkw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-14T17:33:29Z","receivedAt":"2015-03-14T17:33:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:\n\n> Even though in this case the store_credential() function is not used\n> anywhere else, from my personal API design experience I think that\n> cementing the rule of \"the first file in the list is the default\" in\n> the behavior of the function is not a good thing. For example, in the\n> future, we may wish to keep the precedence ordering the same, but if\n> none of the credential files exist, we create the XDG file by default\n> instead. It's a balance of flexibility, but in this case I think\n> putting the default filename in a separate argument is a good thing.\n\nYeah, I see your line of reasoning. I think this is probably a case of\nYAGNI, but it is really a matter of personal preference. It's not a big\ndeal either way.\n\n> > So you can just call the regular string_list_append here (the idea of\n> > declaring the list as DUP or NODUP is that the callers do not have to\n> > care; string_list_append does the right thing).\n> \n> Actually, thinking about it again from a memory management\n> perspective, STRING_LIST_INIT_DUP should be used instead as the\n> string_list `fns` should own the memory of the strings inside it.\n> Thus, in this case since `file` is pulled from the argv array,\n> string_list_append() should be used to duplicate the memory, and for\n> expand_user_path(), string_list_append_nodup() should be used because\n> the returned path is newly-allocated memory. Finally,\n> string_list_clear() should be called at the end to release memory.\n\nYeah, I had the same thought, but I noticed that you don't call\nstring_list_clear. Nor is it really necessary to do so. Since git\nprograms are generally short-lived, we often let the OS take care of\ndeallocating variables like this (it's not appropriate for all\nvariables, of course, but quite frequently there are variables that are\neffectively allocated at program startup and used until program end).\n\nAgain, I think this is a matter of taste. I don't mind if you want to\nstring_list_clear at the end, and I agree that using nodup() is the\nright thing in that case.\n\n-Peff\n"},{"id":"257681","messageId":"20150314174204.GA32706@peff.net","threadId":"38783","inReplyTo":"20150314173328.GA32599@peff.net","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-14T17:42:04Z","receivedAt":"2015-03-14T17:42:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 14, 2015 at 01:33:28PM -0400, Jeff King wrote:\n\n> On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:\n> \n> > Even though in this case the store_credential() function is not used\n> > anywhere else, from my personal API design experience I think that\n> > cementing the rule of \"the first file in the list is the default\" in\n> > the behavior of the function is not a good thing. For example, in the\n> > future, we may wish to keep the precedence ordering the same, but if\n> > none of the credential files exist, we create the XDG file by default\n> > instead. It's a balance of flexibility, but in this case I think\n> > putting the default filename in a separate argument is a good thing.\n> \n> Yeah, I see your line of reasoning. I think this is probably a case of\n> YAGNI, but it is really a matter of personal preference. It's not a big\n> deal either way.\n\nBy the way, I hope this (and the other comment) do not come off as \"you\nare wrong, but I do not feel like arguing with you\". I really do think\nthese are a matter of taste, and while we often express issues of taste\nin reviews, it is ultimately up to the patch submitter (who is, after\nall, doing most of the work) to have the final say on minor issues like\nthis.  Sometimes the response to taste issue is \"oh, I didn't think to\ndo that, thanks for the suggestion\" and sometimes it is \"nah, I like it\nbetter my way\". And both are OK.\n\nOf course there are also taste issues where we insist (e.g., consistent\nwhitespace), but I do not think this is one of them.  :)\n\nMaybe that was all obvious, but since you are new to the list, I wanted\nto make sure I was clear.\n\n-Peff\n"},{"id":"257690","messageId":"xmqqfv97w7k5.fsf@gitster.dls.corp.google.com","threadId":"38783","inReplyTo":"CACRoPnR9pTc2LC87Vf0bMAgTj-FnbsRBpjn+3RCxCP6yrzsCkw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-14T22:14:18Z","receivedAt":"2015-03-14T22:14:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n>> I think you could even get away without passing default_fn here, and\n>> just use the rule \"the first file in the list is the default\". Unless\n>> you are anticipating ever passing something else, but I couldn't think\n>> of a case where that would be useful.\n>\n> Even though in this case the store_credential() function is not used\n> anywhere else, from my personal API design experience I think that\n> cementing the rule of \"the first file in the list is the default\" in\n> the behavior of the function is not a good thing. For example, in the\n> future, we may wish to keep the precedence ordering the same, but if\n> none of the credential files exist, we create the XDG file by default\n> instead.\n\nI am not sure if this is not a premature over-engineering---I am not\nconvinced that such a future need will be fulfilled by passing just\na single default_fn this version already passes, or it needs even\nmore parameters that this version does not pass yet, and the\ninterface to the function needs to be updated at that point when you\nneed it _anyways_. One thing that we all agree is that we don't need\nthe extra parameter within the context of what the current code does.\n\nSo, it smells like a case of YAGNI a bit, at least to me.\n"},{"id":"257715","messageId":"vpqbnju7aex.fsf@anie.imag.fr","threadId":"38783","inReplyTo":"xmqqfv97w7k5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-15T11:44:06Z","receivedAt":"2015-03-15T11:44:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>>> I think you could even get away without passing default_fn here, and\n>>> just use the rule \"the first file in the list is the default\". Unless\n>>> you are anticipating ever passing something else, but I couldn't think\n>>> of a case where that would be useful.\n>>\n>> Even though in this case the store_credential() function is not used\n>> anywhere else, from my personal API design experience I think that\n>> cementing the rule of \"the first file in the list is the default\" in\n>> the behavior of the function is not a good thing. For example, in the\n>> future, we may wish to keep the precedence ordering the same, but if\n>> none of the credential files exist, we create the XDG file by default\n>> instead.\n>\n> I am not sure if this is not a premature over-engineering\n\nI would say so if having this default_fn made the code more complex, but\nhere the code is basically\n\n+\tif (default_fn)\n+\t\tstore_credential_file(default_fn, c);\n\nand\n\n-\t\tstore_credential(file, &c);\n+\t\tstore_credential(&fns, &c, fns.items[0].string);\n\nTaking the first element in the list wouldn't change much.\n\nI'm personally fine with both versions.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257743","messageId":"xmqqy4myt4dt.fsf@gitster.dls.corp.google.com","threadId":"38783","inReplyTo":"vpqbnju7aex.fsf@anie.imag.fr","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T20:03:26Z","receivedAt":"2015-03-15T20:03:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Paul Tan <pyokagan@gmail.com> writes:\n>>\n>>>> I think you could even get away without passing default_fn here, and\n>>>> just use the rule \"the first file in the list is the default\". Unless\n>>>> you are anticipating ever passing something else, but I couldn't think\n>>>> of a case where that would be useful.\n>>>\n>>> Even though in this case the store_credential() function is not used\n>>> anywhere else, from my personal API design experience I think that\n>>> cementing the rule of \"the first file in the list is the default\" in\n>>> the behavior of the function is not a good thing. For example, in the\n>>> future, we may wish to keep the precedence ordering the same, but if\n>>> none of the credential files exist, we create the XDG file by default\n>>> instead.\n>>\n>> I am not sure if this is not a premature over-engineering\n>\n> I would say so if having this default_fn made the code more complex,\n\nTrue, or if it made caller(s) be redundant or repeat themselves\nwithout a good reason.  Otherwise we would end up having to drop the\nredundant and/or unnecessary arguments in future clean-up patches; I\nhad to de-conflict a series with 7ce7c760 (convert: drop arguments\nother than 'path' from would_convert_to_git(), 2014-08-21) recently,\nwhich reminded me of this point ;-).\n\n> but\n> here the code is basically\n>\n> +\tif (default_fn)\n> +\t\tstore_credential_file(default_fn, c);\n>\n> and\n>\n> -\t\tstore_credential(file, &c);\n> +\t\tstore_credential(&fns, &c, fns.items[0].string);\n>\n> Taking the first element in the list wouldn't change much.\n>\n> I'm personally fine with both versions.\n\nTurning the current code to drop the extra parameter and to use the\nfirst element instead wouldn't be a big change, but these things\ntend to add up, so unless this discussion immediately lead to a\nfuture enhancement plan to make use of the flexibility the parameter\ngives us, I'd rather see things kept simpler.\n\nI do not terribly mind either way, either, though.\n"},{"id":"257906","messageId":"CACRoPnRpSV9q1A24=8jP1bRCf5sTZRY7owWqMtWCGq-R1b5Rjg@mail.gmail.com","threadId":"38783","inReplyTo":"xmqqy4myt4dt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/4] git-credential-store: support multiple credential files","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-18T06:39:39Z","receivedAt":"2015-03-18T06:39:39Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi all, thanks for providing your feedback.\n\nOn Sun, Mar 15, 2015 at 6:14 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> I am not sure if this is not a premature over-engineering---I am not\n> convinced that such a future need will be fulfilled by passing just\n> a single default_fn this version already passes, or it needs even\n> more parameters that this version does not pass yet, and the\n> interface to the function needs to be updated at that point when you\n> need it _anyways_. One thing that we all agree is that we don't need\n> the extra parameter within the context of what the current code does.\n\nAfter considering everyone's responses, I've decided to remove the\nargument in the v4 patch. As Junio says, when there is a policy change\nthe code can be modified anyway.\n\nRegards,\nPaul\n"}]}