On Fri, Feb 10, 2012 at 03:51:47PM +0100, Michael Haggerty wrote:
> First problem: everything_local() seems to be either broken or used
> incorrectly. I can't decide which because I don't know what its
> semantics are *supposed* to be.
>
> If everything_local() is trying to check that the references are all in
> the local repository itself, then it is incorrect for clone to enter
> alternates into extra_refs because everything_local() then mistakes them
> for local.
>
> If everything_local() is trying to check that the references are in the
> local repository plus alternates, then it is incorrect that
> everything_local() doesn't consider alternate references in its
> determination. My guess is that this is the case, and that something
> like the following might be the fix:
Junio could answer more authoritatively than I, but I am pretty sure it
is the latter. The point is to skip the expensive find_common
negotiation if we know that there are no objects to fetch. Thus the
"local" here is "do we have them on this side of the git-protocol
connection", not "do we have them in our non-alternates repository".
> > ----------------------------- builtin/fetch-pack.c -----------------------------
> > index 9500f35..4257a8d 100644
> > @@ -581,6 +581,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
> > *refs = newlist;
> > }
> >
> > +static void mark_alternate_complete(const struct ref *ref, void *unused)
> > +{
> > + mark_complete(NULL, ref->old_sha1, 0, NULL);
> > +}
> > +
> > static int everything_local(struct ref **refs, int nr_match, char **match)
> > {
> > struct ref *ref;
> > @@ -609,6 +614,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
> >
> > if (!args.depth) {
> > for_each_ref(mark_complete, NULL);
> > + for_each_alternate_ref(mark_alternate_complete, NULL);
> > if (cutoff)
> > mark_recent_complete_commits(cutoff);
> > }
>
> With this patch, then the full test suite passes even if I take out the
> code that adds the alternate refs to extra_refs.
That looks sane to me.
> Specifically: (without the above patch) I commented out the call to
> add_extra_ref() in clone.c:add_one_reference(), then ran t5700 through
> step 8 then aborted. insert_one_alternate_ref() was called four times:
>
> insert_one_alternate_ref(ccc25a1f9655742174c93f48f616bea8ad0bc6ff)
> insert_one_alternate_ref(ccc25a1f9655742174c93f48f616bea8ad0bc6ff)
> insert_one_alternate_ref(5355551c5a927a2b6349505ada2da4bb702c0a49)
> insert_one_alternate_ref(5355551c5a927a2b6349505ada2da4bb702c0a49)
>
> (The duplication here seems strange.)
I think the duplication can be explained. The alternate-refs mechanism
gets the list of refs by running "git ls-remote" on each alternate.
The symbolic refs appear as refs in that list. So in t5700, for example,
I get:
$ git ls-remote B
56a2e291e54b1a92180fe2072152e6aa0919fc5f HEAD
56a2e291e54b1a92180fe2072152e6aa0919fc5f refs/heads/master
3ee812e8486c2474e2b03be5c0b42e33092da069 refs/remotes/origin/HEAD
3ee812e8486c2474e2b03be5c0b42e33092da069 refs/remotes/origin/master
Because you don't actually care about the alternate refs themselves, but
only about their sha1 values, you could eliminate duplicates early. The
code in receive-pack already does this; fetch-pack should probably do
the same (it's not _wrong_ not to, but in some pathological cases, the
duplicates can make performance worse).
> want 5355551c5a927a2b6349505ada2da4bb702c0a49 multi_ack_detailed
> side-band-64k thin-pack ofs-delta
> want 5355551c5a927a2b6349505ada2da4bb702c0a49
> #E
>
> The "5355551c" object corresponds to refs/remotes/origin/master in the
> alternate object store:
>
> $ (cd B; git for-each-ref)
> ccc25a1f9655742174c93f48f616bea8ad0bc6ff commit refs/heads/master
> 5355551c5a927a2b6349505ada2da4bb702c0a49 commit refs/remotes/origin/HEAD
> 5355551c5a927a2b6349505ada2da4bb702c0a49 commit refs/remotes/origin/master
>
> It seems to me that even in the absence of short-circuiting due to
> everything_local() returning true, the presence of the alternate refs
> should be suppressing the "want" lines for those references.
Yeah, that definitely seems like a bug in find_common.
-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