{"thread":{"id":"38694","subject":"[PATCH] [GSoC15] git-credentials-store: support XDG config dir","startedAt":"2015-03-03T20:24:57Z","lastAt":"2015-03-06T09:57:13Z","messageCount":11,"participants":["Paul Tan","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256923","messageId":"1425414299-24000-1-git-send-email-pyokagan@gmail.com","threadId":"38694","inReplyTo":null,"subject":"[PATCH] [GSoC15] git-credentials-store: support XDG config dir","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-03T20:24:57Z","receivedAt":"2015-03-03T20:24:57Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"\nHi all,\n\nThis is my initial implementation for the GSoC15 microproject for\nsupporting both ~/.git-credentials and the XDG standard\n$XDG_CONFIG_HOME/git/credentials.\n\nI wrote the XDG tests in t0302-credential-store.sh in the end because it\ndepends on the helper_test and check functions defined in\nlib-credential.sh.\n\nFor your feedback please.\n\nRegards,\nPaul\n"},{"id":"256925","messageId":"1425414299-24000-2-git-send-email-pyokagan@gmail.com","threadId":"38694","inReplyTo":"1425414299-24000-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-03T20:24:58Z","receivedAt":"2015-03-03T20:24:58Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Teach git-credential-store to read/write credentials from\n$XDG_CONFIG_HOME/git/credentials and ~/.git-credentials where\nappropriate:\n\n* get: call lookup_credential() on the XDG file first if it exists. If\n  the credential can't be found, call lookup_credential() on the HOME\n  file.\n* erase: Call remove_credential() on both the XDG file if it exists and\n  the HOME file if it exists.\n* store: If the XDG file exists, call store_credential() on the XDG file\n  and remove_credential() on the HOME file to prevent duplicates.\n* If \"--file\" is provided, use the file for all operations instead.\n\nIn order to support the above, parse_credential_file() now returns 1 if\nit finds a matching credential, and 0 if it does not. Likewise,\nlookup_credential() returns 1 if it could find the credential, and 0 if\nit could not.\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n credential-store.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 45 insertions(+), 15 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 925d3f4..18b8897 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\t\tdie_errno(\"unable to open %s\", fn);\n-\t\treturn;\n+\t\treturn 0;\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@@ -111,8 +114,7 @@ static void remove_credential(const char *fn, struct credential *c)\n \n static int lookup_credential(const char *fn, struct credential *c)\n {\n-\tparse_credential_file(fn, c, print_entry, NULL);\n-\treturn c->username && c->password;\n+\treturn parse_credential_file(fn, c, print_entry, NULL);\n }\n \n int main(int argc, char **argv)\n@@ -124,6 +126,9 @@ int main(int argc, char **argv)\n \tconst char *op;\n \tstruct credential c = CREDENTIAL_INIT;\n \tchar *file = NULL;\n+\tchar *home_file = NULL;\n+\tchar *xdg_file = NULL;\n+\tint ret = 0;\n \tstruct option options[] = {\n \t\tOPT_STRING(0, \"file\", &file, \"path\",\n \t\t\t   \"fetch and store credentials in <path>\"),\n@@ -137,21 +142,46 @@ 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-\t\tdie(\"unable to set up default path; use --file\");\n+\tif (!file) {\n+\t\thome_config_paths(NULL, &xdg_file, \"credentials\");\n+\t\thome_file = expand_user_path(\"~/.git-credentials\");\n+\t\tif (!xdg_file && !home_file)\n+\t\t\tdie(\"unable to set up default path; use --file\");\n+\t}\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-\telse if (!strcmp(op, \"erase\"))\n-\t\tremove_credential(file, &c);\n-\telse if (!strcmp(op, \"store\"))\n-\t\tstore_credential(file, &c);\n-\telse\n+\tif (!strcmp(op, \"get\")) {\n+\t\tif (file) {\n+\t\t\tlookup_credential(file, &c);\n+\t\t} else {\n+\t\t\tif (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n+\t\t\t\tret = lookup_credential(xdg_file, &c);\n+\t\t\tif (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n+\t\t\t\tlookup_credential(home_file, &c);\n+\t\t}\n+\t} else if (!strcmp(op, \"erase\")) {\n+\t\tif (file) {\n+\t\t\tremove_credential(file, &c);\n+\t\t} else {\n+\t\t\tif (xdg_file && access(xdg_file, F_OK) == 0)\n+\t\t\t\tremove_credential(xdg_file, &c);\n+\t\t\tif (home_file && access(home_file, F_OK) == 0)\n+\t\t\t\tremove_credential(home_file, &c);\n+\t\t}\n+\t} else if (!strcmp(op, \"store\")) {\n+\t\tif (file) {\n+\t\t\tstore_credential(file, &c);\n+\t\t} else if (xdg_file && access(xdg_file, F_OK) == 0) {\n+\t\t\tstore_credential(xdg_file, &c);\n+\t\t\tif (home_file && access(home_file, F_OK) == 0 &&\n+\t\t\t    c.protocol && (c.host || c.path) && c.username\n+\t\t\t    && c.password)\n+\t\t\t\tremove_credential(home_file, &c);\n+\t\t} else\n+\t\t\tstore_credential(home_file, &c);\n+\t} else\n \t\t; /* Ignore unknown operation. */\n \n \treturn 0;\n-- \n2.1.4\n"},{"id":"256924","messageId":"1425414299-24000-3-git-send-email-pyokagan@gmail.com","threadId":"38694","inReplyTo":"1425414299-24000-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 2/2] tests: test credential-store XDG support","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-03T20:24:59Z","receivedAt":"2015-03-03T20:24:59Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"* \"helper_test store\" is run for when $XDG_CONFIG_HOME/git/credentials\n  exists and ~/.git-credentials does not and the other way around.\n* Test that credentials are stored in XDG file if both XDG and HOME\n  files exist.\n* Test that credentials from XDG file are used if matching credentials\n  are found in both XDG and HOME files.\n* Test that credentials from HOME file are used if a matching credential\n  could not be found in the XDG file.\n* Test that when erasing credentials, matching credentials in both the\n  XDG and HOME files are erase.d\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n t/t0302-credential-store.sh | 74 +++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 74 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex f61b40c..80893b9 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -4,6 +4,80 @@ test_description='credential-store tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n+# Tests for when ~/.git-credentials exists but\n+# $XDG_CONFIG_HOME/git/credentials does not\n helper_test store\n+test_expect_success '~/.git-credentials is written to when XDG git-credentials does not exist' '\n+\ttest -s \"$HOME/.git-credentials\"\n+'\n+test_expect_success 'XDG credentials will not be created if it does not exist' '\n+\ttest ! -e \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+'\n+\n+# Tests for when $XDG_CONFIG_HOME/git/credentials exists but\n+# ~/.git-credentials does not.\n+rm \"$HOME/.git-credentials\"\n+mkdir -p \"${XDG_CONFIG_HOME:-$HOME/.config}/git\"\n+echo '' > \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+helper_test store\n+test_expect_success '~/.git-credentials will not be created if XDG git-credentials exist' '\n+\ttest ! -e \"$HOME/.git-credentials\"\n+'\n+test_expect_success 'XDG credentials will be written to if it exists' '\n+\ttest -s \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+'\n+\n+# Tests for when both $XDG_CONFIG_HOME/git/credentials and\n+# ~/.git-credentials exists.\n+echo '' > \"$HOME/.git-credentials\"\n+echo '' > \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '\n+\tcheck approve store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\tusername=store-user\n+\tpassword=store-pass\n+\tEOF\n+\tread contents < \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+\ttest ! -z \"$contents\"\n+\tread contents < \"$HOME/.git-credentials\"\n+\ttest -z \"$contents\"\n+'\n+test_expect_success 'Credentials from XDG file are used if the credentials exist in both XDG and HOME files' '\n+\techo \"https://baduser:badpass@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=store-user\n+\tpassword=store-pass\n+\t--\n+\tEOF\n+'\n+test_expect_success 'Credentials from HOME file are used if the XDG files does not have them' '\n+\techo \"\" > \"${XDG_CONFIG_HOME:-$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=baduser\n+\tpassword=badpass\n+\t--\n+\tEOF\n+'\n+test_expect_success 'Credentials from both XDG and HOME files meeting the criteria are erased' '\n+\tcheck reject $HELPER <<-\\EOF &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\tread contents < \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n+\ttest -z \"$contents\"\n+\tread contents < \"$HOME/.git-credentials\"\n+\ttest -z \"$contents\"\n+'\n \n test_done\n-- \n2.1.4\n"},{"id":"256936","messageId":"vpqwq2x3fmy.fsf@anie.imag.fr","threadId":"38694","inReplyTo":"1425414299-24000-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-03T22:00:53Z","receivedAt":"2015-03-03T22:00:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> Teach git-credential-store to read/write credentials from\n> $XDG_CONFIG_HOME/git/credentials and ~/.git-credentials where\n> appropriate:\n\nThanks for your patch. Below is a partial review. Don't take my comments\nas negative criticisms, they are all directions for improvement. I'm\nactually positively surprised by the quality for a first patch :-).\nLooking forward to your version2.\n\n> * get: call lookup_credential() on the XDG file first if it exists. If\n>   the credential can't be found, call lookup_credential() on the HOME\n>   file.\n> * erase: Call remove_credential() on both the XDG file if it exists and\n>   the HOME file if it exists.\n> * store: If the XDG file exists, call store_credential() on the XDG file\n>   and remove_credential() on the HOME file to prevent duplicates.\n> * If \"--file\" is provided, use the file for all operations instead.\n\nWhen writting a commit message, always insist on _why_ you did what you\ndid, not _what_ you did (the patch already says it). For example, your\nproposal for erase makes sense because if you're using \"erase\", you\nprobably don't want to leave cleartext passwords in another file. But\nyou didn't give the argument.\n\nIn other words: I hate GNU-style changelogs ;-).\n\nAlso, we usually put blank lines between items (read the output of \"git\nlog --no-merges\" in git.git to get an idea of the conventions).\n\n> Likewise,\n> lookup_credential() returns 1 if it could find the credential, and 0 if\n> it could not.\n\nErr, you're changing the calling convention, and you're not the only\ncaller (git grep lookup_credential).\n\nIf you need to change this existing function, best is to start your\nseries with a preparatory patch that does the calling convention change,\nadapts the other caller, and then write your change on top, as [PATCH 2].\n\n> -\tif (!strcmp(op, \"get\"))\n> -\t\tlookup_credential(file, &c);\n> -\telse if (!strcmp(op, \"erase\"))\n> -\t\tremove_credential(file, &c);\n> -\telse if (!strcmp(op, \"store\"))\n> -\t\tstore_credential(file, &c);\n> -\telse\n> +\tif (!strcmp(op, \"get\")) {\n> +\t\tif (file) {\n> +\t\t\tlookup_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n> +\t\t\t\tret = lookup_credential(xdg_file, &c);\n> +\t\t\tif (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n> +\t\t\t\tlookup_credential(home_file, &c);\n> +\t\t}\n> +\t} else if (!strcmp(op, \"erase\")) {\n> +\t\tif (file) {\n> +\t\t\tremove_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access(xdg_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(home_file, &c);\n\nWhy is it somethimes access_or_warn and sometimes just access? (genuine\nquestion)\n\n> +\t\t}\n> +\t} else if (!strcmp(op, \"store\")) {\n> +\t\tif (file) {\n> +\t\t\tstore_credential(file, &c);\n> +\t\t} else if (xdg_file && access(xdg_file, F_OK) == 0) {\n> +\t\t\tstore_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0 &&\n> +\t\t\t    c.protocol && (c.host || c.path) && c.username\n> +\t\t\t    && c.password)\n\nIt would make sense to introduce a helper like sensible_credential(c),\nor sanity_check(c). It could be used in store_credential too.\n\nI'm not convinced you need to remove the credential from home_file if\nthe xdg_file takes precedence. Not saying you shouldn't, but you should\nargue more at least.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"256937","messageId":"vpqegp53f9s.fsf@anie.imag.fr","threadId":"38694","inReplyTo":"1425414299-24000-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 2/2] tests: test credential-store XDG support","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-03T22:08:47Z","receivedAt":"2015-03-03T22:08:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> +# Tests for when $XDG_CONFIG_HOME/git/credentials exists but\n> +# ~/.git-credentials does not.\n\nAs much as possible, put text in the $1 of test_expect_success instead\nof comments.\n\n> +rm \"$HOME/.git-credentials\"\n> +mkdir -p \"${XDG_CONFIG_HOME:-$HOME/.config}/git\"\n> +echo '' > \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n> +helper_test store\n\nDon't write code outside test_expect_success (read \"Do's, don'ts &\nthings to keep in mind\" in t/README).\n\n> +test_expect_success '~/.git-credentials will not be created if XDG git-credentials exist' '\n> +\ttest ! -e \"$HOME/.git-credentials\"\n> +'\n\ntest_path_is_missing (see test-lib-functions.sh)\n\n> +echo '' > \"$HOME/.git-credentials\"\n> +echo '' > \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n\nNot even that outside test_expect_success ;-).\n\nAlso, no space after > (Documentation/CodingGuidelines).\n\n\n> +test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '\n> +\tcheck approve store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=store-user\n> +\tpassword=store-pass\n> +\tEOF\n> +\tread contents < \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n> +\ttest ! -z \"$contents\"\n> +\tread contents < \"$HOME/.git-credentials\"\n\nNo space after <.\n\n> +\ttest -z \"$contents\"\n> +'\n> +test_expect_success 'Credentials from XDG file are used if the\n> +\tcredentials exist in both XDG and HOME files' '\n\nBlank line between tests please.\n\n> +test_expect_success 'Credentials from both XDG and HOME files meeting the criteria are erased' '\n> +\tcheck reject $HELPER <<-\\EOF &&\n> +\tprotocol=https\n> +\thost=example.com\n> +\tEOF\n> +\tread contents < \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n> +\ttest -z \"$contents\"\n> +\tread contents < \"$HOME/.git-credentials\"\n> +\ttest -z \"$contents\"\n> +'\n\nI'd rather do stg like\n\necho >expected\ntest_cmp expected \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\ntest_cmp expected $HOME/.git-credentials\n\nso that a test failure shows a diagnosis.\n\nRegards,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"256943","messageId":"xmqqegp5adnx.fsf@gitster.dls.corp.google.com","threadId":"38694","inReplyTo":"1425414299-24000-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T23:01:38Z","receivedAt":"2015-03-03T23:01:38Z","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> Teach git-credential-store to read/write credentials from\n> $XDG_CONFIG_HOME/git/credentials and ~/.git-credentials where\n> appropriate:\n>\n> * get: call lookup_credential() on the XDG file first if it exists. If\n>   the credential can't be found, call lookup_credential() on the HOME\n>   file.\n> * erase: Call remove_credential() on both the XDG file if it exists and\n>   the HOME file if it exists.\n> * store: If the XDG file exists, call store_credential() on the XDG file\n>   and remove_credential() on the HOME file to prevent duplicates.\n> * If \"--file\" is provided, use the file for all operations instead.\n>\n> In order to support the above, parse_credential_file() now returns 1 if\n> it finds a matching credential, and 0 if it does not. Likewise,\n> lookup_credential() returns 1 if it could find the credential, and 0 if\n> it could not.\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n\nI agree with everything Matthieu said ;-)\n\n> diff --git a/credential-store.c b/credential-store.c\n> index 925d3f4..18b8897 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\t\tdie_errno(\"unable to open %s\", fn);\n> -\t\treturn;\n> +\t\treturn 0;\n\nReturning found_credential here would be easier to read, no?  After\nall, that is why you explicitly initialized it to 0 up there to say\n\"no we haven't found any yet\".\n\n> +\tif (!strcmp(op, \"get\")) {\n> +\t\tif (file) {\n> +\t\t\tlookup_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n> +\t\t\t\tret = lookup_credential(xdg_file, &c);\n> +\t\t\tif (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n> +\t\t\t\tlookup_credential(home_file, &c);\n> +\t\t}\n> +\t} else if (!strcmp(op, \"erase\")) {\n> +\t\tif (file) {\n> +\t\t\tremove_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access(xdg_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(home_file, &c);\n> +\t\t}\n> +\t} else if (!strcmp(op, \"store\")) {\n> +\t\tif (file) {\n> +\t\t\tstore_credential(file, &c);\n> +\t\t} else if (xdg_file && access(xdg_file, F_OK) == 0) {\n> +\t\t\tstore_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0 &&\n> +\t\t\t    c.protocol && (c.host || c.path) && c.username\n> +\t\t\t    && c.password)\n\nIf you have to chomp, do it like this instead:\n\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0 &&\n> +\t\t\t    c.protocol && (c.host || c.path) &&\n> +\t\t\t    c.username && c.password)\n\nIt would make it more clear that the second line is about the URL\nbeing accessed while the last line is about the user.\n\n> +\t\t\t\tremove_credential(home_file, &c);\n> +\t\t} else\n> +\t\t\tstore_credential(home_file, &c);\n\nThe repetitiousness of the above three blocks looked somewhat\ndisturbing, but I guess you cannot avoid it because of the subtle\ndifferences among these codepaths.  When \"getting\", you want to get\nfrom only one place, and while not having an earlier candidate is\nnot an error, an existing but unreadable file deserves a warning.\nWhen \"erasing\", you want to erase from everywhere.\n\nI am not sure if I understand what you are doing in the store\ncodepath.  If your \"get\" will read from xdg (if available) without\nlooking at home anyway, why do you need to remove it from home?\nWouldn't the leftover be ignored anyway?\n"},{"id":"256944","messageId":"xmqqa8ztad7q.fsf@gitster.dls.corp.google.com","threadId":"38694","inReplyTo":"1425414299-24000-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 2/2] tests: test credential-store XDG support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T23:11:21Z","receivedAt":"2015-03-03T23:11:21Z","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> * \"helper_test store\" is run for when $XDG_CONFIG_HOME/git/credentials\n>   exists and ~/.git-credentials does not and the other way around.\n> * Test that credentials are stored in XDG file if both XDG and HOME\n>   files exist.\n> * Test that credentials from XDG file are used if matching credentials\n>   are found in both XDG and HOME files.\n> * Test that credentials from HOME file are used if a matching credential\n>   could not be found in the XDG file.\n> * Test that when erasing credentials, matching credentials in both the\n>   XDG and HOME files are erase.d\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n\nAgain, I agree with everything Matthieu said ;-)\n\n> +test_expect_success 'XDG credentials will not be created if it does not exist' '\n> +\ttest ! -e \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n> +'\n\nYou repeat this ${XDG_CONFIG_HOME:-$HOME/.config} all over, but is\nthat necessary?  The test environment is under your control, so if\nyou know you have XDG_CONFIG_HOME at this point in the test, don't\nuse the fallback.  If you want to test _both_, on the other hand,\nthen test both in separate tests.\n\n> +# Tests for when both $XDG_CONFIG_HOME/git/credentials and\n> +# ~/.git-credentials exists.\n> +echo '' > \"$HOME/.git-credentials\"\n> +echo '' > \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n\nDo you need one empty line in each of these file, or is existence of\nthese two files what you care?  If the latter, then just redirect\ninto it without any command, like this:\n\n\t>\"$HOME/.git-credentials\"\n\n> +test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '\n> +\tcheck approve store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=store-user\n> +\tpassword=store-pass\n> +\tEOF\n> +\tread contents < \"${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials\"\n> +\ttest ! -z \"$contents\"\n\nIn an earlier part of this patch, you used \"test -s FILE\".  Do you\nhave to use \"read $it && test [!] -z $it\" here?\n\n> +\tread contents < \"$HOME/.git-credentials\"\n> +\ttest -z \"$contents\"\n\nMissing && everywhere (not just this test).\n\n\tcheck approve store <<-\\EOF &&\n        ...\n        EOF\n        read ... &&\n        ...\n\nIf \"check approve\" exited with a non-zero status, that won't be\nnoticed as long as .../git/credentials file happens to have the\ncontents that passes the later part of the test, which is not what\nwe want.\n"},{"id":"256981","messageId":"20150304094505.GA15593@peff.net","threadId":"38694","inReplyTo":"1425414299-24000-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-04T09:45:06Z","receivedAt":"2015-03-04T09:45:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 04, 2015 at 04:24:58AM +0800, Paul Tan wrote:\n\n> @@ -111,8 +114,7 @@ static void remove_credential(const char *fn, struct credential *c)\n>  \n>  static int lookup_credential(const char *fn, struct credential *c)\n>  {\n> -\tparse_credential_file(fn, c, print_entry, NULL);\n> -\treturn c->username && c->password;\n> +\treturn parse_credential_file(fn, c, print_entry, NULL);\n>  }\n\nI wondered if we were losing something here, as the return value from\nparse_credential_file is not the same as \"did we find both a username\nand a password\". But then I realized that the existing \"return\" line is\nnonsensical. The \"c\" variable here is our template of what to look for,\nnot the return.\n\nI think this is leftover from an earlier iteration, where our callback\nfilled in the values, rather than directly printing them. Nobody noticed\nbecause we didn't actually look at the return value of lookup_credential\nat all.\n\nSo I think regardless of the end goal, it is nice to see this oddity\ncleaned up.\n\n> +\tif (!strcmp(op, \"get\")) {\n> +\t\tif (file) {\n> +\t\t\tlookup_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n> +\t\t\t\tret = lookup_credential(xdg_file, &c);\n> +\t\t\tif (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n> +\t\t\t\tlookup_credential(home_file, &c);\n> +\t\t}\n> +\t} else if (!strcmp(op, \"erase\")) {\n> +\t\tif (file) {\n> +\t\t\tremove_credential(file, &c);\n> +\t\t} else {\n> +\t\t\tif (xdg_file && access(xdg_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0)\n> +\t\t\t\tremove_credential(home_file, &c);\n> +\t\t}\n\nThe lookup rules here all look sane. Thanks for paying such attention\nto the details. Like Matthieu, I was unclear on the inconsistent use of\naccess_or_warn.\n\nIf we can use the same access variant everywhere, I wonder if it would\nbe more readable to hoist it into a function like:\n\n  int has_config_file(const char *file)\n  {\n\treturn file && access_or_warn(file, F_OK) == 0;\n  }\n\nIt's a tiny function, but then your repetitious \"if\" statements drop\nsome of the noise:\n\n  if (has_config_file(xdg_file))\n\tret = lookup_credential(xdg_file, &c);\n  if (!ret && has_config_file(home_file))\n\tlookup_credential(home_file, &c);\n\n> +\t} else if (!strcmp(op, \"store\")) {\n> +\t\tif (file) {\n> +\t\t\tstore_credential(file, &c);\n> +\t\t} else if (xdg_file && access(xdg_file, F_OK) == 0) {\n> +\t\t\tstore_credential(xdg_file, &c);\n> +\t\t\tif (home_file && access(home_file, F_OK) == 0 &&\n> +\t\t\t    c.protocol && (c.host || c.path) && c.username\n> +\t\t\t    && c.password)\n> +\t\t\t\tremove_credential(home_file, &c);\n\nI like that you take care not to lose information during the migration,\nbut I really don't like that we have to replicate the \"is this a\nfully-formed credential\" logic. I think I'd rather just store the\ncredential in the xdg_file, and leave it be in home_file. The lookup\nwill prefer the xdg version, and if we ever issue an \"erase\" (e.g.,\nbecause the credential changes), it should remove both of them.\n\n-Peff\n"},{"id":"257058","messageId":"CACRoPnTBmqtB+mvx9wFG3EUDRjfrsM==GQsso6V3q9NHH1k-YA@mail.gmail.com","threadId":"38694","inReplyTo":"20150304094505.GA15593@peff.net","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-05T06:26:39Z","receivedAt":"2015-03-05T06:26:39Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi all,\n\nThanks for the review. I apologize for rushing the patch out as I\nwanted to get feedback on the new behavior before committing to any\nmore code changes. I also wanted to quickly clear any doubts that\npeople may have about my coding ability. (Or maybe it created more ;)\n)\n\n\nOn Wed, Mar 4, 2015 at 6:00 AM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>> * get: call lookup_credential() on the XDG file first if it exists. If\n>>   the credential can't be found, call lookup_credential() on the HOME\n>>   file.\n>> * erase: Call remove_credential() on both the XDG file if it exists and\n>>   the HOME file if it exists.\n>> * store: If the XDG file exists, call store_credential() on the XDG file\n>>   and remove_credential() on the HOME file to prevent duplicates.\n>> * If \"--file\" is provided, use the file for all operations instead.\n>\n> When writting a commit message, always insist on _why_ you did what you\n> did, not _what_ you did (the patch already says it). For example, your\n> proposal for erase makes sense because if you're using \"erase\", you\n> probably don't want to leave cleartext passwords in another file. But\n> you didn't give the argument.\n>\n> In other words: I hate GNU-style changelogs ;-).\n>\n> Also, we usually put blank lines between items (read the output of \"git\n> log --no-merges\" in git.git to get an idea of the conventions).\n\nGenerally, I would like git to have full support for the XDG base dir\nspec because it would allow it to become a good citizen in an\necosystem of software which already support the XDG base dir spec by\ndefault. Personally, I am annoyed when a piece of software does not\nstore its configuration into its own separate directory because I\nversion control the configuration of every piece of software (with git\n;) ) in its own directory, and store them as submodules in my home\ndirectory git repo.\n\nKeeping consistent with the behavior of git-config, the presence of\nthe credentials file in the XDG git directory signals that the user\nwants credentials to be stored in the XDG directory rather than the\nHOME directory. However, we keep reading from the home credentials in\ncase the user is using old versions of git on the same home directory.\n\nIn fact, thinking about it again, I think the behavior implemented in\nthe patch may not be suitable. Comments below.\n\n>> Likewise,\n>> lookup_credential() returns 1 if it could find the credential, and 0 if\n>> it could not.\n>\n> Err, you're changing the calling convention, and you're not the only\n> caller (git grep lookup_credential).\n>\n> If you need to change this existing function, best is to start your\n> series with a preparatory patch that does the calling convention change,\n> adapts the other caller, and then write your change on top, as [PATCH 2].\n\nEh? I thought lookup_credential has static linkage. The only other use\nof lookup_credential is in credential-cache--daemon.c, and that has\nits own function definition with static linkage.\n\n>> -     if (!strcmp(op, \"get\"))\n>> -             lookup_credential(file, &c);\n>> -     else if (!strcmp(op, \"erase\"))\n>> -             remove_credential(file, &c);\n>> -     else if (!strcmp(op, \"store\"))\n>> -             store_credential(file, &c);\n>> -     else\n>> +     if (!strcmp(op, \"get\")) {\n>> +             if (file) {\n>> +                     lookup_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n>> +                             ret = lookup_credential(xdg_file, &c);\n>> +                     if (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n>> +                             lookup_credential(home_file, &c);\n>> +             }\n>> +     } else if (!strcmp(op, \"erase\")) {\n>> +             if (file) {\n>> +                     remove_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access(xdg_file, F_OK) == 0)\n>> +                             remove_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0)\n>> +                             remove_credential(home_file, &c);\n>\n> Why is it somethimes access_or_warn and sometimes just access? (genuine\n> question)\n\nFor \"get\" even though the xdg file cannot be read I believe it should\nnot be a fatal error because the credential may be found in the home\nfile. We should still warn the user though because it may not be what\nthe user wants. However, I see now that I mistakenly broke\ncompatibility with the old behavior, which errors out if the home\ncredential file could not be read.\n\nThinking about it again, the behavior could go both ways. The user may\nactually not want the xdg or home credential file to be unreadable,\nand so we should error out if the files are unreadable. This is\nsomething that the community should decide, I think, so opinions\nplease :) Personally, I think we should go with this behavior. If that\nis so, access(file, F_OK) would be used instead of access_or_warn.\n\nFor \"erase\", as stated above, having the xdg credentials file existing\nsignals that the user wants the xdg location to be used. If the xdg\ncredentials file does not exist then git should just skip over it.\nHence, the use of access instead of access_or_warn.\n\n(As a side note, I discovered the lockfile code does not respect write\npermissions on the target file, and will just rename the lockfile\nover. Perhaps git should do that?)\n\n>> +             }\n>> +     } else if (!strcmp(op, \"store\")) {\n>> +             if (file) {\n>> +                     store_credential(file, &c);\n>> +             } else if (xdg_file && access(xdg_file, F_OK) == 0) {\n>> +                     store_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0 &&\n>> +                         c.protocol && (c.host || c.path) && c.username\n>> +                         && c.password)\n>\n> It would make sense to introduce a helper like sensible_credential(c),\n> or sanity_check(c). It could be used in store_credential too.\n\nYes, I agree that the entire code needs to be refactored. The code\nneeds to be moved into store_credential(), remove_credential() and\nlookup_credential() instead of cluttering up main()\n\n> I'm not convinced you need to remove the credential from home_file if\n> the xdg_file takes precedence. Not saying you shouldn't, but you should\n> argue more at least.\n\nIndeed, I committed a reasoning error there. I was thinking about what\nhappens if the user runs \"store\" on the new version of git, then\nswitches to an old one. Actually, \"store\" should write to both, so\nthat users will get the updated credentials regardless of whether they\nuse the old or new git.\n\nIn fact, the priority needs to be changed. To summarize, for\ncompatibility with older versions, git should read from the home\ncredentials file first, then the XDG one (since store will handle\nupdating of both). For writing, git should write to both files. For\nerasing, git should erase from both files.\n\nOn Wed, Mar 4, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> Teach git-credential-store to read/write credentials from\n>> $XDG_CONFIG_HOME/git/credentials and ~/.git-credentials where\n>> appropriate:\n>>\n>> * get: call lookup_credential() on the XDG file first if it exists. If\n>>   the credential can't be found, call lookup_credential() on the HOME\n>>   file.\n>> * erase: Call remove_credential() on both the XDG file if it exists and\n>>   the HOME file if it exists.\n>> * store: If the XDG file exists, call store_credential() on the XDG file\n>>   and remove_credential() on the HOME file to prevent duplicates.\n>> * If \"--file\" is provided, use the file for all operations instead.\n>>\n>> In order to support the above, parse_credential_file() now returns 1 if\n>> it finds a matching credential, and 0 if it does not. Likewise,\n>> lookup_credential() returns 1 if it could find the credential, and 0 if\n>> it could not.\n>>\n>> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n>> ---\n>\n> I agree with everything Matthieu said ;-)\n\n;-)\n\n>> diff --git a/credential-store.c b/credential-store.c\n>> index 925d3f4..18b8897 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>>                                 struct credential *c,\n>>                                 void (*match_cb)(struct credential *),\n>>                                 void (*other_cb)(struct strbuf *))\n>> @@ -14,18 +14,20 @@ static void parse_credential_file(const char *fn,\n>>       FILE *fh;\n>>       struct strbuf line = STRBUF_INIT;\n>>       struct credential entry = CREDENTIAL_INIT;\n>> +     int found_credential = 0;\n>>\n>>       fh = fopen(fn, \"r\");\n>>       if (!fh) {\n>>               if (errno != ENOENT)\n>>                       die_errno(\"unable to open %s\", fn);\n>> -             return;\n>> +             return 0;\n>\n> Returning found_credential here would be easier to read, no?  After\n> all, that is why you explicitly initialized it to 0 up there to say\n> \"no we haven't found any yet\".\n\nActually I think die_errno is a function that does not return at all.\nThe return is just to shut the compiler up. Perhaps I shall comment\nthat.\n\n>> +     if (!strcmp(op, \"get\")) {\n>> +             if (file) {\n>> +                     lookup_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n>> +                             ret = lookup_credential(xdg_file, &c);\n>> +                     if (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n>> +                             lookup_credential(home_file, &c);\n>> +             }\n>> +     } else if (!strcmp(op, \"erase\")) {\n>> +             if (file) {\n>> +                     remove_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access(xdg_file, F_OK) == 0)\n>> +                             remove_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0)\n>> +                             remove_credential(home_file, &c);\n>> +             }\n>> +     } else if (!strcmp(op, \"store\")) {\n>> +             if (file) {\n>> +                     store_credential(file, &c);\n>> +             } else if (xdg_file && access(xdg_file, F_OK) == 0) {\n>> +                     store_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0 &&\n>> +                         c.protocol && (c.host || c.path) && c.username\n>> +                         && c.password)\n>\n> If you have to chomp, do it like this instead:\n>\n>> +                     if (home_file && access(home_file, F_OK) == 0 &&\n>> +                         c.protocol && (c.host || c.path) &&\n>> +                         c.username && c.password)\n\n> It would make it more clear that the second line is about the URL\n> being accessed while the last line is about the user.\n\nAgreed, but I think I will re-factor out the credential check into its\nseparate function.\n\n>\n>> +                             remove_credential(home_file, &c);\n>> +             } else\n>> +                     store_credential(home_file, &c);\n>\n> The repetitiousness of the above three blocks looked somewhat\n> disturbing, but I guess you cannot avoid it because of the subtle\n> differences among these codepaths.  When \"getting\", you want to get\n> from only one place, and while not having an earlier candidate is\n> not an error, an existing but unreadable file deserves a warning.\n> When \"erasing\", you want to erase from everywhere.\n>\n> I am not sure if I understand what you are doing in the store\n> codepath.  If your \"get\" will read from xdg (if available) without\n> looking at home anyway, why do you need to remove it from home?\n> Wouldn't the leftover be ignored anyway?\n\nActually, I think git should write to both files. See above.\n\n\nOn Wed, Mar 4, 2015 at 5:45 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Mar 04, 2015 at 04:24:58AM +0800, Paul Tan wrote:\n>\n>> @@ -111,8 +114,7 @@ static void remove_credential(const char *fn, struct credential *c)\n>>\n>>  static int lookup_credential(const char *fn, struct credential *c)\n>>  {\n>> -     parse_credential_file(fn, c, print_entry, NULL);\n>> -     return c->username && c->password;\n>> +     return parse_credential_file(fn, c, print_entry, NULL);\n>>  }\n>\n> I wondered if we were losing something here, as the return value from\n> parse_credential_file is not the same as \"did we find both a username\n> and a password\". But then I realized that the existing \"return\" line is\n> nonsensical. The \"c\" variable here is our template of what to look for,\n> not the return.\n>\n> I think this is leftover from an earlier iteration, where our callback\n> filled in the values, rather than directly printing them. Nobody noticed\n> because we didn't actually look at the return value of lookup_credential\n> at all.\n>\n> So I think regardless of the end goal, it is nice to see this oddity\n> cleaned up.\n\nHaha, you just reminded me of that issue when I was coding. At first,\nwhen I was rapidly prototyping the changes, I thought that\nlookup_credential would return some kind of true/false value, but then\nI realized it kept on returning the same value all the time ;) Thus, I\nimplemented the function signature change in parse_credential_file().\nTotally forgot about it when posting the patch though.\n\n>> +     if (!strcmp(op, \"get\")) {\n>> +             if (file) {\n>> +                     lookup_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n>> +                             ret = lookup_credential(xdg_file, &c);\n>> +                     if (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n>> +                             lookup_credential(home_file, &c);\n>> +             }\n>> +     } else if (!strcmp(op, \"erase\")) {\n>> +             if (file) {\n>> +                     remove_credential(file, &c);\n>> +             } else {\n>> +                     if (xdg_file && access(xdg_file, F_OK) == 0)\n>> +                             remove_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0)\n>> +                             remove_credential(home_file, &c);\n>> +             }\n>\n> The lookup rules here all look sane. Thanks for paying such attention\n> to the details. Like Matthieu, I was unclear on the inconsistent use of\n> access_or_warn.\n\nI don't think the lookup rules are sane now ;) See above.\n\n> If we can use the same access variant everywhere, I wonder if it would\n> be more readable to hoist it into a function like:\n>\n>   int has_config_file(const char *file)\n>   {\n>         return file && access_or_warn(file, F_OK) == 0;\n>   }\n>\n> It's a tiny function, but then your repetitious \"if\" statements drop\n> some of the noise:\n>\n>   if (has_config_file(xdg_file))\n>         ret = lookup_credential(xdg_file, &c);\n>   if (!ret && has_config_file(home_file))\n>         lookup_credential(home_file, &c);\n\nYes, refactoring is required.\n\n>> +     } else if (!strcmp(op, \"store\")) {\n>> +             if (file) {\n>> +                     store_credential(file, &c);\n>> +             } else if (xdg_file && access(xdg_file, F_OK) == 0) {\n>> +                     store_credential(xdg_file, &c);\n>> +                     if (home_file && access(home_file, F_OK) == 0 &&\n>> +                         c.protocol && (c.host || c.path) && c.username\n>> +                         && c.password)\n>> +                             remove_credential(home_file, &c);\n>\n> I like that you take care not to lose information during the migration,\n> but I really don't like that we have to replicate the \"is this a\n> fully-formed credential\" logic. I think I'd rather just store the\n> credential in the xdg_file, and leave it be in home_file. The lookup\n> will prefer the xdg version, and if we ever issue an \"erase\" (e.g.,\n> because the credential changes), it should remove both of them.\n\nAt first, I simply followed the existing behavior of\nstore_credential/parse_credential_file/credential_check as it will\nremove duplicate credentials (matching on protocol, host, path,\nusername as keys). Now, however, I think that we should just write to\nboth the xdg and home files. See above.\n\nAgain, thanks everyone for taking time to review my code.\n\nRegards,\nPaul\n"},{"id":"257095","messageId":"xmqqk2yv47fn.fsf@gitster.dls.corp.google.com","threadId":"38694","inReplyTo":"CACRoPnTBmqtB+mvx9wFG3EUDRjfrsM==GQsso6V3q9NHH1k-YA@mail.gmail.com","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-05T18:37:16Z","receivedAt":"2015-03-05T18:37:16Z","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> On Wed, Mar 4, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Paul Tan <pyokagan@gmail.com> writes:\n>>\n>>>       struct credential entry = CREDENTIAL_INIT;\n>>> +     int found_credential = 0;\n>>>\n>>>       fh = fopen(fn, \"r\");\n>>>       if (!fh) {\n>>>               if (errno != ENOENT)\n>>>                       die_errno(\"unable to open %s\", fn);\n>>> -             return;\n>>> +             return 0;\n>>\n>> Returning found_credential here would be easier to read, no?  After\n>> all, that is why you explicitly initialized it to 0 up there to say\n>> \"no we haven't found any yet\".\n>\n> Actually I think die_errno is a function that does not return at all.\n> The return is just to shut the compiler up. Perhaps I shall comment\n> that.\n\nCommenting just on this part (I am not agreeing or disagreeing with\nyou on other parts of your message yet).\n\nWhen fopen() fails because we cannot open an existing file for\nreading, then die_errno() will trigger and we stop there, in which\ncase the return will not be reached.\n\nBut when we try to open fn and we fail only because fn does not\nexist, we do not say \"die\".  We instead return to the caller,\ntelling it that we have not found any credential so far in the file\nsupplied by the caller.\n\nSo the return does matter, and spelling that zero with\nfound_credential does matter for readability, I would think.\n"},{"id":"257144","messageId":"vpqfv9ila86.fsf@anie.imag.fr","threadId":"38694","inReplyTo":"CACRoPnTBmqtB+mvx9wFG3EUDRjfrsM==GQsso6V3q9NHH1k-YA@mail.gmail.com","subject":"Re: [PATCH 1/2] git-credential-store: support XDG config dir","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-06T09:57:13Z","receivedAt":"2015-03-06T09:57:13Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> Hi all,\n>\n> Thanks for the review. I apologize for rushing the patch out as I\n> wanted to get feedback on the new behavior before committing to any\n> more code changes.\n\nThere is no problem sending unfinished versions for discussions. If\nunsure, send it as [RFC/PATCH].\n\n>> When writting a commit message, always insist on _why_ [...]\n>\n> Generally, I would like git to have full support for the XDG base dir\n> spec\n\nThe point is not only why you implement XDG spec (which is not very\ncontroversial), but also why you did it the way you did.\n\n> In fact, thinking about it again, I think the behavior implemented in\n> the patch may not be suitable. Comments below.\n\nWritting more arguments in the commit message helps getting these\nthoughts earlier ;-).\n\n>>> Likewise,\n>>> lookup_credential() returns 1 if it could find the credential, and 0 if\n>>> it could not.\n>>\n>> Err, you're changing the calling convention, and you're not the only\n>> caller (git grep lookup_credential).\n>>\n>> If you need to change this existing function, best is to start your\n>> series with a preparatory patch that does the calling convention change,\n>> adapts the other caller, and then write your change on top, as [PATCH 2].\n>\n> Eh? I thought lookup_credential has static linkage. The only other use\n> of lookup_credential is in credential-cache--daemon.c, and that has\n> its own function definition with static linkage.\n\nIndeed, it was only me looking at \"git grep\" too fast. You're right.\n\n>>> -     if (!strcmp(op, \"get\"))\n>>> -             lookup_credential(file, &c);\n>>> -     else if (!strcmp(op, \"erase\"))\n>>> -             remove_credential(file, &c);\n>>> -     else if (!strcmp(op, \"store\"))\n>>> -             store_credential(file, &c);\n>>> -     else\n>>> +     if (!strcmp(op, \"get\")) {\n>>> +             if (file) {\n>>> +                     lookup_credential(file, &c);\n>>> +             } else {\n>>> +                     if (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)\n>>> +                             ret = lookup_credential(xdg_file, &c);\n>>> +                     if (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)\n>>> +                             lookup_credential(home_file, &c);\n>>> +             }\n>>> +     } else if (!strcmp(op, \"erase\")) {\n>>> +             if (file) {\n>>> +                     remove_credential(file, &c);\n>>> +             } else {\n>>> +                     if (xdg_file && access(xdg_file, F_OK) == 0)\n>>> +                             remove_credential(xdg_file, &c);\n>>> +                     if (home_file && access(home_file, F_OK) == 0)\n>>> +                             remove_credential(home_file, &c);\n>>\n>> Why is it somethimes access_or_warn and sometimes just access? (genuine\n>> question)\n>\n> For \"get\" even though the xdg file cannot be read I believe it should\n> not be a fatal error because the credential may be found in the home\n> file. We should still warn the user though because it may not be what\n> the user wants.\n\nIMHO, this would deserve a short comment in the code, e.g. /* Warn the\nuser, but we may recover by finding credential in another file */ or so.\n\nIt's less sensitive, but there was more subtle breakages with the config\nfile (should Git do something at all when the config files can't be read\ncompletely?).\n\n> However, I see now that I mistakenly broke compatibility with the old\n> behavior, which errors out if the home credential file could not be\n> read.\n\nYou changed the behavior, but it's not really a compatibility breakage:\nI doubt people _rely_ on Git dying in this case.\n\nI have no strong opinion on what behavior is the best, I think yours\nmakes sense, but if you go for it, it should be documented in the commit\nmessage (or even better: the change could be extracted into a separate\npatch)\n\n>> I'm not convinced you need to remove the credential from home_file if\n>> the xdg_file takes precedence. Not saying you shouldn't, but you should\n>> argue more at least.\n>\n> Indeed, I committed a reasoning error there. I was thinking about what\n> happens if the user runs \"store\" on the new version of git, then\n> switches to an old one. Actually, \"store\" should write to both, so\n> that users will get the updated credentials regardless of whether they\n> use the old or new git.\n\nActually, your version made sense too. Credentials are usually\nconfidential data that you don't want to replicate too much. One reason\nfor using \"store\" can be that you want to overwrite your old password\nwith something else, and not leave your old password lying around\n(because, e.g. it's the same password you use for your bank account and\nnuclear launch code, and you haven't changed it there yet).\n\n> In fact, the priority needs to be changed. To summarize, for\n> compatibility with older versions, git should read from the home\n> credentials file first, then the XDG one (since store will handle\n> updating of both). For writing, git should write to both files. For\n> erasing, git should erase from both files.\n\nOTOH, if you really want compatibility with old versions, just don't\ncreate .config/git/ and git will still write to ~/.git-credentials,\nright? As long as XDG remains an opt-in feature, I wouldn't care too\nmuch about backward compatibility.\n\nAt some point, I personnally think XDG should become the default, but we\ncan wait as much as needed to do that (and not everybody may agree with\nme here).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}