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

From: Jeff King <peff@peff.net>
Date: Dec 3, 2024, 21:24 UTC
Message-ID: <20241203212441.GA1424493@coredump.intra.peff.net>
In-Reply-To: <xmqqo71tcovl.fsf@gitster.g>

On Tue, Dec 03, 2024 at 02:33:02PM +0900, Junio C Hamano wrote:

Show 12 quoted lines

> >> It seems that this topic is waiting for a reroll?
> >
> > I think we could go either way. I outlined a few further possible steps,
> > but there is no need to hold up this first step. The only question is
> > whether or not to add a single test to show off and protect the
> > improvement.
> 
> Hmph, a few messages upthread
> <UZMh2lyzbLOgsf0PXfMnq6HnWVnCK3y36jY3IMKUykPi74ztNucf8bgywoeO0DdeApq31JDDGMZiEya99zAcI3l8y_zcVqiN8FpEnT1DRZU=@proton.me>
> (Ugh, why do some MUAs or mail providers use such an overly long
> message ID, Yuck) was where I got the impression that we were
> waiting for a reroll.

Yeah, I think Philip was offering to add some tests. Since he has been quiet since, I do not have a strong opinion on whether we should just take it as-is or let it go unless he comes back.

Show 6 quoted lines

> I am not all that convinced that sprinkling setup_diff_pager() call
> all over the place is a good idea from longer-term maintainability's
> sake to begin with, by the way.  What problem are we really solving?
> Folks who run "git diff --no-such-option" see "behaviour inconsistency"?
> All I see is "error: invalid option: --invalid" followed by a help message,
> which is quite expected.

I think it is just about not starting the pager when there is no useful output produced. Depending on your pager config, it is a little nicer if we avoid it for a one-liner error. E.g., I do not use "-F", so "git diff foo bar" drops me into the pager with a single error line.

-Peff