Re: [PATCH 1/2] difftool: use "struct strvec" API in run_{dir,file}_diff()

[Jeff King <peff@peff.net>]

On Sat, Sep 11, 2021 at 08:21:11PM +0200, Ævar Arnfjörð Bjarmason wrote:

> The underlying run_command() API can take either the "struct strvec
> args", or a "const char **argv". Let's move to the former to use the
> more "native" version of run_command() in both of these functions.

It sounds like we're moving to use child.args (the strvec interface)
instead of child.argv (the const char one). Which I support; I'd like to
eventually get rid of the argv interface entirely because it has
memory-ownership semantics that are easy to get wrong.

But this...

> @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  	child.clean_on_exit = 1;
>  	child.dir = prefix;
>  	child.out = -1;
> -	strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
> -		     NULL);
> -	for (i = 0; i < argc; i++)
> -		strvec_push(&child.args, argv[i]);
> +	child.argv = args->v;
> +

...is going in the opposite direction.

I'd much rather see us continue to use child.args here, like:

  strvec_pushv(&child.args, args->v);

Though really I do think passing the strvec into run_dir_diff() is
questionable in the first place. The caller depends on us to free the
memory in the strvec for them, which is...subtle.

It does let you immediately return here:

>  	if (dir_diff)
> -		return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
> -	return run_file_diff(prompt, prefix, argc, argv);
> +		return run_dir_diff(extcmd, symlinks, prefix, &args);
> +	return run_file_diff(prompt, prefix, &args);

without doing any cleanup. But I think just:

  if (dir_diff)
	ret = run_dir_diff(...);
  else
	ret = run_file_diff(...);

  strvec_clear(&args);
  return ret;

would be a lot more obvious.

I almost suggested that this could be done even simpler by having the
caller pass in one of two pre-made argv lists (in addition to what's in
the original argv). But the _actual_ benefit you want here is making it
easy to construct those lists in the shared code of the caller. Your
commit message didn't at all make that clear (so to me it looked like
your "cleanup" was actively making things worse).

It only becomes apparent with the second patch. I would have found it
much easier to understand with something like the patch below. And then
a further patch to use strvec_pushv instead of manually looping (even
getting rid of the argc parameters entirely!), and one to convert
run_file_diff() to use a struct child_process (which fixes its memory
leak).


-- >8 --
difftool: prepare "diff" cmdline in cmd_difftool()

We call into either run_dir_diff() or run_file_diff(), each of which
sets up a child argv starting with "diff" and some hard-coded options
(depending on which mode we're using). Let's extract that logic into the
caller, which will make it easier to modify the options for cases which
affect both functions.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/difftool.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 6a9242a803..91a8e51b0c 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -393,8 +393,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	child.clean_on_exit = 1;
 	child.dir = prefix;
 	child.out = -1;
-	strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
-		     NULL);
 	for (i = 0; i < argc; i++)
 		strvec_push(&child.args, argv[i]);
 	if (start_command(&child))
@@ -683,7 +681,6 @@ static int run_file_diff(int prompt, const char *prefix,
 		env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";
 
 
-	strvec_push(&args, "diff");
 	for (i = 0; i < argc; i++)
 		strvec_push(&args, argv[i]);
 	return run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env);
@@ -719,6 +716,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
 		OPT_END()
 	};
+	struct strvec args = STRVEC_INIT;
 
 	git_config(difftool_config, NULL);
 	symlinks = has_symlinks;
@@ -768,7 +766,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	 * will invoke a separate instance of 'git-difftool--helper' for
 	 * each file that changed.
 	 */
+	strvec_push(&args, "diff");
+	if (dir_diff)
+		strvec_pushl(&args, "--raw", "--no-abbrev", "-z", NULL);
+	strvec_pushv(&args, argv);
+
 	if (dir_diff)
-		return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
-	return run_file_diff(prompt, prefix, argc, argv);
+		return run_dir_diff(extcmd, symlinks, prefix, args.nr, args.v);
+	return run_file_diff(prompt, prefix, args.nr, args.v);
 }
-- 
2.33.0.811.g40f7f3a89c