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

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability
Date: Sun, 27 Jun 2021 09:44:19 +0200	[thread overview]
Message-ID: <20210627074419.GH6312@szeder.dev> (raw)
In-Reply-To: <patch-1.1-765c2793122-20210624T101839Z-avarab@gmail.com>

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
> 