Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 25, 2018, 18:54 UTC
Message-ID: <20181025185459.206127-1-jonathantanmy@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1810241756390.4546@tvgsbejvaqbjf.bet>

Show 16 quoted lines

> On Wed, 24 Oct 2018, Johannes Schindelin wrote:
> 
> > On Wed, 24 Oct 2018, Junio C Hamano wrote:
> > 
> > > Jonathan, do you see any issues with the use of lookup_commit() in
> > > this change wrt lazy clone?  I am wondering what happens when the
> > > commit in question is at, an immediate parent of, or an immediate
> > > child of a promisor object.  I _think_ this change won't make it
> > > worse for two features in playing together, but thought that it
> > > would be better to double check.
> > 
> > Good point.
> > 
> > Instinctively, I would say that no promised object can be a shallow
> > commit. The entire idea of a shallow commit is that it *is* present, but
> > none of its parents.

I'm envisioning a client repo with a single branch, cloned both with --depth=1 and with --filter=<foo>, that has just fetched to the same branch also with --depth=1 resulting in a fast-forward from A to B.

If A is B's parent, then A would be known to be promised. (Incidentally, the problem is greater in current Git, because for performance reasons, we do not check promisor status when lazily fetching - so it doesn't matter here whether an object is known to be promised or not.)

When pruning shallow and checking the existence of A, this would trigger a fetch for A, which would download all commits and trees reachable from it.

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.

Show 9 quoted lines

> > However, I am curious whether there is a better way to check for the
> > presence of a local commit? Do we have an API function for that, that I
> > missed? (I do not really want to parse the commit, after all, just verify
> > that it is not pruned.)
> 
> Okay, I looked around a bit. It seems that there is an
> `is_promisor_object(oid)` function in `pu` to see whether an object was
> promised. If need be (and I am still not convinced that there is a need),
> then we can always add a call to that function to the condition.

I don't think there is a need for is_promisor_object() either - as described above, it doesn't completely solve the problem.

Show 5 quoted lines

> Coming back to my question whether there is a better way to check for the
> presence of a local commit, I figured that I can use `has_object_file()`
> instead of looking up and parsing the commit, as this code does not really
> need to verify that the shallow entry refers to a commit, but only that it
> refers to a local object.

Note that has_object_file() also triggers the lazy fetch if needed, but I agree that it's better because you don't really need to parse the commit.

There is the possibility of just checking for loose objects (which does not trigger any lazy fetches), which works for builtin/prune since it only prunes loose objects, but doesn't work in the general case, I guess.