Hi, On Tue, 14 Nov 2006, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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 - 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