Re: [PATCH 0/3] t0000 cleanups

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 30, 2013, 18:51 UTC
Message-ID: <20131230185125.GI20443@google.com>
In-Reply-To: <xmqqwqimry2j.fsf@gitster.dls.corp.google.com>

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jeff King wrote:

Show 13 quoted lines

>>> When I want to debug a failing test, I often end up doing:
>>>
>>>   cd t
>>>   ./t4107-<tab> -v -i
>>>   cd tra<tab>
>>>
>>> The test names are long, so tab-completing on the trash directory is
>>> very helpful. Lately I've noticed that there are a bunch of crufty trash
>>> directories in my t/ directory, which makes my tab-completion more
>>> annoying.
>>
>> Ah, and if I'd read this then I wouldn't have had to be confused at
>> all.

[...]

> The third paragraph of 1/3 sufficiently covers it, no?  We could add
> "It makes it less convenient to use tab completion 'cd t/tra<TAB>'
> to go to the trash directory of the failed test to inspect the
> situation" after "... left in the t/ directory.", though.

[4 paragraphs snipped]

I think it can be better, since the commit message left me scratching my head while the patch itself seems pretty simple. How about something like the following?

First, describing the problem:

	Running t0000 produces more trash directories than expected
	and does not clean up after itself:

	 $ ./t0000-basic.sh
	[...]
	 $ ls -d trash\ directory.*
	 trash directory.failing-cleanup
	 trash directory.mixed-results1
	 trash directory.mixed-results2
	 trash directory.partial-pass
	 trash directory.test-verbose
	 trash directory.test-verbose-only-2

Analysis and fix:

	These scratch areas for sub-tests should be under the t0000
	trash directory, but because the TEST_OUTPUT_DIRECTORY
	setting from the toplevel test leaks into the environment
	they are created under the toplevel output directory (typically
	t/) instead.  Because some of the sub-tests simulate failures,
	their trash directories are kept around.

	Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately
	for sub-tests.

And then, optionally, describing rejected alternatives:

	An alternative fix would be to pass the --root parameter that
	only specifies where to put the trash directories, which would
	also work.  However, using TEST_OUTPUT_DIRECTORY is more
	futureproof in case tests want to write more output in
	addition to the test-results/ (which are already suppressed in
	sub-tests using the HARNESS_ACTIVE setting) and trash
	directories.

And more analysis of why this wasn't caught in the first place:

	This fixes a regression introduced by 38b074d (t/test-lib.sh:
	fix TRASH_DIRECTORY handling, 2013-04-14).  Before then, the
	TEST_OUTPUT_DIRECTORY setting was not respected consistently
	so most tests did their work in a "trash" subdirectory of the
	current directory instead of the output dir.

Does that make sense?

Thanks, Jonathan