* [PATCH] progress.c tests: fix breakage with COLUMNS != 80
@ 2021-06-21 7:01 Ævar Arnfjörð Bjarmason
2021-06-23 23:48 ` Jeff King
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 7:01 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor,
Æ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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80
2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason
@ 2021-06-23 23:48 ` Jeff King
2021-06-24 5:12 ` SZEDER Gábor
2021-06-24 15:05 ` Philip Oakley
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
1 sibling, 2 replies; 10+ messages in thread
From: Jeff King @ 2021-06-23 23:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, SZEDER Gábor
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80
2021-06-23 23:48 ` Jeff King
@ 2021-06-24 5:12 ` SZEDER Gábor
2021-06-24 5:40 ` Jeff King
2021-06-24 15:05 ` Philip Oakley
1 sibling, 1 reply; 10+ messages in thread
From: SZEDER Gábor @ 2021-06-24 5:12 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano
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'.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80
2021-06-24 5:12 ` SZEDER Gábor
@ 2021-06-24 5:40 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2021-06-24 5:40 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability
2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason
2021-06-23 23:48 ` Jeff King
@ 2021-06-24 10:19 ` Ævar Arnfjörð Bjarmason
2021-06-24 14:50 ` Jeff King
` (2 more replies)
1 sibling, 3 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-24 10:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
Æ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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
@ 2021-06-24 14:50 ` Jeff King
2021-06-27 7:44 ` SZEDER Gábor
2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2021-06-24 14:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, 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.
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80
2021-06-23 23:48 ` Jeff King
2021-06-24 5:12 ` SZEDER Gábor
@ 2021-06-24 15:05 ` Philip Oakley
1 sibling, 0 replies; 10+ messages in thread
From: Philip Oakley @ 2021-06-24 15:05 UTC (permalink / raw)
To: Jeff King, Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, SZEDER Gábor
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
2021-06-24 14:50 ` Jeff King
@ 2021-06-27 7:44 ` SZEDER Gábor
2021-06-29 1:42 ` Junio C Hamano
2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 10+ messages in thread
From: SZEDER Gábor @ 2021-06-27 7:44 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, 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.
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability
2021-06-27 7:44 ` SZEDER Gábor
@ 2021-06-29 1:42 ` Junio C Hamano
0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2021-06-29 1:42 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Ævar Arnfjörð Bjarmason, git, Jeff King
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] test-lib.sh: set COLUMNS=80 for --verbose repeatability
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
2021-06-24 14:50 ` Jeff King
2021-06-27 7:44 ` SZEDER Gábor
@ 2021-06-29 11:29 ` Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
Æ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
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-29 11:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason
2021-06-23 23:48 ` Jeff King
2021-06-24 5:12 ` SZEDER Gábor
2021-06-24 5:40 ` Jeff King
2021-06-24 15:05 ` Philip Oakley
2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason
2021-06-24 14:50 ` Jeff King
2021-06-27 7:44 ` SZEDER Gábor
2021-06-29 1:42 ` Junio C Hamano
2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.