Re: [PATCH v2 2/6] t: extend test_lazy_prereq

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

>> +	elif test "$eval_ret" = 125; then
>> +		:;
>>  	else
>>  		say >&3 "prerequisite $1 not satisfied"
>>  	fi
>
> The semicolon in ":;" threw me off a bit. Am I missing why we need it or
> is it superfluous?

The latter, of course -).

>> @@ -811,6 +813,9 @@ test_have_prereq () {
>>  				if test_run_lazy_prereq_ "$prerequisite" "$script"
>>  				then
>>  					test_set_prereq $prerequisite
>> +				elif test $? = 125
>> +				then
>> +					BUG "Do not use $prerequisite"
>>  				fi
>>  				lazily_tested_prereq="$lazily_tested_prereq$prerequisite "
>>  			esac
>
> Hm, okay. It feels quite close to overthinking the whole deprecation
> cycle around prerequisites as it's nothing that we tend to do very
> often. But on the other hand the implementation is trivial enough, so I
> don't mind it much.

I agree that this has nothing to do with breaking changes at the Git
3.0 boundary.  We just did not have good documentation for lazy
prerequisites, and we just did not have any good support for marking
a prerequisite should no longer be used.  [1/6] is for the former,
and [2/6] is for the latter.

We can avoid the magic 125 by adding a new helper like test_removed_prereq
and do this instead, which may be cleaner and simpler to reason about.

Another alternative that may make writing tests even be less error
prone but a bit more verbose is to introduce test_unset_prereq and
be explicit about unsatisfied prerequisites.  The effect of the
resulting system becomes larger to include detecting misspelled
prerequisites, and removed prerequisites would be detected as a
natural fallout from the same mechanism.

As we have >50 prerequisites defined with test_set_prereq and we'd
need to add 50 calls to test_unset_prereq to mark them as "known but
not satisified on this platform" to differentiate them from the ones
that are misspelt or removed, if we go that route.  I am not sure if
that is worth it.  Certainly not in the short term, but for a longer
term, if people ever misspelt a prerequisite SYMLINKS as SYMLINK and
wasted time wondering why their tests didn't trigger, it might be
worth it.  I dunno.

 t/test-lib-functions.sh | 14 ++++++++++++++
 t/test-lib.sh           |  5 +++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git c/t/test-lib-functions.sh w/t/test-lib-functions.sh
index 79377bc0fc..3903344fc1 100644
--- c/t/test-lib-functions.sh
+++ w/t/test-lib-functions.sh
@@ -751,7 +751,15 @@ test_set_prereq () {
 		;;
 	esac
 }
+
 satisfied_prereq=" "
+
+removed_prereq=
+# Mark a prerequisite deprecated-and-then-removed
+test_removed_prereq () {
+	removed_prereq="$removed_prereq$1 "
+}
+
 lazily_testable_prereq= lazily_tested_prereq=
 
 # Usage: test_lazy_prereq PREREQ 'script'
@@ -801,6 +809,12 @@ test_have_prereq () {
 			negative_prereq=
 		esac
 
+		case " $removed_prereq " in
+		*" $prerequisite "*)
+			BUG "Do not use $prerequisite"
+			;;
+		esac
+
 		case " $lazily_tested_prereq " in
 		*" $prerequisite "*)
 			;;
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 9001ed3a64..c2c96f5e7a 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -1862,8 +1862,9 @@ test_lazy_prereq CURL '
 	curl --version
 '
 
-test_lazy_prereq WITHOUT_BREAKING_CHANGES '
-	test -z "$WITH_BREAKING_CHANGES"
+test_removed_prereq WITHOUT_BREAKING_CHANGES
+test_lazy_prereq WITH_BREAKING_CHANGES '
+	test -n "$WITH_BREAKING_CHANGES"
 '
 
 # SHA1 is a test if the hash algorithm in use is SHA-1.  This is both for tests