Re: [PATCH 1/2] unpack-trees: lose USE_THE_REPOSITORY

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 06, 2026 at 05:26:52PM -0800, Junio C Hamano wrote:
> This file defines a set of fairly low level functions to implement
> merge and checkout behaviour, many of which depended on and can only
> work with the_repository.
> 
> Two key data structures passed around in the code paths, "struct
> unpack_trees_options" and "struct index_state", both have a pointer
> to the repository instance, and we can use it to make it work on an
> arbitrary repository, not just the_repository.

Okay.

> In any correctly written clients of this library file, the
> repository instance that is pointed at by these two structs MUST be
> the same one.  For now, make sure that is the case (and otherwise
> BUG out) by introducing a unpack_repo() helper function that takes
> these two structures (when available) and returns the repository.

Fair. It's a bit curious of an interface to have this kind of ambiguity.
But as an intermediate step I think the proposed solution is okayish,
and I do like that we at least verify that it's the same repository.

> diff --git a/unpack-trees.c b/unpack-trees.c
> index d6a859420f..79341d7fa6 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>  #define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "git-compat-util.h"
> @@ -82,6 +81,56 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
>  	  ? ((o)->internal.msgs[(type)])      \
>  	  : (unpack_plumbing_errors[(type)]) )
>  
> +
> +/*
> + * Some functions in these code paths get unpack_trees_options, which
> + * has up to two istates, and some get an istate.  These istates can
> + * be used to find the repository to be used for this operation.
> + *
> + * As these are all working on the same repository, they must match.
> + * For now, with a super paranoid check, make sure all potential
> + * sources of the information match before using it.
> + *
> + * Once this proves working well, we could just use the first one we
> + * find in a production setting.
> + */
> +static struct repository *unpack_repo(struct index_state *istate,
> +				      struct unpack_trees_options *o)
> +{
> +	struct repository *repo = NULL;
> +	const char *camefrom = NULL;
> +
> +	if (istate) {
> +		if (!istate->repo)
> +			BUG("istate->repo == NULL");
> +		repo = istate->repo;
> +		camefrom = "istate->repo";
> +	}
> +	if (o) {
> +		if (o->src_index) {
> +			if (!o->src_index->repo)
> +				BUG("o->src_index->repo == NULL");
> +			if (repo && repo != o->src_index->repo)
> +				BUG("%s != o->src_index->repo", camefrom);
> +			repo = o->src_index->repo;
> +			camefrom = "o->src_index->repo";
> +		}
> +
> +		if (o->dst_index) {
> +			if (!o->dst_index->repo)
> +				BUG("o->dst_index->repo == NULL");
> +			if (repo && repo != o->dst_index->repo)
> +				BUG("%s != o->dst_index->repo", camefrom);
> +			repo = o->dst_index->repo;
> +		}
> +	}
> +	if (!repo)
> +		BUG("no repository for unpack_trees()?");
> +	return repo;
> +
> +}

So this overall surprisingly complex, but I think it's sensible to be
defensive.

> +
> +

Nit: two empty lines.

> @@ -345,15 +394,16 @@ static void load_gitmodules_file(struct index_state *index,
>  				 struct checkout *state)
>  {
>  	int pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));
> +	struct repository *repo = unpack_repo(index, NULL);

This conversion here raises the question what we want to do in the case
where the caller has both structures available, but we don't end up
passing them to the pointer. This could in theory hide a bug where we
have mismatching repositories when the entry points don't verify that
property.

I guess that's ultimately not much of a problem. But an alternative take
could ensure the propert only at the borders of this subsystem, so any
externally-visible function. And internally, we'd then only ever pass
around a repository.

But in the end I don't really think this matters much, so please only
take this as me thinking out loud.

Patrick