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 07:26:57PM +0200, Martin Ågren wrote:
> On 9 May 2018 at 12:41, Phillip Wood <phillip.wood@talktalk.net> wrote:
> > On 09/05/18 03:13, Taylor Blau wrote:
> >>
> >>   +--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
>
> >> +       if (opt->columnnum && opt->extended)
> >> +               die(_("--column and extended expressions cannot be combined"));
> >> +
>
> Just so it doesn't get missed: Phillip's comment (which I agree with)
> about "extended" would apply here as well. This would work fine, no?

This check we should retain and change the wording to mention '--and',
'--or', and '--not' specifically.

> One thing to notice is that dying for `--column --not` in combination
> with patch 7/7 makes git-jump unable to handle `--not` (and friends).
> That would be a regression? I suppose git-jump could use a special
> `--maybe-column` which would be "please use --column, unless I give you
> something that won't play well with it". Or --column could do that kind
> of falling back on its own. Or, git-jump could scan the arguments to
> decide whether to use `--column` or not. Hmm... Tricky. :-/

Agree that this is tricky. I don't think that --maybe-column is a
direction that we should take for the reasons I outlined in the cover
letter. Like I said, there are cases under an extended grammar where we
can and cannot display meaningful column offsets.

With regards to regressing 'git-jump', I feel as if 'git-jump --not' is
an uncommon-enough case that I would be comfortable with the tradeoff.
If a caller _is_ using '--not' in 'git-jump', they can reconfigure
'jump.grepCmd' to work around this issue.

Perhaps this is worth warning about in 'git-jump'? Peff, what do you
think?


Thanks,
Taylor