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@xxxxxxxxxxxxxxxxxxx/) :) Sure, thanks for pointing me in the right direction :-). Thanks, Taylor
