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

[Phillip Wood <phillip.wood@talktalk.net>]

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

Best Wishes

Phillip


>   -l::
>   --files-with-matches::
>   --name-only::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 5f32d2ce84..f9f516dfc4 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", &opt.linenum, N_("show line numbers")),
> +		OPT_BOOL(0, "column", &opt.columnnum, N_("show column number of first match")),
>   		OPT_NEGBIT('h', NULL, &opt.pathname, N_("don't show filenames"), 1),
>   		OPT_BIT('H', NULL, &opt.pathname, N_("show filenames"), 1),
>   		OPT_NEGBIT(0, "full-name", &opt.relative,
> @@ -1111,6 +1112,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>   		hit = grep_objects(&opt, &pathspec, the_repository, &list);
>   	}
>   
> +	if (opt.columnnum && opt.invert)
> +		die(_("--column and --invert-match cannot be combined"));
> +
>   	if (num_threads)
>   		hit |= wait_all();
>   	if (hit && show_in_pager)
> diff --git a/grep.c b/grep.c
> index f3fe416791..f4228c23ac 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -995,6 +995,9 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>   	else if (!opt->extended && !opt->debug)
>   		return;
>   
> +	if (opt->columnnum && opt->extended)
> +		die(_("--column and extended expressions cannot be combined"));
> +
>   	p = opt->pattern_list;
>   	if (p)
>   		opt->pattern_expression = compile_pattern_expr(&p);
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 1797f632a3..aa56b21ed9 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-number, --column)" '
> +		{
> +			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
> @@ -1590,4 +1612,14 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success 'grep does not allow --column, --invert-match' '
> +	test_must_fail git grep --column --invert-match pat 2>err &&
> +	test_i18ngrep "\-\-column and \-\-invert-match cannot be combined" err
> +'
> +
> +test_expect_success 'grep does not allow --column, extended' '
> +	test_must_fail git grep --column --not -e pat 2>err &&
> +	test_i18ngrep "\-\-column and extended expressions cannot be combined" err
> +'
> +
>   test_done
>