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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 21, 2024 at 03:00:45PM -0400, Jeff King wrote:
> So I think in an ideal world we'd:
>
>   - convert those two commands to do the pager setup themselves and
>     retire the USE_PAGER flag entirely
>
>   - move configured pager handling down into more commands. So git-log
>     should set DELAY_PAGER_CONFIG and then call setup_auto_pager()
>     rather than setup_pager(). Ideally DELAY_PAGER_CONFIG would be the
>     default, but we can't do that until every builtin makes its own call
>     to setup_auto_pager() at the right moment.
>
>   - push calls to setup_pager() (or setup_auto_pager()) as far down
>     within commands as possible (right before we start generating
>     output). Your patch does that for git-diff, but there may be other
>     cases.
>
>   - consistently handle pager config whether USE_SETUP is set or not.
>     That means git-diff should set DELAY_PAGER_CONFIG, since it handles
>     the pager itself.
>
> And that would make things more consistent overall, and avoid pushing
> early errors into the pager (though of course it would still be possible
> to get some errors in the pager if they happen after we start it).
>
> I don't blame you if you don't want to start down that rabbit hole. :) I
> think it would probably be OK to peck away at it incrementally, and your
> patch does that.

Nicely put. I think what's nice about the patch here is that it starts
us down that direction you outlined above, so we'd want it regardless of
how much of the rest of the work Y5 is willing to do.

> > > would be missing a spot that needed a new setup_diff_pager() call, and I
> > > suspect we don't have good test coverage here.
> >
> > This is actually my concern as well when I was naively testing the coverage
> > using GDB, which turned out to be quite tedious. Would you consider it's fine to
> > add a pager consistency test for builtins, probably in another patch with regard
> > to `t7006-pager.sh` OR a new test `t7007`?
>
> TBH, I am not all that worried about adding tests just for your patch.
> You'd need to identify all of the possible diff code paths in order to
> add tests for them, which is the same thing you had to do to fix the
> code paths. I was mostly just commenting that we're not likely to be
> able to rely on existing tests to help us here.
>
> It might be worth adding a test that shows off your improved diff
> behavior, though I would be OK if it was a representative command and
> not exhaustive. I think adding to t7006 should be fine.

Agreed.

Thanks,
Taylor