Re: [PATCH/DONOTAPPLY 0/4] first steps towards pager.foo.{command,enable}

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 7, 2017, 20:46 UTC
Message-ID: <CAN0heSrgkjpt5MfMbc15L0z3M6ch3F3qBgCqEf7i6VOe73fWtg@mail.gmail.com>
In-Reply-To: <20171106104801.73n27fgmn43y6m3v@sigill.intra.peff.net>

On 6 November 2017 at 11:48, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> On Sun, Nov 05, 2017 at 12:58:18PM +0100, Martin Ågren wrote:
>> In particular, they do not teach `--paginate` to use the pager
>> configured by `pager.foo.command`. It is already now possible to use
>> `pager.foo` to say "I don't want you to page, but if I later give you
>> `pager.foo=true`, this is the pager I want you to use". That does not
>> work with `--paginate`, but this can all be explained -- indeed, we
>> document that `--paginate` overrules `pager.foo`.
>
> Hmm. I think the current behavior is actually buggy. I assume the
> documentation you mean is from git.txt:
>
>   --paginate::
>           Pipe all output into 'less' (or if set, $PAGER) if standard
>           output is a terminal.  This overrides the `pager.<cmd>`
>           configuration options (see the "Configuration Mechanism" section
>           below).

Yes, that's what I meant.

> That comes from 06300d9753 (git.1: Clarify the behavior of the
> --paginate option, 2010-02-14). But I think that commit was just trying
> to clarify that "--paginate" overrides the defaults and config, but not
> does say "always paginate".

Thanks for digging.

You're probably right that this was not intended. I think I put way too much weight on the "s" in "options". I understood it as "both ways pager.foo can be used". From the commit message you point to, it seems that only the boolean-ness was intended. (The "s" probably refers to pager.foo, pager.bar, ...)

Show 7 quoted lines

>> If we teach `--paginate` to respect `pager.foo.command`, it seems that
>> we would either 1) introduce a small (and possibly hard to understand
>> and explain) difference between the old-style and the new-style
>> pager-configuration or 2) knowingly change the behavior of
>> `--paginate`
>> with `pager.foo` or 3) knowingly change the behavior of
>> `pager.foo=false` as documented in the first patch.

> I think I'm suggesting (2), then.

I think you're right. If this topic doesn't get picked up in the near future (in London or elsewhere), I'll get back to this.

A final thought for whoever tackles this: Playing with something like `git -c pager.log=$foo --paginate log` vs `git -c core.pager=$foo --paginate log` might be helpful. Possibly $foo=cat could be particularly interesting, I haven't thought hard about it.

Martin