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

[Jeff King <peff@peff.net>]

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