On Wed, Feb 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> On Mon, Feb 21 2022, Junio C Hamano wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx >> 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 ?
