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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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