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

Jeff King [off-list ref] writes:

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

quoted

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

quoted

@@ -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.
...
+	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);
 }

Yes, I have to say that the end result of not having to rely on the
strvec type, in order to just call a main()- like function, makes it
much more pleasant read.