On 10/5/2021 7:57 AM, Ævar Arnfjörð Bjarmason wrote: > 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@xxxxxxxxx> > --- > builtin/gc.c | 5 +---- > t/t5318-commit-graph.sh | 2 +- > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 26709311601..f59dbedc1fe 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -695,7 +695,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix) > clean_pack_garbage(); > } > > - prepare_repo_settings(the_repository); > if (the_repository->settings.gc_write_commit_graph == 1) 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
