On Mon, Feb 21 2022, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

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.

quoted

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 (local)
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.