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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 5, 2018, 06:49 UTC
Message-ID: <87fu36y4u0.fsf@evledraar.gmail.com>
In-Reply-To: <c8b527c5de3b0e5422d2c1afb91d454d1e46fff4.1525492696.git.me@ttaylorr.com>

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.

Show 12 quoted lines

>  '
>
> +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.

Show 25 quoted lines

> +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.