Re: [PATCH 3/5] allow cloning a repository "shallowly"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 15, 2006, 10:57 UTC
Message-ID: <Pine.LNX.4.63.0611151153490.13772@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vpsbpzzse.fsf@assigned-by-dhcp.cox.net>

Hi,

On Tue, 14 Nov 2006, Junio C Hamano wrote:

Show 27 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I understand "no need making it shallow", but I am not sure if a
> >> non-NULL return from lookup_object() tells us that.
> >
> > You are probably right, how about has_sha1_file()?
> >
> >> I think register_shallow() can take commits that are already shallow() 
> >> so maybe we can remove this "if() continue"?
> >
> > Yes, it can, but that is not necessarily correct: since .git/shallow is 
> > constructed from the registered shallow commits, we would make a commit 
> > shallow which is really not shallow.
> >
> > So, how about
> >
> >> > +				if (lookup_object(sha1) || has_sha1_file(sha1))
> >> > +					continue;
> 
> If I understand the code correctly, this loop is reading what
> the other side thinks your shallows should be (based on your
> earlier "deepen" request or if this is initial fetch based on
> your depth).  Even if we already have that object, if that
> object _is_ shallow on our end, don't we need to keep it marked
> as shallow?  Will we get ancestors of this commit from the other
> end (and "shallow" lines for some of them to properly cauterize
> the chain)?

My thinking was: if we have that object already, and it is _not_ marked as shallow, we probably have either all its ancestors, or at least all ancestors up until shallow commit(s).

Thinking about it again, it seems very fragile: on purpose, we only trust to have the objects which are reachable by at least one ref, so that an interrupted fetch does not corrupt the repository.

So yes, I agree, that "if() continue;" should go away. Even if all of a sudden, commits we already have are no longer reachable.

Ciao, Dscho