[PATCH] progress.c tests: fix breakage with COLUMNS != 80

[PATCH] progress.c tests: fix breakage with COLUMNS != 80

[Ævar Arnfjörð Bjarmason]

The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
broke under anything except COLUMNS=80, i.e. when running them under
the "-v" mode under a differently sized terminal.

Let's set the expected number of COLUMNS at the start of the test to
fix that bug. It's handy not do do this in test-progress.c itself, in
case we'd like to test for a different number of COLUMNS, either
manually or in a future test.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0500-progress-display.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 22058b503ac..66c092a0fe3 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -8,6 +8,11 @@ show_cr () {
 	tr '\015' Q | sed -e "s/Q/<CR>\\$LF/g"
 }
 
+test_expect_success 'setup COLUMNS' '
+	COLUMNS=80 &&
+	export COLUMNS
+'
+
 test_expect_success 'simple progress display' '
 	cat >expect <<-\EOF &&
 	Working hard: 1<CR>
-- 
2.32.0.595.g55105f1e212

Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80

[Jeff King]

On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:

> The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
> broke under anything except COLUMNS=80, i.e. when running them under
> the "-v" mode under a differently sized terminal.
> 
> Let's set the expected number of COLUMNS at the start of the test to
> fix that bug. It's handy not do do this in test-progress.c itself, in
> case we'd like to test for a different number of COLUMNS, either
> manually or in a future test.

Hmm. So I can easily reproduce the problem here, and your patch fixes
it. But my first thought was: shouldn't test-lib.sh be handling this to
give all of the scripts a uniform environment?

And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
a bad setting of $COLUMNS, but rather that in "-v" mode, the
sub-command's stderr is hooked to our tty, and term_columns() is smart
enough to use TIOCGWINSZ to get the value (at least on some platforms).

Setting $COLUMNS again in the environment fixes it, because we prefer
that value to trying the ioctl.

So I don't think what you have here is wrong (though the commit message
is a little misleading). But it seems like the original intent of our
"unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
a known value there (like 80), rather than unsetting it.

I admit this a _bit_ of a nitpick (since as far as we know none of the
other scripts care about the terminal width), so I'm OK with this as-is
if you feel strongly the other way.

-Peff

Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80

[SZEDER Gábor]

On Wed, Jun 23, 2021 at 07:48:25PM -0400, Jeff King wrote:
> On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
> > broke under anything except COLUMNS=80, i.e. when running them under
> > the "-v" mode under a differently sized terminal.
> > 
> > Let's set the expected number of COLUMNS at the start of the test to
> > fix that bug. It's handy not do do this in test-progress.c itself, in
> > case we'd like to test for a different number of COLUMNS, either
> > manually or in a future test.
> 
> Hmm. So I can easily reproduce the problem here, and your patch fixes
> it. But my first thought was: shouldn't test-lib.sh be handling this to
> give all of the scripts a uniform environment?
> 
> And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
> a bad setting of $COLUMNS, but rather that in "-v" mode, the
> sub-command's stderr is hooked to our tty, and term_columns() is smart
> enough to use TIOCGWINSZ to get the value (at least on some platforms).
> 
> Setting $COLUMNS again in the environment fixes it, because we prefer
> that value to trying the ioctl.
> 
> So I don't think what you have here is wrong (though the commit message
> is a little misleading).

It is misleading indeed and needs to be updated.  I did my own
analysis and arrived to the same conclusions wrt COLUMNS being unset
vs. the ioctl() and stderr being a tty.

> But it seems like the original intent of our
> "unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
> a known value there (like 80), rather than unsetting it.
> 
> I admit this a _bit_ of a nitpick (since as far as we know none of the
> other scripts care about the terminal width), so I'm OK with this as-is
> if you feel strongly the other way.

I remember one commit-graph test that does check the number of lines
in the progress output, assuming one progress line per commit-graph
layer, which breaks when we break the progress line in a too narrow
terminal.  Running './t5324-split-commit-graph.sh -v -i' in a 46
column wide terminal fails for me, but succeeds with 47 columns.

I do suggest setting COLUMNS=80 in 'test-lib.sh'.

Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80

[Jeff King]

On Thu, Jun 24, 2021 at 07:12:53AM +0200, SZEDER Gábor wrote:

> > I admit this a _bit_ of a nitpick (since as far as we know none of the
> > other scripts care about the terminal width), so I'm OK with this as-is
> > if you feel strongly the other way.
> 
> I remember one commit-graph test that does check the number of lines
> in the progress output, assuming one progress line per commit-graph
> layer, which breaks when we break the progress line in a too narrow
> terminal.  Running './t5324-split-commit-graph.sh -v -i' in a 46
> column wide terminal fails for me, but succeeds with 47 columns.
> 
> I do suggest setting COLUMNS=80 in 'test-lib.sh'.

Thanks for providing a concrete example. I agree we should just set it
for all scripts via test-lib.sh, then.

-Peff

[PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability

[Ævar Arnfjörð Bjarmason]

Some tests will fail under --verbose because while we've unset COLUMNS
since b1d645b58ac (tests: unset COLUMNS inherited from environment,
2012-03-27), we also look for the columns with an ioctl(..,
TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we
preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take
COLUMNS over TIOCGWINSZ,

This fixes the t0500-progress-display.sh test when run as:

    ./t0500-progress-display.sh --verbose

It broke because of a combination of the this issue and the progress
output reacting to the column width since 545dc345ebd (progress: break
too long progress bar lines, 2019-04-12). The
t5324-split-commit-graph.sh fails in a similar manner due to progress
output, see [1] for details.

A more narrow fix here would be to only do this in the --verbose mode,
but there's no harm in setting this for everything. If we're not
connected to a TTY the COLUMNS setting won't matter.

See ea77e675e56 (Make "git help" react to window size correctly,
2005-12-18) and ad6c3739a33 (pager: find out the terminal width before
spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up
in pager.c

1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This replaces a more narrow fix in
https://lore.kernel.org/git/patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com/,
which as SZEDER points out was also needed by another test using the
progress.c code.

 t/test-lib.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54938c64279..1a6ca772d6c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -406,14 +406,15 @@ LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-export LANG LC_ALL PAGER TZ
+COLUMNS=80
+export LANG LC_ALL PAGER TZ COLUMNS
 EDITOR=:
 
 # A call to "unset" with no arguments causes at least Solaris 10
 # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
 # deriving from the command substitution clustered with the other
 # ones.
-unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
+unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 	my @env = keys %ENV;
 	my $ok = join("|", qw(
 		TRACE
-- 
2.32.0.605.g06ef811e153

Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability

[Jeff King]

On Thu, Jun 24, 2021 at 12:19:31PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Some tests will fail under --verbose because while we've unset COLUMNS
> since b1d645b58ac (tests: unset COLUMNS inherited from environment,
> 2012-03-27), we also look for the columns with an ioctl(..,
> TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we
> preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take
> COLUMNS over TIOCGWINSZ,
> 
> This fixes the t0500-progress-display.sh test when run as:
> 
>     ./t0500-progress-display.sh --verbose
> 
> It broke because of a combination of the this issue and the progress
> output reacting to the column width since 545dc345ebd (progress: break
> too long progress bar lines, 2019-04-12). The
> t5324-split-commit-graph.sh fails in a similar manner due to progress
> output, see [1] for details.
> 
> A more narrow fix here would be to only do this in the --verbose mode,
> but there's no harm in setting this for everything. If we're not
> connected to a TTY the COLUMNS setting won't matter.

Thanks, this explanation and location make sense to me.

(minor s/the this/this/ in the third paragraph above).

-Peff

Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80

[Philip Oakley]

On 24/06/2021 00:48, Jeff King wrote:
> On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
>> broke under anything except COLUMNS=80, i.e. when running them under
>> the "-v" mode under a differently sized terminal.
>>
>> Let's set the expected number of COLUMNS at the start of the test to
>> fix that bug. It's handy not do do this in test-progress.c itself, in
>> case we'd like to test for a different number of COLUMNS, either
>> manually or in a future test.
> Hmm. So I can easily reproduce the problem here, and your patch fixes
> it. But my first thought was: shouldn't test-lib.sh be handling this to
> give all of the scripts a uniform environment?
>
> And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
> a bad setting of $COLUMNS, but rather that in "-v" mode, the
> sub-command's stderr is hooked to our tty, and term_columns() is smart
> enough to use TIOCGWINSZ to get the value (at least on some platforms).
>
> Setting $COLUMNS again in the environment fixes it, because we prefer
> that value to trying the ioctl.
>
> So I don't think what you have here is wrong (though the commit message
> is a little misleading). But it seems like the original intent of our
> "unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
> a known value there (like 80), rather than unsetting it.
>
> I admit this a _bit_ of a nitpick (since as far as we know none of the
> other scripts care about the terminal width), so I'm OK with this as-is
> if you feel strongly the other way.
>
> -Peff
There has been a similar recent issue [1] on Git for Windows regarding
these settings where different terminals had different views  about the
size of the display window which resulted in incorrect out puts.

Philip

[1] https://github.com/git-for-windows/git/issues/3235

Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability

[SZEDER Gábor]

On Thu, Jun 24, 2021 at 12:19:31PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Some tests will fail under --verbose because while we've unset COLUMNS
> since b1d645b58ac (tests: unset COLUMNS inherited from environment,
> 2012-03-27), we also look for the columns with an ioctl(..,
> TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we
> preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take
> COLUMNS over TIOCGWINSZ,
> 
> This fixes the t0500-progress-display.sh test when run as:
> 
>     ./t0500-progress-display.sh --verbose
> 
> It broke because of a combination of the this issue and the progress
> output reacting to the column width since 545dc345ebd (progress: break
> too long progress bar lines, 2019-04-12). The
> t5324-split-commit-graph.sh fails in a similar manner due to progress
> output, see [1] for details.

This issue is not progress-specific.

I run the test suite with 'export COLUMNS=10' in 'test-lib.sh' and
got:

  t0500-progress-display.sh                (Wstat: 256 Tests: 12 Failed: 10)
    Failed tests:  1-5, 7-11
  t4012-diff-binary.sh                     (Wstat: 256 Tests: 13 Failed: 1)
    Failed test:  13
  t4052-stat-output.sh                     (Wstat: 256 Tests: 78 Failed: 14)
    Failed tests:  3-5, 13, 17, 21, 38-41, 49, 52, 55, 57
  t5510-fetch.sh                           (Wstat: 256 Tests: 181 Failed: 1)
    Failed test:  175
  t5324-split-commit-graph.sh              (Wstat: 256 Tests: 36 Failed: 1)
    Failed test:  22
  t5150-request-pull.sh                    (Wstat: 256 Tests: 10 Failed: 1)
    Failed test:  6
  t4016-diff-quote.sh                      (Wstat: 256 Tests: 5 Failed: 1)
    Failed test:  5

Then with COLUMNS=238:

  t0500-progress-display.sh                (Wstat: 256 Tests: 12 Failed: 4)
    Failed tests:  3-6
  t4012-diff-binary.sh                     (Wstat: 256 Tests: 13 Failed: 1)
    Failed test:  13
  t4052-stat-output.sh                     (Wstat: 256 Tests: 78 Failed: 3)
    Failed tests:  3-5

From these only the failures in t0500 and t5324 are caused by the
progress display, the rest fail with things like:

  --- expect      2021-06-26 17:07:58.489958439 +0000
  +++ actual      2021-06-26 17:07:58.957964694 +0000
  @@ -1,2 +1,2 @@
    binfile  |   Bin 0 -> 1026 bytes
  - textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  + textfile | 10000 ++++++++
  
  --- expect80    2021-06-26 17:07:58.321956193 +0000
  +++ actual      2021-06-26 17:07:58.333956354 +0000
  @@ -1 +1 @@
  - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
  + ...aaaaaaaaaaaa | 1 +
  
  --- expect      2021-06-26 17:09:05.198849586 +0000
  +++ actual      2021-06-26 17:09:05.194849532 +0000
  @@ -1,2 +1,2 @@
  -main                 -> origin/main
  +main       -> origin/main
   looooooooooooong-tag -> looooooooooooong-tag
  
  --- expect      2021-06-26 17:14:31.819199273 +0000
  +++ request.fuzzy       2021-06-26 17:14:31.951201026 +0000
  @@ -16,4 +16,5 @@
   ----------------------------------------------------------------
   SHORTLOG
  
  -DIFFSTAT
  + ...nic.txt | 5 +++++
  + 1 file changed, 5 insertions(+)
  
  
  --- expect      2021-06-26 16:55:41.632510754 +0000
  +++ actual      2021-06-26 16:55:42.052516149 +0000
  @@ -1,2 +1,2 @@
    binfile  |   Bin 0 -> 1026 bytes
  - textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  + textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  
  --- expect80    2021-06-26 16:55:41.416507979 +0000
  +++ actual      2021-06-26 16:55:41.436508236 +0000
  @@ -1 +1 @@
  - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
  + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +

So there are more diffstat-related failures than progress-related.

> A more narrow fix here would be to only do this in the --verbose mode,
> but there's no harm in setting this for everything. If we're not
> connected to a TTY the COLUMNS setting won't matter.

This is wrong, we check the terminal width even when not connected to
a TTY, therefore COLUMNS definitely matters; all the failures reported
above were with '--verbose-log'.

> See ea77e675e56 (Make "git help" react to window size correctly,
> 2005-12-18) and ad6c3739a33 (pager: find out the terminal width before
> spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up
> in pager.c
> 
> 1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> This replaces a more narrow fix in
> https://lore.kernel.org/git/patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com/,
> which as SZEDER points out was also needed by another test using the
> progress.c code.
> 
>  t/test-lib.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 54938c64279..1a6ca772d6c 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -406,14 +406,15 @@ LANG=C
>  LC_ALL=C
>  PAGER=cat
>  TZ=UTC
> -export LANG LC_ALL PAGER TZ
> +COLUMNS=80
> +export LANG LC_ALL PAGER TZ COLUMNS
>  EDITOR=:
>  
>  # A call to "unset" with no arguments causes at least Solaris 10
>  # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
>  # deriving from the command substitution clustered with the other
>  # ones.
> -unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
> +unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
>  	my @env = keys %ENV;
>  	my $ok = join("|", qw(
>  		TRACE
> -- 
> 2.32.0.605.g06ef811e153
> 

Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> This issue is not progress-specific.

As discussed elsewhere with Dscho on a Windows specific workaround,
progress is not the only consumer of term_columns() and the code
being "fixed" here would affect other things like "diff --stat" and
hunk header title (aka funcname).

Together with a typo Peff pointed out, the patch deserves an updated
log message before it becomes ready.  The patch text looks good.

Thanks.

[PATCH v2] test-lib.sh: set COLUMNS=80 for --verbose repeatability

[Ævar Arnfjörð Bjarmason]

Some tests will fail under --verbose because while we've unset COLUMNS
since b1d645b58ac (tests: unset COLUMNS inherited from environment,
2012-03-27), we also look for the columns with an ioctl(..,
TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we
preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take
COLUMNS over TIOCGWINSZ,

This fixes t0500-progress-display.sh., which broke because of a
combination of the this issue and the progress output reacting to the
column width since 545dc345ebd (progress: break too long progress bar
lines, 2019-04-12). The t5324-split-commit-graph.sh fails in a similar
manner due to progress output, see [1] for details.

The issue is not specific to progress.c, the diff code also checks
COLUMNS and some of its tests can be made to fail in a similar
manner[2], anything that invokes a pager is potentially affected.

See ea77e675e56 (Make "git help" react to window size correctly,
2005-12-18) and ad6c3739a33 (pager: find out the terminal width before
spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up
in pager.c

1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev
2. https://lore.kernel.org/git/20210627074419.GH6312@szeder.dev/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

No code changes since v1, just the suggested commit message change.

Range-diff against v1:
1:  765c279312 ! 1:  bd2bd89a2b test-lib.sh: set COLUMNS=80 for --verbose repeatability
    @@ Commit message
         preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take
         COLUMNS over TIOCGWINSZ,
     
    -    This fixes the t0500-progress-display.sh test when run as:
    +    This fixes t0500-progress-display.sh., which broke because of a
    +    combination of the this issue and the progress output reacting to the
    +    column width since 545dc345ebd (progress: break too long progress bar
    +    lines, 2019-04-12). The t5324-split-commit-graph.sh fails in a similar
    +    manner due to progress output, see [1] for details.
     
    -        ./t0500-progress-display.sh --verbose
    -
    -    It broke because of a combination of the this issue and the progress
    -    output reacting to the column width since 545dc345ebd (progress: break
    -    too long progress bar lines, 2019-04-12). The
    -    t5324-split-commit-graph.sh fails in a similar manner due to progress
    -    output, see [1] for details.
    -
    -    A more narrow fix here would be to only do this in the --verbose mode,
    -    but there's no harm in setting this for everything. If we're not
    -    connected to a TTY the COLUMNS setting won't matter.
    +    The issue is not specific to progress.c, the diff code also checks
    +    COLUMNS and some of its tests can be made to fail in a similar
    +    manner[2], anything that invokes a pager is potentially affected.
     
         See ea77e675e56 (Make "git help" react to window size correctly,
         2005-12-18) and ad6c3739a33 (pager: find out the terminal width before
    @@ Commit message
         in pager.c
     
         1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev
    +    2. https://lore.kernel.org/git/20210627074419.GH6312@szeder.dev/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     

 t/test-lib.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54938c6427..1a6ca772d6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -406,14 +406,15 @@ LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-export LANG LC_ALL PAGER TZ
+COLUMNS=80
+export LANG LC_ALL PAGER TZ COLUMNS
 EDITOR=:
 
 # A call to "unset" with no arguments causes at least Solaris 10
 # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
 # deriving from the command substitution clustered with the other
 # ones.
-unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
+unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 	my @env = keys %ENV;
 	my $ok = join("|", qw(
 		TRACE
-- 
2.32.0.615.g90fb4d7369