Re: [PATCH 2/4] run-command: use repo_start_command() in strict callers

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2026, 19:26 UTC
Message-ID: <xmqqsea6np5z.fsf@gitster.g>
In-Reply-To: <20260311151923.4178655-3-bkkaracay@gmail.com>

Burak Kaan Karaçay <bkkaracay@gmail.com> writes:

Show 38 quoted lines

> Some callers have been freed from global state and they do not define
> the 'USE_THE_REPOSITORY_VARIABLE' macro.
>
> To complete the mitigation of 'start_command()', update these callers to
> use repo_start_command() and pass their local 'struct repository' as an
> argument, completely eliminating their hidden reliance on the global
> state.
>
> Signed-off-by: Burak Kaan Karaçay <bkkaracay@gmail.com>
> ---
>  builtin/difftool.c | 4 ++--
>  odb.c              | 2 +-
>  pager.c            | 2 +-
>  repack-promisor.c  | 2 +-
>  send-pack.c        | 4 ++--
>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index e4bc1f8316..15ac552edf 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -257,7 +257,7 @@ static void changed_files(struct repository *repo,
>  	diff_files.out = -1;
>  	diff_files.dir = workdir;
>  	strvec_pushf(&diff_files.env, "GIT_INDEX_FILE=%s", index_path);
> -	if (start_command(&diff_files))
> +	if (repo_start_command(repo, &diff_files))
>  		die("could not obtain raw diff");
>  	fp = xfdopen(diff_files.out, "r");
>  	while (!strbuf_getline_nul(&buf, fp)) {
> @@ -437,7 +437,7 @@ static int run_dir_diff(struct repository *repo,
>  	child->clean_on_exit = 1;
>  	child->dir = prefix;
>  	child->out = -1;
> -	if (start_command(child))
> +	if (repo_start_command(repo, child))
>  		die("could not obtain raw diff");
>  	fp = xfdopen(child->out, "r");

Up to [1/4], we called start_command(), running the new process in the context of the_repository, and if these "repo" were referring to a repository different from the_repository, we risk changing the behaviour without meaning to do so.

The difftool command, however, would not be dealing with multiple repositories so it is very likely that the "repo" passed to these two functions are the_repository _anyway_, so these changes are correct, safe, and a move in the right direction.

Show 13 quoted lines

> diff --git a/odb.c b/odb.c
> index 776de5356c..8ec279f84e 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -535,7 +535,7 @@ static void read_alternate_refs(struct repository *repo,
>  
>  	fill_alternate_refs_command(repo, &cmd, path);
>  
> -	if (start_command(&cmd))
> +	if (repo_start_command(repo, &cmd))
>  		return;
>  
>  	fh = xfdopen(cmd.out, "r");

The only semantic change brought in by [1/4] to start_command() is that the object store of the named repository is closed, instead of the object store of the_repository. The read_alternate_refs() function calls fill_alternate_refs_command() to run for-each-ref in the named "repo" to find out the refs _they_ have.

But then, do we really want to close the object database we have been using in the context of that "repo", that is different from "the_repository" we have been using so far?

Of course, there is no guarantee that the_repository is always the "current" repository object we are using and trying to read the refs from this alternate repository in the codebase in an imaginary future where we can start from one repository, add refs from an alternate repository, and while doing so, we may add refs from an alternate of the alternate repository, so always starting from the_repository may not be correct, either. But even before going there, closing object store of "repo" feels more wrong than from "the_repository". Perhaps repo_start_command() needs to be aware of two repositories, one that we have been using and neeed to close the object store if instructed, and the other one that we want to launch the new process in its context? Keeping "the current repository" as a global variable will drag us back to the similar issues these efforts to wean us from "the_repository" global are trying to address, so if our new repo_start_command() does need to be aware of the two repositories, perhaps it needs to take two repository parameters (i.e., where we are coming from, and where we are going to)? I dunno.

Show 13 quoted lines

> diff --git a/pager.c b/pager.c
> index 5531fff50e..9a23ed958d 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -169,7 +169,7 @@ void setup_pager(struct repository *r)
>  	prepare_pager_args(&pager_process, pager);
>  	pager_process.in = -1;
>  	strvec_push(&pager_process.env, "GIT_PAGER_IN_USE");
> -	if (start_command(&pager_process))
> +	if (repo_start_command(r, &pager_process))
>  		die("unable to execute pager '%s'", pager);
>  
>  	/* original process continues, but writes to the pipe */

OK.

Show 13 quoted lines

> diff --git a/repack-promisor.c b/repack-promisor.c
> index 90318ce150..dba161a11a 100644
> --- a/repack-promisor.c
> +++ b/repack-promisor.c
> @@ -125,7 +125,7 @@ void pack_geometry_repack_promisors(struct repository *repo,
>  	prepare_pack_objects(&cmd, args, packtmp);
>  	strvec_push(&cmd.args, "--stdin-packs");
>  	cmd.in = -1;
> -	if (start_command(&cmd))
> +	if (repo_start_command(repo, &cmd))
>  		die(_("could not start pack-objects to repack promisor packs"));
>  
>  	in = xfdopen(cmd.in, "w");

OK.

Show 11 quoted lines

> diff --git a/send-pack.c b/send-pack.c
> index 67d6987b1c..c339c3d1ca 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -92,7 +92,7 @@ static int pack_objects(struct repository *r,
>  	po.out = args->stateless_rpc ? -1 : fd;
>  	po.git_cmd = 1;
>  	po.clean_on_exit = 1;
> -	if (start_command(&po))
> +	if (repo_start_command(r, &po))
>  		die_errno("git pack-objects failed");

OK.

Show 9 quoted lines

> @@ -459,7 +459,7 @@ static void get_commons_through_negotiation(struct repository *r,
>  		return;
>  	}
>  
> -	if (start_command(&child))
> +	if (repo_start_command(r, &child))
>  		die(_("send-pack: unable to fork off fetch subprocess"));
>  
>  	do {

OK.