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

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

[Kevin Daudt]

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.


 column.c          |  3 ++-
 t/t9002-column.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #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) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)
 		if (stdout_is_tty < 0)
 			stdout_is_tty = isatty(1);
 		*colopts &= ~COL_ENABLE_MASK;
-		if (stdout_is_tty)
+		if (stdout_is_tty || pager_in_use())
 			*colopts |= COL_ENABLED;
 	}
 	return 0;
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b..9441145bf 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -2,6 +2,7 @@
 
 test_description='git column'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
 	cat >lista <<\EOF
@@ -177,4 +178,16 @@ EOF
 	test_cmp expected actual
 '
 
+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
+'
 test_done
-- 
2.14.2

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

[Eric Sunshine]

On Mon, Oct 9, 2017 at 5:45 PM, 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.
>
> 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

Presumably: s/autol/auto/

> if the pager is active.
>
> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Kevin Daudt <me@ikke.info>

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

[Martin Ågren]

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.

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

I had slightly more success with PAGER="cat >actual", but the test is
flaky for some reason. 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. In other words,
there is some regression-potential. This is a whitespace-damaged and
hacky attempt to test. Maybe it helps a little. I hope I'll have more
time later today.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..91f2b5871 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -214,6 +214,19 @@ test_expect_success TTY 'git tag as alias
respects pager.tag with -l' '
        ! test -e paginated.out
 '

+test_expect_success TTY 'git tag with column.tag=auto' '
+       test_commit second &&
+       test_commit third &&
+       test_commit fourth &&
+       test_when_finished "git reset --hard HEAD~3" &&
+       cat >expected <<\EOF &&
+fourth   initial  second   third
+EOF
+       rm -f paginated.out &&
+       test_terminal git -c pager.tag -c column.tag=auto tag &&
+       test_cmp expected paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {

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

[Jeff King]

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

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

[Martin Ågren]

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.

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

Right you are.

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

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:
> 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.

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

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

[Jeff King]

On Wed, Oct 11, 2017 at 06:54:04AM +0200, Kevin Daudt wrote:

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

Right, I think it has always been broken. It's just that it became a lot
more widespread with the increased use of the pager.

I specifically wanted to make sure this wasn't a regression in the v2.15
release candidates (since that would mean we'd want to deal with it
before shipping v2.15). It still _would_ be nice to deal with it soon,
but since it's already been in the wild for a bit, it can wait to hit
"maint" in the post-v2.15 cycle.

-Peff

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

[Jeff King]

On Mon, Oct 09, 2017 at 11:45:43PM +0200, Kevin Daudt wrote:

> +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
> +'

I don't think "git column" will run the pager by default, will it?
You'd need "git -p" here.

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.

-Peff

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

[Jeff King]

On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:

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

Ah, yeah, I think that is it. Try:

  echo input | test_terminal sed s/^/foo:/

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

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.

-Peff

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

[Martin Ågren]

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.

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

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:
> 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.

Thanks Jeff and Martin, I will use your tips to build a test based on
git tag instead.

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

[Kevin Daudt]

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.

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/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #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) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)
 		if (stdout_is_tty < 0)
 			stdout_is_tty = isatty(1);
 		*colopts &= ~COL_ENABLE_MASK;
-		if (stdout_is_tty)
+		if (stdout_is_tty || pager_in_use())
 			*colopts |= COL_ENABLED;
 	}
 	return 0;
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
-- 
2.14.2

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

[Martin Ågren]

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`. :-)

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.

This test uses "actual.tag" while most (all?) others in this file use
"actual". Maybe you worry about checking the "wrong" file, e.g., in case
the pager doesn't kick in. You could do `rm -f actual` before the
`test_terminal`-invocation to protect against that.

These were just the thoughts that occurred to me, not sure if any of
them is particularly significant. Thanks for cleaning up after me.

Martin

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

[Kevin Daudt]

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

Right, makes sense.
> 
> > 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`. :-)

Right, it was a bit of a left-over since I assumed the PAGER='cat >paginated.out'
from the beginning of the test was in place and I wasn't getting any
output, but it turned out PAGER wasn't set.

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

> This test uses "actual.tag" while most (all?) others in this file use
> "actual". Maybe you worry about checking the "wrong" file, e.g., in case
> the pager doesn't kick in. You could do `rm -f actual` before the
> `test_terminal`-invocation to protect against that.

Yeah, I actually ran into that, but rm-ing it is better, I agree.

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

Kevin

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

[Martin Ågren]

On 11 October 2017 at 20:36, Kevin Daudt <me@ikke.info> wrote:
> 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?

Right, I think you'd need a pretty crazy bug in order to slip through
all tests here.

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

Well, the general bug/behavior was always there, but I regressed a
particular use-case. In 2.14, you could do `git tag` with column.ui=auto
and it would do the columns-thing. But with ff1e72483, the behavior
changed. To add insult to injury, it might be non-obvious that the pager
is running, since with just a few tags, the pager simply exits silently.
So debugging this could probably be quite frustrating.

Martin

[PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

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.

Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
Signed-off-by: Kevin Daudt <me@ikke.info>
---
Changes since v1:

- Remove redundant -p flag in tests
- Explain why git tag is being tested instead of the more obvious git
  column

 column.c         |  3 ++-
 t/t7006-pager.sh | 14 ++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #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) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)
 		if (stdout_is_tty < 0)
 			stdout_is_tty = isatty(1);
 		*colopts &= ~COL_ENABLE_MASK;
-		if (stdout_is_tty)
+		if (stdout_is_tty || pager_in_use())
 			*colopts |= COL_ENABLED;
 	}
 	return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..e985b6873 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 -c column.ui=auto tag --sort=authordate &&
+	test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

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

[Junio C Hamano]

Kevin Daudt <me@ikke.info> writes:

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

Nicely done.

> +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
> +'

I'd use <<-\EOF so that here document can be intended like other
tests, and also use expect vs actual that are used in the other
tests in the same script, instead of suddenly becoming creative
in only this single test.  I can do these clean-ups locally when
queuing so no need to resend only to collect these.

Thanks, will queue.

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Ouch.  And I was not paying enough attention.

Thanks, I'll queue this on top.

-- >8 --
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(-)

diff --git a/column.c b/column.c
index ded50337f7..49ab85b769 100644
--- 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) : \

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

[Jonathan Nieder]

Junio C Hamano wrote:

> 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(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

> --- 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) : \

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

[Kevin Daudt]

On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote:
> 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

Thanks for catching this.