"brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> writes:
>> > - 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?
> 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.
