From: Glen Choo <chooglen@google.com>
To: git@vger.kernel.org
Cc: Glen Choo <chooglen@google.com>
Subject: [PATCH v2 0/3] Use default values from settings instead of config
Date: Fri, 17 Sep 2021 15:54:56 -0700 [thread overview]
Message-ID: <20210917225459.68086-1-chooglen@google.com> (raw)
In-Reply-To: <20210913181221.42635-1-chooglen@google.com>
Thanks so much for the review, the comments have been really helpful!
v2 doesn't change any business logic, it's mostly focused on
incorporating the feedback around commit messages and tests.
Changes since v1:
* clean up typo in patch 1 commit message
* document the commits that patches 1 and 2 address
* use test helpers in patch 1
* rewrite patch 2's tests so that it's easier to tell that each test
does something different
* reword patch 3 commit message to explain the bug
* add tests to patch 3
Glen Choo (3):
fsck: verify commit graph when implicitly enabled
fsck: verify multi-pack-index when implictly enabled
gc: perform incremental repack when implictly enabled
builtin/fsck.c | 5 ++-
builtin/gc.c | 5 +--
t/t5318-commit-graph.sh | 24 +++++++++-
t/t5319-multi-pack-index.sh | 4 +-
t/t7900-maintenance.sh | 88 ++++++++++++++++++++++++++-----------
5 files changed, 94 insertions(+), 32 deletions(-)
Range-diff against v1:
1: 6bddc4e158 ! 1: 97ab2bb529 fsck: verify commit graph when implicitly enabled
@@ Metadata
## Commit message ##
fsck: verify commit graph when implicitly enabled
- the_repository->settings() is the preferred way to get certain
+ the_repository->settings is the preferred way to get certain
settings (such as "core.commitGraph") because it gets default values
from prepare_repo_settings(). However, cmd_fsck() reads the config
directly via git_config_get_bool(), which bypasses these default values.
This causes fsck to ignore the commit graph if "core.commitgraph" is not
- explicitly set in the config, even though commit graph is enabled by
- default.
+ explicitly set in the config. This worked fine until commit-graph was
+ enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by
+ default, 2019-08-13).
Replace git_config_get_bool("core.commitgraph") in fsck with the
equivalent call to the_repository->settings.core_commit_graph.
@@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
-+ git -c core.commitGraph=false fsck
++ test_config core.commitGraph false &&
++ git fsck
+'
+
+test_expect_success 'git fsck (checks commit-graph when config unset)' '
@@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
-+ git config --unset core.commitGraph &&
++ test_unconfig core.commitGraph &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
+ test_must_fail git fsck
+'
2: 3b86afc45b ! 2: ace92453ca fsck: verify multi-pack-index when implictly enabled
@@ Commit message
Like the previous commit, the_repository->settings.core_multi_pack_index
is preferable to reading "core.multiPackIndex" from the config because
prepare_repo_settings() sets a default value for
- the_repository->settings.
+ the_repository->settings. This worked fine until core.multiPackIndex was
+ enabled by default in 18e449f86b (midx: enable core.multiPackIndex by
+ default, 2020-09-25).
Replace git_config_get_bool("core.multiPackIndex") in fsck with the
equivalent call to the_repository->settings.multi_pack_index.
@@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
## t/t5319-multi-pack-index.sh ##
@@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '
- "incorrect object offset"
- '
-
--test_expect_success 'git-fsck incorrect offset' '
-+test_expect_success 'git-fsck incorrect offset (config set to true)' '
+ test_expect_success 'git-fsck incorrect offset' '
corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
"incorrect object offset" \
- "git -c core.multipackindex=true fsck"
- '
-
-+test_expect_success 'git-fsck incorrect offset (config set to false)' '
-+ corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
-+ "incorrect object offset" \
+- "git -c core.multipackindex=true fsck"
+ "git -c core.multipackindex=true fsck" &&
++ test_must_fail git fsck &&
+ git -c core.multipackindex=false fsck
-+'
-+
-+test_expect_success 'git-fsck incorrect offset (config unset)' '
-+ corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
-+ "incorrect object offset" \
-+ "git fsck"
-+'
-+
+ '
+
test_expect_success 'corrupt MIDX is not reused' '
- corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
- "incorrect object offset" &&
3: d122ce3bf8 ! 3: d6959d61c1 gc: perform incremental repack when implictly enabled
@@ Metadata
## Commit message ##
gc: perform incremental repack when implictly enabled
- builtin/gc.c has two ways of checking of multi-pack-index is enabled:
+ 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_repository->settings.core_multi_pack_index should be preferred
- because it has default values from prepare_repo_settings().
+ 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.
- Standardize on the_repository->settings.core_multi_pack_index to check
- if multi-pack-index is enabled.
+ 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: static int maintenance_task_loose_objects(struct maintenance_run_o
return 0;
git_config_get_int("maintenance.incremental-repack.auto",
+
+ ## t/t7900-maintenance.sh ##
+@@ t/t7900-maintenance.sh: 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' '
--
2.33.0.464.g1972c5931b-goog
next prev parent reply other threads:[~2021-09-17 22:55 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-13 18:12 [PATCH 0/3] Use default values from settings instead of config Glen Choo
2021-09-13 18:12 ` [PATCH 1/3] fsck: verify commit graph when implicitly enabled Glen Choo
2021-09-13 19:29 ` Taylor Blau
2021-09-13 19:33 ` Eric Sunshine
2021-09-13 19:36 ` Taylor Blau
2021-09-13 23:15 ` Glen Choo
2021-09-13 23:32 ` Eric Sunshine
2021-09-14 1:09 ` Taylor Blau
2021-09-14 2:05 ` Eric Sunshine
2021-09-14 1:07 ` Taylor Blau
2021-09-13 18:12 ` [PATCH 2/3] fsck: verify multi-pack-index when implictly enabled Glen Choo
2021-09-13 19:35 ` Taylor Blau
2021-09-13 18:12 ` [PATCH 3/3] gc: perform incremental repack " Glen Choo
2021-09-13 19:37 ` Taylor Blau
2021-09-14 17:41 ` Glen Choo
2021-09-14 4:00 ` Bagas Sanjaya
2021-09-16 17:15 ` Glen Choo
2021-09-13 19:19 ` [PATCH 0/3] Use default values from settings instead of config Taylor Blau
2021-09-17 22:54 ` Glen Choo [this message]
2021-09-17 22:54 ` [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled Glen Choo
2021-09-29 6:09 ` Eric Sunshine
2021-09-30 18:00 ` Glen Choo
2021-09-30 18:35 ` Glen Choo
2021-09-30 18:39 ` Eric Sunshine
2021-10-01 17:28 ` Glen Choo
2021-09-17 22:54 ` [PATCH v2 2/3] fsck: verify multi-pack-index when implictly enabled Glen Choo
2021-09-29 6:20 ` Eric Sunshine
2021-09-29 22:56 ` Glen Choo
2021-09-17 22:54 ` [PATCH v2 3/3] gc: perform incremental repack " Glen Choo
2021-09-29 6:39 ` Eric Sunshine
2021-09-27 17:59 ` [PATCH v2 0/3] Use default values from settings instead of config Glen Choo
2021-09-29 6:43 ` Eric Sunshine
2021-09-29 22:53 ` Glen Choo
2021-10-05 0:19 ` [PATCH v3 " Glen Choo
2021-10-05 0:19 ` [PATCH v3 1/3] fsck: verify commit graph when implicitly enabled Glen Choo
2021-10-05 0:19 ` [PATCH v3 2/3] fsck: verify multi-pack-index when implictly enabled Glen Choo
2021-10-05 0:19 ` [PATCH v3 3/3] gc: perform incremental repack " Glen Choo
2021-10-05 11:57 ` [PATCH v3 0/3] Use default values from settings instead of config Ævar Arnfjörð Bjarmason
2021-10-05 17:43 ` Derrick Stolee
2021-10-05 19:10 ` Ævar Arnfjörð Bjarmason
2021-10-05 22:25 ` Glen Choo
2021-10-09 7:24 ` Junio C Hamano
2021-10-11 19:58 ` Glen Choo
2021-10-11 20:08 ` Junio C Hamano
2021-10-11 20:48 ` Glen Choo
2021-10-12 17:42 ` [PATCH v4 " Glen Choo
2021-10-12 17:42 ` [PATCH v4 1/3] fsck: verify commit graph when implicitly enabled Glen Choo
2021-10-12 17:42 ` [PATCH v4 2/3] fsck: verify multi-pack-index when implictly enabled Glen Choo
2021-10-12 17:42 ` [PATCH v4 3/3] gc: perform incremental repack " Glen Choo
2021-10-12 20:23 ` [PATCH v4 0/3] Use default values from settings instead of config Junio C Hamano
2021-10-12 20:34 ` Ævar Arnfjörð Bjarmason
2021-10-12 22:29 ` Glen Choo
2021-10-14 15:53 ` Ævar Arnfjörð Bjarmason
2021-10-13 13:12 ` Derrick Stolee
2021-10-13 15:57 ` Ævar Arnfjörð Bjarmason
2021-10-14 16:53 ` Derrick Stolee
2021-10-14 22:21 ` Glen Choo
2021-10-14 23:38 ` Ævar Arnfjörð Bjarmason
2021-10-14 22:25 ` Junio C Hamano
2021-10-15 15:57 ` Junio C Hamano
2021-10-15 20:16 ` [PATCH v5 " Glen Choo
2021-10-15 20:16 ` [PATCH v5 1/3] fsck: verify commit graph when implicitly enabled Glen Choo
2021-10-15 20:16 ` [PATCH v5 2/3] fsck: verify multi-pack-index when implictly enabled Glen Choo
2021-10-15 20:16 ` [PATCH v5 3/3] gc: perform incremental repack " Glen Choo
2021-10-15 21:31 ` [PATCH v5 0/3] Use default values from settings instead of config Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210917225459.68086-1-chooglen@google.com \
--to=chooglen@google.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).