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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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 <redacted>
---
 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?