Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com, vdye@github.com,
	newren@gmail.com
Subject: Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes
Date: Mon, 12 Dec 2022 19:27:01 +0100	[thread overview]
Message-ID: <221212.86v8mg4gr2.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <1b2aa60c-633f-0a7f-e8e3-5d0f01ff94b6@github.com>

On Mon, Dec 12 2022, Derrick Stolee wrote:

> On 12/7/2022 5:30 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:
>> 
>>> From: Derrick Stolee <derrickstolee@github.com>
>>> [...]
>>> diff --git a/read-cache.c b/read-cache.c
>>> index fb4d6fb6387..1844953fba7 100644
>>> --- a/read-cache.c
>>> +++ b/read-cache.c
>>> @@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>>>  	int ieot_entries = 1;
>>>  	struct index_entry_offset_table *ieot = NULL;
>>>  	int nr, nr_threads;
>>> -	int skip_hash;
>>>  
>>>  	f = hashfd(tempfile->fd, tempfile->filename.buf);
>>>  
>>> -	if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
>>> -		f->skip_hash = skip_hash;
>>> +	if (istate->repo) {
>>> +		prepare_repo_settings(istate->repo);
>>> +		f->skip_hash = istate->repo->settings.index_skip_hash;
>>> +	}
>> 
>> Urm, are we ever going to find ourselves in a situation where:
>> 
>>  * We have read the settings for the_repository
>>  * We have an index we're about to write out as our "main index", but
>>    the istate->repo *isn't* the_repository.
>>  * Even then, wouldn't the two copies of the repos have read the same
>>    repo settings?
>> 
>> But maybe there's a really obvious submodule / worktree / whatever edge
>> case I'm missing.
>> 
>> But if not, shouldn't we just always read/write this from
>> the_repository?
>
> I don't understand your concern. We call prepare_repo_settings(istate->repo)
> just before using these settings, so we are using whatever repository-local
> config we have available to us.
>
> If you're thinking that we could be writing an index but istate->repo is
> somehow the "wrong" repo, then that is a larger problem. This patch is
> doing the best thing it can with the information it is given.

It's not a concern, just confusion :)

In the preceding step (and this is still the case in your v2) we used
git_config_get_maybe_bool(), if we meant to use istate->repo shouldn't
we have used repo_config_get_maybe_bool() to begin with?

And will we ever get !istate->repo? If not should we BUG() here?
Otherwise the 4/4 changes this to a state where we'll no longer read the
index.skipHash setting if that "repo" is NULL, but our previous
the_repository was non-NULL...