{"thread":{"id":"54836","subject":"[PATCH] t/perf: fix test_export() failure with BSD `sed`","startedAt":"2020-12-16T07:40:42Z","lastAt":"2020-12-21T07:26:42Z","messageCount":8,"participants":["Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"412340","messageId":"20201216073907.62591-1-sunshine@sunshineco.com","threadId":"54836","inReplyTo":null,"subject":"[PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-16T07:39:07Z","receivedAt":"2020-12-16T07:40:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"test_perf() runs each test in its own subshell which makes it difficult\nto persist variables between tests. test_export() addresses this\nshortcoming by grabbing the values of specified variables after a test\nruns but before the subshell exits, and writes those values to a file\nwhich is loaded into the environment of subsequent tests.\n\nTo grab the values to be persisted, test_export() pipes the output of\nthe shell's builtin `set` command through `sed` which plucks them out\nusing a regular expression along the lines of `s/^(var1|var2)/.../p`.\nUnfortunately, though, this use of alternation is not portable. For\ninstance, BSD-lineage `sed` (including macOS `sed`) does not support it\nin the default \"basic regular expression\" mode (BRE). It may be possible\nto enable \"extended regular expression\" mode (ERE) in some cases with\n`sed -E`, however, `-E` is neither portable nor part of POSIX.\n\nFortunately, alternation is unnecessary in this case and can easily be\navoided, so replace it with a series of simple expressions such as\n`s/^var1/.../p;s/^var2/.../p`.\n\nWhile at it, tighten the expressions so they match the variable names\nexactly rather than matching prefixes (i.e. use `s/^var1=/.../p`).\n\nIf the requirements of test_export() become more complex in the future,\nthen an alternative would be to replace `sed` with `perl` which supports\nalternation on all platforms, however, the simple elimination of\nalternation via multiple `sed` expressions suffices for the present.\n\nReported-by: Sangeeta \nDiagnosed-by: Philippe Blain \nSigned-off-by: Eric Sunshine \n---\n t/perf/perf-lib.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 821581a885..22d727cef8 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -148,13 +148,18 @@ test_run_perf_ () {\n . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n test_export () {\n \t[ $# != 0 ] || return 0\n-\ttest_export_=\"$test_export_\\\\|$1\"\n+\ttest_export_=\"$test_export_ $1\"\n \tshift\n \ttest_export \"$@\"\n }\n '\"$1\"'\n ret=$?\n-set | sed -n \"s'\"/'/'\\\\\\\\''/g\"';s/^\\\\($test_export_\\\\)/export '\"'&'\"'/p\" >test_vars\n+needles=\n+for v in $test_export_\n+do\n+\tneedles=\"$needles;s/^$v=/export $v=/p\"\n+done\n+set | sed -n \"s'\"/'/'\\\\\\\\''/g\"'$needles\" >test_vars\n exit $ret' >&3 2>&4\n \teval_ret=$?\n \n-- \n2.30.0.rc0.297.gbcca948854\n\n"},{"id":"412421","messageId":"xmqq5z514lj5.fsf@gitster.c.googlers.com","threadId":"54836","inReplyTo":"20201216073907.62591-1-sunshine@sunshineco.com","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-16T19:07:42Z","receivedAt":"2020-12-16T19:08:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Fortunately, alternation is unnecessary in this case and can easily be\n> avoided, so replace it with a series of simple expressions such as\n> `s/^var1/.../p;s/^var2/.../p`.\n\nSimple is good.\n\n> While at it, tighten the expressions so they match the variable names\n> exactly rather than matching prefixes (i.e. use `s/^var1=/.../p`).\n\nGood eyes. That is quite good.\n\n> @@ -148,13 +148,18 @@ test_run_perf_ () {\n> . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n> test_export () {\n> \t[ $# != 0 ] || return 0\n> -\ttest_export_=\"$test_export_\\\\|$1\"\n> +\ttest_export_=\"$test_export_ $1\"\n> \tshift\n> \ttest_export \"$@\"\n> }\n\nThis \"recursion to consume $@ one by one, instead of looping\" caught\nmy eyes a bit, but the bug being fixed is not caused by it, so it is\nfine to let it pass. Given that the arguments to test_export are\nsupposed to be all variable names (i.e. no funny characters anywhere\nin it), I think it could just be\n\n\ttest_export () {\n\t\ttest_export_=\"$*\"\n\t}\n\nthough.\n\nOh, does anybody need to clear test_export_ to an empty string (or\nunset it), by the way?\n\n> '\"$1\"'\n> ret=$?\n> -set | sed -n \"s'\"/'/'\\\\\\\\''/g\"';s/^\\\\($test_export_\\\\)/export '\"'&'\"'/p\" >test_vars\n> +needles=\n> +for v in $test_export_\n> +do\n> +\tneedles=\"$needles;s/^$v=/export $v=/p\"\n> +done\n> +set | sed -n \"s'\"/'/'\\\\\\\\''/g\"'$needles\" >test_vars\n> exit $ret' >&3 2>&4\n> \teval_ret=$?\n\nOther than these, none of which were \"this is wrong and needs to be\nfixed\", I have no comments. The patch is quite readably done.\n\nWill queue. Thanks.\n"},{"id":"412425","messageId":"CAPig+cR+4Wh4Sgk6UhUML4SHqaQsvYmw_77ih+oec2YmqQJCCg@mail.gmail.com","threadId":"54836","inReplyTo":"xmqq5z514lj5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-16T19:29:26Z","receivedAt":"2020-12-16T19:30:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2020 at 2:07 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > test_export () {\n> > [ $# != 0 ] || return 0\n> > - test_export_=\"$test_export_\\\\|$1\"\n> > + test_export_=\"$test_export_ $1\"\n> > shift\n> > test_export \"$@\"\n> > }\n>\n> This \"recursion to consume $@ one by one, instead of looping\" caught\n> my eyes a bit, but the bug being fixed is not caused by it, so it is\n> fine to let it pass.\n\nThe unusual recursion caught my eye as well.\n\n> Given that the arguments to test_export are\n> supposed to be all variable names (i.e. no funny characters anywhere\n> in it), I think it could just be\n>\n> test_export () {\n> test_export_=\"$*\"\n> }\n>\n> though.\n\nThat's almost good enough, but test_export() needs to accumulate the\nto-be-exported variables across multiple calls, so the non-recursive\ndefinition would likely be:\n\n test_export () {\n test_export_=\"$test_export_ $*\"\n }\n\nwhich would make a nice cleanup atop this portability-fix patch.\n\n> Oh, does anybody need to clear test_export_ to an empty string (or\n> unset it), by the way?\n\nPerhaps a test_unexport() might be handy in the distant future, but\npresently there is only a single call to test_export() in the entire\nsuite, so it's probably not worth worrying about now.\n"},{"id":"412521","messageId":"X9xBRXW7/tXsqLT5@coredump.intra.peff.net","threadId":"54836","inReplyTo":"CAPig+cR+4Wh4Sgk6UhUML4SHqaQsvYmw_77ih+oec2YmqQJCCg@mail.gmail.com","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-18T05:42:29Z","receivedAt":"2020-12-18T05:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote:\n\n> > Oh, does anybody need to clear test_export_ to an empty string (or\n> > unset it), by the way?\n> \n> Perhaps a test_unexport() might be handy in the distant future, but\n> presently there is only a single call to test_export() in the entire\n> suite, so it's probably not worth worrying about now.\n\nI actually wonder if we could drop test_export entirely. I assume you\nmean the call in p0001. It is inside a test_expect_success block, where\nwe don't need to do anything fancier than just \"export\". It is already\nrunning in the main script's environment, just like a normal test. If it\nwere in a test_perf, then we would need to take special care to get it\nback into the main script.\n\nThere are some calls in p0000 like that, but they are really about\ntesting test_export itself.\n\n-Peff\n"},{"id":"412538","messageId":"CAPig+cQvaOBo=zx=f2ZsPy7QnPXgcdc0SUZyGWaZPZ31FUUwZg@mail.gmail.com","threadId":"54836","inReplyTo":"X9xBRXW7/tXsqLT5@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-18T06:15:05Z","receivedAt":"2020-12-18T06:16:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 18, 2020 at 12:44 AM Jeff King wrote:\n> On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote:\n> > Perhaps a test_unexport() might be handy in the distant future, but\n> > presently there is only a single call to test_export() in the entire\n> > suite, so it's probably not worth worrying about now.\n>\n> I actually wonder if we could drop test_export entirely. I assume you\n> mean the call in p0001. It is inside a test_expect_success block, where\n> we don't need to do anything fancier than just \"export\". It is already\n> running in the main script's environment, just like a normal test. If it\n> were in a test_perf, then we would need to take special care to get it\n> back into the main script.\n\nConsidering that test_export() hasn't seen much use since its\nintroduction nine years ago and that the one and only existing call\ndoesn't even need the special subprocess magic, retiring the function\nis certainly an option. On the other hand, aside from this one minor\nportability fix, it hasn't been a maintenance burden and may actually\ncome in handy someday if people start writing more \"perf\" tests. So, I\ndon't feel strongly one way or the other, though I lean somewhat\ntoward keeping it around.\n"},{"id":"412543","messageId":"X9xLNrzyEv6GSKan@coredump.intra.peff.net","threadId":"54836","inReplyTo":"CAPig+cQvaOBo=zx=f2ZsPy7QnPXgcdc0SUZyGWaZPZ31FUUwZg@mail.gmail.com","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-18T06:24:54Z","receivedAt":"2020-12-18T06:25:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 18, 2020 at 01:15:05AM -0500, Eric Sunshine wrote:\n\n> On Fri, Dec 18, 2020 at 12:44 AM Jeff King wrote:\n> > On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote:\n> > > Perhaps a test_unexport() might be handy in the distant future, but\n> > > presently there is only a single call to test_export() in the entire\n> > > suite, so it's probably not worth worrying about now.\n> >\n> > I actually wonder if we could drop test_export entirely. I assume you\n> > mean the call in p0001. It is inside a test_expect_success block, where\n> > we don't need to do anything fancier than just \"export\". It is already\n> > running in the main script's environment, just like a normal test. If it\n> > were in a test_perf, then we would need to take special care to get it\n> > back into the main script.\n> \n> Considering that test_export() hasn't seen much use since its\n> introduction nine years ago and that the one and only existing call\n> doesn't even need the special subprocess magic, retiring the function\n> is certainly an option. On the other hand, aside from this one minor\n> portability fix, it hasn't been a maintenance burden and may actually\n> come in handy someday if people start writing more \"perf\" tests. So, I\n> don't feel strongly one way or the other, though I lean somewhat\n> toward keeping it around.\n\nThat more or less matches my feeling. I just like deleting things. ;)\n\n-Peff\n"},{"id":"412695","messageId":"20201220212740.44273-1-sunshine@sunshineco.com","threadId":"54836","inReplyTo":"20201216073907.62591-1-sunshine@sunshineco.com","subject":"[PATCH 2/1] t/perf: avoid unnecessary test_export() recursion","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-20T21:27:40Z","receivedAt":"2020-12-21T05:10:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"test_export() has been self-recursive since its inception even though a\nsimple for-loop would have served just as well to append its arguments\nto the `test_export_` variable separated by the pipe character \"|\".\nRecently `test_export_` was changed instead to a space-separated list of\ntokens to be exported, an operation which can be accomplished via a\nsingle simple assignment, with no need for looping or recursion.\nTherefore, simplify the implementation.\n\nWhile at it, take advantage of the fact that variable names to be\nexported are shell identifiers, thus won't be composed of special\ncharacters or whitespace, thus simple a `$*` can be used rather than\nmagical `\"$@\"`.\n\nSigned-off-by: Eric Sunshine \n---\n\nThis is a follow-up to [1] as discussed in [2]. It applies cleanly atop\n'es/perf-export-fix' and 'master'.\n\n[1]: https://lore.kernel.org/git/20201216073907.62591-1-sunshine@sunshineco.com/\n[2]: https://lore.kernel.org/git/CAPig+cR+4Wh4Sgk6UhUML4SHqaQsvYmw_77ih+oec2YmqQJCCg@mail.gmail.com/\n\nt/perf/perf-lib.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 22d727cef8..e385c6896f 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -147,10 +147,7 @@ test_run_perf_ () {\n \t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n test_export () {\n-\t[ $# != 0 ] || return 0\n-\ttest_export_=\"$test_export_ $1\"\n-\tshift\n-\ttest_export \"$@\"\n+\ttest_export_=\"$test_export_ $*\"\n }\n '\"$1\"'\n ret=$?\n-- \n2.30.0.rc1.243.g5298b911bd\n\n"},{"id":"412705","messageId":"CAPig+cRK1rq6xpAXp0bG-nekJOgv9gTZKZZV3xsTVh7CoZY4sQ@mail.gmail.com","threadId":"54836","inReplyTo":"CAPig+cR+4Wh4Sgk6UhUML4SHqaQsvYmw_77ih+oec2YmqQJCCg@mail.gmail.com","subject":"Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T07:23:33Z","receivedAt":"2020-12-21T07:26:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2020 at 2:29 PM Eric Sunshine wrote:\n> That's almost good enough, but test_export() needs to accumulate the\n> to-be-exported variables across multiple calls, so the non-recursive\n> definition would likely be:\n>\n> test_export () {\n> test_export_=\"$test_export_ $*\"\n> }\n>\n> which would make a nice cleanup atop this portability-fix patch.\n\nA cleanup patch has been posted[1].\n\n[1]: https://lore.kernel.org/git/20201220212740.44273-1-sunshine@sunshineco.com/\n"}]}