On Thu, Dec 08, 2011 at 01:34:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > Because the pattern takes 0 or more lines and no terminator, we can't
> > distinguish between empty or truncated input and the empty pattern.
>
> I agree that such a positive "Ok here is the end of specification" marker
> is a good idea, even if we do not worry about "an empty set".
>
> When the requestor wants to specify the credentials with host and user,
> but the wire is cut after host is communicated but before user is, we do
> want to notice the communication error, instead of silently erasing all
> the credentials on the host regardless of the user.
OK, I've tweaked the series to require an end-of-credential marker (a
blank line) both in input and output.
In addition, I've changed the code that runs helpers to make reading
from the helpers an all-or-nothing thing (instead of incrementally
ovewriting our credential as we read from it). Before, if a helper
exited with error, we would happily use its partial result. Instead, we
now read its response into a holding area, and only copy it into our
credential when we get a successful exit code. This lets us detect
truncation when reading from the helper, too.
It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).
It makes me a little sad, because the original format (relying on EOF)
was so Unix-y. You could make a helper like this:
echo password=`gpg -qd ~/.secret.gpg`
and now you must remember to tack an extra "echo" at the end. Not a big
deal, but it somehow just feels less elegant to my gut. OTOH, classic
Unix constructs have always been a nightmare for robustness and error
checking[1], so this is certainly nothing new.
The diff from this tip to the old tip is below to give you a sense of
the magnitude of the change (the individual changes are squashed into
their respective patches for the next re-roll, of course). I'll hold off
on posting the whole series to see if we get any more comments.
-Peff
[1] I mean things like:
grep foo bar | sed 's/some/transformation/'
where we totally ignore errors from grep, and where a truncated
output on the pipe would just subtly generate wrong answers.
---
Documentation/technical/api-credentials.txt | 2 +-
credential-cache--daemon.c | 1 +
credential-cache.c | 2 +
credential-store.c | 1 +
credential.c | 39 +++++++++++++++++++++++---
t/lib-credential.sh | 1 +
t/t0300-credentials.sh | 3 ++
t/t5550-http-fetch.sh | 1 +
8 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 21ca6a2..0aac899 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -199,7 +199,7 @@ followed by a newline. The key may contain any bytes except `=`,
newline, or NUL. The value may contain any bytes except newline or NUL.
In both cases, all bytes are treated as-is (i.e., there is no quoting,
and one cannot transmit a value with newline or NUL in it). The list of
-attributes is terminated by a blank line or end-of-file.
+attributes is terminated by a blank line.
Git will send the following attributes (but may not send all of
them for a given credential; for example, a `host` attribute makes no
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..38403645 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -138,6 +138,7 @@ static void serve_one_client(FILE *in, FILE *out)
if (e) {
fprintf(out, "username=%s\n", e->item.username);
fprintf(out, "password=%s\n", e->item.password);
+ fprintf(out, "\n");
}
}
else if (!strcmp(action.buf, "exit"))
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..5b8d8c9 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -70,6 +70,8 @@ static void do_cache(const char *socket, const char *action, int timeout,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("unable to relay credential");
}
+ else
+ strbuf_addch(&buf, '\n');
if (!send_request(socket, &buf))
return;
diff --git a/credential-store.c b/credential-store.c
index ed58768..00e38f0 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -43,6 +43,7 @@ static void print_entry(struct credential *c)
{
printf("username=%s\n", c->username);
printf("password=%s\n", c->password);
+ printf("\n");
}
static void print_line(struct strbuf *buf)
diff --git a/credential.c b/credential.c
index a17eafe..6d2a37d 100644
--- a/credential.c
+++ b/credential.c
@@ -147,8 +147,10 @@ int credential_read(struct credential *c, FILE *fp)
char *key = line.buf;
char *value = strchr(key, '=');
- if (!line.len)
- break;
+ if (!line.len) {
+ strbuf_release(&line);
+ return 0;
+ }
if (!value) {
warning("invalid credential line: %s", key);
@@ -181,7 +183,7 @@ int credential_read(struct credential *c, FILE *fp)
}
strbuf_release(&line);
- return 0;
+ return -1;
}
static void credential_write_item(FILE *fp, const char *key, const char *value)
@@ -198,6 +200,26 @@ static void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path);
credential_write_item(fp, "username", c->username);
credential_write_item(fp, "password", c->password);
+ putc('\n', fp);
+}
+
+static void credential_merge_one(char **dst, char **src)
+{
+ if (!*src)
+ return;
+ free(*dst);
+ *dst = *src;
+ *src = NULL;
+}
+
+static void credential_merge(struct credential *dst,
+ struct credential *src)
+{
+ credential_merge_one(&dst->protocol, &src->protocol);
+ credential_merge_one(&dst->host, &src->host);
+ credential_merge_one(&dst->path, &src->path);
+ credential_merge_one(&dst->username, &src->username);
+ credential_merge_one(&dst->password, &src->password);
}
static int run_credential_helper(struct credential *c,
@@ -206,6 +228,7 @@ static int run_credential_helper(struct credential *c,
{
struct child_process helper;
const char *argv[] = { NULL, NULL };
+ struct credential response = CREDENTIAL_INIT;
FILE *fp;
memset(&helper, 0, sizeof(helper));
@@ -227,17 +250,23 @@ static int run_credential_helper(struct credential *c,
if (want_output) {
int r;
+
fp = xfdopen(helper.out, "r");
- r = credential_read(c, fp);
+ r = credential_read(&response, fp);
fclose(fp);
if (r < 0) {
+ credential_clear(&response);
finish_command(&helper);
return -1;
}
}
- if (finish_command(&helper))
+ if (finish_command(&helper)) {
+ credential_clear(&response);
return -1;
+ }
+
+ credential_merge(c, &response);
return 0;
}
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index fc34447..c0de4e9 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -5,6 +5,7 @@
# separated by "--".
check() {
read_chunk >stdin &&
+ echo >>stdin &&
read_chunk >expect-stdout &&
read_chunk >expect-stderr &&
test-credential "$@" <stdin >stdout 2>stderr &&
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..f0e77dc 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -9,6 +9,7 @@ test_expect_success 'setup helper scripts' '
whoami=`echo $0 | sed s/.*git-credential-//`
echo >&2 "$whoami: $*"
while IFS== read key value; do
+ test -z "$key" && break
echo >&2 "$whoami: $key=$value"
eval "$key=$value"
done
@@ -28,6 +29,7 @@ test_expect_success 'setup helper scripts' '
. ./dump
test -z "$user" || echo username=$user
test -z "$pass" || echo password=$pass
+ echo
EOF
chmod +x git-credential-verbatim &&
@@ -196,6 +198,7 @@ HELPER="!f() {
cat >/dev/null
echo username=foo
echo password=bar
+ echo
}; f"
test_expect_success 'respect configured credentials' '
test_config credential.helper "$HELPER" &&
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 95a133d..b817c69 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -106,6 +106,7 @@ test_expect_success 'http auth respects credential helper config' '
cat >/dev/null
echo username=user@host
echo password=user@host
+ echo
}; f" &&
>askpass-query &&
echo wrong >askpass-response &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html