| Message ID | 20201216073907.62591-1-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d4187bd4d5091cf2700b0a954cbdc379e5d1248d |
| Headers | show |
| Series | t/perf: fix test_export() failure with BSD `sed` | expand |
Eric Sunshine <sunshine@sunshineco.com> 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.
On Wed, Dec 16, 2020 at 2:07 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> 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.
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
On Fri, Dec 18, 2020 at 12:44 AM Jeff King <peff@peff.net> 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.
On Fri, Dec 18, 2020 at 01:15:05AM -0500, Eric Sunshine wrote: > On Fri, Dec 18, 2020 at 12:44 AM Jeff King <peff@peff.net> 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
On Wed, Dec 16, 2020 at 2:29 PM Eric Sunshine <sunshine@sunshineco.com> 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/
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=$?
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 <sangunb09@gmail.com> Diagnosed-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/perf/perf-lib.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)