{"thread":{"id":"29830","subject":"[PATCH 1/2] Allow Overriding GIT_BUILD_DIR","startedAt":"2012-03-04T23:23:56Z","lastAt":"2012-03-06T23:12:02Z","messageCount":18,"participants":["greened@obbligato.org","Junio C Hamano","Thomas Rast","David A. Greene"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"186058","messageId":"1330903437-31386-1-git-send-email-greened@obbligato.org","threadId":"29830","inReplyTo":null,"subject":"[PATCH 1/2] Allow Overriding GIT_BUILD_DIR","fromName":"","fromEmail":"greened@obbligato.org","sentAt":"2012-03-04T23:23:56Z","receivedAt":"2012-03-04T23:23:56Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"From: \"David A. Greene\" \n\nLet tests override GIT_BUILD_DIR so git will work if tests are not at\nthe same directory level as standard git tests. Prior to this change,\nGIT_BUILD_DIR is hardwired to be exactly one directory above where the\ntest lives. A test within contrib/, for example, can now use\ntest-lib.sh and set an appropriate value for GIT_BUILD_DIR.\n\nSigned-off-by: David A. Greene \n---\n t/test-lib.sh | 16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex a65dfc7..cb3a0a2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -55,6 +55,7 @@ unset $(perl -e '\n \t\t.*_TEST\n \t\tPROVE\n \t\tVALGRIND\n+ BUILD_DIR\n \t));\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\n@@ -901,7 +902,20 @@ then\n \t# itself.\n \tTEST_DIRECTORY=$(pwd)\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+\n+if test -z \"$GIT_BUILD_DIR\"\n+then\n+\t# We allow tests to override this, in case they want to run tests\n+\t# outside of t/.\n+ \n+ # For in-tree test scripts, this is one level above the\n+ # TEST_DIRECTORY (t/), but a test script that lives outside t/\n+ # can set this variable to point at the right place so that it\n+ # can find t/ directory that house test helpers like\n+ # lib-pager*.sh and test vectors like t4013/ as well as\n+ # previously built git tools.\n+ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+fi\n \n if test -n \"$valgrind\"\n then\n-- \n1.7.9.1\n"},{"id":"186059","messageId":"1330903437-31386-2-git-send-email-greened@obbligato.org","threadId":"29830","inReplyTo":"1330903437-31386-1-git-send-email-greened@obbligato.org","subject":"[PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"","fromEmail":"greened@obbligato.org","sentAt":"2012-03-04T23:23:57Z","receivedAt":"2012-03-04T23:23:57Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"From: \"David A. Greene\" \n\nAllow tests that do not live in the top-level t/ directory to run\nunder valgrind. This requires exporting a couple more variables to\nindicate where the git tools were built and where the valgrind support\nfiles live.\n\nPrior to this chage the valgrind support files were hard-coded to be\nin a sibling directory to where the valgrind tests are run.\n\nAlso prior to this change the base git build was hard-coded to be\nexactly two directories up from where the valgrind tests are run.\n\nSigned-off-by: David A. Greene \n---\n t/test-lib.sh | 22 ++++++++++++++++++++--\n t/valgrind/valgrind.sh | 4 ++--\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cb3a0a2..0ebb3a8 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -56,6 +56,7 @@ unset $(perl -e '\n \t\tPROVE\n \t\tVALGRIND\n BUILD_DIR\n+ VALGRIND_TOOLS\n \t));\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\n@@ -917,6 +918,20 @@ then\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n fi\n \n+# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.\n+if test -z \"$GIT_VALGRIND_TOOLS\"\n+then\n+\t# We allow tests to override this, in case they want to run tests\n+\t# outside of t/.\n+ \n+ # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind\n+ # (t/valgrind), but a test script that lives outside t/ can\n+ # set this variable to point at the right place so that it can\n+ # find t/valgrind directory that house test helpers like\n+ # valgrind.sh.\n+ GIT_VALGRIND_TOOLS=\"$TEST_DIRECTORY\"/valgrind\n+fi\n+\n if test -n \"$valgrind\"\n then\n \tmake_symlink () {\n@@ -954,11 +969,11 @@ then\n \t\t test ! -d \"$symlink_target\" &&\n \t\t test \"#!\" != \"$(head -c 2 < \"$symlink_target\")\"\n \t\tthen\n-\t\t\tsymlink_target=../valgrind.sh\n+\t\t\tsymlink_target=${GIT_VALGRIND_TOOLS}/valgrind.sh\n \t\tfi\n \t\tcase \"$base\" in\n \t\t*.sh|*.perl)\n-\t\t\tsymlink_target=../unprocessed-script\n+\t\t\tsymlink_target=${GIT_VALGRIND_TOOLS}/unprocessed-script\n \t\tesac\n \t\t# create the link, or replace it if it is out of date\n \t\tmake_symlink \"$symlink_target\" \"$GIT_VALGRIND/bin/$base\" || exit\n@@ -986,7 +1001,10 @@ then\n \tIFS=$OLDIFS\n \tPATH=$GIT_VALGRIND/bin:$PATH\n \tGIT_EXEC_PATH=$GIT_VALGRIND/bin\n+\t# Make these available in valgrind.sh\n+\texport GIT_BUILD_DIR\n \texport GIT_VALGRIND\n+\texport GIT_VALGRIND_TOOLS\n elif test -n \"$GIT_TEST_INSTALLED\" ; then\n \tGIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||\n \terror \"Cannot run git from $GIT_TEST_INSTALLED.\"\ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 582b4dc..d638d10 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -13,10 +13,10 @@ TRACK_ORIGINS=--track-origins=yes\n \n exec valgrind -q --error-exitcode=126 \\\n \t--leak-check=no \\\n-\t--suppressions=\"$GIT_VALGRIND/default.supp\" \\\n+\t--suppressions=\"$GIT_VALGRIND_TOOLS/default.supp\" \\\n \t--gen-suppressions=all \\\n \t$TRACK_ORIGINS \\\n \t--log-fd=4 \\\n \t--input-fd=4 \\\n \t$GIT_VALGRIND_OPTIONS \\\n-\t\"$GIT_VALGRIND\"/../../\"$base\" \"$@\"\n+\t\"$GIT_BUILD_DIR\"/\"$base\" \"$@\"\n-- \n1.7.9.1\n"},{"id":"186085","messageId":"7vaa3v4kwo.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"1330903437-31386-1-git-send-email-greened@obbligato.org","subject":"Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-05T06:33:11Z","receivedAt":"2012-03-05T06:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Both of your changes seem to have broken indentation to use 8-SP at\nthe beginning of some (but not all) lines instead 1-HT. I'll queue\na fixed up version and push the result out in 'pu' later, so please\ndouble check to make sure I didn't screw up.\n\nThanks.\n"},{"id":"186087","messageId":"87aa3vzdoc.fsf@thomas.inf.ethz.ch","threadId":"29830","inReplyTo":"1330903437-31386-2-git-send-email-greened@obbligato.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-05T07:53:39Z","receivedAt":"2012-03-05T07:53:39Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"greened@obbligato.org writes:\n\n> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.\n> +if test -z \"$GIT_VALGRIND_TOOLS\"\n> +then\n> +\t# We allow tests to override this, in case they want to run tests\n> +\t# outside of t/.\n> + \n> + # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind\n> + # (t/valgrind), but a test script that lives outside t/ can\n> + # set this variable to point at the right place so that it can\n> + # find t/valgrind directory that house test helpers like\n> + # valgrind.sh.\n> + GIT_VALGRIND_TOOLS=\"$TEST_DIRECTORY\"/valgrind\n> +fi\n\nI'm a bit curious: why isn't it enough to spell that path\n$GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186130","messageId":"nng399m3om6.fsf@transit.us.cray.com","threadId":"29830","inReplyTo":"7vaa3v4kwo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR","fromName":"David A. Greene","fromEmail":"dag@cray.com","sentAt":"2012-03-05T18:10:41Z","receivedAt":"2012-03-05T18:10:41Z","isPatch":true,"sender":{"key":"dag@cray.com","avatar":null},"body":"Junio C Hamano writes:\n\n> Both of your changes seem to have broken indentation to use 8-SP at\n> the beginning of some (but not all) lines instead 1-HT. I'll queue\n> a fixed up version and push the result out in 'pu' later, so please\n> double check to make sure I didn't screw up.\n\nRight. This is because you flagged an indentation issue with the\nprevious version of the patch. I think what happened is that the\nprevious version included the 1-HT (what is HT - half-tab?) spacing but\nit \"looked funny\" with the additional \"+\" from the diff line.\n\nSo in some way this was deliberate to make the patch look better. I\nhave no problem with you (or me!) changing it back.\n\nThanks for taking these up!\n\n -Dave\n"},{"id":"186131","messageId":"nngy5re29zn.fsf@transit.us.cray.com","threadId":"29830","inReplyTo":"87aa3vzdoc.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"David A. Greene","fromEmail":"dag@cray.com","sentAt":"2012-03-05T18:11:56Z","receivedAt":"2012-03-05T18:11:56Z","isPatch":true,"sender":{"key":"dag@cray.com","avatar":null},"body":"Thomas Rast writes:\n\n> greened@obbligato.org writes:\n>\n>> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.\n>> +if test -z \"$GIT_VALGRIND_TOOLS\"\n>> +then\n>> +\t# We allow tests to override this, in case they want to run tests\n>> +\t# outside of t/.\n>> + \n>> + # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind\n>> + # (t/valgrind), but a test script that lives outside t/ can\n>> + # set this variable to point at the right place so that it can\n>> + # find t/valgrind directory that house test helpers like\n>> + # valgrind.sh.\n>> + GIT_VALGRIND_TOOLS=\"$TEST_DIRECTORY\"/valgrind\n>> +fi\n>\n> I'm a bit curious: why isn't it enough to spell that path\n> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?\n\nFor the same reason that TEST_DIRECTORY is different and unrelated from\nGIT_BUILD_DIR. It's my understanding that GIT_BUILD_DIR could end up\nbeing somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.\nAt least that's why I introduced a new parameter.\n\n -Dave\n"},{"id":"186138","messageId":"7vaa3u24lw.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"nng399m3om6.fsf@transit.us.cray.com","subject":"Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-05T20:08:11Z","receivedAt":"2012-03-05T20:08:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"dag@cray.com (David A. Greene) writes:\n\n> Junio C Hamano writes:\n>\n>> Both of your changes seem to have broken indentation to use 8-SP at\n>> the beginning of some (but not all) lines instead 1-HT. I'll queue\n>> a fixed up version and push the result out in 'pu' later, so please\n>> double check to make sure I didn't screw up.\n>\n> Right. This is because you flagged an indentation issue with the\n> previous version of the patch. I think what happened is that the\n> previous version included the 1-HT (what is HT - half-tab?) spacing but\n> it \"looked funny\" with the additional \"+\" from the diff line.\n\nNo, with your earlier patch, all the existing lines used horizontal\ntabs for indenting, and the line you added used runs of spaces.\nWhen such a hunk is shown in diff output, \"+\" will make it obvious\nthat only the new line you added is wrong (because the initial \"+\"\nand \" \" is absorbed in the first horizontal tab for Tab-indented\nlines) and that is how I noticed and pointed out \"a funny\nindentation\" to you.\n"},{"id":"186181","messageId":"878vje86cy.fsf@thomas.inf.ethz.ch","threadId":"29830","inReplyTo":"nngy5re29zn.fsf@transit.us.cray.com","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-06T08:46:05Z","receivedAt":"2012-03-06T08:46:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"dag@cray.com (David A. Greene) writes:\n\n> Thomas Rast writes:\n>\n>> greened@obbligato.org writes:\n>>\n>>> +# GIT_VALGRIND_TOOLS is the location of tools like valgrind.sh.\n>>> +if test -z \"$GIT_VALGRIND_TOOLS\"\n>>> +then\n>>> +\t# We allow tests to override this, in case they want to run tests\n>>> +\t# outside of t/.\n>>> + \n>>> + # For in-tree test scripts, this is in TEST_DIRECTORY/valgrind\n>>> + # (t/valgrind), but a test script that lives outside t/ can\n>>> + # set this variable to point at the right place so that it can\n>>> + # find t/valgrind directory that house test helpers like\n>>> + # valgrind.sh.\n>>> + GIT_VALGRIND_TOOLS=\"$TEST_DIRECTORY\"/valgrind\n>>> +fi\n>>\n>> I'm a bit curious: why isn't it enough to spell that path\n>> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?\n>\n> For the same reason that TEST_DIRECTORY is different and unrelated from\n> GIT_BUILD_DIR. It's my understanding that GIT_BUILD_DIR could end up\n> being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.\n> At least that's why I introduced a new parameter.\n\nI'm just worried that for such a fringe use-case, the maintainer of the\nout-of-tree tests will never notice that he missed to customize *this*\nparticular parameter. So I'd rather have it spelled in terms of the\nexisting two (?).\n\nDon't we, right now, get stuff as follows:\n\n item path\n --------------------------------------------\n test-lib.sh $TEST_DIRECTORY\n git $GIT_BUILD_DIR/bin-wrappers\n valgrind.sh $TEST_DIRECTORY/valgrind\n git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n\nYou are saying this must change to an entirely new path\n\n valgrind.sh $GIT_VALGRIND_TOOLS\n git (with --valgrind) $GIT_VALGRIND_TOOLS/bin\n\nbut what's wrong with simply\n\n valgrind.sh $GIT_BUILD_DIR/t/valgrind\n git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n\nIn the common case of t/, these just map to what we had before. In the\nout-of-tree case, we'd create valgrind/bin in the test directory for the\n*temporary* stuff, and still look for the wrapping valgrind.sh in the\ngit tree.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186214","messageId":"87r4x5izd8.fsf@smith.obbligato.org","threadId":"29830","inReplyTo":"7vaa3u24lw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Allow Overriding GIT_BUILD_DIR","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2012-03-06T14:21:39Z","receivedAt":"2012-03-06T14:21:39Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Junio C Hamano writes:\n\n>> Right. This is because you flagged an indentation issue with the\n>> previous version of the patch. I think what happened is that the\n>> previous version included the 1-HT (what is HT - half-tab?) spacing but\n>> it \"looked funny\" with the additional \"+\" from the diff line.\n>\n> No, with your earlier patch, all the existing lines used horizontal\n> tabs for indenting, and the line you added used runs of spaces.\n> When such a hunk is shown in diff output, \"+\" will make it obvious\n> that only the new line you added is wrong (because the initial \"+\"\n> and \" \" is absorbed in the first horizontal tab for Tab-indented\n> lines) and that is how I noticed and pointed out \"a funny\n> indentation\" to you.\n\nHmm...when I went back to the file it indeed had horizontal tabs. Ah, I\nthink I know what happened. I had to cut-n-paste into an e-mail because\nI couldn't get git send-email to work at the time (it apparently gives\nup after failing to authenticate even if the server presents more than\none authentication method). So I think the mailer might have replaced\ntabs with spaces. I don't know. In any case, it's moot.\n\nYou indicated you'd fix up the patch. I am happy to do that as well if\nyou want a proper re-submission. Just let me know.\n\n -Dave\n"},{"id":"186218","messageId":"87mx7tiyhh.fsf@smith.obbligato.org","threadId":"29830","inReplyTo":"878vje86cy.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2012-03-06T14:40:42Z","receivedAt":"2012-03-06T14:40:42Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Thomas Rast writes:\n\n>>> I'm a bit curious: why isn't it enough to spell that path\n>>> $GIT_BUILD_DIR/t/valgrind instead of making it fully configurable?\n>>\n>> For the same reason that TEST_DIRECTORY is different and unrelated from\n>> GIT_BUILD_DIR. It's my understanding that GIT_BUILD_DIR could end up\n>> being somewhere compeltely unrelated to where TOP_SRC/t/valgrind is.\n>> At least that's why I introduced a new parameter.\n>\n> I'm just worried that for such a fringe use-case, the maintainer of the\n> out-of-tree tests will never notice that he missed to customize *this*\n> particular parameter. So I'd rather have it spelled in terms of the\n> existing two (?).\n\nI understand your concern. Perhaps it could be mitigated with some\n\"HOWTO\" comments at the top of the script. I'm nervous about basing the\nvalue on other variables because that's what limited the script to such\na narrow scope in the first place.\n\n> Don't we, right now, get stuff as follows:\n>\n> item path\n> --------------------------------------------\n> test-lib.sh $TEST_DIRECTORY\n\nRight now, yes, but it breaks for out-of-tree tests. In the out-of-tree\ncase, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in\nt7900-subtree.sh, I do this:\n\n. ../../../t/test-lib.sh\n\nbecause TEST_DIRECTORY is set to some directory under contrib/subtree.\n\n> git $GIT_BUILD_DIR/bin-wrappers\n\nI think so.\n\n> valgrind.sh $TEST_DIRECTORY/valgrind\n\nThat's what it is now and it's wrong for out-of-tree tests.\n\n> git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n\nYep. This is ok.\n\n> You are saying this must change to an entirely new path\n>\n> valgrind.sh $GIT_VALGRIND_TOOLS\n> git (with --valgrind) $GIT_VALGRIND_TOOLS/bin\n\nThe first, yes. The second, no. We can leave that alone.\n\n> but what's wrong with simply\n>\n> valgrind.sh $GIT_BUILD_DIR/t/valgrind\n> git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n\nThese are two separate issues.\n\nGIT_BUILD_DIR may not be anywhere within the source tree, right? If so,\nt/valgrind may not have any relation whatsoever to GIT_BUILD_DIR. Hence\nGIT_VALGRIND_TOOLS.\n\nThe second part is correct. test-lib.sh sets it up that way (~line 983)\nand it works fine for out-of-tree tests.\n\n> In the common case of t/, these just map to what we had before. In the\n> out-of-tree case, we'd create valgrind/bin in the test directory for the\n> *temporary* stuff, and still look for the wrapping valgrind.sh in the\n> git tree.\n\nPutting valgrind/bin in the test directory is fine. There's no change\nthere. It's this looking for the wrapping valgrind.sh that fails in the\ncurrent scheme. We cannot rely on GIT_BUILD_DIR to find it as noted\nabove.\n\n -Dave\n"},{"id":"186235","messageId":"7veht5tvsi.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"878vje86cy.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T18:43:25Z","receivedAt":"2012-03-06T18:43:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Don't we, right now, get stuff as follows:\n>\n> item path\n> --------------------------------------------\n> test-lib.sh $TEST_DIRECTORY\n> git $GIT_BUILD_DIR/bin-wrappers\n> valgrind.sh $TEST_DIRECTORY/valgrind\n> git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n>\n> You are saying this must change to an entirely new path\n>\n> valgrind.sh $GIT_VALGRIND_TOOLS\n> git (with --valgrind) $GIT_VALGRIND_TOOLS/bin\n>\n> but what's wrong with simply\n>\n> valgrind.sh $GIT_BUILD_DIR/t/valgrind\n> git (with --valgrind) $TEST_DIRECTORY/valgrind/bin\n>\n> In the common case of t/, these just map to what we had before. In the\n> out-of-tree case, we'd create valgrind/bin in the test directory for the\n> *temporary* stuff, and still look for the wrapping valgrind.sh in the\n> git tree.\n\nSounds sane. Simple is good.\n\nNo matter what happens to this particular patch, could we have the\nabove table (the final version of it, that is) in t/README or\nsomething, please?\n\nThanks.\n"},{"id":"186236","messageId":"7vaa3ttvj1.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"87mx7tiyhh.fsf@smith.obbligato.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T18:49:06Z","receivedAt":"2012-03-06T18:49:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"greened@obbligato.org (David A. Greene) writes:\n\n>> Don't we, right now, get stuff as follows:\n>>\n>> item path\n>> --------------------------------------------\n>> test-lib.sh $TEST_DIRECTORY\n>\n> Right now, yes, but it breaks for out-of-tree tests. In the out-of-tree\n> case, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in\n\nCould it be that the reason for the breakage is because you are\nsetting TEST_DIRECTORY to the directory that contains out-of-tree\ntests, instead of $GIT_BUILD_DIR/t/ directory?\n\nThe place you perform your operations are relative to $TRASH_DIRECTORY,\nand you would still find the main git relative to $GIT_BUILD_DIR.\n\nShouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?\nWhat do you find relative to $TEST_DIRECTORY that cannot be found\nrelative to GIT_BUILD_DIR/t?\n"},{"id":"186266","messageId":"87hay1fkfk.fsf@smith.obbligato.org","threadId":"29830","inReplyTo":"7vaa3ttvj1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"David A. Greene","fromEmail":"dag@cray.com","sentAt":"2012-03-06T22:12:31Z","receivedAt":"2012-03-06T22:12:31Z","isPatch":true,"sender":{"key":"dag@cray.com","avatar":null},"body":"Junio C Hamano writes:\n\n> greened@obbligato.org (David A. Greene) writes:\n>\n>>> Don't we, right now, get stuff as follows:\n>>>\n>>> item path\n>>> --------------------------------------------\n>>> test-lib.sh $TEST_DIRECTORY\n>>\n>> Right now, yes, but it breaks for out-of-tree tests. In the out-of-tree\n>> case, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in\n>\n> Could it be that the reason for the breakage is because you are\n> setting TEST_DIRECTORY to the directory that contains out-of-tree\n> tests, instead of $GIT_BUILD_DIR/t/ directory?\n\nWell, yes. I thought that's what out-of-tree tests are supposed to do.\nThey don't live in $GIT_BUILD_DIR/t/ after all.\n\nPerhaps I've misunderstood how the test system is supposed to work. A\ntable as you described in README would be most helpful. I thought\nTEST_DIRECTORY is supposed to point to where the tests to run are\nlocated.\n\n> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?\n> What do you find relative to $TEST_DIRECTORY that cannot be found\n> relative to GIT_BUILD_DIR/t?\n\nIf that's what TEST_DIRECTORY is supposed to be, always, then it should\nbe stated in the comments and README. I had no idea this was an\ninvariant.\n\nThanks for clarifying!\n\n -Dave\n"},{"id":"186267","messageId":"7vboo9qskb.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"87hay1fkfk.fsf@smith.obbligato.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T22:21:24Z","receivedAt":"2012-03-06T22:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David A. Greene\" writes:\n\n> Junio C Hamano writes:\n>\n>> greened@obbligato.org (David A. Greene) writes:\n>>\n>>>> Don't we, right now, get stuff as follows:\n>>>>\n>>>> item path\n>>>> --------------------------------------------\n>>>> test-lib.sh $TEST_DIRECTORY\n>>>\n>>> Right now, yes, but it breaks for out-of-tree tests. In the out-of-tree\n>>> case, TEST_DIRECTORY doesn't contain test-lib.sh. For exmaple, in\n>>\n>> Could it be that the reason for the breakage is because you are\n>> setting TEST_DIRECTORY to the directory that contains out-of-tree\n>> tests, instead of $GIT_BUILD_DIR/t/ directory?\n>\n> Well, yes. I thought that's what out-of-tree tests are supposed to do.\n> They don't live in $GIT_BUILD_DIR/t/ after all.\n>\n> Perhaps I've misunderstood how the test system is supposed to work. A\n> table as you described in README would be most helpful. I thought\n> TEST_DIRECTORY is supposed to point to where the tests to run are\n> located.\n>\n>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?\n>> What do you find relative to $TEST_DIRECTORY that cannot be found\n>> relative to GIT_BUILD_DIR/t?\n>\n> If that's what TEST_DIRECTORY is supposed to be, always, then it should\n> be stated in the comments and README. I had no idea this was an\n> invariant.\n>\n> Thanks for clarifying!\n\nNot so fast. The questions in the message you are responding to were\nnot rhetorical.\n"},{"id":"186269","messageId":"7v7gyxqrty.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"7vboo9qskb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T22:37:13Z","receivedAt":"2012-03-06T22:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"David A. Greene\" writes:\n>\n>> Junio C Hamano writes:\n>> ...\n>>> Could it be that the reason for the breakage is because you are\n>>> setting TEST_DIRECTORY to the directory that contains out-of-tree\n>>> tests, instead of $GIT_BUILD_DIR/t/ directory?\n>> ...\n>>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?\n>>> What do you find relative to $TEST_DIRECTORY that cannot be found\n>>> relative to GIT_BUILD_DIR/t?\n>> ...\n>>\n>> Thanks for clarifying!\n>\n> Not so fast. The questions in the message you are responding to were\n> not rhetorical.\n\nSo I ended up looking at t/test-lib.sh, sigh...\n\nWe have this bit from 62f5390 (test-lib: Allow overriding of\nTEST_DIRECTORY, 2010-08-19):\n\n # Test the binaries we have just built. The tests are kept in\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n then\n # We allow tests to override this, in case they want to run tests\n # outside of t/, e.g. for running tests on the test library\n # itself.\n TEST_DIRECTORY=$(pwd)\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n\nthat says \"As a side benefit this change also makes it easy for us\nto move the t/*.sh tests into subdirectories if we ever want to do\nthat.\"\n\nThis expects that an out-of-tree test script is expected to set\nTEST_DIRECTORY before dot-sourcing test-lib.sh, e.g.\n\n\t#!/bin/sh\n\tTEST_DIRECTORY=/srv/project/git/git.git/t\n test_description='an out-of-tree test'\n . \"$TEST_DIRECTORY/test-lib.sh\"\n\nwhich in turn lets the test framework to learn GIT_BUILD_DIR. From\nthere, 'git' will be found in GIT_BUILD_DIR/bin-wrappers and the\nvalgrind variants are found in a similar way.\n\nOne thing that is potentially missing is a way for such an out-of-tree\ntest scripts to ship with supporting material in a separate file,\nrelative to the test script. The in-tree t/t4013-diff-various.sh\nhas its test vectors kept in t/t4013/ directory and finds them by\ndoing\n\n\texpect=\"$TEST_DIRECTORY/t4013/diff.$test\"\n\nThis is because the working directory after test-lib comes back to\nus may not be \"trash\" directory under TEST_DIRECTORY, and ../t4013/\nis not the right way to find it. If an out-of-tree test t9999 wants\nto do something similar, it needs to do something like:\n\n\t#!/bin/sh\n HERE=$(PWD)\n\tTEST_DIRECTORY=/srv/project/git/git.git/t\n test_description='an out-of-tree test'\n . \"$TEST_DIRECTORY/test-lib.sh\"\n\nand find it relative to $HERE, e.g. \"$HERE/../t9999/diff.$test\"\n\nOf course, it would be nice to use a name better than $HERE for such\na purpose ;-)\n"},{"id":"186273","messageId":"87r4x5e3x4.fsf@smith.obbligato.org","threadId":"29830","inReplyTo":"7vboo9qskb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"David A. Greene","fromEmail":"dag@cray.com","sentAt":"2012-03-06T22:54:31Z","receivedAt":"2012-03-06T22:54:31Z","isPatch":true,"sender":{"key":"dag@cray.com","avatar":null},"body":"Junio C Hamano writes:\n\n>>> Could it be that the reason for the breakage is because you are\n>>> setting TEST_DIRECTORY to the directory that contains out-of-tree\n>>> tests, instead of $GIT_BUILD_DIR/t/ directory?\n>>\n>> Well, yes. I thought that's what out-of-tree tests are supposed to do.\n>> They don't live in $GIT_BUILD_DIR/t/ after all.\n>>\n>> Perhaps I've misunderstood how the test system is supposed to work. A\n>> table as you described in README would be most helpful. I thought\n>> TEST_DIRECTORY is supposed to point to where the tests to run are\n>> located.\n>>\n>>> Shouldn't TEST_DIRECTORY merely a short-hand for GIT_BUILD_DIR/t?\n>>> What do you find relative to $TEST_DIRECTORY that cannot be found\n>>> relative to GIT_BUILD_DIR/t?\n>>\n>> If that's what TEST_DIRECTORY is supposed to be, always, then it should\n>> be stated in the comments and README. I had no idea this was an\n>> invariant.\n>>\n>> Thanks for clarifying!\n>\n> Not so fast. The questions in the message you are responding to were\n> not rhetorical.\n\nAh, ok. I don't think I have the proper guru status to answer them. :)\nRegardless, it seems we need some documentation on what each of these\nvariables is.\n\n -Dave\n"},{"id":"186274","messageId":"87mx7te3ng.fsf@smith.obbligato.org","threadId":"29830","inReplyTo":"7v7gyxqrty.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"David A. Greene","fromEmail":"dag@cray.com","sentAt":"2012-03-06T23:00:19Z","receivedAt":"2012-03-06T23:00:19Z","isPatch":true,"sender":{"key":"dag@cray.com","avatar":null},"body":"Junio C Hamano writes:\n\n> that says \"As a side benefit this change also makes it easy for us\n> to move the t/*.sh tests into subdirectories if we ever want to do\n> that.\"\n>\n> This expects that an out-of-tree test script is expected to set\n> TEST_DIRECTORY before dot-sourcing test-lib.sh, e.g.\n>\n> \t#!/bin/sh\n> \tTEST_DIRECTORY=/srv/project/git/git.git/t\n> test_description='an out-of-tree test'\n> . \"$TEST_DIRECTORY/test-lib.sh\"\n\nHmm...I think I missed this part when I originally tried this. I\noriginally tried to set TEST_DIRECTORY in the environment and I\nmisunderstood its role.\n\n> which in turn lets the test framework to learn GIT_BUILD_DIR. From\n> there, 'git' will be found in GIT_BUILD_DIR/bin-wrappers and the\n> valgrind variants are found in a similar way.\n\nOk, I see. So TEST_DIRECTORY is supposed to point to the \"official\"\nlocation of git's tests and testing support files. That wasn't clear to\nme.\n\n> One thing that is potentially missing is a way for such an out-of-tree\n> test scripts to ship with supporting material in a separate file,\n> relative to the test script. The in-tree t/t4013-diff-various.sh\n> has its test vectors kept in t/t4013/ directory and finds them by\n> doing\n>\n> \texpect=\"$TEST_DIRECTORY/t4013/diff.$test\"\n\nRight. I did not run into this issue but I can see how others might.\n\n> This is because the working directory after test-lib comes back to\n> us may not be \"trash\" directory under TEST_DIRECTORY, and ../t4013/\n> is not the right way to find it. If an out-of-tree test t9999 wants\n> to do something similar, it needs to do something like:\n>\n> \t#!/bin/sh\n> HERE=$(PWD)\n> \tTEST_DIRECTORY=/srv/project/git/git.git/t\n> test_description='an out-of-tree test'\n> . \"$TEST_DIRECTORY/test-lib.sh\"\n>\n> and find it relative to $HERE, e.g. \"$HERE/../t9999/diff.$test\"\n\nAhh...\n\n> Of course, it would be nice to use a name better than $HERE for such\n> a purpose ;-)\n\nI think naming is a big issue here. Perhaps TEST_DIRECTORY needs a\nbetter name, something like GIT_TEST_SUPPORT or such?\n\nSo before you apply my patches let me try to restructure the git-subtree\ntests with this newly provided insight and see if I can get it to work.\n\nThanks, Junio!\n\n -Dave\n"},{"id":"186276","messageId":"7v399lqq7x.fsf@alter.siamese.dyndns.org","threadId":"29830","inReplyTo":"87mx7te3ng.fsf@smith.obbligato.org","subject":"Re: [PATCH 2/2] Support Out-Of-Tree Valgrind Tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T23:12:02Z","receivedAt":"2012-03-06T23:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David A. Greene\" writes:\n\n> Ok, I see. So TEST_DIRECTORY is supposed to point to the \"official\"\n> location of git's tests and testing support files. That wasn't clear to\n> me.\n\nThat is how I read the intent of what test-lib.sh does. I do not\nthink it has much to do with official-ness, but more about where you\nfind pieces of the framework from (e.g. diff-lib.sh, lib-gpg.sh,\netc.)\n\n> I think naming is a big issue here. Perhaps TEST_DIRECTORY needs a\n> better name, something like GIT_TEST_SUPPORT or such?\n\nI do not think so; the biggest problem I see is that nobody\ndocumented these variables like Thomas did in the previous message\nwe saw in this thread (and Thomas knew more about them than all\nbecause he added t/perf/ recently and had to play with these\nvariables).\n\nOnce the roles of variables are well understood, I do not think it\nis worth renaming the existing uses.\n"}]}