{"thread":{"id":"28389","subject":"[PATCH 0/5] credential helper super fun pak","startedAt":"2011-09-14T19:17:04Z","lastAt":"2011-09-18T21:51:22Z","messageCount":12,"participants":["Jeff King","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"175501","messageId":"20110914191704.GA23201@sigill.intra.peff.net","threadId":"28389","inReplyTo":null,"subject":"[PATCH 0/5] credential helper super fun pak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:17:04Z","receivedAt":"2011-09-14T19:17:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a mixed bag of bugfix commits to go on top of what's in\njk/http-auth-keyring.\n\n  [1/5]: docs: minor tweaks to credentials API\n\n    Clarifications in response to comments from Junio on the original\n    series.\n\n  [2/5]: credential-cache: fix expiration calculation corner cases\n\n    This fixes the failures in t0300 on OS X.\n\n  [3/5]: t0300: make askpass tests a little more robust\n  [4/5]: t0300: make alternate username tests more robust\n\n    These two just make the tests more rigorous so that patch 5/5\n    can actually demonstrate the failure it's fixing.\n\n  [5/5]: credential-store: use a better storage format\n\n    This makes multiple usernames on the same host work with\n    credential-store (they already work with credential-cache).\n\n-Peff\n"},{"id":"175502","messageId":"20110914191745.GA28267@sigill.intra.peff.net","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"[PATCH 1/5] docs: minor tweaks to credentials API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:17:45Z","receivedAt":"2011-09-14T19:17:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are in response to Junio's comments:\n\n  1. Clarify how credential rejection works (first we call\n     helpers, then we clear fields, so helpers get fields).\n\n  2. Clarify quoting of keys/values returned from helpers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/api-credentials.txt |   16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt\nindex 335a007..b2eb24c 100644\n--- a/Documentation/technical/api-credentials.txt\n+++ b/Documentation/technical/api-credentials.txt\n@@ -48,10 +48,11 @@ Functions\n `credential_reject`::\n \n \tInform the credential subsystem that the provided credentials\n-\thave been rejected. This will clear the username and password\n-\tfields in `struct credential`, as well as notify any helpers of\n-\tthe rejection (which may, for example, purge the invalid\n-\tcredentials from storage).\n+\thave been rejected. This will notify any helpers of the\n+\trejection (which allows them to, for example, purge the invalid\n+\tcredentials from storage), and then clear the username and\n+\tpassword fields in `struct credential`. It can then be\n+\t`credential_fill`-ed again.\n \n `credential_getpass`::\n \n@@ -98,8 +99,11 @@ command line:\n \n The helper should produce a list of items on stdout, each followed by a\n newline character. Each item should consist of a key-value pair, separated\n-by an `=` (equals) sign. The value may contain any bytes except a\n-newline. When reading the response, git understands the following keys:\n+by an `=` (equals) sign. The key can contain any bytes except `=` or\n+newline. The value may contain any bytes except a newline. In both\n+cases, all bytes are treated as-is (i.e., there is no quoting, and one\n+cannot transmit a value with newline in it). When reading the response,\n+git understands the following keys:\n \n `username`::\n \n-- \n1.7.6.252.ge9c18\n"},{"id":"175503","messageId":"20110914191757.GB28267@sigill.intra.peff.net","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"[PATCH 2/5] credential-cache: fix expiration calculation corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:17:57Z","receivedAt":"2011-09-14T19:17:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The main credential-cache daemon loop calls poll to wait for\na client or to trigger the expiration of credentials. When\nthe last credential we hold expires, we exit.\n\nHowever, there is a corner case: when we first start up, we\nhave no credentials, and are waiting for a client to\nprovide us with one. In this case, we ended up handing\ncomplete junk for the timeout argument to poll(). On some\nsystems, this caused us to just wait a long time for the\nclient (which usually showed up within a second or so). On\nOS X, however, the system quite reasonably complained about\nour junk value with EINVAL.\n\nFixing this is pretty straightforward; we just notice that\nwe have no entries to compare against. However, that bug was\ncovering up another one: our expiration calculation didn't\ngive clients a chance to actually connect and provide us\nwith a credential before we decided that we should exit\nbecause we weren't holding any credentials!\n\nThe new algorithm is:\n\n  1. Sleep until it's time to expire the most recent\n     credential.\n\n  2. If we don't have any credentials yet, wait 30 seconds\n     for a client to contact us and give us one.\n\n  3. After expiring the last credential, wait 30 seconds for\n     a client to provide us with one.\n\nTechnically only parts (1) and (2) are needed to implement\nthe original intended behavior.\n\nBut (3) is a minor optimization that is made easy by the new\ncode. When a client gives us a credential, then removes it\n(e.g., because it had a bogus password), and then gives us\nanother one, we used to exit, forcing the client to start a\nnew daemon instance. Instead, we can just reuse the existing\ndaemon instance.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n credential-cache--daemon.c |   23 +++++++++++++++++++++--\n 1 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex f520347..d6769b1 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -57,20 +57,33 @@ static void remove_credential(const struct credential *c)\n \n static int check_expirations(void)\n {\n+\tstatic unsigned long wait_for_entry_until;\n \tint i = 0;\n \tunsigned long now = time(NULL);\n \tunsigned long next = (unsigned long)-1;\n \n+\t/*\n+\t * Initially give the client 30 seconds to actually contact us\n+\t * and store a credential before we decide there's no point in\n+\t * keeping the daemon around.\n+\t */\n+\tif (!wait_for_entry_until)\n+\t\twait_for_entry_until = now + 30;\n+\n \twhile (i < entries_nr) {\n \t\tif (entries[i].expiration <= now) {\n \t\t\tentries_nr--;\n-\t\t\tif (!entries_nr)\n-\t\t\t\treturn 0;\n \t\t\tfree(entries[i].item.description);\n \t\t\tfree(entries[i].item.unique);\n \t\t\tfree(entries[i].item.username);\n \t\t\tfree(entries[i].item.password);\n \t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n+\t\t\t/*\n+\t\t\t * Stick around 30 seconds in case a new credential\n+\t\t\t * shows up (e.g., because we just removed a failed\n+\t\t\t * one, and we will soon get the correct one).\n+\t\t\t */\n+\t\t\twait_for_entry_until = now + 30;\n \t\t}\n \t\telse {\n \t\t\tif (entries[i].expiration < next)\n@@ -79,6 +92,12 @@ static int check_expirations(void)\n \t\t}\n \t}\n \n+\tif (!entries_nr) {\n+\t\tif (wait_for_entry_until <= now)\n+\t\t\treturn 0;\n+\t\tnext = wait_for_entry_until;\n+\t}\n+\n \treturn next - now;\n }\n \n-- \n1.7.6.252.ge9c18\n"},{"id":"175504","messageId":"20110914191814.GC28267@sigill.intra.peff.net","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"[PATCH 3/5] t0300: make askpass tests a little more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:18:15Z","receivedAt":"2011-09-14T19:18:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our fake askpass always just returned \"askpass-result\";\nlet's actually have it return a different fake value for\nusernames and passwords, to be sure values are getting\nrouted correctly.\n\nAll tests should still pass.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0300-credentials.sh |   77 ++++++++++++++++++++++++-----------------------\n 1 files changed, 39 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5d54976..09d5b15 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -52,7 +52,8 @@ test_expect_success 'setup helper scripts' '\n \tcat >askpass <<-\\EOF &&\n \t#!/bin/sh\n \techo >&2 askpass: $*\n-\techo askpass-result\n+\twhat=`echo $1 | tr A-Z a-z | tr -cd a-z`\n+\techo \"askpass-$what\"\n \tEOF\n \tchmod +x askpass &&\n \tGIT_ASKPASS=askpass &&\n@@ -155,8 +156,8 @@ test_expect_success 'do not bother completing already-full credential' '\n # askpass helper is run, we know the internal getpass is working.\n test_expect_success 'empty methods falls back to internal getpass' '\n \tcheck <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n@@ -166,7 +167,7 @@ test_expect_success 'empty methods falls back to internal getpass' '\n test_expect_success 'internal getpass does not ask for known username' '\n \tcheck --username=foo <<-\\EOF\n \tusername=foo\n-\tpassword=askpass-result\n+\tpassword=askpass-password\n \t--\n \taskpass: Password:\n \tEOF\n@@ -176,7 +177,7 @@ test_expect_success 'internal getpass can pull from config' '\n \tgit config credential.foo.username configured-username\n \tcheck --unique=foo <<-\\EOF\n \tusername=configured-username\n-\tpassword=askpass-result\n+\tpassword=askpass-password\n \t--\n \taskpass: Password:\n \tEOF\n@@ -185,15 +186,15 @@ test_expect_success 'internal getpass can pull from config' '\n test_expect_success 'credential-cache caches password' '\n \ttest_when_finished \"git credential-cache --exit\" &&\n \tcheck --unique=host cache <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tcheck --unique=host cache <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \tEOF\n '\n@@ -201,15 +202,15 @@ test_expect_success 'credential-cache caches password' '\n test_expect_success 'credential-cache requires matching unique token' '\n \ttest_when_finished \"git credential-cache --exit\" &&\n \tcheck --unique=host cache <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tcheck --unique=host2 cache <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n@@ -219,15 +220,15 @@ test_expect_success 'credential-cache requires matching unique token' '\n test_expect_success 'credential-cache requires matching usernames' '\n \ttest_when_finished \"git credential-cache --exit\" &&\n \tcheck --unique=host cache <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tcheck --unique=host --username=other cache <<-\\EOF\n \tusername=other\n-\tpassword=askpass-result\n+\tpassword=askpass-password\n \t--\n \taskpass: Password:\n \tEOF\n@@ -236,16 +237,16 @@ test_expect_success 'credential-cache requires matching usernames' '\n test_expect_success 'credential-cache times out' '\n \ttest_when_finished \"git credential-cache --exit || true\" &&\n \tcheck --unique=host \"cache --timeout=1\" <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tsleep 2 &&\n \tcheck --unique=host cache <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n@@ -255,18 +256,18 @@ test_expect_success 'credential-cache times out' '\n test_expect_success 'credential-cache removes rejected credentials' '\n \ttest_when_finished \"git credential-cache --exit || true\" &&\n \tcheck --unique=host cache <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n-\tcheck --reject --unique=host --username=askpass-result cache <<-\\EOF &&\n+\tcheck --reject --unique=host --username=askpass-username cache <<-\\EOF &&\n \t--\n \tEOF\n \tcheck --unique=host cache <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n@@ -276,15 +277,15 @@ test_expect_success 'credential-cache removes rejected credentials' '\n test_expect_success 'credential-store stores password' '\n \ttest_when_finished \"rm -f .git-credentials\" &&\n \tcheck --unique=host store <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tcheck --unique=host store <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \tEOF\n '\n@@ -292,15 +293,15 @@ test_expect_success 'credential-store stores password' '\n test_expect_success 'credential-store requires matching unique token' '\n \ttest_when_finished \"rm -f .git-credentials\" &&\n \tcheck --unique=host store <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n \tcheck --unique=host2 store <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n@@ -310,18 +311,18 @@ test_expect_success 'credential-store requires matching unique token' '\n test_expect_success 'credential-store removes rejected credentials' '\n \ttest_when_finished \"rm -f .git-credentials\" &&\n \tcheck --unique=host store <<-\\EOF &&\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n-\tcheck --reject --unique=host --username=askpass-result store <<-\\EOF &&\n+\tcheck --reject --unique=host --username=askpass-username store <<-\\EOF &&\n \t--\n \tEOF\n \tcheck --unique=host store <<-\\EOF\n-\tusername=askpass-result\n-\tpassword=askpass-result\n+\tusername=askpass-username\n+\tpassword=askpass-password\n \t--\n \taskpass: Username:\n \taskpass: Password:\n-- \n1.7.6.252.ge9c18\n"},{"id":"175505","messageId":"20110914191857.GD28267@sigill.intra.peff.net","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"[PATCH 4/5] t0300: make alternate username tests more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:18:58Z","receivedAt":"2011-09-14T19:18:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests is to make sure that if we have a cached\ncredential \"user/pass\", that we ignore it when asking for\n\"--username=other\", and instead get a new password. But\nsince askpass returns only a single fake value, we couldn't\ndifferentiate between using the cached password and the new\none.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0300-credentials.sh |   10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 09d5b15..41ab8cc 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -53,7 +53,11 @@ test_expect_success 'setup helper scripts' '\n \t#!/bin/sh\n \techo >&2 askpass: $*\n \twhat=`echo $1 | tr A-Z a-z | tr -cd a-z`\n-\techo \"askpass-$what\"\n+\tif test -f \"askpass-$what\"; then\n+\t\tcat \"askpass-$what\"\n+\telse\n+\t\techo \"askpass-$what\"\n+\tfi\n \tEOF\n \tchmod +x askpass &&\n \tGIT_ASKPASS=askpass &&\n@@ -226,9 +230,11 @@ test_expect_success 'credential-cache requires matching usernames' '\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n+\ttest_when_finished \"rm -f askpass-password\" &&\n+\techo other-password >askpass-password &&\n \tcheck --unique=host --username=other cache <<-\\EOF\n \tusername=other\n-\tpassword=askpass-password\n+\tpassword=other-password\n \t--\n \taskpass: Password:\n \tEOF\n-- \n1.7.6.252.ge9c18\n"},{"id":"175506","messageId":"20110914192117.GE28267@sigill.intra.peff.net","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"[PATCH 5/5] credential-store: use a better storage format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T19:21:17Z","receivedAt":"2011-09-14T19:21:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the name of simplicity, the credential-store helper\nre-used git's config file format for on-disk storage of\ncredentials. The scheme looked something like this:\n\n  [credential \"unique-context\"]\n\tusername = foo\n\tpassword = bar\n\nThis works fine if you have only one credential for each\ncontext. But there's no good way to store separate passwords\nfor user1@unique-context versus user2@unique-context. For\nend users, this meant that:\n\n  1. Trying to access user2@host when you already have a\n     cached credential for user1@host would return user2's\n     username with user1's password.\n\n  2. The stored result would also mix the credentials.\n\n  3. Rejection would remove all entries for the host,\n     regardless of username.\n\nYou can hack around it by assuming that order is important,\nand that:\n\n  [credential \"unique-context\"]\n\tusername = user1\n\tpassword = pass1\n\tusername = user2\n\tpassword = pass2\n\nrefers to two distinct credentials. But the config code\nisn't well setup for that. You have to keep track of the\nlast username field you parsed, and assume the next password\nfield after it is associated with it. Which isn't too bad.\nBut removing just one credential (which we need to do for\n--reject) is impossible.  You can say \"delete the username\nfield that is user2\", but there is no way to say \"delete\nthe password field that comes right after the username field\nthat is user2\" (it's tempting to find the password for user2\nand say \"delete the password field that is pass2\", but\nthat's not strictly correct either).\n\nInstead, let's define a new, very simple storage format.\nEach line is a credential containing three tokens: unique,\nusername, and password. Each is separated by a space, and\nshell-quoted to avoid ambiguity.\n\nThere's now an additional test that we can differentiate the credentials\ncorrectly (matching the similar test we already have for\ncredential-cache). The test for removing credentials is also enhanced to\ncheck that removing one credential leaves an unrelated one stored.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n credential-store.c     |  119 ++++++++++++++++++++++++++++++++++++++++++------\n t/t0300-credentials.sh |   38 ++++++++++++++-\n 2 files changed, 140 insertions(+), 17 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 8ab8582..aae8e0c 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -2,40 +2,129 @@\n #include \"credential.h\"\n #include \"string-list.h\"\n #include \"parse-options.h\"\n+#include \"quote.h\"\n+\n+static struct lock_file credential_lock;\n+\n+static int parse_credential_file(const char *fn,\n+\t\t\t\t struct credential *c,\n+\t\t\t\t int (*match_cb)(const char *username,\n+\t\t\t\t\t\t const char *password,\n+\t\t\t\t\t\t struct credential *c),\n+\t\t\t\t int (*other_cb)(const char *buf))\n+{\n+\tFILE *fh;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char **argv = NULL;\n+\tint alloc = 0;\n+\tint retval = 0;\n+\n+\tfh = fopen(fn, \"r\");\n+\tif (!fh)\n+\t\treturn errno == ENOENT ? 0 : -1;\n+\n+\twhile (strbuf_getwholeline(&buf, fh, '\\n') != EOF) {\n+\t\tint nr = 0;\n+\t\tchar *pristine = xstrdup(buf.buf);\n+\n+\t\tstrbuf_trim(&buf);\n+\t\tif (!sq_dequote_to_argv(buf.buf, &argv, &nr, &alloc) &&\n+\t\t    nr == 3 &&\n+\t\t    !strcmp(c->unique, argv[0]) &&\n+\t\t    (!c->username || !strcmp(c->username, argv[1]))) {\n+\t\t\tif (match_cb(argv[1], argv[2], c) < 0) {\n+\t\t\t\tretval = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\telse if (other_cb) {\n+\t\t\tif (other_cb(pristine) < 0) {\n+\t\t\t\tretval = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tfree(pristine);\n+\t}\n+\n+\tfree(argv);\n+\tstrbuf_release(&buf);\n+\tfclose(fh);\n+\treturn retval;\n+}\n+\n+\n+static int copy_credential(const char *username, const char *password,\n+\t\t\t   struct credential *c)\n+{\n+\tif (!c->username)\n+\t\tc->username = xstrdup(username);\n+\tfree(c->password);\n+\tc->password = xstrdup(password);\n+\treturn 0;\n+}\n \n static int lookup_credential(const char *fn, struct credential *c)\n {\n-\tconfig_exclusive_filename = fn;\n-\tcredential_from_config(c);\n+\tif (!c->unique)\n+\t\treturn 0;\n+\tparse_credential_file(fn, c, copy_credential, NULL);\n \treturn c->username && c->password;\n }\n \n-static void store_item(const char *fn, const char *unique,\n-\t\t       const char *item, const char *value)\n+static int skip_match(const char *username, const char *password,\n+\t\t      struct credential *c)\n {\n-\tstruct strbuf key = STRBUF_INIT;\n+\treturn 0;\n+}\n \n-\tif (!unique)\n-\t\treturn;\n+static int print_entry(const char *buf)\n+{\n+\treturn write_in_full(credential_lock.fd, buf, strlen(buf));\n+}\n \n-\tconfig_exclusive_filename = fn;\n+static int rewrite_credential_file(const char *fn, struct credential *c,\n+\t\t\t\t   int replace)\n+{\n \tumask(077);\n+\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n+\t\treturn -1;\n+\tif (parse_credential_file(fn, c, skip_match, print_entry) < 0) {\n+\t\trollback_lock_file(&credential_lock);\n+\t\treturn -1;\n+\t}\n+\tif (replace) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint r;\n \n-\tstrbuf_addf(&key, \"credential.%s.%s\", unique, item);\n-\tgit_config_set(key.buf, value);\n-\tstrbuf_release(&key);\n+\t\tsq_quote_buf(&buf, c->unique);\n+\t\tstrbuf_addch(&buf, ' ');\n+\t\tsq_quote_buf(&buf, c->username);\n+\t\tstrbuf_addch(&buf, ' ');\n+\t\tsq_quote_buf(&buf, c->password);\n+\t\tstrbuf_addch(&buf, '\\n');\n+\n+\t\tr = write_in_full(credential_lock.fd, buf.buf, buf.len);\n+\t\tstrbuf_release(&buf);\n+\t\tif (r < 0) {\n+\t\t\trollback_lock_file(&credential_lock);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\treturn commit_lock_file(&credential_lock);\n }\n \n static void store_credential(const char *fn, struct credential *c)\n {\n-\tstore_item(fn, c->unique, \"username\", c->username);\n-\tstore_item(fn, c->unique, \"password\", c->password);\n+\tif (!c->unique || !c->username || !c->password)\n+\t\treturn;\n+\trewrite_credential_file(fn, c, 1);\n }\n \n static void remove_credential(const char *fn, struct credential *c)\n {\n-\tstore_item(fn, c->unique, \"username\", NULL);\n-\tstore_item(fn, c->unique, \"password\", NULL);\n+\tif (!c->unique)\n+\t\treturn;\n+\trewrite_credential_file(fn, c, 0);\n }\n \n int main(int argc, const char **argv)\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 41ab8cc..613c3fb 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -314,6 +314,30 @@ test_expect_success 'credential-store requires matching unique token' '\n \tEOF\n '\n \n+test_expect_success 'credential-store requires matching usernames' '\n+\ttest_when_finished \"rm -f .git-credentials\" &&\n+\tcheck --unique=host store <<-\\EOF &&\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Username:\n+\taskpass: Password:\n+\tEOF\n+\ttest_when_finished \"rm -f askpass-password\" &&\n+\techo other-password >askpass-password &&\n+\tcheck --unique=host --username=other store <<-\\EOF &&\n+\tusername=other\n+\tpassword=other-password\n+\t--\n+\taskpass: Password:\n+\tEOF\n+\tcheck --unique=host --username=askpass-username store <<-\\EOF\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\tEOF\n+'\n+\n test_expect_success 'credential-store removes rejected credentials' '\n \ttest_when_finished \"rm -f .git-credentials\" &&\n \tcheck --unique=host store <<-\\EOF &&\n@@ -323,16 +347,26 @@ test_expect_success 'credential-store removes rejected credentials' '\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n+\tcheck --unique=host --username=other store <<-\\EOF &&\n+\tusername=other\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Password:\n+\tEOF\n \tcheck --reject --unique=host --username=askpass-username store <<-\\EOF &&\n \t--\n \tEOF\n-\tcheck --unique=host store <<-\\EOF\n+\tcheck --unique=host --username=askpass-username store <<-\\EOF &&\n \tusername=askpass-username\n \tpassword=askpass-password\n \t--\n-\taskpass: Username:\n \taskpass: Password:\n \tEOF\n+\tcheck --unique=host --username=other store <<-\\EOF\n+\tusername=other\n+\tpassword=askpass-password\n+\t--\n+\tEOF\n '\n \n test_done\n-- \n1.7.6.252.ge9c18\n"},{"id":"175516","messageId":"7vlitqst3w.fsf@alter.siamese.dyndns.org","threadId":"28389","inReplyTo":"20110914191704.GA23201@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] credential helper super fun pak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-14T21:37:39Z","receivedAt":"2011-09-14T21:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's a mixed bag of bugfix commits to go on top of what's in\n> jk/http-auth-keyring.\n\nHeh, it indeed is a super-fun-pak. Will queue.\n"},{"id":"175560","messageId":"201109151037.12248.trast@student.ethz.ch","threadId":"28389","inReplyTo":"20110914191757.GB28267@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] credential-cache: fix expiration calculation corner cases","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-15T08:37:11Z","receivedAt":"2011-09-15T08:37:11Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> However, there is a corner case: when we first start up, we\n> have no credentials, and are waiting for a client to\n> provide us with one. In this case, we ended up handing\n> complete junk for the timeout argument to poll(). On some\n> systems, this caused us to just wait a long time for the\n> client (which usually showed up within a second or so). On\n> OS X, however, the system quite reasonably complained about\n> our junk value with EINVAL.\n\nTested, works now.  Thanks!\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"175640","messageId":"a6397f7f28a5adcd34aeac98cca6500e336698aa.1316173346.git.trast@student.ethz.ch","threadId":"28389","inReplyTo":"20110914191757.GB28267@sigill.intra.peff.net","subject":"[PATCH 1/2] t0300: add missing EOF terminator for <<","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-16T11:51:34Z","receivedAt":"2011-09-16T11:51:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nThis doesn't really matter on my system, but the shell warns about it.\n\n\n t/t0300-credentials.sh |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 613c3fb..076a109 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -136,6 +136,7 @@ test_expect_success 'usernames can be preserved' '\n \tpassword=three\n \t--\n \tverbatim: --username=one\n+\tEOF\n '\n \n test_expect_success 'usernames can be overridden' '\n-- \n1.7.7.rc1.366.ge210a6\n"},{"id":"175641","messageId":"29010bf6134beb20efca498e7b4f7a9d9bdb21a6.1316173346.git.trast@student.ethz.ch","threadId":"28389","inReplyTo":"a6397f7f28a5adcd34aeac98cca6500e336698aa.1316173346.git.trast@student.ethz.ch","subject":"[PATCH 2/2] check_expirations: don't copy over same element","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-16T11:51:35Z","receivedAt":"2011-09-16T11:51:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The credentials expiry loop looks at a credential, and if it's\nexpired, frees it and memcpy()s the last credential in the now free\nplace.\n\nThis results in a memcpy() with source=destination, which technically\nyields undefined behaviour.  Instead of turning it into a memmove,\ndon't copy anything if we deleted the last entry.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nValgrind complained.  Sorry for not running it earlier when we were\ndiscussing the poll issue...\n\n\n credential-cache--daemon.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex d6769b1..128c5ce 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -77,7 +77,8 @@ static int check_expirations(void)\n \t\t\tfree(entries[i].item.unique);\n \t\t\tfree(entries[i].item.username);\n \t\t\tfree(entries[i].item.password);\n-\t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n+\t\t\tif (i != entries_nr)\n+\t\t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n \t\t\t/*\n \t\t\t * Stick around 30 seconds in case a new credential\n \t\t\t * shows up (e.g., because we just removed a failed\n-- \n1.7.7.rc1.366.ge210a6\n"},{"id":"175646","messageId":"20110916154718.GA13740@sigill.intra.peff.net","threadId":"28389","inReplyTo":"29010bf6134beb20efca498e7b4f7a9d9bdb21a6.1316173346.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] check_expirations: don't copy over same element","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-16T15:47:18Z","receivedAt":"2011-09-16T15:47:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 16, 2011 at 01:51:35PM +0200, Thomas Rast wrote:\n\n> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\n> index d6769b1..128c5ce 100644\n> --- a/credential-cache--daemon.c\n> +++ b/credential-cache--daemon.c\n> @@ -77,7 +77,8 @@ static int check_expirations(void)\n>  \t\t\tfree(entries[i].item.unique);\n>  \t\t\tfree(entries[i].item.username);\n>  \t\t\tfree(entries[i].item.password);\n> -\t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n> +\t\t\tif (i != entries_nr)\n> +\t\t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n\nThanks. I even remember while writing this loop considering the case of\n(i == entries_nr), but decided it didn't need special-casing. But I\nobviously forgot about memcpy.\n\nBoth this and the prior patch are:\n\nAcked-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"175757","messageId":"7vzki1bjtx.fsf@alter.siamese.dyndns.org","threadId":"28389","inReplyTo":"20110914191757.GB28267@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] credential-cache: fix expiration calculation corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-18T21:51:22Z","receivedAt":"2011-09-18T21:51:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The main credential-cache daemon loop calls poll to wait for\n> a client or to trigger the expiration of credentials. When\n> the last credential we hold expires, we exit.\n> ...\n\nThanks.\n"}]}