{"thread":{"id":"35579","subject":"[PATCH 0/3] t0000 cleanups","startedAt":"2013-12-28T09:27:32Z","lastAt":"2014-01-03T01:04:44Z","messageCount":18,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"232465","messageId":"20131228092731.GA26337@sigill.intra.peff.net","threadId":"35579","inReplyTo":null,"subject":"[PATCH 0/3] t0000 cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-28T09:27:32Z","receivedAt":"2013-12-28T09:27:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When I want to debug a failing test, I often end up doing:\n\n cd t\n ./t4107- -v -i\n cd tra\n\nThe test names are long, so tab-completing on the trash directory is\nvery helpful. Lately I've noticed that there are a bunch of crufty trash\ndirectories in my t/ directory, which makes my tab-completion more\nannoying.\n\nIt turns out they're leftovers from t0000 running, due to a bad\ninteraction with some other fixes from last April. The first patch fixes\nthat.\n\nThe second patch is a follow-on cleanup enabled by the first.\n\nThe third is an unrelated cleanup that I noticed when I ran t0000 47\ntimes in a row. :)\n\n [1/3]: t0000: set TEST_OUTPUT_DIRECTORY for sub-tests\n [2/3]: t0000: simplify HARNESS_ACTIVE hack\n [3/3]: t0000: drop \"known breakage\" test\n\n t/t0000-basic.sh | 19 +++++++------------\n t/test-lib.sh | 2 --\n 2 files changed, 7 insertions(+), 14 deletions(-)\n\n-Peff\n"},{"id":"232466","messageId":"20131228092915.GA21109@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131228092731.GA26337@sigill.intra.peff.net","subject":"[PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-28T09:29:15Z","receivedAt":"2013-12-28T09:29:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, the test-lib library would create trash\ndirectories in the current working directory, unless we were\nexplicitly told to put it elsewhere via --root. As a result,\nt0000 created the sub-test trash directories inside its own\ntrash directory.\n\nHowever, we noticed that this did not cover all cases, since\nwe would need to respect $TEST_OUTPUT_DIRECTORY even if\n--root is not given (or is relative). Commit 38b074d fixed\nthis to consistently use the full path.\n\nAs a result, t0000's sub-tests are now created in git's\noriginal test output directory rather than in our trash\ndirectory. Furthermore, since some of the sub-tests simulate\nfailures, the trash directories do not get cleaned up, and\nthe cruft is left in the t/ directory.\n\nWe could fix this by passing a new \"--root=$TRASH_DIRECTORY\"\noption to the sub-test. However, we do not want the sub-tests\nto write anything at all to git's directory (e.g., they\nshould not be writing to t/test-results, either, although\nthis is already handled by separate code). So the best\nsolution is to simply reset $TEST_OUTPUT_DIRECTORY entirely\nin the sub-test, which covers this case, as well as any\nfuture ones.\n\nSigned-off-by: Jeff King \n---\n t/t0000-basic.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 10be52b..bc4e3e2 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -71,6 +71,8 @@ run_sub_test_lib_test () {\n \t\tcat >>\"$name.sh\" &&\n \t\tchmod +x \"$name.sh\" &&\n \t\texport TEST_DIRECTORY &&\n+\t\tTEST_OUTPUT_DIRECTORY=$(pwd) &&\n+\t\texport TEST_OUTPUT_DIRECTORY &&\n \t\t./\"$name.sh\" \"$@\" >out 2>err\n \t)\n }\n-- \n1.8.5.1.399.g900e7cd\n"},{"id":"232467","messageId":"20131228093149.GB21109@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131228092731.GA26337@sigill.intra.peff.net","subject":"[PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-28T09:31:49Z","receivedAt":"2013-12-28T09:31:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 517cd55 set HARNESS_ACTIVE unconditionally in\nsub-tests, because that value affects the output of\n\"--verbose\". t0000 needs stable output from its sub-tests,\nand we may or may not be running under a TAP harness.\n\nThat commit made the decision to always set the variable,\nsince it has another useful side effect, which is\nsuppressing writes to t/test-results by the sub-tests (which\nwould just pollute the real results).\n\nSince the last commit, though, the sub-tests have their own\ntest-results directories, so this is no longer an issue. We\ncan now update a few comments that are no longer accurate\nnor necessary.\n\nWe can also revisit the choice of HARNESS_ACTIVE. Since we\nmust choose one value for stability, it's probably saner to\nhave it off. This means that future patches could test\nthings like the test-results writing, or the \"--quiet\"\noption, which is currently ignored when run under a harness.\n\nSigned-off-by: Jeff King \n---\nI do not have any plans to write such tests, and I'd be OK if we wanted\nto stop this just at fixing up the comments. But it took me a while to\nfigure out what is going on, and I believe unsetting HARNESS_ACTIVE in\nthe sub-tests is the choice that is least likely to cause somebody in\nthe future to have to re-figure it out. :)\n\n t/t0000-basic.sh | 14 +++++---------\n t/test-lib.sh | 2 --\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex bc4e3e2..e6c5b63 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -50,11 +50,11 @@ run_sub_test_lib_test () {\n \tshift 2\n \tmkdir \"$name\" &&\n \t(\n-\t\t# Pretend we're a test harness. This prevents\n-\t\t# test-lib from writing the counts to a file that will\n-\t\t# later be summarized, showing spurious \"failed\" tests\n-\t\tHARNESS_ACTIVE=t &&\n-\t\texport HARNESS_ACTIVE &&\n+\t\t# Pretend we're not running under a test harness, whether we\n+\t\t# are or not. The test-lib output depends on the setting of\n+\t\t# this variable, so we need a stable setting under which to run\n+\t\t# the sub-test.\n+\t\tsane_unset HARNESS_ACTIVE &&\n \t\tcd \"$name\" &&\n \t\tcat >\"$name.sh\" <<-EOF &&\n \t\t#!$SHELL_PATH\n@@ -235,16 +235,13 @@ test_expect_success 'test --verbose' '\n \tgrep -v \"^Initialized empty\" test-verbose/out+ >test-verbose/out &&\n \tcheck_sub_test_lib_test test-verbose <<-\\EOF\n \t> expecting success: true\n-\t> Z\n \t> ok 1 - passing test\n \t> Z\n \t> expecting success: echo foo\n \t> foo\n-\t> Z\n \t> ok 2 - test with output\n \t> Z\n \t> expecting success: false\n-\t> Z\n \t> not ok 3 - failing test\n \t> #\tfalse\n \t> Z\n@@ -267,7 +264,6 @@ test_expect_success 'test --verbose-only' '\n \t> Z\n \t> expecting success: echo foo\n \t> foo\n-\t> Z\n \t> ok 2 - test with output\n \t> Z\n \t> not ok 3 - failing test\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1cf78d5..1531c24 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -481,8 +481,6 @@ test_at_end_hook_ () {\n test_done () {\n \tGIT_EXIT_OK=t\n \n-\t# Note: t0000 relies on $HARNESS_ACTIVE disabling the .counts\n-\t# output file\n \tif test -z \"$HARNESS_ACTIVE\"\n \tthen\n \t\ttest_results_dir=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n-- \n1.8.5.1.399.g900e7cd\n"},{"id":"232468","messageId":"20131228093340.GC21109@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131228092731.GA26337@sigill.intra.peff.net","subject":"[PATCH 3/3] t0000: drop \"known breakage\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-28T09:33:40Z","receivedAt":"2013-12-28T09:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Having a simulated \"known breakage\" test means that the test\nsuite will always tell us there is a bug to be fixed, even\nthough it is only simulated.\n\nThe right way to test this is in a sub-test, that can also\ncheck that we provide the correct exit status and output.\nFortunately, we already have such a test (added much later\nby 5ebf89e).\n\nWe could arguably get rid of the simulated success test\nimmediately above, as well, as it is also redundant with the\ntests added in 5ebf89e. However, it does not have the\nannoying behavior of the \"known breakage\" test. It may also\nbe easier to debug if the test suite is truly broken, since\nit is not a test-within-a-test, as the later tests are.\n\nSigned-off-by: Jeff King \n---\nI am not _that_ bothered by the \"known breakage\", but AFAICT there is\nzero benefit to keeping this redundant test. But maybe I am missing\nsomething.\n\n t/t0000-basic.sh | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex e6c5b63..a2bb63c 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -41,9 +41,6 @@ test_expect_success '.git/objects should have 3 subdirectories' '\n test_expect_success 'success is reported like this' '\n \t:\n '\n-test_expect_failure 'pretend we have a known breakage' '\n-\tfalse\n-'\n \n run_sub_test_lib_test () {\n \tname=\"$1\" descr=\"$2\" # stdin is the body of the test code\n-- \n1.8.5.1.399.g900e7cd\n"},{"id":"232492","messageId":"20131228205104.GA5544@google.com","threadId":"35579","inReplyTo":"20131228093340.GC21109@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t0000: drop \"known breakage\" test","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-28T20:51:04Z","receivedAt":"2013-12-28T20:51:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I am not _that_ bothered by the \"known breakage\", but AFAICT there is\n> zero benefit to keeping this redundant test.\n\nDevil's advocate: it ensures that anyone wrapping git's tests (like\nthe old smoketest infrastructure experiment) is able to handle an\nexpected failure.\n\nBut in practice I don't mind the behavior before or after this patch.\nIf the test harness is that broken, we'll know. And people writing\ncode that wraps git's tests can write their own custom sanity-checks.\n"},{"id":"232494","messageId":"20131228221313.GB5544@google.com","threadId":"35579","inReplyTo":"20131228092915.GA21109@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-28T22:13:13Z","receivedAt":"2013-12-28T22:13:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Once upon a time, the test-lib library would create trash\n> directories in the current working directory, unless we were\n> explicitly told to put it elsewhere via --root. As a result,\n> t0000 created the sub-test trash directories inside its own\n> trash directory.\n>\n> However, we noticed that this did not cover all cases, since\n> we would need to respect $TEST_OUTPUT_DIRECTORY even if\n> --root is not given (or is relative). Commit 38b074d fixed\n> this to consistently use the full path.\n\nSo the idea if I am reading correctly is \"Instead of relying on the\nimplicit output directory chosen with chdir, which doesn't even work\nany more, set TEST_OUTPUT_DIRECTORY to decide where output for the\nsub-tests used by t0000's sanity checks for the test harness go\".\n\nI'm not sure I completely understand the regression caused by 38b074d.\nIs the idea that before that commit, TEST_OUTPUT_DIRECTORY was only\nused for the test-results/ directory so the only harm done was some\nmixing of test results?\n\nWhat is the symptom this patch alleviates?\n\n> As a result, t0000's sub-tests are now created in git's\n> original test output directory rather than in our trash\n> directory.\n\nThis might be the source of my confusion. Is \"sub-tests\" an\nabbreviation for \"sub-test trash directories\" here?\n\n> Furthermore, since some of the sub-tests simulate\n> failures, the trash directories do not get cleaned up, and\n> the cruft is left in the t/ directory.\n>\n> We could fix this by passing a new \"--root=$TRASH_DIRECTORY\"\n> option to the sub-test. However, we do not want the sub-tests\n> to write anything at all to git's directory (e.g., they\n> should not be writing to t/test-results, either, although\n> this is already handled by separate code).\n\nAh, HARNESS_ACTIVE prevents output of test-results.\n\nDoes the git test harness write something else to\nTEST_OUTPUT_DIRECTORY? Is the idea that using --root would be\nfunctionally equivalent but (1) more confusing and (2) less\nfutureproof?\n\n> So the best\n> solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely\n> in the sub-test, which covers this case, as well as any\n> future ones.\n\nSo, to sum up: if I understand correctly\n\n - git used to only use TEST_OUTPUT_DIRECTORY to decide where test\n results go. You'd have to use --root to set a custom location for\n trash directories.\n\n - in that old setup, t0000 leaves around extra trash directories with\n --root, since the sub-tests inherit the parent test's $root and put\n trash directories there.\n\n - after 38b074d, that old problem still exists and furthermore\n t0000 leaves around extra trash directories even when --root is not\n in use, since the sub-tests inherit the value of\n TEST_OUTPUT_DIRECTORY from the parent test.\n\n - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root\n problem) by setting TEST_OUTPUT_DIRECTORY explicitly\n\nDoes that sound right? If so, should sub-tests unset $root, too?\n\nThanks and hope that helps,\nJonathan\n"},{"id":"232495","messageId":"20131228221417.GC5544@google.com","threadId":"35579","inReplyTo":"20131228093149.GB21109@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-28T22:14:17Z","receivedAt":"2013-12-28T22:14:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -50,11 +50,11 @@ run_sub_test_lib_test () {\n> \tshift 2\n> \tmkdir \"$name\" &&\n> \t(\n> -\t\t# Pretend we're a test harness. This prevents\n> -\t\t# test-lib from writing the counts to a file that will\n> -\t\t# later be summarized, showing spurious \"failed\" tests\n> -\t\tHARNESS_ACTIVE=t &&\n> -\t\texport HARNESS_ACTIVE &&\n> +\t\t# Pretend we're not running under a test harness, whether we\n> +\t\t# are or not. The test-lib output depends on the setting of\n> +\t\t# this variable, so we need a stable setting under which to run\n> +\t\t# the sub-test.\n> +\t\tsane_unset HARNESS_ACTIVE &&\n\nMakes sense.\n\nThanks,\nJonathan\n"},{"id":"232496","messageId":"20131228222003.GD5544@google.com","threadId":"35579","inReplyTo":"20131228221313.GB5544@google.com","subject":"Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-28T22:20:03Z","receivedAt":"2013-12-28T22:20:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> - git used to only use TEST_OUTPUT_DIRECTORY to decide where test\n> results go. You'd have to use --root to set a custom location for\n> trash directories.\n>\n> - in that old setup, t0000 leaves around extra trash directories with\n> --root, since the sub-tests inherit the parent test's $root and put\n> trash directories there.\n\nNope, since sub-tests are run with fork + exec, which loses $root...\n\n> - after 38b074d, that old problem still exists and furthermore\n> t0000 leaves around extra trash directories even when --root is not\n> in use, since the sub-tests inherit the value of\n> TEST_OUTPUT_DIRECTORY from the parent test.\n\n... meaning the TEST_OUTPUT_DIRECTORY problem is the only problem....\n\n> - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root\n> problem) by setting TEST_OUTPUT_DIRECTORY explicitly\n>\n> Does that sound right? If so, should sub-tests unset $root, too?\n\n... and there's no need to 'unset root'.\n\nSo the patch itself looks right. I think describing the symptoms up\nfront would probably be enough to make the commit message less\nconfusing to read.\n\nJonathan\n"},{"id":"232497","messageId":"20131228222129.GE5544@google.com","threadId":"35579","inReplyTo":"20131228092731.GA26337@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-28T22:21:29Z","receivedAt":"2013-12-28T22:21:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When I want to debug a failing test, I often end up doing:\n>\n> cd t\n> ./t4107- -v -i\n> cd tra\n>\n> The test names are long, so tab-completing on the trash directory is\n> very helpful. Lately I've noticed that there are a bunch of crufty trash\n> directories in my t/ directory, which makes my tab-completion more\n> annoying.\n\nAh, and if I'd read this then I wouldn't have had to be confused at\nall. Would it work to replace the commit message with something like\nthis?\n\nThanks,\nJonathan\n"},{"id":"232498","messageId":"20131229071758.GA31788@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131228221313.GB5544@google.com","subject":"Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-29T07:17:59Z","receivedAt":"2013-12-29T07:17:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2013 at 02:13:13PM -0800, Jonathan Nieder wrote:\n\n> So the idea if I am reading correctly is \"Instead of relying on the\n> implicit output directory chosen with chdir, which doesn't even work\n> any more, set TEST_OUTPUT_DIRECTORY to decide where output for the\n> sub-tests used by t0000's sanity checks for the test harness go\".\n\nRight.\n\n> I'm not sure I completely understand the regression caused by 38b074d.\n> Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only\n> used for the test-results/ directory so the only harm done was some\n> mixing of test results?\n\n$TEST_OUTPUT_DIRECTORY was actually used in $TRASH_DIRECTORY, but some\ncode paths properly used $TRASH_DIRECTORY, and some used another\nvariable that (sometimes) contained a relative form of $TRASH_DIRECTORY.\nThe creation of the repo was one such code-path. So there were already\npotentially problems before 38b074d (any sub-test looking at its\n$TRASH_DIRECTORY variable would find the wrong path), but I do not know\noffhand if it could trigger any bugs.\n\nPost-38b074d, we consistently use $TRASH_DIRECTORY (and therefore\nrespect $TEST_OUTPUT_DIRECTORY) everywhere.\n\n> What is the symptom this patch alleviates?\n> \n> > As a result, t0000's sub-tests are now created in git's\n> > original test output directory rather than in our trash\n> > directory.\n> \n> This might be the source of my confusion. Is \"sub-tests\" an\n> abbreviation for \"sub-test trash directories\" here?\n\nYes, I should have said \"sub-test trash directories\". And I think that\nanswers your \"what is the symptom\" question.\n\n> > We could fix this by passing a new \"--root=$TRASH_DIRECTORY\"\n> > option to the sub-test. However, we do not want the sub-tests\n> > to write anything at all to git's directory (e.g., they\n> > should not be writing to t/test-results, either, although\n> > this is already handled by separate code).\n> \n> Ah, HARNESS_ACTIVE prevents output of test-results.\n\nYes. My original notion was \"Oh, and this fixes broken test-results,\ntoo!\". But then I noticed that it is already handled in a different way.\n:)\n\n> Does the git test harness write something else to\n> TEST_OUTPUT_DIRECTORY? Is the idea that using --root would be\n> functionally equivalent but (1) more confusing and (2) less\n> futureproof?\n\nExactly. I do not think TEST_OUTPUT_DIRECTORY is used for anything else,\nbut if someone were to ever add a new use, the sub-tests would almost\ncertainly want that use to affect only the t0000 trash directory.\n\n> So, to sum up: if I understand correctly\n\nYou answered these yourself in your follow-up. :)\n\n> So the patch itself looks right. I think describing the symptoms up\n> front would probably be enough to make the commit message less\n> confusing to read.\n\nWould adding the missing \"trash directories\" wording above be\nsufficient?\n\n-Peff\n"},{"id":"232499","messageId":"20131229072227.GB31788@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131228205104.GA5544@google.com","subject":"Re: [PATCH 3/3] t0000: drop \"known breakage\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-29T07:22:27Z","receivedAt":"2013-12-29T07:22:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2013 at 12:51:04PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I am not _that_ bothered by the \"known breakage\", but AFAICT there is\n> > zero benefit to keeping this redundant test.\n> \n> Devil's advocate: it ensures that anyone wrapping git's tests (like\n> the old smoketest infrastructure experiment) is able to handle an\n> expected failure.\n\nThanks. One of the things I love about open source is that as soon as I\nsay \"I can't see how...\", the answer is crowd-sourced for me. :)\n\nThat being said, even if the test has a non-zero possible value...\n\n> But in practice I don't mind the behavior before or after this patch.\n> If the test harness is that broken, we'll know. And people writing\n> code that wraps git's tests can write their own custom sanity-checks.\n\n...I think for these reasons that the value is smaller than the\ndisruption caused by the test, and the patch is a net win.\n\n-Peff\n"},{"id":"232515","messageId":"xmqqwqimry2j.fsf@gitster.dls.corp.google.com","threadId":"35579","inReplyTo":"20131228222129.GE5544@google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-30T18:30:28Z","receivedAt":"2013-12-30T18:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>\n>> When I want to debug a failing test, I often end up doing:\n>>\n>> cd t\n>> ./t4107- -v -i\n>> cd tra\n>>\n>> The test names are long, so tab-completing on the trash directory is\n>> very helpful. Lately I've noticed that there are a bunch of crufty trash\n>> directories in my t/ directory, which makes my tab-completion more\n>> annoying.\n>\n> Ah, and if I'd read this then I wouldn't have had to be confused at\n> all. Would it work to replace the commit message with something like\n> this?\n\nThe third paragraph of 1/3 sufficiently covers it, no? We could add\n\"It makes it less convenient to use tab completion 'cd t/tra'\nto go to the trash directory of the failed test to inspect the\nsituation\" after \"... left in the t/ directory.\", though.\n\n Once upon a time, the test-lib library would create trash\n directories in the current working directory, unless we were\n explicitly told to put it elsewhere via --root. As a result,\n t0000 created the sub-test trash directories inside its own\n trash directory.\n\n However, we noticed that this did not cover all cases, since\n we would need to respect $TEST_OUTPUT_DIRECTORY even if\n --root is not given (or is relative). Commit 38b074d fixed\n this to consistently use the full path.\n\n As a result, trash directories used by t0000's sub-tests are now\n created in git's original test output directory rather than in our\n trash directory. Furthermore, since some of the sub-tests simulate\n failures, the trash directories do not get cleaned up, and the cruft\n is left in the t/ directory.\n\n We could fix this by passing a new \"--root=$TRASH_DIRECTORY\"\n option to the sub-test. However, we do not want the sub-tests\n to write anything at all to git's directory (e.g., they\n should not be writing to t/test-results, either, although\n this is already handled by separate code). So the best\n solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely\n in the sub-test, which covers this case, as well as any\n future ones.\n"},{"id":"232516","messageId":"20131230185125.GI20443@google.com","threadId":"35579","inReplyTo":"xmqqwqimry2j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-30T18:51:25Z","receivedAt":"2013-12-30T18:51:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Jeff King wrote:\n\n>>> When I want to debug a failing test, I often end up doing:\n>>>\n>>> cd t\n>>> ./t4107- -v -i\n>>> cd tra\n>>>\n>>> The test names are long, so tab-completing on the trash directory is\n>>> very helpful. Lately I've noticed that there are a bunch of crufty trash\n>>> directories in my t/ directory, which makes my tab-completion more\n>>> annoying.\n>>\n>> Ah, and if I'd read this then I wouldn't have had to be confused at\n>> all.\n[...]\n> The third paragraph of 1/3 sufficiently covers it, no? We could add\n> \"It makes it less convenient to use tab completion 'cd t/tra'\n> to go to the trash directory of the failed test to inspect the\n> situation\" after \"... left in the t/ directory.\", though.\n[4 paragraphs snipped]\n\nI think it can be better, since the commit message left me scratching\nmy head while the patch itself seems pretty simple. How about\nsomething like the following?\n\nFirst, describing the problem:\n\n\tRunning t0000 produces more trash directories than expected\n\tand does not clean up after itself:\n\n\t $ ./t0000-basic.sh\n\t[...]\n\t $ ls -d trash\\ directory.*\n\t trash directory.failing-cleanup\n\t trash directory.mixed-results1\n\t trash directory.mixed-results2\n\t trash directory.partial-pass\n\t trash directory.test-verbose\n\t trash directory.test-verbose-only-2\n\nAnalysis and fix:\n\n\tThese scratch areas for sub-tests should be under the t0000\n\ttrash directory, but because the TEST_OUTPUT_DIRECTORY\n\tsetting from the toplevel test leaks into the environment\n\tthey are created under the toplevel output directory (typically\n\tt/) instead. Because some of the sub-tests simulate failures,\n\ttheir trash directories are kept around.\n\n\tFix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately\n\tfor sub-tests.\n\nAnd then, optionally, describing rejected alternatives:\n\n\tAn alternative fix would be to pass the --root parameter that\n\tonly specifies where to put the trash directories, which would\n\talso work. However, using TEST_OUTPUT_DIRECTORY is more\n\tfutureproof in case tests want to write more output in\n\taddition to the test-results/ (which are already suppressed in\n\tsub-tests using the HARNESS_ACTIVE setting) and trash\n\tdirectories.\n\nAnd more analysis of why this wasn't caught in the first place:\n\n\tThis fixes a regression introduced by 38b074d (t/test-lib.sh:\n\tfix TRASH_DIRECTORY handling, 2013-04-14). Before then, the\n\tTEST_OUTPUT_DIRECTORY setting was not respected consistently\n\tso most tests did their work in a \"trash\" subdirectory of the\n\tcurrent directory instead of the output dir.\n\nDoes that make sense?\n\nThanks,\nJonathan\n"},{"id":"232520","messageId":"xmqqd2kervk4.fsf@gitster.dls.corp.google.com","threadId":"35579","inReplyTo":"20131230185125.GI20443@google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-30T19:24:43Z","receivedAt":"2013-12-30T19:24:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Does that make sense?\n\nSure, sounds good to me.\n"},{"id":"232540","messageId":"20131231103323.GA14823@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20131230185125.GI20443@google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-31T10:33:23Z","receivedAt":"2013-12-31T10:33:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:\n\n> I think it can be better, since the commit message left me scratching\n> my head while the patch itself seems pretty simple. How about\n> something like the following?\n\nI am fine with that format, though...\n\n> Analysis and fix:\n> \n> \tThese scratch areas for sub-tests should be under the t0000\n> \ttrash directory, but because the TEST_OUTPUT_DIRECTORY\n> \tsetting from the toplevel test leaks into the environment\n> \tthey are created under the toplevel output directory (typically\n> \tt/) instead. Because some of the sub-tests simulate failures,\n> \ttheir trash directories are kept around.\n\nThis is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not\nleak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can\nfind test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that\nas a default if it is not explicitly set.\n\nThe rest of your rewrite looks correct.\n\n-Peff\n"},{"id":"232595","messageId":"20140102222833.GR20443@google.com","threadId":"35579","inReplyTo":"20131231103323.GA14823@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-02T22:28:33Z","receivedAt":"2014-01-02T22:28:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:\n\n>> \tThese scratch areas for sub-tests should be under the t0000\n>> \ttrash directory, but because the TEST_OUTPUT_DIRECTORY\n>> \tsetting from the toplevel test leaks\n[...]\n> This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not\n> leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can\n> find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that\n> as a default if it is not explicitly set.\n\nSo I should have said something like the following instead:\n\n\tThese scratch areas for sub-tests should be under the t0000 trash\n\tdirectory, but because TEST_OUTPUT_DIRECTORY defaults to\n\tTEST_DIRECTORY which is exported to help sub-tests find test-lib.sh,\n\tthe sub-test trash directories are created under the toplevel t/\n\tdirectory instead. Because some of the sub-tests simulate failures,\n\ttheir trash directories are kept around.\n\n\tFix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately\n\tfor sub-tests.\n\nThanks for catching it.\n\nJonathan\n"},{"id":"232596","messageId":"xmqqob3unh19.fsf@gitster.dls.corp.google.com","threadId":"35579","inReplyTo":"20140102222833.GR20443@google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T22:41:06Z","receivedAt":"2014-01-02T22:41:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>> On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote:\n>\n>>> \tThese scratch areas for sub-tests should be under the t0000\n>>> \ttrash directory, but because the TEST_OUTPUT_DIRECTORY\n>>> \tsetting from the toplevel test leaks\n> [...]\n>> This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not\n>> leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can\n>> find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that\n>> as a default if it is not explicitly set.\n>\n> So I should have said something like the following instead:\n>\n> \tThese scratch areas for sub-tests should be under the t0000 trash\n> \tdirectory, but because TEST_OUTPUT_DIRECTORY defaults to\n> \tTEST_DIRECTORY which is exported to help sub-tests find test-lib.sh,\n> \tthe sub-test trash directories are created under the toplevel t/\n> \tdirectory instead. Because some of the sub-tests simulate failures,\n> \ttheir trash directories are kept around.\n\nI had a private rewrite queued already, but the above is easier to\nread, so I'll replace it with this.\n\nThanks.\n\n>\n> \tFix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately\n> \tfor sub-tests.\n>\n> Thanks for catching it.\n>\n> Jonathan\n"},{"id":"232603","messageId":"20140103010444.GA13018@sigill.intra.peff.net","threadId":"35579","inReplyTo":"20140102222833.GR20443@google.com","subject":"Re: [PATCH 0/3] t0000 cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-03T01:04:44Z","receivedAt":"2014-01-03T01:04:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2014 at 02:28:33PM -0800, Jonathan Nieder wrote:\n\n> > This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not\n> > leak. t0000 sets $TEST_DIRECTORY (which it must, so the sub-scripts can\n> > find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that\n> > as a default if it is not explicitly set.\n> \n> So I should have said something like the following instead:\n> [...]\n\nYes, looks good to me.\n\n-Peff\n"}]}