patcht/perf: fix test_export() failure with BSD `sed`

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Simple is good.

Good eyes.  That is quite good.

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?

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:

The unusual recursion caught my eye as well.

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.

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:

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:

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:

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:

A cleanup patch has been posted[1].

[1]: https://lore.kernel.org/git/20201220212740.44273-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 <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(-)