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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 6, 2018, 05:39 UTC
Message-ID: <CAPig+cTJuvOjyzkmAj5axAkS3q1UyeV03jQk6-oB==KyzbEK_w@mail.gmail.com>
In-Reply-To: <20180706005321.124643-17-dstolee@microsoft.com>

On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 20 quoted lines

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

Show 15 quoted lines

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

Show 22 quoted lines

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

> +
> +compare_results_with_midx "one v2 pack"