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

Patrick Steinhardt <[email protected]> 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.

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.

 * 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".

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