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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 05 May 2018 08:49:43 +0200

On Sat, May 05 2018, Taylor Blau wrote:

> +--o::
> +--only-matching::
> +	Show only the matching part of the lines.
> +

Makes sense to steal GNU grep's description here:

    Print only the matched (non-empty) parts of a matching line, with
    each such part on a separate output line.

> +			if (!opt->only_matching)
> +				output_color(opt, bol, match.rm_so, line_color);

This should also have braces, see "When there are multiple arms to a
conditional" in Documentation/CodingGuidelines.

>  '
> +cat >expected <<EOF
> +file:1:5:mmap
> +file:2:5:mmap
> +file:3:5:mmap
> +file:3:14:mmap
> +file:4:5:mmap
> +file:4:14:mmap
> +file:5:5:mmap
> +file:5:14:mmap
> +EOF

This should be set up as part of the test itself, see e.g. my c8b2cec09e
("branch: add test for -m renaming multiple config sections",
2017-06-18) for how to do that.

> +test_expect_success 'grep --only-matching' '
> +	git grep --only-matching --line-number --column mmap file >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +file
> +1:5:mmap
> +2:5:mmap
> +3:5:mmap
> +3:14:mmap
> +4:5:mmap
> +4:14:mmap
> +5:5:mmap
> +5:14:mmap
> +EOF
> +
> +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.

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