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

["Martin Ågren" <martin.agren@gmail.com>]

On 21 April 2018 at 05:45, 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.
>
> For example:
>
>   $ git grep -mn example | head -n3
>   .clang-format:51:14:# myFunction(foo, bar, baz);
>   .clang-format:64:7:# int foo();
>   .clang-format:75:8:# void foo()

"example" vs "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.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---

One thing I've noted is that your messages are light on the imperative
"do this", preferring "this commit does" or "we do". That said, I found
myself following along quite well in what they're saying.

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1159,6 +1159,8 @@ color.grep.<slot>::
>         function name lines (when using `-p`)
>  `linenumber`;;
>         line number prefix (when using `-n`)
> +`columnnumber`;;
> +       column number prefix (when using `-m`)
>  `match`;;
>         matching text (same as setting `matchContext` and `matchSelected`)
>  `matchContext`;;
> @@ -1708,6 +1710,9 @@ gitweb.snapshot::
>  grep.lineNumber::
>         If set to true, enable `-n` option by default.
>
> +grep.columnNumber::
> +       If set to true, enable `-m` option by default.
> +
>  grep.patternType::
>         Set the default matching behavior. Using a value of 'basic', 'extended',
>         'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,

You're doing what the immediate neighbours are doing, but the end result
is a bit inconsistent: columnnumber vs columnNumber. I think the ideal
end-game is columnNumber, lineNumber, and so on.

Maybe use "columnNumber" consistently since you're introducing it and
want it to be perfect from the start ;-) and if that leaves "linenumber"
looking too inconsistent, then change it while at it? (I'm not suggesting
changing all foobar to fooBar while at it...)

Martin