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

[Junio C Hamano <gitster@pobox.com>]

Æ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.