Re: [PATCH v2 1/2] BreakingChanges: announce switch to "reftable" format

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 3, 2025, 12:24 UTC
Message-ID: <CAOLa=ZQUp6S3yQ-zCfPxCvwoJeiPdXh5U9mojH+9-P1RAAa2gg@mail.gmail.com>
In-Reply-To: <aGZslJSks2GF3uB7@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 27 quoted lines

> On Thu, Jul 03, 2025 at 12:54:24PM +0200, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > diff --git a/setup.c b/setup.c
>> > index f93bd6a24a5..f0c06c655a9 100644
>> > --- a/setup.c
>> > +++ b/setup.c
>> > @@ -2541,6 +2541,8 @@ static void repository_format_configure(struct repository_format *repo_fmt,
>> >  			repo_fmt->ref_storage_format = ref_format;
>> >  	} else if (cfg.ref_format != REF_STORAGE_FORMAT_UNKNOWN) {
>> >  		repo_fmt->ref_storage_format = cfg.ref_format;
>> > +	} else {
>> > +		repo_fmt->ref_storage_format = REF_STORAGE_FORMAT_DEFAULT;
>> >  	}
>> >  	repo_set_ref_storage_format(the_repository, repo_fmt->ref_storage_format);
>> >  }
>>
>> Shouldn't this change be instead made to REPOSITORY_FORMAT_INIT?
>
> It made me a bit uneasy to change `REPOSITORY_FORMAT_INIT` as it is used
> in several places. So I opted for the more contained change.
>
> In any case, I found the logic to be hard to follow anyway as it is not
> immediately clear where the default value actually comes from without
> the `else` branch. So I consider it a good change regardless. In fact, I
> would argue we could go even further and change `REPOSITORY_FORMAT_INIT`
> to be set to `_UNKNOWN`. Same for the hash.
>

Exactly, I just read your patch and the existing code around it and was a bit confused because I couldn't pinpoint where we set the default to '_FILES' when there is no ENV or config setup.

I think changing `REPOSITORY_FORMAT_INIT` to be set to `_UNKNOWN` makes a lot of sense combined with your change. I'll leave it to you if you want to include that in this series or not.

> Patrick

Thanks!