Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Apr 23, 2018, 01:14 UTC
- Message-ID
- <20180423011431.GC78148@syl.local>
- In-Reply-To
- <CAPig+cTozduqSAxh+w4H85m7en72Yo09asdx+1KSTswqbnBr4w@mail.gmail.com>
On Sun, Apr 22, 2018 at 08:32:28PM -0400, Eric Sunshine wrote:
Show 9 quoted lines
> 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.
Show 13 quoted lines
> > 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.
Show 21 quoted lines
> > 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