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

Message

Glen Choo Sept. 13, 2021, 6:12 p.m. UTC

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

Comments

Taylor Blau Sept. 13, 2021, 7:19 p.m. UTC | #1

On Mon, Sep 13, 2021 at 11:12:18AM -0700, Glen Choo wrote:
> 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:

Welcome to the Git community! :-).

> - prepare_repo_settings() may have undesirable side effects or may
>   not always be callable

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