On 4/3/2021 4:21 PM, Tom Saeger wrote: > On Fri, Apr 02, 2021 at 06:32:56PM -0400, Eric Sunshine wrote: >> On Fri, Apr 2, 2021 at 4:43 PM Derrick Stolee <stolee@xxxxxxxxx> wrote: >>> I have a branch available [1], but I'm seeing some failures only >>> on FreeBSD [2] and I can't understand why that platform is failing >>> this test. The current version (as of this writing) does not do >>> the substring replacement technique, and hence it just gives up >>> on exact matches. I will try the substring approach as an >>> alternative and see where that gets me. >>> >>> [1] https://github.com/gitgitgadget/git/pull/924 >>> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534 >> >> The "+" in patterns such as `+refs/heads/\\*:refs/prefetch...` is what >> is throwing it off. FreeBSD `grep` doesn't seem to like it, though >> it's not clear why. Escaping it the same way as you escaped "*" >> doesn't make it work. Replacing "+" with catchall "." does work, so >> that's one way to fix it. >> >> However, all the escaping you need to do in these refspec patterns to >> pass them to `grep` is ugly. A much better solution may be to change >> the `grep` in test-lib-functions.sh:test_subcommand() to `grep -F` to >> force it to match literally. That way, you can drop all the backslash >> escaping, including those in front of "[" and "]". A cursory audit of >> callers test_subcommand() seems to indicate that none of them pass >> regex patterns, so using `-F` is probably safe and a good idea. Yes, this is an excellent idea. Thanks. >> By the way, the `coccinelle` check is also "failing", correctly >> suggesting that you change: >> >> strbuf_addf(&replace, ":refs/prefetch/"); >> >> to: >> >> strbuf_addstr(&replace, ":refs/prefetch/"); >> >> in `builtin/gc.c`. Good point! I'll get that, too. > Was curious - so rolled all Eric's feedback into a patch. > > This passes 'make test' and 'make coccicheck' locally, not sure about FreeBSD. > > Stolee - can you squash this onto your PR and try it if you agree? I made my own version of a patch before seeing this one, but thanks for working on it. > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 6348e8d7339c..e9a1cf3e227a 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1652,9 +1652,9 @@ test_subcommand () { > > if test -n "$negate" > then > - ! grep "\[$expr\]" > + ! grep -F "$expr" > else > - grep "\[$expr\]" > + grep -F "$expr" > fi Specifically I would use "[$expr]" here so it includes the terminating characters of the JSON array that we are testing against. It can be important that we have the complete set of arguments, so these brackets are important. Thanks, all. I'm taking a look at a mechanism for doing this without munging the raw string directly, as Junio mentioned. It might require adding an output format method in the refspec API, but that's probably a good thing, anyway. Thanks, -Stolee
