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

[Taylor Blau]

On Mon, May 07, 2018 at 11:13:12PM +0900, Junio C Hamano wrote:
> Taylor Blau  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) []]
> >[-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

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

[Taylor Blau]

On Sun, May 06, 2018 at 07:56:42PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, May 05 2018, Taylor Blau wrote:
>
>
> > +   test_expect_success "grep -w $L (with --{line,column}-number)" '
>
> It's now --column in v4 but this still refers to v3 --column-number.

Thanks, I certainly missed this one.


Thanks,
Taylor

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

[Taylor Blau]

On Sat, May 05, 2018 at 08:15:03AM +0200, Duy Nguyen wrote:
> On Sat, May 5, 2018 at 4:43 AM, 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.
>
> Why? Or put it another way, what is this option used for? Only
> git-jump? (which should also be mentioned here if true)

Good question. My primary intention is giving
'contrib/git-jump/git-jump' the information it needs in order to tell
$EDITOR how to seek to the relevant position within a line.

I have amended this patch to include the relevant bits, and will attach
in v5.


Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau  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) []]
>  [-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?

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, May 05 2018, Taylor Blau wrote:


> + test_expect_success "grep -w $L (with --{line,column}-number)" '

It's now --column in v4 but this still refers to v3 --column-number.

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

[Phillip Wood]

Hi Taylor

On 05/05/18 03:43, 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.
> 
> 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 
> ---
>  Documentation/git-grep.txt |  5 -
>  builtin/grep.c |  1 +
>  t/t7810-grep.sh| 22 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> 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) []]
>  [-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.
> +

I think it would be useful to explain what the column number actually is
so that users know how to consume it. Is it a count of bytes, multi-byte
characters or graphemes? It would probably be worth testing with a file
that contains multi-byte characters to check for future regressions.

Best Wishes

Phillip

>  -l::
>  --files-with-matches::
>  --name-only::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 5f32d2ce84..5c83f17759 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   GREP_PATTERN_TYPE_PCRE),
>   OPT_GROUP(""),
>   OPT_BOOL('n', "line-number", , N_("show line 
> numbers")),
> + OPT_BOOL(0, "column", , N_("show column number of 
> first match")),
>   OPT_NEGBIT('h', NULL, , N_("don't show 
> filenames"), 1),
>   OPT_BIT('H', NULL, , N_("show filenames"), 1),
>   OPT_NEGBIT(0, "full-name", ,
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 1797f632a3..a03c3416e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
>   test_cmp expected actual
>   '
>  
> + test_expect_success "grep -w $L (with --column)" '
> + {
> + echo ${HC}file:5:foo mmap bar
> + echo ${HC}file:14:foo_mmap bar mmap
> + echo ${HC}file:5:foo mmap bar_mmap
> + echo ${HC}file:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
> + test_expect_success "grep -w $L (with --{line,column}-number)" '
> + {
> + echo ${HC}file:1:5:foo mmap bar
> + echo ${HC}file:3:14:foo_mmap bar mmap
> + echo ${HC}file:4:5:foo mmap bar_mmap
> + echo ${HC}file:5:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep -n --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
>   test_expect_success "grep -w $L" '
>   {
>   echo ${HC}file:1:foo mmap bar
> 

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

[Duy Nguyen]

On Sat, May 5, 2018 at 4:43 AM, 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.

Why? Or put it another way, what is this option used for? Only
git-jump? (which should also be mentioned here if true)

>
> 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()
>
-- 
Duy