Re: What's cooking in git.git (Jul 2021, #03; Tue, 13)

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Patrick Steinhardt <ps@pks.im>,
	Felipe Contreras <felipe.contreras@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	git@vger.kernel.org
Subject: Re: What's cooking in git.git (Jul 2021, #03; Tue, 13)
Date: Tue, 20 Jul 2021 02:21:16 -0500	[thread overview]
Message-ID: <60f6796c18a4d_14a799208fb@natae.notmuch> (raw)
In-Reply-To: <YPZtxy1/742YlrlK@ncase>

Patrick Steinhardt wrote:
> On Mon, Jul 19, 2021 at 02:57:23PM -0500, Felipe Contreras wrote:
> > Junio C Hamano wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > > 
> > > > On Mon, Jul 19, 2021 at 04:33:54AM -0400, Jeff King wrote:
> > > >> On Mon, Jul 19, 2021 at 09:35:36AM +0200, Patrick Steinhardt wrote:
> > > >> 
> > > >> > On Tue, Jul 13, 2021 at 06:07:12PM -0700, Junio C Hamano wrote:
> > > >> > [snip]
> > > >> > > * ps/perf-with-separate-output-directory (2021-07-02) 1 commit
> > > >> > >  - perf: fix when running with TEST_OUTPUT_DIRECTORY
> > > >> > > 
> > > >> > >  Test update.
> > > >> > > 
> > > >> > >  What's the status of this one?
> > > >> > 
> > > >> > From my point of view this is ready, but it's still missing reviews so
> > > >> > far. The lack of interest seems to indicate that nobody has hit the
> > > >> > issue so far, and I wonder why that is. Am I the only one who sets
> > > >> > TEST_OUTPUT_DIRECTORY to a tmpfs directory in his config.mak to speed up
> > > >> > tests?
> > > >> 
> > > >> I had marked it to look at, but just hadn't gotten around to it. I just
> > > >> gave it a review (but the upshot is that it looks fine to me).
> > > >> 
> > > >> I don't set TEST_OUTPUT_DIRECTORY myself; instead I do:
> > > >> 
> > > >>   GIT_TEST_OPTS = --root=/path/to/tmpfs
> > > >> 
> > > >> TBH, I had never really considered using TEST_OUTPUT_DIRECTORY for this
> > > >> (--root predates it, and was written explicitly for the tmpfs case). But
> > > >> I also think --root is more convenient:
> > > >> 
> > > >>   - "make test" will run in the tmpfs for speed, but "./t1234-foo.sh -i"
> > > >>     will run locally, which makes it easy to "cd" in to inspect the
> > > >>     result
> > > >> 
> > > >>   - likewise, I find accessing the results in t/test-results/*.out a
> > > >>     little more convenient
> > > >> 
> > > >> But all of that is preference. I don't think you're wrong to use
> > > >> TEST_OUTPUT_DIRECTORY this way, but the above points might be
> > > >> interesting to you.
> > > >
> > > > It is, thanks a lot for the hint. But given your first point about
> > > > direct execution, this in fact makes me want TEST_OUTPUT_DIRECTORY in
> > > > contrast to `--root=/path/to/tmpfs`: especially in the context of perf
> > > > tests, I never run all of them together given that it takes such a long
> > > > time. So I instead either run them directly or via the `./run` script,
> > > > and in both cases I definitely want to have them in tmpfs given that
> > > > there's a lot of disk churn if you're using biggish repos.
> > > >
> > > > Patrick
> > > 
> > > Thanks, all.  Let me mark the patch for 'next'.
> > 
> > OK, if you don't care that TEST_OUTPUT_DIRECTORY is broken, so be it.
> 
> I just think we shouldn't strip away features if we should instead look
> at why the testcase is broken and how to fix the root cause.

I mean, we didn't really strip away any features, you just happened to add
one feature at the same time the fix was proposed.

> I'll send a patch in a minute which fixes the testcase without
> dropping support for TEST_OUTPUT_DIRECTORY.

OK. You mean without breaking functonality that is flying.

-- 
Felipe Contreras