Re: [PATCH 05/10] setup: use the default algorithm to initialize repo format

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2025, 21:05 UTC
Message-ID: <xmqqqzze15ug.fsf@gitster.g>
In-Reply-To: <aFXEgLRLRdbaPOb2@fruit.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 13 quoted lines

>> > -	if (format->version == -1)
>> > +	if (format->version == -1) {
>> 
>> And if there is no core.repositoryformatversion set, we will come
>> here.  According to the comment before handle_extension_v0(), some
>> extensions.* should still be honored even in such a repository, and
>> the above call to git_config_from_file() should have handled them
>> just fine.
>> 
>> However, I do not understand why we clear all of what we read with
>> another call to clear_repository_format() here.
>
> Because this is the case where there's no config file.

But my worries come from that .version == -1 does not necessarily mean a missing config. Missing config will give .version == -1 but the opposite may not be true, no?

> If nobody
> bothered to write a configuration file, then we want to reset everything
> to the default.

True. If config file is missing, yes, .version will be -1 and clearing may make sense. But if the file is missing, we wouldn't have anything to "reset to the default" because we wouldn't have read anything, so what clear_repository_format() call initialized to the default before we read config from file would still be there, no?

Show 5 quoted lines

> I don't know what we do if we have a repository with a config file and
> no version, but literally every repository since Git 0.99.3 (I believe)
> has core.repositoryformatversion written into the repo.  I'm certain
> that the behaviour we'd want if nobody specified one was to do the most
> compatible thing, so the defaults seem prudent.

If our assumption is that no config file in repositories we care about should lack core.repositoryformatversion, then what you wrote above makes perfect sense, but then we should probably update the comment before the handle_extension_v0() because it is stale. The new semantics is that any extension.* found in a config file that lacks core.repositoryformatversion will be ignored with the new code, right? If that is our intention, it should be documented.

The current code will not clear, so there is a change in behaviour. I do not know if we particularly care about this behaviour change, though.

> The reason we need to read all the extensions is that different config
> options aren't ordered ...

Yes, that is where my "somewhat questionable" comes from. We'd have to read everything in and then refrain from touching a repository with extensions that should not exist (i.e., the ones we do not understand, or the ones that should not be used with the stated format version) in verify_repository_format() as you said.

Thanks.