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

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

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.