Re: [PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 19, 2022, 01:58 UTC
Message-ID: <220219.86y227fvz1.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqee3zycpg.fsf@gitster.g>

On Fri, Feb 18 2022, Junio C Hamano wrote:

Show 67 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
>> "/path/to/build". The "TEST_DIRECTORY" here is already made an
>> absolute path a few lines above this.
>>
>> This will be helpful to LSAN_OPTIONS which will want to strip the
>> build directory path from filenames, which we couldn't do if we had a
>> "/.." in there.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/test-lib.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 3212966a82f..4f523b82ce5 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -34,7 +34,7 @@ then
>>  	# elsewhere
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>
> This makes perfect sense in the normal case, but the provision the
> code that precedes this part has, i.e.
>
>     if test -z "$TEST_DIRECTORY"
>     then
>             # We allow tests to override this, in case they want to run tests
>             # outside of t/, e.g. for running tests on the test library
>             # itself.
>             TEST_DIRECTORY=$(pwd)
>     else
>             # ensure that TEST_DIRECTORY is an absolute path so that it
>             # is valid even if the current working directory is changed
>             TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
>     fi
>
> to allow TEST_DIRECTORY to be set externally robs the guarantee that
> you can sensibly strip "/t" from its tail and expect everything to
> work correctly.  The only thing the original requires on such an
> externally given TEST_DIRECTORY is that one level above it is usable
> as GIT_BUILD_DIR.
>
> IOW,
>
> 	GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"
>
> would give you what you want to achieve in either code path, as long
> as the original was working correctly for whatever value that is
> given to TEST_DIRECTORY externally.
>
> So, perhaps
>
> 	if test -z "$TEST_DIRECTORY"
> 	then
> 		TEST_DIRECTORY=$(pwd)
> 		GIT_BUILD_DIR=${TEST_DIRECTORY%/t}
> 	else
> 		TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) &&
> 		GIT_BUILD_DIR=$(cd "$TEST_DIRECTORY/.." && pwd)
> 	fi
>
> or something like that?  I dunno.

I think you're being led astray by the "we allow tests to override this" comment, which is something I added in 62f539043c7 (test-lib: Allow overriding of TEST_DIRECTORY, 2010-08-19). I'm having some trouble understanding what I meant at the time.

But it's not the case that we support a TEST_DIRECTORY pointing to
something that isn't inside the "t/" directory in our tree, as looking
at uses of the two shows:
    
    $ git grep -E '\$(GIT_BUILD_DIR|TEST_DIRECTORY)' -- t/test-lib.sh
    t/test-lib.sh:if test -z "$TEST_DIRECTORY"
    t/test-lib.sh:  TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
    t/test-lib.sh:  TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
    t/test-lib.sh:GIT_BUILD_DIR="$TEST_DIRECTORY"/..
    t/test-lib.sh:if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
    t/test-lib.sh:. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
    t/test-lib.sh:"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
    t/test-lib.sh:. "$TEST_DIRECTORY/test-lib-functions.sh"
    t/test-lib.sh:                  $(printf '%s\n' "$1" | sed -f "$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!')
    t/test-lib.sh:                  symlink_target="$GIT_BUILD_DIR/t/helper/$base"
    t/test-lib.sh:                  symlink_target="$GIT_BUILD_DIR/$base"
    t/test-lib.sh:  GIT_VALGRIND=$TEST_DIRECTORY/valgrind
    t/test-lib.sh:  for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/t/helper/test-*
    t/test-lib.sh:  make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
    t/test-lib.sh:  PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
    t/test-lib.sh:          git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
    t/test-lib.sh:  GIT_EXEC_PATH=$GIT_BUILD_DIR
    t/test-lib.sh:          PATH="$GIT_BUILD_DIR:$GIT_BUILD_DIR/t/helper:$PATH"
    t/test-lib.sh:GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
    t/test-lib.sh:GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
    t/test-lib.sh:test -d "$GIT_BUILD_DIR"/templates/blt || {
    t/test-lib.sh:if ! test -x "$GIT_BUILD_DIR"/t/helper/test-tool$X

I.e. we already depend on the build dir being one-above the TEST_DIRECTORY, and per test-lib.sh we load test-lib-functions.sh from $TEST_DIRECTORY, and refer to $GIT_BUILD_DIR/t/... for various things (e.g. t/helper).

That being said it was a bit of a micro-optimization of mine to avoid a "&& pwd" there, and perhaps it was too clever.

But I do think we can 100% rely on just stripping the "/t" from the end of the string.

Re-reading it again, what that comment really meant to say is that we allow overriding TEST_DIRECTORY from $(pwd) because when we run that test-lib.sh we may be in another directory.

But that overriding code still sets us to the same directory, which ends in a "/t". See my subsequent 7b905119703 (t/t0000-basic.sh: Run the passing TODO test inside its own test-lib, 2010-08-19) for the first use of it.

I.e. it's for the t0000-basic.sh testing where we run sub-tests, which now live in t/lib-subtest.sh.

So I could keep that code as-is for a re-roll, but adjust the misleading comment while I'm at it, how does that sound?