Re: [PATCH] apply: do not fetch when checking object existence

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 28, 2020, 18:23 UTC
Message-ID: <20200728182341.342162-1-jonathantanmy@google.com>
In-Reply-To: <xmqqwo2oe8r9.fsf@gitster.c.googlers.com>

Show 25 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > There have been a few bugs wherein Git fetches missing objects whenever
> > the existence of an object is checked, even though it does not need to
> > perform such a fetch. To resolve these bugs, we could look at all the
> > places that has_object_file() (or a similar function) is used. As a
> > first step, introduce a new function has_object() that checks for the
> > existence of an object, with a default behavior of not fetching if the
> > object is missing and the repository is a partial clone. As we verify
> > each has_object_file() (or similar) usage, we can replace it with
> > has_object(), and we will know that we are done when we can delete
> > has_object_file() (and the other similar functions).
> 
> I wonder if we want to name the two (i.e. one variant that refuses
> to go to network because it is trying to see if a lazy fetch is
> needed, and the other that goes to network behind caller's back for
> ease of use in a lazy clone) a bit more distinctly so that which one
> could potentially go outside.
> 
> Depending on one's view which one is _normal_ access pattern, giving
> an explicit adverb to one variant while leaving the other one bland
> might be sufficient.  For example, I _think_ most of the places do
> not want to handle the details of lazily fetching themselves, and I
> suspect that the traditional has_object_file() semantics without "do
> not trigger lazy fetch" option would be the normal access pattern.

Right now, I think that most (if not all) places would not want to fetch at all - so *with* "do not trigger lazy fetch" would be the normal access pattern. This is because (in my opinion) if a caller checks the existence of an object, it most likely can tolerate the object's absence; if the caller couldn't tolerate it, it would just directly query for its type or contents or something like that.

I tried to communicate this in my documentation of the deprecated functions/macros, but perhaps it could be written better.

(One other option to consider is to just change has_object_file() to never fetch, although I think this is more risky.)

Show 9 quoted lines

> In which case, renaming your new "has_object" to something like
> "has_object_locally()" would be a good name for a special case
> codepath that wants to care---if the object does not exist locally
> and needs to be obtained lazily from elsewhere, the function would
> say "no".
> 
> And all the other names like has_object_file() that by default gives
> callers a transparent access to lazily fetched objects can stay the
> same.

If my analysis above is wrong, then yes I agree that we should do this. But we might need to find another way to indicate which has_object_file() has been checked and which hasn't - changing away from has_object_file() completely gives us a way to indicate this, but if we're sticking with has_object_file(), we have to find another way of indicating that we've looked at this call and it is OK.

Show 12 quoted lines

> > I mentioned the idea for this change here:
> > https://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@google.com/
> 
> Yup, I think that is going in a good direction.  I suspect that
> apply will not be the only remaining case we need to "fix", and
> using the new helper function, codepaths that have already been
> "fixed" by passing "do not lazily fetch" option to the traditional
> API functions would become easier to read.  And if that is the case,
> let's have the introduction of the helper function as a separate
> patch, with each of [PATCH 2-N/N] be a fix for separate codepaths.
> 
> Thanks.

OK - I'll separate out the helper function into its own patch in version 2.