Re: To page or not to page

From: Jeff King <peff@peff.net>
Date: May 5, 2008, 21:59 UTC
Message-ID: <20080505215924.GA9228@sigill.intra.peff.net>
In-Reply-To: <7v1w4ky3hh.fsf@gitster.siamese.dyndns.org>

On Fri, May 02, 2008 at 11:18:02AM -0700, Junio C Hamano wrote:

Show 28 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > My bigger worry is that this affects only builtins. Which makes it
> > sufficient for turning off the pager for anything that does USE_PAGER.
> 
> Hmm. How about doing things this way?
> 
>  - at the beginning of handle_options() remember argv[0]
> 
>  - restructure handle_options() so that it does not run setup_pager() and
>    setenv("GIT_PAGER", "cat", 1) inside the loop, but instead remember
>    what we had on the command line;
> 
>  - after the handle_options() loop, if we saw an explicit --pager,
>    --no-pager, that's the decision;
> 
>  - otherwise:
> 
>    - look at argv[0] to see what the command is;
> 
>    - do the config thing to see if there is user preference; if there is
>      one, that setting decides;
> 
>    - otherwise:
> 
>      - see the built-in defaults;
> 
>  - and finally use or not use pager depending on what we found above.

OK, that makes some sense. I think some of what you describe is just refactoring (e.g., it doesn't matter if we actually do things when we see --no-pager or afterwards, since it always takes precedence). The key things are:

  - work not just on running builtins, but before we even figure out
    whether we have a builtin or a script

  - in my patch the config just says "ignore the default USE_PAGER", but
    it really should be "turn off the pager via GIT_PAGER=cat". That way
    you can say pager.stash = false, and it will impact the git-diff
    invocation run by stash.

But that isn't to say the refactoring isn't worth doing to keep things clean. I will take a stab at restructuring it the way you specified.

There is one remaining annoyance, though: this code is only run via the git wrapper. That means that you will get different behavior for "git-stash" versus "git stash". To make that work, we would have to put equivalent support into each script (although we could hit several at once with git-sh-setup.sh) and each non-builtin.

-Peff