[PATCH 0/3] t0000 cleanups

[PATCH 0/3] t0000 cleanups

[Jeff King]

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.

It turns out they're leftovers from t0000 running, due to a bad
interaction with some other fixes from last April. The first patch fixes
that.

The second patch is a follow-on cleanup enabled by the first.

The third is an unrelated cleanup that I noticed when I ran t0000 47
times in a row. :)

  [1/3]: t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
  [2/3]: t0000: simplify HARNESS_ACTIVE hack
  [3/3]: t0000: drop "known breakage" test

 t/t0000-basic.sh | 19 +++++++------------
 t/test-lib.sh    |  2 --
 2 files changed, 7 insertions(+), 14 deletions(-)

-Peff

[PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

[Jeff King]

Once upon a time, the test-lib library would create trash
directories in the current working directory, unless we were
explicitly told to put it elsewhere via --root. As a result,
t0000 created the sub-test trash directories inside its own
trash directory.

However, we noticed that this did not cover all cases, since
we would need to respect $TEST_OUTPUT_DIRECTORY even if
--root is not given (or is relative). Commit 38b074d fixed
this to consistently use the full path.

As a result, t0000's sub-tests are now created in git's
original test output directory rather than in our trash
directory. Furthermore, since some of the sub-tests simulate
failures, the trash directories do not get cleaned up, and
the cruft is left in the t/ directory.

We could fix this by passing a new "--root=$TRASH_DIRECTORY"
option to the sub-test. However, we do not want the sub-tests
to write anything at all to git's directory (e.g., they
should not be writing to t/test-results, either, although
this is already handled by separate code).  So the best
solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely
in the sub-test, which covers this case, as well as any
future ones.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0000-basic.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 10be52b..bc4e3e2 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -71,6 +71,8 @@ run_sub_test_lib_test () {
 		cat >>"$name.sh" &&
 		chmod +x "$name.sh" &&
 		export TEST_DIRECTORY &&
+		TEST_OUTPUT_DIRECTORY=$(pwd) &&
+		export TEST_OUTPUT_DIRECTORY &&
 		./"$name.sh" "$@" >out 2>err
 	)
 }
-- 
1.8.5.1.399.g900e7cd

Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

[Jonathan Nieder]

Hi,

Jeff King wrote:

> Once upon a time, the test-lib library would create trash
> directories in the current working directory, unless we were
> explicitly told to put it elsewhere via --root. As a result,
> t0000 created the sub-test trash directories inside its own
> trash directory.
>
> However, we noticed that this did not cover all cases, since
> we would need to respect $TEST_OUTPUT_DIRECTORY even if
> --root is not given (or is relative). Commit 38b074d fixed
> this to consistently use the full path.

So the idea if I am reading correctly is "Instead of relying on the
implicit output directory chosen with chdir, which doesn't even work
any more, set TEST_OUTPUT_DIRECTORY to decide where output for the
sub-tests used by t0000's sanity checks for the test harness go".

I'm not sure I completely understand the regression caused by 38b074d.
Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only
used for the test-results/ directory so the only harm done was some
mixing of test results?

What is the symptom this patch alleviates?

> As a result, t0000's sub-tests are now created in git's
> original test output directory rather than in our trash
> directory.

This might be the source of my confusion.  Is "sub-tests" an
abbreviation for "sub-test trash directories" here?

>            Furthermore, since some of the sub-tests simulate
> failures, the trash directories do not get cleaned up, and
> the cruft is left in the t/ directory.
>
> We could fix this by passing a new "--root=$TRASH_DIRECTORY"
> option to the sub-test. However, we do not want the sub-tests
> to write anything at all to git's directory (e.g., they
> should not be writing to t/test-results, either, although
> this is already handled by separate code).

Ah, HARNESS_ACTIVE prevents output of test-results.

Does the git test harness write something else to
TEST_OUTPUT_DIRECTORY?  Is the idea that using --root would be
functionally equivalent but (1) more confusing and (2) less
futureproof?

>                                            So the best
> solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely
> in the sub-test, which covers this case, as well as any
> future ones.

So, to sum up: if I understand correctly

 - git used to only use TEST_OUTPUT_DIRECTORY to decide where test
   results go.  You'd have to use --root to set a custom location for
   trash directories.

 - in that old setup, t0000 leaves around extra trash directories with
   --root, since the sub-tests inherit the parent test's $root and put
   trash directories there.

 - after 38b074d, that old problem still exists and furthermore
   t0000 leaves around extra trash directories even when --root is not
   in use, since the sub-tests inherit the value of
   TEST_OUTPUT_DIRECTORY from the parent test.

 - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root
   problem) by setting TEST_OUTPUT_DIRECTORY explicitly

Does that sound right?  If so, should sub-tests unset $root, too?

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

[Jonathan Nieder]

Jonathan Nieder wrote:

>  - git used to only use TEST_OUTPUT_DIRECTORY to decide where test
>    results go.  You'd have to use --root to set a custom location for
>    trash directories.
>
>  - in that old setup, t0000 leaves around extra trash directories with
>    --root, since the sub-tests inherit the parent test's $root and put
>    trash directories there.

Nope, since sub-tests are run with fork + exec, which loses $root...

>  - after 38b074d, that old problem still exists and furthermore
>    t0000 leaves around extra trash directories even when --root is not
>    in use, since the sub-tests inherit the value of
>    TEST_OUTPUT_DIRECTORY from the parent test.

... meaning the TEST_OUTPUT_DIRECTORY problem is the only problem....

>  - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root
>    problem) by setting TEST_OUTPUT_DIRECTORY explicitly
>
> Does that sound right?  If so, should sub-tests unset $root, too?

... and there's no need to 'unset root'.

So the patch itself looks right.  I think describing the symptoms up
front would probably be enough to make the commit message less
confusing to read.

Jonathan

Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests

[Jeff King]

On Sat, Dec 28, 2013 at 02:13:13PM -0800, Jonathan Nieder wrote:

> So the idea if I am reading correctly is "Instead of relying on the
> implicit output directory chosen with chdir, which doesn't even work
> any more, set TEST_OUTPUT_DIRECTORY to decide where output for the
> sub-tests used by t0000's sanity checks for the test harness go".

Right.

> I'm not sure I completely understand the regression caused by 38b074d.
> Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only
> used for the test-results/ directory so the only harm done was some
> mixing of test results?

$TEST_OUTPUT_DIRECTORY was actually used in $TRASH_DIRECTORY, but some
code paths properly used $TRASH_DIRECTORY, and some used another
variable that (sometimes) contained a relative form of $TRASH_DIRECTORY.
The creation of the repo was one such code-path.  So there were already
potentially problems before 38b074d (any sub-test looking at its
$TRASH_DIRECTORY variable would find the wrong path), but I do not know
offhand if it could trigger any bugs.

Post-38b074d, we consistently use $TRASH_DIRECTORY (and therefore
respect $TEST_OUTPUT_DIRECTORY) everywhere.

> What is the symptom this patch alleviates?
> 
> > As a result, t0000's sub-tests are now created in git's
> > original test output directory rather than in our trash
> > directory.
> 
> This might be the source of my confusion.  Is "sub-tests" an
> abbreviation for "sub-test trash directories" here?

Yes, I should have said "sub-test trash directories". And I think that
answers your "what is the symptom" question.

> > We could fix this by passing a new "--root=$TRASH_DIRECTORY"
> > option to the sub-test. However, we do not want the sub-tests
> > to write anything at all to git's directory (e.g., they
> > should not be writing to t/test-results, either, although
> > this is already handled by separate code).
> 
> Ah, HARNESS_ACTIVE prevents output of test-results.

Yes. My original notion was "Oh, and this fixes broken test-results,
too!". But then I noticed that it is already handled in a different way.
:)

> Does the git test harness write something else to
> TEST_OUTPUT_DIRECTORY?  Is the idea that using --root would be
> functionally equivalent but (1) more confusing and (2) less
> futureproof?

Exactly. I do not think TEST_OUTPUT_DIRECTORY is used for anything else,
but if someone were to ever add a new use, the sub-tests would almost
certainly want that use to affect only the t0000 trash directory.

> So, to sum up: if I understand correctly

You answered these yourself in your follow-up. :)

> So the patch itself looks right.  I think describing the symptoms up
> front would probably be enough to make the commit message less
> confusing to read.

Would adding the missing "trash directories" wording above be
sufficient?

-Peff

[PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack

[Jeff King]

Commit 517cd55 set HARNESS_ACTIVE unconditionally in
sub-tests, because that value affects the output of
"--verbose". t0000 needs stable output from its sub-tests,
and we may or may not be running under a TAP harness.

That commit made the decision to always set the variable,
since it has another useful side effect, which is
suppressing writes to t/test-results by the sub-tests (which
would just pollute the real results).

Since the last commit, though, the sub-tests have their own
test-results directories, so this is no longer an issue. We
can now update a few comments that are no longer accurate
nor necessary.

We can also revisit the choice of HARNESS_ACTIVE. Since we
must choose one value for stability, it's probably saner to
have it off. This means that future patches could test
things like the test-results writing, or the "--quiet"
option, which is currently ignored when run under a harness.

Signed-off-by: Jeff King <peff@peff.net>
---
I do not have any plans to write such tests, and I'd be OK if we wanted
to stop this just at fixing up the comments. But it took me a while to
figure out what is going on, and I believe unsetting HARNESS_ACTIVE in
the sub-tests is the choice that is least likely to cause somebody in
the future to have to re-figure it out. :)

 t/t0000-basic.sh | 14 +++++---------
 t/test-lib.sh    |  2 --
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index bc4e3e2..e6c5b63 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -50,11 +50,11 @@ run_sub_test_lib_test () {
 	shift 2
 	mkdir "$name" &&
 	(
-		# Pretend we're a test harness.  This prevents
-		# test-lib from writing the counts to a file that will
-		# later be summarized, showing spurious "failed" tests
-		HARNESS_ACTIVE=t &&
-		export HARNESS_ACTIVE &&
+		# Pretend we're not running under a test harness, whether we
+		# are or not. The test-lib output depends on the setting of
+		# this variable, so we need a stable setting under which to run
+		# the sub-test.
+		sane_unset HARNESS_ACTIVE &&
 		cd "$name" &&
 		cat >"$name.sh" <<-EOF &&
 		#!$SHELL_PATH
@@ -235,16 +235,13 @@ test_expect_success 'test --verbose' '
 	grep -v "^Initialized empty" test-verbose/out+ >test-verbose/out &&
 	check_sub_test_lib_test test-verbose <<-\EOF
 	> expecting success: true
-	> Z
 	> ok 1 - passing test
 	> Z
 	> expecting success: echo foo
 	> foo
-	> Z
 	> ok 2 - test with output
 	> Z
 	> expecting success: false
-	> Z
 	> not ok 3 - failing test
 	> #	false
 	> Z
@@ -267,7 +264,6 @@ test_expect_success 'test --verbose-only' '
 	> Z
 	> expecting success: echo foo
 	> foo
-	> Z
 	> ok 2 - test with output
 	> Z
 	> not ok 3 - failing test
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1cf78d5..1531c24 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -481,8 +481,6 @@ test_at_end_hook_ () {
 test_done () {
 	GIT_EXIT_OK=t
 
-	# Note: t0000 relies on $HARNESS_ACTIVE disabling the .counts
-	# output file
 	if test -z "$HARNESS_ACTIVE"
 	then
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
-- 
1.8.5.1.399.g900e7cd

Re: [PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack

[Jonathan Nieder]

Jeff King wrote:

> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -50,11 +50,11 @@ run_sub_test_lib_test () {
>  	shift 2
>  	mkdir "$name" &&
>  	(
> -		# Pretend we're a test harness.  This prevents
> -		# test-lib from writing the counts to a file that will
> -		# later be summarized, showing spurious "failed" tests
> -		HARNESS_ACTIVE=t &&
> -		export HARNESS_ACTIVE &&
> +		# Pretend we're not running under a test harness, whether we
> +		# are or not. The test-lib output depends on the setting of
> +		# this variable, so we need a stable setting under which to run
> +		# the sub-test.
> +		sane_unset HARNESS_ACTIVE &&

Makes sense.

Thanks,
Jonathan

[PATCH 3/3] t0000: drop "known breakage" test

[Jeff King]

Having a simulated "known breakage" test means that the test
suite will always tell us there is a bug to be fixed, even
though it is only simulated.

The right way to test this is in a sub-test, that can also
check that we provide the correct exit status and output.
Fortunately, we already have such a test (added much later
by 5ebf89e).

We could arguably get rid of the simulated success test
immediately above, as well, as it is also redundant with the
tests added in 5ebf89e. However, it does not have the
annoying behavior of the "known breakage" test. It may also
be easier to debug if the test suite is truly broken, since
it is not a test-within-a-test, as the later tests are.

Signed-off-by: Jeff King <peff@peff.net>
---
I am not _that_ bothered by the "known breakage", but AFAICT there is
zero benefit to keeping this redundant test. But maybe I am missing
something.

 t/t0000-basic.sh | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index e6c5b63..a2bb63c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -41,9 +41,6 @@ test_expect_success '.git/objects should have 3 subdirectories' '
 test_expect_success 'success is reported like this' '
 	:
 '
-test_expect_failure 'pretend we have a known breakage' '
-	false
-'
 
 run_sub_test_lib_test () {
 	name="$1" descr="$2" # stdin is the body of the test code
-- 
1.8.5.1.399.g900e7cd

Re: [PATCH 3/3] t0000: drop "known breakage" test

[Jonathan Nieder]

Jeff King wrote:

> I am not _that_ bothered by the "known breakage", but AFAICT there is
> zero benefit to keeping this redundant test.

Devil's advocate: it ensures that anyone wrapping git's tests (like
the old smoketest infrastructure experiment) is able to handle an
expected failure.

But in practice I don't mind the behavior before or after this patch.
If the test harness is that broken, we'll know.  And people writing
code that wraps git's tests can write their own custom sanity-checks.

Re: [PATCH 3/3] t0000: drop "known breakage" test

[Jeff King]

On Sat, Dec 28, 2013 at 12:51:04PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > I am not _that_ bothered by the "known breakage", but AFAICT there is
> > zero benefit to keeping this redundant test.
> 
> Devil's advocate: it ensures that anyone wrapping git's tests (like
> the old smoketest infrastructure experiment) is able to handle an
> expected failure.

Thanks. One of the things I love about open source is that as soon as I
say "I can't see how...", the answer is crowd-sourced for me. :)

That being said, even if the test has a non-zero possible value...

> But in practice I don't mind the behavior before or after this patch.
> If the test harness is that broken, we'll know.  And people writing
> code that wraps git's tests can write their own custom sanity-checks.

...I think for these reasons that the value is smaller than the
disruption caused by the test, and the patch is a net win.

-Peff

Re: [PATCH 0/3] t0000 cleanups

[Jonathan Nieder]

Jeff King wrote:

> 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.  Would it work to replace the commit message with something like
this?

Thanks,
Jonathan

Re: [PATCH 0/3] t0000 cleanups

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> 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.  Would it work to replace the commit message with something like
> this?

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.

    Once upon a time, the test-lib library would create trash
    directories in the current working directory, unless we were
    explicitly told to put it elsewhere via --root. As a result,
    t0000 created the sub-test trash directories inside its own
    trash directory.

    However, we noticed that this did not cover all cases, since
    we would need to respect $TEST_OUTPUT_DIRECTORY even if
    --root is not given (or is relative). Commit 38b074d fixed
    this to consistently use the full path.

    As a result, trash directories used by t0000's sub-tests are now
    created in git's original test output directory rather than in our
    trash directory. Furthermore, since some of the sub-tests simulate
    failures, the trash directories do not get cleaned up, and the cruft
    is left in the t/ directory.

    We could fix this by passing a new "--root=$TRASH_DIRECTORY"
    option to the sub-test. However, we do not want the sub-tests
    to write anything at all to git's directory (e.g., they
    should not be writing to t/test-results, either, although
    this is already handled by separate code).  So the best
    solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely
    in the sub-test, which covers this case, as well as any
    future ones.

Re: [PATCH 0/3] t0000 cleanups

[Jonathan Nieder]

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

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

Re: [PATCH 0/3] t0000 cleanups

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Does that make sense?

Sure, sounds good to me.

Re: [PATCH 0/3] t0000 cleanups

[Jeff King]

On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:

> 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?

I am fine with that format, though...

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

This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not
leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can
find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that
as a default if it is not explicitly set.

The rest of your rewrite looks correct.

-Peff

Re: [PATCH 0/3] t0000 cleanups

[Jonathan Nieder]

Jeff King wrote:
> On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:

>> 	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
[...]
> This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not
> leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can
> find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that
> as a default if it is not explicitly set.

So I should have said something like the following instead:

	These scratch areas for sub-tests should be under the t0000 trash
	directory, but because TEST_OUTPUT_DIRECTORY defaults to
	TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh,
	the sub-test trash directories are created under the toplevel t/
	directory 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.

Thanks for catching it.

Jonathan

Re: [PATCH 0/3] t0000 cleanups

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>> On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:
>
>>> 	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
> [...]
>> This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not
>> leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can
>> find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that
>> as a default if it is not explicitly set.
>
> So I should have said something like the following instead:
>
> 	These scratch areas for sub-tests should be under the t0000 trash
> 	directory, but because TEST_OUTPUT_DIRECTORY defaults to
> 	TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh,
> 	the sub-test trash directories are created under the toplevel t/
> 	directory instead.  Because some of the sub-tests simulate failures,
> 	their trash directories are kept around.

I had a private rewrite queued already, but the above is easier to
read, so I'll replace it with this.

Thanks.

>
> 	Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately
> 	for sub-tests.
>
> Thanks for catching it.
>
> Jonathan

Re: [PATCH 0/3] t0000 cleanups

[Jeff King]

On Thu, Jan 02, 2014 at 02:28:33PM -0800, Jonathan Nieder wrote:

> > This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not
> > leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can
> > find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that
> > as a default if it is not explicitly set.
> 
> So I should have said something like the following instead:
> [...]

Yes, looks good to me.

-Peff