Re: [PATCH] t5582: remove spurious 'cd "$D"' line

From: Junio C Hamano <hidden>
Date: 2021-08-30 16:42:10

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

On Wed, Aug 25, 2021 at 09:12:37AM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

Hmm. I think that one is different, in that the "cd" is not redundant,
but wrong. But it turns out not to matter to the test. ;)

Funny. 

We are lucky because 'cd ""' stays in the same repository as the
current one and not to a random place,

Actually, the results of 'cd ""' are unspecified, though most shells
do as you said.  Do we want something like this?

I doubt it, as the root issue is not "cd" but "$D" and other
variables that we use before setting.

I wonder how close our test suite is for being "set -u" clean.
Running our tests under "set -u" may not be a bad endpoint, but
only if we can get there without too much pain.

Thanks.

Re: [PATCH] t5582: remove spurious 'cd "$D"' line

From: Jeff King <hidden>
Date: 2021-08-30 18:58:57

On Mon, Aug 30, 2021 at 09:42:03AM -0700, Junio C Hamano wrote:

I wonder how close our test suite is for being "set -u" clean.
Running our tests under "set -u" may not be a bad endpoint, but
only if we can get there without too much pain.

I wondered, too, but I don't think we're very close.

Just throwing "set -u" at the top of test-lib.sh shows many issues:

  - we don't initialize some known variables, like say, verbose_only. It
    might be reasonable to have a big list of:

      verbose_only=
      verbose=

    etc. That would probably be an improvement, though a slight
    maintenance burden.

  - reading optional environment variables needs to be careful. You
    can't just say:

      if test -z "$GIT_TEST_CMP"

    but need:

      if test -z "${GIT_TEST_CMP:-}"

    It's doubly annoying when you're passing it to "test -z", of course,
    since the whole point is to see if it's set. In this case you can't
    just initialize to empty, since we might get the variable from the
    environment. I guess you could do:

      : ${GIT_TEST_CMP:=}

    near the top of the script to pre-declare all such variables.

  - optional arguments in functions need to be annotated. Like:

      foo=${2:-}

    Kind of annoying, but at least it communicates the intent in some
    ways.

Below is a diff that gets "./t0001-init.sh" running. But:

  - I have no illusions that there aren't tons of problems still
    lurking. All of these spots were found by running repeatedly and
    fixing up any errors. So whatever code paths I didn't run are
    probably full of similar bugs.

  - Most of the fixes were in test-lib.sh, so they'd apply to the other
    tests. If that's the extent, then maybe it's not so bad. But it's
    not clear how many individual scripts will need to be fixed, too
    (and likewise, for script authors to remain aware as they write new
    scripts). t0000, for example, fails half of its tests.

So I dunno. It could help find some bugs, but it seems like a
non-trivial burden.

 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 75 ++++++++++++++++++++++-------------
 2 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75..c1449fb91c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -1518,7 +1518,7 @@ test_detect_hash () {
 # Load common hash metadata and common placeholder object IDs for use with
 # test_oid.
 test_oid_init () {
-	test -n "$test_hash_algo" || test_detect_hash &&
+	test -n "${test_hash_algo:-}" || test_detect_hash &&
 	test_oid_cache <"$TEST_DIRECTORY/oid-info/hash-info" &&
 	test_oid_cache <"$TEST_DIRECTORY/oid-info/oid"
 }

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d6..72344ddf78 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -15,9 +15,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .
 
+set -u
+
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
-if test -z "$TEST_DIRECTORY"
+if test -z "${TEST_DIRECTORY:-}"
 then
 	# We allow tests to override this, in case they want to run tests
 	# outside of t/, e.g. for running tests on the test library

@@ -28,7 +30,7 @@ else
 	# is valid even if the current working directory is changed
 	TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
 fi
-if test -z "$TEST_OUTPUT_DIRECTORY"
+if test -z "${TEST_OUTPUT_DIRECTORY:-}"
 then
 	# Similarly, override this to store the test-results subdir
 	# elsewhere

@@ -61,13 +63,13 @@ export PERL_PATH SHELL_PATH
 # the developer has TEST_OUTPUT_DIRECTORY part of his build options, then we'd
 # reset this value to instead contain what the developer has specified. We thus
 # have this knob to allow overriding the directory.
-if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE}"
+if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE:-}"
 then
 	TEST_OUTPUT_DIRECTORY="${TEST_OUTPUT_DIRECTORY_OVERRIDE}"
 fi
 
 # Disallow the use of abbreviated options in the test suite by default
-if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:-}"
 then
 	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
 	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS

@@ -204,6 +206,24 @@ parse_option () {
 	esac
 }
 
+debug=
+help=
+immediate=
+quiet=
+run_list=
+root=
+stress=
+tee=
+trace=
+valgrind=
+valgrind_only=
+verbose=
+verbose_log=
+verbose_only=
+write_junit_xml=
+
+skip_all=
+
 # Parse options while taking care to leave $@ intact, so we will still
 # have all the original command line options when executing the test
 # script again for '--tee' and '--verbose-log' later.

@@ -271,7 +291,7 @@ case "$TRASH_DIRECTORY" in
 esac
 
 # If --stress was passed, run this test repeatedly in several parallel loops.
-if test "$GIT_TEST_STRESS_STARTED" = "done"
+if test "${GIT_TEST_STRESS_STARTED:-}" = "done"
 then
 	: # Don't stress test again.
 elif test -n "$stress"

@@ -359,7 +379,7 @@ fi
 
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
-if test "$GIT_TEST_TEE_STARTED" = "done"
+if test "${GIT_TEST_TEE_STARTED:-}" = "done"
 then
 	: # do not redirect again
 elif test -n "$tee"

@@ -391,7 +411,7 @@ then
 	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
 	# '--verbose-log', so the right shell is checked and the
 	# warning is issued only once.
-	if test -n "$BASH_VERSION" && eval '
+	if test -n "${BASH_VERSION:-}" && eval '
 	     test ${BASH_VERSINFO[0]} -gt 4 || {
 	       test ${BASH_VERSINFO[0]} -eq 4 &&
 	       test ${BASH_VERSINFO[1]} -ge 1

@@ -413,7 +433,7 @@ fi
 # 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
+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.

@@ -483,7 +503,7 @@ then
 	export GIT_INDEX_VERSION
 fi
 
-if test -n "$GIT_TEST_PERL_FATAL_WARNINGS"
+if test -n "${GIT_TEST_PERL_FATAL_WARNINGS:-}"
 then
 	GIT_PERL_FATAL_WARNINGS=1
 	export GIT_PERL_FATAL_WARNINGS

@@ -492,7 +512,7 @@ fi
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if test -n "$valgrind" ||
-   test -n "$TEST_NO_MALLOC_CHECK"
+   test -n "${TEST_NO_MALLOC_CHECK:-}"
 then
 	setup_malloc_check () {
 		: nothing

@@ -517,7 +537,7 @@ unset CDPATH
 unset GREP_OPTIONS
 unset UNZIP
 
-case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
+case $(echo ${GIT_TRACE:-} |tr "[A-Z]" "[a-z]") in
 1|2|true)
 	GIT_TRACE=4
 	;;

@@ -603,7 +623,7 @@ say () {
 	say_color info "$*"
 }
 
-if test -n "$HARNESS_ACTIVE"
+if test -n "${HARNESS_ACTIVE:-}"
 then
 	if test "$verbose" = t || test -n "$verbose_only"
 	then

@@ -892,12 +912,12 @@ maybe_setup_verbose () {
 }
 
 maybe_teardown_valgrind () {
-	test -z "$GIT_VALGRIND" && return
+	test -z "${GIT_VALGRIND:-}" && return
 	GIT_VALGRIND_ENABLED=
 }
 
 maybe_setup_valgrind () {
-	test -z "$GIT_VALGRIND" && return
+	test -z "${GIT_VALGRIND:-}" && return
 	if test -z "$valgrind_only"
 	then
 		GIT_VALGRIND_ENABLED=t

@@ -969,12 +989,12 @@ test_eval_ () {
 
 test_run_ () {
 	test_cleanup=:
-	expecting_failure=$2
+	expecting_failure=${2:-}
 
 	if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then
 		# turn off tracing for this test-eval, as it simply creates
 		# confusing noise in the "-x" output
-		trace_tmp=$trace
+		trace_tmp=${trace:-}
 		trace=
 		# 117 is magic because it is unlikely to match the exit
 		# code of other programs

@@ -1022,7 +1042,7 @@ test_finish_ () {
 	echo >&3 ""
 	maybe_teardown_valgrind
 	maybe_teardown_verbose
-	if test -n "$GIT_TEST_TEE_OFFSET"
+	if test -n "${GIT_TEST_TEE_OFFSET:-}"
 	then
 		GIT_TEST_TEE_OFFSET=$(test-tool path-utils file-size \
 			"$GIT_TEST_TEE_OUTPUT_FILE")

@@ -1032,7 +1052,7 @@ test_finish_ () {
 test_skip () {
 	to_skip=
 	skipped_reason=
-	if match_pattern_list $this_test.$test_count "$GIT_SKIP_TESTS"
+	if match_pattern_list $this_test.$test_count "${GIT_SKIP_TESTS:-}"
 	then
 		to_skip=t
 		skipped_reason="GIT_SKIP_TESTS"

@@ -1150,7 +1170,7 @@ test_done () {
 
 	finalize_junit_xml
 
-	if test -z "$HARNESS_ACTIVE"
+	if test -z "${HARNESS_ACTIVE:-}"
 	then
 		mkdir -p "$TEST_RESULTS_DIR"

@@ -1312,17 +1332,18 @@ then
 	GIT_VALGRIND_ENABLED=t
 	test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
 	export GIT_VALGRIND_ENABLED
-elif test -n "$GIT_TEST_INSTALLED"
+elif test -n "${GIT_TEST_INSTALLED:-}"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
 	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
-	if test -n "$no_bin_wrappers"
+	if test -n "${no_bin_wrappers:-}"
 	then
 		with_dashes=t
 	else
+		with_dashes=
 		git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
 		if ! test -x "$git_bin_dir/git"
 		then

@@ -1345,9 +1366,9 @@ GIT_CONFIG_NOSYSTEM=1
 GIT_ATTR_NOSYSTEM=1
 export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
 
-if test -z "$GIT_TEST_CMP"
+if test -z "${GIT_TEST_CMP:-}"
 then
-	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
+	if test -n "${GIT_TEST_CMP_USE_COPIED_CONTEXT:-}"
 	then
 		GIT_TEST_CMP="$DIFF -c"
 	else

@@ -1372,7 +1393,7 @@ fi
 remove_trash=
 this_test=${0##*/}
 this_test=${this_test%%-*}
-if match_pattern_list "$this_test" "$GIT_SKIP_TESTS"
+if match_pattern_list "$this_test" "${GIT_SKIP_TESTS:-}"
 then
 	say_color info >&3 "skipping test $this_test altogether"
 	skip_all="skip all tests in $this_test"

@@ -1392,7 +1413,7 @@ rm -fr "$TRASH_DIRECTORY" || {
 }
 
 remove_trash=t
-if test -z "$TEST_NO_CREATE_REPO"
+if test -z "${TEST_NO_CREATE_REPO:-}"
 then
 	git init "$TRASH_DIRECTORY" >&3 2>&4 ||
 	error "cannot run git init"

@@ -1463,7 +1484,7 @@ yes () {
 # to call "git env--helper" (via test_bool_env). Only do that work
 # if needed by seeing if GIT_TEST_FAIL_PREREQS is set at all.
 GIT_TEST_FAIL_PREREQS_INTERNAL=
-if test -n "$GIT_TEST_FAIL_PREREQS"
+if test -n "${GIT_TEST_FAIL_PREREQS:-}"
 then
 	if test_bool_env GIT_TEST_FAIL_PREREQS false
 	then

@@ -1534,7 +1555,7 @@ test -n "$USE_LIBPCRE2" && test_set_prereq PCRE
 test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 
-if test -z "$GIT_TEST_CHECK_CACHE_TREE"
+if test -z "${GIT_TEST_CHECK_CACHE_TREE:-}"
 then
 	GIT_TEST_CHECK_CACHE_TREE=true
 	export GIT_TEST_CHECK_CACHE_TREE

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help