[PATCH 0/5] credential helper super fun pak

[PATCH 0/5] credential helper super fun pak

[Jeff King]

Here's a mixed bag of bugfix commits to go on top of what's in
jk/http-auth-keyring.

  [1/5]: docs: minor tweaks to credentials API

    Clarifications in response to comments from Junio on the original
    series.

  [2/5]: credential-cache: fix expiration calculation corner cases

    This fixes the failures in t0300 on OS X.

  [3/5]: t0300: make askpass tests a little more robust
  [4/5]: t0300: make alternate username tests more robust

    These two just make the tests more rigorous so that patch 5/5
    can actually demonstrate the failure it's fixing.

  [5/5]: credential-store: use a better storage format

    This makes multiple usernames on the same host work with
    credential-store (they already work with credential-cache).

-Peff

[PATCH 1/5] docs: minor tweaks to credentials API

[Jeff King]

These are in response to Junio's comments:

  1. Clarify how credential rejection works (first we call
     helpers, then we clear fields, so helpers get fields).

  2. Clarify quoting of keys/values returned from helpers.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-credentials.txt |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 335a007..b2eb24c 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -48,10 +48,11 @@ Functions
 `credential_reject`::
 
 	Inform the credential subsystem that the provided credentials
-	have been rejected. This will clear the username and password
-	fields in `struct credential`, as well as notify any helpers of
-	the rejection (which may, for example, purge the invalid
-	credentials from storage).
+	have been rejected. This will notify any helpers of the
+	rejection (which allows them to, for example, purge the invalid
+	credentials from storage), and then clear the username and
+	password fields in `struct credential`. It can then be
+	`credential_fill`-ed again.
 
 `credential_getpass`::
 
@@ -98,8 +99,11 @@ command line:
 
 The helper should produce a list of items on stdout, each followed by a
 newline character. Each item should consist of a key-value pair, separated
-by an `=` (equals) sign. The value may contain any bytes except a
-newline. When reading the response, git understands the following keys:
+by an `=` (equals) sign. The key can contain any bytes except `=` or
+newline. The value may contain any bytes except a newline. In both
+cases, all bytes are treated as-is (i.e., there is no quoting, and one
+cannot transmit a value with newline in it). When reading the response,
+git understands the following keys:
 
 `username`::
 
-- 
1.7.6.252.ge9c18

[PATCH 2/5] credential-cache: fix expiration calculation corner cases

[Jeff King]

The main credential-cache daemon loop calls poll to wait for
a client or to trigger the expiration of credentials. When
the last credential we hold expires, we exit.

However, there is a corner case: when we first start up, we
have no credentials, and are waiting for a client to
provide us with one. In this case, we ended up handing
complete junk for the timeout argument to poll(). On some
systems, this caused us to just wait a long time for the
client (which usually showed up within a second or so). On
OS X, however, the system quite reasonably complained about
our junk value with EINVAL.

Fixing this is pretty straightforward; we just notice that
we have no entries to compare against. However, that bug was
covering up another one: our expiration calculation didn't
give clients a chance to actually connect and provide us
with a credential before we decided that we should exit
because we weren't holding any credentials!

The new algorithm is:

  1. Sleep until it's time to expire the most recent
     credential.

  2. If we don't have any credentials yet, wait 30 seconds
     for a client to contact us and give us one.

  3. After expiring the last credential, wait 30 seconds for
     a client to provide us with one.

Technically only parts (1) and (2) are needed to implement
the original intended behavior.

But (3) is a minor optimization that is made easy by the new
code. When a client gives us a credential, then removes it
(e.g., because it had a bogus password), and then gives us
another one, we used to exit, forcing the client to start a
new daemon instance. Instead, we can just reuse the existing
daemon instance.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential-cache--daemon.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index f520347..d6769b1 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -57,20 +57,33 @@ static void remove_credential(const struct credential *c)
 
 static int check_expirations(void)
 {
+	static unsigned long wait_for_entry_until;
 	int i = 0;
 	unsigned long now = time(NULL);
 	unsigned long next = (unsigned long)-1;
 
+	/*
+	 * Initially give the client 30 seconds to actually contact us
+	 * and store a credential before we decide there's no point in
+	 * keeping the daemon around.
+	 */
+	if (!wait_for_entry_until)
+		wait_for_entry_until = now + 30;
+
 	while (i < entries_nr) {
 		if (entries[i].expiration <= now) {
 			entries_nr--;
-			if (!entries_nr)
-				return 0;
 			free(entries[i].item.description);
 			free(entries[i].item.unique);
 			free(entries[i].item.username);
 			free(entries[i].item.password);
 			memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
+			/*
+			 * Stick around 30 seconds in case a new credential
+			 * shows up (e.g., because we just removed a failed
+			 * one, and we will soon get the correct one).
+			 */
+			wait_for_entry_until = now + 30;
 		}
 		else {
 			if (entries[i].expiration < next)
@@ -79,6 +92,12 @@ static int check_expirations(void)
 		}
 	}
 
+	if (!entries_nr) {
+		if (wait_for_entry_until <= now)
+			return 0;
+		next = wait_for_entry_until;
+	}
+
 	return next - now;
 }
 
-- 
1.7.6.252.ge9c18

Re: [PATCH 2/5] credential-cache: fix expiration calculation corner cases

[Thomas Rast]

Jeff King wrote:
> However, there is a corner case: when we first start up, we
> have no credentials, and are waiting for a client to
> provide us with one. In this case, we ended up handing
> complete junk for the timeout argument to poll(). On some
> systems, this caused us to just wait a long time for the
> client (which usually showed up within a second or so). On
> OS X, however, the system quite reasonably complained about
> our junk value with EINVAL.

Tested, works now.  Thanks!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH 1/2] t0300: add missing EOF terminator for <<

[Thomas Rast]

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This doesn't really matter on my system, but the shell warns about it.


 t/t0300-credentials.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 613c3fb..076a109 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -136,6 +136,7 @@ test_expect_success 'usernames can be preserved' '
 	password=three
 	--
 	verbatim: --username=one
+	EOF
 '
 
 test_expect_success 'usernames can be overridden' '
-- 
1.7.7.rc1.366.ge210a6

[PATCH 2/2] check_expirations: don't copy over same element

[Thomas Rast]

The credentials expiry loop looks at a credential, and if it's
expired, frees it and memcpy()s the last credential in the now free
place.

This results in a memcpy() with source=destination, which technically
yields undefined behaviour.  Instead of turning it into a memmove,
don't copy anything if we deleted the last entry.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Valgrind complained.  Sorry for not running it earlier when we were
discussing the poll issue...


 credential-cache--daemon.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index d6769b1..128c5ce 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -77,7 +77,8 @@ static int check_expirations(void)
 			free(entries[i].item.unique);
 			free(entries[i].item.username);
 			free(entries[i].item.password);
-			memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
+			if (i != entries_nr)
+				memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
 			/*
 			 * Stick around 30 seconds in case a new credential
 			 * shows up (e.g., because we just removed a failed
-- 
1.7.7.rc1.366.ge210a6

Re: [PATCH 2/2] check_expirations: don't copy over same element

[Jeff King]

On Fri, Sep 16, 2011 at 01:51:35PM +0200, Thomas Rast wrote:

> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
> index d6769b1..128c5ce 100644
> --- a/credential-cache--daemon.c
> +++ b/credential-cache--daemon.c
> @@ -77,7 +77,8 @@ static int check_expirations(void)
>  			free(entries[i].item.unique);
>  			free(entries[i].item.username);
>  			free(entries[i].item.password);
> -			memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
> +			if (i != entries_nr)
> +				memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));

Thanks. I even remember while writing this loop considering the case of
(i == entries_nr), but decided it didn't need special-casing. But I
obviously forgot about memcpy.

Both this and the prior patch are:

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH 2/5] credential-cache: fix expiration calculation corner cases

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The main credential-cache daemon loop calls poll to wait for
> a client or to trigger the expiration of credentials. When
> the last credential we hold expires, we exit.
> ...

Thanks.

[PATCH 3/5] t0300: make askpass tests a little more robust

[Jeff King]

Our fake askpass always just returned "askpass-result";
let's actually have it return a different fake value for
usernames and passwords, to be sure values are getting
routed correctly.

All tests should still pass.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0300-credentials.sh |   77 ++++++++++++++++++++++++-----------------------
 1 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 5d54976..09d5b15 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -52,7 +52,8 @@ test_expect_success 'setup helper scripts' '
 	cat >askpass <<-\EOF &&
 	#!/bin/sh
 	echo >&2 askpass: $*
-	echo askpass-result
+	what=`echo $1 | tr A-Z a-z | tr -cd a-z`
+	echo "askpass-$what"
 	EOF
 	chmod +x askpass &&
 	GIT_ASKPASS=askpass &&
@@ -155,8 +156,8 @@ test_expect_success 'do not bother completing already-full credential' '
 # askpass helper is run, we know the internal getpass is working.
 test_expect_success 'empty methods falls back to internal getpass' '
 	check <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
@@ -166,7 +167,7 @@ test_expect_success 'empty methods falls back to internal getpass' '
 test_expect_success 'internal getpass does not ask for known username' '
 	check --username=foo <<-\EOF
 	username=foo
-	password=askpass-result
+	password=askpass-password
 	--
 	askpass: Password:
 	EOF
@@ -176,7 +177,7 @@ test_expect_success 'internal getpass can pull from config' '
 	git config credential.foo.username configured-username
 	check --unique=foo <<-\EOF
 	username=configured-username
-	password=askpass-result
+	password=askpass-password
 	--
 	askpass: Password:
 	EOF
@@ -185,15 +186,15 @@ test_expect_success 'internal getpass can pull from config' '
 test_expect_success 'credential-cache caches password' '
 	test_when_finished "git credential-cache --exit" &&
 	check --unique=host cache <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	check --unique=host cache <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	EOF
 '
@@ -201,15 +202,15 @@ test_expect_success 'credential-cache caches password' '
 test_expect_success 'credential-cache requires matching unique token' '
 	test_when_finished "git credential-cache --exit" &&
 	check --unique=host cache <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	check --unique=host2 cache <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
@@ -219,15 +220,15 @@ test_expect_success 'credential-cache requires matching unique token' '
 test_expect_success 'credential-cache requires matching usernames' '
 	test_when_finished "git credential-cache --exit" &&
 	check --unique=host cache <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	check --unique=host --username=other cache <<-\EOF
 	username=other
-	password=askpass-result
+	password=askpass-password
 	--
 	askpass: Password:
 	EOF
@@ -236,16 +237,16 @@ test_expect_success 'credential-cache requires matching usernames' '
 test_expect_success 'credential-cache times out' '
 	test_when_finished "git credential-cache --exit || true" &&
 	check --unique=host "cache --timeout=1" <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	sleep 2 &&
 	check --unique=host cache <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
@@ -255,18 +256,18 @@ test_expect_success 'credential-cache times out' '
 test_expect_success 'credential-cache removes rejected credentials' '
 	test_when_finished "git credential-cache --exit || true" &&
 	check --unique=host cache <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
-	check --reject --unique=host --username=askpass-result cache <<-\EOF &&
+	check --reject --unique=host --username=askpass-username cache <<-\EOF &&
 	--
 	EOF
 	check --unique=host cache <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
@@ -276,15 +277,15 @@ test_expect_success 'credential-cache removes rejected credentials' '
 test_expect_success 'credential-store stores password' '
 	test_when_finished "rm -f .git-credentials" &&
 	check --unique=host store <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	check --unique=host store <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	EOF
 '
@@ -292,15 +293,15 @@ test_expect_success 'credential-store stores password' '
 test_expect_success 'credential-store requires matching unique token' '
 	test_when_finished "rm -f .git-credentials" &&
 	check --unique=host store <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
 	check --unique=host2 store <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
@@ -310,18 +311,18 @@ test_expect_success 'credential-store requires matching unique token' '
 test_expect_success 'credential-store removes rejected credentials' '
 	test_when_finished "rm -f .git-credentials" &&
 	check --unique=host store <<-\EOF &&
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
 	EOF
-	check --reject --unique=host --username=askpass-result store <<-\EOF &&
+	check --reject --unique=host --username=askpass-username store <<-\EOF &&
 	--
 	EOF
 	check --unique=host store <<-\EOF
-	username=askpass-result
-	password=askpass-result
+	username=askpass-username
+	password=askpass-password
 	--
 	askpass: Username:
 	askpass: Password:
-- 
1.7.6.252.ge9c18

[PATCH 4/5] t0300: make alternate username tests more robust

[Jeff King]

One of the tests is to make sure that if we have a cached
credential "user/pass", that we ignore it when asking for
"--username=other", and instead get a new password. But
since askpass returns only a single fake value, we couldn't
differentiate between using the cached password and the new
one.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0300-credentials.sh |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 09d5b15..41ab8cc 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -53,7 +53,11 @@ test_expect_success 'setup helper scripts' '
 	#!/bin/sh
 	echo >&2 askpass: $*
 	what=`echo $1 | tr A-Z a-z | tr -cd a-z`
-	echo "askpass-$what"
+	if test -f "askpass-$what"; then
+		cat "askpass-$what"
+	else
+		echo "askpass-$what"
+	fi
 	EOF
 	chmod +x askpass &&
 	GIT_ASKPASS=askpass &&
@@ -226,9 +230,11 @@ test_expect_success 'credential-cache requires matching usernames' '
 	askpass: Username:
 	askpass: Password:
 	EOF
+	test_when_finished "rm -f askpass-password" &&
+	echo other-password >askpass-password &&
 	check --unique=host --username=other cache <<-\EOF
 	username=other
-	password=askpass-password
+	password=other-password
 	--
 	askpass: Password:
 	EOF
-- 
1.7.6.252.ge9c18

[PATCH 5/5] credential-store: use a better storage format

[Jeff King]

In the name of simplicity, the credential-store helper
re-used git's config file format for on-disk storage of
credentials. The scheme looked something like this:

  [credential "unique-context"]
	username = foo
	password = bar

This works fine if you have only one credential for each
context. But there's no good way to store separate passwords
for user1@unique-context versus user2@unique-context. For
end users, this meant that:

  1. Trying to access user2@host when you already have a
     cached credential for user1@host would return user2's
     username with user1's password.

  2. The stored result would also mix the credentials.

  3. Rejection would remove all entries for the host,
     regardless of username.

You can hack around it by assuming that order is important,
and that:

  [credential "unique-context"]
	username = user1
	password = pass1
	username = user2
	password = pass2

refers to two distinct credentials. But the config code
isn't well setup for that. You have to keep track of the
last username field you parsed, and assume the next password
field after it is associated with it. Which isn't too bad.
But removing just one credential (which we need to do for
--reject) is impossible.  You can say "delete the username
field that is user2", but there is no way to say "delete
the password field that comes right after the username field
that is user2" (it's tempting to find the password for user2
and say "delete the password field that is pass2", but
that's not strictly correct either).

Instead, let's define a new, very simple storage format.
Each line is a credential containing three tokens: unique,
username, and password. Each is separated by a space, and
shell-quoted to avoid ambiguity.

There's now an additional test that we can differentiate the credentials
correctly (matching the similar test we already have for
credential-cache). The test for removing credentials is also enhanced to
check that removing one credential leaves an unrelated one stored.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential-store.c     |  119 ++++++++++++++++++++++++++++++++++++++++++------
 t/t0300-credentials.sh |   38 ++++++++++++++-
 2 files changed, 140 insertions(+), 17 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 8ab8582..aae8e0c 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -2,40 +2,129 @@
 #include "credential.h"
 #include "string-list.h"
 #include "parse-options.h"
+#include "quote.h"
+
+static struct lock_file credential_lock;
+
+static int parse_credential_file(const char *fn,
+				 struct credential *c,
+				 int (*match_cb)(const char *username,
+						 const char *password,
+						 struct credential *c),
+				 int (*other_cb)(const char *buf))
+{
+	FILE *fh;
+	struct strbuf buf = STRBUF_INIT;
+	const char **argv = NULL;
+	int alloc = 0;
+	int retval = 0;
+
+	fh = fopen(fn, "r");
+	if (!fh)
+		return errno == ENOENT ? 0 : -1;
+
+	while (strbuf_getwholeline(&buf, fh, '\n') != EOF) {
+		int nr = 0;
+		char *pristine = xstrdup(buf.buf);
+
+		strbuf_trim(&buf);
+		if (!sq_dequote_to_argv(buf.buf, &argv, &nr, &alloc) &&
+		    nr == 3 &&
+		    !strcmp(c->unique, argv[0]) &&
+		    (!c->username || !strcmp(c->username, argv[1]))) {
+			if (match_cb(argv[1], argv[2], c) < 0) {
+				retval = -1;
+				break;
+			}
+		}
+		else if (other_cb) {
+			if (other_cb(pristine) < 0) {
+				retval = -1;
+				break;
+			}
+		}
+		free(pristine);
+	}
+
+	free(argv);
+	strbuf_release(&buf);
+	fclose(fh);
+	return retval;
+}
+
+
+static int copy_credential(const char *username, const char *password,
+			   struct credential *c)
+{
+	if (!c->username)
+		c->username = xstrdup(username);
+	free(c->password);
+	c->password = xstrdup(password);
+	return 0;
+}
 
 static int lookup_credential(const char *fn, struct credential *c)
 {
-	config_exclusive_filename = fn;
-	credential_from_config(c);
+	if (!c->unique)
+		return 0;
+	parse_credential_file(fn, c, copy_credential, NULL);
 	return c->username && c->password;
 }
 
-static void store_item(const char *fn, const char *unique,
-		       const char *item, const char *value)
+static int skip_match(const char *username, const char *password,
+		      struct credential *c)
 {
-	struct strbuf key = STRBUF_INIT;
+	return 0;
+}
 
-	if (!unique)
-		return;
+static int print_entry(const char *buf)
+{
+	return write_in_full(credential_lock.fd, buf, strlen(buf));
+}
 
-	config_exclusive_filename = fn;
+static int rewrite_credential_file(const char *fn, struct credential *c,
+				   int replace)
+{
 	umask(077);
+	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
+		return -1;
+	if (parse_credential_file(fn, c, skip_match, print_entry) < 0) {
+		rollback_lock_file(&credential_lock);
+		return -1;
+	}
+	if (replace) {
+		struct strbuf buf = STRBUF_INIT;
+		int r;
 
-	strbuf_addf(&key, "credential.%s.%s", unique, item);
-	git_config_set(key.buf, value);
-	strbuf_release(&key);
+		sq_quote_buf(&buf, c->unique);
+		strbuf_addch(&buf, ' ');
+		sq_quote_buf(&buf, c->username);
+		strbuf_addch(&buf, ' ');
+		sq_quote_buf(&buf, c->password);
+		strbuf_addch(&buf, '\n');
+
+		r = write_in_full(credential_lock.fd, buf.buf, buf.len);
+		strbuf_release(&buf);
+		if (r < 0) {
+			rollback_lock_file(&credential_lock);
+			return -1;
+		}
+	}
+	return commit_lock_file(&credential_lock);
 }
 
 static void store_credential(const char *fn, struct credential *c)
 {
-	store_item(fn, c->unique, "username", c->username);
-	store_item(fn, c->unique, "password", c->password);
+	if (!c->unique || !c->username || !c->password)
+		return;
+	rewrite_credential_file(fn, c, 1);
 }
 
 static void remove_credential(const char *fn, struct credential *c)
 {
-	store_item(fn, c->unique, "username", NULL);
-	store_item(fn, c->unique, "password", NULL);
+	if (!c->unique)
+		return;
+	rewrite_credential_file(fn, c, 0);
 }
 
 int main(int argc, const char **argv)
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 41ab8cc..613c3fb 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -314,6 +314,30 @@ test_expect_success 'credential-store requires matching unique token' '
 	EOF
 '
 
+test_expect_success 'credential-store requires matching usernames' '
+	test_when_finished "rm -f .git-credentials" &&
+	check --unique=host store <<-\EOF &&
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username:
+	askpass: Password:
+	EOF
+	test_when_finished "rm -f askpass-password" &&
+	echo other-password >askpass-password &&
+	check --unique=host --username=other store <<-\EOF &&
+	username=other
+	password=other-password
+	--
+	askpass: Password:
+	EOF
+	check --unique=host --username=askpass-username store <<-\EOF
+	username=askpass-username
+	password=askpass-password
+	--
+	EOF
+'
+
 test_expect_success 'credential-store removes rejected credentials' '
 	test_when_finished "rm -f .git-credentials" &&
 	check --unique=host store <<-\EOF &&
@@ -323,16 +347,26 @@ test_expect_success 'credential-store removes rejected credentials' '
 	askpass: Username:
 	askpass: Password:
 	EOF
+	check --unique=host --username=other store <<-\EOF &&
+	username=other
+	password=askpass-password
+	--
+	askpass: Password:
+	EOF
 	check --reject --unique=host --username=askpass-username store <<-\EOF &&
 	--
 	EOF
-	check --unique=host store <<-\EOF
+	check --unique=host --username=askpass-username store <<-\EOF &&
 	username=askpass-username
 	password=askpass-password
 	--
-	askpass: Username:
 	askpass: Password:
 	EOF
+	check --unique=host --username=other store <<-\EOF
+	username=other
+	password=askpass-password
+	--
+	EOF
 '
 
 test_done
-- 
1.7.6.252.ge9c18

Re: [PATCH 0/5] credential helper super fun pak

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a mixed bag of bugfix commits to go on top of what's in
> jk/http-auth-keyring.

Heh, it indeed is a super-fun-pak. Will queue.