Re: [PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2016, 19:53 UTC
Message-ID: <xmqq7fej44iq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqshx75psw.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> Others, I am not sure how seriously we would want to support those
> who use "set -u"; the damage to test-lib.sh so far (see below) does
> not look _too_ bad, but the reason why I have "set -u" after
> including lib-bash.sh is because it will trigger a lot more errors
> while lib-bash and test-lib do their preparation.

Ehh, this didn't come out very well as I hoped, primarily because my thinking behind it was muddy X-<.

I do want to see us keep supporting those who "set -u" in their interactive shell environment, and we should strive to make sure that the "prompt" and the "completion", which are part of their interactive shell experience, work well with "set -u".

It does not mean "cd t && sh t$anything.sh" must be "set -u" clean at all (this is where my thinking was muddy); it is sufficient if we can successfully run t9903 (prompt) with "set -u" before the script includes "git-prompt". We do not have to be able to insert "set -u" at the beginning of test-lib.sh and cleanly run other tests.

Having said that, some parts of "minimum" change shown in my "how about this" patch would probably be worth doing anyway. They make sure that:

 (1) variables the test framework uses will always be initialized
     properly, without getting affected a stray environment variable
     the user may have.  E.g. "trace".

 (2) environment variables the test framework may accept to change
     the behaviour, e.g. GIT_SKIP_TESTS, are clearly documented by
     using the idiom "set to an empty string if not set".

So in that sense, even though we may not add "set -u" at the beginning of test-lib.sh in the production code, doing so may be a good developer aid to catch other variables the attached patch did not catch and complete the above two points.

Show 40 quoted lines

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0055ebb..36ca13e 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -186,6 +186,9 @@ test "x$TERM" != "xdumb" && (
>  	) &&
>  	color=t
>  
> +HARNESS_ACTIVE= quiet= verbose_only= run_list= trace=
> +: ${GIT_SKIP_TESTS=}
> +
>  while test "$#" -ne 0
>  do
>  	case "$1" in
> @@ -585,7 +588,7 @@ 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
> @@ -750,6 +753,8 @@ test_done () {
>  	esac
>  }
>  
> +GIT_VALGRIND=
> +
>  if test -n "$valgrind"
>  then
>  	make_symlink () {
> @@ -902,6 +907,7 @@ fi
>  # in subprocesses like git equals our $PWD (for pathname comparisons).
>  cd -P "$TRASH_DIRECTORY" || exit 1
>  
> +skip_all=
>  this_test=${0##*/}
>  this_test=${this_test%%-*}
>  if match_pattern_list "$this_test" $GIT_SKIP_TESTS