Re: [PATCH] fix simple deepening of a repo
- From
Daniel Barkalow <barkalow@iabervon.org>
- Date
- Aug 24, 2009, 16:26 UTC
- Message-ID
- <alpine.LNX.2.00.0908240144530.28290@iabervon.org>
- In-Reply-To
- <7vocq5q0j7.fsf@alter.siamese.dyndns.org>
On Sun, 23 Aug 2009, Junio C Hamano wrote:
Show 35 quoted lines
> Nicolas Pitre <nico@cam.org> 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@cam.org>
> > ---
>
> 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*