Re: [PATCH 1/2] tests: make sure nested lazy prereqs work reliably

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 18, 2020, 20:00 UTC
Message-ID: <xmqqh7pm1llj.fsf@gitster.c.googlers.com>
In-Reply-To: <20201118190414.32616-1-szeder.dev@gmail.com>

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

Show 6 quoted lines

> This can be problematic, because lazy prereqs are evaluated in the
> '$TRASH_DIRECTORY/prereq-test-dir' directory, which is the same for
> every prereq, and which is automatically removed after the prereq has
> been evaluated.  So if the inner prereq (BAR above) is a lazy prereq
> that hasn't been evaluated yet, then after its evaluation the
> 'prereq-test-dir' shared with the outer prereq will be removed.

OK.

> Consequently, 'check-foo' will find itself in a non-existing
> directory, and won't be able to create/access any files in its cwd,
> which could result in an unfulfilled outer prereq.

I have a feeling that this would be filesystem dependent what happens in an unlinked directory. But in any case, if an outer lazy prereq creates a file in the test directory, tries to evaluate another lazy prereq (which would use the same directory and then would remove everything in it and the directory itself when done), and then expects that the file it created before evaluating the inner lazy prereq is still there, it would not work, so I think this is a good change. I just think "won't be able to" is a bit too strong here ("may not be able to (depending on the filesystem)", I would buy).

Show 11 quoted lines

> +test_lazy_prereq NESTED_INNER '
> +	>inner &&
> +	rm -f outer
> +'
> +test_lazy_prereq NESTED_PREREQ '
> +	>outer &&
> +	test_have_prereq NESTED_INNER &&
> +	echo "can create new file in cwd" >file &&
> +	test -f outer &&
> +	test ! -f inner
> +'

And the existence check for outer is exactly what I wrote above. That would portably break without separte directory. I do not know if "can create new file" step would fail portably.

Show 14 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8d59b90348..94395b9807 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -474,15 +474,15 @@ test_lazy_prereq () {
>  
>  test_run_lazy_prereq_ () {
>  	script='
> -mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
> +mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&
>  (
> -	cd "$TRASH_DIRECTORY/prereq-test-dir" &&'"$2"'
> +	cd "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&'"$2"'
>  )'

Qoting rules are a bit tricky here, but I think it does not matter too much, as $1 (name or prereq) won't have $IFS or "'" or anything funny in it.

Show 6 quoted lines

>  	say >&3 "checking prerequisite: $1"
>  	say >&3 "$script"
>  	test_eval_ "$script"
>  	eval_ret=$?
> -	rm -rf "$TRASH_DIRECTORY/prereq-test-dir"
> +	rm -rf "$TRASH_DIRECTORY/prereq-test-dir-$1"

And this is obviously safe no matter what is in $1 ;-)

>  	if test "$eval_ret" = 0; then
>  		say >&3 "prerequisite $1 ok"
>  	else

Looks good.