Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 12, 2022, 18:27 UTC
- Message-ID
- <221212.86v8mg4gr2.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <1b2aa60c-633f-0a7f-e8e3-5d0f01ff94b6@github.com>
On Mon, Dec 12 2022, Derrick Stolee wrote:
Show 46 quoted lines
> 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...