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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 16, 2020, 19:07 UTC
Message-ID: <xmqq5z514lj5.fsf@gitster.c.googlers.com>
In-Reply-To: <20201216073907.62591-1-sunshine@sunshineco.com>

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.

Show 9 quoted lines

> @@ -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?

Show 11 quoted lines

>  '"$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.