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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2020, 01:19 UTC
Message-ID: <xmqqwo2oe8r9.fsf@gitster.c.googlers.com>
In-Reply-To: <20200728010403.95142-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 10 quoted lines

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

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.

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