| Message ID | 20210917225459.68086-4-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use default values from settings instead of config | expand |
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 ) '
diff --git a/builtin/gc.c b/builtin/gc.c index f05d2f0a1a..070b7dccb1 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1051,12 +1051,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts) static int incremental_repack_auto_condition(void) { struct packed_git *p; - int enabled; int incremental_repack_auto_limit = 10; int count = 0; - if (git_config_get_bool("core.multiPackIndex", &enabled) || - !enabled) + prepare_repo_settings(the_repository); + if (!the_repository->settings.core_multi_pack_index) return 0; git_config_get_int("maintenance.incremental-repack.auto", diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 58f46c77e6..2c77ddded1 100755 --- 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 repack -adk && - git config core.multiPackIndex true && - 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 A && - 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 B && - 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 + ( + 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 + ) ' test_expect_success 'pack-refs task' '
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> --- builtin/gc.c | 5 +-- t/t7900-maintenance.sh | 88 ++++++++++++++++++++++++++++++------------ 2 files changed, 65 insertions(+), 28 deletions(-)