Re: [PATCH v2] pager: fix crash when pager program doesn't exist

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2021, 02:10 UTC
Message-ID: <xmqqfsrplz3z.fsf@gitster.g>
In-Reply-To: <YZqSBlvzz2KgOMnJ@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> We'd usually leave of "Reviewed-by" until the reviewer has had a chance
> to see _this_ version of the patch. I.e., usually it would not be added
> by the submitter, but by the maintainer (unless you are resending
> verbatim a patch that already got review).
>
>> diff --git a/run-command.c b/run-command.c
>> index f329391154ae..a7bf81025afb 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)
>>  {
>>  	strvec_clear(&child->args);
>>  	strvec_clear(&child->env_array);
>> +	child_process_init(child);
>>  }
>
> And naturally I agree that the patch itself looks good. :)

Well, not to me X-<. This is way too aggressive a change to be made lightly without auditing the current users of run_command API.

It is rather common for us to reuse "struct child_process" in a code path, e.g. builtin/worktree.c::add_worktree() prepares a single instance of such a struct, sets cp.git_cmd to true, and runs either "update-ref" or "symbolic-ref" to update "HEAD". After successful invocation of such a git subcommand, it then runs "git reset --hard", with this piece of code:

	cp.git_cmd = 1;

	if (!is_branch)
		strvec_pushl(&cp.args, "update-ref", "HEAD",
			     oid_to_hex(&commit->object.oid), NULL);
	else {
		strvec_pushl(&cp.args, "symbolic-ref", "HEAD",
			     symref.buf, NULL);
		if (opts->quiet)
			strvec_push(&cp.args, "--quiet");
	}

	cp.env = child_env.v;
	ret = run_command(&cp);
	if (ret)
		goto done;

	if (opts->checkout) {
		cp.argv = NULL;
		strvec_clear(&cp.args);
		strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
		if (opts->quiet)
			strvec_push(&cp.args, "--quiet");
		cp.env = child_env.v;
		ret = run_command(&cp);
		if (ret)
			goto done;
	}

Now, we could argue that this existing caller is too lazy to assume that cp.git_cmd bit will be retained after run_command() successfully finishes and can reuse the structure without setting the bit again, and it should be more defensive. And "successful run_command() clears the child process so that you'll get a clean slate" may even be a better API in the longer term.

But then a change like this one that changes the world order to make it a better place is also responsible to ensure that the callers are already following the new world order.

When merged to 'seen', this literally destroys tons of tests (the first and easiest one to observe may be t0002).

Thanks.