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

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

[Mickey Endito]

The variable D is never defined in test t5582, more severely the test
fails if D is defined by something outside the test suite, so remove
this spurious line.

Signed-off-by: Mickey Endito <mickey.endito.2323@protonmail.com>
---
To reproduce a failure do
D=/some/path/which/does/not/exist t/t5582-fetch-negative-refspec.sh

Note: The variable D seems to be a reminiscent similar to t/t5510-fetch.sh,
which defines "D=$(pwd)". If you want to adopt that way, then you have
to code a fix yourself. ;-)

 t/t5582-fetch-negative-refspec.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index e5d2e79ad3..7a80e47c2b 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -105,7 +105,6 @@ test_expect_success "fetch with negative pattern refspec does not expand prefix"
 '

 test_expect_success "fetch with negative refspec avoids duplicate conflict" '
-	cd "$D" &&
 	(
 		cd one &&
 		git branch dups/a &&
--
2.30.2

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

[Junio C Hamano]

Mickey Endito <mickey.endito.2323@protonmail.com> writes:

> The variable D is never defined in test t5582, more severely the test
> fails if D is defined by something outside the test suite, so remove
> this spurious line.

Wow.  Well spotted.

When D is left unset, we end up executing

	cd "" && ...

and it explains why nobody noticed the breakage for nearly a year
since c0192df6 (refspec: add support for negative refspecs,
2020-09-30) was written.


Unlike the apparent
copy-and-paste source, this is a more modern script that limits the
chdir inside subshells to avoid moving around in the main flow of
the test, and the fix proposed here looks the most sensible.  

> Signed-off-by: Mickey Endito <mickey.endito.2323@protonmail.com>
> ---
> To reproduce a failure do
> D=/some/path/which/does/not/exist t/t5582-fetch-negative-refspec.sh
>
> Note: The variable D seems to be a reminiscent similar to t/t5510-fetch.sh,
> which defines "D=$(pwd)". If you want to adopt that way, then you have
> to code a fix yourself. ;-)
>
>  t/t5582-fetch-negative-refspec.sh | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> index e5d2e79ad3..7a80e47c2b 100755
> --- a/t/t5582-fetch-negative-refspec.sh
> +++ b/t/t5582-fetch-negative-refspec.sh
> @@ -105,7 +105,6 @@ test_expect_success "fetch with negative pattern refspec does not expand prefix"
>  '
>
>  test_expect_success "fetch with negative refspec avoids duplicate conflict" '
> -	cd "$D" &&
>  	(
>  		cd one &&
>  		git branch dups/a &&
> --
> 2.30.2

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

[SZEDER Gábor]

On Mon, Aug 23, 2021 at 04:32:39PM -0700, Junio C Hamano wrote:
> Mickey Endito <mickey.endito.2323@protonmail.com> writes:
> 
> > The variable D is never defined in test t5582, more severely the test
> > fails if D is defined by something outside the test suite, so remove
> > this spurious line.
> 
> Wow.  Well spotted.
> 
> When D is left unset, we end up executing
> 
> 	cd "" && ...
> 
> and it explains why nobody noticed the breakage for nearly a year
> since c0192df6 (refspec: add support for negative refspecs,
> 2020-09-30) was written.
> 
> 
> Unlike the apparent
> copy-and-paste source, this is a more modern script that limits the
> chdir inside subshells to avoid moving around in the main flow of
> the test, and the fix proposed here looks the most sensible.  

'grep " cd $" test-results/*.out' shows that there is a similar case
in 't5323-pack-redundant.sh' as well, in test 'master: pack-redundant
works with no packfile'.

> > Signed-off-by: Mickey Endito <mickey.endito.2323@protonmail.com>
> > ---
> > To reproduce a failure do
> > D=/some/path/which/does/not/exist t/t5582-fetch-negative-refspec.sh
> >
> > Note: The variable D seems to be a reminiscent similar to t/t5510-fetch.sh,
> > which defines "D=$(pwd)". If you want to adopt that way, then you have
> > to code a fix yourself. ;-)
> >
> >  t/t5582-fetch-negative-refspec.sh | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> > index e5d2e79ad3..7a80e47c2b 100755
> > --- a/t/t5582-fetch-negative-refspec.sh
> > +++ b/t/t5582-fetch-negative-refspec.sh
> > @@ -105,7 +105,6 @@ test_expect_success "fetch with negative pattern refspec does not expand prefix"
> >  '
> >
> >  test_expect_success "fetch with negative refspec avoids duplicate conflict" '
> > -	cd "$D" &&
> >  	(
> >  		cd one &&
> >  		git branch dups/a &&
> > --
> > 2.30.2

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Mon, Aug 23, 2021 at 04:32:39PM -0700, Junio C Hamano wrote:
>> Mickey Endito <mickey.endito.2323@protonmail.com> writes:
>> 
>> > The variable D is never defined in test t5582, more severely the test
>> > fails if D is defined by something outside the test suite, so remove
>> > this spurious line.
>> 
>> Wow.  Well spotted.
>> 
>> When D is left unset, we end up executing
>> 
>> 	cd "" && ...
>> 
>> and it explains why nobody noticed the breakage for nearly a year
>> since c0192df6 (refspec: add support for negative refspecs,
>> 2020-09-30) was written.
>> 
>> 
>> Unlike the apparent
>> copy-and-paste source, this is a more modern script that limits the
>> chdir inside subshells to avoid moving around in the main flow of
>> the test, and the fix proposed here looks the most sensible.  
>
> 'grep " cd $" test-results/*.out' shows that there is a similar case
> in 't5323-pack-redundant.sh' as well, in test 'master: pack-redundant
> works with no packfile'.

OK.  A candidate for a separate patch, which would be a low-hanging
fruit, I guess.

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

[Jeff King]

On Tue, Aug 24, 2021 at 08:59:42PM +0200, SZEDER Gábor wrote:

> > Unlike the apparent
> > copy-and-paste source, this is a more modern script that limits the
> > chdir inside subshells to avoid moving around in the main flow of
> > the test, and the fix proposed here looks the most sensible.  
> 
> 'grep " cd $" test-results/*.out' shows that there is a similar case
> in 't5323-pack-redundant.sh' as well, in test 'master: pack-redundant
> works with no packfile'.

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

-- >8 --
Subject: [PATCH] t5323: drop mentions of "master"

Commit 0696232390 (pack-redundant: fix crash when one packfile in repo,
2020-12-16) added one some new tests to t5323. At the time, the sub-repo
we used was called "master". But in a parallel branch, this was switched
to "main".

When the latter branch was merged in 27d7c8599b (Merge branch
'js/default-branch-name-tests-final-stretch', 2021-01-25), some of those
spots caused textual conflicts, but some (for tests that were far enough
away from other changed code) were just semantic. The merge resolution
fixed up most spots, but missed this one.

Even though this did impact actual code, it turned out not to fail the
tests. Running 'cd "$master_repo"' ended up staying in the same
directory, running the test in the main trash repo instead of the
sub-repo. But because the point of the test is checking behavior when
there are no packfiles, it worked in either repo (since both are empty
at this point in the script).

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5323-pack-redundant.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 8b01793845..8dbbcc5e51 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -114,9 +114,9 @@ test_expect_success 'setup main repo' '
 	create_commits_in "$main_repo" A B C D E F G H I J K L M N O P Q R
 '
 
-test_expect_success 'master: pack-redundant works with no packfile' '
+test_expect_success 'main: pack-redundant works with no packfile' '
 	(
-		cd "$master_repo" &&
+		cd "$main_repo" &&
 		cat >expect <<-EOF &&
 			fatal: Zero packs found!
 			EOF
-- 
2.33.0.394.gce7abe507a

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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, and because the current one
happens to have no pack and running the command in a repository
without any pack is what the test wants to do anyway.

Thanks, will queue.


> diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
> index 8b01793845..8dbbcc5e51 100755
> --- a/t/t5323-pack-redundant.sh
> +++ b/t/t5323-pack-redundant.sh
> @@ -114,9 +114,9 @@ test_expect_success 'setup main repo' '
>  	create_commits_in "$main_repo" A B C D E F G H I J K L M N O P Q R
>  '
>  
> -test_expect_success 'master: pack-redundant works with no packfile' '
> +test_expect_success 'main: pack-redundant works with no packfile' '
>  	(
> -		cd "$master_repo" &&
> +		cd "$main_repo" &&
>  		cat >expect <<-EOF &&
>  			fatal: Zero packs found!
>  			EOF

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

[SZEDER Gábor]

On Wed, Aug 25, 2021 at 09:12:37AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > 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?

  ---  >8  ---

Subject: [PATCH] test-lib: catch 'cd "$dir"' with unset variable

We just had to fix two test cases [1] that invoked 'cd "$dir"' while
the given variable was accidentally unset.  While POSIX states that if
'cd's directory parameter "is an empty string, the results are
unspecified" [2], most shells treat this as a no-op [3], i.e. they
neither change directory nor return error, that's why both of those
tests happened to succeed on common shells, and thus these issues
remained hidden for a while.

Catch 'cd ""' by adding a thin wrapper function overriding the
shell's 'cd' command to verify the non-emptiness of its parameters and
call BUG if necessary.

[1] bd72824c60 (t5582: remove spurious 'cd "$D"' line, 2021-08-23)
    c21b2511c2 (t5323: drop mentions of "master", 2021-08-24)
[2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/cd.html
[3] 'ksh' and 'ksh93' are the only shells I found that error out on
    'cd ""' with "cd: bad directory" (though these shells are unable
    to run significant portion of our test sute anyway).

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d6..06b75d8430 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1436,6 +1436,14 @@ EMPTY_TREE=$(test_oid empty_tree)
 EMPTY_BLOB=$(test_oid empty_blob)
 _z40=$ZERO_OID
 
+cd () {
+	if test -z "$@"
+	then
+		BUG "cd invoked with empty parameter"
+	fi
+	command cd "$@"
+}
+
 # Provide an implementation of the 'yes' utility; the upper bound
 # limit is there to help Windows that cannot stop this loop from
 # wasting cycles when the downstream stops reading, so do not be
-- 
2.33.0.358.g803110d36e

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Wed, Aug 25, 2021 at 09:12:37AM -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.
>
>       : ${GIT_TEST_CMP:=}
>
>     near the top of the script to pre-declare all such variables.

These two are sensible clean-up, I would think, whether we aim to
achieve "set -u" cleanness.  The original issue triggered this
thread was about $D that can be confused by a leaked environment
variable, but these "known variables" that are not assigned to in
the early part of the tests are inviting similar troubles.

Thanks.