Re: [PATCH] test: fix for COLUMNS and bash 5

SZEDER Gábor [off-list ref] writes:

quoted

@@ -419,6 +419,12 @@ COLUMNS=80

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

quoted

 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.)

OK.  I tend to agree that a less invasive solution like this is
preferred over adding new code to only help tests in the everyday
binary, especially this close to the final.  Taking the above, I'd
queue this and hopefully I can merge it by -rc2 at the latest.

---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----
From: Felipe Contreras <redacted>
Date: Thu, 5 Aug 2021 14:48:25 -0500
Subject: [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>
[jc: with SZEDER Gábor's suggestion to do this before setting COLUMNS]
Signed-off-by: Junio C Hamano <redacted>
---
 t/test-lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4d4439a917..52701afaea 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -395,6 +395,12 @@ then
 	verbose=t
 fi
 
+# Since bash 5.0, checkwinsize is enabled by default which does
+# update the COLUMNS variable every time a non-builtin command
+# completes, even for non-interactive shells.
+# Disable that since we are aiming for repeatability.
+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.33.0-rc0-215-ge3efc65752

On Fri, Aug 06 2021, Junio C Hamano wrote:

SZEDER Gábor [off-list ref] writes:

quoted

quoted

@@ -419,6 +419,12 @@ COLUMNS=80

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

quoted

 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.)

OK.  I tend to agree that a less invasive solution like this is
preferred over adding new code to only help tests in the everyday
binary, especially this close to the final.  Taking the above, I'd
queue this and hopefully I can merge it by -rc2 at the latest.

Now that we're post-release are you interested in a re-roll of
https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/
+ removal of the bash-specific checkwinsize added here, or would you
like to just keep Felipe's version which you integrated as 390b44eb2bc
(test: fix for COLUMNS and bash 5, 2021-08-05).

Either works for me, just poking to see whether I should drop this from
my local TODO list or not...