Re: [PATCH v2] repository: move fetch_if_missing into struct repository

[Tian Yuchen <cat@malon.dev>]

On 8/5/26 01:38, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
>>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>>> index 0793dc595c..74f9694662 100644
>>> --- a/builtin/index-pack.c
>>> +++ b/builtin/index-pack.c
>>> @@ -1898,15 +1898,16 @@ int cmd_index_pack(int argc,
>>>   	int report_end_of_input = 0;
>>>   	int hash_algo = 0;
>>>   
>>> +	show_usage_if_asked(argc, argv, index_pack_usage);
>>> +
>>>   	/*
>>>   	 * index-pack never needs to fetch missing objects except when
>>>   	 * REF_DELTA bases are missing (which are explicitly handled). It only
>>>   	 * accesses the repo to do hash collision checks and to check which
>>>   	 * REF_DELTA bases need to be fetched.
>>>   	 */
>>> -	fetch_if_missing = 0;
>>> -
>>> -	show_usage_if_asked(argc, argv, index_pack_usage);
>>> +	if (repo)
>>> +		the_repository->fetch_if_missing = 0;
>>>   
>>>   	disable_replace_refs();
>>>   
>>
>> This one looks a bit weird -- we check for `repo`, but then set
>> `the_repository->fetch_if_missing`. We can probably just loose the
>> conditional completely, and furthermore we don't need to reorder any
>> code here at all anymore.
> 
> The 4-line comment is about disabling fetch-if-missing, so the code
> movement is not even unnecessary, but it is harmful, I think.  If
> the command can work without repository, incoming "repo" might be
> NULL, and unconditionally doing
> 
> 	repo->fetch_if_missing = 0;
> 
> may cause a crash.  But that is not an excuse to blindly add
> 
> 	if (repo)
> 
> in front of such an assignment.

Yes, this is a mistake. :(

> 
> It gives you a chance to rethink what you are doing.
> 
> If a command can work without a repository, yet it cares about how
> fetch_if_missing bit is set, it hints that it may be a mistake in
> the first place to try associating fetch_if_missing bit with a
> particular struct repository instance, as you must be prepared to
> work with repo==NULL.
> 
> There could be at least three approaches you may have to think about
> at that point.
> 
>   * Perhaps the command may not have to work outside a repository at
>     all.  If so, then it is a bug for the caller to call this
>     function with repo==NULL.  So we should just say
> 
> 	repo->fetch_if_missing = 0;
> 
>     without "if (repo)" check at all here.  After all, the situation
>     we might want to enable fetch_if_missing is where we have a place
>     to fetch into, so by definition, we _should_ have a repository in
>     such a case.
> 

Yep, this approach looks the most reasonable to me. I think I will apply 
this approach and update the commit message.

>   * Perhaps the command may want to work outside a repository but it
>     may be acceptable to operate in a degraded way.  By definition,
>     when we are outside a repository, we have no object store to
>     fetch objects lazily into, so fetch_if_missing MUST BE off.
> 
>     Because Git is primarily about working inside a repository,
>     perhaps it may be acceptable, even when you are outside a
>     repository, to assume that the_repository can be used as a
>     back-up "fake repository" object, and fetch_if_missing and its
>     friends that are necessary to have their meaning to be in that
>     fake repository object.  If that the stance we are going to take,
>     this part should probably say:
> 
> 	(repo ? repo : the_repository)->fetch_if_missing = 0;
> 
>     We need to make sure that everybody who passes the code paths
>     that ever reference fetch_if_missing would pass the_repository
>     down when the command is running outside a repository, though.
> 
>   * Or perhaps there are some settings that really need to be
>     available whether you are in a repository or not.  I think
>     fetch_if_missing is a borderline case, but more generally, things
>     like user.name should conceptually be available even outside a
>     repository, with in-repository configuration files overriding
>     them.  And it may be a mistake to force such settings to be
>     stored in an instance of "struct repository" (or repo_settings
>     that is part of it).  We would need a framework to represent a
>     structure in which a basic setting, which does not belong to any
>     repository (whose members may be the same as those in "struct
>     repo_settings", so I think it is OK to use an instance of that
>     struct to represnt this "basic settings that is global"), exists
>     globally, and it is overriden by per repository setting, which is
>     in "struct repo_settings" embedded in "struct repository".
> 

I'm not sure if I'm right, but I feel that 'fetch_if_missing' 
essentially carries two meanings simultaneously: "Does this repository 
allow lazy fetching?" and "Does this object lookup allow network 
behavior to be triggered?" In other words, suppose we consider 
'fetch_if_missing' to meet the third case, allowing it to exist in an 
ambiguous situation, i.e. it makes sense both within and outside the 
repository...Can we then consider the existence of this variable itself 
to have some semantic ambiguity? If so, do we really need to invent 
another mechanism to accommodate this ambiguity? Or should we 
temporarily apply minimal changes and discuss the ownership issue when 
the time is right (i.e. subsystems ready for 'repo' rather than 
'the_repository' only)? Perhaps this is what you mean by "a borderline 
case".

Furthermore: when we do libification, is the goal to eliminate global 
state, or to "package global state more reasonably"? The third approach 
seems to be the latter one.

> The earlier choices require fewer changes than the later choices,
> but the later choices are more concepturely pure, I think.
> 

All in all, I think your core point is that we shouldn't blindly put all 
global variables in a struct repository. I totally agree with that.

> 

Thanks! yuchen