replay: replace the_repository with repo parameter passed to cmd_replay

[newren]

The point of this patch is not to remove USE_THE_REPOSITORY_VARIABLE; I can't yet because DEFAULT_ABBREV and get_commit_output_encoding() both require it and have no current alternatives. However, I still think it's worthwhile to stop using the_repository everywhere while ignoring the repo parameter explicitly passed in. That looks kinda ugly, and since I'm poking around in replay right now, I don't want to push the_repository in even more places when we have the appropriate value available -- especially since that might make my local work conflict should someone else come along and try to clean this up.

--color-words is handy when viewing this patch; it may make it easier to see the changes.

cc: Patrick Steinhardt ps@pks.im

[newren]

/submit

[gitgitgadget]

Submitted as pull.1921.git.1747254806067.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1921/newren/replay-repo-v1

To fetch this version to local tag pr-1921/newren/replay-repo-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1921/newren/replay-repo-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     require it and have no current alternatives. However, I still think it's
>     worthwhile to stop using the_repository everywhere while ignoring the
>     repo parameter explicitly passed in.

Sensible.

> diff --git a/builtin/replay.c b/builtin/replay.c
> index 032c172b65e..225cef08807 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -20,21 +20,22 @@
>  #include <oidset.h>
>  #include <tree.h>
>  
> -static const char *short_commit_name(struct commit *commit)
> +static const char *short_commit_name(struct repository *repo,
> +				     struct commit *commit)
>  {
> -	return repo_find_unique_abbrev(the_repository, &commit->object.oid,
> +	return repo_find_unique_abbrev(repo, &commit->object.oid,
>  				       DEFAULT_ABBREV);
>  }

I do not mind this, but I do have to wonder if it is simpler to make
the two callers of this "helper" (which is not quite helping anything)
to make these calls themselves.

>  int cmd_replay(int argc,
>  	       const char **argv,
>  	       const char *prefix,
> -	       struct repository *repo UNUSED)
> +	       struct repository *repo)
>  {
>  	const char *advance_name_opt = NULL;
>  	char *advance_name = NULL;
> @@ -329,7 +334,7 @@ int cmd_replay(int argc,
>  		    "--advance", "--contained");
>  	advance_name = xstrdup_or_null(advance_name_opt);
>  
> -	repo_init_revisions(the_repository, &revs, prefix);
> +	repo_init_revisions(repo, &revs, prefix);

OK, since this command is marked as RUN_SETUP, it is safe to
unconditionally use repo here.  The only situation where it is
called with repo==NULL is when somebody said "git replay -h" outside
a repository, which would have made parse_options() to do the right
thing and exited already without reaching this code, so we should be
able to trust "repo" to be usable.

Will queue.  Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@4a19d0c.

[gitgitgadget]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, May 14, 2025 at 08:33:25PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Replace the_repository everywhere with repo, feed repo from cmd_replay()
> to all the other functions in the file that need it, and remove the
> UNUSED annotation on repo.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     replay: replace the_repository with repo parameter passed to cmd_replay
>     
>     The point of this patch is not to remove USE_THE_REPOSITORY_VARIABLE; I
>     can't yet because DEFAULT_ABBREV and get_commit_output_encoding() both
>     require it and have no current alternatives. However, I still think it's
>     worthwhile to stop using the_repository everywhere while ignoring the
>     repo parameter explicitly passed in. That looks kinda ugly, and since
>     I'm poking around in replay right now, I don't want to push
>     the_repository in even more places when we have the appropriate value
>     available -- especially since that might make my local work conflict
>     should someone else come along and try to clean this up.

Makes sense. Especially the first one is something I have encountered as
a frequent blocker for removing `USE_THE_REPOSITORY_VARIABLE`. I guess
it would be nice to tackle it sooner rather than later.

Cc'd Ayush, as he will be working on the global state reduction project
as part of GSoC.

Patrick

[gitgitgadget]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget]

This branch is now known as en/replay-wo-the-repository.

[gitgitgadget]

This patch series was integrated into seen via git@5a8c154.

[gitgitgadget]

This patch series was integrated into seen via git@51aa248.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch en/replay-wo-the-repository on the Git mailing list:

The dependency on the_repository variable has been reduced from the
code paths in "git replay".

Will merge to 'next'.
source: <pull.1921.git.1747254806067.gitgitgadget@gmail.com>