Re: [PATCH 5/8] replace and remove run_command_v_opt_cd_env()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 27, 2022, 20:16 UTC
Message-ID: <221027.864jvpau2d.gmgdl@evledraar.gmail.com>
In-Reply-To: <e72a1abe-3098-e4fb-f064-a8b5c8f14239@web.de>

On Thu, Oct 27 2022, René Scharfe wrote:

> run_command_v_opt_cd_env() is only used in an example in a comment.

Is it? I could have sworn that....

....ah, it is used be before this series, but whereas I arranged it as:

 1. Remove run_command_v_opt_cd_env() and its last user:
    https://lore.kernel.org/git/patch-v2-07.10-3b3d3777232-20221017T170316Z-avarab@gmail.com/
 2. Remove run_command_v_opt_tr2() and its last user:
    https://lore.kernel.org/git/patch-v2-08.10-4f1a051823f-20221017T170316Z-avarab@gmail.com/

You are:

 1. In your 3/8 you rewrite a bunch of callers, and one of those is the
    odd one out in 3/8 using run_command_v_opt_cd_env().
 2. There's an in-between unrelated cleanup in 4/8
 3. This 5/8 commit, removing the now-stale run_command_v_opt_cd_env().
 4. A nicely atomic 6/8 removing run_command_v_opt_tr2() and its last caller
 5. Your 7/8 has the run_command_v_opt_cd_env_tr2(), but unlike this
    commit you didn't bundle up the "Use [...it...] directly" like here.

Anyway, I find the end state to be mostly OK, but FWIW I wouldn't mind if this were a bit less confusing.

You seem to have ended up with this because of grouping the '"args" and "env" directly' callers together.

FWIW I'd be fine with just squashing most of this together, it's already a ~200 line commit, the commit message could just call out these exceptions.

*Or* do it more incrementally, but then the choice of not doing the last callers of the odd ones out atomically seems a bit weird....

Anyway, if you want to just keep it as it is I'm also fine with it. I mainly wrote the above while narrating the state of this to myself, to see if there were any lurking issues...

Show 73 quoted lines

> Use
> the struct child_process member "env" and run_command() directly instead
> and then remove the unused convenience function.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  run-command.c | 7 +------
>  run-command.h | 4 +---
>  tmp-objdir.h  | 4 ++--
>  3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 5ec3a46dcc..1c9ed510f8 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1006,7 +1006,7 @@ int run_command(struct child_process *cmd)
>
>  int run_command_v_opt(const char **argv, int opt)
>  {
> -	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
> +	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
>  }
>
>  int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
> @@ -1014,11 +1014,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
>  	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
>  }
>
> -int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
> -{
> -	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
> -}
> -
>  int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
>  				 const char *const *env, const char *tr2_class)
>  {
> diff --git a/run-command.h b/run-command.h
> index 0e85e5846a..a5e210fd1a 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -151,8 +151,7 @@ struct child_process {
>
>  /**
>   * The functions: child_process_init, start_command, finish_command,
> - * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear
> - * do the following:
> + * run_command, run_command_v_opt, child_process_clear do the following:
>   *
>   * - If a system call failed, errno is set and -1 is returned. A diagnostic
>   *   is printed.
> @@ -250,7 +249,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
>   * env (the environment) is to be formatted like environ: "VAR=VALUE".
>   * To unset an environment variable use just "VAR".
>   */
> -int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
>  int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
>  				 const char *const *env, const char *tr2_class);
>
> diff --git a/tmp-objdir.h b/tmp-objdir.h
> index 76efc7edee..615b188573 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -11,8 +11,8 @@
>   * Example:
>   *
>   *	struct tmp_objdir *t = tmp_objdir_create("incoming");
> - *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
> - *	    !tmp_objdir_migrate(t))
> + *	strvec_pushv(&cmd.env, tmp_objdir_env(t));
> + *	if (!run_command(&cmd)) && !tmp_objdir_migrate(t))
>   *		printf("success!\n");
>   *	else
>   *		die("failed...tmp_objdir will clean up for us");