# bug: origin refs updated too soon locally 8 messages from 2007-10-18 to 2007-10-18. Participants: Perry Wagle, Shawn O. Pearce, Jeff King. Thread: https://gitlist.dev/t/10350 ## Perry Wagle, 2007-10-18 01:35 Subject: bug: origin refs updated too soon locally Message-ID: <8CEF6150-4BE7-4B4D-B58C-12CE4671007E@cs.indiana.edu> URL: https://gitlist.dev/e/8CEF6150-4BE7-4B4D-B58C-12CE4671007E%40cs.indiana.edu ``` 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 ``` ## Perry Wagle, 2007-10-18 01:43 Subject: Re: bug: origin refs updated too soon locally Message-ID: <8E28A8CC-DC3B-4C97-8B14-742DAA8D3CE2@cs.indiana.edu> URL: https://gitlist.dev/e/8E28A8CC-DC3B-4C97-8B14-742DAA8D3CE2%40cs.indiana.edu In-Reply-To: <8CEF6150-4BE7-4B4D-B58C-12CE4671007E@cs.indiana.edu> ``` 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 ``` ## Shawn O. Pearce, 2007-10-18 04:53 Subject: Re: bug: origin refs updated too soon locally Message-ID: <20071018045358.GB14735@spearce.org> URL: https://gitlist.dev/e/20071018045358.GB14735%40spearce.org In-Reply-To: <8CEF6150-4BE7-4B4D-B58C-12CE4671007E@cs.indiana.edu> ``` 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. 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. ``` ## Jeff King, 2007-10-18 05:28 Subject: Re: bug: origin refs updated too soon locally Message-ID: <20071018052809.GA11938@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071018052809.GA11938%40coredump.intra.peff.net In-Reply-To: <20071018045358.GB14735@spearce.org> ``` 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 ``` ## Jeff King, 2007-10-18 06:17 Subject: [PATCH] t5516: test update of local refs on push Message-ID: <20071018061746.GA29531@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071018061746.GA29531%40coredump.intra.peff.net In-Reply-To: <20071018045358.GB14735@spearce.org> ``` The first test (updating local refs) should succeed, but the second one (not updating on error) currently fails. Signed-off-by: Jeff King --- 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 ``` ## Jeff King, 2007-10-18 06:19 Subject: [PATCH 2/2] send-pack: don't update tracking refs on error Message-ID: <20071018061915.GB29531@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071018061915.GB29531%40coredump.intra.peff.net In-Reply-To: <20071018045358.GB14735@spearce.org> ``` 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 --- 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 ``` ## Jeff King, 2007-10-18 06:21 Subject: Re: [PATCH] t5516: test update of local refs on push Message-ID: <20071018062136.GB11938@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071018062136.GB11938%40coredump.intra.peff.net In-Reply-To: <20071018061746.GA29531@coredump.intra.peff.net> ``` 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 ``` ## Shawn O. Pearce, 2007-10-18 06:59 Subject: Re: [PATCH] t5516: test update of local refs on push Message-ID: <20071018065928.GJ14735@spearce.org> URL: https://gitlist.dev/e/20071018065928.GJ14735%40spearce.org In-Reply-To: <20071018062136.GB11938@coredump.intra.peff.net> ``` Jeff King 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. ```