Jeff King wrote: > On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote: > > > > Unfortunately, this isn't a complete solution. > > > > Software will never be perfect. > > > > We don't need to wait for a perfect solution, all we need is something > > better than the current siuation. > > Sure, but if you don't fully understand the situation (e.g., that --root > and TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up > revisiting the incomplete fix later, The fact that you may end up revisiting a solution is a fact for *all* changes (including 2d14e13c56 (test output: respect $TEST_OUTPUT_DIRECTORY, 2013-04-29)). > or even making things worse (as this patch did). I think breaking the test suite is objectively worse than having a few extra files in the output directory, but to each his own. > > > I think solving the whole issue would require a mechanism for passing > > > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an > > > environment variable or the command-line). > > > > Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking > > for a reason there's 2d14e13c56 (test output: respect > > $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for > > valgrind/analyze.sh. > > > > I don't know who uses that script, or how. There's no documentaion, > > nothing on the mailing list, and nothing found on Google. > > Perhaps 268fac6919 (Add a script to coalesce the valgrind outputs, > 2009-02-04) is enlightening. That makes it clearer. > I don't know if anybody still uses it these days, though. I suspect it's > outlived its usefulness, in that we would typically not have any > valgrind errors at all (so coalescing them is not that interesting). > > Possibly folks investigating leak-checking via valgrind could find it > useful, but even there I think LSan is a much better path forward. Yeah, but even if they do run this tool, they can set TEST_OUTPUT_DIRECTORY manually. The needs of the few should not otweight needs of the many. > > So maybe: > > > > diff --git a/Makefile b/Makefile > > index c3565fc0f8..2e25489569 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE > > @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+ > > @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+ > > @echo X=\'$(X)\' >>$@+ > > -ifdef TEST_OUTPUT_DIRECTORY > > - @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+ > > -endif > > I don't personally have any problem with that. It does mean that "make > t1234-foo.sh" will behave differently than "./t1234-foo.sh", but that is > already true if you set GIT_TEST_OPTS. Only if you haven't changed TEST_OUTPUT_DIRECTORY since the last time you ran 'make' on the top level directory. And of course if somebody really wants their environment to be honored, that's what "make -e t1234-foo.sh" is for. Cheers. -- Felipe Contreras
