| Message ID | patch-1.1-765c2793122-20210624T101839Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: set COLUMNS=80 for --verbose repeatability | expand |
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
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 >
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.
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
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(-)