{"thread":{"id":"46940","subject":"[RFC] column: show auto columns when pager is active","startedAt":"2017-10-09T21:46:11Z","lastAt":"2017-10-24T06:58:12Z","messageCount":21,"participants":["Kevin Daudt","Eric Sunshine","Martin Ågren","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330054","messageId":"20171009214543.12986-1-me@ikke.info","threadId":"46940","inReplyTo":null,"subject":"[RFC] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-09T21:45:43Z","receivedAt":"2017-10-09T21:46:11Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"When columns are set to automatic for git tag and the output is\npaginated by git, the output is a single column instead of multiple\ncolumns.\n\nStandard behaviour in git is to honor auto values when the pager is\nactive, which happens for example with commands like git log showing\ncolors when being paged.\n\nSince ff1e72483 (tag: change default of `pager.tag` to \"on\",\n2017-08-02), the pager has been enabled by default, exposing this\nproblem to more people.\n\nfinalize_colopts in column.c only checks whether the output is a TTY to\ndetermine if columns should be enabled with columns set to autol. Also check\nif the pager is active.\n\nHelped-by: Rafael Ascensão \nSigned-off-by: Kevin Daudt \n---\nThis came to light when someone wondered on irc why\ncolumn.tag=[auto|always] did not work on Mac OS. Testing it myself, I\nfound it to work with always, but not with auto.\n\nI could not get the test to work yet, because somehow it's not giving\nany output, so feedback regarding that is welcome.\n\n\n column.c | 3 ++-\n t/t9002-column.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/column.c b/column.c\nindex ff7bdab1a..ded50337f 100644\n--- a/column.c\n+++ b/column.c\n@@ -5,6 +5,7 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"utf8.h\"\n+#include \"pager.c\"\n \n #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \\\n \t\t\t (x) * (d)->rows + (y) : \\\n@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)\n \t\tif (stdout_is_tty < 0)\n \t\t\tstdout_is_tty = isatty(1);\n \t\t*colopts &= ~COL_ENABLE_MASK;\n-\t\tif (stdout_is_tty)\n+\t\tif (stdout_is_tty || pager_in_use())\n \t\t\t*colopts |= COL_ENABLED;\n \t}\n \treturn 0;\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 89983527b..9441145bf 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -2,6 +2,7 @@\n \n test_description='git column'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n test_expect_success 'setup' '\n \tcat >lista <<\\EOF\n@@ -177,4 +178,16 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success TTY '20 columns, mode auto, pager' '\n+\tcat >expected <<\\EOF &&\n+one seven\n+two eight\n+three nine\n+four ten\n+five eleven\n+six\n+EOF\n+\ttest_terminal env PAGER=\"cat|cat\" git column --mode=auto actual &&\n+\ttest_cmp expected actual\n+'\n test_done\n-- \n2.14.2\n\n"},{"id":"330065","messageId":"CAPig+cTnm01CJJN6tf-K_6Ah7uDoMeMJAxHUbC9YE1TxvAKF5g@mail.gmail.com","threadId":"46940","inReplyTo":"20171009214543.12986-1-me@ikke.info","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-09T23:27:19Z","receivedAt":"2017-10-09T23:27:24Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 9, 2017 at 5:45 PM, Kevin Daudt wrote:\n> When columns are set to automatic for git tag and the output is\n> paginated by git, the output is a single column instead of multiple\n> columns.\n>\n> Standard behaviour in git is to honor auto values when the pager is\n> active, which happens for example with commands like git log showing\n> colors when being paged.\n>\n> Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n> 2017-08-02), the pager has been enabled by default, exposing this\n> problem to more people.\n>\n> finalize_colopts in column.c only checks whether the output is a TTY to\n> determine if columns should be enabled with columns set to autol. Also check\n\nPresumably: s/autol/auto/\n\n> if the pager is active.\n>\n> Helped-by: Rafael Ascensão \n> Signed-off-by: Kevin Daudt \n"},{"id":"330104","messageId":"CAN0heSoWTpVZXmJkoEQtc_rjFj3PdPWV_37B9LukxxsYy5ox1w@mail.gmail.com","threadId":"46940","inReplyTo":"20171009214543.12986-1-me@ikke.info","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-10T10:30:49Z","receivedAt":"2017-10-10T10:30:56Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 October 2017 at 23:45, Kevin Daudt wrote:\n> When columns are set to automatic for git tag and the output is\n> paginated by git, the output is a single column instead of multiple\n> columns.\n>\n> Standard behaviour in git is to honor auto values when the pager is\n> active, which happens for example with commands like git log showing\n> colors when being paged.\n>\n> Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n> 2017-08-02), the pager has been enabled by default, exposing this\n> problem to more people.\n\nOh. :( I didn't know about \"column\" to be honest.\n\n> finalize_colopts in column.c only checks whether the output is a TTY to\n> determine if columns should be enabled with columns set to autol. Also check\n> if the pager is active.\n>\n> Helped-by: Rafael Ascensão \n> Signed-off-by: Kevin Daudt \n> ---\n> This came to light when someone wondered on irc why\n> column.tag=[auto|always] did not work on Mac OS. Testing it myself, I\n> found it to work with always, but not with auto.\n>\n> I could not get the test to work yet, because somehow it's not giving\n> any output, so feedback regarding that is welcome.\n\nI had slightly more success with PAGER=\"cat >actual\", but the test is\nflaky for some reason. In any case, it might make sense to test an\nactual use-case also. Of course, the code should be largely the same,\nbut in builtin/tag.c, it's quite important that `setup_auto_pager()`\nand `finalize_colopts()` are called in the right order. In other words,\nthere is some regression-potential. This is a whitespace-damaged and\nhacky attempt to test. Maybe it helps a little. I hope I'll have more\ntime later today.\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex f0f1abd1c..91f2b5871 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -214,6 +214,19 @@ test_expect_success TTY 'git tag as alias\nrespects pager.tag with -l' '\n ! test -e paginated.out\n '\n\n+test_expect_success TTY 'git tag with column.tag=auto' '\n+ test_commit second &&\n+ test_commit third &&\n+ test_commit fourth &&\n+ test_when_finished \"git reset --hard HEAD~3\" &&\n+ cat >expected <<\\EOF &&\n+fourth initial second third\n+EOF\n+ rm -f paginated.out &&\n+ test_terminal git -c pager.tag -c column.tag=auto tag &&\n+ test_cmp expected paginated.out\n+'\n+\n # A colored commit log will begin with an appropriate ANSI escape\n # for the first color; the text \"commit\" comes later.\n colorful() {\n"},{"id":"330118","messageId":"20171010140118.quyesljvz44wvfx6@sigill.intra.peff.net","threadId":"46940","inReplyTo":"CAN0heSoWTpVZXmJkoEQtc_rjFj3PdPWV_37B9LukxxsYy5ox1w@mail.gmail.com","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T14:01:18Z","receivedAt":"2017-10-10T14:01:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:\n\n> On 9 October 2017 at 23:45, Kevin Daudt wrote:\n> > When columns are set to automatic for git tag and the output is\n> > paginated by git, the output is a single column instead of multiple\n> > columns.\n> >\n> > Standard behaviour in git is to honor auto values when the pager is\n> > active, which happens for example with commands like git log showing\n> > colors when being paged.\n> >\n> > Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n> > 2017-08-02), the pager has been enabled by default, exposing this\n> > problem to more people.\n> \n> Oh. :( I didn't know about \"column\" to be honest.\n\nYeah, I didn't think of that with respect to the pager. This is a\nregression in v2.14.2, I think.\n\nI agree that anything that is \"auto\" on stdout probably ought to kick in\nwhen the pager is in effect (since that only kicks in when stdout _was_\na tty before we stuck a pager in front of it).\n\n> I had slightly more success with PAGER=\"cat >actual\", but the test is\n> flaky for some reason.\n\nThe test in t9002 should be immune to this, but the one you suggest in\nt7006 would need to set COLUMNS to get consistent output, I think.\n\n> In any case, it might make sense to test an\n> actual use-case also. Of course, the code should be largely the same,\n> but in builtin/tag.c, it's quite important that `setup_auto_pager()`\n> and `finalize_colopts()` are called in the right order.\n\nI think it might work out either way. If we have started the pager when\nwe finalize_colopts(), then the pager_in_use() bit will kick in. If we\nhaven't, then either:\n\n 1. stdout is a tty, and we'll kick in the auto behavior for columns,\n and then later for the pager.\n\n 2. stdout isn't a tty, in which case we also won't kick in the pager.\n\n-Peff\n"},{"id":"330119","messageId":"20171010141019.3vfowj4fvxdvvl3c@sigill.intra.peff.net","threadId":"46940","inReplyTo":"20171009214543.12986-1-me@ikke.info","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T14:10:19Z","receivedAt":"2017-10-10T14:10:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 09, 2017 at 11:45:43PM +0200, Kevin Daudt wrote:\n\n> +test_expect_success TTY '20 columns, mode auto, pager' '\n> +\tcat >expected <<\\EOF &&\n> +one seven\n> +two eight\n> +three nine\n> +four ten\n> +five eleven\n> +six\n> +EOF\n> +\ttest_terminal env PAGER=\"cat|cat\" git column --mode=auto actual &&\n> +\ttest_cmp expected actual\n> +'\n\nI don't think \"git column\" will run the pager by default, will it?\nYou'd need \"git -p\" here.\n\nThat said, I'm still puzzled why it would return zero output. Strace\nshows that the read from stdin is getting no input. I suspect this may\nhave to do with how we redirect stdin in test-terminal.perl.\n\nSee 18d8c26930 (test_terminal: redirect child process' stdin to a pty,\n2015-08-04), which claims there's some raciness with closing the\ndescriptor.\n\n-Peff\n"},{"id":"330120","messageId":"20171010142930.vao2nqg4pv7f2znk@sigill.intra.peff.net","threadId":"46940","inReplyTo":"20171010141019.3vfowj4fvxdvvl3c@sigill.intra.peff.net","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T14:29:30Z","receivedAt":"2017-10-10T14:29:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:\n\n> That said, I'm still puzzled why it would return zero output. Strace\n> shows that the read from stdin is getting no input. I suspect this may\n> have to do with how we redirect stdin in test-terminal.perl.\n> \n> See 18d8c26930 (test_terminal: redirect child process' stdin to a pty,\n> 2015-08-04), which claims there's some raciness with closing the\n> descriptor.\n\nAh, yeah, I think that is it. Try:\n\n echo input | test_terminal sed s/^/foo:/\n\nit will randomly succeed or fail, depending on whether sed manages to\nread the input before the stdin terminal is closed.\n\nI'm not sure of an easy way to fix test-terminal, but we could work\naround it like this (which also uses \"-p\" to actually invoke the pager,\nand uses a pager that makes it clear when it's being run):\n\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 9441145bf0..d322c3b745 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -180,14 +180,14 @@ EOF\n \n test_expect_success TTY '20 columns, mode auto, pager' '\n \tcat >expected <<\\EOF &&\n-one seven\n-two eight\n-three nine\n-four ten\n-five eleven\n-six\n+paged:one seven\n+paged:two eight\n+paged:three nine\n+paged:four ten\n+paged:five eleven\n+paged:six\n EOF\n-\ttest_terminal env PAGER=\"cat|cat\" git column --mode=auto actual &&\n+\ttest_terminal env PAGER=\"sed s/^/paged:/\" sh -c \"git -p column --mode=auto actual &&\n \ttest_cmp expected actual\n '\n test_done\n\nAll that said, I think I'd just as soon test a real command like \"git\ntag\", which doesn't care about reading from stdin.\n\n-Peff\n"},{"id":"330122","messageId":"CAN0heSqZn1pqskK-KP=DEW1-Kz=-avX0TUON38Dk_TVKAsBAiQ@mail.gmail.com","threadId":"46940","inReplyTo":"20171010140118.quyesljvz44wvfx6@sigill.intra.peff.net","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-10T17:02:00Z","receivedAt":"2017-10-10T17:02:07Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 10 October 2017 at 16:01, Jeff King wrote:\n> On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:\n>> On 9 October 2017 at 23:45, Kevin Daudt wrote:\n>> > Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n>> > 2017-08-02), the pager has been enabled by default, exposing this\n>> > problem to more people.\n>>\n>> Oh. :( I didn't know about \"column\" to be honest.\n>\n> Yeah, I didn't think of that with respect to the pager. This is a\n> regression in v2.14.2, I think.\n\nYes.\n\n>> In any case, it might make sense to test an\n>> actual use-case also. Of course, the code should be largely the same,\n>> but in builtin/tag.c, it's quite important that `setup_auto_pager()`\n>> and `finalize_colopts()` are called in the right order.\n>\n> I think it might work out either way. If we have started the pager when\n> we finalize_colopts(), then the pager_in_use() bit will kick in. If we\n> haven't, then either:\n>\n> 1. stdout is a tty, and we'll kick in the auto behavior for columns,\n> and then later for the pager.\n>\n> 2. stdout isn't a tty, in which case we also won't kick in the pager.\n\nRight you are.\n"},{"id":"330123","messageId":"CAN0heSr5EN2Ls5Xd-Sg6b-z9WsJM9s1xB7ZczDkRgv79hxhsaA@mail.gmail.com","threadId":"46940","inReplyTo":"20171010142930.vao2nqg4pv7f2znk@sigill.intra.peff.net","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-10T17:04:42Z","receivedAt":"2017-10-10T17:04:48Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 10 October 2017 at 16:29, Jeff King wrote:\n> On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:\n>\n> it will randomly succeed or fail, depending on whether sed manages to\n> read the input before the stdin terminal is closed.\n>\n> I'm not sure of an easy way to fix test-terminal, but we could work\n> around it like this (which also uses \"-p\" to actually invoke the pager,\n> and uses a pager that makes it clear when it's being run):\n>\n> diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> index 9441145bf0..d322c3b745 100755\n> --- a/t/t9002-column.sh\n> +++ b/t/t9002-column.sh\n> @@ -180,14 +180,14 @@ EOF\n>\n> test_expect_success TTY '20 columns, mode auto, pager' '\n> cat >expected <<\\EOF &&\n> -one seven\n> -two eight\n> -three nine\n> -four ten\n> -five eleven\n> -six\n> +paged:one seven\n> +paged:two eight\n> +paged:three nine\n> +paged:four ten\n> +paged:five eleven\n> +paged:six\n> EOF\n> - test_terminal env PAGER=\"cat|cat\" git column --mode=auto actual &&\n> + test_terminal env PAGER=\"sed s/^/paged:/\" sh -c \"git -p column --mode=auto actual &&\n> test_cmp expected actual\n> '\n> test_done\n\nMakes sense. FWIW, I don't see the flakyness with this.\n\n> All that said, I think I'd just as soon test a real command like \"git\n> tag\", which doesn't care about reading from stdin.\n\nFor reference for Kevin, in case you consider testing, e.g., git tag,\nthe main reason I referred to the test I posted as \"hacky\", was that I\njust inserted it at a more-or-less random place in t7006. So it had to\nplay with `git reset` to avoid upsetting the later tests. It could\nobviously go to the end instead, but I was too lazy to move it and\ndefine a pager.\n"},{"id":"330130","messageId":"20171010183247.GA16800@alpha.vpn.ikke.info","threadId":"46940","inReplyTo":"CAN0heSr5EN2Ls5Xd-Sg6b-z9WsJM9s1xB7ZczDkRgv79hxhsaA@mail.gmail.com","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-10T18:32:47Z","receivedAt":"2017-10-10T18:32:53Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:\n> On 10 October 2017 at 16:29, Jeff King wrote:\n> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:\n> >\n> > it will randomly succeed or fail, depending on whether sed manages to\n> > read the input before the stdin terminal is closed.\n> >\n> > I'm not sure of an easy way to fix test-terminal, but we could work\n> > around it like this (which also uses \"-p\" to actually invoke the pager,\n> > and uses a pager that makes it clear when it's being run):\n> >\n> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> > index 9441145bf0..d322c3b745 100755\n> > --- a/t/t9002-column.sh\n> > +++ b/t/t9002-column.sh\n> > @@ -180,14 +180,14 @@ EOF\n> >\n> > test_expect_success TTY '20 columns, mode auto, pager' '\n> > cat >expected <<\\EOF &&\n> > -one seven\n> > -two eight\n> > -three nine\n> > -four ten\n> > -five eleven\n> > -six\n> > +paged:one seven\n> > +paged:two eight\n> > +paged:three nine\n> > +paged:four ten\n> > +paged:five eleven\n> > +paged:six\n> > EOF\n> > - test_terminal env PAGER=\"cat|cat\" git column --mode=auto actual &&\n> > + test_terminal env PAGER=\"sed s/^/paged:/\" sh -c \"git -p column --mode=auto actual &&\n> > test_cmp expected actual\n> > '\n> > test_done\n> \n> Makes sense. FWIW, I don't see the flakyness with this.\n> \n> > All that said, I think I'd just as soon test a real command like \"git\n> > tag\", which doesn't care about reading from stdin.\n> \n> For reference for Kevin, in case you consider testing, e.g., git tag,\n> the main reason I referred to the test I posted as \"hacky\", was that I\n> just inserted it at a more-or-less random place in t7006. So it had to\n> play with `git reset` to avoid upsetting the later tests. It could\n> obviously go to the end instead, but I was too lazy to move it and\n> define a pager.\n\nThanks Jeff and Martin, I will use your tips to build a test based on\ngit tag instead.\n"},{"id":"330148","messageId":"20171011045404.GB16800@alpha.vpn.ikke.info","threadId":"46940","inReplyTo":"CAN0heSqZn1pqskK-KP=DEW1-Kz=-avX0TUON38Dk_TVKAsBAiQ@mail.gmail.com","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-11T04:54:04Z","receivedAt":"2017-10-11T04:54:11Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:\n> On 10 October 2017 at 16:01, Jeff King wrote:\n> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:\n> >> On 9 October 2017 at 23:45, Kevin Daudt wrote:\n> >> > Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n> >> > 2017-08-02), the pager has been enabled by default, exposing this\n> >> > problem to more people.\n> >>\n> >> Oh. :( I didn't know about \"column\" to be honest.\n> >\n> > Yeah, I didn't think of that with respect to the pager. This is a\n> > regression in v2.14.2, I think.\n> \n> Yes.\n\nThough 2.14.2 enabled the pager by default, even before that when\nsomeone would've enabled the pager theirselves (by setting pager.tag for\nexample), it would also shown just a single column.\n\nI could reproduce it as far as 2.8.3 (I could not test further due to\nlibssl incompattibility).\n"},{"id":"330184","messageId":"20171011172310.2932-1-me@ikke.info","threadId":"46940","inReplyTo":"20171009214543.12986-1-me@ikke.info","subject":"[PATCH] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-11T17:23:10Z","receivedAt":"2017-10-11T17:24:06Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"When columns are set to automatic for git tag and the output is\npaginated by git, the output is a single column instead of multiple\ncolumns.\n\nStandard behaviour in git is to honor auto values when the pager is\nactive, which happens for example with commands like git log showing\ncolors when being paged.\n\nSince ff1e72483 (tag: change default of `pager.tag` to \"on\",\n2017-08-02), the pager has been enabled by default, exposing this\nproblem to more people.\n\nfinalize_colopts in column.c only checks whether the output is a TTY to\ndetermine if columns should be enabled with columns set to auto. Also check\nif the pager is active.\n\nHelped-by: Rafael Ascensão \nSigned-off-by: Kevin Daudt \n---\n column.c | 3 ++-\n t/t7006-pager.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/column.c b/column.c\nindex ff7bdab1a..ded50337f 100644\n--- a/column.c\n+++ b/column.c\n@@ -5,6 +5,7 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"utf8.h\"\n+#include \"pager.c\"\n \n #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \\\n \t\t\t (x) * (d)->rows + (y) : \\\n@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)\n \t\tif (stdout_is_tty < 0)\n \t\t\tstdout_is_tty = isatty(1);\n \t\t*colopts &= ~COL_ENABLE_MASK;\n-\t\tif (stdout_is_tty)\n+\t\tif (stdout_is_tty || pager_in_use())\n \t\t\t*colopts |= COL_ENABLED;\n \t}\n \treturn 0;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex f0f1abd1c..44c2ca5d3 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'git tag with auto-columns ' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four &&\n+\ttest_commit five &&\n+\tcat >expected <<\\EOF &&\n+initial one two three four five\n+EOF\n+\ttest_terminal env PAGER=\"cat >actual.tag\" COLUMNS=80 \\\n+\t\tgit -p -c column.ui=auto tag --sort=authordate &&\n+\ttest_cmp expected actual.tag\n+'\n+\n test_done\n-- \n2.14.2\n\n"},{"id":"330192","messageId":"CAN0heSqbD8TJu+_d11gj2eftG3gR+n0j621q_uSnuLQc9t_pbQ@mail.gmail.com","threadId":"46940","inReplyTo":"20171011172310.2932-1-me@ikke.info","subject":"Re: [PATCH] column: show auto columns when pager is active","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-11T18:12:35Z","receivedAt":"2017-10-11T18:12:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 October 2017 at 19:23, Kevin Daudt wrote:\n> finalize_colopts in column.c only checks whether the output is a TTY to\n> determine if columns should be enabled with columns set to auto. Also check\n> if the pager is active.\n\nMaybe you could say something about the difficulties of writing a test\nfor `git column` proper. Something like this perhaps:\n\n Adding a test for git column is possible but requires some care to\n work around a race on stdin. See commit 18d8c2693 (test_terminal:\n redirect child process' stdin to a pty, 2015-08-04). Test git tag\n instead, since that does not involve stdin, and since that was the\n original motivation for this patch.\n\n> Helped-by: Rafael Ascensão \n> Signed-off-by: Kevin Daudt \n> ---\n> column.c | 3 ++-\n> t/t7006-pager.sh | 14 ++++++++++++++\n> 2 files changed, 16 insertions(+), 1 deletion(-)\n\nDoes the documentation on `column.ui` need to be updated? It talks about\n\"if the output is to the terminal\". That's similar to the documentation\non the various `color.*`, so we should be fine, and arguably it's even\nbetter not to say anything since that makes it consistent.\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index f0f1abd1c..44c2ca5d3 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success TTY 'git tag with auto-columns ' '\n> + test_commit one &&\n> + test_commit two &&\n> + test_commit three &&\n> + test_commit four &&\n> + test_commit five &&\n> + cat >expected <<\\EOF &&\n> +initial one two three four five\n> +EOF\n> + test_terminal env PAGER=\"cat >actual.tag\" COLUMNS=80 \\\n> + git -p -c column.ui=auto tag --sort=authordate &&\n> + test_cmp expected actual.tag\n> +'\n> +\n> test_done\n\nSince `git tag` pages when it's listing, you don't need the `-p`. But\nit's not like it hurts to have it. Yeah, I know, you needed it with `git\ncolumn`. :-)\n\nI wonder if it's useful to set COLUMNS a bit lower so that this has to\nsplit across more than one line (but not six), i.e., to do something\nnon-trivial. I suppose that might lower the chances of some weird\nbreakage slipping through.\n\nThis test uses \"actual.tag\" while most (all?) others in this file use\n\"actual\". Maybe you worry about checking the \"wrong\" file, e.g., in case\nthe pager doesn't kick in. You could do `rm -f actual` before the\n`test_terminal`-invocation to protect against that.\n\nThese were just the thoughts that occurred to me, not sure if any of\nthem is particularly significant. Thanks for cleaning up after me.\n\nMartin\n"},{"id":"330195","messageId":"20171011183640.GC16800@alpha.vpn.ikke.info","threadId":"46940","inReplyTo":"CAN0heSqbD8TJu+_d11gj2eftG3gR+n0j621q_uSnuLQc9t_pbQ@mail.gmail.com","subject":"Re: [PATCH] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-11T18:36:40Z","receivedAt":"2017-10-11T18:36:46Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:\n> On 11 October 2017 at 19:23, Kevin Daudt wrote:\n> > finalize_colopts in column.c only checks whether the output is a TTY to\n> > determine if columns should be enabled with columns set to auto. Also check\n> > if the pager is active.\n> \n> Maybe you could say something about the difficulties of writing a test\n> for `git column` proper. Something like this perhaps:\n> \n> Adding a test for git column is possible but requires some care to\n> work around a race on stdin. See commit 18d8c2693 (test_terminal:\n> redirect child process' stdin to a pty, 2015-08-04). Test git tag\n> instead, since that does not involve stdin, and since that was the\n> original motivation for this patch.\n\nRight, makes sense.\n> \n> > Helped-by: Rafael Ascensão \n> > Signed-off-by: Kevin Daudt \n> > ---\n> > column.c | 3 ++-\n> > t/t7006-pager.sh | 14 ++++++++++++++\n> > 2 files changed, 16 insertions(+), 1 deletion(-)\n> \n> Does the documentation on `column.ui` need to be updated? It talks about\n> \"if the output is to the terminal\". That's similar to the documentation\n> on the various `color.*`, so we should be fine, and arguably it's even\n> better not to say anything since that makes it consistent.\n> \n> > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> > index f0f1abd1c..44c2ca5d3 100755\n> > --- a/t/t7006-pager.sh\n> > +++ b/t/t7006-pager.sh\n> > @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_success TTY 'git tag with auto-columns ' '\n> > + test_commit one &&\n> > + test_commit two &&\n> > + test_commit three &&\n> > + test_commit four &&\n> > + test_commit five &&\n> > + cat >expected <<\\EOF &&\n> > +initial one two three four five\n> > +EOF\n> > + test_terminal env PAGER=\"cat >actual.tag\" COLUMNS=80 \\\n> > + git -p -c column.ui=auto tag --sort=authordate &&\n> > + test_cmp expected actual.tag\n> > +'\n> > +\n> > test_done\n> \n> Since `git tag` pages when it's listing, you don't need the `-p`. But\n> it's not like it hurts to have it. Yeah, I know, you needed it with `git\n> column`. :-)\n\nRight, it was a bit of a left-over since I assumed the PAGER='cat >paginated.out'\nfrom the beginning of the test was in place and I wasn't getting any\noutput, but it turned out PAGER wasn't set.\n\n> I wonder if it's useful to set COLUMNS a bit lower so that this has to\n> split across more than one line (but not six), i.e., to do something\n> non-trivial. I suppose that might lower the chances of some weird\n> breakage slipping through.\n\nYeah, I was doubting about that, but wouldn't this amount to testing\nwhether git column is working properly, instead of just testing whether\nit's being done at all?\n\n> This test uses \"actual.tag\" while most (all?) others in this file use\n> \"actual\". Maybe you worry about checking the \"wrong\" file, e.g., in case\n> the pager doesn't kick in. You could do `rm -f actual` before the\n> `test_terminal`-invocation to protect against that.\n\nYeah, I actually ran into that, but rm-ing it is better, I agree.\n\n> These were just the thoughts that occurred to me, not sure if any of\n> them is particularly significant. Thanks for cleaning up after me.\n> \n\nnp. Just as I posted earlier, I think you did not actually cause the bug\n(because this has never worked), it just made it visible to more users.\n\nKevin\n"},{"id":"330198","messageId":"CAN0heSqJuE5RorQSf8yX3TF_giqL-Mt+DJ5-V08eH3=xi+Q9wA@mail.gmail.com","threadId":"46940","inReplyTo":"20171011183640.GC16800@alpha.vpn.ikke.info","subject":"Re: [PATCH] column: show auto columns when pager is active","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-11T19:02:27Z","receivedAt":"2017-10-11T19:02:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 October 2017 at 20:36, Kevin Daudt wrote:\n> On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:\n>> On 11 October 2017 at 19:23, Kevin Daudt wrote:\n>> I wonder if it's useful to set COLUMNS a bit lower so that this has to\n>> split across more than one line (but not six), i.e., to do something\n>> non-trivial. I suppose that might lower the chances of some weird\n>> breakage slipping through.\n>\n> Yeah, I was doubting about that, but wouldn't this amount to testing\n> whether git column is working properly, instead of just testing whether\n> it's being done at all?\n\nRight, I think you'd need a pretty crazy bug in order to slip through\nall tests here.\n\n>> These were just the thoughts that occurred to me, not sure if any of\n>> them is particularly significant. Thanks for cleaning up after me.\n>>\n>\n> np. Just as I posted earlier, I think you did not actually cause the bug\n> (because this has never worked), it just made it visible to more users.\n\nWell, the general bug/behavior was always there, but I regressed a\nparticular use-case. In 2.14, you could do `git tag` with column.ui=auto\nand it would do the columns-thing. But with ff1e72483, the behavior\nchanged. To add insult to injury, it might be non-obvious that the pager\nis running, since with just a few tags, the pager simply exits silently.\nSo debugging this could probably be quite frustrating.\n\nMartin\n"},{"id":"330284","messageId":"20171012142424.5d76jrwxex24wsxz@sigill.intra.peff.net","threadId":"46940","inReplyTo":"20171011045404.GB16800@alpha.vpn.ikke.info","subject":"Re: [RFC] column: show auto columns when pager is active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T14:24:24Z","receivedAt":"2017-10-12T14:24:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2017 at 06:54:04AM +0200, Kevin Daudt wrote:\n\n> > > Yeah, I didn't think of that with respect to the pager. This is a\n> > > regression in v2.14.2, I think.\n> > \n> > Yes.\n> \n> Though 2.14.2 enabled the pager by default, even before that when\n> someone would've enabled the pager theirselves (by setting pager.tag for\n> example), it would also shown just a single column.\n> \n> I could reproduce it as far as 2.8.3 (I could not test further due to\n> libssl incompattibility).\n\nRight, I think it has always been broken. It's just that it became a lot\nmore widespread with the increased use of the pager.\n\nI specifically wanted to make sure this wasn't a regression in the v2.15\nrelease candidates (since that would mean we'd want to deal with it\nbefore shipping v2.15). It still _would_ be nice to deal with it soon,\nbut since it's already been in the wild for a bit, it can wait to hit\n\"maint\" in the post-v2.15 cycle.\n\n-Peff\n"},{"id":"330478","messageId":"20171016183511.12528-1-me@ikke.info","threadId":"46940","inReplyTo":"20171011172310.2932-1-me@ikke.info","subject":"[PATCH v2] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-16T18:35:11Z","receivedAt":"2017-10-16T18:35:49Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"When columns are set to automatic for git tag and the output is\npaginated by git, the output is a single column instead of multiple\ncolumns.\n\nStandard behaviour in git is to honor auto values when the pager is\nactive, which happens for example with commands like git log showing\ncolors when being paged.\n\nSince ff1e72483 (tag: change default of `pager.tag` to \"on\",\n2017-08-02), the pager has been enabled by default, exposing this\nproblem to more people.\n\nfinalize_colopts in column.c only checks whether the output is a TTY to\ndetermine if columns should be enabled with columns set to auto. Also\ncheck if the pager is active.\n\nAdding a test for git column is possible but requires some care to work\naround a race on stdin. See commit 18d8c2693 (test_terminal: redirect\nchild process' stdin to a pty, 2015-08-04). Test git tag instead, since\nthat does not involve stdin, and since that was the original motivation\nfor this patch.\n\nHelped-by: Rafael Ascensão \nSigned-off-by: Kevin Daudt \n---\nChanges since v1:\n\n- Remove redundant -p flag in tests\n- Explain why git tag is being tested instead of the more obvious git\n column\n\n column.c | 3 ++-\n t/t7006-pager.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/column.c b/column.c\nindex ff7bdab1a..ded50337f 100644\n--- a/column.c\n+++ b/column.c\n@@ -5,6 +5,7 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"utf8.h\"\n+#include \"pager.c\"\n \n #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \\\n \t\t\t (x) * (d)->rows + (y) : \\\n@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int stdout_is_tty)\n \t\tif (stdout_is_tty < 0)\n \t\t\tstdout_is_tty = isatty(1);\n \t\t*colopts &= ~COL_ENABLE_MASK;\n-\t\tif (stdout_is_tty)\n+\t\tif (stdout_is_tty || pager_in_use())\n \t\t\t*colopts |= COL_ENABLED;\n \t}\n \treturn 0;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex f0f1abd1c..e985b6873 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'git tag with auto-columns ' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four &&\n+\ttest_commit five &&\n+\tcat >expected <<\\EOF &&\n+initial one two three four five\n+EOF\n+\ttest_terminal env PAGER=\"cat >actual.tag\" COLUMNS=80 \\\n+\t\tgit -c column.ui=auto tag --sort=authordate &&\n+\ttest_cmp expected actual.tag\n+'\n+\n test_done\n-- \n2.14.2\n\n"},{"id":"330508","messageId":"xmqqtvyypikn.fsf@gitster.mtv.corp.google.com","threadId":"46940","inReplyTo":"20171016183511.12528-1-me@ikke.info","subject":"Re: [PATCH v2] column: show auto columns when pager is active","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T03:19:20Z","receivedAt":"2017-10-17T03:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> When columns are set to automatic for git tag and the output is\n> paginated by git, the output is a single column instead of multiple\n> columns.\n>\n> Standard behaviour in git is to honor auto values when the pager is\n> active, which happens for example with commands like git log showing\n> colors when being paged.\n>\n> Since ff1e72483 (tag: change default of `pager.tag` to \"on\",\n> 2017-08-02), the pager has been enabled by default, exposing this\n> problem to more people.\n>\n> finalize_colopts in column.c only checks whether the output is a TTY to\n> determine if columns should be enabled with columns set to auto. Also\n> check if the pager is active.\n>\n> Adding a test for git column is possible but requires some care to work\n> around a race on stdin. See commit 18d8c2693 (test_terminal: redirect\n> child process' stdin to a pty, 2015-08-04). Test git tag instead, since\n> that does not involve stdin, and since that was the original motivation\n> for this patch.\n\nNicely done.\n\n> +test_expect_success TTY 'git tag with auto-columns ' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three &&\n> +\ttest_commit four &&\n> +\ttest_commit five &&\n> +\tcat >expected <<\\EOF &&\n> +initial one two three four five\n> +EOF\n> +\ttest_terminal env PAGER=\"cat >actual.tag\" COLUMNS=80 \\\n> +\t\tgit -c column.ui=auto tag --sort=authordate &&\n> +\ttest_cmp expected actual.tag\n> +'\n\nI'd use <<-\\EOF so that here document can be intended like other\ntests, and also use expect vs actual that are used in the other\ntests in the same script, instead of suddenly becoming creative\nin only this single test. I can do these clean-ups locally when\nqueuing so no need to resend only to collect these.\n\nThanks, will queue.\n\n\n"},{"id":"330857","messageId":"20171023215246.r3xevx5jey5pyuuz@aiede.mtv.corp.google.com","threadId":"46940","inReplyTo":"20171016183511.12528-1-me@ikke.info","subject":"Re: [PATCH v2] column: show auto columns when pager is active","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-23T21:52:46Z","receivedAt":"2017-10-23T21:52:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nKevin Daudt wrote:\n\n> --- a/column.c\n> +++ b/column.c\n> @@ -5,6 +5,7 @@\n> #include \"parse-options.h\"\n> #include \"run-command.h\"\n> #include \"utf8.h\"\n> +#include \"pager.c\"\n\nShould this be pager.h?\n\nThanks,\nJonathan\n"},{"id":"330880","messageId":"xmqqy3o19wp5.fsf@gitster.mtv.corp.google.com","threadId":"46940","inReplyTo":"20171023215246.r3xevx5jey5pyuuz@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] column: show auto columns when pager is active","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-24T01:11:18Z","receivedAt":"2017-10-24T01:11:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> Kevin Daudt wrote:\n>\n>> --- a/column.c\n>> +++ b/column.c\n>> @@ -5,6 +5,7 @@\n>> #include \"parse-options.h\"\n>> #include \"run-command.h\"\n>> #include \"utf8.h\"\n>> +#include \"pager.c\"\n>\n> Should this be pager.h?\n\nOuch. And I was not paying enough attention.\n\nThanks, I'll queue this on top.\n\n-- >8 --\nSubject: column: do not include pager.c\n\nEverything this file needs from the pager API (e.g. term_columns(),\npager_in_use()) is already declared in the header file it includes.\n\nNoticed-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n column.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/column.c b/column.c\nindex ded50337f7..49ab85b769 100644\n--- a/column.c\n+++ b/column.c\n@@ -5,7 +5,6 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"utf8.h\"\n-#include \"pager.c\"\n \n #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \\\n \t\t\t (x) * (d)->rows + (y) : \\\n"},{"id":"330881","messageId":"20171024011803.tgigclx2swfg744r@aiede.mtv.corp.google.com","threadId":"46940","inReplyTo":"xmqqy3o19wp5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] column: show auto columns when pager is active","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-24T01:18:03Z","receivedAt":"2017-10-24T01:18:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Subject: column: do not include pager.c\n>\n> Everything this file needs from the pager API (e.g. term_columns(),\n> pager_in_use()) is already declared in the header file it includes.\n>\n> Noticed-by: Jonathan Nieder \n> Signed-off-by: Junio C Hamano \n> ---\n> column.c | 1 -\n> 1 file changed, 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n\n> --- a/column.c\n> +++ b/column.c\n> @@ -5,7 +5,6 @@\n> #include \"parse-options.h\"\n> #include \"run-command.h\"\n> #include \"utf8.h\"\n> -#include \"pager.c\"\n> \n> #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \\\n> \t\t\t (x) * (d)->rows + (y) : \\\n"},{"id":"330898","messageId":"20171024065806.GA7987@alpha.vpn.ikke.info","threadId":"46940","inReplyTo":"20171023215246.r3xevx5jey5pyuuz@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] column: show auto columns when pager is active","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-24T06:58:06Z","receivedAt":"2017-10-24T06:58:12Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Kevin Daudt wrote:\n> \n> > --- a/column.c\n> > +++ b/column.c\n> > @@ -5,6 +5,7 @@\n> > #include \"parse-options.h\"\n> > #include \"run-command.h\"\n> > #include \"utf8.h\"\n> > +#include \"pager.c\"\n> \n> Should this be pager.h?\n> \n> Thanks,\n> Jonathan\n\nThanks for catching this.\n"}]}