On 18/03/23 04:28, Junio C Hamano wrote:
Kousik Sanagavarapu <five231003@xxxxxxxxx> 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.
