Re: [PATCH v2 3/8] setup: deduplicate logic to apply repository format

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jun 03, 2026 at 05:43:34AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/repository.c b/repository.c
> > index db57b8308b..58a13f7c4f 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -262,8 +262,8 @@ void repo_set_worktree(struct repository *repo, const char *path)
> >  	trace2_def_repo(repo);
> >  }
> >
> > -static int read_and_verify_repository_format(struct repository_format *format,
> > -					     const char *commondir)
> > +static int read_repository_format_from_commondir(struct repository_format *format,
> > +						 const char *commondir)
> 
> Nit: The commit explicitly calls out one rename, but this one wasn't.

Fair. I'll add a sentence or two about this.

> > @@ -272,11 +272,6 @@ static int read_and_verify_repository_format(struct repository_format *format,
> >  	read_repository_format(format, sb.buf);
> >  	strbuf_reset(&sb);
> >
> > -	if (verify_repository_format(format, &sb) < 0) {
> > -		warning("%s", sb.buf);
> > -		ret = -1;
> > -	}
> > -
> 
> So we remove this, so that the callee would independently verify the
> format I assume.
> 
> Edit: seems like we call verify_repository_format() within
> apply_repository_format() and the latter is called by the callee.
> 
> >  	strbuf_release(&sb);
> >  	return ret;
> >  }

Yeah. I guess this could be explained a bit better.

> > @@ -290,6 +285,8 @@ int repo_init(struct repository *repo,
> >  	      const char *worktree)
> >  {
> >  	struct repository_format format = REPOSITORY_FORMAT_INIT;
> > +	struct strbuf err = STRBUF_INIT;
> > +
> >  	memset(repo, 0, sizeof(*repo));
> >
> >  	initialize_repository(repo);
> > @@ -297,21 +294,13 @@ int repo_init(struct repository *repo,
> >  	if (repo_init_gitdir(repo, gitdir))
> >  		goto error;
> >
> > -	if (read_and_verify_repository_format(&format, repo->commondir))
> > +	if (read_repository_format_from_commondir(&format, repo->commondir))
> >  		goto error;
> >
> > -	repo_set_hash_algo(repo, format.hash_algo);
> > -	repo_set_compat_hash_algo(repo, format.compat_hash_algo);
> > -	repo_set_ref_storage_format(repo, format.ref_storage_format,
> > -				    format.ref_storage_payload);
> > -	repo->repository_format_worktree_config = format.worktree_config;
> > -	repo->repository_format_relative_worktrees = format.relative_worktrees;
> > -	repo->repository_format_precious_objects = format.precious_objects;
> > -	repo->repository_format_submodule_path_cfg = format.submodule_path_cfg;
> > -
> > -	/* take ownership of format.partial_clone */
> 
> I see that we now do an xstrdup for format.partial_clone, meaning we
> have our own memory segment to care about. Do we have to worry about
> format.partial_clone not being free'd?

No, `clear_repository_format()` already releases the memory for us. It
also did beforehand, but there we did the dance of just moving ownership
over. So we already had to free the string before.

> > diff --git a/setup.h b/setup.h
> > index 9409326fe4..5ed92f53fa 100644
> > --- a/setup.h
> > +++ b/setup.h
> > @@ -221,6 +221,15 @@ void clear_repository_format(struct repository_format *format);
> >  int verify_repository_format(const struct repository_format *format,
> >  			     struct strbuf *err);
> >
> > +/*
> > + * Apply the given repository format to the repo. This initializes extensions
> > + * and basic data structures required for normal operation. Returns 0 on
> > + * success, a negative error code otherwise.
> > + */
> 
> Nit: perhaps we should also mention that we verify the format?

Will do.

Patrick