Re: [RFC] column: show auto columns when pager is active

From: Jeff King <peff@peff.net>
Date: Oct 10, 2017, 14:01 UTC
Message-ID: <20171010140118.quyesljvz44wvfx6@sigill.intra.peff.net>
In-Reply-To: <CAN0heSoWTpVZXmJkoEQtc_rjFj3PdPWV_37B9LukxxsYy5ox1w@mail.gmail.com>

On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:

Show 14 quoted lines

> On 9 October 2017 at 23:45, Kevin Daudt <me@ikke.info> wrote:
> > When columns are set to automatic for git tag and the output is
> > paginated by git, the output is a single column instead of multiple
> > columns.
> >
> > Standard behaviour in git is to honor auto values when the pager is
> > active, which happens for example with commands like git log showing
> > colors when being paged.
> >
> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> > 2017-08-02), the pager has been enabled by default, exposing this
> > problem to more people.
> 
> Oh. :( I didn't know about "column" to be honest.

Yeah, I didn't think of that with respect to the pager. This is a regression in v2.14.2, I think.

I agree that anything that is "auto" on stdout probably ought to kick in when the pager is in effect (since that only kicks in when stdout _was_ a tty before we stuck a pager in front of it).

> I had slightly more success with PAGER="cat >actual", but the test is
> flaky for some reason.

The test in t9002 should be immune to this, but the one you suggest in t7006 would need to set COLUMNS to get consistent output, I think.

> In any case, it might make sense to test an
> actual use-case also. Of course, the code should be largely the same,
> but in builtin/tag.c, it's quite important that `setup_auto_pager()`
> and `finalize_colopts()` are called in the right order.

I think it might work out either way. If we have started the pager when we finalize_colopts(), then the pager_in_use() bit will kick in. If we haven't, then either:

  1. stdout is a tty, and we'll kick in the auto behavior for columns,
     and then later for the pager.

  2. stdout isn't a tty, in which case we also won't kick in the pager.

-Peff