From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 23 Apr 2018 00:32:28 GMT
Subject: Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number
Message-ID: <CAPig+cTozduqSAxh+w4H85m7en72Yo09asdx+1KSTswqbnBr4w@mail.gmail.com>
In-Reply-To: <6dd73ad84cad5a749bf05c500295955970577e04.1524429778.git.me@ttaylorr.com>

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> option builds upon previous commits to show the column number of the
> first match on a non-context line.

Imperative mood (and dropping unnecessary "builds upon previous"):

    Teach 'git-grep(1)' a new option '--column-number' which shows the
    column number of the first match on a non-context line.

> For example:
>
>   $ ./git-grep -n --column-number foo | head -n3
>   .clang-format:51:14:# myFunction(foo, bar, baz);
>   .clang-format:64:7:# int foo();
>   .clang-format:75:8:# void foo()
>
> Now that configuration variables such as grep.columnNumber and
> color.grep.columnNumber have a visible effect, we document them in this
> patch as well.

As mentioned in my review of patch 2, document the configuration
variables in the patch which introduces them.

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
> +       test_expect_success "grep -w $L" '
> +               ...
> +       '
> +
> +       test_expect_success "grep -w $L" '
> +               ...
> +       '
> +
>         test_expect_success "grep -w $L" '

I realize that several existing tests in this script are already
guilty of this sin, but please give each new test a distinct title
reflective of what it is actually testing in order to make it easier
to correlate failed test output with the actual test code.