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

Jonathan Tan [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

quoted

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

quoted

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.

Junio C Hamano [off-list ref] writes:

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.

I think this "family" concept is a good way to think of it. I did
use to think that it would be better to be consistent throughout Git
and choose one world-view, and if I had to choose, it would be the one
without fetch_if_missing=0. But now it does make sense to me to have
two families:

 (a) The more low-level code that the lazy fetching itself relies on
     (and maybe things like builtin/fsck.c as well) where we really need
     to be careful about what we fetch, and it would be better to err
     on the side of not fetching. The test cases for these would need to
     cover both the partial clone cases and the regular cases.

     For these cases, the consequence of lazy-fetching when we shouldn't
     might be as bad as an infinite loop, so it makes sense to default
     not lazy-fetching here.

 (b) The more high-level code, in which I think that it is better to err
     on the side of fetching. The test cases would generally not need to
     cover the partial clone cases (except when there are specific
     optimizations needed, such as in checkout where we bulk prefetch
     missing objects).

     For these cases, the consequences of lazy-fetching when we shouldn't
     are generally performance-related, so it might not be so bad to let
     development happen in these areas of code without great
     consideration to whether a lazy-fetch would happen if an object
     didn't exist. (I do think it would be ideal for all new code to pay
     attention to when they read objects, which would help not only in
     partial clone but also in a potential future in which we have non-
     disk object stores, but we're probably not there yet as a project.)

And indeed, pack-index would go in (a).

On Sat, 11 Mar 2023 at 03:11, Junio C Hamano [off-list ref] wrote:

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?

I think having a single function to check for object existence, which is
compatible with partial clones is better, because the end goal is to
completely integrate the concept of partial clones with git's codebase
and not have code that worries "Oh, there maybe bugs here because
what if the user has a partial clone", everytime the code does an object
existence check (that is, a call to has_object_file() or any of its
related functions) and just have fetch_if_missing set to 1 or 0, according
to the particular command.

It is also true that has_object() is not that "single function", because
in cases where we are missing an object in partial clone and want it,
has_object() has no way of fetching it. With or without flags (it only
supports one flag which, when set, rechecks packed storage), it does not
lazy-fetch in a partial clone. But there are cases where we need such
objects, such as the commands that come into "family (b)" [1].

So, why not use oid_object_info_extended() directly, instead of wrapping
it with some other function, whenever we are checking for an object's
existence. We can skip lazy-fetches whenever we want with
OBJECT_INFO_SKIP_FETCH_OBJECT and can also prefetch with
OBJECT_INFO_FOR_PREFETCH [2].

[1] https://lore.kernel.org/git/20230311025906.4170554-1-jonathantanmy@google.com/

[2] pack-index itself is one example of where this is done.

    When we don't have REF_DELTA bases, we bulk prefetch them

	    if (has_promisor_remote()) {
                     /*
                      * Prefetch the delta bases.
                      */
                     struct oid_array to_fetch = OID_ARRAY_INIT;
		     for (i = 0; i < nr_ref_deltas; i++) {
                             struct ref_delta_entry *d = sorted_by_pos[i];
                             if (!oid_object_info_extended(the_repository, &d->oid,
                                                           NULL,
                                                           OBJECT_INFO_FOR_PREFETCH))
                                     continue;
                             oid_array_append(&to_fetch, &d->oid);
                     }
                     promisor_remote_get_direct(the_repository,
                                                to_fetch.oid, to_fetch.nr);
                     oid_array_clear(&to_fetch);
             }

    Instead of going object-by-object, which is basically like an
    infinite loop in large repos and partial clones are widely used
    in large repos.