[PATCH] push: fix local refs update if already up-to-date

[PATCH] push: fix local refs update if already up-to-date

[Clemens Buchacher]

git push normally updates local refs only after a successful push. If the
remote already has the updates -- pushed indirectly through another repository,
for example -- we forget to update local tracking refs.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

The hashcpy for new_ref is now executed more often than absolutely
necessary. But this is not a critical path, right? So I decided to keep
things simple.

 builtin-send-pack.c   |   11 +++++------
 t/t5516-fetch-push.sh |   18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..66ad492 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -224,7 +224,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
 
-	if (ref->status != REF_STATUS_OK)
+	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
 		return;
 
 	rs.src = ref->name;
@@ -438,15 +438,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		}
 		else
 			new_sha1 = ref->peer_ref->new_sha1;
+		hashcpy(ref->new_sha1, new_sha1);
 
-
-		ref->deletion = is_null_sha1(new_sha1);
+		ref->deletion = is_null_sha1(ref->new_sha1);
 		if (ref->deletion && !allow_deleting_refs) {
 			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}
 		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, new_sha1)) {
+		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
 			ref->status = REF_STATUS_UPTODATE;
 			continue;
 		}
@@ -474,14 +474,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		    !ref->deletion &&
 		    !is_null_sha1(ref->old_sha1) &&
 		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(new_sha1, ref->old_sha1));
+		      || !ref_newer(ref->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 (!ref->deletion)
 			new_refs++;
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f0030ad..d211f93 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -437,6 +437,24 @@ test_expect_success 'push updates local refs' '
 
 '
 
+test_expect_success 'push updates local refs (2)' '
+
+	rm -rf parent child &&
+	mkdir parent &&
+	(cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one) &&
+	git clone parent child1 &&
+	git clone parent child2 &&
+	(cd child1 &&
+		echo two >foo && git commit -a -m two &&
+		git push) &&
+	(cd child2 &&
+		git pull ../child1 master &&
+		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 &&
-- 
1.6.0.3

Re: [PATCH] push: fix local refs update if already up-to-date

[Jeff King]

On Tue, Nov 04, 2008 at 01:07:45AM +0100, Clemens Buchacher wrote:

> git push normally updates local refs only after a successful push. If
> the remote already has the updates -- pushed indirectly through
> another repository, for example -- we forget to update local tracking
> refs.

I think this goal is a good enhancement.

> The hashcpy for new_ref is now executed more often than absolutely
> necessary. But this is not a critical path, right? So I decided to keep
> things simple.

No, I don't think the loop is tight enough to care about an extra
hashcpy. The minimally invasive change would be to just set
ref->new_sha1 in the UPTODATE code path. IOW, just:

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 298bd71..b8788f2 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -454,6 +454,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		if (!ref->deletion &&
 		    !hashcmp(ref->old_sha1, new_sha1)) {
 			ref->status = REF_STATUS_UPTODATE;
+			hashcpy(ref->new_sha1, new_sha1);
 			continue;
 		}
 

Your patch makes ref->new_sha1 "valid" for every status case. Ordinarily
I would be in favor of that, since it reduces coupling with other parts
of the code (which have to know _which_ status flags provide a useful
value in ->new_sha1). But in this case, I think the value we would be
sticking in is not necessarily useful for every status flag we end up
setting; so any consumers of the ref structure still need to know which
flags set it. So even though it has a defined value, it is not really
"valid" in all cases.

> @@ -224,7 +224,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
>  {
>  	struct refspec rs;
>  
> -	if (ref->status != REF_STATUS_OK)
> +	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
>  		return;
>  
>  	rs.src = ref->name;

Hmm. I was hoping to see more in update_tracking_ref. With your patch,
we end up calling update_ref for _every_ uptodate ref, which results in
writing a new unpacked ref file for each one. And that _is_ a
performance problem for people with large numbers of refs.

So I think we need a check to make sure we aren't just updating with the
same value. Something like:

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 4c17f48..0e66e8f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -237,8 +237,17 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	rs.dst = NULL;
 
 	if (!remote_find_tracking(remote, &rs)) {
+		unsigned char old_tracking_sha1[20];
+
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
+
+		if (!resolve_ref(rs.dst, old_tracking_sha1, 0, NULL) ||
+		    !hashcmp(old_tracking_sha1, ref->new_sha1)) {
+			free(rs.dst);
+			return;
+		}
+
 		if (ref->deletion) {
 			delete_ref(rs.dst, NULL, 0);
 		} else

Though I am not happy that we have to look up the tracking ref for every
uptodate ref. I think it shouldn't be a big performance problem with
packed refs, though, since they are cached (i.e., we pay only to compare
the hashes, not touch the filesystem for each ref).

> +test_expect_success 'push updates local refs (2)' '

Nit: Just reading the test, it is hard to see what is interesting about
it (though obviously I can blame it back to your commit :) ). Maybe a
more descriptive title like 'push updates uptodate local refs' would
make sense.

-Peff

Re: [PATCH] push: fix local refs update if already up-to-date

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Though I am not happy that we have to look up the tracking ref for every
> uptodate ref. I think it shouldn't be a big performance problem with
> packed refs, though, since they are cached (i.e., we pay only to compare
> the hashes, not touch the filesystem for each ref).

It is either (1) the user pays the cost of finding what remote tracking
branch we are mirroring when you push for all up-to-date refs, like you
did in your "here is an improvement" patch; or (2) the user pays the cost
of fetching from there, immediately after pushing.  I'd imagine that the
cost to do (1) would be smaller than (2).  The question is if seeing stale
tracking branches is such a big deal, as next "git fetch" from there will
update them anyway.  If it is a big deal, (1) would be a price worth
paying.

In short, I agree with everything you said in your analysis.  Thanks for
being a very good reviewer.

Clemens, care to reroll the patch?

Re: [PATCH] push: fix local refs update if already up-to-date

[Clemens Buchacher]

On Tue, Nov 04, 2008 at 12:38:37AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Though I am not happy that we have to look up the tracking ref for every
> > uptodate ref. I think it shouldn't be a big performance problem with
> > packed refs, though, since they are cached (i.e., we pay only to compare
> > the hashes, not touch the filesystem for each ref).
> 
> It is either (1) the user pays the cost of finding what remote tracking
> branch we are mirroring when you push for all up-to-date refs, like you
> did in your "here is an improvement" patch; or (2) the user pays the cost
> of fetching from there, immediately after pushing.  I'd imagine that the
> cost to do (1) would be smaller than (2).  The question is if seeing stale
> tracking branches is such a big deal, as next "git fetch" from there will
> update them anyway.  If it is a big deal, (1) would be a price worth
> paying.

Right. I think it is a big deal. I found out about this bug, because a user
on #git was confused by the fact that push reported "Everything up-to-date",
even though there were changes. A fetch fixed that, of course. But it is
confusing and inconsistent with normal push behavior. So I really think it's
worth a small performance hit.

> Clemens, care to reroll the patch?

I will do so later today.

Re: [PATCH] push: fix local refs update if already up-to-date

[Clemens Buchacher]

On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:
> > The hashcpy for new_ref is now executed more often than absolutely
> > necessary. But this is not a critical path, right? So I decided to keep
> > things simple.
> 
[...]
> Your patch makes ref->new_sha1 "valid" for every status case. Ordinarily
> I would be in favor of that, since it reduces coupling with other parts
> of the code (which have to know _which_ status flags provide a useful
> value in ->new_sha1). But in this case, I think the value we would be
> sticking in is not necessarily useful for every status flag we end up
> setting; so any consumers of the ref structure still need to know which
> flags set it. So even though it has a defined value, it is not really
> "valid" in all cases.

The other status flags are REF_STATUS_REJECT_NODELETE and
REF_STATUS_REJECT_NONFASTFORWARD. So in these cases the "new sha1" is going
to be the "old sha1". The default for new_sha1 is the null sha1. So while
the sha1 we're trying to push may not be more valid than the null sha1, it's
not less valid either, is it? And it even makes sense if you interpret
new_sha1 as the sha1 the client attempts to push.

> Hmm. I was hoping to see more in update_tracking_ref. With your patch,
> we end up calling update_ref for _every_ uptodate ref, which results in
> writing a new unpacked ref file for each one. And that _is_ a
> performance problem for people with large numbers of refs.
> 
> So I think we need a check to make sure we aren't just updating with the
> same value. Something like:

I think update_ref already takes care of that. See this check in
write_ref_sha1:

        if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
                unlock_ref(lock);
                return 0;
        }

> Though I am not happy that we have to look up the tracking ref for every
> uptodate ref. I think it shouldn't be a big performance problem with
> packed refs, though, since they are cached (i.e., we pay only to compare
> the hashes, not touch the filesystem for each ref).

I don't think we can avoid that, though.

I agree with your other comments.

Re: [PATCH] push: fix local refs update if already up-to-date

[Jeff King]

On Tue, Nov 04, 2008 at 09:56:30AM +0100, Clemens Buchacher wrote:

> The other status flags are REF_STATUS_REJECT_NODELETE and
> REF_STATUS_REJECT_NONFASTFORWARD. So in these cases the "new sha1" is going
> to be the "old sha1". The default for new_sha1 is the null sha1. So while
> the sha1 we're trying to push may not be more valid than the null sha1, it's
> not less valid either, is it? And it even makes sense if you interpret
> new_sha1 as the sha1 the client attempts to push.

I have to admit I did not exhaustively look at all of the status cases
when I reviewed earlier, and there are fewer than I realized. So I think
your change is reasonable.

However, I would like to make one additional request.  Since you are
killing off all usage of new_sha1 initial assignment, I think it makes
sense to just get rid of the variable entirely, so it cannot create
confusion later. Like this (on top of your patch):

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 4c17f48..d139eba 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -435,16 +435,13 @@ 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) {
-		const unsigned char *new_sha1;
-
 		if (!ref->peer_ref) {
 			if (!args.send_mirror)
 				continue;
-			new_sha1 = null_sha1;
+			hashcpy(ref->new_sha1, null_sha1);
 		}
 		else
-			new_sha1 = ref->peer_ref->new_sha1;
-		hashcpy(ref->new_sha1, new_sha1);
+			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
 
 		ref->deletion = is_null_sha1(ref->new_sha1);
 		if (ref->deletion && !allow_deleting_refs) {

> > Hmm. I was hoping to see more in update_tracking_ref. With your patch,
> > we end up calling update_ref for _every_ uptodate ref, which results in
> > writing a new unpacked ref file for each one. And that _is_ a
> > performance problem for people with large numbers of refs.
> [...]
> I think update_ref already takes care of that. See this check in
> write_ref_sha1:
> 
>         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
>                 unlock_ref(lock);
>                 return 0;
>         }

Nope. That check is a concurrency safeguard. It checks that when we are
moving the ref from "A" to "B", that the ref still _is_ "A" when we lock
it.

But more importantly, it is easy to demonstrate the problem with your
patch:

  mkdir parent &&
    (cd parent &&
       git init && touch file && git add file && git commit -m one) &&
  git clone parent child &&
    (cd child &&
       echo BEFORE: && ls -l .git/refs/remotes/origin &&
       git push &&
       echo AFTER:  && ls -l .git/refs/remotes/origin)

I get:

  BEFORE:
  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
  Everything up-to-date
  AFTER:
  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
  -rw-r--r-- 1 peff peff 41 2008-11-04 21:43 master

Oops. With the patch snippet I posted in my previous message, the
'master' ref is not created by the uptodate push.

> > Though I am not happy that we have to look up the tracking ref for every
> > uptodate ref. I think it shouldn't be a big performance problem with
> > packed refs, though, since they are cached (i.e., we pay only to compare
> > the hashes, not touch the filesystem for each ref).
> 
> I don't think we can avoid that, though.

No, you can't avoid it (without totally giving up on your patch's goal,
which I think is a good one). So I think it is worth it, and I was just
being paranoid about hurting performance. Even with packed refs, I think
we do still end up stat()ing for each ref, but we will have to live with
it. I was thinking we might be able to do something clever with values
we had already read for the push, but it is impossible: we have read the
refs we are going to _push_, but we have not looked at the remote
tracking branches, which are what contain the interesting information.

-Peff

Re: [PATCH] push: fix local refs update if already up-to-date

[Clemens Buchacher]

On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:
[...]
> However, I would like to make one additional request.  Since you are
> killing off all usage of new_sha1 initial assignment, I think it makes
> sense to just get rid of the variable entirely, so it cannot create
> confusion later.

Ok, I can live with that.

> > > Hmm. I was hoping to see more in update_tracking_ref. With your patch,
> > > we end up calling update_ref for _every_ uptodate ref, which results in
> > > writing a new unpacked ref file for each one. And that _is_ a
> > > performance problem for people with large numbers of refs.
> > [...]
> > I think update_ref already takes care of that. See this check in
> > write_ref_sha1:
> > 
> >         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
> >                 unlock_ref(lock);
> >                 return 0;
> >         }
> 
> Nope. That check is a concurrency safeguard. It checks that when we are
> moving the ref from "A" to "B", that the ref still _is_ "A" when we lock
> it.

I think you are confusing this with verify_lock(). The code in
write_ref_sha1() really does compare with the new sha1.

> But more importantly, it is easy to demonstrate the problem with your
> patch:
> 
>   mkdir parent &&
>     (cd parent &&
>        git init && touch file && git add file && git commit -m one) &&
>   git clone parent child &&
>     (cd child &&
>        echo BEFORE: && ls -l .git/refs/remotes/origin &&
>        git push &&
>        echo AFTER:  && ls -l .git/refs/remotes/origin)
> 
> I get:
> 
>   BEFORE:
>   -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
>   Everything up-to-date
>   AFTER:
>   -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
>   -rw-r--r-- 1 peff peff 41 2008-11-04 21:43 master
> 
> Oops. With the patch snippet I posted in my previous message, the
> 'master' ref is not created by the uptodate push.

The reason it doesn't work is a bug in lock_ref_sha1_basic(). Dating back to
pre-"pack-refs" times, this code forces a write if the ref file does not
exist. I will resubmit the patch including your above testcase and a bugfix
for lock_ref_sha1_basic().

Clemens

[PATCH 1/2] do not force write of packed refs

[Clemens Buchacher]

We force writing a ref if it does not exist. Originally, we only had to look
for the ref file to check if it existed. Now we have to look for a packed ref
as well. Luckily, resolve_ref already does all the work for us.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 refs.c               |    7 ++++---
 t/t3210-pack-refs.sh |    7 +++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 0a126fa..fc8105d 100644
--- a/refs.c
+++ b/refs.c
@@ -794,10 +794,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	char *ref_file;
 	const char *orig_ref = ref;
 	struct ref_lock *lock;
-	struct stat st;
 	int last_errno = 0;
 	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
+	int inexistent = 0;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
@@ -825,12 +825,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 			orig_ref, strerror(errno));
 		goto error_return;
 	}
+	inexistent = is_null_sha1(lock->old_sha1);
 	/* When the ref did not exist and we are creating it,
 	 * make sure there is no existing ref that is packed
 	 * whose name begins with our refname, nor a ref whose
 	 * name is a proper prefix of our refname.
 	 */
-	if (is_null_sha1(lock->old_sha1) &&
+	if (inexistent &&
             !is_refname_available(ref, NULL, get_packed_refs(), 0))
 		goto error_return;
 
@@ -844,7 +845,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	lock->ref_name = xstrdup(ref);
 	lock->orig_ref_name = xstrdup(orig_ref);
 	ref_file = git_path("%s", ref);
-	if (lstat(ref_file, &st) && errno == ENOENT)
+	if (inexistent)
 		lock->force_write = 1;
 	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
 		lock->force_write = 1;
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 087ef75..413019a 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -96,6 +96,13 @@ test_expect_success \
      git branch -d n/o/p &&
      git branch n'
 
+test_expect_success \
+	'see if up-to-date packed refs are preserved' \
+	'git branch q &&
+	 git pack-refs --all --prune &&
+	 git update-ref refs/heads/q refs/heads/q &&
+	 ! test -f .git/refs/heads/q'
+
 test_expect_success 'pack, prune and repack' '
 	git tag foo &&
 	git pack-refs --all --prune &&
-- 
1.6.0.3.617.ge4eb0

[PATCH 2/2] push: fix local refs update if already up-to-date

[Clemens Buchacher]

git push normally updates local refs only after a successful push. If the
remote already has the updates -- pushed indirectly through another repository,
for example -- we forget to update local tracking refs.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin-send-pack.c   |   11 +++++------
 t/t5516-fetch-push.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d68ce2d..c91c12f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -230,7 +230,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
 
-	if (ref->status != REF_STATUS_OK)
+	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
 		return;
 
 	rs.src = ref->name;
@@ -444,15 +444,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		}
 		else
 			new_sha1 = ref->peer_ref->new_sha1;
+		hashcpy(ref->new_sha1, new_sha1);
 
-
-		ref->deletion = is_null_sha1(new_sha1);
+		ref->deletion = is_null_sha1(ref->new_sha1);
 		if (ref->deletion && !allow_deleting_refs) {
 			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}
 		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, new_sha1)) {
+		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
 			ref->status = REF_STATUS_UPTODATE;
 			continue;
 		}
@@ -480,14 +480,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		    !ref->deletion &&
 		    !is_null_sha1(ref->old_sha1) &&
 		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(new_sha1, ref->old_sha1));
+		      || !ref_newer(ref->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 (!ref->deletion)
 			new_refs++;
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f0030ad..598664c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -437,6 +437,37 @@ test_expect_success 'push updates local refs' '
 
 '
 
+test_expect_success 'push updates up-to-date local refs' '
+
+	rm -rf parent child &&
+	mkdir parent &&
+	(cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one) &&
+	git clone parent child1 &&
+	git clone parent child2 &&
+	(cd child1 &&
+		echo two >foo && git commit -a -m two &&
+		git push) &&
+	(cd child2 &&
+		git pull ../child1 master &&
+		git push &&
+	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
+
+'
+
+test_expect_success 'push preserves up-to-date packed refs' '
+
+	rm -rf parent child &&
+	mkdir parent &&
+	(cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one) &&
+	git clone parent child &&
+	(cd child &&
+		git push &&
+	! test -f .git/refs/remotes/origin/master)
+
+'
+
 test_expect_success 'push does not update local refs on failure' '
 
 	rm -rf parent child &&
-- 
1.6.0.3.617.ge4eb0

Re: [PATCH] push: fix local refs update if already up-to-date

[Clemens Buchacher]

On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:
> On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:
> [...]
> > However, I would like to make one additional request.  Since you are
> > killing off all usage of new_sha1 initial assignment, I think it makes
> > sense to just get rid of the variable entirely, so it cannot create
> > confusion later.

Considering that the ref is initialized to the null_sha1, do you think it
would be Ok to do the following instead? The call to hashcpy would not be
needed twice and we get rid of the temporary new_sha1.

--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -435,24 +435,18 @@ static int do_send_pack(int in, int out, struct remote *re
         */
        new_refs = 0;
        for (ref = remote_refs; ref; ref = ref->next) {
-               const unsigned char *new_sha1;
-
-               if (!ref->peer_ref) {
-                       if (!args.send_mirror)
-				continue;
-                       new_sha1 = null_sha1;
-               }
-               else
-                       new_sha1 = ref->peer_ref->new_sha1;
-
+               if (ref->peer_ref)
+                       hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+               else if (!args.send_mirror)
+                       continue;

Re: [PATCH] push: fix local refs update if already up-to-date

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:
>> On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:
>> [...]
>> > However, I would like to make one additional request.  Since you are
>> > killing off all usage of new_sha1 initial assignment, I think it makes
>> > sense to just get rid of the variable entirely, so it cannot create
>> > confusion later.
>
> Considering that the ref is initialized to the null_sha1, do you think it
> would be Ok to do the following instead?

Yeah, that's pretty much what I munged and committed.

Re: [PATCH] push: fix local refs update if already up-to-date

[Jeff King]

On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:

> The reason it doesn't work is a bug in lock_ref_sha1_basic(). Dating back to
> pre-"pack-refs" times, this code forces a write if the ref file does not
> exist. I will resubmit the patch including your above testcase and a bugfix
> for lock_ref_sha1_basic().

OK, thanks for looking into it further. Both patches look sane to me.

-Peff

[PATCH v2] push: fix local refs update if already up-to-date

[Clemens Buchacher]

git push normally updates local refs only after a successful push. If the
remote already has the updates -- pushed indirectly through another repository,
for example -- we forget to update local tracking refs.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:
> Nit: Just reading the test, it is hard to see what is interesting about
> it (though obviously I can blame it back to your commit :) ). Maybe a
> more descriptive title like 'push updates uptodate local refs' would
> make sense.

That is all I changed in this update. Pending an Ack/Nack from Jeff I feel
that I'm done.

 builtin-send-pack.c   |   11 +++++------
 t/t5516-fetch-push.sh |   18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d68ce2d..c91c12f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -230,7 +230,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
 
-	if (ref->status != REF_STATUS_OK)
+	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
 		return;
 
 	rs.src = ref->name;
@@ -444,15 +444,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		}
 		else
 			new_sha1 = ref->peer_ref->new_sha1;
+		hashcpy(ref->new_sha1, new_sha1);
 
-
-		ref->deletion = is_null_sha1(new_sha1);
+		ref->deletion = is_null_sha1(ref->new_sha1);
 		if (ref->deletion && !allow_deleting_refs) {
 			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}
 		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, new_sha1)) {
+		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
 			ref->status = REF_STATUS_UPTODATE;
 			continue;
 		}
@@ -480,14 +480,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		    !ref->deletion &&
 		    !is_null_sha1(ref->old_sha1) &&
 		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(new_sha1, ref->old_sha1));
+		      || !ref_newer(ref->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 (!ref->deletion)
 			new_refs++;
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f0030ad..a82ce5a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -437,6 +437,24 @@ test_expect_success 'push updates local refs' '
 
 '
 
+test_expect_success 'push updates up-to-date local refs' '
+
+	rm -rf parent child &&
+	mkdir parent &&
+	(cd parent && git init &&
+		echo one >foo && git add foo && git commit -m one) &&
+	git clone parent child1 &&
+	git clone parent child2 &&
+	(cd child1 &&
+		echo two >foo && git commit -a -m two &&
+		git push) &&
+	(cd child2 &&
+		git pull ../child1 master &&
+		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 &&
-- 
1.6.0.3.617.ge4eb0

Re: [PATCH v2] push: fix local refs update if already up-to-date

[Jeff King]

On Tue, Nov 04, 2008 at 09:57:43PM +0100, Clemens Buchacher wrote:

> On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:
> > Nit: Just reading the test, it is hard to see what is interesting about
> > it (though obviously I can blame it back to your commit :) ). Maybe a
> > more descriptive title like 'push updates uptodate local refs' would
> > make sense.
> 
> That is all I changed in this update. Pending an Ack/Nack from Jeff I feel
> that I'm done.

I have to NAK, because the extra written ref is still a problem (see my
other mail). But with that fix (and I hope you both will agree with the
style fixup on removing new_sha1, too), I think it should be good.

-Peff