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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 21, 2022, 18:55 UTC
Message-ID: <220221.868ru4avw6.gmgdl@evledraar.gmail.com>
In-Reply-To: <YhPL+wSxtI0KIz07@nand.local>

On Mon, Feb 21 2022, Taylor Blau wrote:

Show 14 quoted lines

> On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 77c3fabd041..ff13321bfd3 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -41,7 +41,7 @@ then
>>  	# elsewhere
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>
> Sorry to notice this so late, but this hunk caught my eye. What happens
> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?

I think that the preceding 2/4 should cover your concern here, i.e. I think that's not possible.

Show 5 quoted lines

> Before this change, we would have set GIT_BUILD_DIR to the parent of
> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
> TEST_DIRECTORY, which is a behavior change.

Indeed, but I believe (again see 2/4) that can't happen.

Show 9 quoted lines

> If you just want GIT_BUILD_DIR to be absolute in order to for LSan to
> understand how to correctly strip it out of filenames, could we replace
> this with:
>
>     GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"
>
> or (an admittedly less clear)
>
>     GIT_BUILD_DIR="$(dirname "$TEST_DIRECTORY")"

Yes. I almost changed it to the former in this re-roll, but as noted in <220219.86y227fvz1.gmgdl@evledraar.gmail.com> thought it was better to have it clear that this really wasn't a generic mechanism, we really do need/want the actual "t" directory here.