git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 1/2] tests: make sure nested lazy prereqs work reliably
Date: Wed, 18 Nov 2020 12:00:40 -0800
Message-ID: <xmqqh7pm1llj.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201118190414.32616-1-szeder.dev@gmail.com> ("SZEDER =?utf-8?Q?G=C3=A1bor=22's?= message of "Wed, 18 Nov 2020 20:04:13 +0100")

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

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

> +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.

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

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

  parent reply	other threads:[~2020-11-18 20:04 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-18 19:04 SZEDER Gábor
2020-11-18 19:04 ` [PATCH 2/2] tests: fix description of 'test_set_prereq' SZEDER Gábor
2020-11-18 20:00 ` Junio C Hamano [this message]
2020-11-19 15:58 ` [PATCH 1/2] tests: make sure nested lazy prereqs work reliably Jeff King
2020-11-19 17:56   ` Jeff King
2020-11-19 19:50     ` Junio C Hamano
2020-11-20  0:14       ` Jeff King
2020-11-20  0:17         ` [PATCH 1/4] t0000: keep clean-up tests together Jeff King
2020-11-20  0:20         ` [PATCH 2/4] t0000: run prereq tests inside sub-test Jeff King
2020-11-20  0:22         ` [PATCH 3/4] t0000: run cleaning test " Jeff King
2020-11-20  0:27         ` [PATCH 4/4] t0000: consistently use single quotes for outer tests Jeff King
2020-11-20  1:32         ` [PATCH 1/2] tests: make sure nested lazy prereqs work reliably Junio C Hamano
2020-11-20  0:07   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh7pm1llj.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git