On Wed, Oct 14, 2020 at 11:09 AM Jeff King <peff@xxxxxxxx> wrote:
>
> On Wed, Oct 14, 2020 at 10:50:03AM -0700, Elijah Newren wrote:
>
> > > > @@ -819,9 +821,8 @@ match_test_selector_list () {
> > > > *)
> > > > if expr "z$selector" : "z[0-9]*[^0-9]" >/dev/null
> > > > then
> > > > - echo "error: $title: invalid non-numeric in test" \
> > > > - "selector: '$orig_selector'" >&2
> > > > - exit 1
> > > > + echo "$title" | grep -q "$selector" && return
> > > > + continue
> > > > fi
> > >
> > > I like that you allow regexes. It's unfortunate that the skip-check
> > > costs us a process in every test. It may not be that big a deal since we
> > > only pay it if you use a non-numeric selector. But I wonder if there's
> > > any reason not to use "expr" here, as well.
> >
> > I originally used [[ $title =~ "$selector" ]] in order to avoid the
> > sub-shell...but that was bash-specific. I briefly looked to see if
> > there was a shell portable way to handle regexes, wasn't having much
> > luck, and decided that this is only likely to arise when people are
> > passing --run and thus only running a single script and they avoid all
> > the subprocesses that would have been invoked inside the test, so it's
> > still a big net win overall. Does expr handle regexes, portably? Or
> > are you suggesting dropping the regex handling and limit it to
> > substring matching? In either case, does using expr save us anything
> > (isn't expr a shell command)?
>
> There's an expr command doing a regex match in the diff context quoted
> above. :)
Oh, indeed. How'd I miss that?
> I was wrong that it would save us a process, though. I thought it was a
> builtin in modern shells, but it doesn't appear to be in either dash or
> bash. So there's little reason to prefer it over grep here (switching to
> globs could be done with case and would save a process).
I guess it's at least slightly more consistent with the surrounding code.
Oh, and I found a bug in that my code was not handling negated
substrings correctly, e.g.
./test-script.sh --run="!rename"
would run all the tests instead of excluding the ones that had
"rename" as a substring. So I'll send a reroll fixing that.
