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

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.

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