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

From: Martin Ågren <martin.agren@gmail.com>
Date: May 9, 2018, 17:26 UTC
Message-ID: <CAN0heSpXCajyDKwOrXuQHWRODsVnFewUWTMitOoRQueFV4eSiw@mail.gmail.com>
In-Reply-To: <882bdfe8-6caa-dd9c-7752-ee4884f135f9@talktalk.net>

On 9 May 2018 at 12:41, Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 22 quoted lines

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

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

Martin