[RFC/PATCH 0/3] tracking per-ref errors on push

[RFC/PATCH 0/3] tracking per-ref errors on push

[Jeff King]

Following is a series to track push errors for individual refs.  This
should have three immediate advantages (the first two of which I
implement in the series):

  - updating tracking refs only for non-error refs
  - incorporating remote rejection into the printed status
  - printing errors in a different order than we processed
    (e.g., consolidating non-ff errors near the end with
    a special message)

The patches are:

  1. send-pack: track errors for each ref
     This is the groundwork for the other 2. I also think it makes the
     code a bit more readable by splitting out the print formatting from
     the push logic, and by explicitly naming the different states that
     we were previously deducing from the states of other variables.

  2. send-pack: check ref->status before updating tracking refs
     This should fix the bugs that people are seeing from 334f483

  3. send-pack: assign remote errors to each ref
     This may have some problems, as I will explain below.

I have a few concerns which make this an RFC and not a real patch
submission:

  - I have done pretty minimal testing, and there are no automated tests
    (at least 2, which fixes a real bug, should get a test).
    Unfortunately, I am out of time to work on this and am leaving the
    country for a week. Between that and Thanksgiving travel, I'm not
    sure when I'll have time to finish up, so I thought it best to have
    comments waiting (and others should feel free to pick up the work if
    they want -- Alex, I think your error consolidation should go nicely
    on top of this).

  - It looks like the push mirror code just made it into next, which
    is going to require a conflict-heavy rebase on my part.

  - There is a potential performance bottleneck in patch 3. See the
    commit message.

  - In patch 3, the 'ng' message sent by the remote are not
    unambiguously parseable.  See the commit message.

-Peff

[PATCH/RFC 1/3] send-pack: track errors for each ref

[Jeff King]

Instead of keeping the 'ret' variable, we instead have a
status flag for each ref that tracks what happened to it.
We then print the ref status after all of the refs have
been examined.

This paves the way for three improvements:
  - updating tracking refs only for non-error refs
  - incorporating remote rejection into the printed status
  - printing errors in a different order than we processed
    (e.g., consolidating non-ff errors near the end with
    a special message)
---
 builtin-send-pack.c |  201 ++++++++++++++++++++++++++++-----------------------
 cache.h             |   13 +++-
 2 files changed, 121 insertions(+), 93 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5a0f5c6..3ac2615 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -207,8 +207,9 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	}
 }
 
-static const char *prettify_ref(const char *name)
+static const char *prettify_ref(const struct ref *ref)
 {
+	const char *name = ref->name;
 	return name + (
 		!prefixcmp(name, "refs/heads/") ? 11 :
 		!prefixcmp(name, "refs/tags/") ? 10 :
@@ -218,15 +219,90 @@ static const char *prettify_ref(const char *name)
 
 #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
+static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+{
+	fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+	if (from)
+		fprintf(stderr, "%s -> %s", prettify_ref(from), prettify_ref(to));
+	else
+		fputs(prettify_ref(to), stderr);
+	if (msg) {
+		fputs(" (", stderr);
+		fputs(msg, stderr);
+		fputc(')', stderr);
+	}
+	fputc('\n', stderr);
+}
+
+
+static void print_push_status(const char *dest, struct ref *refs)
+{
+	struct ref *ref;
+	int shown_dest = 0;
+
+	for (ref = refs; ref; ref = ref->next) {
+		if (!ref->status)
+			continue;
+		if (ref->status == REF_STATUS_UPTODATE && !args.verbose)
+			continue;
+
+		if (!shown_dest) {
+			fprintf(stderr, "To %s\n", dest);
+			shown_dest = 1;
+		}
+
+		switch(ref->status) {
+		case REF_STATUS_NONE:
+			print_ref_status('X', "[no match]", ref, NULL, NULL);
+			break;
+		case REF_STATUS_NODELETE:
+			print_ref_status('!', "[rejected]", ref, NULL,
+					"remote does not support deleting refs");
+			break;
+		case REF_STATUS_UPTODATE:
+			print_ref_status('=', "[up to date]", ref, ref->peer_ref, NULL);
+			break;
+		case REF_STATUS_NONFF:
+			print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
+			break;
+		case REF_STATUS_OK:
+			if (ref->deletion)
+				print_ref_status('-', "[deleted]", ref, NULL, NULL);
+			else if (is_null_sha1(ref->old_sha1))
+				print_ref_status('*',
+						(prefixcmp(ref->name, "refs/tags/") ?
+						  "[new branch]" : "[new tag]"),
+						ref, ref->peer_ref, NULL);
+			else {
+				char quickref[83];
+				char type = ' ';
+				const char *msg = NULL;
+				const char *old_abb;
+
+				old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
+				strcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));
+				if (ref->nonff) {
+					strcat(quickref, "...");
+					type = '+';
+					msg = " (forced update)";
+				}
+				else
+					strcat(quickref, "..");
+				strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+
+				print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+			}
+		}
+	}
+}
+
 static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
 	struct ref *ref;
 	int new_refs;
-	int ret = 0;
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
 	int expect_status_report = 0;
-	int shown_dest = 0;
 
 	/* No funny business with the matcher */
 	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
@@ -256,35 +332,17 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-		char old_hex[60], *new_hex;
-		int will_delete_ref;
-		const char *pretty_ref;
-		const char *pretty_peer;
-
 		if (!ref->peer_ref)
 			continue;
 
-		if (!shown_dest) {
-			fprintf(stderr, "To %s\n", dest);
-			shown_dest = 1;
-		}
-
-		pretty_ref = prettify_ref(ref->name);
-		pretty_peer = prettify_ref(ref->peer_ref->name);
-
-		will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
-		if (will_delete_ref && !allow_deleting_refs) {
-			fprintf(stderr, " ! %-*s %s (remote does not support deleting refs)\n",
-					SUMMARY_WIDTH, "[rejected]", pretty_ref);
-			ret = -2;
+		ref->deletion = is_null_sha1(ref->peer_ref->new_sha1);
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_NODELETE;
 			continue;
 		}
-		if (!will_delete_ref &&
+		if (!ref->deletion &&
 		    !hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
-			if (args.verbose)
-				fprintf(stderr, " = %-*s %s -> %s\n",
-					SUMMARY_WIDTH, "[up to date]",
-					pretty_peer, pretty_ref);
+			ref->status = REF_STATUS_UPTODATE;
 			continue;
 		}
 
@@ -307,34 +365,26 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		 *     always allowed.
 		 */
 
-		if (!args.force_update &&
-		    !will_delete_ref &&
+		ref->nonff =
+		    !ref->deletion &&
 		    !is_null_sha1(ref->old_sha1) &&
-		    !ref->force) {
-			if (!has_sha1_file(ref->old_sha1) ||
-			    !ref_newer(ref->peer_ref->new_sha1,
-				       ref->old_sha1)) {
-				/* We do not have the remote ref, or
-				 * we know that the remote ref is not
-				 * an ancestor of what we are trying to
-				 * push.  Either way this can be losing
-				 * commits at the remote end and likely
-				 * we were not up to date to begin with.
-				 */
-				fprintf(stderr, " ! %-*s %s -> %s (non-fast forward)\n",
-						SUMMARY_WIDTH, "[rejected]",
-						pretty_peer, pretty_ref);
-				ret = -2;
-				continue;
-			}
+		    (!has_sha1_file(ref->old_sha1)
+		      || !ref_newer(ref->peer_ref->new_sha1, ref->old_sha1));
+
+		if (ref->nonff && !ref->force && !args.force_update) {
+			ref->status = REF_STATUS_NONFF;
+			continue;
 		}
+
 		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		if (!will_delete_ref)
+		if (!ref->deletion)
 			new_refs++;
-		strcpy(old_hex, sha1_to_hex(ref->old_sha1));
-		new_hex = sha1_to_hex(ref->new_sha1);
+		ref->status = REF_STATUS_OK;
 
 		if (!args.dry_run) {
+			char *old_hex = sha1_to_hex(ref->old_sha1);
+			char *new_hex = sha1_to_hex(ref->new_sha1);
+
 			if (ask_for_status_report) {
 				packet_write(out, "%s %s %s%c%s",
 					old_hex, new_hex, ref->name, 0,
@@ -346,63 +396,32 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 				packet_write(out, "%s %s %s",
 					old_hex, new_hex, ref->name);
 		}
-		if (will_delete_ref)
-			fprintf(stderr, " - %-*s %s\n",
-				SUMMARY_WIDTH, "[deleting]",
-				pretty_ref);
-		else if (is_null_sha1(ref->old_sha1)) {
-			const char *msg;
-
-			if (!prefixcmp(ref->name, "refs/tags/"))
-				msg = "[new tag]";
-			else
-				msg = "[new branch]";
-			fprintf(stderr, " * %-*s %s -> %s\n",
-				SUMMARY_WIDTH, msg,
-				pretty_peer, pretty_ref);
-		}
-		else {
-			char quickref[83];
-			char type = ' ';
-			const char *msg = "";
-			const char *old_abb;
-			old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
-			strcpy(quickref, old_abb ? old_abb : old_hex);
-			if (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))
-				strcat(quickref, "..");
-			else {
-				strcat(quickref, "...");
-				type = '+';
-				msg = " (forced update)";
-			}
-			strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-
-			fprintf(stderr, " %c %-*s %s -> %s%s\n",
-				type,
-				SUMMARY_WIDTH, quickref,
-				pretty_peer, pretty_ref,
-				msg);
-		}
 	}
 
 	packet_flush(out);
-	if (new_refs && !args.dry_run)
-		ret = pack_objects(out, remote_refs);
+	if (new_refs && !args.dry_run) {
+		if (pack_objects(out, remote_refs) < 0) {
+			close(out);
+			return -1;
+		}
+	}
 	close(out);
 
+	print_push_status(dest, remote_refs);
+
 	if (expect_status_report) {
 		if (receive_status(in))
-			ret = -4;
+			return -1;
 	}
 
-	if (!args.dry_run && remote && ret == 0) {
+	if (!args.dry_run && remote) {
 		for (ref = remote_refs; ref; ref = ref->next)
 			update_tracking_ref(remote, ref);
 	}
 
-	if (!new_refs && ret == 0)
+	if (!new_refs)
 		fprintf(stderr, "Everything up-to-date\n");
-	return ret;
+	return 0;
 }
 
 static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index 5f40e12..ca5d96d 100644
--- a/cache.h
+++ b/cache.h
@@ -499,8 +499,17 @@ struct ref {
 	struct ref *next;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
-	unsigned char force;
-	unsigned char merge;
+	unsigned char force : 1;
+	unsigned char merge : 1;
+	unsigned char nonff : 1;
+	unsigned char deletion : 1;
+	enum {
+		REF_STATUS_NONE = 0,
+		REF_STATUS_OK,
+		REF_STATUS_NONFF,
+		REF_STATUS_NODELETE,
+		REF_STATUS_UPTODATE,
+	} status;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
-- 
1.5.3.5.1704.g24d42-dirty

[PATCH/RFC 2/3] send-pack: check ref->status before updating tracking refs

[Jeff King]

Previously, we manually checked the 'NONE' and 'UPTODATE'
conditions. Now that we have ref->status, we can easily
say "only update if we pushed successfully".
---
 builtin-send-pack.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 3ac2615..2805c92 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -180,24 +180,17 @@ static int receive_status(int in)
 static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
-	int will_delete_ref;
 
-	rs.src = ref->name;
-	rs.dst = NULL;
-
-	if (!ref->peer_ref)
+	if (ref->status != REF_STATUS_OK)
 		return;
 
-	will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
-
-	if (!will_delete_ref &&
-			!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1))
-		return;
+	rs.src = ref->name;
+	rs.dst = NULL;
 
 	if (!remote_find_tracking(remote, &rs)) {
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (is_null_sha1(ref->peer_ref->new_sha1)) {
+		if (ref->deletion) {
 			if (delete_ref(rs.dst, NULL))
 				error("Failed to delete");
 		} else
-- 
1.5.3.5.1704.g24d42-dirty

[PATCH/RFC 3/3] send-pack: assign remote errors to each ref

[Jeff King]

This lets us show remote errors (e.g., a denied hook) along
with the usual push output. There are two drawbacks to this
change:

  1. cross-referencing the incoming status with the ref list
     is worst case O(n^2) (where n = number of refs); this
     can be fixed with a smarter implementation

  2. the status parsing is not foolproof. We get a line like

         ng refs/heads/master arbitrary msg

     which cannot be parsed unambiguously in the face of
     refnames with spaces. We do a prefix-match so that
     you will only run into problems if you have two refs,
     one of which is a prefix match of the other, and the
     longer having a space right after the prefix.
---
 builtin-send-pack.c |   44 +++++++++++++++++++++++++++++++++++++-------
 cache.h             |    2 ++
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2805c92..a2307fa 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,14 +146,34 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static int receive_status(int in)
+static void set_ref_error(struct ref *refs, const char *line) {
+	struct ref *ref;
+
+	for (ref = refs; ref; ref = ref->next) {
+		const char *msg;
+		if (prefixcmp(line, ref->name))
+			continue;
+		msg = line + strlen(ref->name);
+		if (*msg++ != ' ')
+			continue;
+		ref->status = REF_STATUS_REMOTE_REJECT;
+		ref->error = xstrdup(msg);
+		ref->error[strlen(ref->error)-1] = '\0';
+		return;
+	}
+}
+
+/* a return value of -1 indicates that an error occurred,
+ * but we were able to set individual ref errors. A return
+ * value of -2 means we couldn't even get that far. */
+static int receive_status(int in, struct ref *refs)
 {
 	char line[1000];
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
 	if (len < 10 || memcmp(line, "unpack ", 7)) {
 		fprintf(stderr, "did not receive status back\n");
-		return -1;
+		return -2;
 	}
 	if (memcmp(line, "unpack ok\n", 10)) {
 		fputs(line, stderr);
@@ -171,7 +191,7 @@ static int receive_status(int in)
 		}
 		if (!memcmp(line, "ok", 2))
 			continue;
-		fputs(line, stderr);
+		set_ref_error(refs, line + 3);
 		ret = -1;
 	}
 	return ret;
@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)
 		case REF_STATUS_NONFF:
 			print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
 			break;
+		case REF_STATUS_REMOTE_REJECT:
+			if (ref->deletion)
+				print_ref_status('!', "[remote rejected]", ref, NULL, ref->error);
+			else
+				print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
+			break;
 		case REF_STATUS_OK:
 			if (ref->deletion)
 				print_ref_status('-', "[deleted]", ref, NULL, NULL);
@@ -296,6 +322,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
 	int expect_status_report = 0;
+	int ret;
 
 	/* No funny business with the matcher */
 	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
@@ -400,12 +427,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	}
 	close(out);
 
-	print_push_status(dest, remote_refs);
-
 	if (expect_status_report) {
-		if (receive_status(in))
+		ret = receive_status(in, remote_refs);
+		if (ret == -2)
 			return -1;
 	}
+	else
+		ret = 0;
+
+	print_push_status(dest, remote_refs);
 
 	if (!args.dry_run && remote) {
 		for (ref = remote_refs; ref; ref = ref->next)
@@ -414,7 +444,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	if (!new_refs)
 		fprintf(stderr, "Everything up-to-date\n");
-	return 0;
+	return ret;
 }
 
 static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index ca5d96d..082e03b 100644
--- a/cache.h
+++ b/cache.h
@@ -509,7 +509,9 @@ struct ref {
 		REF_STATUS_NONFF,
 		REF_STATUS_NODELETE,
 		REF_STATUS_UPTODATE,
+		REF_STATUS_REMOTE_REJECT,
 	} status;
+	char *error;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
-- 
1.5.3.5.1704.g24d42-dirty

Re: [PATCH/RFC 3/3] send-pack: assign remote errors to each ref

[Jeff King]

On Tue, Nov 13, 2007 at 05:29:00AM -0500, Jeff King wrote:

> +		ref->error = xstrdup(msg);

Another problem with this patch: this is a leak.

-Peff

Re: [PATCH/RFC 1/3] send-pack: track errors for each ref

[Jeff King]

On Tue, Nov 13, 2007 at 05:27:09AM -0500, Jeff King wrote:

> Instead of keeping the 'ret' variable, we instead have a
> status flag for each ref that tracks what happened to it.
> We then print the ref status after all of the refs have
> been examined.

Argh, this fails t5400 quite badly without the following patch:

---
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 3ac2615..eff84e0 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -421,6 +421,16 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	if (!new_refs)
 		fprintf(stderr, "Everything up-to-date\n");
+	for (ref = remote_refs; ref; ref = ref->next) {
+		switch (ref->status) {
+		case REF_STATUS_NONE:
+		case REF_STATUS_UPTODATE:
+		case REF_STATUS_OK:
+			break;
+		default:
+			return -1;
+		}
+	}
 	return 0;
 }
 

Re: [RFC/PATCH 0/3] tracking per-ref errors on push

[Jeff King]

On Tue, Nov 13, 2007 at 05:25:01AM -0500, Jeff King wrote:

>   - It looks like the push mirror code just made it into next, which
>     is going to require a conflict-heavy rebase on my part.

This turned out not to be too bad. A rebased series (with the extra
patch I just posted) follows, and it passes the test suite (though I
still think more tests are in order).

-Peff

[PATCH v2 1/3] send-pack: track errors for each ref

[Jeff King]

Instead of keeping the 'ret' variable, we instead have a
status flag for each ref that tracks what happened to it.
We then print the ref status after all of the refs have
been examined.

This paves the way for three improvements:
  - updating tracking refs only for non-error refs
  - incorporating remote rejection into the printed status
  - printing errors in a different order than we processed
    (e.g., consolidating non-ff errors near the end with
    a special message)

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |  213 +++++++++++++++++++++++++++++----------------------
 cache.h             |   13 +++-
 2 files changed, 132 insertions(+), 94 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 418925e..c01a9c4 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -207,8 +207,9 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	}
 }
 
-static const char *prettify_ref(const char *name)
+static const char *prettify_ref(const struct ref *ref)
 {
+	const char *name = ref->name;
 	return name + (
 		!prefixcmp(name, "refs/heads/") ? 11 :
 		!prefixcmp(name, "refs/tags/") ? 10 :
@@ -218,15 +219,90 @@ static const char *prettify_ref(const char *name)
 
 #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
+static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+{
+	fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+	if (from)
+		fprintf(stderr, "%s -> %s", prettify_ref(from), prettify_ref(to));
+	else
+		fputs(prettify_ref(to), stderr);
+	if (msg) {
+		fputs(" (", stderr);
+		fputs(msg, stderr);
+		fputc(')', stderr);
+	}
+	fputc('\n', stderr);
+}
+
+
+static void print_push_status(const char *dest, struct ref *refs)
+{
+	struct ref *ref;
+	int shown_dest = 0;
+
+	for (ref = refs; ref; ref = ref->next) {
+		if (!ref->status)
+			continue;
+		if (ref->status == REF_STATUS_UPTODATE && !args.verbose)
+			continue;
+
+		if (!shown_dest) {
+			fprintf(stderr, "To %s\n", dest);
+			shown_dest = 1;
+		}
+
+		switch(ref->status) {
+		case REF_STATUS_NONE:
+			print_ref_status('X', "[no match]", ref, NULL, NULL);
+			break;
+		case REF_STATUS_NODELETE:
+			print_ref_status('!', "[rejected]", ref, NULL,
+					"remote does not support deleting refs");
+			break;
+		case REF_STATUS_UPTODATE:
+			print_ref_status('=', "[up to date]", ref, ref->peer_ref, NULL);
+			break;
+		case REF_STATUS_NONFF:
+			print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
+			break;
+		case REF_STATUS_OK:
+			if (ref->deletion)
+				print_ref_status('-', "[deleted]", ref, NULL, NULL);
+			else if (is_null_sha1(ref->old_sha1))
+				print_ref_status('*',
+						(prefixcmp(ref->name, "refs/tags/") ?
+						  "[new branch]" : "[new tag]"),
+						ref, ref->peer_ref, NULL);
+			else {
+				char quickref[83];
+				char type = ' ';
+				const char *msg = NULL;
+				const char *old_abb;
+
+				old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
+				strcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));
+				if (ref->nonff) {
+					strcat(quickref, "...");
+					type = '+';
+					msg = " (forced update)";
+				}
+				else
+					strcat(quickref, "..");
+				strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+
+				print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+			}
+		}
+	}
+}
+
 static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
 	struct ref *ref;
 	int new_refs;
-	int ret = 0;
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
 	int expect_status_report = 0;
-	int shown_dest = 0;
 	int flags = MATCH_REFS_NONE;
 
 	if (args.send_all)
@@ -262,10 +338,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-		char old_hex[60], *new_hex;
-		int will_delete_ref;
-		const char *pretty_ref;
-		const char *pretty_peer = NULL; /* only used when not deleting */
 		const unsigned char *new_sha1;
 
 		if (!ref->peer_ref) {
@@ -276,29 +348,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		else
 			new_sha1 = ref->peer_ref->new_sha1;
 
-		if (!shown_dest) {
-			fprintf(stderr, "To %s\n", dest);
-			shown_dest = 1;
-		}
-
-		will_delete_ref = is_null_sha1(new_sha1);
-
-		pretty_ref = prettify_ref(ref->name);
-		if (!will_delete_ref)
-			pretty_peer = prettify_ref(ref->peer_ref->name);
 
-		if (will_delete_ref && !allow_deleting_refs) {
-			fprintf(stderr, " ! %-*s %s (remote does not support deleting refs)\n",
-					SUMMARY_WIDTH, "[rejected]", pretty_ref);
-			ret = -2;
+		ref->deletion = is_null_sha1(new_sha1);
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_NODELETE;
 			continue;
 		}
-		if (!will_delete_ref &&
+		if (!ref->deletion &&
 		    !hashcmp(ref->old_sha1, new_sha1)) {
-			if (args.verbose)
-				fprintf(stderr, " = %-*s %s -> %s\n",
-					SUMMARY_WIDTH, "[up to date]",
-					pretty_peer, pretty_ref);
+			ref->status = REF_STATUS_UPTODATE;
 			continue;
 		}
 
@@ -321,33 +379,26 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		 *     always allowed.
 		 */
 
-		if (!args.force_update &&
-		    !will_delete_ref &&
+		ref->nonff =
+		    !ref->deletion &&
 		    !is_null_sha1(ref->old_sha1) &&
-		    !ref->force) {
-			if (!has_sha1_file(ref->old_sha1) ||
-			    !ref_newer(new_sha1, ref->old_sha1)) {
-				/* We do not have the remote ref, or
-				 * we know that the remote ref is not
-				 * an ancestor of what we are trying to
-				 * push.  Either way this can be losing
-				 * commits at the remote end and likely
-				 * we were not up to date to begin with.
-				 */
-				fprintf(stderr, " ! %-*s %s -> %s (non-fast forward)\n",
-						SUMMARY_WIDTH, "[rejected]",
-						pretty_peer, pretty_ref);
-				ret = -2;
-				continue;
-			}
+		    (!has_sha1_file(ref->old_sha1)
+		      || !ref_newer(new_sha1, ref->old_sha1));
+
+		if (ref->nonff && !ref->force && !args.force_update) {
+			ref->status = REF_STATUS_NONFF;
+			continue;
 		}
+
 		hashcpy(ref->new_sha1, new_sha1);
-		if (!will_delete_ref)
+		if (!ref->deletion)
 			new_refs++;
-		strcpy(old_hex, sha1_to_hex(ref->old_sha1));
-		new_hex = sha1_to_hex(ref->new_sha1);
+		ref->status = REF_STATUS_OK;
 
 		if (!args.dry_run) {
+			char *old_hex = sha1_to_hex(ref->old_sha1);
+			char *new_hex = sha1_to_hex(ref->new_sha1);
+
 			if (ask_for_status_report) {
 				packet_write(out, "%s %s %s%c%s",
 					old_hex, new_hex, ref->name, 0,
@@ -359,64 +410,42 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 				packet_write(out, "%s %s %s",
 					old_hex, new_hex, ref->name);
 		}
-		if (will_delete_ref)
-			fprintf(stderr, " - %-*s %s\n",
-				SUMMARY_WIDTH, "[deleting]",
-				pretty_ref);
-		else if (is_null_sha1(ref->old_sha1)) {
-			const char *msg;
-
-			if (!prefixcmp(ref->name, "refs/tags/"))
-				msg = "[new tag]";
-			else
-				msg = "[new branch]";
-			fprintf(stderr, " * %-*s %s -> %s\n",
-				SUMMARY_WIDTH, msg,
-				pretty_peer, pretty_ref);
-		}
-		else {
-			char quickref[83];
-			char type = ' ';
-			const char *msg = "";
-			const char *old_abb;
-			old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
-			strcpy(quickref, old_abb ? old_abb : old_hex);
-			if (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))
-				strcat(quickref, "..");
-			else {
-				strcat(quickref, "...");
-				type = '+';
-				msg = " (forced update)";
-			}
-			strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-
-			fprintf(stderr, " %c %-*s %s -> %s%s\n",
-				type,
-				SUMMARY_WIDTH, quickref,
-				pretty_peer, pretty_ref,
-				msg);
-		}
 	}
 
 	packet_flush(out);
-	if (new_refs && !args.dry_run)
-		ret = pack_objects(out, remote_refs);
+	if (new_refs && !args.dry_run) {
+		if (pack_objects(out, remote_refs) < 0) {
+			close(out);
+			return -1;
+		}
+	}
 	close(out);
 
+	print_push_status(dest, remote_refs);
+
 	if (expect_status_report) {
 		if (receive_status(in))
-			ret = -4;
+			return -1;
 	}
 
-	if (!args.dry_run && remote && ret == 0) {
+	if (!args.dry_run && remote) {
 		for (ref = remote_refs; ref; ref = ref->next)
-			if (!is_null_sha1(ref->new_sha1))
-				update_tracking_ref(remote, ref);
+			update_tracking_ref(remote, ref);
 	}
 
-	if (!new_refs && ret == 0)
+	if (!new_refs)
 		fprintf(stderr, "Everything up-to-date\n");
-	return ret;
+	for (ref = remote_refs; ref; ref = ref->next) {
+		switch (ref->status) {
+		case REF_STATUS_NONE:
+		case REF_STATUS_UPTODATE:
+		case REF_STATUS_OK:
+			break;
+		default:
+			return -1;
+		}
+	}
+	return 0;
 }
 
 static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index 5f40e12..ca5d96d 100644
--- a/cache.h
+++ b/cache.h
@@ -499,8 +499,17 @@ struct ref {
 	struct ref *next;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
-	unsigned char force;
-	unsigned char merge;
+	unsigned char force : 1;
+	unsigned char merge : 1;
+	unsigned char nonff : 1;
+	unsigned char deletion : 1;
+	enum {
+		REF_STATUS_NONE = 0,
+		REF_STATUS_OK,
+		REF_STATUS_NONFF,
+		REF_STATUS_NODELETE,
+		REF_STATUS_UPTODATE,
+	} status;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
-- 
1.5.3.5.1731.g0763

[PATCH v2 2/3] send-pack: check ref->status before updating tracking refs

[Jeff King]

Previously, we manually checked the 'NONE' and 'UPTODATE'
conditions. Now that we have ref->status, we can easily
say "only update if we pushed successfully".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index c01a9c4..5c84766 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -180,24 +180,17 @@ static int receive_status(int in)
 static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
-	int will_delete_ref;
 
-	rs.src = ref->name;
-	rs.dst = NULL;
-
-	if (!ref->peer_ref)
+	if (ref->status != REF_STATUS_OK)
 		return;
 
-	will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
-
-	if (!will_delete_ref &&
-			!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1))
-		return;
+	rs.src = ref->name;
+	rs.dst = NULL;
 
 	if (!remote_find_tracking(remote, &rs)) {
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (is_null_sha1(ref->peer_ref->new_sha1)) {
+		if (ref->deletion) {
 			if (delete_ref(rs.dst, NULL))
 				error("Failed to delete");
 		} else
-- 
1.5.3.5.1731.g0763

[PATCH v2 3/3] send-pack: assign remote errors to each ref

[Jeff King]

This lets us show remote errors (e.g., a denied hook) along
with the usual push output. There are two drawbacks to this
change:

  1. cross-referencing the incoming status with the ref list
     is worst case O(n^2) (where n = number of refs); this
     can be fixed with a smarter implementation

  2. the status parsing is not foolproof. We get a line like

         ng refs/heads/master arbitrary msg

     which cannot be parsed unambiguously in the face of
     refnames with spaces. We do a prefix-match so that
     you will only run into problems if you have two refs,
     one of which is a prefix match of the other, and the
     longer having a space right after the prefix.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |   44 ++++++++++++++++++++++++++++++++++++++------
 cache.h             |    2 ++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5c84766..7d466d9 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,14 +146,34 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static int receive_status(int in)
+static void set_ref_error(struct ref *refs, const char *line) {
+	struct ref *ref;
+
+	for (ref = refs; ref; ref = ref->next) {
+		const char *msg;
+		if (prefixcmp(line, ref->name))
+			continue;
+		msg = line + strlen(ref->name);
+		if (*msg++ != ' ')
+			continue;
+		ref->status = REF_STATUS_REMOTE_REJECT;
+		ref->error = xstrdup(msg);
+		ref->error[strlen(ref->error)-1] = '\0';
+		return;
+	}
+}
+
+/* a return value of -1 indicates that an error occurred,
+ * but we were able to set individual ref errors. A return
+ * value of -2 means we couldn't even get that far. */
+static int receive_status(int in, struct ref *refs)
 {
 	char line[1000];
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
 	if (len < 10 || memcmp(line, "unpack ", 7)) {
 		fprintf(stderr, "did not receive status back\n");
-		return -1;
+		return -2;
 	}
 	if (memcmp(line, "unpack ok\n", 10)) {
 		fputs(line, stderr);
@@ -171,7 +191,7 @@ static int receive_status(int in)
 		}
 		if (!memcmp(line, "ok", 2))
 			continue;
-		fputs(line, stderr);
+		set_ref_error(refs, line + 3);
 		ret = -1;
 	}
 	return ret;
@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)
 		case REF_STATUS_NONFF:
 			print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
 			break;
+		case REF_STATUS_REMOTE_REJECT:
+			if (ref->deletion)
+				print_ref_status('!', "[remote rejected]", ref, NULL, ref->error);
+			else
+				print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
+			break;
 		case REF_STATUS_OK:
 			if (ref->deletion)
 				print_ref_status('-', "[deleted]", ref, NULL, NULL);
@@ -297,6 +323,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	int allow_deleting_refs = 0;
 	int expect_status_report = 0;
 	int flags = MATCH_REFS_NONE;
+	int ret;
 
 	if (args.send_all)
 		flags |= MATCH_REFS_ALL;
@@ -414,12 +441,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	}
 	close(out);
 
-	print_push_status(dest, remote_refs);
-
 	if (expect_status_report) {
-		if (receive_status(in))
+		ret = receive_status(in, remote_refs);
+		if (ret == -2)
 			return -1;
 	}
+	else
+		ret = 0;
+
+	print_push_status(dest, remote_refs);
 
 	if (!args.dry_run && remote) {
 		for (ref = remote_refs; ref; ref = ref->next)
@@ -428,6 +458,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	if (!new_refs)
 		fprintf(stderr, "Everything up-to-date\n");
+	if (ret < 0)
+		return ret;
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
diff --git a/cache.h b/cache.h
index ca5d96d..082e03b 100644
--- a/cache.h
+++ b/cache.h
@@ -509,7 +509,9 @@ struct ref {
 		REF_STATUS_NONFF,
 		REF_STATUS_NODELETE,
 		REF_STATUS_UPTODATE,
+		REF_STATUS_REMOTE_REJECT,
 	} status;
+	char *error;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
-- 
1.5.3.5.1731.g0763

Re: [PATCH/RFC 1/3] send-pack: track errors for each ref

[Alex Riesen]

Jeff King, Tue, Nov 13, 2007 11:27:09 +0100:
> diff --git a/cache.h b/cache.h
> index 5f40e12..ca5d96d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -499,8 +499,17 @@ struct ref {
>  	struct ref *next;
>  	unsigned char old_sha1[20];
>  	unsigned char new_sha1[20];
> -	unsigned char force;
> -	unsigned char merge;
> +	unsigned char force : 1;
> +	unsigned char merge : 1;
> +	unsigned char nonff : 1;

        unsigned char fastforward : 1; ? ffwd?

> +	unsigned char deletion : 1;
> +	enum {
> +		REF_STATUS_NONE = 0,
> +		REF_STATUS_OK,
> +		REF_STATUS_NONFF,

isn't it a duplication of nonff?

Re: [PATCH/RFC 1/3] send-pack: track errors for each ref

[Alex Riesen]

Jeff King, Tue, Nov 13, 2007 11:27:09 +0100:
> Instead of keeping the 'ret' variable, we instead have a
> status flag for each ref that tracks what happened to it.
> We then print the ref status after all of the refs have
> been examined.

It wont apply to current master. How ready is built-in send-pack/push?
Should I fix send-pack.c properly?

Re: [PATCH/RFC 1/3] send-pack: track errors for each ref

[Jeff King]

On Tue, Nov 13, 2007 at 08:04:47PM +0100, Alex Riesen wrote:

> > -	unsigned char force;
> > -	unsigned char merge;
> > +	unsigned char force : 1;
> > +	unsigned char merge : 1;
> > +	unsigned char nonff : 1;
> 
>         unsigned char fastforward : 1; ? ffwd?

I agree it is a little funny to have a negative flag, but it is the
exceptional case to be nonff, so I think it makes the code flow a little
better (you would just be asking for !ref->fastforward all the time). I
am fine with a more readable name.

> > +	unsigned char deletion : 1;
> > +	enum {
> > +		REF_STATUS_NONE = 0,
> > +		REF_STATUS_OK,
> > +		REF_STATUS_NONFF,
> 
> isn't it a duplication of nonff?

No. The nonff flag is "is this push a non-fast forward". The
REF_STATUS_NONFF state is "we didn't push because this is a nonff case".
So you can have nonff = 1 and status = REF_STATUS_OK, which means that
the push was forced (either by ->force, or args.force).

So perhaps a better name is REF_STATUS_REJECT_NONFF (or something longer
if NONFF is too non-obvious).

An alternative is that the deletion and nonff flags can be folded into
the status. But then checking for "is everything OK" becomes
non-obvious (it's something like REF_STATUS_NONFF_FORCED ||
REF_STATUS_DELETION_OK || REF_STATUS_PUSH_OK). I tried it that way and
found the code is a bit more readable by pulling those features away
from status (so status is "did we succeed, or did we fail, and if the
latter, for what reason?").

-Peff

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This lets us show remote errors (e.g., a denied hook) along
> with the usual push output. ...

Yay!

> ... There are two drawbacks to this
> change:
>
>   1. cross-referencing the incoming status with the ref list
>      is worst case O(n^2) (where n = number of refs); this
>      can be fixed with a smarter implementation

Sure.

>   2. the status parsing is not foolproof. We get a line like
>
>          ng refs/heads/master arbitrary msg
>
>      which cannot be parsed unambiguously in the face of
>      refnames with spaces. We do a prefix-match so that
>      you will only run into problems if you have two refs,
>      one of which is a prefix match of the other, and the
>      longer having a space right after the prefix.

Is it really "arbitrary msg", or just a fixed set of strings?

Also I think we can rely on the order report-status extension
reports the per-ref result.  It gives back the information the
same order send-pack side supplies the head information, no?

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>   2. the status parsing is not foolproof. We get a line like
>>
>>          ng refs/heads/master arbitrary msg
>>
>>      which cannot be parsed unambiguously...
>
> Is it really "arbitrary msg", or just a fixed set of strings?

This was a wrong thing to say.  We may add new error message in
later versions of receive-pack and we want to make it compatible
with today's send-pack with minimum hassle.  So that is a
problem.  But...

> Also I think we can rely on the order report-status extension
> reports the per-ref result.  It gives back the information the
> same order send-pack side supplies the head information, no?

I was hoping that this can solve both of the problems you
cited...

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   2. the status parsing is not foolproof. We get a line like
> >
> >          ng refs/heads/master arbitrary msg
> >
> >      which cannot be parsed unambiguously in the face of
> >      refnames with spaces.

Since when can refnames contain spaces?  In my copy of git, bad_ref_char() 
in refs.c returns 1 if ch <= ' '.  It's the first error path, even.

Ciao,
Dscho

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> >   2. the status parsing is not foolproof. We get a line like
>> >
>> >          ng refs/heads/master arbitrary msg
>> >
>> >      which cannot be parsed unambiguously in the face of
>> >      refnames with spaces.
>
> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() 
> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.

Bah.  You are right.

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Jeff King]

On Tue, Nov 13, 2007 at 05:41:39PM -0800, Junio C Hamano wrote:

> Is it really "arbitrary msg", or just a fixed set of strings?

It is for a fixed version of the remote receive-pack, but I don't want
to start relying on that.

> Also I think we can rely on the order report-status extension
> reports the per-ref result.  It gives back the information the
> same order send-pack side supplies the head information, no?

I considered that, but I didn't want to rely on it without your say-so.
We could also just use it as a hint to boost performance. I.e., track
the last match, and start our linear search there, but if we fail, drop
back to searching the whole list. Something like (on top of my other
patch):

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 7d466d9..8e9580a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,7 +146,8 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static void set_ref_error(struct ref *refs, const char *line) {
+static struct ref *set_ref_error(struct ref *refs, const char *line)
+{
 	struct ref *ref;
 
 	for (ref = refs; ref; ref = ref->next) {
@@ -159,8 +160,9 @@ static void set_ref_error(struct ref *refs, const char *line) {
 		ref->status = REF_STATUS_REMOTE_REJECT;
 		ref->error = xstrdup(msg);
 		ref->error[strlen(ref->error)-1] = '\0';
-		return;
+		return ref;
 	}
+	return NULL;
 }
 
 /* a return value of -1 indicates that an error occurred,
@@ -168,6 +170,7 @@ static void set_ref_error(struct ref *refs, const char *line) {
  * value of -2 means we couldn't even get that far. */
 static int receive_status(int in, struct ref *refs)
 {
+	struct ref *hint;
 	char line[1000];
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
@@ -179,6 +182,7 @@ static int receive_status(int in, struct ref *refs)
 		fputs(line, stderr);
 		ret = -1;
 	}
+	hint = NULL;
 	while (1) {
 		len = packet_read_line(in, line, sizeof(line));
 		if (!len)
@@ -191,7 +195,10 @@ static int receive_status(int in, struct ref *refs)
 		}
 		if (!memcmp(line, "ok", 2))
 			continue;
-		set_ref_error(refs, line + 3);
+		if (hint)
+			hint = set_ref_error(hint, line + 3);
+		if (!hint)
+			hint = set_ref_error(refs, line + 3);
 		ret = -1;
 	}
 	return ret;

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Jeff King]

On Wed, Nov 14, 2007 at 02:01:14AM +0000, Johannes Schindelin wrote:

> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() 
> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.

Oops, I'm clearly an idiot. I explicitly looked at bad_ref_char before
writing the original message, and somehow read that as strictly less
than.

So the parsing problem goes away, and I think using the sort order as a
hint takes away the potential performance problem (I don't even know if
it was a problem -- there may be other O(n^2) behavior).

I will take a look at the tests Alex has been working on, maybe add a
few to it, and submit a cleaned-up series.

-Peff

Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 14, 2007 at 02:01:14AM +0000, Johannes Schindelin wrote:
>
>> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() 
>> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.
>
> Oops, I'm clearly an idiot. I explicitly looked at bad_ref_char before
> writing the original message, and somehow read that as strictly less
> than.
>
> So the parsing problem goes away, and I think using the sort order as a
> hint takes away the potential performance problem (I don't even know if
> it was a problem -- there may be other O(n^2) behavior).
>
> I will take a look at the tests Alex has been working on, maybe add a
> few to it, and submit a cleaned-up series.

Thanks.