patch, 3 partsUse default values from settings instead of config

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.

I'm fairly new to the codebase (this is my first patch!), so I have some
questions/concerns that I wasn't able to figure out:
- prepare_repo_settings() may have undesirable side effects or may
  not always be callable
- I can't find tests that cover the changes to gc, so I'm not sure if my
  understanding of gc is correct

Let me know if there is something I'm missing :)

CC-ing Derrick Stolee who has done work on both prepare_repo_settings and gc.

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     | 23 ++++++++++++++++++++++-
 t/t5319-multi-pack-index.sh | 15 ++++++++++++++-
 4 files changed, 41 insertions(+), 7 deletions(-)

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.

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c          |  3 ++-
 t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

On Mon, Sep 13, 2021 at 11:12:19AM -0700, Glen Choo wrote:

Small nit; "the_repository->settings()" should be spelled as "the_repository->settings", since "settings" is not a function.

It may be worth noting that this was totally fine before core.commitGraph's default changed to true. That happened in 31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13), which is the first time this sort of regression would have appeared.

Here's the main part of your change, which is obviously correct (I'm glossing over the earlier part where you call prepare_repo_settings(); also required and obviously correct).

Nit; I recommend replacing the `-c` style configuration with `test_config`, which modifies `$GIT_DIR/config` but only for the duration of the sub-shell.

... which would allow you to get rid of something like this (since you can avoid modifying the state visible outside of this test).

But I'm not aware of a way to temporarily unset a configuration variable for the duration of a test, so here I would probably write:

    test_must_fail git -c core.commitGraph= fsck

which Git interprets as "pretend this variable is unset in-core".

Thanks, Taylor

On Mon, Sep 13, 2021 at 3:29 PM Taylor Blau <me@ttaylorr.com> wrote:

Small correction: There is no subshell here, so it would be more accurate to say "... for the duration of the test".

Aside: In fact, test_config() can't be used in subshells due to
implementation limitations which I won't go into.

On Mon, Sep 13, 2021 at 03:33:40PM -0400, Eric Sunshine wrote:

Yes, sorry about that: I did not mean to say "subshell" here (Glen: hopefully you didn't read too far into that, since test_config() does not work in subshells).

Thanks for catching that, Eric.

Thanks, Taylor

Thanks for the thorough review! I really appreciate it :)

On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:

Oh that's a good catch. Thanks!

Sounds good, I'll note that down.

Sounds good. This works great :)

From my testing, I suspect that git does not pretend the variable is unset, but rather, it pretends that the variable is set to the empty string. It seems that git behaves as if the variable is set to "false". This is called out in Documentation/git.txt:

  Including the equals but with an empty value (like `git -c
  foo.bar= ...`) sets `foo.bar` to the empty string which `git config
  --type=bool` will convert to `false`.

If the variable really is set to false, how might we proceed here? Shall we stick with test_when_finished?

On Mon, Sep 13, 2021 at 7:19 PM Glen Choo <chooglen@google.com> wrote:

That's probably reasonable, however, for robustness, you should probably use test_unconfig() rather than raw `git config --unset` to clear the variable.

Aside: This certainly makes one wonder if we should have a new
function in t/test-lib-functions.sh which unsets a variable for the
duration of a test only. However, that's outside the scope of this
submission.

On Mon, Sep 13, 2021 at 07:32:07PM -0400, Eric Sunshine wrote:

Hmm. I'm not so sure, do other tests rely on the value of that variable? If so, test_unconfig() won't restore them.

Even if we don't have any such tests now, it seems like we should err on the side of leaving it alone (although I suppose that future tests could set core.commitGraph to whatever value they need as long as they use the test_config function).

:-). I thought the same thing to myself when reviewing earlier today. That's why I recommended using test_when_finished upthread, but either approach is fine (my comments are definitely cosmetic, and don't matter to the substance of this thread, so ultimately I am fine with either).

Thanks, Taylor

On Mon, Sep 13, 2021 at 9:09 PM Taylor Blau <me@ttaylorr.com> wrote:

There may be a misunderstanding. I wasn't saying that test_unconfig() alone would work. My "That's probably reasonable" referred to Glen's proposal of combining `git config --unset` with test_when_finished() to restore the variable. In addition to that, I suggested test_unconfig() as being a more robust choice in that recipe.

Yep.

On Mon, Sep 13, 2021 at 04:15:09PM -0700, Glen Choo wrote:

Hmm, I thought that we did support `git -c foo.bar=` as shorthand to unset the key `foo.bar`, but I must have been mistaken, because the documentation there is quite clear.

In that case, I think your original approach to use test_when_finished makes sense and is good. Thanks for double checking!

Thanks, Taylor

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.

Replace git_config_get_bool("core.multiPackIndex") in fsck with the equivalent call to the_repository->settings.multi_pack_index.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c              |  2 +-
 t/t5319-multi-pack-index.sh | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

On Mon, Sep 13, 2021 at 11:12:20AM -0700, Glen Choo wrote:

My same suggestion about referencing the first commit for which this would have been broken applies here, too. In this case, that'd be 18e449f86b (midx: enable core.multiPackIndex by default, 2020-09-25).

Good.

This test is a little awkward, since it looks like the first command is the same as the previous test (making it unnecessary). But it turns out that it *is* necessary, since it sets up the corrupt MIDX (which is thrown away at the end of each test).

So I would suggest one of two things:

  - Either replace this with a more flexible version of
    corrupt_midx_and_verify that allows the enclosing test to pass. Note
    that this may be awkward, since the whole point is that we want to
    notice the corruption in the first place.

  - Or, (more favorably) combine this all into a single test, like so:

        corrupt_midx_and_verify "$MIDX_BYTE_OFFSET" "\377" $objdir \
          "incorrect object offset" \
          "git fsck" &&
        test_must_fail git -c core.multiPackIndex=true fsck &&
        git -c core.multiPackIndex=false fsck

Which does all of the setup for the three tests at once, and then specifies each behavior individually.

Thanks, Taylor

builtin/gc.c has two ways of checking of 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().

Standardize on 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 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

On Mon, Sep 13, 2021 at 11:12:21AM -0700, Glen Choo wrote:

Is this hinting at why there are no new tests added here? If so, it may need to be explained more clearly, since I was a tad confused after reading it. But if not, this patch message deserves an extra sentence or two saying why tests aren't necessary.

Or if none of that is the case, and tests *are* necessary, then they should be added ;).

Thanks, Taylor

On Mon, Sep 13, 2021 at 03:37:26PM -0400, Taylor Blau wrote:

Looks like I'll need to be clearer; this wasn't my intention at all. I was hoping to describe the current state of affairs and to show that there are two different approaches. Thus, this patch is an attempt to 'standardize' the two approaches.

I initially did not include tests because it *seemed* to me that there were no tests for this. But on a second pass, it looks like that assumption was completely wrong (the tests are in t7900-maintenance.sh).

So, tests are necessary, and so I will add them :)

On 14/09/21 01.12, Glen Choo wrote:

I was wonder what the correlations between checking if MIDX is enabled and performing incremental repack are. What are these?

On Tue, Sep 14, 2021 at 11:00:34AM +0700, Bagas Sanjaya wrote:

(I'm not an expert on the issue, so don't take what I say as definitive :))

Here's what I was able to find in Documentation/git-maintenance.txt:

  The `incremental-repack` job repacks the object directory
	using the `multi-pack-index` feature.[...]

My guess at the intended behavior is that disabling midx should make git-maintenance behave as if it were unaware of midx features, such as incremental repack. This is quite akin to the documented behavior of fsck in this patch series, where fsck only verifies midx and commit-graphs when the respective features are enabled.

On Mon, Sep 13, 2021 at 11:12:18AM -0700, Glen Choo wrote:

Welcome to the Git community! :-).

Calling prepare_repo_settings() is definitely the right thing to do, because (as you note) it centralizes the default values for settings that it keeps track of.

You can call prepare_repo_settings() so long as you have a repository to call it on. Since fsck and gc must run inside of a repository, the callers you added are safe. And note that prepare_repo_settings() is idempotent, i.e., that it is a noop when called more than once.

Thanks, Taylor

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

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

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c          |  3 ++-
 t/t5318-commit-graph.sh | 24 +++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

On 9/17/21 6:54 PM, Glen Choo wrote:

Because I took the time to scan backward through this test script, I understand that `core.commitGraph` is already `true` by the time this test is reached, thus the new test title is accurate (for now). However, it would be a bit easier to reason about this and make it more robust by having the test itself guarantee that it is set to `true` by invoking `git config core.commitGraph true`.

test_config() unsets the config variable when the test completes, so I'm wondering if its use is in fact desirable here. I ask because, from a quick scan through the file, it appears that many tests in this script assume that `core.commitGraph` is `true`, so it's not clear that unsetting it at the end of this test is a good idea in general.

I find it somewhat confusing to reason about the behavior of test_when_finished() when it is invoked like this before the `cd`. It's true that the end-of-test `git config core.commitGraph true` will be invoked within `full` as desired (except in the very unusual case of the `cd` failing), so it's probably correct, but it requires extra thought to come to that conclusion. Switching the order of these two lines might lower the cognitive load a bit.

Taking Taylor's comment[1] on v1 patch [2/3] into account, I wonder if it would be simpler for these all to be combined into a single test. Something like (untested):

     cd "$TRASH_DIRECTORY/full" &&
     test_when_finished "git config core.commitGraph true" &&
     git config core.commitGraph true &&
     git fsck &&
     corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
         "incorrect checksum" &&
     cp commit-graph-pre-write-test $objdir/info/commit-graph &&
     test_must_fail git fsck &&
     git config core.commitGraph false &&
     git fsck &&
     git config --unset-all core.commitGraph &&
     test_must_fail git fsck

I didn't bother using test_unconfig() since, in this case, we _know_ that the config variable is set, thus don't have to worry about `git config --unset-all` failing.

A downside of combining the tests like this is that it makes it a bit more cumbersome to diagnose a failure (because there is more code and more cases to wade through).

Anyhow, I don't have a strong feeling one way or the other about combining the test or leaving them separate.

[1]: https://lore.kernel.org/git/YT+n81yGPYEiMXwQ@nand.local/

Eric Sunshine <sunshine@sunshineco.com> writes:

Reading this and..

I'll make both of these changes. Test readability is important and people shouldn't be wasting time thinking about test correctness.

This is a good point. I made the original change in response to Taylor's comment, but I think we both didn't consider that this script assumes `core.commitGraph` is `true`. The rest of the tests pass, but only because the default value is true and I'd rather not have tests rely on a happy accident.

Interesting thought. The marginal benefit here is much lower than in patch [2/3]. The difference is that corrupt_midx_and_verify() uses an additional $COMMAND to perform verification, but corrupt_graph_and_verify() does not. The result is that corrupt_midx_and_verify() is subtle and confusing when used in separate tests, but corrupt_graph_and_verify() is not so bad.

Yes, and we also lose the test labels that would guide the reader. It might not be that obvious why the tests for a boolean value has 3 cases {true,false,unset}.

Taking churn into account, I'm inclined to keep the tests separate.

I said I would incorporate these suggestions, but I didn't propose what changes I would actually make.

Given that each test depends on a global config value in the setup phase, it might be simplest to read if we try to avoid anything that touches that value. The easiest way to achieve this is to use git -c core.commitGraph {true,false} for the {true,false} cases. Since there is no -c equivalent for the unset case, I'll continue to use test_unconfig() + test_when_finished() to temporarily unset the value.

On Thu, Sep 30, 2021 at 2:35 PM Glen Choo <chooglen@google.com> wrote:

That was my thought, as well. (I wasn't quite sure why Taylor recommended test_config() over `-c` which you used originally. It may just be his personal preference. Perhaps he can chime in?)

Eric Sunshine <sunshine@sunshineco.com> writes:

I'd also appreciate thoughts on test_config() over `-c` :). I won't re-roll this series yet in case there's more to the test_config() story, but I've included a patch that shows the `-c` work that we discussed.

---
 t/t5318-commit-graph.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c              | 2 +-
 t/t5319-multi-pack-index.sh | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

On 9/17/21 6:54 PM, Glen Choo wrote:

I guess the newly-added `test_must_fail git fsck` line is checking the fallback case then `core.multipackindex` is not set. We could make this check a bit more robust by _ensuring_ that it is unset rather than relying upon whatever state the configuration is in by the time this test is reached. Perhaps something like this:

     ...
     "git -c core.multipackindex=true fsck" &&
     test_unconfig core.multipackindex &&
     test_must_fail git fsck &&
     git -c core.multipackindex=false fsck

The indentation is a bit unusual. It aligns nicely and is, in some sense, easy to read, but the two new lines are over-indented considering that they are siblings of the corrupt_midx_and_verify() call.

Don't know if any of this is worth a re-roll, though.

Eric Sunshine <sunshine@sunshineco.com> writes:

I think this extra robustness is worth it. I sometimes find that the tests are a bit too interdependent to read on their own, so this is a good step forward.

I agree. This was a typo from me, not a conscious choice.

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

On 9/17/21 6:54 PM, Glen Choo wrote:

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

I wasn't clear about this in the original message, but I think this is ready to merge in its current form. I'd love to hear from reviewers who can poke holes in that :)

Cc Eric and Taylor who have given the most review on v1 (thanks again!). I believe I've addressed your comments and I'd be interesting in hearing your thoughts on v2.

On Mon, Sep 27, 2021 at 1:59 PM Glen Choo <chooglen@google.com> wrote:

I re-read the entire thread and left a few comments on the v2 patches. One or two of the comments might be actionable (i.e. deserve a re-roll), but most are probably subjective.

Oh, I forgot to mention in my review of [3/3] that in:

    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.

you can probably drop one or both "just"s.

Eric Sunshine <sunshine@sunshineco.com> writes:

Thanks so much again!

Yes, this is definitely a typo from me.

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.

I don't anticipate any more business logic changes and the previous comments were focused on testing style, so hopefully this should be good to merge.

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/t5318-commit-graph.sh     | 23 ++++++++++++++++++++++-
 t/t5319-multi-pack-index.sh |  5 ++++-
 t/t7900-maintenance.sh      | 28 ++++++++++++++++++++++++----
 5 files changed, 55 insertions(+), 11 deletions(-)

Range-diff against v2:
1:  97ab2bb529 ! 1:  2f9ff949e6 fsck: verify commit graph when implicitly enabled
    @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '
      	cd "$TRASH_DIRECTORY/full" &&
      	git fsck &&
      	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
    -@@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
    - 	test_must_fail git fsck
    - '
    - 
    + 		"incorrect checksum" &&
    + 	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
    ++	test_must_fail git -c core.commitGraph=true fsck
    ++'
    ++
     +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '
     +	cd "$TRASH_DIRECTORY/full" &&
     +	git fsck &&
     +	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
     +		"incorrect checksum" &&
     +	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
    -+	test_config core.commitGraph false &&
    -+	git fsck
    ++	git -c core.commitGraph=false fsck
     +'
     +
     +test_expect_success 'git fsck (checks commit-graph when config unset)' '
    ++	cd "$TRASH_DIRECTORY/full" &&
     +	test_when_finished "git config core.commitGraph true" &&
     +
    -+	cd "$TRASH_DIRECTORY/full" &&
     +	git fsck &&
     +	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
     +		"incorrect checksum" &&
     +	test_unconfig core.commitGraph &&
     +	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
    -+	test_must_fail git fsck
    -+'
    -+
    - test_expect_success 'setup non-the_repository tests' '
    - 	rm -rf repo &&
    - 	git init repo &&
    + 	test_must_fail git fsck
    + '
    + 
2:  ace92453ca ! 2:  b13ca2a695 fsck: verify multi-pack-index when implictly enabled
    @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '
      	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
    ++		"git -c core.multiPackIndex=true fsck" &&
    ++	test_unconfig core.multiPackIndex &&
    ++	test_must_fail git fsck &&
    ++	git -c core.multiPackIndex=false fsck
      '
      
      test_expect_success 'corrupt MIDX is not reused' '
3:  d6959d61c1 < -:  ---------- gc: perform incremental repack when implictly enabled
-:  ---------- > 3:  76943aff80 gc: perform incremental repack when implictly 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).

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c          |  3 ++-
 t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c              | 2 +-
 t/t5319-multi-pack-index.sh | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

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 always 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 | 28 ++++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 7 deletions(-)

On Mon, Oct 04 2021, Glen Choo wrote:

I've looked this over carefully, it LGTM. I noticed some bugs in this area, but they pre-date your commits. I've pushed a "gc/use-repo-settings" to https://github.com/avar/git.git that you can check out, consider that branch partially as commentary, but feel free to pick/squash anything you find there.

The range-diff for that is quoted below. Notes:

 * This is somewhat of a case of throwing rocks from a glass house, but
   I think the commit messages could really be shorter/get to the
   point. I.e. say right away what's being changed and why, instead of
   giving the reader an overview of the whole control flow.

   I just adjusted adusted 1/3 in that direction below, I think 2/3
   could do with being squashed into it & adjusted, it's really the
   exact same bug/improvement, just with a different config key that
   behaves the same.

 * The "test_unconfig" in your 2/3 is redundant, so is the "rm -rf" of
   directories that can't exist by that point in 3/3.

 * I wondered if we could just call prepare_repo_settings() in one place
   earlier in gc/maintenance instead of adding yet another call deeper
   in the control flow. I've got a 4/3 below that tries to do that, and
   (as noted in its commit message) intentionally breaks "gc.c" in a way
   that our tests don't catch, which is perhaps fallout from
   31b1de6a09b?

   So that's very much on the topic of extra improvements, i.e. your
   patches don't break that anymore than it is now, but I for one would
   very much like to see an end-state where we're confident that
   gc/maintenance do the right thing on each of the tri-states of these
   boolean variables.

   It's also a point about how to write reliable tests (again,
   pre-dating your series). I.e. instead of asserting "we do X when Y is
   true" have the test assert all of Y=[unset|false|true] in a way that
   breaks if any are different than expected.

   One of the tests you're changing has a "graph_git_two_modes" helper
   does 2/3 of that, maybe it would be easy to extend it?

 * You use corrupting the graph as a shorthand for "did we run this
   command?". See test_region for an approach that might be better,
   i.e. just log with trace2 and grep that to see if we started the
   relevant command. The test_region helper can't be used as-is for that
   (you could manually grep the emitted PERF/EVENT output), but perhaps
   that's easier/more reliable?

 * This is again, something that pre-dates your patches, but I think
   following the "is enabled?" behavior here for these variables in "git
   fsck" is highly questionable. So just some thoughts after having
   looked at that:

   Just because your config doen't say "use the graph" doesn't mean
   another process won't do so, and if the graph is broken they might
   probably encounter an error.

   Although I think we *should* have cleaned up those being fatal a
   while ago). See 43d35618055 (commit-graph write: don't die if the
   existing graph is corrupt, 2019-03-25).

   But in general for these auxiliary files (commit-graph, midix, *.rev
   etc.) I think there's a good argument to be made that fsck should
   *always* check them, and at most use the config to say something
   like:

       Hey, you've got a 100% broken commit-graph/midx/rev in your .git,
       looks like the core.XYZ enabling it is off *right now* though, so
       maybe we won't use it.  I hope you're feeling lucky...

   :)

1:  fd9a58e2c7b ! 1:  c6041f3b633 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 start "commit-graph verify" if the
    +    config key was missing, which since 31b1de6a09 (commit-graph: turn on
    +    commit-graph by default, 2019-08-13) has meant that it's 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 check that this works, using
    +    the detection of a corrupt commit-graph as a method of seeing if
    +    "commit-graph verify" was invoked.
     
         Signed-off-by: Glen Choo <chooglen@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/fsck.c ##
     @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
2:  9d6813bc280 ! 2:  d11209ebfe1 fsck: verify multi-pack-index when implictly enabled
    @@ Commit message
     
         Signed-off-by: Glen Choo <chooglen@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/fsck.c ##
     @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
    @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '
      		"incorrect object offset" \
     -		"git -c core.multipackindex=true fsck"
     +		"git -c core.multiPackIndex=true fsck" &&
    -+	test_unconfig core.multiPackIndex &&
     +	test_must_fail git fsck &&
     +	git -c core.multiPackIndex=false fsck
      '
3:  260f46568c6 ! 3:  017e2003e85 gc: perform incremental repack when implictly enabled
    @@ Commit message
     
         Signed-off-by: Glen Choo <chooglen@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/gc.c ##
     @@ builtin/gc.c: static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)
    @@ t/t7900-maintenance.sh: test_expect_success 'maintenance.incremental-repack.auto
     +}
     +
     +test_expect_success 'maintenance.incremental-repack.auto' '
    -+	rm -rf incremental-repack-true &&
     +	git init incremental-repack-true &&
     +	(
     +		cd incremental-repack-true &&
    @@ t/t7900-maintenance.sh: test_expect_success 'maintenance.incremental-repack.auto
     +'
     +
     +test_expect_success 'maintenance.incremental-repack.auto (when config is unset)' '
    -+	rm -rf incremental-repack-unset &&
     +	git init incremental-repack-unset &&
     +	(
     +		cd incremental-repack-unset &&
-:  ----------- > 4:  04d1527f180 WIP gc/maintenance: just call prepare_repo_settings() earlier

-- >8 --
Subject: [PATCH] WIP gc/maintenance: just call prepare_repo_settings() earlier

Consolidate the various calls to prepare_repo_settings() to happen at the start of cmd_maintenance() (TODO: and cmd_gc()). This WIP commit intentionally breaks things, we seem to be lacking test coverage for cmd_gc(), perhaps since 31b1de6a09b (commit-graph: turn on commit-graph by default, 2019-08-13)?

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c            | 5 +----
 t/t5318-commit-graph.sh | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

On 10/5/2021 7:57 AM, Ævar Arnfjörð Bjarmason wrote:

I think that removing these calls is dangerous. prepare_repo_settings() already returns immediately if the repository already has its settings populated. The pattern of "call prepare before using a setting" is a safe way to future-proof the check from a movement of the call.

Putting that potential-future-problem aside, I don't see how this change is a _benefit_ other than fewer lines of code, which is not a quality measure in itself.

Thanks, -Stolee

On Tue, Oct 05 2021, Derrick Stolee wrote:

Very dangerous, yes :) As I noted this hunk intentionaly breaks the "git gc" command. Search for "I wondered if we could just call prepare_repo_settings[...]" upthread.

I.e. I was commenting on how Glen's patch makes an attempt to test this tri-state config for some cases, but not others, and that in this case we can break "git gc" without any existing test catching it.

Which as noted might have started happening when you flipped the core.commitGraph default, but I didn't test or confirm that, it just seemed the most likely place to start digging from a quick "git log -G...".

Anyway, as also noted I think this series is fine as-is, just some feedback in case Glen or anyone would be interested in either doing a v4 or a follow-up, i.e. when I tested this I found that some code/tests in this area were (probably) either fragile or already broken.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Your review is very thoughtful, thank you :)

In hindsight, I think this is how I should have initially structured my commits because this improves the signal-noise ratio. I'll keep it in mind in the future for sure. I'm hesitant to send a v4, but I can be persuaded to do so :)

These are redundant, but they serve a defensive purpose. By being extra defensive, I was hoping to make it clear to the reader that the test is *guaranteed* not to conflict with/rely on previous state and reduce the number of greps needed.

My intention here was not to assert "did git fsck call the function I expected?", but something more along the lines of "did git fsck catch the corrupted commit-graph?". Calling the function seems like an implementation detail, but the actual desired behavior is that git fsck catches the breakage.

Perhaps the test title should be reflect what I meant, like

  - git fsck (checks commit-graph when config set to true)
  + git fsck detects corrupted commit-graph

I think it depends on how we think about core.commitGraph (and others). Do we want Git to be able to pretend that commit-graph doesn't exist at all? If so, then we should skip the check in git fsck. Alternatively, is commit-graph actually part of Git's core offering? If so, core.commitGraph is more like a way to opt *out* of some commit-graph behavior, but fsck should still verify our core data structures. Given that we default to true, it sounds like more of the latter, but I'm not sure what use case this serves.

While that's true, I think that the user assumes a certain degree of risk if they are using different commands with different config values. I don't think it's unreasonable to say that "git -c core.commitGraph=false fsck" won't catch issues that will arise in "git -c core.commitGraph=true foo".

I think "always check" sounds like the best way to maximize visibility for users, though I don't think core.commitGraph should control the severity of the warning/error. If we pursue "always check", I think we could adopt fsck.<msg-id> instead.

Glen Choo <chooglen@google.com> writes:

With this merged to 'seen', the CI job with the extra set of GIT_TEST_X settings fail. When this topic is excluded, with all the other topics in flight in 'seen', everything seems to be OK.

For which GIT_TEST_X environment variables to set and export while testing to trigger the problem, see [*1*]

For a successful test run of 'seen' without this topic, see [*2*]

For the test log of the failing run with this topic, see [*3*]; you'd need to be logged into GitHub to see the details of the errors (e.g. click on "regular (linux-gcc...)" with red X sign on the left hand side, then open "Run ci/print-test-failures.sh" and look for "not ok").

[References]

*1* https://github.com/git/git/runs/3843549095?check_suite_focus=true#step:4:1677

    export GIT_TEST_SPLIT_INDEX=yes
    export GIT_TEST_MERGE_ALGORITHM=recursive
    export GIT_TEST_FULL_IN_PACK_ARRAY=true
    export GIT_TEST_OE_SIZE=10
    export GIT_TEST_OE_DELTA_SIZE=5
    export GIT_TEST_COMMIT_GRAPH=1
    export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
    export GIT_TEST_MULTI_PACK_INDEX=1
    export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
    export GIT_TEST_ADD_I_USE_BUILTIN=1
    export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
    export GIT_TEST_WRITE_REV_INDEX=1
    export GIT_TEST_CHECKOUT_WORKERS=2

*2* https://github.com/git/git/actions/runs/1322907901 (feff65d)

    A successful CI run of 'seen' without gc/use-repo-settings.

*3* https://github.com/git/git/actions/runs/1322842689 (54a31af)

    CI run of 'seen' with gc/use-repo-settings that fails.

    The commits that is in the failing 'seen' but not in the
    succeeding tree are those from this topic, as can be seen here:

    $ git shortlog --no-merges 54a31af ^feff65d
    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

Junio C Hamano <gitster@pobox.com> writes:

Thanks for the feedback, I did two CI runs on my fork in response:

* [1] uses the same tree as seen. Looks like osx-clang passes but
  linux-gcc fails.
* [2] is rebased onto master. Again, osx-clang passes but linux-gcc
  fails.

This should be enough for me to hunt down the issue, though I would appreciate any hints if anyone has any :)

[1] https://github.com/chooglen/git/runs/3862097340?check_suite_focus=true [2] https://github.com/chooglen/git/runs/3862670035?check_suite_focus=true

Glen Choo <chooglen@google.com> writes:

Using the set of GIT_TEST_FOO... settings I cited in the Reference#1, in the message you are responding to, will let you reproduce the issue locally on glinux boxes.

Junio C Hamano <gitster@pobox.com> writes:

Ah, I didn't notice that ci/run-build-and-tests.sh uses different settings for different environments. With those settings, I can reproduce the failure on my OS X machine.

Thanks so much!

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

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.

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           |  3 ++-
 t/t0410-partial-clone.sh |  6 +++++-
 t/t3800-mktag.sh         |  5 +++++
 t/t5318-commit-graph.sh  | 23 ++++++++++++++++++++++-
 4 files changed, 34 insertions(+), 3 deletions(-)

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              | 2 +-
 t/t5319-multi-pack-index.sh | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

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 always 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 | 28 ++++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 7 deletions(-)

Glen Choo <chooglen@google.com> writes:

OK.  Will replace.  Thanks.

On Tue, Oct 12 2021, Glen Choo wrote:

That's easy, but...

...how isn't disabling those t3800-mktag.sh tests just plasting over corruption that we're noticing because of your changes to (rightly) fix the bug where "fsck" wasn't checking the graph at all?

IOW haven't we just found exactly the sort of bug that "GIT_TEST_COMMIT_GRAPH" is put in place to find for us, but now instead of fixing it we're hiding it?

If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file I see that we fail N number of tests, but all of them are actually fallout of just this test:

        git replace $head_parent $head && 
        git replace -f $tree $blob 

I.e. we've created a replacement object replacing a tree with a blob, as part of tests I added to test how mktag handles those sorts of weird edge cases.

This then causes the graph verify code to throw a hissy fit with:

    root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in
    commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=
    0fbca9850869684085d654f9e1380c9780802570

I.e. when we wrote the graph we somehow didn't notice that the root tree node we wrote is to an object that's not actually a tree? Isn't this a bug where some part of the commit graph writing should be doing its own extended OID lookup that's replacement-object aware, it didn't, and we wrote a corrupt graph as a result?

If there is a legitimate reason why we're not just hiding a bug we've turned up with these fixes let's disable that one test, not the entire test file.

If you don't run the one test that fails (which is split up into 3 individual pieces) there's still 143 other tests that are run, all of those presumably benefit from finding future bugs with GIT_TEST_COMMIT_GRAPH=true, particularly since the test file seems to just have turned up one just now...

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I think this falls on my shoulders. I assumed that the failures were expected behavior, not bugs. You are right that we shouldn't be plastering over bugs.

I'll have to ask for help here because I don't know enough about mktag to distinguish between 'expected' and 'unexpected' failures. The best I can do is to add GIT_TEST_COMMIT_GRAPH=0 + NEEDSWORK for the failing tests. But if that's good enough for now, I'll just do that :)

On Tue, Oct 12 2021, Glen Choo wrote:

I don't think your series needs to be tasked with fixing a generic issue in the commit-graph you stumbled across.

So for your series I'd think the only required fix would be to narrowlry skip *just* the broken tests, not the entire test file.

But in this case (see http://lore.kernel.org/git/87pms8hq4s.fsf@evledraar.gmail.com) the fix seems rather trivial to me.

I.e. just adding one line to builtin/commit-graph.c, so perhaps you can see if that works for you and such a "this bug was missed because this mode didn't work" fix could be part of a re-roll of yours?

On 10/12/2021 4:34 PM, Ævar Arnfjörð Bjarmason wrote:...

The commit-graph should be disabled if replace-objects are enabled. If there is a bug being introduced here it is because the commit-graph is being checked during fsck even though it would never be read when the replace-objects exist.

Thanks, -Stolee

On Wed, Oct 13 2021, Derrick Stolee wrote:

Thanks, isn't the obvious fix for this to extend your d6538246d3d (commit-graph: not compatible with replace objects, 2018-08-20) to do "read_replace_refs = 0;" in graph_verify()? That works for me on this case.

I.e. if we ignore replace refs when writing the graph, and we don't use it if there are any due to commit_graph_compatible(), why would we consider them when verifying it?

On 10/13/2021 11:57 AM, Ævar Arnfjörð Bjarmason wrote:

Ignoring the replace refs while verifying will allow you to verify the
on-disk commit-graph file without issue.
 

I generally consider any interaction with the commit-graph while replace refs are enabled to be the bug. We don't read the commit-graph when replace refs are on, so why are we verifying it?

But, I understand the desire to check the on-disk content even though it is not being used, so disabling replace refs to do the verify should be sufficient.

Thanks, -Stolee

Derrick Stolee <stolee@gmail.com> writes:

It seems like we've converged on doing read_replace_refs = 0 \o/

If we are going to do this twice in graph_verify() and graph_write(), is there any reason why I shouldn't just do "read_replace_refs = 0" once in cmd_commit_graph()? IOW any time we do anything with commit-graphs, we should just ignore replace refs because they're incompatible.

On Thu, Oct 14 2021, Glen Choo wrote:

No reason, I think that's the best way to do this.

I've submitted a series to fix that verify issue, as noted in the CL these patches on top of it without that disabling of the mktag tests will pass with GIT_TEST_COMMIT_GRAPH=true): https://lore.kernel.org/git/cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com

Derrick Stolee <stolee@gmail.com> writes:

That's quite a sensible argument. Sounds like we have a way to solve this that everybody agrees with.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Would it be sensible to silently ignore, though? Would a warning, when we see replacement is in use and an explicit request by an end user is made to make use of or update the commit-graph, be too loud?

  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.

v5 is based off Ævar's fix (commit-graph: make "verify" work with replace refs, [1]). As Ævar noted in v4, the mktag test failures indicated buggy behavior "git commit-graph verify" when replace refs are enabled, so we should fix the bug instead of just disabling commit-graph.

Ævar, Derrick, thanks for the productive discussion, I feel like I understand this area a little better now.

Barring any unseen bugs, this should be ready to merge.

[1] https://lore.kernel.org/git/cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com/

Changes since v4
* Rebase onto [1], which fixes a bug with commit-graph verify with replace refs
  enabled.
* Remove GIT_TEST_COMMIT_GRAPH=0 in t3800-mktag.sh because the bug that it was
  covering up is now fixed.

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

Range-diff against v4:
1:  aac1253e7b ! 1:  567d40849a fsck: verify commit graph when implicitly enabled
    @@ Commit message
     
         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).
    +    disable GIT_TEST_COMMIT_GRAPH in t/t0410-partial-clone.sh because some
    +    test cases use fsck in ways that assume that commit-graph checking is
    +    disabled.
     
         Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Glen Choo <chooglen@google.com>
    @@ t/t0410-partial-clone.sh: test_expect_success 'rev-list stops traversal at missi
      	! 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:  ed64983430 = 2:  7cb44a7aeb fsck: verify multi-pack-index when implictly enabled
3:  821b492d8b = 3:  a436c45af7 gc: perform incremental repack when implictly enabled

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.

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 in t/t0410-partial-clone.sh because some test cases use fsck in ways that assume that commit-graph checking is disabled.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/fsck.c           |  3 ++-
 t/t0410-partial-clone.sh |  6 +++++-
 t/t5318-commit-graph.sh  | 23 ++++++++++++++++++++++-
 3 files changed, 29 insertions(+), 3 deletions(-)

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              | 2 +-
 t/t5319-multi-pack-index.sh | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

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 always 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 | 28 ++++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 7 deletions(-)

Thanks, all three changes look quite well reasoned.

Will queue.