On Mon, Oct 21, 2024 at 12:11:33AM +0000, Philip Yung wrote:
> > errors will likewise go to the pager. E.g., try "git log --foo".
>
> Hope that I didn't take it the wrong way, but I don't think `git log --foo`
> starts a pager, where the routine `setup_pager()` is put after argv parsing.
> (checked by `strace`)
Hmm, this actually depends on config. If you have pager.log defined,
we'll start it early in git.c, but otherwise not until the setup_pager()
call.
I was mildly surprised that pager.diff would not have the same effect,
even with your patch. But that's because we only handle pager config if
RUN_SETUP is true, which it is not for diff (because we might be doing
an out-of-repo --index diff). And the reason for that is mostly
historical, as reading config early interferes with repo setup (though
I'm even sure that's still the case, as check_pager_config() these days
uses the "early" config mechanism which is supposed to address that).
What a horrid mess of inconsistency and hacks. ;)
Likewise, any builtin that sets USE_PAGER in git.c will turn on the
pager early. So "git shortlog --foo" will go through the pager, as will
range-diff. I was somewhat surprised those are the only two these days.
Looks like 1fda91b511 (Fix 'git log' early pager startup error case,
2010-08-24) dropped many. And I think your patch is the spiritual
successor to that.
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.
> > 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.
If we fixed some of the bits I mentioned above, some of that should
likewise be covered by tests.
-Peff
