[PATCH] test: fix for COLUMNS and bash 5

Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) multiple tests have been failing when using
bash 5 because checkwinsize is enabled by default, therefore COLUMNS is
reset using TIOCGWINSZ even for non-interactive shells.

It's debatable whether or not bash should even be doing that, but for
now we can avoid this undesirable behavior by disabling this option.

Reported-by: Fabian Stelzer <redacted>
Signed-off-by: Felipe Contreras <redacted>
---
 t/test-lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index db61081d6b..a2b7dfecee 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -419,6 +419,12 @@ COLUMNS=80
 export LANG LC_ALL PAGER TZ COLUMNS
 EDITOR=:
 
+# Since bash 5.0, checkwinsize is enabled by default which does update the
+# COLUMNS variable every time a command completes, even for non-interactive
+# shells.
+# Disable that since we are aiming for reproducibility.
+test -n "$BASH_VERSION" && shopt -u checkwinsize 2>/dev/null
+
 # 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

-- 
2.32.0.40.gb9b36f9b52

On Thu, Aug 05 2021, Felipe Contreras wrote:

Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) multiple tests have been failing when using
bash 5 because checkwinsize is enabled by default, therefore COLUMNS is
reset using TIOCGWINSZ even for non-interactive shells.

It's debatable whether or not bash should even be doing that, but for
now we can avoid this undesirable behavior by disabling this option.

Reported-by: Fabian Stelzer <redacted>
Signed-off-by: Felipe Contreras <redacted>

I've got an alternative way of solving the same immeditate issue in[1],
there's discussion on that approach in the latest What's Cooking[2]. I
belive this approach would make at least SZEDER happier than with my
series :)

As noted in that discussion I'd prefer going for mine, but would also be
fine with this if it's what Junio decides to pick up. We should have one
or the other before 2.33 is out.

My preference for mine is in no small part that I'd like to not be
responsible for into-the-past test suite breakage the next time a
popular shell decides to be clever about COLUMNS.

But this way we'll solve the immediate problem with bash, and I can say
I told you so if that submarine breakage occurs with this approach :)

1. https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/

I don't think this needs updating, but FWIW I think that around bash 4.X
(I think 4.1, but that's from memory) is where it started doing this for
non-interactive shells, so we could have had breakage going back that
far.

But we're seeing this in practice now because with 5.0 this was turned
on by default.

I.e. I think if you turn on checkwinsize it'll also break with the tests
on bash 4.1 (or whatever it was), but not on 3.x.

None of that's something I've tested, just from memory of skimming the
bash commit logs for changes related to checkwinsize a some days ago
when I came up with my fix for this.

On Thu, Aug 05, 2021 at 02:48:25PM -0500, Felipe Contreras wrote:

COLUMNS is set just before the start of the hunk context ...

 export LANG LC_ALL PAGER TZ COLUMNS
 EDITOR=:

... so these two "commands" above are executed while COLUMNS is
already set but checkwinsize is not yet disabled.  The reason I put
quotes around that commands is that while exporting and setting
variables are indeed commands as defined in the POSIX Shell Command
Language specs, Bash with checkwinsize enabled only "checks the window
size after each extern (non-builtin) command" (quoting 'man bash').

So even though it is safe to execute these variable setting and
exporting commands after setting COLUMNS but disabling checkwinsize, I
think it would be prudent to disable checkwinsize before initializing
COLUMNS.  (And perhaps adding "non-builtin" to the comment below.)

+# Since bash 5.0, checkwinsize is enabled by default which does update the
+# COLUMNS variable every time a command completes, even for non-interactive
+# shells.
+# Disable that since we are aiming for reproducibility.
+test -n "$BASH_VERSION" && shopt -u checkwinsize 2>/dev/null
+
 # 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
-- 
2.32.0.40.gb9b36f9b52

Ævar Arnfjörð Bjarmason wrote:

On Thu, Aug 05 2021, Felipe Contreras wrote:

quoted

Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) multiple tests have been failing when using
bash 5 because checkwinsize is enabled by default, therefore COLUMNS is
reset using TIOCGWINSZ even for non-interactive shells.

It's debatable whether or not bash should even be doing that, but for
now we can avoid this undesirable behavior by disabling this option.

Reported-by: Fabian Stelzer <redacted>
Signed-off-by: Felipe Contreras <redacted>

I've got an alternative way of solving the same immeditate issue in[1],
there's discussion on that approach in the latest What's Cooking[2].

Yes, but I'm not allowed to participate in that discussion.

My preference for mine is in no small part that I'd like to not be
responsible for into-the-past test suite breakage the next time a
popular shell decides to be clever about COLUMNS.

But this way we'll solve the immediate problem with bash, and I can say
I told you so if that submarine breakage occurs with this approach :)

I believe what bash is doing is a mistake. I don't think COLUMNS should
be updated within a non-interactive shell by default. I'll report that
as a bug once I'm able to subsscribe to the bug-bash mailing list.

So I think it's the opposite: not only would similar fixes not be needed
for other shells, but it won't be needed for bash either.

-- 
Felipe Contreras

Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) multiple tests have been failing when using
bash 5 because checkwinsize is enabled by default, therefore COLUMNS is
reset using TIOCGWINSZ even for non-interactive shells.

It's debatable whether or not bash should even be doing that, but for
now we can avoid this undesirable behavior by disabling this option.

Reported-by: Fabian Stelzer <redacted>
Signed-off-by: Felipe Contreras <redacted>
---

Since v1 moved the code before setting COLUMNS as SZEDER Gábor suggested
and mention checkwinsize could be set before bash 5 as Ævar Arnfjörð
Bjarmason mentioned.

Range-diff against v1:
1:  40273074de < -:  ---------- test: fix for COLUMNS and bash 5
-:  ---------- > 1:  9f8c3ffa6a test: fix for COLUMNS and bash 5

 t/test-lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index db61081d6b..6b1015a5af 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -409,6 +409,12 @@ then
 	verbose=t
 fi
 
+# In bash if checkwinsize is enabled the COLUMNS variable is updated every time
+# an external command completes, even for non-interactive shells. Since bash 5.0
+# this is enabled by default.
+# Disable that since we are aiming for reproducibility.
+test -n "$BASH_VERSION" && shopt -u checkwinsize 2>/dev/null
+
 # For repeatability, reset the environment to known value.
 # TERM is sanitized below, after saving color control sequences.
 LANG=C

-- 
2.32.0.40.gb9b36f9b52

Felipe Contreras [off-list ref] 于2021年8月7日周六 上午12:44写道：

This can work on Arch-Linux. LGTM.

Thanks.
--
ZheNing Hu