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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 11, 2023, 02:59 UTC
Message-ID: <20230311025906.4170554-1-jonathantanmy@google.com>
In-Reply-To: <xmqqttys4746.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

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