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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
