{"thread":{"id":"47001","subject":"[PATCH 0/3] making test-suite tracing more useful","startedAt":"2017-10-19T21:01:47Z","lastAt":"2017-12-21T09:47:59Z","messageCount":36,"participants":["Jeff King","Stefan Beller","SZEDER Gábor","Simon Ruderich","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"330681","messageId":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","threadId":"47001","inReplyTo":null,"subject":"[PATCH 0/3] making test-suite tracing more useful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:01:40Z","receivedAt":"2017-10-19T21:01:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I sometimes run git's test suite as part of an automated testing\nprocess. I was hoping to add \"-x\" support to get more details when a\ntest fails (since failures are sometimes hard to reproduce). But I hit a\nfew small snags:\n\n - you have to run with bash, since BASH_XTRACEFD is required to avoid\n failures in some tests when we capture the stderr of shell functions\n or subshells (which get polluted with the \"set -x\" outupt). This\n requirement isn't a big deal for me, but it showed some other\n issues.\n\n - the output with BASH_XTRACEFD is a little confusing; fixed by patch\n 1.\n\n - there's one test that _only_ fails with BASH_XTRACEFD. That's fixed\n in patch 2.\n\n - I wanted to use \"prove\" since the output of \"make -j64 test\" is\n unreadable. But that means using \"--verbose-log\", which was\n incompatible with \"-x\". That's patch 3.\n\nWith these patches, I can now do:\n\n make -j64 test \\\n SHELL_PATH=/bin/bash \\\n GIT_TEST_OPTS=\"--verbose-log -x\" \\\n DEFAULT_TEST_TARGET=prove \\\n GIT_PROVE_OPTS=-j64 || {\n echo \"Failing tests:\"\n echo \"--------------\"\n\t grep -l '[^0]' t/test-results/*.exit |\n\t while read failed; do\n\t base=${failed%.exit}\n\t\t name=${base#t/test-results/}\n\t\t echo \"==> $name\"\n\t\t cat \"$base.out\"\n\t done\n\t exit 1\n }\n\nand get fairly readable output (a nice summary from prove, and then a\ndump of any failing test output).\n\nJohannes, I've seen that you do \"-x\" in the tests that the\ngit-for-windows bot uses to comment on GitHub. You may have seen the\nbogus failure in t5615, which this series should fix (you may also have\nseen the \"set +x\" cruft at the end of each test, which is fixed here,\ntoo).\n\nLars, I think with this it should be possible to turn on \"-x\" for the\nTravis build.\n\n [1/3]: test-lib: silence \"-x\" cleanup under bash\n [2/3]: t5615: avoid re-using descriptor 4\n [3/3]: test-lib: make \"-x\" work with \"--verbose-log\"\n\n t/t5615-alternate-env.sh | 6 +++---\n t/test-lib.sh | 44 ++++++++++++++++++++++++++++++++------------\n 2 files changed, 35 insertions(+), 15 deletions(-)\n\n-Peff\n"},{"id":"330682","messageId":"20171019210301.drphcjxx6koxtyf5@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"[PATCH 1/3] test-lib: silence \"-x\" cleanup under bash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:03:02Z","receivedAt":"2017-10-19T21:03:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the test suite's \"-x\" option is used with bash, we end\nup seeing cleanup cruft in the output:\n\n $ bash t0001-init.sh -x\n [...]\n ++ diff -u expected actual\n + test_eval_ret_=0\n + want_trace\n + test t = t\n + test t = t\n + set +x\n ok 42 - re-init from a linked worktree\n\nThis ranges from mildly annoying (for a successful test) to\ndownright confusing (when we say \"last command exited with\nerror\", but it's really 5 commands back).\n\nWe normally are able to suppress this cleanup. As the\nin-code comment explains, we can't convince the shell not to\nprint it, but we can redirect its stderr elsewhere.\n\nBut since d88785e424 (test-lib: set BASH_XTRACEFD\nautomatically, 2016-05-11), that doesn't hold for bash. It\nsends the \"set -x\" output directly to descriptor 4, not to\nstderr.\n\nWe can fix this by also redirecting descriptor 4, and\npaying close attention to which commands redirect and\nwhich do not (see the updated comment).\n\nTwo alternatives I considered and rejected:\n\n - unsetting and setting BASH_XTRACEFD; doing so closes the\n descriptor, which we must avoid\n\n - we could keep everything in a single block as before,\n redirect 4>/dev/null there, but retain 5>&4 as a copy.\n And then selectively restore 4>&5 for commands which\n should be allowed to trace. This would work, but the\n descriptor swapping seems unnecessarily confusing.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 34 ++++++++++++++++++++++++----------\n 1 file changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9b61f16f7a..3399fc3c88 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -600,26 +600,40 @@ test_eval_inner_ () {\n }\n \n test_eval_ () {\n-\t# We run this block with stderr redirected to avoid extra cruft\n-\t# during a \"-x\" trace. Once in \"set -x\" mode, we cannot prevent\n+\t# If \"-x\" tracing is in effect, then we want to avoid polluting stderr\n+\t# with non-test commands. But once in \"set -x\" mode, we cannot prevent\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n \t#\n-\t# The test itself is run with stderr put back to &4 (so either to\n-\t# /dev/null, or to the original stderr if --verbose was used).\n+\t# There are a few subtleties here:\n+\t#\n+\t# - we have to redirect descriptor 4 in addition to 2, to cover\n+\t# BASH_XTRACEFD\n+\t#\n+\t# - the actual eval has to come before the redirection block (since\n+\t# it needs to see descriptor 4 to set up its stderr)\n+\t#\n+\t# - likewise, any error message we print must be outside the block to\n+\t# access descriptor 4\n+\t#\n+\t# - checking $? has to come immediately after the eval, but it must\n+\t# be _inside_ the block to avoid polluting the \"set -x\" output\n+\t#\n+\n+\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n-\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\tset +x\n-\t\t\tif test \"$test_eval_ret_\" != 0\n-\t\t\tthen\n-\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\t\t\tfi\n \t\tfi\n-\t} 2>/dev/null\n+\t} 2>/dev/null 4>&2\n+\n+\tif test \"$test_eval_ret_\" != 0 && want_trace\n+\tthen\n+\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\tfi\n \treturn $test_eval_ret_\n }\n \n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330684","messageId":"20171019210730.sgm4g4tmor2dgjv7@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"[PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:07:31Z","receivedAt":"2017-10-19T21:07:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"File descriptors 3 and 4 are special in our test suite, as\nthey link back to the test script's original stdout and\nstderr. Normally this isn't something tests need to worry\nabout: they are free to clobber these descriptors for\nsub-commands without affecting the overall script.\n\nBut there's one very special thing about descriptor 4: since\nd88785e424 (test-lib: set BASH_XTRACEFD automatically,\n2016-05-11), we ask bash to output \"set -x\" output to it by\nnumber. This goes to _any_ descriptor 4, even if it no\nlonger points to the place it did when we set BASH_XTRACEFD.\n\nBut in t5615, we run a shell loop with descriptor 4\nredirected. As a result, t5615 works with non-bash shells\neven with \"-x\". And it works with bash without \"-x\". But the\ncombination of \"bash t5615-alternate-env.sh -x\" gets a test\nfailure (because our \"set -x\" output pollutes one of the\nfiles).\n\nWe can fix this by using any descriptor _except_ the magical\n4. So let's switch arbitrarily to using 5/6 in this loop,\nnot 3/4.\n\nSigned-off-by: Jeff King \n---\nI also considered trying to bump the \"set -x\" output descriptor to \"9\".\nThat just moves the problem around, but presumably scripts are less\nlikely to go that high. :)\n\nIt would also be possible to pick something insanely high, like \"999\".\nMany shells choke on descriptors higher than 9, but since the issue is\nrelated to BASH_XTRACEFD, we could make it a bash-only thing. I don't\nknow if it's worth the trouble. I hate leaving a subtle \"don't use\ndescriptor 4 in a subshell or your script will break\" hand-grenade like\nthis lying around, but we do seem to have only one instance of it over\nthe whole test suite.\n\n t/t5615-alternate-env.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh\nindex d2d883f3a1..b4905b822c 100755\n--- a/t/t5615-alternate-env.sh\n+++ b/t/t5615-alternate-env.sh\n@@ -7,9 +7,9 @@ check_obj () {\n \talt=$1; shift\n \twhile read obj expect\n \tdo\n-\t\techo \"$obj\" >&3 &&\n-\t\techo \"$obj $expect\" >&4\n-\tdone 3>input 4>expect &&\n+\t\techo \"$obj\" >&5 &&\n+\t\techo \"$obj $expect\" >&6\n+\tdone 5>input 6>expect &&\n \tGIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \\\n \t\tgit \"$@\" cat-file --batch-check='%(objectname) %(objecttype)' \\\n \t\tactual &&\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330685","messageId":"20171019210812.lje3o64v4qdnzcuc@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"[PATCH 3/3] test-lib: make \"-x\" work with \"--verbose-log\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:08:13Z","receivedAt":"2017-10-19T21:08:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-x\" tracing option implies \"--verbose\". This is a\nproblem when running under a TAP harness like \"prove\", where\nwe need to use \"--verbose-log\" instead. Instead, let's\nhandle this the same way we do for --valgrind, including the\nrecent fix from 88c6e9d31c (test-lib: --valgrind should not\noverride --verbose-log, 2017-09-05). Namely, let's enable\n--verbose only when we there isn't a more specific verbosity\noption indicated.\n\nNote that we also have to tweak `want_trace` to turn it on\n(we re-check $verbose in each test because that's how we\nhandle --verbose-only, but --verbose-log is always on for\nall tests).\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 3399fc3c88..14fac6d6f2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -263,7 +263,6 @@ do\n \t\tshift ;;\n \t-x)\n \t\ttrace=t\n-\t\tverbose=t\n \t\tshift ;;\n \t--verbose-log)\n \t\tverbose_log=t\n@@ -282,6 +281,11 @@ then\n \ttest -z \"$verbose_log\" && verbose=t\n fi\n \n+if test -n \"$trace\" && test -z \"$verbose_log\"\n+then\n+\tverbose=t\n+fi\n+\n if test -n \"$color\"\n then\n \t# Save the color control sequences now rather than run tput\n@@ -585,7 +589,9 @@ maybe_setup_valgrind () {\n }\n \n want_trace () {\n-\ttest \"$trace\" = t && test \"$verbose\" = t\n+\ttest \"$trace\" = t && {\n+\t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n+\t}\n }\n \n # This is a separate function because some tests use\n-- \n2.15.0.rc1.560.g5f0609e481\n"},{"id":"330692","messageId":"CAGZ79kYTmUnM+fcf222-cvwB3Fg4+J_xy28E7A3AYryx4qDf_w@mail.gmail.com","threadId":"47001","inReplyTo":"20171019210730.sgm4g4tmor2dgjv7@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T21:46:33Z","receivedAt":"2017-10-19T21:46:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I also considered trying to bump the \"set -x\" output descriptor to \"9\".\n> That just moves the problem around, but presumably scripts are less\n> likely to go that high. :)\n>\n> It would also be possible to pick something insanely high, like \"999\".\n> Many shells choke on descriptors higher than 9, but since the issue is\n> related to BASH_XTRACEFD, we could make it a bash-only thing. I don't\n> know if it's worth the trouble. I hate leaving a subtle \"don't use\n> descriptor 4 in a subshell or your script will break\" hand-grenade like\n> this lying around, but we do seem to have only one instance of it over\n> the whole test suite.\n\nI would imagine that a higher fd for BASH_XTRACEFD\nwould be less explosive than requiring the tests to skip\nthe low number 4. (It is not *that* low. In e.g. git-submodule.sh\nwe make use of 3 in cmd_foreach, not needing more.)\n\nThanks,\nStefan\n"},{"id":"330698","messageId":"20171019232337.zfd7occtjboem7f4@sigill.intra.peff.net","threadId":"47001","inReplyTo":"CAGZ79kYTmUnM+fcf222-cvwB3Fg4+J_xy28E7A3AYryx4qDf_w@mail.gmail.com","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T23:23:37Z","receivedAt":"2017-10-19T23:23:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:46:33PM -0700, Stefan Beller wrote:\n\n> > I also considered trying to bump the \"set -x\" output descriptor to \"9\".\n> > That just moves the problem around, but presumably scripts are less\n> > likely to go that high. :)\n> >\n> > It would also be possible to pick something insanely high, like \"999\".\n> > Many shells choke on descriptors higher than 9, but since the issue is\n> > related to BASH_XTRACEFD, we could make it a bash-only thing. I don't\n> > know if it's worth the trouble. I hate leaving a subtle \"don't use\n> > descriptor 4 in a subshell or your script will break\" hand-grenade like\n> > this lying around, but we do seem to have only one instance of it over\n> > the whole test suite.\n> \n> I would imagine that a higher fd for BASH_XTRACEFD\n> would be less explosive than requiring the tests to skip\n> the low number 4. (It is not *that* low. In e.g. git-submodule.sh\n> we make use of 3 in cmd_foreach, not needing more.)\n\nSo one trick is that we can't just set it to a higher number. We have to\nalso open and manage that descriptor. It might be enough to do:\n\n if test -n \"$BASH_VERSION\"\n then\n\texec 999>&4\n\tBASH_XTRACEFD=999\n fi\n\nbut since we fiddle with descriptors on a per-test basis, I'm not sure\nif that's it or not. I think for convincing output to go to it, it\nprobably is. We tend to point things _at_ descriptor 4, not point\ndescriptor 4 elsewhere. But the fix in patch 1 is an exception, where we\ntry to suppress the \"set +x\" output. For that we have to redirect 999,\ntoo (which is hard because \">&999\" is syntactically invalid in other\nshells, so we have to follow a separate code path for bash or get into\nevals).\n\nOr start playing games with unsetting BASH_XTRACEFD (which closes 999,\nand then we re-open 999>&4 and reset XTRACEFD).\n\nI think it might be workable, but I'm worried we're opening a can of\nworms. Or continuing to dig into the can of worms from the original\nBASH_XTRACEFD, if you prefer. :)\n\n-Peff\n"},{"id":"330713","messageId":"20171020055058.s5dgxyfjnqtlxv4d@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019232337.zfd7occtjboem7f4@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-20T05:50:58Z","receivedAt":"2017-10-20T05:51:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 07:23:37PM -0400, Jeff King wrote:\n\n> So one trick is that we can't just set it to a higher number. We have to\n> also open and manage that descriptor. It might be enough to do:\n> \n> if test -n \"$BASH_VERSION\"\n> then\n> \texec 999>&4\n> \tBASH_XTRACEFD=999\n> fi\n> [...]\n> I think it might be workable, but I'm worried we're opening a can of\n> worms. Or continuing to dig into the can of worms from the original\n> BASH_XTRACEFD, if you prefer. :)\n\nSo this is the best I came up with (on top of my other patches for\nease of reading, though I'd re-work them if this is the route we're\ngoing to go):\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 14fac6d6f2..833ceaefd2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -377,7 +377,12 @@ fi\n #\n # Note also that we don't need or want to export it. The tracing is local to\n # this shell, and we would not want to influence any shells we exec.\n-BASH_XTRACEFD=4\n+if test -n \"$BASH_VERSION\"\n+then\n+\texec 999>&4\n+\tBASH_XTRACEFD=999\n+\tsilence_trace=\"999>/dev/null\"\n+fi\n \n test_failure=0\n test_count=0\n@@ -627,14 +632,16 @@ test_eval_ () {\n \t# be _inside_ the block to avoid polluting the \"set -x\" output\n \t#\n \n-\ttest_eval_inner_ \"$@\" &3 2>&4\n-\t{\n-\t\ttest_eval_ret_=$?\n-\t\tif want_trace\n-\t\tthen\n-\t\t\tset +x\n-\t\tfi\n-\t} 2>/dev/null 4>&2\n+\teval '\n+\t\ttest_eval_inner_ \"$@\" &3 2>&4\n+\t\t{\n+\t\t\ttest_eval_ret_=$?\n+\t\t\tif want_trace\n+\t\t\tthen\n+\t\t\t\tset +x\n+\t\t\tfi\n+\t\t} 2>/dev/null '\"$silence_trace\"'\n+\t'\n \n \tif test \"$test_eval_ret_\" != 0 && want_trace\n \tthen\n\nI really hate that extra eval, since it adds an extra layer of \"+\" to\nthe tracing output in bash.\n\nBut I can't find a way to make it work without it. The \"999>/dev/null\"\nis syntactically bogus in non-bash shells, so it _must_ appear as part\nof an eval. We can't conditionally stick it at the end of the eval run\nby test_eval_inner_, because that one is running the actual test code\n(so it may bail early with \"return\", for example).\n\nTBH, I'm not 100% sure that the initial \"exec 999>&4\" won't cause\ncomplaints in some shells. Dash seems to handle it, but others might\nnot. That can be worked around with another eval, though.\n\nSo I dunno. It does solve the problem in a way that the individual test\nscripts wouldn't have to care about. But it's a lot of eval trickery.\n\n-Peff\n"},{"id":"330743","messageId":"CAGZ79kaSu+nHHntFPWbkXk1TC4ys9htesRg9HHXcpK+Y6MwZeg@mail.gmail.com","threadId":"47001","inReplyTo":"20171020055058.s5dgxyfjnqtlxv4d@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-20T21:27:40Z","receivedAt":"2017-10-20T21:27:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 10:50 PM, Jeff King wrote:\n> On Thu, Oct 19, 2017 at 07:23:37PM -0400, Jeff King wrote:\n>\n>> So one trick is that we can't just set it to a higher number. We have to\n>> also open and manage that descriptor. It might be enough to do:\n>>\n>> if test -n \"$BASH_VERSION\"\n>> then\n>> exec 999>&4\n>> BASH_XTRACEFD=999\n>> fi\n>> [...]\n>> I think it might be workable, but I'm worried we're opening a can of\n>> worms. Or continuing to dig into the can of worms from the original\n>> BASH_XTRACEFD, if you prefer. :)\n>\n> So this is the best I came up with (on top of my other patches for\n> ease of reading, though I'd re-work them if this is the route we're\n> going to go):\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 14fac6d6f2..833ceaefd2 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -377,7 +377,12 @@ fi\n> #\n> # Note also that we don't need or want to export it. The tracing is local to\n> # this shell, and we would not want to influence any shells we exec.\n> -BASH_XTRACEFD=4\n> +if test -n \"$BASH_VERSION\"\n> +then\n> + exec 999>&4\n> + BASH_XTRACEFD=999\n> + silence_trace=\"999>/dev/null\"\n> +fi\n>\n> test_failure=0\n> test_count=0\n> @@ -627,14 +632,16 @@ test_eval_ () {\n> # be _inside_ the block to avoid polluting the \"set -x\" output\n> #\n>\n> - test_eval_inner_ \"$@\" &3 2>&4\n> - {\n> - test_eval_ret_=$?\n> - if want_trace\n> - then\n> - set +x\n> - fi\n> - } 2>/dev/null 4>&2\n> + eval '\n> + test_eval_inner_ \"$@\" &3 2>&4\n> + {\n> + test_eval_ret_=$?\n> + if want_trace\n> + then\n> + set +x\n> + fi\n> + } 2>/dev/null '\"$silence_trace\"'\n> + '\n>\n> if test \"$test_eval_ret_\" != 0 && want_trace\n> then\n>\n> I really hate that extra eval, since it adds an extra layer of \"+\" to\n> the tracing output in bash.\n..\n> So I dunno. It does solve the problem in a way that the individual test\n> scripts wouldn't have to care about. But it's a lot of eval trickery.\n>\n\nI agree. Maybe just stick with the original patch?\n\nThanks,\nStefan\n"},{"id":"330753","messageId":"20171020224608.3ataitvdq5r33iih@sigill.intra.peff.net","threadId":"47001","inReplyTo":"CAGZ79kaSu+nHHntFPWbkXk1TC4ys9htesRg9HHXcpK+Y6MwZeg@mail.gmail.com","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-20T22:46:08Z","receivedAt":"2017-10-20T22:46:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2017 at 02:27:40PM -0700, Stefan Beller wrote:\n\n> > So I dunno. It does solve the problem in a way that the individual test\n> > scripts wouldn't have to care about. But it's a lot of eval trickery.\n> \n> I agree. Maybe just stick with the original patch?\n\nOK. Why don't we live with that for now, then. The only advantage of the\n\"999\" trickery is that it's less likely to come up again. If it doesn't,\nthen we're happy. If it does, then we can always switch then.\n\nI also noticed that our GIT_TRACE=4 trickery has the same problem (it\ndidn't trigger in the t5615 case because it doesn't actually run git\ninside the subshell). If we end up doing a \"999\" descriptor eventually,\nwe should probably point GIT_TRACE at it, too.\n\n-Peff\n"},{"id":"330754","messageId":"20171020225340.6qgybldewi7knxvz@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"[PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-20T22:53:40Z","receivedAt":"2017-10-20T22:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:\n\n> I sometimes run git's test suite as part of an automated testing\n> process. I was hoping to add \"-x\" support to get more details when a\n> test fails (since failures are sometimes hard to reproduce). But I hit a\n> few small snags:\n> \n> - you have to run with bash, since BASH_XTRACEFD is required to avoid\n> failures in some tests when we capture the stderr of shell functions\n> or subshells (which get polluted with the \"set -x\" outupt). This\n> requirement isn't a big deal for me, but it showed some other\n> issues.\n\nActually, this did lead me to one more fix.\n\nI was building with SHELL_PATH=/bin/bash to make BASH_XTRACEFD work. But\nthat impacts not only the test scripts, but also the build itself.\nI'd prefer to test something closer to my normal builds (which use\nbin/sh). This patch lets me do:\n\n make \\\n TEST_SHELL_PATH=/bin/bash \\\n GIT_TEST_OPTS=\"--verbose-log -x\" \\\n test\n\nto run the whole test suite using /bin/sh for the build, for getting the\nbenefits of bash's \"-x\" tracing.\n\n-- >8 --\nSubject: [PATCH] t/Makefile: introduce TEST_SHELL_PATH\n\nYou may want to run the test suite with a different shell\nthan you use to build Git. For instance, you may build with\nSHELL_PATH=/bin/sh (because it's faster, or it's what you\nexpect to exist on systems where the build will be used) but\nwant to run the test suite with bash (e.g., since that\nallows using \"-x\" reliably across the whole test suite).\nThere's currently no good way to do this.\n\nYou might think that doing two separate make invocations,\nlike:\n\n make &&\n make -C t SHELL_PATH=/bin/bash\n\nwould work. And it _almost_ does. The second make will see\nour bash SHELL_PATH, and we'll use that to run the\nindividual test scripts (or tell prove to use it to do so).\nSo far so good.\n\nBut this breaks down when \"--tee\" or \"--verbose-log\" is\nused. Those options cause the test script to actually\nre-exec itself using $SHELL_PATH. But wait, wouldn't our\nsecond make invocation have set SHELL_PATH correctly in the\nenvironment?\n\nYes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we\nbuilt during the first \"make\". And that overrides the\nenvironment, giving us the original SHELL_PATH again.\n\nLet's introduce a new variable that lets you specify a\nspecific shell to be run for the test scripts. Note that we\nhave to touch both the main and t/ Makefiles, since we have\nto record it in GIT-BUILD-OPTIONS in one, and use it in the\nlatter.\n\nSigned-off-by: Jeff King \n---\n Makefile | 8 ++++++++\n t/Makefile | 6 ++++--\n t/test-lib.sh | 2 +-\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex cd75985991..9baa3c4b50 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -425,6 +425,10 @@ all::\n #\n # to say \"export LESS=FRX (and LV=-c) if the environment variable\n # LESS (and LV) is not set, respectively\".\n+#\n+# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for\n+# running the test scripts (e.g., bash has better support for \"set -x\"\n+# tracing).\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -727,6 +731,8 @@ endif\n export PERL_PATH\n export PYTHON_PATH\n \n+TEST_SHELL_PATH = $(SHELL_PATH)\n+\n LIB_FILE = libgit.a\n XDIFF_LIB = xdiff/lib.a\n VCSSVN_LIB = vcs-svn/lib.a\n@@ -1721,6 +1727,7 @@ prefix_SQ = $(subst ','\\'',$(prefix))\n gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))\n \n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n PYTHON_PATH_SQ = $(subst ','\\'',$(PYTHON_PATH))\n TCLTK_PATH_SQ = $(subst ','\\'',$(TCLTK_PATH))\n@@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n # and the first level quoting from the shell that runs \"echo\".\n GIT-BUILD-OPTIONS: FORCE\n \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n+\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n \t@echo PERL_PATH=\\''$(subst ','\\'',$(PERL_PATH_SQ))'\\' >>$@+\n \t@echo DIFF=\\''$(subst ','\\'',$(subst ','\\'',$(DIFF)))'\\' >>$@+\n \t@echo PYTHON_PATH=\\''$(subst ','\\'',$(PYTHON_PATH_SQ))'\\' >>$@+\ndiff --git a/t/Makefile b/t/Makefile\nindex 1bb06c36f2..96317a35f4 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -8,6 +8,7 @@\n \n #GIT_TEST_OPTS = --verbose --debug\n SHELL_PATH ?= $(SHELL)\n+TEST_SHELL_PATH ?= $(SHELL_PATH)\n PERL_PATH ?= /usr/bin/perl\n TAR ?= $(TAR)\n RM ?= rm -f\n@@ -23,6 +24,7 @@ endif\n \n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n TEST_RESULTS_DIRECTORY_SQ = $(subst ','\\'',$(TEST_RESULTS_DIRECTORY))\n \n@@ -42,11 +44,11 @@ failed:\n \ttest -z \"$$failed\" || $(MAKE) $$failed\n \n prove: pre-clean $(TEST_LINT)\n-\t@echo \"*** prove ***\"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n+\t@echo \"*** prove ***\"; $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n \t$(MAKE) clean-except-prove-cache\n \n $(T):\n-\t@echo \"*** $@ ***\"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n+\t@echo \"*** $@ ***\"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n \n pre-clean:\n \t$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 14fac6d6f2..e373ad1e12 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -80,7 +80,7 @@ done,*)\n \t# from any previous runs.\n \t>\"$GIT_TEST_TEE_OUTPUT_FILE\"\n \n-\t(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} \"$0\" \"$@\" 2>&1;\n+\t(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} \"$0\" \"$@\" 2>&1;\n \t echo $? >\"$BASE.exit\") | tee -a \"$GIT_TEST_TEE_OUTPUT_FILE\"\n \ttest \"$(cat \"$BASE.exit\")\" = 0\n \texit\n-- \n2.15.0.rc2.500.g047c67c30e\n\n"},{"id":"330756","messageId":"20171020235001.5072-1-szeder.dev@gmail.com","threadId":"47001","inReplyTo":"20171020225340.6qgybldewi7knxvz@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-10-20T23:50:01Z","receivedAt":"2017-10-20T23:50:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:\n> \n> > I sometimes run git's test suite as part of an automated testing\n> > process. I was hoping to add \"-x\" support to get more details when a\n> > test fails (since failures are sometimes hard to reproduce).\n\nWould it make sense (or be feasible) to enable \"-x\" on Travis CI?\n\n\n> diff --git a/Makefile b/Makefile\n> index cd75985991..9baa3c4b50 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -425,6 +425,10 @@ all::\n> #\n> # to say \"export LESS=FRX (and LV=-c) if the environment variable\n> # LESS (and LV) is not set, respectively\".\n> +#\n> +# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for\n> +# running the test scripts (e.g., bash has better support for \"set -x\"\n> +# tracing).\n> \n> GIT-VERSION-FILE: FORCE\n> \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n> @@ -727,6 +731,8 @@ endif\n> export PERL_PATH\n> export PYTHON_PATH\n> \n> +TEST_SHELL_PATH = $(SHELL_PATH)\n> +\n> LIB_FILE = libgit.a\n> XDIFF_LIB = xdiff/lib.a\n> VCSSVN_LIB = vcs-svn/lib.a\n> @@ -1721,6 +1727,7 @@ prefix_SQ = $(subst ','\\'',$(prefix))\n> gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))\n> \n> SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n> +TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n> PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n> PYTHON_PATH_SQ = $(subst ','\\'',$(PYTHON_PATH))\n> TCLTK_PATH_SQ = $(subst ','\\'',$(TCLTK_PATH))\n> @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n> # and the first level quoting from the shell that runs \"echo\".\n> GIT-BUILD-OPTIONS: FORCE\n> \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n> +\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n\nThis redirection overwrites, loosing the just written SHELL_PATH. It\nshould append like the lines below.\n\n> \t@echo PERL_PATH=\\''$(subst ','\\'',$(PERL_PATH_SQ))'\\' >>$@+\n> \t@echo DIFF=\\''$(subst ','\\'',$(subst ','\\'',$(DIFF)))'\\' >>$@+\n> \t@echo PYTHON_PATH=\\''$(subst ','\\'',$(PYTHON_PATH_SQ))'\\' >>$@+\n\n\nGábor\n\n"},{"id":"330757","messageId":"20171021001916.bohcqjuxcufxvpam@ruderich.org","threadId":"47001","inReplyTo":"20171020224608.3ataitvdq5r33iih@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-10-21T00:19:16Z","receivedAt":"2017-10-21T00:19:23Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:\n>> I agree. Maybe just stick with the original patch?\n>\n> OK. Why don't we live with that for now, then. The only advantage of the\n> \"999\" trickery is that it's less likely to come up again. If it doesn't,\n> then we're happy. If it does, then we can always switch then.\n\nI think switching the 4 to 9 (which you already brought up in\nthis thread) is a good idea. It makes accidental conflicts less\nlikely (it's rare to use so many file descriptors) and is easy to\nimplement.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"330761","messageId":"xmqq7evpgswz.fsf@gitster.mtv.corp.google.com","threadId":"47001","inReplyTo":"20171021001916.bohcqjuxcufxvpam@ruderich.org","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T02:02:04Z","receivedAt":"2017-10-21T02:02:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich writes:\n\n> On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:\n>>> I agree. Maybe just stick with the original patch?\n>>\n>> OK. Why don't we live with that for now, then. The only advantage of the\n>> \"999\" trickery is that it's less likely to come up again. If it doesn't,\n>> then we're happy. If it does, then we can always switch then.\n>\n> I think switching the 4 to 9 (which you already brought up in\n> this thread) is a good idea. It makes accidental conflicts less\n> likely (it's rare to use so many file descriptors) and is easy to\n> implement.\n\nYeah, I like the simplicity of implementation, and I more like the\nfact that it is simpler to reason about its limitation.\n"},{"id":"330770","messageId":"20171021031258.m5skrtfk6nqlk567@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171020235001.5072-1-szeder.dev@gmail.com","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-21T03:12:59Z","receivedAt":"2017-10-21T03:13:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 21, 2017 at 01:50:01AM +0200, SZEDER Gábor wrote:\n\n> > On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:\n> > \n> > > I sometimes run git's test suite as part of an automated testing\n> > > process. I was hoping to add \"-x\" support to get more details when a\n> > > test fails (since failures are sometimes hard to reproduce).\n> \n> Would it make sense (or be feasible) to enable \"-x\" on Travis CI?\n\nYes, after this series I think it may be workable to do so.\n\n> > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n> > # and the first level quoting from the shell that runs \"echo\".\n> > GIT-BUILD-OPTIONS: FORCE\n> > \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n> > +\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n> \n> This redirection overwrites, loosing the just written SHELL_PATH. It\n> should append like the lines below.\n\nDoh, thank you. It's interesting that nothing broke with this error. But\nI think when run via Make that we end up with SHELL_PATH via the\nenvironment (set to the default /bin/sh, which was suitable for my\nsystem).\n\nI'll fix it.\n\n-Peff\n"},{"id":"330771","messageId":"20171021032300.fnbp55ruzak43dzh@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171021001916.bohcqjuxcufxvpam@ruderich.org","subject":"Re: [PATCH 2/3] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-21T03:23:00Z","receivedAt":"2017-10-21T03:23:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 21, 2017 at 02:19:16AM +0200, Simon Ruderich wrote:\n\n> On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:\n> >> I agree. Maybe just stick with the original patch?\n> >\n> > OK. Why don't we live with that for now, then. The only advantage of the\n> > \"999\" trickery is that it's less likely to come up again. If it doesn't,\n> > then we're happy. If it does, then we can always switch then.\n> \n> I think switching the 4 to 9 (which you already brought up in\n> this thread) is a good idea. It makes accidental conflicts less\n> likely (it's rare to use so many file descriptors) and is easy to\n> implement.\n\nI'm not sure it does make accidental conflicts less likely. Grepping for\n'9>' shows a problematic one in t0008, and one in t9300. That's two\nversus the one for \"4\". :)\n\nWe often use descriptors 8 or 9 as \"high and not taken for any specific\nuse\" in our tests, and do things like:\n\n mkfifo foo\n exec 9>foo\n ...\n exec 9>&-\n\nThis is unlike a redirection in a sub-command (like \"foo 9>bar\") because\nit effects the whole test suite's state. So it would break the test\nunder \"-x\" (because we'd get random cruft sent to the fifo), as well as\nbreaking the \"-x\" output itself (because we close the descriptor).\n\nI actually think \"7\" is the safest descriptor right now. It's not used\nfor anything, and it's not high enough for tests to think \"this probably\nisn't used for anything\".\n\n-Peff\n"},{"id":"330827","messageId":"alpine.DEB.2.21.1.1710231255450.6482@virtualbox","threadId":"47001","inReplyTo":"20171019210812.lje3o64v4qdnzcuc@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] test-lib: make \"-x\" work with \"--verbose-log\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-23T10:56:19Z","receivedAt":"2017-10-23T10:56:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 19 Oct 2017, Jeff King wrote:\n\n> [...]\n> override --verbose-log, 2017-09-05). Namely, let's enable\n> --verbose only when we there isn't a more specific verbosity\n> option indicated.\n\ns/we there/we know that there/\n\nThanks,\nDscho\n"},{"id":"330828","messageId":"alpine.DEB.2.21.1.1710231258451.6482@virtualbox","threadId":"47001","inReplyTo":"20171020225340.6qgybldewi7knxvz@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-23T11:01:42Z","receivedAt":"2017-10-23T11:02:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 20 Oct 2017, Jeff King wrote:\n\n> @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n> # and the first level quoting from the shell that runs \"echo\".\n> GIT-BUILD-OPTIONS: FORCE\n> \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n> +\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n\nDo we really want to force the test shell path to be hardcoded at runtime?\nIt may be a better idea not to write this into GIT-BUILD-OPTIONS.\n\nOr alternatively we could prefix the assignment by\n\n\ttest -n \"$TEST_SHELL_PATH\" ||\n\nor use the pattern\n\n\tTEST_SHELL_PATH=\"${TEST_SHELL_PATH:-[...]}\"\n\nCiao,\nDscho\n"},{"id":"330829","messageId":"alpine.DEB.2.21.1.1710231302480.6482@virtualbox","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] making test-suite tracing more useful","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-23T11:04:45Z","receivedAt":"2017-10-23T11:05:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 19 Oct 2017, Jeff King wrote:\n\n> I sometimes run git's test suite as part of an automated testing\n> process. I was hoping to add \"-x\" support to get more details when a\n> test fails (since failures are sometimes hard to reproduce).\n\nThank you for working on this.\n\n> Johannes, I've seen that you do \"-x\" in the tests that the\n> git-for-windows bot uses to comment on GitHub. You may have seen the\n> bogus failure in t5615, which this series should fix (you may also have\n> seen the \"set +x\" cruft at the end of each test, which is fixed here,\n> too).\n\nSurprisingly enough, I did not see any test failure in that test. I do\nre-run failed tests after the complete test run to make sure that the\nfailure is not due to any resource scarcity, and that re-run is performed\nwith -x (to double as documentation what really went wrong).\n\nCiao,\nDscho\n"},{"id":"330882","messageId":"20171024013130.5nuxmjlwnm2l3zrq@sigill.intra.peff.net","threadId":"47001","inReplyTo":"alpine.DEB.2.21.1.1710231258451.6482@virtualbox","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T01:31:31Z","receivedAt":"2017-10-24T01:31:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:\n\n> On Fri, 20 Oct 2017, Jeff King wrote:\n> \n> > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n> > # and the first level quoting from the shell that runs \"echo\".\n> > GIT-BUILD-OPTIONS: FORCE\n> > \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n> > +\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n> \n> Do we really want to force the test shell path to be hardcoded at runtime?\n> It may be a better idea not to write this into GIT-BUILD-OPTIONS.\n\nMy intent was to make it work \"out of the box\" in the same way as\nSHELL_PATH does now. So that:\n\n echo TEST_SHELL_PATH=whatever >>config.mak\n make test\n cd t && ./t1234-*\n\nboth respect it. Without going through BUILD-OPTIONS, I don't think it\nmakes it into the environment via config.mak (it _does_ if you specify\nit on the command-line of \"make\", though).\n\nFor my purposes it would be fine to just use the environment, but I was\ntrying to have it match the other variables for consistency.\n\n> Or alternatively we could prefix the assignment by\n> \n> \ttest -n \"$TEST_SHELL_PATH\" ||\n> \n> or use the pattern\n> \n> \tTEST_SHELL_PATH=\"${TEST_SHELL_PATH:-[...]}\"\n\nI'm not quite sure what this is fixing. Is there a case where we\nwouldn't have TEST_SHELL_PATH set when running the tests? I think there\nare already other bits that assume that \"make\" has been run (including\nthe existing reference to $SHELL_PATH, I think).\n\n-Peff\n"},{"id":"330883","messageId":"20171024013234.ogma2sf37c7hfpib@sigill.intra.peff.net","threadId":"47001","inReplyTo":"alpine.DEB.2.21.1.1710231302480.6482@virtualbox","subject":"Re: [PATCH 0/3] making test-suite tracing more useful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T01:32:35Z","receivedAt":"2017-10-24T01:32:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2017 at 01:04:45PM +0200, Johannes Schindelin wrote:\n\n> > Johannes, I've seen that you do \"-x\" in the tests that the\n> > git-for-windows bot uses to comment on GitHub. You may have seen the\n> > bogus failure in t5615, which this series should fix (you may also have\n> > seen the \"set +x\" cruft at the end of each test, which is fixed here,\n> > too).\n> \n> Surprisingly enough, I did not see any test failure in that test. I do\n> re-run failed tests after the complete test run to make sure that the\n> failure is not due to any resource scarcity, and that re-run is performed\n> with -x (to double as documentation what really went wrong).\n\nAh, OK, that explains it. If t5615 never failed in the non-x run, then you\nwouldn't have run into it. So it was waiting to bite you, but that test\nscript is simple enough that it never failed. :)\n\n-Peff\n"},{"id":"331042","messageId":"alpine.DEB.2.21.1.1710252334000.6482@virtualbox","threadId":"47001","inReplyTo":"20171024013130.5nuxmjlwnm2l3zrq@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-25T21:35:44Z","receivedAt":"2017-10-25T21:36:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 23 Oct 2017, Jeff King wrote:\n\n> On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:\n> \n> > On Fri, 20 Oct 2017, Jeff King wrote:\n> > \n> > > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE\n> > > # and the first level quoting from the shell that runs \"echo\".\n> > > GIT-BUILD-OPTIONS: FORCE\n> > > \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n> > > +\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >$@+\n> > \n> > Do we really want to force the test shell path to be hardcoded at runtime?\n> > It may be a better idea not to write this into GIT-BUILD-OPTIONS.\n> \n> My intent was to make it work \"out of the box\" in the same way as\n> SHELL_PATH does now. So that:\n> \n> echo TEST_SHELL_PATH=whatever >>config.mak\n> make test\n> cd t && ./t1234-*\n> \n> both respect it. Without going through BUILD-OPTIONS, I don't think it\n> makes it into the environment via config.mak (it _does_ if you specify\n> it on the command-line of \"make\", though).\n> \n> For my purposes it would be fine to just use the environment, but I was\n> trying to have it match the other variables for consistency.\n\nMakes sense.\n\n> > Or alternatively we could prefix the assignment by\n> > \n> > \ttest -n \"$TEST_SHELL_PATH\" ||\n> > \n> > or use the pattern\n> > \n> > \tTEST_SHELL_PATH=\"${TEST_SHELL_PATH:-[...]}\"\n> \n> I'm not quite sure what this is fixing. Is there a case where we\n> wouldn't have TEST_SHELL_PATH set when running the tests? I think there\n> are already other bits that assume that \"make\" has been run (including\n> the existing reference to $SHELL_PATH, I think).\n\nThe way I read your patch, setting the environment variable differnently\nat test time than at build time would be ignored: GIT-BUILD-OPTIONS is\nsourced and would override whatever you told the test suite to use.\n\nI guess it does not really matter all that much in practice.\n\nCiao,\nDscho\n"},{"id":"331043","messageId":"20171025215015.irnyvk7kvmxqkniy@sigill.intra.peff.net","threadId":"47001","inReplyTo":"alpine.DEB.2.21.1.1710252334000.6482@virtualbox","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-25T21:50:15Z","receivedAt":"2017-10-25T21:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:\n\n> > > Or alternatively we could prefix the assignment by\n> > > \n> > > \ttest -n \"$TEST_SHELL_PATH\" ||\n> > > \n> > > or use the pattern\n> > > \n> > > \tTEST_SHELL_PATH=\"${TEST_SHELL_PATH:-[...]}\"\n> > \n> > I'm not quite sure what this is fixing. Is there a case where we\n> > wouldn't have TEST_SHELL_PATH set when running the tests? I think there\n> > are already other bits that assume that \"make\" has been run (including\n> > the existing reference to $SHELL_PATH, I think).\n> \n> The way I read your patch, setting the environment variable differnently\n> at test time than at build time would be ignored: GIT-BUILD-OPTIONS is\n> sourced and would override whatever you told the test suite to use.\n> \n> I guess it does not really matter all that much in practice.\n\nRight. I find that behavior mildly irritating at times, but it's\nconsistent with other items like NO_PERL, etc. E.g., you cannot do:\n\n make NO_PERL=\n cd t\n NO_PERL=Nope ./t3701-*\n\nand disable perl. It's testing what got built.\n\n-Peff\n"},{"id":"331154","messageId":"alpine.DEB.2.21.1.1710271625190.6482@virtualbox","threadId":"47001","inReplyTo":"20171025215015.irnyvk7kvmxqkniy@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T14:26:02Z","receivedAt":"2017-10-27T14:56:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 25 Oct 2017, Jeff King wrote:\n\n> On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:\n> \n> > > > Or alternatively we could prefix the assignment by\n> > > > \n> > > > \ttest -n \"$TEST_SHELL_PATH\" ||\n> > > > \n> > > > or use the pattern\n> > > > \n> > > > \tTEST_SHELL_PATH=\"${TEST_SHELL_PATH:-[...]}\"\n> > > \n> > > I'm not quite sure what this is fixing. Is there a case where we\n> > > wouldn't have TEST_SHELL_PATH set when running the tests? I think there\n> > > are already other bits that assume that \"make\" has been run (including\n> > > the existing reference to $SHELL_PATH, I think).\n> > \n> > The way I read your patch, setting the environment variable differnently\n> > at test time than at build time would be ignored: GIT-BUILD-OPTIONS is\n> > sourced and would override whatever you told the test suite to use.\n> > \n> > I guess it does not really matter all that much in practice.\n> \n> Right. I find that behavior mildly irritating at times, but it's\n> consistent with other items like NO_PERL, etc. E.g., you cannot do:\n> \n> make NO_PERL=\n> cd t\n> NO_PERL=Nope ./t3701-*\n> \n> and disable perl. It's testing what got built.\n\nThe difference between NO_PERL and TEST_SHELL_PATH, of course, is that\nNO_PERL affects the build and the test phase, while TEST_SHELL_PATH really\nhas no impact whatsoever on the build phase.\n\nCiao,\nDscho\n"},{"id":"334408","messageId":"20171208104647.GA4016@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net","subject":"[PATCH v2 0/4] making test-suite tracing more useful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:46:47Z","receivedAt":"2017-12-08T10:47:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes some rough edges in the \"-x\" feature of the test\nsuite. With it, it should be possible to turn on tracing for CI runs.\n\nThis is mostly a repost of v1 at:\n\n https://public-inbox.org/git/20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net\n\nwhich had some discussion, but wasn't picked up.\n\nI fixed a few typos pointed out by reviewers, and I tried to summarize\nthe discussion around \"magic descriptor 4\" in the commit message of\npatch 2. My conclusion there was that none of the options is\nparticularly good. The only thing thing that might make sense is making\n\"7\" the magical descriptor. But given that \"4\" only had one troubling\ncall, I'm inclined to just leave it as-is and see if this ever comes up\nagain (especially if we start using \"-x\" in the Travis builds, then we'd\ncatch any problems).\n\n [1/4]: test-lib: silence \"-x\" cleanup under bash\n [2/4]: t5615: avoid re-using descriptor 4\n [3/4]: test-lib: make \"-x\" work with \"--verbose-log\"\n [4/4]: t/Makefile: introduce TEST_SHELL_PATH\n\n Makefile | 8 ++++++++\n t/Makefile | 6 ++++--\n t/t5615-alternate-env.sh | 6 +++---\n t/test-lib.sh | 46 +++++++++++++++++++++++++++++++++-------------\n 4 files changed, 48 insertions(+), 18 deletions(-)\n\n-Peff\n"},{"id":"334409","messageId":"20171208104708.GA4939@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171208104647.GA4016@sigill.intra.peff.net","subject":"[PATCH v2 1/4] test-lib: silence \"-x\" cleanup under bash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:47:08Z","receivedAt":"2017-12-08T10:47:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the test suite's \"-x\" option is used with bash, we end\nup seeing cleanup cruft in the output:\n\n $ bash t0001-init.sh -x\n [...]\n ++ diff -u expected actual\n + test_eval_ret_=0\n + want_trace\n + test t = t\n + test t = t\n + set +x\n ok 42 - re-init from a linked worktree\n\nThis ranges from mildly annoying (for a successful test) to\ndownright confusing (when we say \"last command exited with\nerror\", but it's really 5 commands back).\n\nWe normally are able to suppress this cleanup. As the\nin-code comment explains, we can't convince the shell not to\nprint it, but we can redirect its stderr elsewhere.\n\nBut since d88785e424 (test-lib: set BASH_XTRACEFD\nautomatically, 2016-05-11), that doesn't hold for bash. It\nsends the \"set -x\" output directly to descriptor 4, not to\nstderr.\n\nWe can fix this by also redirecting descriptor 4, and\npaying close attention to which commands redirected and\nwhich are not (see the updated comment).\n\nTwo alternatives I considered and rejected:\n\n - unsetting and setting BASH_XTRACEFD; doing so closes the\n descriptor, which we must avoid\n\n - we could keep everything in a single block as before,\n redirect 4>/dev/null there, but retain 5>&4 as a copy.\n And then selectively restore 4>&5 for commands which\n should be allowed to trace. This would work, but the\n descriptor swapping seems unnecessarily confusing.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 34 ++++++++++++++++++++++++----------\n 1 file changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 116bd6a70c..7914453a3b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -601,26 +601,40 @@ test_eval_inner_ () {\n }\n \n test_eval_ () {\n-\t# We run this block with stderr redirected to avoid extra cruft\n-\t# during a \"-x\" trace. Once in \"set -x\" mode, we cannot prevent\n+\t# If \"-x\" tracing is in effect, then we want to avoid polluting stderr\n+\t# with non-test commands. But once in \"set -x\" mode, we cannot prevent\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n \t#\n-\t# The test itself is run with stderr put back to &4 (so either to\n-\t# /dev/null, or to the original stderr if --verbose was used).\n+\t# There are a few subtleties here:\n+\t#\n+\t# - we have to redirect descriptor 4 in addition to 2, to cover\n+\t# BASH_XTRACEFD\n+\t#\n+\t# - the actual eval has to come before the redirection block (since\n+\t# it needs to see descriptor 4 to set up its stderr)\n+\t#\n+\t# - likewise, any error message we print must be outside the block to\n+\t# access descriptor 4\n+\t#\n+\t# - checking $? has to come immediately after the eval, but it must\n+\t# be _inside_ the block to avoid polluting the \"set -x\" output\n+\t#\n+\n+\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n-\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\tset +x\n-\t\t\tif test \"$test_eval_ret_\" != 0\n-\t\t\tthen\n-\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\t\t\tfi\n \t\tfi\n-\t} 2>/dev/null\n+\t} 2>/dev/null 4>&2\n+\n+\tif test \"$test_eval_ret_\" != 0 && want_trace\n+\tthen\n+\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\tfi\n \treturn $test_eval_ret_\n }\n \n-- \n2.15.1.659.g8bd2eae3ea\n\n"},{"id":"334410","messageId":"20171208104713.GB4939@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171208104647.GA4016@sigill.intra.peff.net","subject":"[PATCH v2 2/4] t5615: avoid re-using descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:47:13Z","receivedAt":"2017-12-08T10:47:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"File descriptors 3 and 4 are special in our test suite, as\nthey link back to the test script's original stdout and\nstderr. Normally this isn't something tests need to worry\nabout: they are free to clobber these descriptors for\nsub-commands without affecting the overall script.\n\nBut there's one very special thing about descriptor 4: since\nd88785e424 (test-lib: set BASH_XTRACEFD automatically,\n2016-05-11), we ask bash to output \"set -x\" output to it by\nnumber. This goes to _any_ descriptor 4, even if it no\nlonger points to the place it did when we set BASH_XTRACEFD.\n\nBut in t5615, we run a shell loop with descriptor 4\nredirected. As a result, t5615 works with non-bash shells\neven with \"-x\". And it works with bash without \"-x\". But the\ncombination of \"bash t5615-alternate-env.sh -x\" gets a test\nfailure (because our \"set -x\" output pollutes one of the\nfiles).\n\nWe can fix this by using any descriptor _except_ the magical\n4. So let's switch arbitrarily to using 5/6 in this loop,\nnot 3/4.\n\nAnother alternative is to use a different descriptor for\nBASH_XTRACEFD. But picking an unused one turns out to be\nhard. Most shells limit us to 9 numbered descriptors. Bash\ncan handle more, but:\n\n - while the BASH_XTRACEFD is specific to bash, GIT_TRACE=4\n has a similar problem, and would affect all shells\n\n - constructs like \"999>/dev/null\" are synticatically\n invalid to non-bash shells. So we have to actually bury\n it inside an eval, which creates more complications.\n\nOf the numbers 1-9, you might think that \"9\" would be less\nused than \"4\". But it's not; many of our scripts use\ndescriptors 8 and 9 (probably under the assumption that they\nare high and therefore unused). The least-used descriptor is\ncurrently \"7\". We could switch to that, but we're just\ntrading one magic number for another.\n\nSigned-off-by: Jeff King \n---\n t/t5615-alternate-env.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh\nindex d2d883f3a1..b4905b822c 100755\n--- a/t/t5615-alternate-env.sh\n+++ b/t/t5615-alternate-env.sh\n@@ -7,9 +7,9 @@ check_obj () {\n \talt=$1; shift\n \twhile read obj expect\n \tdo\n-\t\techo \"$obj\" >&3 &&\n-\t\techo \"$obj $expect\" >&4\n-\tdone 3>input 4>expect &&\n+\t\techo \"$obj\" >&5 &&\n+\t\techo \"$obj $expect\" >&6\n+\tdone 5>input 6>expect &&\n \tGIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \\\n \t\tgit \"$@\" cat-file --batch-check='%(objectname) %(objecttype)' \\\n \t\tactual &&\n-- \n2.15.1.659.g8bd2eae3ea\n\n"},{"id":"334411","messageId":"20171208104717.GC4939@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171208104647.GA4016@sigill.intra.peff.net","subject":"[PATCH v2 3/4] test-lib: make \"-x\" work with \"--verbose-log\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:47:17Z","receivedAt":"2017-12-08T10:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-x\" tracing option implies \"--verbose\". This is a\nproblem when running under a TAP harness like \"prove\", where\nwe need to use \"--verbose-log\" instead. Instead, let's\nhandle this the same way we do for --valgrind, including the\nrecent fix from 88c6e9d31c (test-lib: --valgrind should not\noverride --verbose-log, 2017-09-05). Namely, let's enable\n--verbose only when we know there isn't a more specific\nverbosity option indicated.\n\nNote that we also have to tweak `want_trace` to turn it on\n(previously we just lumped $verbose_log in with $verbose,\nbut now we don't necessarily auto-set the latter).\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7914453a3b..b8dd5e79ac 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -264,7 +264,6 @@ do\n \t\tshift ;;\n \t-x)\n \t\ttrace=t\n-\t\tverbose=t\n \t\tshift ;;\n \t--verbose-log)\n \t\tverbose_log=t\n@@ -283,6 +282,11 @@ then\n \ttest -z \"$verbose_log\" && verbose=t\n fi\n \n+if test -n \"$trace\" && test -z \"$verbose_log\"\n+then\n+\tverbose=t\n+fi\n+\n if test -n \"$color\"\n then\n \t# Save the color control sequences now rather than run tput\n@@ -586,7 +590,9 @@ maybe_setup_valgrind () {\n }\n \n want_trace () {\n-\ttest \"$trace\" = t && test \"$verbose\" = t\n+\ttest \"$trace\" = t && {\n+\t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n+\t}\n }\n \n # This is a separate function because some tests use\n-- \n2.15.1.659.g8bd2eae3ea\n\n"},{"id":"334412","messageId":"20171208104722.GD4939@sigill.intra.peff.net","threadId":"47001","inReplyTo":"20171208104647.GA4016@sigill.intra.peff.net","subject":"[PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:47:22Z","receivedAt":"2017-12-08T10:47:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You may want to run the test suite with a different shell\nthan you use to build Git. For instance, you may build with\nSHELL_PATH=/bin/sh (because it's faster, or it's what you\nexpect to exist on systems where the build will be used) but\nwant to run the test suite with bash (e.g., since that\nallows using \"-x\" reliably across the whole test suite).\nThere's currently no good way to do this.\n\nYou might think that doing two separate make invocations,\nlike:\n\n make &&\n make -C t SHELL_PATH=/bin/bash\n\nwould work. And it _almost_ does. The second make will see\nour bash SHELL_PATH, and we'll use that to run the\nindividual test scripts (or tell prove to use it to do so).\nSo far so good.\n\nBut this breaks down when \"--tee\" or \"--verbose-log\" is\nused. Those options cause the test script to actually\nre-exec itself using $SHELL_PATH. But wait, wouldn't our\nsecond make invocation have set SHELL_PATH correctly in the\nenvironment?\n\nYes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we\nbuilt during the first \"make\". And that overrides the\nenvironment, giving us the original SHELL_PATH again.\n\nLet's introduce a new variable that lets you specify a\nspecific shell to be run for the test scripts. Note that we\nhave to touch both the main and t/ Makefiles, since we have\nto record it in GIT-BUILD-OPTIONS in one, and use it in the\nlatter.\n\nSigned-off-by: Jeff King \n---\n Makefile | 8 ++++++++\n t/Makefile | 6 ++++--\n t/test-lib.sh | 2 +-\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex fef9c8d272..8a21c4d8f1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -425,6 +425,10 @@ all::\n #\n # to say \"export LESS=FRX (and LV=-c) if the environment variable\n # LESS (and LV) is not set, respectively\".\n+#\n+# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for\n+# running the test scripts (e.g., bash has better support for \"set -x\"\n+# tracing).\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -729,6 +733,8 @@ endif\n export PERL_PATH\n export PYTHON_PATH\n \n+TEST_SHELL_PATH = $(SHELL_PATH)\n+\n LIB_FILE = libgit.a\n XDIFF_LIB = xdiff/lib.a\n VCSSVN_LIB = vcs-svn/lib.a\n@@ -1725,6 +1731,7 @@ prefix_SQ = $(subst ','\\'',$(prefix))\n gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))\n \n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n PYTHON_PATH_SQ = $(subst ','\\'',$(PYTHON_PATH))\n TCLTK_PATH_SQ = $(subst ','\\'',$(TCLTK_PATH))\n@@ -2355,6 +2362,7 @@ GIT-LDFLAGS: FORCE\n # and the first level quoting from the shell that runs \"echo\".\n GIT-BUILD-OPTIONS: FORCE\n \t@echo SHELL_PATH=\\''$(subst ','\\'',$(SHELL_PATH_SQ))'\\' >$@+\n+\t@echo TEST_SHELL_PATH=\\''$(subst ','\\'',$(TEST_SHELL_PATH_SQ))'\\' >>$@+\n \t@echo PERL_PATH=\\''$(subst ','\\'',$(PERL_PATH_SQ))'\\' >>$@+\n \t@echo DIFF=\\''$(subst ','\\'',$(subst ','\\'',$(DIFF)))'\\' >>$@+\n \t@echo PYTHON_PATH=\\''$(subst ','\\'',$(PYTHON_PATH_SQ))'\\' >>$@+\ndiff --git a/t/Makefile b/t/Makefile\nindex 1bb06c36f2..96317a35f4 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -8,6 +8,7 @@\n \n #GIT_TEST_OPTS = --verbose --debug\n SHELL_PATH ?= $(SHELL)\n+TEST_SHELL_PATH ?= $(SHELL_PATH)\n PERL_PATH ?= /usr/bin/perl\n TAR ?= $(TAR)\n RM ?= rm -f\n@@ -23,6 +24,7 @@ endif\n \n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n TEST_RESULTS_DIRECTORY_SQ = $(subst ','\\'',$(TEST_RESULTS_DIRECTORY))\n \n@@ -42,11 +44,11 @@ failed:\n \ttest -z \"$$failed\" || $(MAKE) $$failed\n \n prove: pre-clean $(TEST_LINT)\n-\t@echo \"*** prove ***\"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n+\t@echo \"*** prove ***\"; $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n \t$(MAKE) clean-except-prove-cache\n \n $(T):\n-\t@echo \"*** $@ ***\"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n+\t@echo \"*** $@ ***\"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n \n pre-clean:\n \t$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b8dd5e79ac..1ae0fc02d0 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -80,7 +80,7 @@ done,*)\n \t# from any previous runs.\n \t>\"$GIT_TEST_TEE_OUTPUT_FILE\"\n \n-\t(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} \"$0\" \"$@\" 2>&1;\n+\t(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} \"$0\" \"$@\" 2>&1;\n \t echo $? >\"$BASE.exit\") | tee -a \"$GIT_TEST_TEE_OUTPUT_FILE\"\n \ttest \"$(cat \"$BASE.exit\")\" = 0\n \texit\n-- \n2.15.1.659.g8bd2eae3ea\n"},{"id":"334418","messageId":"alpine.DEB.2.21.1.1712081602570.4318@virtualbox","threadId":"47001","inReplyTo":"20171208104722.GD4939@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-08T15:08:19Z","receivedAt":"2017-12-08T15:08:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nthe other three patches look good to me.\n\nOn Fri, 8 Dec 2017, Jeff King wrote:\n\n> You may want to run the test suite with a different shell\n> than you use to build Git. For instance, you may build with\n> SHELL_PATH=/bin/sh (because it's faster, or it's what you\n> expect to exist on systems where the build will be used) but\n> want to run the test suite with bash (e.g., since that\n> allows using \"-x\" reliably across the whole test suite).\n> There's currently no good way to do this.\n> \n> You might think that doing two separate make invocations,\n> like:\n> \n> make &&\n> make -C t SHELL_PATH=/bin/bash\n> \n> would work. And it _almost_ does. The second make will see\n> our bash SHELL_PATH, and we'll use that to run the\n> individual test scripts (or tell prove to use it to do so).\n> So far so good.\n> \n> But this breaks down when \"--tee\" or \"--verbose-log\" is\n> used. Those options cause the test script to actually\n> re-exec itself using $SHELL_PATH. But wait, wouldn't our\n> second make invocation have set SHELL_PATH correctly in the\n> environment?\n> \n> Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we\n> built during the first \"make\". And that overrides the\n> environment, giving us the original SHELL_PATH again.\n\n... and we could simply see whether the environment variable\nTEST_SHELL_PATH (which we would set in t/Makefile from the passed-in\nSHELL_PATH) is set, and override it again.\n\nI still think we can do without recording test-phase details in the\nbuild-phase (which may, or may not, know what the test-phase wants to do).\n\nIn other words, I believe that we can make the invocation you mentioned\nabove work, by touching only t/Makefile (to pass SHELL_PATH as\nTEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from\nGIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).\n\nCiao,\nDscho\n"},{"id":"334508","messageId":"20171208220046.GA26270@sigill.intra.peff.net","threadId":"47001","inReplyTo":"alpine.DEB.2.21.1.1712081602570.4318@virtualbox","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T22:00:46Z","receivedAt":"2017-12-08T22:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:\n\n> > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we\n> > built during the first \"make\". And that overrides the\n> > environment, giving us the original SHELL_PATH again.\n> \n> ... and we could simply see whether the environment variable\n> TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in\n> SHELL_PATH) is set, and override it again.\n> \n> I still think we can do without recording test-phase details in the\n> build-phase (which may, or may not, know what the test-phase wants to do).\n> \n> In other words, I believe that we can make the invocation you mentioned\n> above work, by touching only t/Makefile (to pass SHELL_PATH as\n> TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from\n> GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).\n\nWe could do that, but it makes TEST_SHELL_PATH inconsistent with all of\nthe other config.mak variables.\n\n-Peff\n"},{"id":"334540","messageId":"alpine.DEB.2.21.1.1712091443560.4318@virtualbox","threadId":"47001","inReplyTo":"20171208220046.GA26270@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-09T13:44:44Z","receivedAt":"2017-12-09T13:45:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Dec 2017, Jeff King wrote:\n\n> On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:\n> \n> > > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we\n> > > built during the first \"make\". And that overrides the\n> > > environment, giving us the original SHELL_PATH again.\n> > \n> > ... and we could simply see whether the environment variable\n> > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in\n> > SHELL_PATH) is set, and override it again.\n> > \n> > I still think we can do without recording test-phase details in the\n> > build-phase (which may, or may not, know what the test-phase wants to do).\n> > \n> > In other words, I believe that we can make the invocation you mentioned\n> > above work, by touching only t/Makefile (to pass SHELL_PATH as\n> > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from\n> > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).\n> \n> We could do that, but it makes TEST_SHELL_PATH inconsistent with all of\n> the other config.mak variables.\n\nIt is already inconsistent with the other variables because its scope is\nthe \"test\" phase, not the \"build\" phase.\n\nCiao,\nDscho\n"},{"id":"334565","messageId":"20171210142309.GA19453@sigill.intra.peff.net","threadId":"47001","inReplyTo":"alpine.DEB.2.21.1.1712091443560.4318@virtualbox","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-10T14:23:09Z","receivedAt":"2017-12-10T14:23:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 09, 2017 at 02:44:44PM +0100, Johannes Schindelin wrote:\n\n> > > ... and we could simply see whether the environment variable\n> > > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in\n> > > SHELL_PATH) is set, and override it again.\n> > > \n> > > I still think we can do without recording test-phase details in the\n> > > build-phase (which may, or may not, know what the test-phase wants to do).\n> > > \n> > > In other words, I believe that we can make the invocation you mentioned\n> > > above work, by touching only t/Makefile (to pass SHELL_PATH as\n> > > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from\n> > > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).\n> > \n> > We could do that, but it makes TEST_SHELL_PATH inconsistent with all of\n> > the other config.mak variables.\n> \n> It is already inconsistent with the other variables because its scope is\n> the \"test\" phase, not the \"build\" phase.\n\nI'm not sure that's true. Look at what already goes into\nGIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.\n\nInterestingly, many of those do something like this in the Makefile:\n\n ifdef GIT_TEST_CMP\n\t@echo GIT_TEST_OPTS=... >>$@+\n endif\n\nwhich seems utterly confusing to me. Because it means that if you build\nwith those options set, then they will override anything in the\nenvironment. But if you don't, then you _may_ override them in the\nenvironment. In other words:\n\n make\n cd t\n GIT_TEST_CMP=foo ./t0000-*\n\nwill respect that variable. But:\n\n make GIT_TEST_CMP=foo\n cd t\n GIT_TEST_CMP=bar ./t0000-*\n\nwill not. Which seems weird. But I guess we could follow that pattern\nwith TEST_SHELL_PATH.\n\n-Peff\n"},{"id":"334628","messageId":"xmqq4lox57c9.fsf@gitster.mtv.corp.google.com","threadId":"47001","inReplyTo":"20171210142309.GA19453@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-11T20:37:42Z","receivedAt":"2017-12-11T20:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure that's true. Look at what already goes into\n> GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.\n>\n> Interestingly, many of those do something like this in the Makefile:\n>\n> ifdef GIT_TEST_CMP\n> \t@echo GIT_TEST_OPTS=... >>$@+\n> endif\n>\n> which seems utterly confusing to me. Because it means that if you build\n> with those options set, then they will override anything in the\n> environment. But if you don't, then you _may_ override them in the\n> environment. In other words:\n>\n> make\n> cd t\n> GIT_TEST_CMP=foo ./t0000-*\n>\n> will respect that variable. But:\n>\n> make GIT_TEST_CMP=foo\n> cd t\n> GIT_TEST_CMP=bar ./t0000-*\n>\n> will not. Which seems weird. But I guess we could follow that pattern\n> with TEST_SHELL_PATH.\n\nOr perhaps we can start setting a better example with the new\nvariable, and migrate those weird existing ones over to the new way\nof not forbidding run-time overriding?\n\nThere is a long outstanding NEEDSWORK comment in help.c that wonders\nif we want to embed contents from GIT-BUILD-OPTIONS in the resulting\nbinary, and the distinction Dscho brought up between \"build\" and\n\"test\" phases would start to matter even more once we go in that\ndirection.\n\n\n"},{"id":"334874","messageId":"20171215104101.GA11637@sigill.intra.peff.net","threadId":"47001","inReplyTo":"xmqq4lox57c9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-15T10:41:01Z","receivedAt":"2017-12-15T10:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 11, 2017 at 12:37:42PM -0800, Junio C Hamano wrote:\n\n> > Interestingly, many of those do something like this in the Makefile:\n> >\n> > ifdef GIT_TEST_CMP\n> > \t@echo GIT_TEST_OPTS=... >>$@+\n> > endif\n> >\n> > which seems utterly confusing to me. Because it means that if you build\n> > with those options set, then they will override anything in the\n> > environment. But if you don't, then you _may_ override them in the\n> > environment. In other words:\n> >\n> > make\n> > cd t\n> > GIT_TEST_CMP=foo ./t0000-*\n> >\n> > will respect that variable. But:\n> >\n> > make GIT_TEST_CMP=foo\n> > cd t\n> > GIT_TEST_CMP=bar ./t0000-*\n> >\n> > will not. Which seems weird. But I guess we could follow that pattern\n> > with TEST_SHELL_PATH.\n> \n> Or perhaps we can start setting a better example with the new\n> variable, and migrate those weird existing ones over to the new way\n> of not forbidding run-time overriding?\n\nThis turns out to be quite tricky, because GIT-BUILD-OPTIONS must be\nparsed as both a Makefile and shell-script.\n\nSo we can do:\n\n 1. Omit them from GIT-BUILD-OPTIONS, which means they don't work for\n cases where the tests aren't started from the Makefile (which would\n have put them in the environment). I think this is a non-starter.\n\n 2. Add a Makefile-level ifdef when generating GIT-BUILD-OPTIONS, so\n that unused options can be overridden by the environment That's the\n \"weird\" thing above that we do now for some variables.\n\n 3. Add something like:\n\n test -z \"$FOO\" && FOO=...\n\n when building GIT-BUILD-OPTIONS (instead of just FOO=...). But that\n doesn't work because it must parse as Makefile.\n\n 4. In test-lib.sh, save and restore each such variable so that the\n existing environment takes precedence. Like:\n\n FOO_save=$FOO\n BAR_save=$BAR\n ...etc for each...\n\n . GIT-BUILD-OPTIONS\n\n test -n \"$FOO_save\" && FOO=$FOO_save\n test -n \"$BAR_save\" && BAR=$BAR_save\n ...etc...\n\n We have to do the save/restore dance rather than just reordering\n the assignments, because the existing environment is being passed\n into us (so we can't just \"assign\" it to overwrite what's in the\n build options file).\n\n This could be made slightly less tedious with a loop and an eval.\n It could possibly be made very succinct but very magical with\n something like:\n\n saved=$(export)\n\t. GIT-BUILD-OPTIONS\n\teval \"$saved\"\n\n 5. Give up on the dual nature of GIT-BUILD-OPTIONS, and generate two\n such files (with identical values but different syntax).\n\nI think (4) and (5) are the only things that actually change the\nbehavior in a meaningful way. But they're a bit more hacky and\nrepetitive than I'd like. Especially given that I'm not really sure\nwe're solving an interesting problem. I'm happy enough with the patch as\nshown, and I do not recall anybody complaining about the current\nbehavior of these options.\n\n> There is a long outstanding NEEDSWORK comment in help.c that wonders\n> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting\n> binary, and the distinction Dscho brought up between \"build\" and\n> \"test\" phases would start to matter even more once we go in that\n> direction.\n\nI guess you're implying having a GIT-BUILD-OPTIONS and a\nGIT-TEST-OPTIONS here. But that doesn't save us from the Makefile/shell\nduality, unfortunately. Some of the test options need to be read by\nt/Makefile, and some need to be read by test-lib.sh (and I suspect there\nare some needed in both places).\n\n-Peff\n"},{"id":"334893","messageId":"xmqqmv2kq66p.fsf@gitster.mtv.corp.google.com","threadId":"47001","inReplyTo":"20171215104101.GA11637@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-15T16:58:22Z","receivedAt":"2017-12-15T16:58:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think (4) and (5) are the only things that actually change the\n> behavior in a meaningful way. But they're a bit more hacky and\n> repetitive than I'd like. Especially given that I'm not really sure\n> we're solving an interesting problem. I'm happy enough with the patch as\n> shown, and I do not recall anybody complaining about the current\n> behavior of these options.\n\nOK. Thanks for thinking it through.\n\n>> There is a long outstanding NEEDSWORK comment in help.c that wonders\n>> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting\n>> binary, and the distinction Dscho brought up between \"build\" and\n>> \"test\" phases would start to matter even more once we go in that\n>> direction.\n>\n> I guess you're implying having a GIT-BUILD-OPTIONS and a\n> GIT-TEST-OPTIONS here.\n\nI admit that my thinking did not go that far to introduce the\nlatter, as \"git version --how-did-we-build-this-exact-git\" only\nneeds the former. But you're right that some information given\nat the top-level must be stored somewhere t/test-lib.sh reads in\norder to allow us run tests from outside Makefile (your point 1.)\n"},{"id":"335120","messageId":"20171221094749.GA23160@sigill.intra.peff.net","threadId":"47001","inReplyTo":"xmqqmv2kq66p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-21T09:47:49Z","receivedAt":"2017-12-21T09:47:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 15, 2017 at 08:58:22AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think (4) and (5) are the only things that actually change the\n> > behavior in a meaningful way. But they're a bit more hacky and\n> > repetitive than I'd like. Especially given that I'm not really sure\n> > we're solving an interesting problem. I'm happy enough with the patch as\n> > shown, and I do not recall anybody complaining about the current\n> > behavior of these options.\n> \n> OK. Thanks for thinking it through.\n\nI took one final look at this, wondering if it ought to follow the\n\"write to BUILD-OPTIONS only if set\" pattern that some other variables\ndo. But I think that just ends up more confusing, because of the way we\nuse the variable from both the Makefile and test-lib.sh. So it makes\nthis work:\n\n make\n make -C t TEST_SHELL_PATH=whatever\n\nbut not quite this:\n\n make TEST_SHELL_PATH=one\n make -C t TEST_SHELL_PATH=two\n\nbecause in the second case, we use \"two\" to invoke the test script, but\na \"--tee\" re-exec would use \"one\". Which is pretty subtle.\n\nI really wish there was a way for a shell script to find out which\ninterpreter it was currently using, but I couldn't come up with a\nportable way to do so (on some systems, /proc/$$/exe works, but that's\nobviously not something we should count on).\n\nSo anyway. I think I'm OK with the series as-is.\n\n-Peff\n"}]}