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

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 22, 2018, 23:26 UTC
Message-ID: <20180422232637.GB48072@syl.local>
In-Reply-To: <878t9eewu2.fsf@evledraar.gmail.com>

On Sun, Apr 22, 2018 at 11:48:53PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 13 quoted lines

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

Thanks, I have adjusted this change in my copy and will attach it in a subsequent re-roll.

Show 15 quoted lines

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

Fixed all of these, thanks for pointing them out :-).

Show 10 quoted lines

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

I settled on "show column number of first match", and have noted its use for callers like git-jump in the documentation.

Thanks, Taylor