Re: [PATCH v3 0/3] Use default values from settings instead of config

[Derrick Stolee <stolee@gmail.com>]

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@gmail.com>
> ---
>  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