Re: [PATCH 2/3] setup: fix reinit of repos with incompatible GIT_DEFAULT_REF_FORMAT

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 3, 2025, 05:29 UTC
Message-ID: <Z6BUOs7k1rzGOGTN@pks.im>
In-Reply-To: <xmqqv7tu623n.fsf@gitster.g>

On Fri, Jan 31, 2025 at 02:38:20PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Instead of retaining the current ref format, the reinitialization tries
> > to reinitialize the repository with the different format. This action
> > fails when git-init(1) tries to write the ".git/refs/heads" stub, which
> > in the context of the reftable backend is always written as a file so
> > that we can detect clients which inadvertently try to access the repo
> > with the wrong ref format. Seems like the protection mechanism works for
> > this case, as well.
> >
> > Fix the issue by ignoring the environment variable in case the repo has
> > already been initialized with a ref storage format.
> 
> It certainly is better than corrupting the repository, but if we are
> to do this change, shouldn't we at least issue a warning to tell
> users that (a part of) their request was ignored, instead of
> silently ignoring the specified ref-format?

I don't think we should. If this was passed on the command line then yes, we should flag this and already die indeed. But this is an environment variable that allows you to set the default format. From my point of view it is totally expected that this doesn't cause the format of existing repositories to change.

Show 30 quoted lines

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  setup.c         | 4 +++-
> >  t/t0001-init.sh | 9 +++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/setup.c b/setup.c
> > index 8a488f3e7c..53ffeabc5b 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -2534,7 +2534,9 @@ static void repository_format_configure(struct repository_format *repo_fmt,
> >  		ref_format = ref_storage_format_by_name(env);
> >  		if (ref_format == REF_STORAGE_FORMAT_UNKNOWN)
> >  			die(_("unknown ref storage format '%s'"), env);
> > -		repo_fmt->ref_storage_format = ref_format;
> > +		if (repo_fmt->version < 0 ||
> > +		    repo_fmt->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
> > +			repo_fmt->ref_storage_format = ref_format;
> 
> Perhaps something silly like this?
> 
> 		if (0 <= repo_fmt->version &&
> 		    repo_fmt->ref_storage_format != REF_STORAGE_FORMAT_UNKNOWN)
> 			warning("ignoring the specified ref-format");
> 		else
> 			repo_fmt->ref_storage_format = ref_format;
> 
> In the longer term, we might want to consider automatically
> migrating the ref backend (by calling into "git ref migrate"),
> but it is a good first move to stop damaging the repository.

I think keeping migrations explicit is worthwhile. Migrations are a somewhat risky thing, so explicitly making the user ask for them is not a bad thing. I personally wouldn't expect git-init(1) to migrate data. After all, it is supposed to initialize stuff, not rewrite it.

This is doubly true for environment variables, where it is so extremely easy to accidentally still have them defined. I don't think implicitly converting every git-init(1) to do migrations would be a good idea there as it would likely do the wrong thing in many cases.

So from my point of view we should treat the environment variables the same as we treat "init.defaultRefFormat" and "init.defaultObjectFormat". Those indicate defaults, but do not cause us to change the format of existing repostiories.

Patrick