Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau <me@ttaylorr.com>]

On Sun, Apr 22, 2018 at 08:32:28PM -0400, Eric Sunshine wrote:
> 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.

Thanks. I am not used to writing in this mood, but have amended my
patches locally to conform to your proposed layout and have reworded
each to be in the imperative mood.

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

Thanks again, I have moved this introduction to the relevant patch.

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

:-). I have changed this locally to indicate which is which in the hopes
that it will provide more clarity should these tests fail at any point.


Thanks,
Taylor