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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 13, 2021, 19:13 UTC
Message-ID: <87a6kg46xh.fsf@evledraar.gmail.com>
In-Reply-To: <YT+SkhLsfE/yIqhN@coredump.intra.peff.net>

On Mon, Sep 13 2021, Jeff King wrote:

Show 17 quoted lines

> On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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).

*nod*

Show 25 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");

Yes, I had something like that in the earlier round, but decided to try to keep the diff minimal, churn and all that. So it would be nice, but let's do it as some later cleanup if someone's interested.

Show 8 quoted lines

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

*nod*, but can be left for some other time.

Aside: I did most of that removal of "argv" from the child_process
struct you suggested, it's in
avar/run-command-use-less-argv-use-args-instead in my repo if you're
curious / wanted to pick any of that up. I won't be submitting it any
time soon for the reasons noted upthread.

In any case it has rather large semantic and textual conflicts with the outstanding hook.[ch] topic, but with that branch + that applied + a few changes on top I was able to get the whole tree compiling & testing successfully earlier.