On Fri, Feb 18 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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?
