Re: [PATCH] diff: setup pager only before diff contents truly ready

From: Jeff King <peff@peff.net>
Date: Oct 19, 2024, 21:19 UTC
Message-ID: <20241019211938.GA589728@coredump.intra.peff.net>
In-Reply-To: <pull.1817.git.git.1729370390416.gitgitgadget@gmail.com>

On Sat, Oct 19, 2024 at 08:39:50PM +0000, Y5 via GitGitGadget wrote:

> git-diff setups pager at an early stage in cmd_diff; running diff with
> invalid options like git diff --invalid will unexpectedly starts a
> pager, which causes behavior inconsistency.

I do think the outcome is a little nicer for the user, but I'd hesitate to call it more inconsistent. Most of the rest of Git is setting up the pager in git.c, before we even call into the builtin. So any early errors will likewise go to the pager. E.g., try "git log --foo".

So I dunno. I'm not strictly opposed to making things nicer when we can do so easily. But the endgame of this is probably getting rid of USE_PAGER entirely and asking each builtin to decide when to commit to using the pager (presumably after option parsing).

And even then, it wouldn't apply to commands implemented as an external process. And of course we can still die(), etc, after starting the pager. So it would never be totally consistent.

> Signed-off-by: y5c4l3 <y5c4l3@proton.me>

We usually ask for something approaching a legal name, as this sign-off is supposed to be certifying the DCO (see the "dco" section in Documentation/SubmittingPatches).

>  builtin/diff.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

The patch itself looks plausibly correct. The biggest regression risk would be missing a spot that needed a new setup_diff_pager() call, and I suspect we don't have good test coverage here. But going top-down from builtin_diff(), I don't see any paths you missed.

-Peff