Re: [PATCH 3/4] run-command: move envvar-resetting function

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Jun 01, 2021 at 02:34:18PM -0700, Jonathan Tan wrote:
> 
> There is a function that resets environment variables, used when
> invoking a sub-process in a submodule. The lazy-fetching code (used in
> partial clones) will need this function in a subsequent commit, so move
> it to a more central location.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  run-command.c | 10 ++++++++++
>  run-command.h |  7 +++++++
>  submodule.c   | 14 ++------------
>  3 files changed, 19 insertions(+), 12 deletions(-)
> diff --git a/run-command.h b/run-command.h
> index d08414a92e..6f61ec7703 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -483,4 +483,11 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
>  			       task_finished_fn, void *pp_cb,
>  			       const char *tr2_category, const char *tr2_label);
>  
> +/**
> + * Convenience function that adds entries to env_array that resets all
> + * repo-specific environment variables except for CONFIG_DATA_ENVIRONMENT. See
> + * local_repo_env in cache.h for more information.
> + */
> +void prepare_other_repo_env(struct strvec *env_array);

I like the name change on the function as well.

>  void prepare_submodule_repo_env(struct strvec *out)
>  {
> -	prepare_submodule_repo_env_no_git_dir(out);
> +	prepare_other_repo_env(out);
>  	strvec_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
>  		     DEFAULT_GIT_DIR_ENVIRONMENT);
>  }
>  
>  static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)
>  {
> -	prepare_submodule_repo_env_no_git_dir(out);
> +	prepare_other_repo_env(out);
>  	strvec_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
>  }

This call was used in less places than I thought (I guess that's part of
why you're making it more public/central), so my worry about having some
large scale change was for nothing.

As for Taylor's comment about the CONFIG_DATA_ENVIRONMENT variable, it
was named like that before you got here, so I am not too worried whether
or not you change it.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>