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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> 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.

> 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.

> 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.

> 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.

> @@ -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.