bug: origin refs updated too soon locally

bug: origin refs updated too soon locally

[Perry Wagle]

If I clone a remote repository, make a few commits, push them to the  
remote repository, and the update hook on the remote repository  
rejects them (exit 1), the local origin refs are still updated as if  
the push had gone through.  The workaround is to do a pull to set the  
origin refs back.

-- Perry

Re: bug: origin refs updated too soon locally

[Perry Wagle]

I take it back.  A git-pull is not a workaround if the ref moved on  
the remote end.

-- Perry


On Oct 17, 2007, at 6:35 PM, Perry Wagle wrote:

> If I clone a remote repository, make a few commits, push them to  
> the remote repository, and the update hook on the remote repository  
> rejects them (exit 1), the local origin refs are still updated as  
> if the push had gone through.  The workaround is to do a pull to  
> set the origin refs back.
>
> -- Perry
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bug: origin refs updated too soon locally

[Shawn O. Pearce]

Perry Wagle <wagle@cs.indiana.edu> wrote:
> If I clone a remote repository, make a few commits, push them to the  
> remote repository, and the update hook on the remote repository  
> rejects them (exit 1), the local origin refs are still updated as if  
> the push had gone through.  The workaround is to do a pull to set the  
> origin refs back.

Heh.  Yes, that's a known bug.  Someone should really fix it.
The problem is we are updating the local tracking ref before we
actually get confirmation from the remote side that the remote side
has accepted (or rejected) that update request.

This is probably easier to do after the db/fetch-pack topic is
merged as the improvements there might make this easier.  But I
could be wrong.  Be nice if someone proved me wrong by writing up
a patch for git-send-pack.

For the time being the best way to recover from this is to use
git-fetch rather than git-pull.  Recall that git-pull is defined as
"fetch then merge".  You really just need to refetch the tracking
branches again, so your tracking branches have the same value as
the remote side.

-- 
Shawn.

Re: bug: origin refs updated too soon locally

[Jeff King]

On Thu, Oct 18, 2007 at 12:53:58AM -0400, Shawn O. Pearce wrote:

> This is probably easier to do after the db/fetch-pack topic is
> merged as the improvements there might make this easier.  But I
> could be wrong.  Be nice if someone proved me wrong by writing up
> a patch for git-send-pack.

It doesn't look too bad...patch series in a few minutes.

-Peff

[PATCH] t5516: test update of local refs on push

[Jeff King]

The first test (updating local refs) should succeed, but the
second one (not updating on error) currently fails.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5516-fetch-push.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ca46aaf..dd329d7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,4 +244,32 @@ test_expect_success 'push with colon-less refspec (4)' '
 
 '
 
+test_expect_success 'push updates local refs' '
+
+	rm -rf parent child &&
+	mkdir parent && cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one &&
+	cd .. &&
+	git clone parent child && cd child &&
+		echo two >foo && git commit -a -m two &&
+		git push &&
+	test $(git rev-parse master) = $(git rev-parse remotes/origin/master)
+
+'
+
+test_expect_success 'push does not update local refs on failure' '
+
+	rm -rf parent child &&
+	mkdir parent && cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one &&
+		echo exit 1 >.git/hooks/pre-receive &&
+		chmod +x .git/hooks/pre-receive &&
+	cd .. &&
+	git clone parent child && cd child &&
+		echo two >foo && git commit -a -m two || exit 1
+		git push && exit 1
+	test $(git rev-parse master) != $(git rev-parse remotes/origin/master)
+
+'
+
 test_done
-- 
1.5.3.4.1162.gc3e8e-dirty

[PATCH 2/2] send-pack: don't update tracking refs on error

[Jeff King]

Previously, we updated the tracking refs (which match refs we
are pushing) while generating the list of refs to send.
However, at that point we don't know whether the refs were
accepted.

Instead, we now wait until we get a response code from the
server. If an error was indicated, we don't update any local
tracking refs. Technically some refs could have been updated
on the remote, but since the local ref update is just an
optimization to avoid an extra fetch, we are better off
erring on the side of correctness.

The user-visible message is now generated much later in the
program, and has been tweaked to make more sense.

Signed-off-by: Jeff King <peff@peff.net>
---
 send-pack.c |   50 ++++++++++++++++++++++++++++++++++----------------
 1 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index f74e66a..25d5c25 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -177,6 +177,35 @@ static int receive_status(int in)
 	return ret;
 }
 
+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)
+		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;
+
+	if (!remote_find_tracking(remote, &rs)) {
+		fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
+		if (is_null_sha1(ref->peer_ref->new_sha1)) {
+			if (delete_ref(rs.dst, NULL))
+				error("Failed to delete");
+		} else
+			update_ref("update by push", rs.dst,
+					ref->new_sha1, NULL, 0, 0);
+		free(rs.dst);
+	}
+}
+
 static int send_pack(int in, int out, struct remote *remote, int nr_refspec, char **refspec)
 {
 	struct ref *ref;
@@ -302,22 +331,6 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 			fprintf(stderr, "\n  from %s\n  to   %s\n",
 				old_hex, new_hex);
 		}
-		if (remote) {
-			struct refspec rs;
-			rs.src = ref->name;
-			rs.dst = NULL;
-			if (!remote_find_tracking(remote, &rs)) {
-				fprintf(stderr, " Also local %s\n", rs.dst);
-				if (will_delete_ref) {
-					if (delete_ref(rs.dst, NULL)) {
-						error("Failed to delete");
-					}
-				} else
-					update_ref("update by push", rs.dst,
-						ref->new_sha1, NULL, 0, 0);
-				free(rs.dst);
-			}
-		}
 	}
 
 	packet_flush(out);
@@ -330,6 +343,11 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 			ret = -4;
 	}
 
+	if (remote && ret == 0) {
+		for (ref = remote_refs; ref; ref = ref->next)
+			update_tracking_ref(remote, ref);
+	}
+
 	if (!new_refs && ret == 0)
 		fprintf(stderr, "Everything up-to-date\n");
 	return ret;
-- 
1.5.3.4.1162.gc3e8e-dirty

Re: [PATCH] t5516: test update of local refs on push

[Jeff King]

On Thu, Oct 18, 2007 at 02:17:46AM -0400, Jeff King wrote:

> The first test (updating local refs) should succeed, but the
> second one (not updating on error) currently fails.

Oops, this should of course be labeled as 1/2.

For the fix, I didn't need anything from 'next', after all, and 2/2 also
works fine there (it was almost literally a code move).

-Peff

Re: [PATCH] t5516: test update of local refs on push

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Thu, Oct 18, 2007 at 02:17:46AM -0400, Jeff King wrote:
> > The first test (updating local refs) should succeed, but the
> > second one (not updating on error) currently fails.
> 
> Oops, this should of course be labeled as 1/2.
> 
> For the fix, I didn't need anything from 'next', after all, and 2/2 also
> works fine there (it was almost literally a code move).

Yay. I like it when I'm proven wrong.  Especially by a short patch.
:)

This will be in next tonight.  Give it a few days, then probably
graduate up to master.

-- 
Shawn.