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

[Taylor Blau <me@ttaylorr.com>]

On Mon, May 07, 2018 at 11:13:12PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..5409a24399 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,9 @@ providing this option will cause it to die.
> >  --line-number::
> >  	Prefix the line number to matching lines.
> >
> > +--column::
> > +	Prefix the 1-indexed column number of the first match on non-context lines.
> > +
>
> Two questions.
>
>  - It is fine that the leftmost column is 1, but what does this
>    number count?  The number of bytes on the same line before the
>    first byte of the hit (plus 1)?  The display width of the initial
>    non-matching part of the line (plus 1) on a fixed-width terminal?
>    The number of "characters"?  Something else?

The count is the byte offset from the 1-index (which is the beginning of
the line, as you noted).

Incidentally, Peff and I chatted briefly offline about this, and agree
that it makes the most sense, since (1) Vim treats it correctly, and (2)
we can't be sure of options like display width, character count, etc.,
without knowing the character encoding.

Nonetheless, other folks in this thread seem to be curious about this
as well. I'll add it to the documentation for --column in
Documentation/git-grep.txt.

>  - Does --column combined with -v make any sense?  If not, shouldn't
>    the command error out when both are given at the same time?

I hadn't thought of this. They do not work together, since 'git grep -v
--column' would require us to either (1) not output the column number,
or (2) output '0', or some other non-value.

I think that both (1) and (2) require callers to complicate their
scripts to understand either approach. As such, I think that we should
die() here, and add a test in t7810 to ensure that that's indeed what
happens.

Does this seem sensible to include in v5?


Thanks,
Taylor