Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 05, 2018 at 08:49:43AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > +test_expect_success 'grep --only-matching --heading' '
> > +	git grep --only-matching --heading --line-number --column mmap file >actual &&
> > +	test_cmp expected actual
> > +'
> > +
> >  cat >expected <<EOF
> >  <BOLD;GREEN>hello.c<RESET>
> >  4:int main(int argc, const <BLACK;BYELLOW>char<RESET> **argv)
> 
> We should test this a lot more, I think a good way to do that would be
> to extend this series by first importing GNU grep's -o tests, see
> http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
> license-compatible. Then change the grep_test() function to call git
> grep instead.

I'm trying to figure out what GNU grep's tests are actually checking
that we don't have. I see:

 - they check that "-i" returns the actual found string in its original
   case. This seems like a subset of finding a non-trivial regex. I.e.,
   "foo.*" should find "foobar". We probably should have a test like
   that.

 - they test multiple hits on the same line, which seems like an
   important and easy-to-screw-up case; we do that, too.

 - they test everything other thing with and without "-i" because those
   are apparently separate code paths in GNU grep, but I don't think
   that applies to us.

 - they test each case with "-b", but we don't have that (we do test
   with "--column", which is good)

 - they test with "-n", which we do here (we don't test without, but
   that seems like an unlikely bug, knowing how it is implemented)

 - they test with -H, but that is already the default for git-grep

 - they test with context (-C3) for us. It looks like GNU grep omits
   context lines with "-o", but we show a bunch of blank lines. This is
   I guess a bug (though it seems kind of an odd combination to specify
   in the first place)

So I think it probably makes sense to just pick through the list I just
wrote and write our own tests than to try to import GNU grep's specific
tests (and there's a ton of other unrelated tests in that file that may
or may not even run with git-grep).

> It should also be tested with the various grep.patternType options to
> make sure it works with basic, extended, perl, fixed etc.

Hmm, this code is all external to the actual matching. So unless one of
those is totally screwing up the regmatch_t return, I'm not sure that's
accomplishing much (and if one of them is, it's totally broken for
coloring, "-c", and maybe other features).

We've usually taken a pretty white-box approach to our testing, covering
things that seem likely to go wrong given the general problem space and
our implementation. But maybe I'm missing something in particular that
you think might be tricky.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux