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