From: Kousik Sanagavarapu Date: Sun, 19 Mar 2023 06:17:01 GMT Subject: Re: [PATCH v4] index-pack: remove fetch_if_missing=0 Message-ID: <92c321c4-7968-e993-4157-f0d06edb9283@gmail.com> In-Reply-To: On 18/03/23 04:28, Junio C Hamano wrote: > Kousik Sanagavarapu writes: > >> 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? Thanks for the review. Also, thanks for pointing out the direction of work in this area. Really helpful.