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

From: Jeff King <peff@peff.net>
Date: Dec 29, 2013, 07:17 UTC
Message-ID: <20131229071758.GA31788@sigill.intra.peff.net>
In-Reply-To: <20131228221313.GB5544@google.com>

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.

Show 8 quoted lines

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

Show 7 quoted lines

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