On Mon, 11 Jan 2010, Junio C Hamano wrote:
> Ahh, you are right. It walks from objects the remote side told us are at
> the tip, and stops at what we know are complete (i.e. reachable from our
> tip of objects); immediately after --dry-run slurped objects, the next
> fetch will prove everything is locally available and complete before going
> over the network.
>
> But either I am very confused or the use of fields from "struct ref" is
> unintuitive in this codepath.
>
> Why does it feed ref->old_sha1? We are feeding _their_ tip commits to:
>
> rev-list --objects --stdin --not --all
>
> and expecting it to report failure when some of their tip commits lead to
> what we don't have yet. The reason why we have old_sha1[] vs new_sha1[]
> is because we want to report what changed from what, and also to protect
> us from simultaneous updates by doing compare-and-swap using the value we
> read from our refs when we started in old_sha1[], so I would have expected
> that ref_map elements would have _their_ commits on the new_sha1[] side,
> but apparently that is not what is happening, and it has been this way for
> a long time. The use of old_sha1[] came from 4191c35 (git-fetch: avoid
> local fetching from alternate (again), 2007-11-11), so it is a lot more
> likely that I am confused than the code is wrong and nobody noticed so
> far.
Very confusing indeed. I first discovered about quickfetch() myself
when I fixed shallow clone leading to commit 86386829.
If old_sha1[] was our refs then quickfetch() would always succeed and
we'd never fetch anything.
> What am I missing?
Digging a bit, it looks like get_remote_heads() is storing the remote's
heads into old_sha1. And so is performed in get_refs_from_bundle(), and
in insert_packed_refs() from get_refs_via_rsync(), etc.
Looking at the struct ref definition, we can see:
struct ref {
struct ref *next;
unsigned char old_sha1[20];
unsigned char new_sha1[20];
[...]
struct ref *peer_ref; /* when renaming */
[...]
};
And apparently store_updated_refs() ends up using that peer_ref like
this:
for (rm = ref_map; rm; rm = rm->next) {
struct ref *ref = NULL;
if (rm->peer_ref) {
ref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);
strcpy(ref->name, rm->peer_ref->name);
hashcpy(ref->old_sha1, rm->peer_ref->old_sha1);
hashcpy(ref->new_sha1, rm->old_sha1);
ref->force = rm->peer_ref->force;
}
So.... Doesn't this all look like a total mess of needless (and even
leaked in this case) allocations and duplications, besides being
completely unintuitive? Both hashcpy() above certainly throw my sense
of logic aside...
Nicolas
--
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