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@xxxxxxxxxx>
>>> [...]
>>> 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...
