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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 26, 2018, 07:59 UTC
Message-ID: <nycvar.QRO.7.76.6.1810260956230.4546@tvgsbejvaqbjf.bet>
In-Reply-To: <20181025185459.206127-1-jonathantanmy@google.com>

Hi Jonathan,

On Thu, 25 Oct 2018, Jonathan Tan wrote:

Show 11 quoted lines

> > On Wed, 24 Oct 2018, Johannes Schindelin wrote:
> > 
> > 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.

Thanks for confirming.

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.

> 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.

In the test case I added, the commit object is actually packed. And then, because it became unreachable, it is dropped.

If I only checked for loose objects here, the shallow line would already be removed when the commit gets packed, which would be the wrong thing to do.

In short: thank you for confirming that the current version of the patch seems to be the best we can do for now.

Ciao, Dscho