Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau <me@ttaylorr.com>]

On Mon, Apr 23, 2018 at 10:01:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Apr 23 2018, Taylor Blau wrote:
>
> > For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno
>
> Looks good to me aside from two minor issues I noticed:
>
>  * In "grep.c: display column number of first match" you use a comment
>    style we don't normally use, i.e. /**\n not /*\n. See "Multi-line
>    comments" in Documentation/CodingGuidelines.
>
>  * You're not updating contrib/git-jump/README to document the new
>    output format. It just refers to the old format, but now depending on
>    if you use "grep" or not it'll use this new thing. It also makes
>    sense to update the example added in 007d06aa57 ("contrib/git-jump:
>    allow to configure the grep command", 2017-11-20) which seems to have
>    added jump.grepCmd as a workaround for not having this.
>
> But also, after just looking at this the second time around; Is there a
> reason we shouldn't just call this --column, not --column-number? I
> realize the former works because of the lazyness of our getopt parsing
> (also --colu though..).
>
> I think when we add features to git-grep we should be as close to GNU
> grep as possible (e.g. not add this -m alias meaning something different
> as in your v1), but if GNU grep doesn't have something go with the trend
> of other grep tools, as noted at
> https://beyondgrep.com/feature-comparison/ (and I found another one that
> has this: https://github.com/beyondgrep/website/pull/83), so there's
> already 3 prominent grep tools that call this just --column.
>
> I think we should just go with that.

I would be happy with either, though I think that my preference is to
retain '--column-number', as introduced in v2. I think that given the
choice between (1) staying closer to our conventions (i.e.,
'--line-number' instead of '--line') and (2) staying closer to other
tools', I'd choose (1).

That said, I'll happily pick up whichever the majority prefers, so if
that's --column and not --column-number, that works OK for me. I believe
that the ultimate say should be up to Junio.

> Also, as a bonus question, since you're poking at this column code
> anyway, interested in implementing -o (--only-matching)? That would be
> super-useful (see
> https://public-inbox.org/git/87in9ucsbb.fsf@evledraar.gmail.com/) :)

Sure, thanks for pointing me in the right direction :-).


Thanks,
Taylor