[PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 2, 2021, 13:46 UTC
- Message-ID
- <patch-v2-3.3-6cbbb955e9a-20210802T134610Z-avarab@gmail.com>
- In-Reply-To
- <cover-v2-0.3-00000000000-20210802T134610Z-avarab@gmail.com>
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 --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
-- 2.32.0.1070.gec115ccd780