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

[Derrick Stolee <stolee@gmail.com>]

On 11/6/2026 8:26 PM, 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.
> 
> 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.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * I wrote this change when I ranted in
> 
>    https://lore.kernel.org/git/xmqq33tdcuub.fsf@gitster.g/
> 
>    on 'seen', and "git checkout -m --detach master" was sufficient
>    to build and start a new branch based on 'master' and there
>    wasn't any need for adjustment.  All tests seem to pass.
> 
>  unpack-trees.c | 94 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 18 deletions(-)
> 
> 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;
Overall, this is a good helper implementation.

> +	if (!repo)
> +		BUG("no repository for unpack_trees()?");
> +	return repo;
> +
> +}

nit: extra empty line.

>  static const char *super_prefixed(const char *path, const char *super_prefix)
>  {
>  	/*
> @@ -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);
>  
>  	if (pos >= 0) {
>  		struct cache_entry *ce = index->cache[pos];
>  		if (!state && ce->ce_flags & CE_WT_REMOVE) {
> -			repo_read_gitmodules(the_repository, 0);
> +			repo_read_gitmodules(repo, 0);
>  		} else if (state && (ce->ce_flags & CE_UPDATE)) {
> -			submodule_free(the_repository);
> +			submodule_free(repo);
>  			checkout_entry(ce, state, NULL, NULL);
> -			repo_read_gitmodules(the_repository, 0);
> +			repo_read_gitmodules(repo, 0);
>  		}
>  	}

This sample of how it is used makes it clear how this is a
valuable abstraction.

Thanks,
-Stolee