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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 05 2021, Derrick Stolee wrote:

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

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.