Re: [PATCH v2 7/8] test-lib: test_region looks for trace2 regions

[Derrick Stolee <stolee@gmail.com>]

On 1/22/2021 2:42 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> +	grep -e "\"region_enter\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"
> 
> ... this makes sure there is enter/category on a line (and
> leave/category on a line with another check).  Makes sense.
> 
> But...
> 
> 	test_region '!' '\(unmatching capture)' 'two' 'three'
> 
> would try to use an invalid regexp and cause grep to exit with 2,
> which would mean ...
> 
>> +	exitcode=$?
>> +
>> +	if test $exitcode != $expect_exit
> 
> ... this will not trigger and we return "success" (i.e. "failed as
> expected")?

Am I misunderstanding something here? If exitcode is 2, then this
will always trigger and return 1, signaling a failure. That would
propagate to the parent test and cause the test to fail. That seems
like the correct intention, but I'm not 100% confident about that.

> 	Clarification.  The point is *NOT* that the grep pattern is
> 	not robust against funnies in $1 and $2---after all, these
> 	strings are under our control.  The point is what should
> 	happen when "grep" exits with an error when asked to ensure
> 	that there is no region detected.

I'll be more robust to these in the next version. We'll expect
exit code equal to zero or _not_ equal to zero, depending on the
presence of '!'. This has the downside of returning success for
bad input strings when '!' is specified.

Basically, the approach I'm taking for v3 is here:

	if [test $expect_exit = 1] && [test $exitcode = 0]
	then
		return 1
	elif [test $expect_exit = 0] && [test $exitcode != 0]
	then
		return 1
	fi

>> +	grep -e "\"region_leave\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"
> 
> The same comment on "what about an error from grep" applies to this
> one.
> 
> It might be easier to read to avoid having to say too many
> backslash-quoted double quotes:
> 
> 	grep -e	'"region_leave".*"category":"'"$1"'","label":"'"$2"\" "$3"
> 
> This comment applies to the earlier "grep", too.

Thanks. This does look a bit cleaner.

-Stolee