git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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*
Previous: Junio C HamanoNext: Julian Phillips
Message 7 of 22 in “git fetch --depth=* broken?”
  1. Nicolas PitreAug 22, 2009
  2. fix simple deepening of a repoNicolas Pitre, Aug 24, 2009
  3. Junio C HamanoAug 24, 2009
  4. Nicolas PitreAug 24, 2009
  5. Johan HerlandAug 24, 2009
  6. Junio C HamanoAug 24, 2009
  7. Daniel BarkalowAug 24, 2009
  8. Julian PhillipsAug 24, 2009
  9. Nicolas PitreAug 25, 2009
  10. Shawn O. PearceAug 25, 2009
  11. Sverre RabbelierAug 25, 2009
  12. Junio C HamanoAug 25, 2009
  13. Shawn O. PearceAug 25, 2009
  14. Junio C HamanoAug 25, 2009
  15. Shawn O. PearceAug 25, 2009
  16. Shawn O. PearceAug 26, 2009
  17. Johannes SixtAug 26, 2009
  18. Shawn O. PearceAug 26, 2009
  19. Junio C HamanoAug 26, 2009
  20. Shawn O. PearceAug 26, 2009
  21. upload-pack: expand capability advertises additional refsShawn O. Pearce, Aug 28, 2009
  22. Junio C HamanoAug 28, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.