From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 29 Sep 2021 06:39:39 GMT
Subject: Re: [PATCH v2 3/3] gc: perform incremental repack when implictly enabled
Message-ID: <f0eb2fd0-ce1e-5833-4d64-8569ba3bbedc@sunshineco.com>
In-Reply-To: <20210917225459.68086-4-chooglen@google.com>

On 9/17/21 6:54 PM, Glen Choo wrote:
> builtin/gc.c has two ways of checking if multi-pack-index is enabled:
> - git_config_get_bool() in incremental_repack_auto_condition()
> - the_repository->settings.core_multi_pack_index in
>    maintenance_task_incremental_repack()
> 
> The two implementations have existed since the incremental-repack task
> was introduced in e841a79a13 (maintenance: add incremental-repack auto
> condition, 2020-09-25). These two values can diverge because
> prepare_repo_settings() enables the feature in the_repository->settings
> by default.
> 
> In the case where core.multiPackIndex is not set in the config, the auto
> condition would fail, causing the incremental-repack task to not be
> run. Because we always want to consider the default values, we should
> just always just use the_repository->settings.
> 
> Standardize on using the_repository->settings.core_multi_pack_index to
> check if multi-pack-index is enabled.
> 
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -322,31 +322,69 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '
>   test_expect_success 'maintenance.incremental-repack.auto' '
> +	(
> +		git init incremental-repack-true &&
> +		cd incremental-repack-true &&
> +		git config core.multiPackIndex true &&
> +		test_commit A &&
> +		git repack -adk &&
> +		git multi-pack-index write &&
> +		GIT_TRACE2_EVENT="$(pwd)/midx-init.txt" git \
> +			-c maintenance.incremental-repack.auto=1 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand ! git multi-pack-index write --no-progress <midx-init.txt &&
> +		test_commit B &&
> +		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
> +		HEAD
> +		^HEAD~1
> +		EOF
> +		GIT_TRACE2_EVENT=$(pwd)/trace-A git \
> +			-c maintenance.incremental-repack.auto=2 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand ! git multi-pack-index write --no-progress <trace-A &&
> +		test_commit C &&
> +		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
> +		HEAD
> +		^HEAD~1
> +		EOF
> +		GIT_TRACE2_EVENT=$(pwd)/trace-B git \
> +			-c maintenance.incremental-repack.auto=2 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand git multi-pack-index write --no-progress <trace-B
> +	)
> +'
> +
> +test_expect_success 'maintenance.incremental-repack.auto (when config is unset)' '
> +	(
> +		git init incremental-repack-unset &&
> +		cd incremental-repack-unset &&
> +		test_unconfig core.multiPackIndex &&
> +		test_commit A &&
> +		git repack -adk &&
> +		git multi-pack-index write &&
> +		GIT_TRACE2_EVENT="$(pwd)/midx-init.txt" git \
> +			-c maintenance.incremental-repack.auto=1 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand ! git multi-pack-index write --no-progress <midx-init.txt &&
> +		test_commit B &&
> +		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
> +		HEAD
> +		^HEAD~1
> +		EOF
> +		GIT_TRACE2_EVENT=$(pwd)/trace-A git \
> +			-c maintenance.incremental-repack.auto=2 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand ! git multi-pack-index write --no-progress <trace-A &&
> +		test_commit C &&
> +		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
> +		HEAD
> +		^HEAD~1
> +		EOF
> +		GIT_TRACE2_EVENT=$(pwd)/trace-B git \
> +			-c maintenance.incremental-repack.auto=2 \
> +			maintenance run --auto --task=incremental-repack 2>/dev/null &&
> +		test_subcommand git multi-pack-index write --no-progress <trace-B
> +	)
>   '

The amount of code common between the two tests is significant. In 
simple cases, such duplication may not be worth worrying about, but it 
feels like we can do better here. There are a number of ways to re-use 
the code between tests. One such way would be to do something like this:

     run_incremental_repack () {
         test_commit A &&
         ...
         test_subcommand ...
     }

     test_expect_success 'maintenance.incremental-repack.auto' '
         rm -rf incremental-repack &&
         git init incremental-repack &&
         (
             cd incremental-repack &&
             git config core.multiPackIndex true &&
             run_incremental_repack
         )
     '

     test_expect_success 'maintenance.incremental-repack.auto (config 
unset)' '
         rm -rf incremental-repack &&
         git init incremental-repack &&
         (
             cd incremental-repack &&
             test_unconfig core.multiPackIndex &&
             run_incremental_repack
         )
     '