On 1/22/2021 2:42 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
