From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sun, 22 Apr 2018 21:48:53 GMT
Subject: Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number
Message-ID: <878t9eewu2.fsf@evledraar.gmail.com>
In-Reply-To: <6dd73ad84cad5a749bf05c500295955970577e04.1524429778.git.me@ttaylorr.com>


On Sun, Apr 22 2018, Taylor Blau wrote:

I think this part though...

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
> [...]
> -`linenumber`;;
> +`lineNumber`;;

Makes sense as its own patch at the beginning of the series, since it's
just related cleanup.

> +`columnNumber`;;
> +	column number prefix (when using `--column-number`)

Here you're using --column-number...

> +grep.columnNumber::
> +	If set to true, enable `-m` option by default.

...But not here. This needs to be updated

> +grep.columnNumber::
> +	If set to true, enable `-m` option by default.
> +

...ditto.

> +--column-number::
> +	Prefix the 1-indexed column number of the first match on non-context lines.
> +
> [...]
>  		OPT_GROUP(""),
>  		OPT_BOOL('n', "line-number", &opt.linenum, N_("show line numbers")),
> +		OPT_BOOL(0, "column-number", &opt.columnnum, N_("show column numbers")),

Maybe "show first matching column"? I.e. the main docs say "just shows
the first", but this seems to give a different impression.

It would also be nice if the docs briefly explained what this is for,
i.e. the git-jump use-case.