On Sun, 23 Aug 2009, Junio C Hamano wrote:
> Nicolas Pitre <nico@xxxxxxx> writes:
>
> > If all refs sent by the remote repo during a fetch are reachable
> > locally, then no further conversation is performed with the remote. This
> > check is skipped when the --depth argument is provided to allow the
> > deepening of a shallow clone which corresponding remote repo has no
> > changed.
> >
> > However, some additional filtering was added in commit c29727d5 to
> > remove those refs which are equal on both sides. If the remote repo has
> > not changed, then the list of refs to give the remote process becomes
> > empty and simply attempting to deepen a shallow repo always fails.
> >
> > Let's stop being smart in that case and simply send the whole list over
> > when that condition is met. The remote will do the right thing anyways.
> >
> > Test cases for this issue are also provided.
> >
> > Signed-off-by: Nicolas Pitre <nico@xxxxxxx>
> > ---
>
> Thanks. The fix looks correct (as usual with patches from you).
>
> But it makes me wonder if this logic to filter refs is buying us anything.
>
> > for (rm = refs; rm; rm = rm->next) {
> > + nr_refs++;
> > if (rm->peer_ref &&
> > !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))
> > continue;
> ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
> heads[nr_heads++] = rm;
> }
>
> What is the point of not asking for the refs that we know are the same?
This code is part of the original C implementation of fetch; I suspect the
optimization was somehow in the shell version and made sense there,
perhaps because there wasn't a quickfetch in the shell version or that
there was some non-negligable per-ref cost in the code around there, since
it was calling helper programs and such.
Anyway, I think it makes sense to remove the filtering from
transport_fetch_refs(), like your patch does.
If it makes a difference for the actual protocol, fetch_refs_via_pack()
could filter them at that stage.
-Daniel
*This .sig left intentionally blank*
--
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