diff mbox series

[v2,3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

Message ID patch-v2-3.3-6cbbb955e9a-20210802T134610Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS | expand

Commit Message

Ævar Arnfjörð Bjarmason Aug. 2, 2021, 1:46 p.m. UTC
In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) the test suite started breaking on recent
versions of bash.

This is because it sets "shopt -s checkwinsize" starting with version
5.0, furthermore it started setting COLUMNS under "shopt -s
checkwinsize" for non-interactive use around version 4.3.

A narrow fix for that issue would be to add this just above our
setting of COLUMNS in test-lib.sh:

	shopt -u checkwinsize >/dev/null 2>&1

But we'd then be at the mercy of the next shell or terminal that wants
to be clever about COLUMNS.

Let's instead solve this more thoroughly. We'll now take
GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
COLUMNS variable to break any tests that rely on it being set to a
sane value.

If something breaks because we have a codepath that's not
term_columns() checking COLUMNS we'd like to know about it, the narrow
"shopt -u checkwinsize" fix won't give us that.

The "shopt" fix won't future-poof us against other 3rd party software
changes either. If that third-party software e.g. takes TIOCGWINSZ
over columns on some platforms, our tests would be flaky and break
there even without this change.

This approach does mean that any tests of ours that expected to test
term_columns() behavior by setting COLUMNS will need to explicitly
unset GIT_TEST_COLUMNS, or set it to the empty string. Let's do that
in the new test_with_columns() helper, which we previously changed all
the tests that set COLUMNS to use.

Reported-by: Fabian Stelzer <fabian.stelzer@campoint.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c                 |  7 +++++++
 t/test-lib-functions.sh |  7 +++++++
 t/test-lib.sh           | 13 +++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)
diff mbox series

Patch

diff --git a/pager.c b/pager.c
index 52f27a6765c..cfcc6dc04bd 100644
--- a/pager.c
+++ b/pager.c
@@ -165,6 +165,13 @@  int term_columns(void)
 	term_columns_at_startup = 80;
 	term_columns_guessed = 1;
 
+	col_string = getenv("GIT_TEST_COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0) {
+		term_columns_at_startup = n_cols;
+		term_columns_guessed = 0;
+		return term_columns_at_startup;
+	}
+
 	col_string = getenv("COLUMNS");
 	if (col_string && (n_cols = atoi(col_string)) > 0) {
 		term_columns_at_startup = n_cols;
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 536339faaa2..959354e0c6e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1724,5 +1724,12 @@  test_with_columns () {
 	local columns=$1
 	shift
 
+	if ! is_git_command_name "$@"
+	then
+		echo >&7 "test_with_columns: only 'git' is allowed: $*"
+		return 1
+	fi
+
+	GIT_TEST_COLUMNS= \
 	COLUMNS=$columns "$@"
 }
diff --git a/t/test-lib.sh b/t/test-lib.sh
index da13190970f..4bb231e5729 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -415,10 +415,19 @@  LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-COLUMNS=80
-export LANG LC_ALL PAGER TZ COLUMNS
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 
+# For repeatability we need to set term_columns()'s idea of
+# columns. We do this via GIT_TEST_COLUMNS and not COLUMNS because
+# e.g. versions of bash >= 5.0 have "shopt -s checkwinsize" on by
+# default. We could do "shopt -u checkwinsize >/dev/null 2>&1" here to
+# fix that particular issue, but this is not shell specific, and
+# future-proof the tests.
+GIT_TEST_COLUMNS=80
+COLUMNS=10
+export GIT_TEST_COLUMNS COLUMNS
+
 # 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