Re: [PATCH v3 16/24] config: create core.multiPackIndex setting

> 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).

>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
>> @@ -61,12 +63,42 @@ test_expect_success 'write midx with one v1 pack' '
>> +midx_git_two_modes() {
>> +       git -c core.multiPackIndex=false $1 >expect &&
>> +       git -c core.multiPackIndex=true $1 >actual &&
>> +       test_cmp expect actual
>> +}
>> +
>> +compare_results_with_midx() {
>> +       MSG=$1
>> +       test_expect_success "check normal git operations: $MSG" '
>> +               midx_git_two_modes "rev-list --objects --all" &&
>> +               midx_git_two_modes "log --raw"
>> +       '
>> +}
> Here, you define midx_git_two_modes() and compare_results_with_midx()...
>
>>   test_expect_success 'write midx with one v2 pack' '
>> -       git pack-objects --index-version=2,0x40 pack/test <obj-list &&
>> -       git multi-pack-index --object-dir=. write &&
>> -       midx_read_expect 1 17 4 .
>> +       git pack-objects --index-version=2,0x40 $objdir/pack/test <obj-list &&
>> +       git multi-pack-index --object-dir=$objdir write &&
>> +       midx_read_expect 1 17 4 $objdir
>>   '
>>
>> +midx_git_two_modes() {
>> +       git -c core.multiPackIndex=false $1 >expect &&
>> +       git -c core.multiPackIndex=true $1 >actual &&
>> +       test_cmp expect actual
>> +}
>> +
>> +compare_results_with_midx() {
>> +       MSG=$1
>> +       test_expect_success "check normal git operations: $MSG" '
>> +               midx_git_two_modes "rev-list --objects --all" &&
>> +               midx_git_two_modes "log --raw"
>> +       '
>> +}
> ... and here you define both functions again.