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

> 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.
>
> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to autol. Also check

> 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.

> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to autol. Also check
> if the pager is active.
>
> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Kevin Daudt <me@ikke.info>
> ---
> This came to light when someone wondered on irc why
> column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
> found it to work with always, but not with auto.
>
> I could not get the test to work yet, because somehow it's not giving
> any output, so feedback regarding that is welcome.

> 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.

> On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
>> On 9 October 2017 at 23:45, Kevin Daudt <me@ikke.info> wrote:
>> > 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.

>> 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.

> On 10 October 2017 at 16:01, Jeff King <peff@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
> >> On 9 October 2017 at 23:45, Kevin Daudt <me@ikke.info> wrote:
> >> > 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.
> 
> Yes.

> > > Yeah, I didn't think of that with respect to the pager. This is a
> > > regression in v2.14.2, I think.
> > 
> > Yes.
> 
> Though 2.14.2 enabled the pager by default, even before that when
> someone would've enabled the pager theirselves (by setting pager.tag for
> example), it would also shown just a single column.
> 
> I could reproduce it as far as 2.8.3 (I could not test further due to
> libssl incompattibility).

> +test_expect_success TTY '20 columns, mode auto, pager' '
> +	cat >expected <<\EOF &&
> +one    seven
> +two    eight
> +three  nine
> +four   ten
> +five   eleven
> +six
> +EOF
> +	test_terminal env PAGER="cat|cat" git column --mode=auto <lista >actual &&
> +	test_cmp expected actual
> +'

> That said, I'm still puzzled why it would return zero output. Strace
> shows that the read from stdin is getting no input. I suspect this may
> have to do with how we redirect stdin in test-terminal.perl.
> 
> See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), which claims there's some raciness with closing the
> descriptor.

> On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
>
> it will randomly succeed or fail, depending on whether sed manages to
> read the input before the stdin terminal is closed.
>
> I'm not sure of an easy way to fix test-terminal, but we could work
> around it like this (which also uses "-p" to actually invoke the pager,
> and uses a pager that makes it clear when it's being run):
>
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 9441145bf0..d322c3b745 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -180,14 +180,14 @@ EOF
>
>  test_expect_success TTY '20 columns, mode auto, pager' '
>         cat >expected <<\EOF &&
> -one    seven
> -two    eight
> -three  nine
> -four   ten
> -five   eleven
> -six
> +paged:one    seven
> +paged:two    eight
> +paged:three  nine
> +paged:four   ten
> +paged:five   eleven
> +paged:six
>  EOF
> -       test_terminal env PAGER="cat|cat" git column --mode=auto <lista >actual &&
> +       test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column --mode=auto <lista" >actual &&
>         test_cmp expected actual
>  '
>  test_done

> On 10 October 2017 at 16:29, Jeff King <peff@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
> >
> > it will randomly succeed or fail, depending on whether sed manages to
> > read the input before the stdin terminal is closed.
> >
> > I'm not sure of an easy way to fix test-terminal, but we could work
> > around it like this (which also uses "-p" to actually invoke the pager,
> > and uses a pager that makes it clear when it's being run):
> >
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 9441145bf0..d322c3b745 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -180,14 +180,14 @@ EOF
> >
> >  test_expect_success TTY '20 columns, mode auto, pager' '
> >         cat >expected <<\EOF &&
> > -one    seven
> > -two    eight
> > -three  nine
> > -four   ten
> > -five   eleven
> > -six
> > +paged:one    seven
> > +paged:two    eight
> > +paged:three  nine
> > +paged:four   ten
> > +paged:five   eleven
> > +paged:six
> >  EOF
> > -       test_terminal env PAGER="cat|cat" git column --mode=auto <lista >actual &&
> > +       test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column --mode=auto <lista" >actual &&
> >         test_cmp expected actual
> >  '
> >  test_done
> 
> Makes sense. FWIW, I don't see the flakyness with this.
> 
> > All that said, I think I'd just as soon test a real command like "git
> > tag", which doesn't care about reading from stdin.
> 
> For reference for Kevin, in case you consider testing, e.g., git tag,
> the main reason I referred to the test I posted as "hacky", was that I
> just inserted it at a more-or-less random place in t7006. So it had to
> play with `git reset` to avoid upsetting the later tests. It could
> obviously go to the end instead, but I was too lazy to move it and
> define a pager.

> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Kevin Daudt <me@ikke.info>
> ---
>  column.c         |  3 ++-
>  t/t7006-pager.sh | 14 ++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index f0f1abd1c..44c2ca5d3 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '
>         test_cmp expect actual
>  '
>
> +test_expect_success TTY 'git tag with auto-columns ' '
> +       test_commit one &&
> +       test_commit two &&
> +       test_commit three &&
> +       test_commit four &&
> +       test_commit five &&
> +       cat >expected <<\EOF &&
> +initial  one      two      three    four     five
> +EOF
> +       test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> +               git -p -c column.ui=auto tag --sort=authordate &&
> +       test_cmp expected actual.tag
> +'
> +
>  test_done

> On 11 October 2017 at 19:23, Kevin Daudt <me@ikke.info> wrote:
> > finalize_colopts in column.c only checks whether the output is a TTY to
> > determine if columns should be enabled with columns set to auto. Also check
> > if the pager is active.
> 
> Maybe you could say something about the difficulties of writing a test
> for `git column` proper. Something like this perhaps:
> 
>   Adding a test for git column is possible but requires some care to
>   work around a race on stdin. See commit 18d8c2693 (test_terminal:
>   redirect child process' stdin to a pty, 2015-08-04). Test git tag
>   instead, since that does not involve stdin, and since that was the
>   original motivation for this patch.

> 
> > Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> > Signed-off-by: Kevin Daudt <me@ikke.info>
> > ---
> >  column.c         |  3 ++-
> >  t/t7006-pager.sh | 14 ++++++++++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> Does the documentation on `column.ui` need to be updated? It talks about
> "if the output is to the terminal". That's similar to the documentation
> on the various `color.*`, so we should be fine, and arguably it's even
> better not to say anything since that makes it consistent.
> 
> > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> > index f0f1abd1c..44c2ca5d3 100755
> > --- a/t/t7006-pager.sh
> > +++ b/t/t7006-pager.sh
> > @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '
> >         test_cmp expect actual
> >  '
> >
> > +test_expect_success TTY 'git tag with auto-columns ' '
> > +       test_commit one &&
> > +       test_commit two &&
> > +       test_commit three &&
> > +       test_commit four &&
> > +       test_commit five &&
> > +       cat >expected <<\EOF &&
> > +initial  one      two      three    four     five
> > +EOF
> > +       test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> > +               git -p -c column.ui=auto tag --sort=authordate &&
> > +       test_cmp expected actual.tag
> > +'
> > +
> >  test_done
> 
> Since `git tag` pages when it's listing, you don't need the `-p`. But
> it's not like it hurts to have it. Yeah, I know, you needed it with `git
> column`. :-)

> On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
>> On 11 October 2017 at 19:23, Kevin Daudt <me@ikke.info> wrote:
>> I wonder if it's useful to set COLUMNS a bit lower so that this has to
>> split across more than one line (but not six), i.e., to do something
>> non-trivial. I suppose that might lower the chances of some weird
>> breakage slipping through.
>
> Yeah, I was doubting about that, but wouldn't this amount to testing
> whether git column is working properly, instead of just testing whether
> it's being done at all?

>> These were just the thoughts that occurred to me, not sure if any of
>> them is particularly significant. Thanks for cleaning up after me.
>>
>
> np. Just as I posted earlier, I think you did not actually cause the bug
> (because this has never worked), it just made it visible to more users.

> 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.
>
> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to auto. Also
> check if the pager is active.
>
> Adding a test for git column is possible but requires some care to work
> around a race on stdin. See commit 18d8c2693 (test_terminal: redirect
> child process' stdin to a pty, 2015-08-04). Test git tag instead, since
> that does not involve stdin, and since that was the original motivation
> for this patch.

> +test_expect_success TTY 'git tag with auto-columns ' '
> +	test_commit one &&
> +	test_commit two &&
> +	test_commit three &&
> +	test_commit four &&
> +	test_commit five &&
> +	cat >expected <<\EOF &&
> +initial  one      two      three    four     five
> +EOF
> +	test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> +		git -c column.ui=auto tag --sort=authordate &&
> +	test_cmp expected actual.tag
> +'

> --- a/column.c
> +++ b/column.c
> @@ -5,6 +5,7 @@
>  #include "parse-options.h"
>  #include "run-command.h"
>  #include "utf8.h"
> +#include "pager.c"

> Hi,
>
> Kevin Daudt wrote:
>
>> --- a/column.c
>> +++ b/column.c
>> @@ -5,6 +5,7 @@
>>  #include "parse-options.h"
>>  #include "run-command.h"
>>  #include "utf8.h"
>> +#include "pager.c"
>
> Should this be pager.h?

> Subject: column: do not include pager.c
>
> Everything this file needs from the pager API (e.g. term_columns(),
> pager_in_use()) is already declared in the header file it includes.
>
> Noticed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  column.c | 1 -
>  1 file changed, 1 deletion(-)

> --- a/column.c
> +++ b/column.c
> @@ -5,7 +5,6 @@
>  #include "parse-options.h"
>  #include "run-command.h"
>  #include "utf8.h"
> -#include "pager.c"
>  
>  #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
>  			    (x) * (d)->rows + (y) : \

> Hi,
> 
> Kevin Daudt wrote:
> 
> > --- a/column.c
> > +++ b/column.c
> > @@ -5,6 +5,7 @@
> >  #include "parse-options.h"
> >  #include "run-command.h"
> >  #include "utf8.h"
> > +#include "pager.c"
> 
> Should this be pager.h?
> 
> Thanks,
> Jonathan