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

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

[Jeff King]

Here are cleaned-up versions of the previous patches I sent. The
improvements are:

  1/3 send-pack: track errors for each ref
    - there was a t5404 regression because I removed the is_null_sha1()
      check before updating a tracking ref (but the replacement fix
      doesn't come until 2/3). Even though this check is not correct,
      it's better to fix it all at once correctly in 2/3.
    - clarified the desired git-push exit code in t5404
    - I renamed the struct elements to (hopefully) be a bit more obvious
    - readability cleanups (fixed some very long lines, hoisted some
      code into its own functions)

  2/3 send-pack: check ref->status before updating tracking refs
    - moved in fix from 1/3 mentioned above
    - add test for deleting tracking branches, which was broken in next
      but fixed by this patch

  3/3 send-pack: assign remote errors to each ref
    - squashed optimization patch
    - remove bogus parsing drawback in commit message
    - add test

I'm hoping to get feedback from the cc'd people:
  - Alex: please OK the modifications to t5404
  - Pierre: this should fix the tracking ref update issues you reported
  - Daniel: a general OK, since I am mangling your code :)

-Peff

[PATCH 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          |  225 +++++++++++++++++++++++++-----------------
 cache.h                      |   13 ++-
 t/t5404-tracking-branches.sh |   14 ++-
 3 files changed, 157 insertions(+), 95 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 418925e..90ca2d3 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,105 @@ 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 const char *status_abbrev(unsigned char sha1[20])
+{
+	const char *abbrev;
+	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
+	return abbrev ? abbrev : sha1_to_hex(sha1);
+}
+
+static void print_ok_ref_status(struct ref *ref)
+{
+	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 tag]" :
+			  "[new branch]"),
+			ref, ref->peer_ref, NULL);
+	else {
+		char quickref[83];
+		char type;
+		const char *msg;
+
+		strcpy(quickref, status_abbrev(ref->old_sha1));
+		if (ref->nonfastforward) {
+			strcat(quickref, "...");
+			type = '+';
+			msg = " (forced update)";
+		}
+		else {
+			strcat(quickref, "..");
+			type = ' ';
+			msg = NULL;
+		}
+		strcat(quickref, status_abbrev(ref->new_sha1));
+
+		print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+	}
+}
+
+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_REJECT_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_REJECT_NONFASTFORWARD:
+			print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+					"non-fast forward");
+			break;
+		case REF_STATUS_OK:
+			print_ok_ref_status(ref);
+			break;
+		}
+	}
+}
+
 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 +353,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 +363,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_REJECT_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 +394,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->nonfastforward =
+		    !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->nonfastforward && !ref->force && !args.force_update) {
+			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+			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 +425,43 @@ 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);
 	}
 
-	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 e9b3ce3..2768342 100644
--- a/cache.h
+++ b/cache.h
@@ -500,8 +500,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 nonfastforward : 1;
+	unsigned char deletion : 1;
+	enum {
+		REF_STATUS_NONE = 0,
+		REF_STATUS_OK,
+		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_UPTODATE,
+	} status;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 20718d4..799e47e 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -19,7 +19,7 @@ test_expect_success 'setup' '
 	git commit -a -m b2
 '
 
-test_expect_success 'check tracking branches updated correctly after push' '
+test_expect_success 'prepare pushable branches' '
 	cd aa &&
 	b1=$(git rev-parse origin/b1) &&
 	b2=$(git rev-parse origin/b2) &&
@@ -31,8 +31,16 @@ test_expect_success 'check tracking branches updated correctly after push' '
 	git commit -a -m aa-b2 &&
 	git checkout master &&
 	echo aa-master >>file &&
-	git commit -a -m aa-master &&
-	git push &&
+	git commit -a -m aa-master
+'
+
+test_expect_success 'mixed-success push returns error' '! git push'
+
+test_expect_success 'check tracking branches updated correctly after push' '
+	test "$(git rev-parse origin/master)" = "$(git rev-parse master)"
+'
+
+test_expect_success 'check tracking branches not updated for failed refs' '
 	test "$(git rev-parse origin/b1)" = "$b1" &&
 	test "$(git rev-parse origin/b2)" = "$b2"
 '
-- 
1.5.3.5.1795.gf2a4e-dirty

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

[Alex Riesen]

Jeff King, Sat, Nov 17, 2007 13:54:27 +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.
> 
> 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>

Acked-by: Alex Riesen <raa.lkml@gmail.com>

Still would like to see the deletion of tracing branches checked.
Like this perhaps?

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 799e47e..4fe4a07 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -10,6 +10,7 @@ test_expect_success 'setup' '
 	git commit -m 1 &&
 	git branch b1 &&
 	git branch b2 &&
+	git branch b3 &&
 	git clone . aa &&
 	git checkout b1 &&
 	echo b1 >>file &&
@@ -23,6 +24,7 @@ test_expect_success 'prepare pushable branches' '
 	cd aa &&
 	b1=$(git rev-parse origin/b1) &&
 	b2=$(git rev-parse origin/b2) &&
+	b3=$(git rev-parse origin/b3) &&
 	git checkout -b b1 origin/b1 &&
 	echo aa-b1 >>file &&
 	git commit -a -m aa-b1 &&
@@ -45,4 +47,12 @@ test_expect_success 'check tracking branches not updated for failed refs' '
 	test "$(git rev-parse origin/b2)" = "$b2"
 '
 
+test_expect_success 'delete remote branch' '
+	git push origin :refs/heads/b3 &&
+	{
+		git rev-parse --verify origin/b3
+		test $? != 0
+        }
+'
+
 test_done
-- 
1.5.3.5.747.gf06543

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

[Daniel Barkalow]

On Sat, 17 Nov 2007, Jeff King wrote:

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 418925e..90ca2d3 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -218,15 +219,105 @@ 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)

Isn't "from" always "to->peer_ref"? It'd be nice to make this function 
unable to print something different from what we actually did. (Actually 
it might be "to->deletion ? NULL : to->peer_ref", but that would also be 
better to have as an explicit feature of how you display "to", rather than 
implicit in the set of callers.

> +static const char *status_abbrev(unsigned char sha1[20])
> +{
> +	const char *abbrev;
> +	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> +	return abbrev ? abbrev : sha1_to_hex(sha1);
> +}

Maybe we should have a find_unique_abbrev()-like function that doesn't 
mind if the requested object doesn't exist?

> +
> +static void print_ok_ref_status(struct ref *ref)
> +{
> +	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 tag]" :
> +			  "[new branch]"),
> +			ref, ref->peer_ref, NULL);
> +	else {
> +		char quickref[83];

Shouldn't this be 40 + 3 + 40 + 1?

> +		char type;
> +		const char *msg;
> +
> +		strcpy(quickref, status_abbrev(ref->old_sha1));
> +		if (ref->nonfastforward) {
> +			strcat(quickref, "...");
> +			type = '+';
> +			msg = " (forced update)";
> +		}
> +		else {

Coding style, IIRC.

Regardless of these nits, it all looks good to me.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*

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

[Jeff King]

On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:

> > +static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
> 
> Isn't "from" always "to->peer_ref"? It'd be nice to make this function 
> unable to print something different from what we actually did. (Actually 
> it might be "to->deletion ? NULL : to->peer_ref", but that would also be 
> better to have as an explicit feature of how you display "to", rather than 
> implicit in the set of callers.

Yes, I also considered changing "from" to "show peer" which might have
been nicer. I am not opposed to such a cleanup, but again, not sure if
it worth it now that we are merged.

> > +static const char *status_abbrev(unsigned char sha1[20])
> > +{
> > +	const char *abbrev;
> > +	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> > +	return abbrev ? abbrev : sha1_to_hex(sha1);
> > +}
> 
> Maybe we should have a find_unique_abbrev()-like function that doesn't 
> mind if the requested object doesn't exist?

I agree, though I don't feel qualified to comment on what other places
that should be used (I was a bit surprised to find out that
find_unique_abbrev ever returned NULL, but changing the semantics at
this point is probably going to cause some subtle bug).

> > +		char quickref[83];
> Shouldn't this be 40 + 3 + 40 + 1?

Oops, yes. I think it should be hard to trigger (both commits would have
to either not be in your db, or not be unique to 40 digits). But clearly
it should be fixed, and it looks like Junio did.

It was a stupid cut-and-paste from Nicolas' fetch code, but it looks
like he correctly allocates 84 bytes for the "..." case.

> > +		char type;
> > +		const char *msg;
> > +
> > +		strcpy(quickref, status_abbrev(ref->old_sha1));
> > +		if (ref->nonfastforward) {
> > +			strcat(quickref, "...");
> > +			type = '+';
> > +			msg = " (forced update)";
> > +		}
> > +		else {
> 
> Coding style, IIRC.

Sorry, I don't see the style nit you're mentioning here.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:
>
>> > +		if (ref->nonfastforward) {
>> > +			strcat(quickref, "...");
>> > +			type = '+';
>> > +			msg = " (forced update)";
>> > +		}
>> > +		else {
>> 
>> Coding style, IIRC.
>
> Sorry, I don't see the style nit you're mentioning here.

I think Daniel is referring to "Put 'else' on the same line as
the brace that closes the corresponding 'if' opened", iow:

	if (...) {
        	...
	} else {
		...
	}

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

[Jeff King]

On Sat, Nov 17, 2007 at 05:21:59PM -0800, Junio C Hamano wrote:

> > Sorry, I don't see the style nit you're mentioning here.
> 
> I think Daniel is referring to "Put 'else' on the same line as
> the brace that closes the corresponding 'if' opened", iow:
> 
> 	if (...) {
>         	...
> 	} else {
> 		...
> 	}

Ah. We are really inconsistent about that (I count about 333 non-cuddled
(but possible to cuddle) versus 499 cuddled). Should we put something in
the CodingGuidelines? I am also happy to roll this change into a style
patch.

For reference, I counted with:

# cuddled:
grep '} *else' *.c | wc -l

# possible but non-cuddled
# when we find an 'else', if the previous line had a '}'
# on it, then it is a hit. There are a few false positives
# when the "} else" is from an outer block scope
# And no, this sed invocation probably isn't totally portable. :)
sed -n '/else/{x; /}/{x;p}}; h' *.c | wc -l
# or to check the accuracy
sed -n '/else/{x; /}/{p;x;p}}; h' *.c

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Looks good.

Except that

> @@ -218,15 +219,105 @@ 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);
> +}

msg is parenthesized here, so...

> +static void print_ok_ref_status(struct ref *ref)
> +{
> +	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 tag]" :
> +			  "[new branch]"),
> +			ref, ref->peer_ref, NULL);
> +	else {
> +		char quickref[83];
> +		char type;
> +		const char *msg;
> +
> +		strcpy(quickref, status_abbrev(ref->old_sha1));
> +		if (ref->nonfastforward) {
> +			strcat(quickref, "...");
> +			type = '+';
> +			msg = " (forced update)";

... you do not need the " (" and ")" here.

All merged to 'next'.  Thanks.

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

[Jeff King]

On Sat, Nov 17, 2007 at 12:53:14PM -0800, Junio C Hamano wrote:

> > +		strcpy(quickref, status_abbrev(ref->old_sha1));
> > +		if (ref->nonfastforward) {
> > +			strcat(quickref, "...");
> > +			type = '+';
> > +			msg = " (forced update)";
> 
> ... you do not need the " (" and ")" here.

Oops, good catch.

> All merged to 'next'.  Thanks.

I see you fixed up the off-by-one buffer, too. Thanks.

-Peff

[PATCH 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".

This adds a test for and fixes a regression introduced in
ed31df31 where deleted refs did not have their tracking
branches removed. This was due to a bogus per-ref error test
that is superseded by the more accurate ref->status flag.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c          |   18 +++++-------------
 t/t5404-tracking-branches.sh |    5 +++++
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 90ca2d3..c7d07aa 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
@@ -445,8 +438,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	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)
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 799e47e..1493a92 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -45,4 +45,9 @@ test_expect_success 'check tracking branches not updated for failed refs' '
 	test "$(git rev-parse origin/b2)" = "$b2"
 '
 
+test_expect_success 'deleted branches have their tracking branches removed' '
+	git push origin :b1 &&
+	test "$(git rev-parse origin/b1)" = "origin/b1"
+'
+
 test_done
-- 
1.5.3.5.1795.gf2a4e-dirty

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

[Alex Riesen]

Jeff King, Sat, Nov 17, 2007 13:55:15 +0100:
> diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
> index 799e47e..1493a92 100755
> --- a/t/t5404-tracking-branches.sh
> +++ b/t/t5404-tracking-branches.sh
> @@ -45,4 +45,9 @@ test_expect_success 'check tracking branches not updated for failed refs' '
>  	test "$(git rev-parse origin/b2)" = "$b2"
>  '
>  
> +test_expect_success 'deleted branches have their tracking branches removed' '
> +	git push origin :b1 &&
> +	test "$(git rev-parse origin/b1)" = "origin/b1"
> +'
> +
>  test_done

Oh, missed that.

Completely-Acked-By: Alex "Sleepy" Riesen <raa.lkml@gmail.com>

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

[Jeff King]

On Sat, Nov 17, 2007 at 02:45:46PM +0100, Alex Riesen wrote:

> > +test_expect_success 'deleted branches have their tracking branches removed' '
> > +	git push origin :b1 &&
> > +	test "$(git rev-parse origin/b1)" = "origin/b1"
> > +'
> > +
> >  test_done
> 
> Oh, missed that.
> 
> Completely-Acked-By: Alex "Sleepy" Riesen <raa.lkml@gmail.com>

Yes, I didn't put it in the first patch, because it was broken there. ;)

-Peff

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

[Daniel Barkalow]

On Sat, 17 Nov 2007, Jeff King wrote:

> 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".
> 
> This adds a test for and fixes a regression introduced in
> ed31df31 where deleted refs did not have their tracking
> branches removed. This was due to a bogus per-ref error test
> that is superseded by the more accurate ref->status flag.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*

[PATCH 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 is a slightly clever optimization in receive_status
that bears explanation. We need to correlate the returned
status and our ref objects, which naively could be an O(m*n)
operation. However, since the current implementation of
receive-pack returns the errors to us in the same order that
we sent them, we optimistically look for the next ref to be
looked up to come after the last one we have found. So it
should be an O(m+n) merge if the receive-pack behavior
holds, but we fall back to a correct but slower behavior if
it should change.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c       |   51 +++++++++++++++++++++++++++++++++++++++-----
 cache.h                   |    2 +
 t/t5406-remote-rejects.sh |   24 +++++++++++++++++++++
 3 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100755 t/t5406-remote-rejects.sh

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index c7d07aa..bcf7143 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,19 +146,43 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static int receive_status(int in)
+static struct ref *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 ref;
+	}
+	return NULL;
+}
+
+/* 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)
+{
+	struct ref *hint;
 	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);
 		ret = -1;
 	}
+	hint = NULL;
 	while (1) {
 		len = packet_read_line(in, line, sizeof(line));
 		if (!len)
@@ -171,7 +195,10 @@ static int receive_status(int in)
 		}
 		if (!memcmp(line, "ok", 2))
 			continue;
-		fputs(line, stderr);
+		if (hint)
+			hint = set_ref_error(hint, line + 3);
+		if (!hint)
+			hint = set_ref_error(refs, line + 3);
 		ret = -1;
 	}
 	return ret;
@@ -297,6 +324,12 @@ static void print_push_status(const char *dest, struct ref *refs)
 			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:
 			print_ok_ref_status(ref);
 			break;
@@ -312,6 +345,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;
@@ -429,12 +463,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)
@@ -443,6 +480,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 2768342..ba9178f 100644
--- a/cache.h
+++ b/cache.h
@@ -510,7 +510,9 @@ struct ref {
 		REF_STATUS_REJECT_NONFASTFORWARD,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
+		REF_STATUS_REMOTE_REJECT,
 	} status;
+	char *error;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
diff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh
new file mode 100755
index 0000000..46b2cb4
--- /dev/null
+++ b/t/t5406-remote-rejects.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+test_description='remote push rejects are reported by client'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir .git/hooks &&
+	(echo "#!/bin/sh" ; echo "exit 1") >.git/hooks/update &&
+	chmod +x .git/hooks/update &&
+	echo 1 >file &&
+	git add file &&
+	git commit -m 1 &&
+	git clone . child &&
+	cd child &&
+	echo 2 >file &&
+	git commit -a -m 2
+'
+
+test_expect_success 'push reports error' '! git push 2>stderr'
+
+test_expect_success 'individual ref reports error' 'grep rejected stderr'
+
+test_done
-- 
1.5.3.5.1795.gf2a4e-dirty

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

[Daniel Barkalow]

On Sat, 17 Nov 2007, Jeff King wrote:

> This lets us show remote errors (e.g., a denied hook) along
> with the usual push output.
> 
> There is a slightly clever optimization in receive_status
> that bears explanation. We need to correlate the returned
> status and our ref objects, which naively could be an O(m*n)
> operation. However, since the current implementation of
> receive-pack returns the errors to us in the same order that
> we sent them, we optimistically look for the next ref to be
> looked up to come after the last one we have found. So it
> should be an O(m+n) merge if the receive-pack behavior
> holds, but we fall back to a correct but slower behavior if
> it should change.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin-send-pack.c       |   51 +++++++++++++++++++++++++++++++++++++++-----
>  cache.h                   |    2 +
>  t/t5406-remote-rejects.sh |   24 +++++++++++++++++++++
>  3 files changed, 71 insertions(+), 6 deletions(-)
>  create mode 100755 t/t5406-remote-rejects.sh
> 
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index c7d07aa..bcf7143 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -146,19 +146,43 @@ static void get_local_heads(void)
>  	for_each_ref(one_local_ref, NULL);
>  }
>  
> -static int receive_status(int in)
> +static struct ref *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 ref;
> +	}
> +	return NULL;
> +}

Maybe this should take both the full list and the hint and do both passes 
internally? IMHO, the logic in receive_status() looks like it might be 
setting the error twice or not at all, unless you read very carefully.

But, regardless,

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*

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

[Jeff King]

On Sat, Nov 17, 2007 at 01:05:29PM -0500, Daniel Barkalow wrote:

> Maybe this should take both the full list and the hint and do both passes 
> internally? IMHO, the logic in receive_status() looks like it might be 
> setting the error twice or not at all, unless you read very carefully.

Not unreasonable, though I started in that direction and it ended up
being less readable (which isn't to say that it's not possible). But it
looks like Junio has merged to next, so I think it is best to leave it
as-is.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index c7d07aa..bcf7143 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -146,19 +146,43 @@ static void get_local_heads(void)
>  	for_each_ref(one_local_ref, NULL);
>  }
>  
> -static int receive_status(int in)
> +static struct ref *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 ref;
> +	}
> +	return NULL;
> +}

It probably would not matter for sane repositories, but with
thousands of refs, strlen() and prefixcmp() may start to hurt:

	struct ref *ref;
	int reflen;
	const char *msg = strchr(line, ' ');

        if (!msg)
        	return NULL;
	reflen = msg - line;
	msg++;
	for (ref = refs; ref; ref = ref->next) {
        	if (strncmp(line, ref->name, reflen) || line[reflen] != ' ')
			continue;
		...
		return ref->next;
	}
	return NULL;

but the "hint" optimization probably make the above
micro-optimization irrelevant.

> +/* 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. */

It is preferred to have a multi-line comment like this:

	/*
         * A return value of -1 ...
	 * ...
	 * ... couldn't even get that far.
	 */

> +static int receive_status(int in, struct ref *refs)
> ...
> +	hint = NULL;
>  	while (1) {
>  		len = packet_read_line(in, line, sizeof(line));
>  		if (!len)
> @@ -171,7 +195,10 @@ static int receive_status(int in)
>  		}
>  		if (!memcmp(line, "ok", 2))
>  			continue;
> -		fputs(line, stderr);
> +		if (hint)
> +			hint = set_ref_error(hint, line + 3);
> +		if (!hint)
> +			hint = set_ref_error(refs, line + 3);

Clever... taking advantage of the order receive-pack reports to
optimize.

Before receive_status() is called, can the refs already have the
error status and string set?

> @@ -429,12 +463,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;

Hmm.  When we did not receive status, we cannot tell what
succeeded or failed, but what we _can_ tell the user is which
refs we attempted to push.  I wonder if robbing that information
from the user with this "return -1" is a good idea.  Perhaps we
would instead want to set the status of all the refs to error
and call print_push_status() anyway?  I dunno.

>  	}
> +	else
> +		ret = 0;
> +
> +	print_push_status(dest, remote_refs);
>  
>  	if (!args.dry_run && remote) {
>  		for (ref = remote_refs; ref; ref = ref->next)

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

[Jeff King]

On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:

> > +	for (ref = refs; ref; ref = ref->next) {
> > +		const char *msg;
> > +		if (prefixcmp(line, ref->name))
> > +			continue;
> 
> It probably would not matter for sane repositories, but with
> thousands of refs, strlen() and prefixcmp() may start to hurt:

It is actually _just_ prefixcmp. Or do you mean the strlen we call in
prefixcmp? If so, I think the right solution is to make prefixcmp
faster.  :)

> but the "hint" optimization probably make the above
> micro-optimization irrelevant.

Agreed.

> It is preferred to have a multi-line comment like this:
> 
> 	/*
>          * A return value of -1 ...
> 	 * ...
> 	 * ... couldn't even get that far.
> 	 */

OK. Since it is already in next, do you want a style fixup patch?

> Before receive_status() is called, can the refs already have the
> error status and string set?

Nothing else sets the string, so the latter is not possible (perhaps it
should be "remote_error" for clarity). It is less clear that we are not
overwriting another status; however, if you look at do_send_pack, we
only actually send the remote refs that are getting REF_STATUS_OK.

A broken or malicious remote could change the push status of an
arbitrary ref to rejection, but I don't really see the point. We could
explicitly check that we are changing from OK to REMOTE_REJECTED in
set_ref_error.

> >  	if (expect_status_report) {
> > -		if (receive_status(in))
> > +		ret = receive_status(in, remote_refs);
> > +		if (ret == -2)
> >  			return -1;
> 
> Hmm.  When we did not receive status, we cannot tell what
> succeeded or failed, but what we _can_ tell the user is which
> refs we attempted to push.  I wonder if robbing that information
> from the user with this "return -1" is a good idea.  Perhaps we
> would instead want to set the status of all the refs to error
> and call print_push_status() anyway?  I dunno.

That is a reasonable behavior (although they have already seen an
"error: " message, I think). We might also consider returning something
besides "-1" to differentiate "ok, but some refs failed" from "terribly
broken". The old code used to use "-2" and "-4", but I checked and all
of the error checking paths seemed to end up as a boolean.

I can work up a patch if there is consensus.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:
>
>> > +	for (ref = refs; ref; ref = ref->next) {
>> > +		const char *msg;
>> > +		if (prefixcmp(line, ref->name))
>> > +			continue;
>> 
>> It probably would not matter for sane repositories, but with
>> thousands of refs, strlen() and prefixcmp() may start to hurt:
>
> It is actually _just_ prefixcmp. Or do you mean the strlen we call in
> prefixcmp? If so, I think the right solution is to make prefixcmp
> faster.  :)

I was referring to strlen(ref->name) taken for all refs during
the loop.  Micro-optimized one finds the end of refname on the
"ng" line once before entering the loop.

> OK. Since it is already in next, do you want a style fixup patch?

I do not think it is particularly a big deal -- perhaps clean it
before we touch the vicinity of the code next time.  The same
goes for the "} else {" stuff.

>> Before receive_status() is called, can the refs already have the
>> error status and string set?
>
> Nothing else sets the string, so the latter is not possible (perhaps it
> should be "remote_error" for clarity). It is less clear that we are not
> ... if you look at do_send_pack, we
> only actually send the remote refs that are getting REF_STATUS_OK.

Ah, Ok.

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

[Jeff King]

On Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:

>>> +     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 ref;
>>> +     }
>>> +     return NULL;
>>> +}
> > It is actually _just_ prefixcmp. Or do you mean the strlen we call in
> > prefixcmp? If so, I think the right solution is to make prefixcmp
> > faster.  :)
> I was referring to strlen(ref->name) taken for all refs during
> the loop.  Micro-optimized one finds the end of refname on the
> "ng" line once before entering the loop.

I don't see such a strlen, except in the implementation of prefixcmp,
because we continue most of the time based on its result. If you have a
false match on the prefixcmp (i.e., a prefix of another ref), you do an
extra strlen.

But I don't think this is worth micro-optimizing.

-Peff