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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2023, 22:58 UTC
Message-ID: <xmqqlejvf0j6.fsf@gitster.g>
In-Reply-To: <20230317175601.4250-1-five231003@gmail.com>

Kousik Sanagavarapu <five231003@gmail.com> writes:

Show 8 quoted lines

> A collision test is triggered in sha1_object(), whenever there is an
> object file in our repo. If our repo is a partial clone, then checking
> for this file existence does not lazy-fetch the object (if the object
> is missing and if there are one or more promisor remotes) when
> fetch_if_missing is set to 0.
>
> Though this global lets us control lazy-fetching in regions of code,
> it prevents multi-threading [1].

Sorry, but I really do not see the point.

We already have read_lock/read_unlock to prevent multiple threads from stomping on the in-core object database structure either way.

If somebody needs to dynamically change the value of fetch_if_missing after the program started and spawned multiple threads, yes, the update to the single variable would become a problem point in multi-threading.

But that is not what we are doing, and you already discovered that this was done as "a temporary measure" to selectively let some programs use 0 and others use 1 for lazy-fetching, at a very early part of these programs.

If we are to reduce this global, perhaps we should teach more codepaths not to lazy fetch by default. Once everybody gets converted like so, then index-pack can lose the assignment of 0 to the variable, as the global variable would be initialized to 0 and nobody will flip it to 1 to "temporarily opt into lazy fetching by default until it gets fixed". At that point, we can lose the global variable.

So "we want to reduce the use of this global" is not a good reason to do this change at all, without a convincing argument that says why everybody should do automatic lazy fetching of objects. If everybody should avoid doing automatic lazy fetching, a good first step to reduce the use of this global is not to touch index-pack that has already been fixed not to do so, no?