Hi Jonathan, On Fri, 26 Oct 2018, Jonathan Tan wrote: > > So even better would be to use `is_promisor_object(oid) || > > has_object_file(oid)`, right? > > > > This is something that is probably not even needed: as I mentioned, > > the shallow commits are *expected* to be local. It should not ever > > happen that they are fetched. > > That would help, but I don't think it would help in the "fast-forward > from A to B where A is B's parent" case I describe in [1]. I don't think that that analysis is correct. It assumes that there could be a promised commit that is also listed as shallow. I do not think that is a possible scenario. And even if it were, why would asking for a promised commit object download not only that object but also all of its trees and all of its ancestors? That's not how I understood the idea of the lazy clone: I understood promised objects to be downloaded on demand, individually. > My suggestion was: > > > It sounds safer to me to use the fast approach in this patch when the > > repository is not partial, and stick to the slow approach when it is. > > which can be done by replacing "prune_shallow(0, 1)" in patch 3 with > "prune_shallow(0, !repository_format_partial_clone)", possibly with a comment > that the fast method checks object existence for each shallow line directly, > which is undesirable when the repository is a partial clone. I am afraid that that would re-introduce the bug pointed out by Peff: you *really* would need to traverse all reachable commits to use the non-fast pruning method. And we simply don't. Ciao, Dscho > (repository_format_partial_clone is non-NULL with the name of the promisor > remote if the repository is a partial clone, and NULL otherwise). > > [1] https://public-inbox.org/git/20181025185459.206127-1-jonathantanmy@xxxxxxxxxx/ >
