threads / rfc / 10843

RFC patch, 3 partstracking per-ref errors on push

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

## tl;dr

20 messages between Nov 13, 2007 and Nov 16, 2007. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Jeff King· Nov 13, 2007, 10:25 UTC · lore

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
Jeff King· Nov 13, 2007, 10:27 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +121 −93

builtin-send-pack.c, cache.h

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
Jeff King· Nov 13, 2007, 11:32 UTC · re: Jeff King · lore

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

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:
---
Show changes to builtin-send-pack.c +10 −0
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;
 }
 
Alex Riesen· Nov 13, 2007, 19:04 UTC · re: Jeff King · lore

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

Jeff King, Tue, Nov 13, 2007 11:27:09 +0100:
Show 13 quoted lines
> 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?
Show 5 quoted lines
> +	unsigned char deletion : 1;
> +	enum {
> +		REF_STATUS_NONE = 0,
> +		REF_STATUS_OK,
> +		REF_STATUS_NONFF,
isn't it a duplication of nonff?
Jeff King· Nov 13, 2007, 22:30 UTC · re: Alex Riesen · lore

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

On Tue, Nov 13, 2007 at 08:04:47PM +0100, Alex Riesen wrote:
Show 7 quoted lines
> > -	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.

Show 7 quoted lines
> > +	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
Alex Riesen· Nov 13, 2007, 20:18 UTC · re: Jeff King · lore

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

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?

Jeff King· Nov 13, 2007, 10:27 UTC · re: Jeff King · lore

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

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(-)
Show changes to builtin-send-pack.c +4 −11
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
Jeff King· Nov 13, 2007, 10:29 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +39 −7

builtin-send-pack.c, cache.h

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
Jeff King· Nov 13, 2007, 10:29 UTC · re: Jeff King · lore

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

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
Jeff King· Nov 13, 2007, 11:34 UTC · re: Jeff King · lore

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

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
Jeff King· Nov 13, 2007, 11:36 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +132 −94

builtin-send-pack.c, cache.h

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
Jeff King· Nov 13, 2007, 11:36 UTC · re: Jeff King · lore

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

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(-)
Show changes to builtin-send-pack.c +4 −11
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
Jeff King· Nov 13, 2007, 11:37 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +40 −6

builtin-send-pack.c, cache.h

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
Junio C Hamano· Nov 14, 2007, 01:41 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> This lets us show remote errors (e.g., a denied hook) along
> with the usual push output. ...
Yay!
Show 6 quoted lines
> ... 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.
Show 9 quoted lines
>   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?

Junio C Hamano· Nov 14, 2007, 01:56 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
>>   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...

Johannes Schindelin· Nov 14, 2007, 02:01 UTC · re: Junio C Hamano · lore

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

Hi,
On Tue, 13 Nov 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 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

Junio C Hamano· Nov 14, 2007, 06:12 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
>> >   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.
Jeff King· Nov 15, 2007, 04:54 UTC · re: Johannes Schindelin · lore

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

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
Junio C Hamano· Nov 16, 2007, 05:05 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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.
Jeff King· Nov 15, 2007, 04:47 UTC · re: Junio C Hamano · lore

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

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):

Show changes to builtin-send-pack.c +10 −3
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;

← back to recent threads