Re: [PATCH v2 3/4] difftool: use run_command() API in run_file_diff()

From: Jeff King <peff@peff.net>
Date: Sep 13, 2021, 18:04 UTC
Message-ID: <YT+SkhLsfE/yIqhN@coredump.intra.peff.net>
In-Reply-To: <patch-v2-3.4-2b093bd71fc-20210913T033204Z-avarab@gmail.com>

On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> Change the run_file_diff() function to use the run_command() API
> directly, instead of invoking the run_command_v_opt_cd_env() wrapper.
> 
> This allows it, like run_dir_diff(), to use the "args" from "struct
> strvec", instead of the "const char **argv" passed into
> cmd_difftool(). This will be used in the subsequent commit to get rid
> of OPT_ARGUMENT() from cmd_difftool().

It also fixes the existing leak of its "args" strvec.

I like the general direction here of putting the child_process in the parent. There's a few opportunities for further cleanup it opens, but I'm happy whether you want to pursue them or not (I'm also happy to do them as real patches on top myself, but don't want to de-rail your series).

Show 15 quoted lines

>  static int run_file_diff(int prompt, const char *prefix,
> -			 int argc, const char **argv)
> +			 struct child_process *child)
>  {
> -	struct strvec args = STRVEC_INIT;
>  	const char *env[] = {
>  		"GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL,
>  		NULL
>  	};
> -	int i;
>  
>  	if (prompt > 0)
>  		env[2] = "GIT_DIFFTOOL_PROMPT=true";
>  	else if (!prompt)
>  		env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";

This "save a NULL in env where we might stick something in later" is ugly. Now that we have a child_process, it might be nicer as:

  strvec_push(&env.args, "GIT_PAGER=");
  strvec_push(&env.args, "GIT_EXTERNAL_DIFF=git-difftool--helper");
  if (prompt > 0)
	strvec_push(&env.args, "GIT_DIFFTOOL_PROMPT=true");
  else if (!prompt)
	strvec_push(&env.args, "GIT_DIFFTOOL_NO_PROMPT=true");

> +	child->git_cmd = 1;
> +	child->dir = prefix;

These are the same in run_dir_diff(). We could do more of the shared prep of the child_process in the caller. Likewise, we might want to pick up run_dir_diff()'s no_stdin and clean_on_exit settings (and unsetting use_shell, but I think that is already pointless; it defaults to false in the first place, and git_cmd takes precedence over it anyway).

-Peff