Re: [PATCH] t0000: fix test if run with TEST_OUTPUT_DIRECTORY

From: Jeff King <peff@peff.net>
Date: Jul 20, 2021, 07:11 UTC
Message-ID: <YPZ3JmWKLrUldK4R@coredump.intra.peff.net>
In-Reply-To: <44006e7b0bdda50dc51153cc2efb6ae954d4eecb.1626762728.git.ps@pks.im>

On Tue, Jul 20, 2021 at 08:32:26AM +0200, Patrick Steinhardt wrote:

> Fix the issue by adding a new TEST_OUTPUT_DIRECTORY_OVERRIDE variable.
> If set, then we'll always override the TEST_OUTPUT_DIRECTORY with its
> value after sourcing GIT-BUILD-OPTIONS.

Thanks, I like this approach much better than removing TEST_OUTPUT_DIRECTORY entirely (and I confirmed that it fixes the problem).

I do wish we had a more generic way of overriding stuff in GIT-BUILD-OPTIONS, rather than introducing manual _OVERRIDE variables for each. But there's not an easy solution here (see the earlier thread for some discussion), so this seems like a good immediate step to take.

One small note on the commit message:

> While this works as expected in the general case, it falls apart when
> the developer has TEST_OUTPUT_DIRECTORY explicitly defined either via
> the environment or via config.mak.

The mention of the environment confused me for a moment, since:

  TEST_OUTPUT_DIRECTORY=/tmp/foo ./t0000-basic.sh

is already OK. But you probably meant that:

  TEST_OUTPUT_DIRECTORY=/tmp/foo make test

would fail, since "make" would pick up the variable and then write it into GIT-BUILD-OPTIONS (just as it would if you put it in config.mak, or on the command-line of make).

I don't think it's sufficiently confusing to rewrite the commit message, but just something I noted while reading it.

Show 14 quoted lines

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -57,6 +57,15 @@ fi
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
>  export PERL_PATH SHELL_PATH
>  
> +# In t0000, we need to override test directories of nested testcases. In case
> +# 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}"
> +then
> +	TEST_OUTPUT_DIRECTORY="${TEST_OUTPUT_DIRECTORY_OVERRIDE}"
> +fi

Thanks for this comment. Hopefully that will make it clear to anybody that the override mechanism is not meant for general use by developers.

-Peff