On Thu, Jul 03, 2025 at 08:24:01AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > On Thu, Jul 03, 2025 at 12:54:24PM +0200, Karthik Nayak wrote:
> >> Patrick Steinhardt <ps@xxxxxx> 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.
I'd prefer to leave it out of this patch series. It's going to be a bit
more involved than just switching out the values and adding the `else`
branch for the hash, as well. The repository format code (or rather all
of "setup.c") is a can of worms that I don't really want to open right
now.
Patrick
