Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau <me@ttaylorr.com>]

On Wed, May 09, 2018 at 11:41:02AM +0100, Phillip Wood wrote:
> Hi Taylor
>
> On 09/05/18 03:13, Taylor Blau wrote:
> > Teach 'git-grep(1)' a new option, '--column', to show the column
> > number of the first match on a non-context line. This makes it possible
> > to teach 'contrib/git-jump/git-jump' how to seek to the first matching
> > position of a grep match in your editor, and allows similar additional
> > scripting capabilities.
> >
> > For example:
> >
> >    $ git grep -n --column foo | head -n3
> >    .clang-format:51:14:# myFunction(foo, bar, baz);
> >    .clang-format:64:7:# int foo();
> >    .clang-format:75:8:# void foo()
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >   Documentation/git-grep.txt |  6 +++++-
> >   builtin/grep.c             |  4 ++++
> >   grep.c                     |  3 +++
> >   t/t7810-grep.sh            | 32 ++++++++++++++++++++++++++++++++
> >   4 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..75f1561112 100644
> > --- a/Documentation/git-grep.txt
> > +++ b/Documentation/git-grep.txt
> > @@ -13,7 +13,7 @@ SYNOPSIS
> >   	   [-v | --invert-match] [-h|-H] [--full-name]
> >   	   [-E | --extended-regexp] [-G | --basic-regexp]
> >   	   [-P | --perl-regexp]
> > -	   [-F | --fixed-strings] [-n | --line-number]
> > +	   [-F | --fixed-strings] [-n | --line-number] [--column]
> >   	   [-l | --files-with-matches] [-L | --files-without-match]
> >   	   [(-O | --open-files-in-pager) [<pager>]]
> >   	   [-z | --null]
> > @@ -169,6 +169,10 @@ providing this option will cause it to die.
> >   --line-number::
> >   	Prefix the line number to matching lines.
> > +--column::
> > +	Prefix the 1-indexed byte-offset of the first match on non-context lines. This
> > +	option is incompatible with '--invert-match', and extended expressions.
> > +
>
> Sorry to be fussy, but while this is clearer I think to could be improved to
> make it clear that it is the offset from the start of the matching line.
> Also the mention of 'extended expressions' made me think of 'grep -E' but I
> think (correct me if I'm wrong) you mean the boolean options '--and',
> '--not' and '--or'. The man page only uses the word extended when talking
> about extended regexes. I think something like
>
> Print the 1-indexed byte-offset of the first match from the start of the
> matching line. This option is incompatible with '--invert-match', '--and',
> '--not' and '--or'.
>
> would be clearer

I agree, and would be happy to change it as-such. I think that there is
some pending discussion about regressing 'git-jump' no longer supporting
'--not', so I'll wait for that to resolve before resending this patch.

Thanks,
Taylor