Re: [PATCH v2] index-pack: remove fetch_if_missing=0

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2023, 21:41 UTC
Message-ID: <xmqqttys4746.fsf@gitster.g>
In-Reply-To: <20230310211321.4135748-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 30 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>> > Hence, use has_object() to check for the existence of an object, which
>> > has the default behavior of not lazy-fetching in a partial clone. It is
>> > worth mentioning that this is the only place where there is potential for
>> > lazy-fetching and all other cases are properly handled, making it safe to
>> > remove this global here.
>> 
>> This paragraph is very well explained.
>
> It might be good if the "all other cases" were enumerated here in the
> commit message (since the consequence of missing a case might be an
> infinite loop of fetching).
>
>> OK.  The comment describes the design choice we made to flip the
>> fetch_if_missing flag off.  The old world-view was that we would
>> notice a breakage by non-functioning index-pack when a lazy clone is
>> missing objects that we need by disabling auto-fetching, and we
>> instead explicitly handle any missing and necessary objects by lazy
>> fetching (like "when we lack REF_DELTA bases").  It does sound like
>> a conservative thing to do, compared to the opposite approach we are
>> taking with this patch, i.e. we would not fail if we tried to access
>> objects we do not need to, because we have lazy fetching enabled,
>> and we just ended up with bloated object store nobody may notice.
>> 
>> To protect us from future breakage that can come from the new
>> approach, it is a very good thing that you added new tests to ensure
>> no unnecessary lazy fetching is done (I am not offhand sure if that
>> test is sufficient, though).
>
> I don't think the test is sufficient - I'll explain that below.

I admit I haven't thought about it any longer than anybody who touched this topic, but should "fetch_if_missing=0" really be treated as "it was a dirty hack in the past, now we do not need it, as all callers into the object layer avoids lazy fetching when they do not have to, so let's remove it"? It looks to me more and more that the old world-view to disable lazy fetching by default and have individual calls to the object layer opt into fetching as needed may give us a better resulting code, or is it just me? The possible error modes in new code that fails to follow the world-view with and without this change are:

 * If the lazy fetching is disabled by default (i.e. without this
   patch), a new code can by mistake call has_object(), which does
   not lazy fetch, when it does need to have the object and should
   be using something like has_object_file_with_flags(), and dies
   loudly.

 * If the lazy fetching is enabled by default, on the other hand, a
   new code can by mistake call has_object_file_with_flags(), which
   does lazy fetch, when it does not need to have the object.  It
   does not die, it just lazily fetches objects it does not need.
   The (performance) "bug" will stay hidden until somebody complains.

In short, the world-view of the current code seems to give us tighter control over what gets lazy fetched, simply because we do not allow lazy fetching without thinking.

Do we have other uses of fetch_if_missing (i.e. disable lazy fetching)?

    $ git grep -l fetch_if_missing
    Documentation/technical/partial-clone.txt
    builtin/fetch-pack.c
    builtin/fsck.c
    builtin/pack-objects.c
    builtin/prune.c
    builtin/rev-list.c
    cache.h
    midx.c
    object-file.c
    revision.c

As the default is 1, all these hits (outside the header, doc, and object-file.c) are to disable lazy fetching. Judging from the list of "family" that want tighter control over what gets fetched, I have a feeling that pack-index may want to stay to be in the family.

Or am I missing some big picture goal to eventually getting rid of this mechanism and always allowing lazy fetching?

Thanks.