[PATCH] t/perf: fix test_export() failure with BSD `sed`

[PATCH] t/perf: fix test_export() failure with BSD `sed`

[Eric Sunshine]

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(-)

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

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Junio C Hamano]

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.

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Eric Sunshine]

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.

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Jeff King]

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

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Eric Sunshine]

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.

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Jeff King]

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

Re: [PATCH] t/perf: fix test_export() failure with BSD `sed`

[Eric Sunshine]

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/

[PATCH 2/1] t/perf: avoid unnecessary test_export() recursion

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---

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