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

On Sun, Nov 21, 2021 at 06:10:08PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

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

quoted

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.

Yikes. Thanks for a dose of sanity. I was looking too much at just the
pager tests.

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;
	}

This is a pretty horrid interface, in that the caller has to understand
which bits of "cp" need to be adjusted: setting cp.argv to NULL, but
also potentially cp.env (if cp.env_array was used), and clearing any
stdin/out/err descriptors created as pipes in the previous command. And
probably more; that's just off the top of my head.

But clearly there's a bunch of code relying on the current state of
affairs.

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.

Yep. But I do worry a bit about changing the interface in such a subtle
way, as nothing would catch topics in flight.

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

Forget 'seen'. Applying it on master shows plenty of breakages. :)

I think we should probably punt on this direction, and just make sure
that setup_pager() either reinitializes the child_process as appropriate
(as in the patch I showed in the earlier thread) or just refuses to try
running the pager twice (I didn't show a patch, but it should just be a
matter of setting a static flag).

-Peff