From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 12 Jul 2018 16:30:22 GMT
Subject: Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
Message-ID: <5ff95525-4677-872d-e249-b01b5aaf6c68@gmail.com>
In-Reply-To: <2270ec32-f5e3-e23a-5605-f5f0e7a44c46@gmail.com>

On 7/12/2018 9:19 AM, Derrick Stolee wrote:
> On 7/6/2018 1:39 AM, Eric Sunshine wrote:
>> On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee <stolee@gmail.com> wrote:
>>> The core.multiPackIndex config setting controls the multi-pack-
>>> index (MIDX) feature. If false, the setting will disable all reads
>>> from the multi-pack-index file.
>>>
>>> Add comparison commands in t5319-multi-pack-index.sh to check
>>> typical Git behavior remains the same as the config setting is turned
>>> on and off. This currently includes 'git rev-list' and 'git log'
>>> commands to trigger several object database reads.
>>>
>>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>>> ---
>>> diff --git a/cache.h b/cache.h
>>> @@ -814,6 +814,7 @@ extern char *git_replace_ref_base;
>>> +extern int core_multi_pack_index;
>>> diff --git a/config.c b/config.c
>>> @@ -1313,6 +1313,11 @@ static int git_default_core_config(const char 
>>> *var, const char *value)
>>> +       if (!strcmp(var, "core.multipackindex")) {
>>> +               core_multi_pack_index = git_config_bool(var, value);
>>> +               return 0;
>>> +       }
>> This is a rather unusual commit. This new configuration is assigned,
>> but it's never actually consulted, which means that it's impossible
>> for it to have any impact on functionality, yet tests are being added
>> to check whether it _did_ have any impact on functionality. Confusing.
>>
>> Patch 17 does consult 'core_multi_pack_index', so it's only at that
>> point that it could have any impact. This situation would be less
>> confusing if you swapped patches 16 and 17 (and, of course, move the
>> declaration of 'core_multi_pack_index' to patch 17 with a reasonable
>> default value).
>
> You're right that this commit is a bit too aware of the future, but I 
> disagree with the recommendation to change it.
>
> Yes, in this commit there is no possible way that these tests could 
> fail. The point is that patches 17-23 all change behavior if this 
> setting is on, and we want to make sure we do not break at any point 
> along that journey (or in future iterations of the multi-pack-index 
> feature).
>
> With this in mind, I don't think there is a better commit to place 
> these tests.

Of course, as I convert this global config variable into an on-demand 
check as promised [1] this commit seems even more trivial. I'm going to 
squash it with PATCH 17.

[1] 
https://public-inbox.org/git/b5733625-29c8-4317-ff44-d27c2fca11ce@gmail.com/