# [PATCH] t/perf: fix test_export() failure with BSD `sed` 8 messages from 2020-12-16 to 2020-12-21. Participants: Eric Sunshine, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/54836 ## Eric Sunshine, 2020-12-16 07:39 Subject: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: <20201216073907.62591-1-sunshine@sunshineco.com> URL: https://gitlist.dev/e/20201216073907.62591-1-sunshine%40sunshineco.com ``` test_perf() runs each test in its own subshell which makes it difficult to persist variables between tests. test_export() addresses this shortcoming by grabbing the values of specified variables after a test runs but before the subshell exits, and writes those values to a file which is loaded into the environment of subsequent tests. To grab the values to be persisted, test_export() pipes the output of the shell's builtin `set` command through `sed` which plucks them out using a regular expression along the lines of `s/^(var1|var2)/.../p`. Unfortunately, though, this use of alternation is not portable. For instance, BSD-lineage `sed` (including macOS `sed`) does not support it in the default "basic regular expression" mode (BRE). It may be possible to enable "extended regular expression" mode (ERE) in some cases with `sed -E`, however, `-E` is neither portable nor part of POSIX. Fortunately, alternation is unnecessary in this case and can easily be avoided, so replace it with a series of simple expressions such as `s/^var1/.../p;s/^var2/.../p`. While at it, tighten the expressions so they match the variable names exactly rather than matching prefixes (i.e. use `s/^var1=/.../p`). If the requirements of test_export() become more complex in the future, then an alternative would be to replace `sed` with `perl` which supports alternation on all platforms, however, the simple elimination of alternation via multiple `sed` expressions suffices for the present. Reported-by: Sangeeta Diagnosed-by: Philippe Blain Signed-off-by: Eric Sunshine --- t/perf/perf-lib.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 821581a885..22d727cef8 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -148,13 +148,18 @@ test_run_perf_ () { . '"$TEST_DIRECTORY"/test-lib-functions.sh' test_export () { [ $# != 0 ] || return 0 - test_export_="$test_export_\\|$1" + test_export_="$test_export_ $1" shift test_export "$@" } '"$1"' ret=$? -set | sed -n "s'"/'/'\\\\''/g"';s/^\\($test_export_\\)/export '"'&'"'/p" >test_vars +needles= +for v in $test_export_ +do + needles="$needles;s/^$v=/export $v=/p" +done +set | sed -n "s'"/'/'\\\\''/g"'$needles" >test_vars exit $ret' >&3 2>&4 eval_ret=$? -- 2.30.0.rc0.297.gbcca948854 ``` ## Junio C Hamano, 2020-12-16 19:07 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/xmqq5z514lj5.fsf%40gitster.c.googlers.com In-Reply-To: <20201216073907.62591-1-sunshine@sunshineco.com> ``` Eric Sunshine writes: > Fortunately, alternation is unnecessary in this case and can easily be > avoided, so replace it with a series of simple expressions such as > `s/^var1/.../p;s/^var2/.../p`. Simple is good. > While at it, tighten the expressions so they match the variable names > exactly rather than matching prefixes (i.e. use `s/^var1=/.../p`). Good eyes. That is quite good. > @@ -148,13 +148,18 @@ test_run_perf_ () { > . '"$TEST_DIRECTORY"/test-lib-functions.sh' > test_export () { > [ $# != 0 ] || return 0 > - test_export_="$test_export_\\|$1" > + test_export_="$test_export_ $1" > shift > test_export "$@" > } This "recursion to consume $@ one by one, instead of looping" caught my eyes a bit, but the bug being fixed is not caused by it, so it is fine to let it pass. Given that the arguments to test_export are supposed to be all variable names (i.e. no funny characters anywhere in it), I think it could just be test_export () { test_export_="$*" } though. Oh, does anybody need to clear test_export_ to an empty string (or unset it), by the way? > '"$1"' > ret=$? > -set | sed -n "s'"/'/'\\\\''/g"';s/^\\($test_export_\\)/export '"'&'"'/p" >test_vars > +needles= > +for v in $test_export_ > +do > + needles="$needles;s/^$v=/export $v=/p" > +done > +set | sed -n "s'"/'/'\\\\''/g"'$needles" >test_vars > exit $ret' >&3 2>&4 > eval_ret=$? Other than these, none of which were "this is wrong and needs to be fixed", I have no comments. The patch is quite readably done. Will queue. Thanks. ``` ## Eric Sunshine, 2020-12-16 19:29 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR%2B4Wh4Sgk6UhUML4SHqaQsvYmw_77ih%2Boec2YmqQJCCg%40mail.gmail.com In-Reply-To: ``` On Wed, Dec 16, 2020 at 2:07 PM Junio C Hamano wrote: > Eric Sunshine writes: > > test_export () { > > [ $# != 0 ] || return 0 > > - test_export_="$test_export_\\|$1" > > + test_export_="$test_export_ $1" > > shift > > test_export "$@" > > } > > This "recursion to consume $@ one by one, instead of looping" caught > my eyes a bit, but the bug being fixed is not caused by it, so it is > fine to let it pass. The unusual recursion caught my eye as well. > Given that the arguments to test_export are > supposed to be all variable names (i.e. no funny characters anywhere > in it), I think it could just be > > test_export () { > test_export_="$*" > } > > though. That's almost good enough, but test_export() needs to accumulate the to-be-exported variables across multiple calls, so the non-recursive definition would likely be: test_export () { test_export_="$test_export_ $*" } which would make a nice cleanup atop this portability-fix patch. > Oh, does anybody need to clear test_export_ to an empty string (or > unset it), by the way? Perhaps a test_unexport() might be handy in the distant future, but presently there is only a single call to test_export() in the entire suite, so it's probably not worth worrying about now. ``` ## Jeff King, 2020-12-18 05:42 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/X9xBRXW7%2FtXsqLT5%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote: > > Oh, does anybody need to clear test_export_ to an empty string (or > > unset it), by the way? > > Perhaps a test_unexport() might be handy in the distant future, but > presently there is only a single call to test_export() in the entire > suite, so it's probably not worth worrying about now. I actually wonder if we could drop test_export entirely. I assume you mean the call in p0001. It is inside a test_expect_success block, where we don't need to do anything fancier than just "export". It is already running in the main script's environment, just like a normal test. If it were in a test_perf, then we would need to take special care to get it back into the main script. There are some calls in p0000 like that, but they are really about testing test_export itself. -Peff ``` ## Eric Sunshine, 2020-12-18 06:15 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQvaOBo%3Dzx%3Df2ZsPy7QnPXgcdc0SUZyGWaZPZ31FUUwZg%40mail.gmail.com In-Reply-To: ``` On Fri, Dec 18, 2020 at 12:44 AM Jeff King wrote: > On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote: > > Perhaps a test_unexport() might be handy in the distant future, but > > presently there is only a single call to test_export() in the entire > > suite, so it's probably not worth worrying about now. > > I actually wonder if we could drop test_export entirely. I assume you > mean the call in p0001. It is inside a test_expect_success block, where > we don't need to do anything fancier than just "export". It is already > running in the main script's environment, just like a normal test. If it > were in a test_perf, then we would need to take special care to get it > back into the main script. Considering that test_export() hasn't seen much use since its introduction nine years ago and that the one and only existing call doesn't even need the special subprocess magic, retiring the function is certainly an option. On the other hand, aside from this one minor portability fix, it hasn't been a maintenance burden and may actually come in handy someday if people start writing more "perf" tests. So, I don't feel strongly one way or the other, though I lean somewhat toward keeping it around. ``` ## Jeff King, 2020-12-18 06:24 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/X9xLNrzyEv6GSKan%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Dec 18, 2020 at 01:15:05AM -0500, Eric Sunshine wrote: > On Fri, Dec 18, 2020 at 12:44 AM Jeff King wrote: > > On Wed, Dec 16, 2020 at 02:29:26PM -0500, Eric Sunshine wrote: > > > Perhaps a test_unexport() might be handy in the distant future, but > > > presently there is only a single call to test_export() in the entire > > > suite, so it's probably not worth worrying about now. > > > > I actually wonder if we could drop test_export entirely. I assume you > > mean the call in p0001. It is inside a test_expect_success block, where > > we don't need to do anything fancier than just "export". It is already > > running in the main script's environment, just like a normal test. If it > > were in a test_perf, then we would need to take special care to get it > > back into the main script. > > Considering that test_export() hasn't seen much use since its > introduction nine years ago and that the one and only existing call > doesn't even need the special subprocess magic, retiring the function > is certainly an option. On the other hand, aside from this one minor > portability fix, it hasn't been a maintenance burden and may actually > come in handy someday if people start writing more "perf" tests. So, I > don't feel strongly one way or the other, though I lean somewhat > toward keeping it around. That more or less matches my feeling. I just like deleting things. ;) -Peff ``` ## Eric Sunshine, 2020-12-20 21:27 Subject: [PATCH 2/1] t/perf: avoid unnecessary test_export() recursion Message-ID: <20201220212740.44273-1-sunshine@sunshineco.com> URL: https://gitlist.dev/e/20201220212740.44273-1-sunshine%40sunshineco.com In-Reply-To: <20201216073907.62591-1-sunshine@sunshineco.com> ``` test_export() has been self-recursive since its inception even though a simple for-loop would have served just as well to append its arguments to the `test_export_` variable separated by the pipe character "|". Recently `test_export_` was changed instead to a space-separated list of tokens to be exported, an operation which can be accomplished via a single simple assignment, with no need for looping or recursion. Therefore, simplify the implementation. While at it, take advantage of the fact that variable names to be exported are shell identifiers, thus won't be composed of special characters or whitespace, thus simple a `$*` can be used rather than magical `"$@"`. Signed-off-by: Eric Sunshine --- This is a follow-up to [1] as discussed in [2]. It applies cleanly atop 'es/perf-export-fix' and 'master'. [1]: https://lore.kernel.org/git/20201216073907.62591-1-sunshine@sunshineco.com/ [2]: https://lore.kernel.org/git/CAPig+cR+4Wh4Sgk6UhUML4SHqaQsvYmw_77ih+oec2YmqQJCCg@mail.gmail.com/ t/perf/perf-lib.sh | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 22d727cef8..e385c6896f 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -147,10 +147,7 @@ test_run_perf_ () { "$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c ' . '"$TEST_DIRECTORY"/test-lib-functions.sh' test_export () { - [ $# != 0 ] || return 0 - test_export_="$test_export_ $1" - shift - test_export "$@" + test_export_="$test_export_ $*" } '"$1"' ret=$? -- 2.30.0.rc1.243.g5298b911bd ``` ## Eric Sunshine, 2020-12-21 07:23 Subject: Re: [PATCH] t/perf: fix test_export() failure with BSD `sed` Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRK1rq6xpAXp0bG-nekJOgv9gTZKZZV3xsTVh7CoZY4sQ%40mail.gmail.com In-Reply-To: ``` On Wed, Dec 16, 2020 at 2:29 PM Eric Sunshine wrote: > That's almost good enough, but test_export() needs to accumulate the > to-be-exported variables across multiple calls, so the non-recursive > definition would likely be: > > test_export () { > test_export_="$test_export_ $*" > } > > which would make a nice cleanup atop this portability-fix patch. A cleanup patch has been posted[1]. [1]: https://lore.kernel.org/git/20201220212740.44273-1-sunshine@sunshineco.com/ ```