[PATCH v4 0/3] Use default values from settings instead of config

[Glen Choo <chooglen@google.com>]

Hi everyone! This patch was created in response to something we observed in
Google, where fsck failed to detect that the commit graph was invalid. We
initially assumed that fsck never checked the commit graph, but it turns out
that it does so only when core.commitgraph is set, even though we set defaults
for "whether to use the commit graph" in the repository settings.

Instead of using the config, let's use repository settings where
available. Replace core.commitGraph and core.multiPackIndex with their
equivalent repository settings in fsck and gc.

This re-roll is primarily motivated by the CI failures noted by Junio in [1].
The underlying cause is that GIT_TEST_COMMIT_GRAPH=1 (enabled in the linux-gcc
job) causes commands like "git commit" to write commit-graphs, but certain tests
like t/t0410-partial-clone.sh and t/t3800-mktag.sh intentionally create
corruptions that cause commit-graphs to become out-of-sync/invalid. Patch 1
fixes a bug where fsck did not check commit-graphs by default, which means that
these tests will now fail because they have invalid commit-graphs. The easiest
solution I found is to disable this confusing and noisy behavior with
GIT_TEST_COMMIT_GRAPH=0.

And since I am re-rolling, I incorporated Ævar's feedback regarding the commit
messages (thanks!). I considered combining patches 1 and 2, but patch 1 has a
grown a little to fix the CI issues, so I've decided to keep patches 1 and 2
separate.

This series has also seen a healthy amount of interest in test style and
coverage. We haven't converged on a path for the future, but I'd like to think
that the tests here are still a step in (approximately) the right direction.
Hopefully this version LGT us while we figure out what to do with tests :)

[1] https://lore.kernel.org/git/xmqqfstafyox.fsf@gitster.g/

Changes since v3
* Disable GIT_TEST_COMMIT_GRAPH in tests that intentionally corrupt things in a
  way that is incompatible with commit-graphs.
* Make patch 1 and 2's commit messages more concise (thanks Ævar!).

Changes since v2:
* Various small test fixes (thanks Eric!). Most notably, I've used -c instead of
  test_config because test_config can affect the values in subsequent tests.
* Rewording fix in patch 3 commit message
* Refactor tests in patch 3 so that we use a single helper function instead of
  copy-pasted code

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/t0410-partial-clone.sh    |  6 +++++-
 t/t3800-mktag.sh            |  5 +++++
 t/t5318-commit-graph.sh     | 23 ++++++++++++++++++++++-
 t/t5319-multi-pack-index.sh |  5 ++++-
 t/t7900-maintenance.sh      | 28 ++++++++++++++++++++++++----
 7 files changed, 65 insertions(+), 12 deletions(-)

Range-diff against v3:
1:  2f9ff949e6 ! 1:  aac1253e7b 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
    -    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. This worked fine until commit-graph was
    -    enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by
    -    default, 2019-08-13).
    +    Change fsck to check the "core_commit_graph" variable set in
    +    "repo-settings.c" instead of reading the "core.commitGraph" variable.
    +    This fixes a bug where we wouldn't verify the commit-graph if the
    +    config key was missing. This bug was introduced in
    +    31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13),
    +    where core.commitGraph was turned on by default.
     
    -    Replace git_config_get_bool("core.commitgraph") in fsck with the
    -    equivalent call to the_repository->settings.core_commit_graph.
    -
    -    The expected behavior is that fsck respects the config value when it is
    -    set, but uses the default value when it is unset. For example, for
    -    core.commitGraph, there are three cases:
    -
    -    - Config value is set to true -> enabled
    -    - Config value is set to false -> disabled
    -    - Config value is unset -> enabled
    -
    -    As such, tests cover all three cases.
    +    Add tests to "t5318-commit-graph.sh" to verify that fsck checks the
    +    commit-graph as expected for the 3 values of core.commitGraph. Also,
    +    disable GIT_TEST_COMMIT_GRAPH for tests that use fsck in ways that
    +    assume that commit-graph checking is disabled (t/t3800-mktag.sh,
    +    t/t0410-partial-clone.sh).
     
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Glen Choo <chooglen@google.com>
     
      ## builtin/fsck.c ##
    @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
      		const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL };
      
     
    + ## t/t0410-partial-clone.sh ##
    +@@ t/t0410-partial-clone.sh: test_description='partial clone'
    + 
    + # missing promisor objects cause repacks which write bitmaps to fail
    + GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
    ++# When enabled, some commands will write commit-graphs. This causes fsck
    ++# to fail when delete_object() is called because fsck will attempt to
    ++# verify the out-of-sync commit graph.
    ++GIT_TEST_COMMIT_GRAPH=0
    + 
    + delete_object () {
    + 	rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')
    +@@ t/t0410-partial-clone.sh: test_expect_success 'rev-list stops traversal at missing and promised commit' '
    + 
    + 	git -C repo config core.repositoryformatversion 1 &&
    + 	git -C repo config extensions.partialclone "arbitrary string" &&
    +-	GIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out &&
    ++	git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
    + 	grep $(git -C repo rev-parse bar) out &&
    + 	! grep $FOO out
    + '
    +
    + ## t/t3800-mktag.sh ##
    +@@ t/t3800-mktag.sh: test_description='git mktag: tag object verify test'
    + 
    + . ./test-lib.sh
    + 
    ++# When enabled, some commands will automatically write commit-graphs.
    ++# This will cause the mktag tests to fail because fsck will attempt to
    ++# verify the out-of-sync commit graph.
    ++GIT_TEST_COMMIT_GRAPH=0
    ++
    + ###########################################################
    + # check the tag.sig file, expecting verify_tag() to fail,
    + # and checking that the error message matches the pattern
    +
      ## t/t5318-commit-graph.sh ##
     @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '
      		$GRAPH_CHUNK_LOOKUP_OFFSET
2:  b13ca2a695 ! 2:  ed64983430 fsck: verify multi-pack-index when implictly enabled
    @@ Metadata
      ## Commit message ##
         fsck: verify multi-pack-index when implictly enabled
     
    -    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. 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.
    +    Like the previous commit, change fsck to check the
    +    "core_multi_pack_index" variable set in "repo-settings.c" instead of
    +    reading the "core.multiPackIndex" config variable. This fixes a bug
    +    where we wouldn't verify midx if the config key was missing. This bug
    +    was introduced in 18e449f86b (midx: enable core.multiPackIndex by
    +    default, 2020-09-25) where core.multiPackIndex was turned on by default.
     
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Glen Choo <chooglen@google.com>
     
      ## builtin/fsck.c ##
3:  76943aff80 = 3:  821b492d8b gc: perform incremental repack when implictly enabled
-- 
2.33.0.882.g93a45727a2-goog