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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Feb 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, Feb 21 2022, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>>> 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.
>>>>
>>>>> 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.
>>>
>>> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY
>>> override logic did not mean to support such a use case".
>>
>> To clarify with "can't happen" I mean (and should have said) that "can't
>> work", i.e. it would error out anyway.
>>
>> E.g. try in a git.git checkout:
>>     
>>     (
>>         mv t t2 &&
>>         cd t &&
>>         ./t0001-init.sh
>>     )
>>
>> It will die with:
>>     
>>     You need to build test-tool:
>>     Run "make t/helper/test-tool" in the source (toplevel) directory
>>     FATAL: Unexpected exit with code 1
>>
>> And if you were to manually patch test-lib.sh to get past that error it
>> would start erroring on e.g.:
>>
>>     sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory
>>
>> And if you "fix" that it'll error out on something else.
>>
>> I.e. we'll have discovered that $(pwd)/.. must be our build directory,
>> and we then construct paths by adding the string "/t/[...]" to that.
>>
>>> I am perfectly fine if we declared that we do not to support the use
>>> of that override mechanism by anybody but the "subtest" thing we do
>>> ourselves.  If we can catch a workflow that misuses the mechansim
>>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set
>>> and it does not end in "/t"), we should do so, I would think, instead
>>> of doing the "go up and do pwd", which will make things worse.
>>
>> What I was going for in 2/4 in
>> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com
>> is that we've already declared that. I.e. test-lib.sh has various
>> assumptions about appending "/t/..." to the build directory being a
>> valid way to get paths to various test-lib.sh-adjacent code.
>>
>> So trimming off "/t" here with a string operation v.s. $(cd .. && pwd)
>> is being consistent with that code.
>>
>> It would be odd to make the bit at the top very generic, only to have
>> the reader keep reading and wonder how that generic mechanism and the
>> subsequent hardcoding of "/t/[...]" are supposed to work together.
>
> Correct.  That is why I said $(... pwd) to pretend that we can take
> anything would make it worse in a separate message.
>
> If we have to strip off /t anyway, piggy-backing on that part to
> detect and abort when somebody misused the mechanism would be a good
> idea---which is what I said in the message you are responding to and
> not responding to.

So you're OK with the assumption/method being used here, but would
prefer if we also added an explicit check/"exit 1"? E.g.:
	
	if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
	then
	        echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
	        exit 1
	fi

?