[PATCH 1/6] grep.c: take regmatch_t as argument in match_line()

[PATCH 1/6] grep.c: take regmatch_t as argument in match_line()

[Taylor Blau]

In a subsequent patch, we teach show_line() to optionally include the
column number of the first match on each matched line.

The regmatch_t involved in match_line() and match_one_pattern() both
contain this information (via regmatch_t->rm_so), but their current
implementation throws this stack variable away at the end of the call.

Instead, let's teach match_line() to take in a 'regmatch_t *' so that
callers can inspect the result of their calls. This will prove useful in
a subsequent commit when a caller will forward on information from the
regmatch_t into show_line (as described above).

The return condition remains unchanged, therefore the only change
required of callers is the addition of a single argument.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

show_line() currently receives the line number within the
'grep_opt->buf' in order to determine which line number to display. In
order to display information about the matching column number--if
requested--we must additionally take in that information.

To do so, we extend the signature of show_line() to take in an
additional unsigned "cno". "cno" is either:

  * A 1-indexed column number of the first match on the given line, or
  * 0, if the column number is irrelevant (when displaying a function
    name, context lines, etc).

We additionally modify all calls to show_line() in order to pass the new
required argument.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index 1c25782355..29bc799ecf 100644
--- a/grep.c
+++ b/grep.c
@@ -1361,7 +1361,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1501,7 +1501,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1566,7 +1566,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1830,7 +1830,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1859,7 +1859,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

[PATCH 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

In preparation of adding --column-number to 'git-grep(1)', we extend
grep_opt to take in the requisite new members.

We additionally teach the 'grep.columnnumber' and
'color.grep.columnumber' configuration variables to configure showing
and coloring the column number, respectively. (These options remain
undocumented until 'git-grep(1)' learns the --column option in a
forthcoming commit.)

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 8 ++++++++
 grep.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/grep.c b/grep.c
index 29bc799ecf..7872a5d868 100644
--- a/grep.c
+++ b/grep.c
@@ -95,6 +95,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.columnnumber")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -111,6 +115,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.columnumber"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
@@ -155,6 +161,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +171,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH 4/6] grep.c: display column number of first match

[Taylor Blau]

Building upon our work in the previous commit to add members 'columnnum'
and 'color_columno' to 'grep_opt', we teach show_line() how to respect
those options.

When requested, show_line() will display the column number of the first
match on a non-context line. show_line() differentiates between context
and non-context lines through the '&& cno' check. 'cno' will be equal to
zero if and only if show_line() is invoked on a context line. It will be
a non-zero value otherwise.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/grep.c b/grep.c
index 7872a5d868..5aeb893263 100644
--- a/grep.c
+++ b/grep.c
@@ -1404,6 +1404,12 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

This commit teaches 'git-grep(1)' a new option, '--column-number'. This
option builds upon previous commits to show the column number of the
first match on a non-context line.

For example:

  $ git grep -mn example | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Now that configuration variables such as grep.columnNumber and
color.grep.columnNumber have a visible effect, we document them in this
patch as well.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   |  5 +++++
 Documentation/git-grep.txt |  9 ++++++++-
 builtin/grep.c             |  1 +
 t/t7810-grep.sh            | 22 ++++++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..02fd4b662b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `linenumber`;;
 	line number prefix (when using `-n`)
+`columnnumber`;;
+	column number prefix (when using `-m`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable `-m` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..dd90f74ded 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] [-m | --column-number]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable `-m` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
@@ -169,6 +172,10 @@ providing this option will cause it to die.
 --line-number::
 	Prefix the line number to matching lines.
 
+-m::
+--column-number::
+	Prefix the 1-indexed column number of the first match on non-context lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..faa65abab5 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('m', "column-number", &opt.columnnum, N_("show column numbers")),
 		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,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..0cf654824d 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" '
+		{
+			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 -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep -w $L" '
+		{
+			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 -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "grep -w $L" '
 		{
 			echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Taylor Blau]

This patch adds the '--column-number' synonym '-m' to the default
grep command so that callers are brought to the correct line _and_
column of each matched location.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/git-jump | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..2706963690 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n -m"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

Re: [PATCH 5/6] builtin/grep.c: show column numbers via --column-number

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> ...
> +`columnnumber`;;
> +	column number prefix (when using `-m`)

Is there other people's tool (preferrably some variant of "grep")
that has an option to tell it to show the column number of the hit?
What is the option called there?  Does that tool let the option
squat on short-and-sweet '-m'?

Thanks.

Re: [PATCH 5/6] builtin/grep.c: show column numbers via --column-number

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Taylor Blau <me@ttaylorr.com> writes:
>
>> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
>> ...
>> +`columnnumber`;;
>> +	column number prefix (when using `-m`)
>
> Is there other people's tool (preferrably some variant of "grep")
> that has an option to tell it to show the column number of the hit?
> What is the option called there?  Does that tool let the option
> squat on short-and-sweet '-m'?
>
> Thanks.

I still do not know if we have a good existing model system to take
the longer optoin name from, but at least, GNU grep seems to use -m
to mean a completely different thing, so I'd think that we would not
assign '-m' to this new feature.

Re: [PATCH 5/6] builtin/grep.c: show column numbers via --column-number

[René Scharfe]

Am 21.04.2018 um 06:14 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>>> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
>>> ...
>>> +`columnnumber`;;
>>> +	column number prefix (when using `-m`)
>>
>> Is there other people's tool (preferrably some variant of "grep")
>> that has an option to tell it to show the column number of the hit?
>> What is the option called there?  Does that tool let the option
>> squat on short-and-sweet '-m'?
>>
>> Thanks.
> 
> I still do not know if we have a good existing model system to take
> the longer optoin name from, but at least, GNU grep seems to use -m
> to mean a completely different thing, so I'd think that we would not
> assign '-m' to this new feature.

https://beyondgrep.com/feature-comparison/ is a good resource for such
considerations.

ack and ripgrep use --column and no short option.  If git grep got
--column-number, then users could abbreviate it to --column (until it
gets some other option that makes it ambiguous).

-m seems to be used for --max-count across the board ("Stop searching
in each file after NUM matches") .

René

Re: [PATCH 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Martin Ågren]

On 21 April 2018 at 05:45, Taylor Blau <me@ttaylorr.com> wrote:
> diff --git a/grep.c b/grep.c
> index 29bc799ecf..7872a5d868 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -95,6 +95,10 @@ int grep_config(const char *var, const char *value, void *cb)
>                 opt->linenum = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp(var, "grep.columnnumber")) {
> +               opt->columnnum = git_config_bool(var, value);
> +               return 0;
> +       }
>
>         if (!strcmp(var, "grep.fullname")) {
>                 opt->relative = !git_config_bool(var, value);
> @@ -111,6 +115,8 @@ int grep_config(const char *var, const char *value, void *cb)
>                 color = opt->color_function;
>         else if (!strcmp(var, "color.grep.linenumber"))
>                 color = opt->color_lineno;
> +       else if (!strcmp(var, "color.grep.columnumber"))
> +               color = opt->color_columnno;

missing 'n' (s/umnum/umnnum/). Those characters are almost perfectly
designed for hiding precisely this mistake. ;-)

Re: [PATCH 5/6] builtin/grep.c: show column numbers via --column-number

[Martin Ågren]

On 21 April 2018 at 05:45, Taylor Blau <me@ttaylorr.com> wrote:
> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> option builds upon previous commits to show the column number of the
> first match on a non-context line.
>
> For example:
>
>   $ git grep -mn example | head -n3
>   .clang-format:51:14:# myFunction(foo, bar, baz);
>   .clang-format:64:7:# int foo();
>   .clang-format:75:8:# void foo()

"example" vs "foo" :-)

> Now that configuration variables such as grep.columnNumber and
> color.grep.columnNumber have a visible effect, we document them in this
> patch as well.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---

One thing I've noted is that your messages are light on the imperative
"do this", preferring "this commit does" or "we do". That said, I found
myself following along quite well in what they're saying.

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1159,6 +1159,8 @@ color.grep.<slot>::
>         function name lines (when using `-p`)
>  `linenumber`;;
>         line number prefix (when using `-n`)
> +`columnnumber`;;
> +       column number prefix (when using `-m`)
>  `match`;;
>         matching text (same as setting `matchContext` and `matchSelected`)
>  `matchContext`;;
> @@ -1708,6 +1710,9 @@ gitweb.snapshot::
>  grep.lineNumber::
>         If set to true, enable `-n` option by default.
>
> +grep.columnNumber::
> +       If set to true, enable `-m` option by default.
> +
>  grep.patternType::
>         Set the default matching behavior. Using a value of 'basic', 'extended',
>         'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,

You're doing what the immediate neighbours are doing, but the end result
is a bit inconsistent: columnnumber vs columnNumber. I think the ideal
end-game is columnNumber, lineNumber, and so on.

Maybe use "columnNumber" consistently since you're introducing it and
want it to be perfect from the start ;-) and if that leaves "linenumber"
looking too inconsistent, then change it while at it? (I'm not suggesting
changing all foobar to fooBar while at it...)

Martin

[PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is a re-roll of the series to add --column-number to
'git-grep(1)'.

Since last time, I have changed the following (an inter-diff is
available below for easier consumption):

  * Removed '-m' as an alias for '--column-number', per René's
    suggestion [1].

  * Fix some incorrect spelling of 'columnnumber'.

  * Change casing of 'color.grep.linenumber' to 'color.grep.lineNumber'
    to be consistent with 'color.grep.columnNumber'. This is an
    unrelated change, and one which I am happy to drop from this series.
    It was suggested by Martin in [2].

Thanks in advance for your second round of review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/cef29224-718f-21e9-0242-8bcd8e9c20a6@web.de/
[2]: https://public-inbox.org/git/CAN0heSp_bGqKF26g4TDOw6WpsvR2cEW6EqF3aJtKCv5POU_HmQ@mail.gmail.com/

Taylor Blau (6):
  grep.c: take regmatch_t as argument in match_line()
  grep.c: take column number as argument to show_line()
  grep.[ch]: teach columnnum, color_columnno to grep_opt
  grep.c: display column number of first match
  builtin/grep.c: show column numbers via --column-number
  contrib/git-jump/git-jump: use column number when grep-ing

 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt |  8 +++++++-
 builtin/grep.c             |  1 +
 contrib/git-jump/git-jump  |  2 +-
 grep.c                     | 33 ++++++++++++++++++++++++---------
 grep.h                     |  2 ++
 t/t7810-grep.sh            | 22 ++++++++++++++++++++++
 7 files changed, 63 insertions(+), 12 deletions(-)

Inter-diff (since v1):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 02fd4b662b..1645fcf2ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,10 +1157,10 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
-`columnnumber`;;
-	column number prefix (when using `-m`)
+`columnNumber`;;
+	column number prefix (when using `--column-number`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index dd90f74ded..b75a039768 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] [-m | --column-number]
+	   [-F | --fixed-strings] [-n | --line-number] [--column-number]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
@@ -172,7 +172,6 @@ providing this option will cause it to die.
 --line-number::
 	Prefix the line number to matching lines.

--m::
 --column-number::
 	Prefix the 1-indexed column number of the first match on non-context lines.

diff --git a/builtin/grep.c b/builtin/grep.c
index faa65abab5..23ce97f998 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +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('m', "column-number", &opt.columnnum, N_("show column numbers")),
+		OPT_BOOL(0, "column-number", &opt.columnnum, N_("show column numbers")),
 		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,
diff --git a/grep.c b/grep.c
index 5aeb893263..23250e60d0 100644
--- a/grep.c
+++ b/grep.c
@@ -115,7 +115,7 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
-	else if (!strcmp(var, "color.grep.columnumber"))
+	else if (!strcmp(var, "color.grep.columnnumber"))
 		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 0cf654824d..7349c7fadc 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -106,7 +106,7 @@ do
 			echo ${HC}file:5:foo mmap bar_mmap
 			echo ${HC}file:14:foo_mmap bar mmap baz
 		} >expected &&
-		git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
+		git grep --column-number -w -e mmap $H >actual &&
 		test_cmp expected actual
 	'

@@ -117,7 +117,7 @@ do
 			echo ${HC}file:4:5:foo mmap bar_mmap
 			echo ${HC}file:5:14:foo_mmap bar mmap baz
 		} >expected &&
-		git -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
+		git grep -n --column-number -w -e mmap $H >actual &&
 		test_cmp expected actual
 	'

--
2.17.0

[PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Taylor Blau]

In a subsequent patch, we teach show_line() to optionally include the
column number of the first match on each matched line.

The regmatch_t involved in match_line() and match_one_pattern() both
contain this information (via regmatch_t->rm_so), but their current
implementation throws this stack variable away at the end of the call.

Instead, let's teach match_line() to take in a 'regmatch_t *' so that
callers can inspect the result of their calls. This will prove useful in
a subsequent commit when a caller will forward on information from the
regmatch_t into show_line (as described above).

The return condition remains unchanged, therefore the only change
required of callers is the addition of a single argument.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

show_line() currently receives the line number within the
'grep_opt->buf' in order to determine which line number to display. In
order to display information about the matching column number--if
requested--we must additionally take in that information.

To do so, we extend the signature of show_line() to take in an
additional unsigned "cno". "cno" is either:

  * A 1-indexed column number of the first match on the given line, or
  * 0, if the column number is irrelevant (when displaying a function
    name, context lines, etc).

We additionally modify all calls to show_line() in order to pass the new
required argument.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index 1c25782355..29bc799ecf 100644
--- a/grep.c
+++ b/grep.c
@@ -1361,7 +1361,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1501,7 +1501,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1566,7 +1566,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1830,7 +1830,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1859,7 +1859,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

[PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

In preparation of adding --column-number to 'git-grep(1)', we extend
grep_opt to take in the requisite new members.

We additionally teach the 'grep.columnnumber' and
'color.grep.columnnumber' configuration variables to configure showing
and coloring the column number, respectively. (These options remain
undocumented until 'git-grep(1)' learns the --column option in a
forthcoming commit.)

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 8 ++++++++
 grep.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/grep.c b/grep.c
index 29bc799ecf..922ab92eff 100644
--- a/grep.c
+++ b/grep.c
@@ -95,6 +95,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.columnnumber")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -111,6 +115,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.columnnumber"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
@@ -155,6 +161,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +171,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v2 4/6] grep.c: display column number of first match

[Taylor Blau]

Building upon our work in the previous commit to add members 'columnnum'
and 'color_columno' to 'grep_opt', we teach show_line() how to respect
those options.

When requested, show_line() will display the column number of the first
match on a non-context line. show_line() differentiates between context
and non-context lines through the '&& cno' check. 'cno' will be equal to
zero if and only if show_line() is invoked on a context line. It will be
a non-zero value otherwise.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/grep.c b/grep.c
index 922ab92eff..23250e60d0 100644
--- a/grep.c
+++ b/grep.c
@@ -1404,6 +1404,12 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

This commit teaches 'git-grep(1)' a new option, '--column-number'. This
option builds upon previous commits to show the column number of the
first match on a non-context line.

For example:

  $ ./git-grep -n --column-number foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Now that configuration variables such as grep.columnNumber and
color.grep.columnNumber have a visible effect, we document them in this
patch as well.

While we're at it, change color.grep.linenumber to color.grep.lineNumber
to match the casing of nearby variables.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt |  8 +++++++-
 builtin/grep.c             |  1 +
 t/t7810-grep.sh            | 22 ++++++++++++++++++++++
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..1645fcf2ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,8 +1157,10 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
+`columnNumber`;;
+	column number prefix (when using `--column-number`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable `-m` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..b75a039768 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-number]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable `-m` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
@@ -169,6 +172,9 @@ providing this option will cause it to die.
 --line-number::
 	Prefix the line number to matching lines.
 
+--column-number::
+	Prefix the 1-indexed column number of the first match on non-context lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..23ce97f998 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-number", &opt.columnnum, N_("show column numbers")),
 		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,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..7349c7fadc 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" '
+		{
+			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-number -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep -w $L" '
+		{
+			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-number -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "grep -w $L" '
 		{
 			echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Taylor Blau]

This patch adds the '--column-number' synonym '-m' to the default
grep command so that callers are brought to the correct line _and_
column of each matched location.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/git-jump | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..2706963690 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n -m"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

I think [345]/6 would make much more sense as just one patch. Comments
on them to follow...

> In preparation of adding --column-number to 'git-grep(1)', we extend
> grep_opt to take in the requisite new members.

Just a nit: Makes sense to refer to these camel-cased in docs & commit
messages.

> diff --git a/grep.c b/grep.c
> [...]

All of the boilerplate looks fine.

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

I think this part though...

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
> [...]
> -`linenumber`;;
> +`lineNumber`;;

Makes sense as its own patch at the beginning of the series, since it's
just related cleanup.

> +`columnNumber`;;
> +	column number prefix (when using `--column-number`)

Here you're using --column-number...

> +grep.columnNumber::
> +	If set to true, enable `-m` option by default.

...But not here. This needs to be updated

> +grep.columnNumber::
> +	If set to true, enable `-m` option by default.
> +

...ditto.

> +--column-number::
> +	Prefix the 1-indexed column number of the first match on non-context lines.
> +
> [...]
>  		OPT_GROUP(""),
>  		OPT_BOOL('n', "line-number", &opt.linenum, N_("show line numbers")),
> +		OPT_BOOL(0, "column-number", &opt.columnnum, N_("show column numbers")),

Maybe "show first matching column"? I.e. the main docs say "just shows
the first", but this seems to give a different impression.

It would also be nice if the docs briefly explained what this is for,
i.e. the git-jump use-case.

Re: [PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

> This patch adds the '--column-number' synonym '-m' to the default
> grep command so that callers are brought to the correct line _and_
> column of each matched location.
> [...]
> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> index 80ab0590bc..2706963690 100755
> --- a/contrib/git-jump/git-jump
> +++ b/contrib/git-jump/git-jump
> @@ -52,7 +52,7 @@ mode_merge() {
>  # editor shows them to us in the status bar.
>  mode_grep() {
>  	cmd=$(git config jump.grepCmd)
> -	test -n "$cmd" || cmd="git grep -n"
> +	test -n "$cmd" || cmd="git grep -n -m"
>  	$cmd "$@" |
>  	perl -pe '
>  	s/[ \t]+/ /g;

So this re-roll doesn't have the alias -m anymore, but this makes use of
it. Seems you just forgot to update this from v1, unless I'm missing
something while skimming this...

Re: [PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> In a subsequent patch, we teach show_line() to optionally include the
> column number of the first match on each matched line.
>
> The regmatch_t involved in match_line() and match_one_pattern() both
> contain this information (via regmatch_t->rm_so), but their current
> implementation throws this stack variable away at the end of the call.
>
> Instead, let's teach match_line() to take in a 'regmatch_t *' so that
> callers can inspect the result of their calls. This will prove useful in
> a subsequent commit when a caller will forward on information from the
> regmatch_t into show_line (as described above).
>
> The return condition remains unchanged, therefore the only change
> required of callers is the addition of a single argument.

Is 'rm_so' the only piece of information which callers will ever want
to extract from 'regmatch_t'? If so, this patch's approach might be
overly broad, unnecessarily exposing too much of match_lines()'s
internal implementation. An alternative would be to narrow the
interface and limit exposure by passing in an 'int *matched_col' or
some such.

(Not necessarily a big deal, but something to consider.)

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/grep.c b/grep.c
> @@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
>  static int match_line(struct grep_opt *opt, char *bol, char *eol,
> -                     enum grep_context ctx, int collect_hits)
> +                     regmatch_t *match, enum grep_context ctx,
> +                     int collect_hits)
>  {
>         struct grep_pat *p;
> -       regmatch_t match;
>
>         if (opt->extended)
>                 return match_expr(opt, bol, eol, ctx, collect_hits);

The new 'match' argument has no impact in the 'opt->extended' case.
Perhaps this deserves calling out in the commit message.

>         /* we do not call with collect_hits without being extended */
>         for (p = opt->pattern_list; p; p = p->next) {
> -               if (match_one_pattern(p, bol, eol, ctx, &match, 0))
> +               if (match_one_pattern(p, bol, eol, ctx, match, 0))
>                         return 1;
>         }
>         return 0;

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> > In preparation of adding --column-number to 'git-grep(1)', we extend
> > grep_opt to take in the requisite new members.
>
> Just a nit: Makes sense to refer to these camel-cased in docs & commit
> messages.

Could you clarify which? I am not sure if you mean --column-number,
'git-grep(1)', or grep_opt.


Thanks,
Taylor

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:48:53PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> I think this part though...
>
> > While we're at it, change color.grep.linenumber to color.grep.lineNumber
> > to match the casing of nearby variables.
> > [...]
> > -`linenumber`;;
> > +`lineNumber`;;
>
> Makes sense as its own patch at the beginning of the series, since it's
> just related cleanup.

Thanks, I have adjusted this change in my copy and will attach it in a
subsequent re-roll.

> > +`columnNumber`;;
> > +	column number prefix (when using `--column-number`)
>
> Here you're using --column-number...
>
> > +grep.columnNumber::
> > +	If set to true, enable `-m` option by default.
>
> ...But not here. This needs to be updated
>
> > +grep.columnNumber::
> > +	If set to true, enable `-m` option by default.
> > +
>
> ...ditto.

Fixed all of these, thanks for pointing them out :-).

> > +--column-number::
> > +	Prefix the 1-indexed column number of the first match on non-context lines.
> > +
> > [...]
> >  		OPT_GROUP(""),
> >  		OPT_BOOL('n', "line-number", &opt.linenum, N_("show line numbers")),
> > +		OPT_BOOL(0, "column-number", &opt.columnnum, N_("show column numbers")),
>
> Maybe "show first matching column"? I.e. the main docs say "just shows
> the first", but this seems to give a different impression.

I settled on "show column number of first match", and have noted its use
for callers like git-jump in the documentation.

Thanks,
Taylor

Re: [PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:49:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> > This patch adds the '--column-number' synonym '-m' to the default
> > grep command so that callers are brought to the correct line _and_
> > column of each matched location.
> > [...]
> > diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> > index 80ab0590bc..2706963690 100755
> > --- a/contrib/git-jump/git-jump
> > +++ b/contrib/git-jump/git-jump
> > @@ -52,7 +52,7 @@ mode_merge() {
> >  # editor shows them to us in the status bar.
> >  mode_grep() {
> >  	cmd=$(git config jump.grepCmd)
> > -	test -n "$cmd" || cmd="git grep -n"
> > +	test -n "$cmd" || cmd="git grep -n -m"
> >  	$cmd "$@" |
> >  	perl -pe '
> >  	s/[ \t]+/ /g;
>
> So this re-roll doesn't have the alias -m anymore, but this makes use of
> it. Seems you just forgot to update this from v1, unless I'm missing
> something while skimming this...

Ack; another good catch. I have updated this in my copy and will include
it in v3.

Thanks,
Taylor

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>   * Removed '-m' as an alias for '--column-number', per René's
>     suggestion [1].
>
>   * Fix some incorrect spelling of 'columnnumber'.
>
>   * Change casing of 'color.grep.linenumber' to 'color.grep.lineNumber'
>     to be consistent with 'color.grep.columnNumber'. This is an
>     unrelated change, and one which I am happy to drop from this series.
>     It was suggested by Martin in [2].

All sounds like good updates.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 0cf654824d..7349c7fadc 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -106,7 +106,7 @@ do
>  			echo ${HC}file:5:foo mmap bar_mmap
>  			echo ${HC}file:14:foo_mmap bar mmap baz
>  		} >expected &&
> -		git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
> +		git grep --column-number -w -e mmap $H >actual &&
>  		test_cmp expected actual
>  	'
>
> @@ -117,7 +117,7 @@ do
>  			echo ${HC}file:4:5:foo mmap bar_mmap
>  			echo ${HC}file:5:14:foo_mmap bar mmap baz
>  		} >expected &&
> -		git -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
> +		git grep -n --column-number -w -e mmap $H >actual &&
>  		test_cmp expected actual
>  	'

It seems that these two used to be "even when it is configured not
to show linenumber, with -n it is shown and without -n it is not,
when the new --column-number feature forces the command to show the
"filename plus colon plus location info plus coon" header.  I'm
guessing that the reason why these got changed this round is because
the old way was found too defensive (perhaps nobody sets
grep.linenumber in .git/config in the tests that come before these)?

Re: [PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Taylor Blau]

On Sun, Apr 22, 2018 at 07:14:58PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > In a subsequent patch, we teach show_line() to optionally include the
> > column number of the first match on each matched line.
> >
> > The regmatch_t involved in match_line() and match_one_pattern() both
> > contain this information (via regmatch_t->rm_so), but their current
> > implementation throws this stack variable away at the end of the call.
> >
> > Instead, let's teach match_line() to take in a 'regmatch_t *' so that
> > callers can inspect the result of their calls. This will prove useful in
> > a subsequent commit when a caller will forward on information from the
> > regmatch_t into show_line (as described above).
> >
> > The return condition remains unchanged, therefore the only change
> > required of callers is the addition of a single argument.
>
> Is 'rm_so' the only piece of information which callers will ever want
> to extract from 'regmatch_t'? If so, this patch's approach might be
> overly broad, unnecessarily exposing too much of match_lines()'s
> internal implementation. An alternative would be to narrow the
> interface and limit exposure by passing in an 'int *matched_col' or
> some such.
>
> (Not necessarily a big deal, but something to consider.)

Thanks; I do not think that this is overly broad, although I suppose
that it does bind us to the implementation of regmatch_t. The definition
of regmatch_t, however, is fairly minimal:

	typedef struct
	{
		regoff_t rm_so;  /* Byte offset from string's start to substring's start.  */
		regoff_t rm_eo;  /* Byte offset from string's start to substring's end.  */
	} regmatch_t;

I can imagine that other callers would be interested in the other piece
of information, 'rm_eo', as well. Therefore, I think it makes sense to
provide access to it along with 'rm_so'.

I think that if we are concerned with being bound to regmatch_t, then we
could typedef a new struct that contains a 'start' and 'end' unsigned,
but I think that this extra effort would not be fruitful, since the
majority of grep.c assumes regmatch_t (and friends).

> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/grep.c b/grep.c
> > @@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
> >  static int match_line(struct grep_opt *opt, char *bol, char *eol,
> > -                     enum grep_context ctx, int collect_hits)
> > +                     regmatch_t *match, enum grep_context ctx,
> > +                     int collect_hits)
> >  {
> >         struct grep_pat *p;
> > -       regmatch_t match;
> >
> >         if (opt->extended)
> >                 return match_expr(opt, bol, eol, ctx, collect_hits);
>
> The new 'match' argument has no impact in the 'opt->extended' case.
> Perhaps this deserves calling out in the commit message.

I have noted this in the commit message, which I'll include in the next
re-roll.

Thanks,
Taylor

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Taylor Blau]

On Mon, Apr 23, 2018 at 08:28:14AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > index 0cf654824d..7349c7fadc 100755
> > --- a/t/t7810-grep.sh
> > +++ b/t/t7810-grep.sh
> > @@ -106,7 +106,7 @@ do
> >  			echo ${HC}file:5:foo mmap bar_mmap
> >  			echo ${HC}file:14:foo_mmap bar mmap baz
> >  		} >expected &&
> > -		git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
> > +		git grep --column-number -w -e mmap $H >actual &&
> >  		test_cmp expected actual
> >  	'
> >
> > @@ -117,7 +117,7 @@ do
> >  			echo ${HC}file:4:5:foo mmap bar_mmap
> >  			echo ${HC}file:5:14:foo_mmap bar mmap baz
> >  		} >expected &&
> > -		git -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
> > +		git grep -n --column-number -w -e mmap $H >actual &&
> >  		test_cmp expected actual
> >  	'
>
> It seems that these two used to be "even when it is configured not
> to show linenumber, with -n it is shown and without -n it is not,
> when the new --column-number feature forces the command to show the
> "filename plus colon plus location info plus coon" header.  I'm
> guessing that the reason why these got changed this round is because
> the old way was found too defensive (perhaps nobody sets
> grep.linenumber in .git/config in the tests that come before these)?

Sort of. My aim with these new tests is to ensure that git-grep(1) works
with:

  - '--line-number'
  - '--column-number'
  - '--line-number' and '--column-number' together

It seemed unrelated to be testing the various permutations of
grep.linenumber and --line-number along with the above three so I
removed these in order to focus only on the above three.

Do you think that it is worth testing --column-number with and without
grep.columnnumber as above?


Thanks,
Taylor

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> show_line() currently receives the line number within the
> 'grep_opt->buf' in order to determine which line number to display. In
> order to display information about the matching column number--if
> requested--we must additionally take in that information.
>
> To do so, we extend the signature of show_line() to take in an
> additional unsigned "cno". "cno" is either:
>
>   * A 1-indexed column number of the first match on the given line, or
>   * 0, if the column number is irrelevant (when displaying a function
>     name, context lines, etc).

This information about how 'cno' is interpreted seems important enough
to have as an in-code comment somewhere. Unfortunately, this patch
never actually uses 'cno', so it's hard to add such a comment to
non-existent code. In fact, the granularity of this patch feels wrong;
it seems to exist for some purpose but, at the same time, is a
do-nothing patch.

This issue illustrates a larger problem with how this patch series is
structured overall. In his review, Ævar suggested collapsing several
patches into one, but the problems go deeper than that when you have
patches which implement some bit of functionality but don't document
that functionality until some later step which exposes some other bit
of functionality, and so forth. As a reviewer, I expect a patch series
to hold my hand and lead me on a straightforward journey from building
blocks to final product, but this series tends to jump around without
apparent logic.

One way to achieve a more coherent patch series would be to build the
machinery first and then expose it to the user in various ways. Also,
each patch which implements some user-facing functionality should also
document that functionality. For instance, a more understandable
series might be structured something like this:

  1. grep: match_line: expose matched column
  2. grep: extend grep_opt to allow showing matched column
  3. grep: display column number of first match
  4. builtin/grep: add --column-number option
  5. grep: add configuration variables to show matched column
  6. contrib/git-jump: jump to match column in addition to line

There may be fewer or more patches than shown here (I believe Ævar
suggested a cleanup patch), but this should give the general idea.
Patches 4 and 5 might also be swapped if that seems more logical.

(Sorry if any of the above sounds harsh; it's not meant to be, but is
intended to be constructive.)

> We additionally modify all calls to show_line() in order to pass the new
> required argument.

Nit: No need to state the obvious; this final sentence could easily be dropped.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Eric Sunshine]

On Sun, Apr 22, 2018 at 7:24 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ęvar Arnfjörš Bjarmason wrote:
>> On Sun, Apr 22 2018, Taylor Blau wrote:
>> > In preparation of adding --column-number to 'git-grep(1)', we extend
>> > grep_opt to take in the requisite new members.
>>
>> Just a nit: Makes sense to refer to these camel-cased in docs & commit
>> messages.
>
> Could you clarify which? I am not sure if you mean --column-number,
> 'git-grep(1)', or grep_opt.

I think Ævar was referring to this bit from the commit message (which
unfortunately got snipped in his review):

    We additionally teach the 'grep.columnnumber' and
    'color.grep.columnnumber' configuration variables to configure
    showing and coloring the column number, respectively. (These
    options remain undocumented until 'git-grep(1)' learns the
    --column option in a forthcoming commit.)

He was suggesting camel-casing 'grep.columnNumber' and
'color.grep.columNumber' even in the commit message.

Re: [PATCH v2 4/6] grep.c: display column number of first match

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> Building upon our work in the previous commit to add members 'columnnum'
> and 'color_columno' to 'grep_opt', we teach show_line() how to respect
> those options.
>
> When requested, show_line() will display the column number of the first
> match on a non-context line. show_line() differentiates between context
> and non-context lines through the '&& cno' check. 'cno' will be equal to
> zero if and only if show_line() is invoked on a context line. It will be
> a non-zero value otherwise.

This interpretation of 'cno' seems important enough to deserve an
in-code comment. (And, you may be able to drop some of the same
information from the commit message if it merely duplicates the
in-code comment.)

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/grep.c b/grep.c
> @@ -1404,6 +1404,12 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
> +       if (opt->columnnum && cno) {
> +               char buf[32];
> +               xsnprintf(buf, sizeof(buf), "%d", cno);
> +               output_color(opt, buf, strlen(buf), opt->color_columnno);
> +               output_sep(opt, sign);
> +       }

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> option builds upon previous commits to show the column number of the
> first match on a non-context line.

Imperative mood (and dropping unnecessary "builds upon previous"):

    Teach 'git-grep(1)' a new option '--column-number' which shows the
    column number of the first match on a non-context line.

> For example:
>
>   $ ./git-grep -n --column-number foo | head -n3
>   .clang-format:51:14:# myFunction(foo, bar, baz);
>   .clang-format:64:7:# int foo();
>   .clang-format:75:8:# void foo()
>
> Now that configuration variables such as grep.columnNumber and
> color.grep.columnNumber have a visible effect, we document them in this
> patch as well.

As mentioned in my review of patch 2, document the configuration
variables in the patch which introduces them.

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
> +       test_expect_success "grep -w $L" '
> +               ...
> +       '
> +
> +       test_expect_success "grep -w $L" '
> +               ...
> +       '
> +
>         test_expect_success "grep -w $L" '

I realize that several existing tests in this script are already
guilty of this sin, but please give each new test a distinct title
reflective of what it is actually testing in order to make it easier
to correlate failed test output with the actual test code.

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:21:33PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 7:24 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ęvar Arnfjörš Bjarmason wrote:
> >> On Sun, Apr 22 2018, Taylor Blau wrote:
> >> > In preparation of adding --column-number to 'git-grep(1)', we extend
> >> > grep_opt to take in the requisite new members.
> >>
> >> Just a nit: Makes sense to refer to these camel-cased in docs & commit
> >> messages.
> >
> > Could you clarify which? I am not sure if you mean --column-number,
> > 'git-grep(1)', or grep_opt.
>
> I think Ævar was referring to this bit from the commit message (which
> unfortunately got snipped in his review):
>
>     We additionally teach the 'grep.columnnumber' and
>     'color.grep.columnnumber' configuration variables to configure
>     showing and coloring the column number, respectively. (These
>     options remain undocumented until 'git-grep(1)' learns the
>     --column option in a forthcoming commit.)
>
> He was suggesting camel-casing 'grep.columnNumber' and
> 'color.grep.columNumber' even in the commit message.

Thanks for the clarification, Eric. I have amended my copy of this
patch to correctly case those variables.

Thanks,
Taylor

Re: [PATCH v2 4/6] grep.c: display column number of first match

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:24:55PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > Building upon our work in the previous commit to add members 'columnnum'
> > and 'color_columno' to 'grep_opt', we teach show_line() how to respect
> > those options.
> >
> > When requested, show_line() will display the column number of the first
> > match on a non-context line. show_line() differentiates between context
> > and non-context lines through the '&& cno' check. 'cno' will be equal to
> > zero if and only if show_line() is invoked on a context line. It will be
> > a non-zero value otherwise.
>
> This interpretation of 'cno' seems important enough to deserve an
> in-code comment. (And, you may be able to drop some of the same
> information from the commit message if it merely duplicates the
> in-code comment.)

Thanks, I agree that the interpretation of 'cno' is subtle and should
not require a 'git blame' to understand. I have adopted your proposed
commit structure [1] and moved this description from the patch message
into grep.c as a comment above the relevant conditional.

Thanks,
Taylor

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:32:28PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> > option builds upon previous commits to show the column number of the
> > first match on a non-context line.
>
> Imperative mood (and dropping unnecessary "builds upon previous"):
>
>     Teach 'git-grep(1)' a new option '--column-number' which shows the
>     column number of the first match on a non-context line.

Thanks. I am not used to writing in this mood, but have amended my
patches locally to conform to your proposed layout and have reworded
each to be in the imperative mood.

> > For example:
> >
> >   $ ./git-grep -n --column-number foo | head -n3
> >   .clang-format:51:14:# myFunction(foo, bar, baz);
> >   .clang-format:64:7:# int foo();
> >   .clang-format:75:8:# void foo()
> >
> > Now that configuration variables such as grep.columnNumber and
> > color.grep.columnNumber have a visible effect, we document them in this
> > patch as well.
>
> As mentioned in my review of patch 2, document the configuration
> variables in the patch which introduces them.

Thanks again, I have moved this introduction to the relevant patch.

> > While we're at it, change color.grep.linenumber to color.grep.lineNumber
> > to match the casing of nearby variables.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > @@ -99,6 +99,28 @@ do
> > +       test_expect_success "grep -w $L" '
> > +               ...
> > +       '
> > +
> > +       test_expect_success "grep -w $L" '
> > +               ...
> > +       '
> > +
> >         test_expect_success "grep -w $L" '
>
> I realize that several existing tests in this script are already
> guilty of this sin, but please give each new test a distinct title
> reflective of what it is actually testing in order to make it easier
> to correlate failed test output with the actual test code.

:-). I have changed this locally to indicate which is which in the hopes
that it will provide more clarity should these tests fail at any point.


Thanks,
Taylor

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > show_line() currently receives the line number within the
> > 'grep_opt->buf' in order to determine which line number to display. In
> > order to display information about the matching column number--if
> > requested--we must additionally take in that information.
> >
> > To do so, we extend the signature of show_line() to take in an
> > additional unsigned "cno". "cno" is either:
> >
> >   * A 1-indexed column number of the first match on the given line, or
> >   * 0, if the column number is irrelevant (when displaying a function
> >     name, context lines, etc).
>
> This information about how 'cno' is interpreted seems important enough
> to have as an in-code comment somewhere. Unfortunately, this patch
> never actually uses 'cno', so it's hard to add such a comment to
> non-existent code. In fact, the granularity of this patch feels wrong;
> it seems to exist for some purpose but, at the same time, is a
> do-nothing patch.
>
> This issue illustrates a larger problem with how this patch series is
> structured overall. In his review, Ævar suggested collapsing several
> patches into one, but the problems go deeper than that when you have
> patches which implement some bit of functionality but don't document
> that functionality until some later step which exposes some other bit
> of functionality, and so forth. As a reviewer, I expect a patch series
> to hold my hand and lead me on a straightforward journey from building
> blocks to final product, but this series tends to jump around without
> apparent logic.
>
> One way to achieve a more coherent patch series would be to build the
> machinery first and then expose it to the user in various ways. Also,
> each patch which implements some user-facing functionality should also
> document that functionality. For instance, a more understandable
> series might be structured something like this:
>
>   1. grep: match_line: expose matched column
>   2. grep: extend grep_opt to allow showing matched column
>   3. grep: display column number of first match
>   4. builtin/grep: add --column-number option
>   5. grep: add configuration variables to show matched column
>   6. contrib/git-jump: jump to match column in addition to line
>
> There may be fewer or more patches than shown here (I believe Ævar
> suggested a cleanup patch), but this should give the general idea.
> Patches 4 and 5 might also be swapped if that seems more logical.
>
> (Sorry if any of the above sounds harsh; it's not meant to be, but is
> intended to be constructive.)

I appreciate the suggestion, and did not take it as harsh. I think that
the existing structure is OK, but I think that I am biased since I'm the
author of the series. I have given your proposed structure a shot and I
think that it should improve the readability of this series for others
on the list as well.

To avoid sending more email than is necessary, I have pushed a draft of
this series to my copy of Git, and would greatly appreciate your
feedback on whether the structure of these patches is sensible. If they
all seem OK to you, I'll happily push v3.

For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

> > We additionally modify all calls to show_line() in order to pass the new
> > required argument.
>
> Nit: No need to state the obvious; this final sentence could easily be dropped.

Fair; I have removed this from the patch.


Thanks,
Taylor

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 9:17 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote:
>> One way to achieve a more coherent patch series would be to build the
>> machinery first and then expose it to the user in various ways. Also,
>> each patch which implements some user-facing functionality should also
>> document that functionality. For instance, a more understandable
>> series might be structured something like this:
>>
>>   1. grep: match_line: expose matched column
>>   2. grep: extend grep_opt to allow showing matched column
>>   3. grep: display column number of first match
>>   4. builtin/grep: add --column-number option
>>   5. grep: add configuration variables to show matched column
>>   6. contrib/git-jump: jump to match column in addition to line
>
> I appreciate the suggestion, and did not take it as harsh. I think that
> the existing structure is OK, but I think that I am biased since I'm the
> author of the series. I have given your proposed structure a shot and I
> think that it should improve the readability of this series for others
> on the list as well.
>
> To avoid sending more email than is necessary, I have pushed a draft of
> this series to my copy of Git, and would greatly appreciate your
> feedback on whether the structure of these patches is sensible. If they
> all seem OK to you, I'll happily push v3.
>
> For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

Thanks. Perhaps not surprisingly, I find the restructured patch series
easier to follow and review. Each patch has a very specific purpose,
thus demands lower review-time cognitive load than with the old
structure.

One important issue I noticed is that patch 3/7 neglects to update
grep.c:init_grep_defaults() to initialize opt.color_columnno.

Looking at the tests again, are you gaining anything by placing them
inside that for-loop? (Genuine question.)

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 23 2018, Eric Sunshine wrote:

> On Sun, Apr 22, 2018 at 9:17 PM, Taylor Blau <me@ttaylorr.com> wrote:
> One important issue I noticed is that patch 3/7 neglects to update
> grep.c:init_grep_defaults() to initialize opt.color_columnno.

I think this is fine for fields that are 0 by default, since the struct
 is already zero'd out. See my e62ba43244 ("grep: remove redundant
 double assignment to 0", 2017-06-29) for some prior art.

> Looking at the tests again, are you gaining anything by placing them
> inside that for-loop? (Genuine question.)

The tests in that loop are just to stress-test grep with/without a
working tree. Even though we can see from the implementation that it's
the same in both cases here, I think it makes sense to add new stuff to
that loop by default to stress that case.

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Mon, Apr 23, 2018 at 3:27 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Apr 23 2018, Eric Sunshine wrote:
>> One important issue I noticed is that patch 3/7 neglects to update
>> grep.c:init_grep_defaults() to initialize opt.color_columnno.
>
> I think this is fine for fields that are 0 by default, since the struct
>  is already zero'd out. See my e62ba43244 ("grep: remove redundant
>  double assignment to 0", 2017-06-29) for some prior art.

Indeed, I wasn't worried about opt.columnnum, which is fine being
zero'd out by the memset(). What I was concerned about was
opt.color_columnno; the corresponding opt.color_lineno is handled
explicitly in init_grep_defaults():

    color_set(opt->color_lineno, "");

I'd expect opt.color_columnno to be treated likewise.

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 23 2018, Taylor Blau wrote:

> For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

Looks good to me aside from two minor issues I noticed:

 * In "grep.c: display column number of first match" you use a comment
   style we don't normally use, i.e. /**\n not /*\n. See "Multi-line
   comments" in Documentation/CodingGuidelines.

 * You're not updating contrib/git-jump/README to document the new
   output format. It just refers to the old format, but now depending on
   if you use "grep" or not it'll use this new thing. It also makes
   sense to update the example added in 007d06aa57 ("contrib/git-jump:
   allow to configure the grep command", 2017-11-20) which seems to have
   added jump.grepCmd as a workaround for not having this.

But also, after just looking at this the second time around; Is there a
reason we shouldn't just call this --column, not --column-number? I
realize the former works because of the lazyness of our getopt parsing
(also --colu though..).

I think when we add features to git-grep we should be as close to GNU
grep as possible (e.g. not add this -m alias meaning something different
as in your v1), but if GNU grep doesn't have something go with the trend
of other grep tools, as noted at
https://beyondgrep.com/feature-comparison/ (and I found another one that
has this: https://github.com/beyondgrep/website/pull/83), so there's
already 3 prominent grep tools that call this just --column.

I think we should just go with that.

Also, as a bonus question, since you're poking at this column code
anyway, interested in implementing -o (--only-matching)? That would be
super-useful (see
https://public-inbox.org/git/87in9ucsbb.fsf@evledraar.gmail.com/) :)

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> It seems that these two used to be "even when it is configured not
>> to show linenumber, with -n it is shown and without -n it is not,
>> when the new --column-number feature forces the command to show the
>> "filename plus colon plus location info plus coon" header.  I'm
>> guessing that the reason why these got changed this round is because
>> the old way was found too defensive (perhaps nobody sets
>> grep.linenumber in .git/config in the tests that come before these)?
> ...
> Do you think that it is worth testing --column-number with and without
> grep.columnnumber as above?

I view the "git -c var.val=foo cmd" as a reasonable way to make sure
that the test is not affected by any stale state in .git/config left
behind by an earlier test that did "git config var.val bar" and then
failed to clean itself up, but I do not think it is all that
intereseting to test the inter-changeability between config and
command line option "-n" while checking its interaction with another
option e.g. "--column".

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

On Mon, Apr 23, 2018 at 03:34:21AM -0400, Eric Sunshine wrote:
> On Mon, Apr 23, 2018 at 3:27 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Mon, Apr 23 2018, Eric Sunshine wrote:
> >> One important issue I noticed is that patch 3/7 neglects to update
> >> grep.c:init_grep_defaults() to initialize opt.color_columnno.
> >
> > I think this is fine for fields that are 0 by default, since the struct
> >  is already zero'd out. See my e62ba43244 ("grep: remove redundant
> >  double assignment to 0", 2017-06-29) for some prior art.
>
> Indeed, I wasn't worried about opt.columnnum, which is fine being
> zero'd out by the memset(). What I was concerned about was
> opt.color_columnno; the corresponding opt.color_lineno is handled
> explicitly in init_grep_defaults():
>
>     color_set(opt->color_lineno, "");
>
> I'd expect opt.color_columnno to be treated likewise.

I agree with Ævar and Eric, we should certainly zero-out
opt->color_lineno in grep.c's init_grep_defaults().

I recall doing this in v1, but I think that I must have dropped this
part of the patch on the floor. In either case, I have amended my local
copy to include this color_set() invocation and will include it in v3
(which I hope to send later this evening).


Thanks,
Taylor

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

On Mon, Apr 23, 2018 at 10:01:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Apr 23 2018, Taylor Blau wrote:
>
> > For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno
>
> Looks good to me aside from two minor issues I noticed:
>
>  * In "grep.c: display column number of first match" you use a comment
>    style we don't normally use, i.e. /**\n not /*\n. See "Multi-line
>    comments" in Documentation/CodingGuidelines.
>
>  * You're not updating contrib/git-jump/README to document the new
>    output format. It just refers to the old format, but now depending on
>    if you use "grep" or not it'll use this new thing. It also makes
>    sense to update the example added in 007d06aa57 ("contrib/git-jump:
>    allow to configure the grep command", 2017-11-20) which seems to have
>    added jump.grepCmd as a workaround for not having this.
>
> But also, after just looking at this the second time around; Is there a
> reason we shouldn't just call this --column, not --column-number? I
> realize the former works because of the lazyness of our getopt parsing
> (also --colu though..).
>
> I think when we add features to git-grep we should be as close to GNU
> grep as possible (e.g. not add this -m alias meaning something different
> as in your v1), but if GNU grep doesn't have something go with the trend
> of other grep tools, as noted at
> https://beyondgrep.com/feature-comparison/ (and I found another one that
> has this: https://github.com/beyondgrep/website/pull/83), so there's
> already 3 prominent grep tools that call this just --column.
>
> I think we should just go with that.

I would be happy with either, though I think that my preference is to
retain '--column-number', as introduced in v2. I think that given the
choice between (1) staying closer to our conventions (i.e.,
'--line-number' instead of '--line') and (2) staying closer to other
tools', I'd choose (1).

That said, I'll happily pick up whichever the majority prefers, so if
that's --column and not --column-number, that works OK for me. I believe
that the ultimate say should be up to Junio.

> Also, as a bonus question, since you're poking at this column code
> anyway, interested in implementing -o (--only-matching)? That would be
> super-useful (see
> https://public-inbox.org/git/87in9ucsbb.fsf@evledraar.gmail.com/) :)

Sure, thanks for pointing me in the right direction :-).


Thanks,
Taylor

[PATCH v3 0/7] Teach '--column-number' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is v3 of my series to teach '--column-numbers' to 'git-grep(1)'.

Since last time, I have:

  * Removed '-m' in a few places that I forgot to during v2 [1] [2].

  * Expanded upon the definition of '--column-number' in 'git grep
    --help'. [3]

  * Initialized some new fields in grep_opt to 0 that would be avoided
    by memset(). [4]

  * Block comment to indicate the desired use of 'cno' in grep.c's
    'show_line()' [5], [6].

  * Annotated the new tests with the permutation that they are testing.

The most notable change since last time is reorganizing the series to
match Eric's suggestion, which I hope makes the reviewing experience a
little easier. I will strive to apply these lessons in future series.

Little has changed on the implementation front since v2, so I think that
the most outstanding further discussion will be centered around --column
vs --column-number. I have posted some thoughts about that in [7].

Thanks as always for your review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/878t9eewu2.fsf@evledraar.gmail.com
[2]: https://public-inbox.org/git/877eoyewss.fsf@evledraar.gmail.com
[3]: https://public-inbox.org/git/878t9eewu2.fsf@evledraar.gmail.com
[4]: https://public-inbox.org/git/CAPig+cRXkSrPHPyEEhp6_ndRBNW3hE7HkspSk1atPSE5pn_sMw@mail.gmail.com
[5]: https://public-inbox.org/git/CAPig+cQ2+wTTXE0mhnGnp2pZug=Po0SCVwCO_2agxUDaOsFRLw@mail.gmail.com
[6]: https://public-inbox.org/git/CAPig+cR0unM2uKcapHyAFrvMyPx4VgsW4wDswb_GNwE4EcYb8Q@mail.gmail.com
[7]: https://public-inbox.org/git/20180424043140.GA82406@syl.local

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column-number' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt |  8 +++++++-
 builtin/grep.c             |  1 +
 contrib/git-jump/git-jump  |  2 +-
 grep.c                     | 39 +++++++++++++++++++++++++++++---------
 grep.h                     |  2 ++
 t/t7810-grep.sh            | 22 +++++++++++++++++++++
 7 files changed, 69 insertions(+), 12 deletions(-)

Inter-diff (since v2):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1645fcf2ae..8a2893d1e1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1711,7 +1711,7 @@ grep.lineNumber::
 	If set to true, enable `-n` option by default.

 grep.columnNumber::
-	If set to true, enable `-m` option by default.
+	If set to true, enable the `--column-number` option by default.

 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index b75a039768..c5c4d712e6 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -45,7 +45,7 @@ grep.lineNumber::
 	If set to true, enable `-n` option by default.

 grep.columnNumber::
-	If set to true, enable `-m` option by default.
+	If set to true, enable the `--column-number` option by default.

 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
diff --git a/builtin/grep.c b/builtin/grep.c
index 23ce97f998..512f60c591 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +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-number", &opt.columnnum, N_("show column numbers")),
+		OPT_BOOL(0, "column-number", &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,
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 2706963690..8bc57ea0f8 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n -m"
+	test -n "$cmd" || cmd="git grep -n --column-number"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
diff --git a/grep.c b/grep.c
index 23250e60d0..7284dec155 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
 	color_set(opt->color_lineno, "");
+	color_set(opt->color_columnno, "");
 	color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_selected, "");
@@ -1404,6 +1405,11 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	/**
+	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
+	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
+	 * being called with a context line.
+	 */
 	if (opt->columnnum && cno) {
 		char buf[32];
 		xsnprintf(buf, sizeof(buf), "%d", cno);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 7349c7fadc..bbce57c8b1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,7 +99,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep -w $L" '
+	test_expect_success "grep -w $L (with --column-number)" '
 		{
 			echo ${HC}file:5:foo mmap bar
 			echo ${HC}file:14:foo_mmap bar mmap
@@ -110,7 +110,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep -w $L" '
+	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

--
2.17.0

[PATCH v3 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v3 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH v3 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
 	color_set(opt->color_lineno, "");
+	color_set(opt->color_columnno, "");
 	color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v3 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column-number', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..d58d940afb 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	/**
+	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
+	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
+	 * being called with a context line.
+	 */
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

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

[Taylor Blau]

Teach 'git-grep(1)' a new option, '--column-number', to show the column
number of the first match on a non-context line.

For example:

  $ ./git-grep -n --column-number 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 |  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..51dcfa5093 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-number]
 	   [-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-number::
+	Prefix the 1-indexed column number of the first match on non-context lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..512f60c591 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-number", &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,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..bbce57c8b1 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-number)" '
+		{
+			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-number -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-number -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "grep -w $L" '
 		{
 			echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH v3 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column-number', document and
teach grep.c how to interpret relevant configuration options, similar to
those associated with '--line-number'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   | 5 +++++
 Documentation/git-grep.txt | 3 +++
 grep.c                     | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..8a2893d1e1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `lineNumber`;;
 	line number prefix (when using `-n`)
+`columnNumber`;;
+	column number prefix (when using `--column-number`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable the `--column-number` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 51dcfa5093..c5c4d712e6 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+	If set to true, enable the `--column-number` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index d58d940afb..7284dec155 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.columnnumber")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.columnnumber"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v3 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column-number' in order
to teach Peff's 'git-jump' script how to jump to the correct column for
any given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/git-jump | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..8bc57ea0f8 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n --column-number"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

Re: [PATCH v3 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Eric Sunshine]

On Tue, Apr 24, 2018 at 1:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> Take advantage of 'git-grep(1)''s new option, '--column-number' in order
> to teach Peff's 'git-jump' script how to jump to the correct column for
> any given match.
>
> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> no additional cleanup is necessary.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  contrib/git-jump/git-jump | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Based upon Ævar review[1], I was expecting to see git-jump/README
modified by this patch, as well. Perhaps you overlooked or forgot
about that review comment, or perhaps you disagreed with it?

[1]: https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com/

> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> index 80ab0590bc..8bc57ea0f8 100755
> --- a/contrib/git-jump/git-jump
> +++ b/contrib/git-jump/git-jump
> @@ -52,7 +52,7 @@ mode_merge() {
>  # editor shows them to us in the status bar.
>  mode_grep() {
>         cmd=$(git config jump.grepCmd)
> -       test -n "$cmd" || cmd="git grep -n"
> +       test -n "$cmd" || cmd="git grep -n --column-number"
>         $cmd "$@" |
>         perl -pe '
>         s/[ \t]+/ /g;
> --
> 2.17.0

Re: [PATCH v3 4/7] grep.c: display column number of first match

[Eric Sunshine]

On Tue, Apr 24, 2018 at 1:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> To prepare for 'git grep' learning '--column-number', teach grep.c's
> show_line() how to show the column of the first match on non-context
> line.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/grep.c b/grep.c
> @@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
> +       /**
> +        * Treat 'cno' as the 1-indexed offset from the start of a non-context
> +        * line to its first match. Otherwise, 'cno' is 0 indicating that we are
> +        * being called with a context line.
> +        */

Nit: Ævar's review[1] mentioned that this project tends to use
/*...*/, not /**...*/.

[1]: https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com/

> +       if (opt->columnnum && cno) {
> +               char buf[32];
> +               xsnprintf(buf, sizeof(buf), "%d", cno);
> +               output_color(opt, buf, strlen(buf), opt->color_columnno);
> +               output_sep(opt, sign);
> +       }

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I think when we add features to git-grep we should be as close to GNU
> grep as possible (e.g. not add this -m alias meaning something different
> as in your v1), but if GNU grep doesn't have something go with the trend
> of other grep tools, as noted at
> https://beyondgrep.com/feature-comparison/ (and I found another one that
> has this: https://github.com/beyondgrep/website/pull/83), so there's
> already 3 prominent grep tools that call this just --column.
>
> I think we should just go with that.

OK.  If they called it --column-number, that might have been more in
line with GNU grep's --line-number, but that is not something we can
dictate retroactively anyway, so --column to match them would be
better than trying to be consistent and ending up with being
different from everybody else.

> Also, as a bonus question, since you're poking at this column code
> anyway, interested in implementing -o (--only-matching)? That would be
> super-useful (see
> https://public-inbox.org/git/87in9ucsbb.fsf@evledraar.gmail.com/) :)

Sounds good ;-).

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

On Tue, Apr 24, 2018 at 03:13:55PM +0900, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I think when we add features to git-grep we should be as close to GNU
> > grep as possible (e.g. not add this -m alias meaning something different
> > as in your v1), but if GNU grep doesn't have something go with the trend
> > of other grep tools, as noted at
> > https://beyondgrep.com/feature-comparison/ (and I found another one that
> > has this: https://github.com/beyondgrep/website/pull/83), so there's
> > already 3 prominent grep tools that call this just --column.
> >
> > I think we should just go with that.
>
> OK.  If they called it --column-number, that might have been more in
> line with GNU grep's --line-number, but that is not something we can
> dictate retroactively anyway, so --column to match them would be
> better than trying to be consistent and ending up with being
> different from everybody else.

That sounds sensible. Let's call the new option '--column', and the
configuration options grep.column and color.grep.column to match
(instead of s/column/columnNumber/g), yes?

Thanks,
Taylor

Re: [PATCH v3 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

On Tue, Apr 24, 2018 at 01:37:36AM -0400, Eric Sunshine wrote:
> On Tue, Apr 24, 2018 at 1:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > Take advantage of 'git-grep(1)''s new option, '--column-number' in order
> > to teach Peff's 'git-jump' script how to jump to the correct column for
> > any given match.
> >
> > 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> > no additional cleanup is necessary.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  contrib/git-jump/git-jump | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Based upon Ævar review[1], I was expecting to see git-jump/README
> modified by this patch, as well. Perhaps you overlooked or forgot
> about that review comment, or perhaps you disagreed with it?

Yes, and thank you for pointing that out. I recall reading his mail and
thought that when I prepared v3 that I had already included his changes,
but I had in fact not done so.

I amended the git-jump's README to prepare for v4, but was somewhat
confused by Ævar's comment when I reread [1]. I believe he was
suggesting updating the example to remove a reference to ag(1)'s
'--column' when configuring jump.grepCmd to 'ag --column'. Since
git-{grep,jump} support this now by default, I changed that line to
simply 'ag', instead of 'ag --column', as such:

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -37,3 +37,3 @@ Git-jump can generate four types of interesting lists:

-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.

@@ -67,3 +67,3 @@ git jump grep -i foo_bar
 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag --column"
+git config jump.grepCmd "ag"
 --------------------------------------------------
@@ -84,3 +84,3 @@ leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.

---

Does this look OK?

Thanks,
Taylor

[PATCH v4 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my fourth--and what I anticipate to be the final--re-roll of
my series to add teach 'git-grep(1)' a new '--column' flag.

Since last time, I have changed the following:

  * Respond to Ævar's review suggesting that I (1) change git-jump's
    README, (2) use --column over --column-number, and (3) use /*\n, not
    /**\n. [1].

This change comprises the majority of the inter-diff between v3..v4,
which is added below for conveniency. I have chosen to additionally
rename the configuration variables from columnNumber to column, to be
consistent with the new flag name.

Thanks in advance for your review. I am going to send out my next patch
(which Ævar suggested) to add '--only-matching' to 'git-grep(1)'.


Thanks,
Taylor

[1]: https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt |  8 +++++++-
 builtin/grep.c             |  1 +
 contrib/git-jump/README    |  6 +++---
 contrib/git-jump/git-jump  |  2 +-
 grep.c                     | 39 +++++++++++++++++++++++++++++---------
 grep.h                     |  2 ++
 t/t7810-grep.sh            | 22 +++++++++++++++++++++
 8 files changed, 72 insertions(+), 15 deletions(-)

Inter-diff (since v3):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a2893d1e1..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,8 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `lineNumber`;;
 	line number prefix (when using `-n`)
-`columnNumber`;;
-	column number prefix (when using `--column-number`)
+`column`;;
+	column number prefix (when using `--column`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1710,8 +1710,8 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.

-grep.columnNumber::
-	If set to true, enable the `--column-number` option by default.
+grep.column::
+	If set to true, enable the `--column` option by default.

 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c5c4d712e6..d451cd8883 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] [--column-number]
+	   [-F | --fixed-strings] [-n | --line-number] [--column]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
@@ -44,8 +44,8 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.

-grep.columnNumber::
-	If set to true, enable the `--column-number` option by default.
+grep.column::
+	If set to true, enable the `--column` option by default.

 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
@@ -172,7 +172,7 @@ providing this option will cause it to die.
 --line-number::
 	Prefix the line number to matching lines.

---column-number::
+--column::
 	Prefix the 1-indexed column number of the first match on non-context lines.

 -l::
diff --git a/builtin/grep.c b/builtin/grep.c
index 512f60c591..5c83f17759 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +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-number", &opt.columnnum, N_("show column number of first match")),
+		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,
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:

   2. The beginning of any merge conflict markers.

-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.

   4. Any whitespace errors detected by `git diff --check`.

@@ -65,7 +65,7 @@ git jump grep foo_bar
 git jump grep -i foo_bar

 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag --column"
+git config jump.grepCmd "ag"
 --------------------------------------------------


@@ -82,7 +82,7 @@ which does something similar to `git jump grep`. However, it is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.


 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 8bc57ea0f8..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n --column-number"
+	test -n "$cmd" || cmd="git grep -n --column"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
diff --git a/grep.c b/grep.c
index 7284dec155..37bb39a4a8 100644
--- a/grep.c
+++ b/grep.c
@@ -96,7 +96,7 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "grep.columnnumber")) {
+	if (!strcmp(var, "grep.column")) {
 		opt->columnnum = git_config_bool(var, value);
 		return 0;
 	}
@@ -116,7 +116,7 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
-	else if (!strcmp(var, "color.grep.columnnumber"))
+	else if (!strcmp(var, "color.grep.column"))
 		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
@@ -1405,7 +1405,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
-	/**
+	/*
 	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
 	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
 	 * being called with a context line.
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index bbce57c8b1..a03c3416e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,14 +99,14 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep -w $L (with --column-number)" '
+	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-number -w -e mmap $H >actual &&
+		git grep --column -w -e mmap $H >actual &&
 		test_cmp expected actual
 	'

@@ -117,7 +117,7 @@ do
 			echo ${HC}file:4:5:foo mmap bar_mmap
 			echo ${HC}file:5:14:foo_mmap bar mmap baz
 		} >expected &&
-		git grep -n --column-number -w -e mmap $H >actual &&
+		git grep -n --column -w -e mmap $H >actual &&
 		test_cmp expected actual
 	'

--
2.17.0

[PATCH v4 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v4 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH v4 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
 	color_set(opt->color_lineno, "");
+	color_set(opt->color_columnno, "");
 	color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v4 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	/*
+	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
+	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
+	 * being called with a context line.
+	 */
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

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

[Taylor Blau]

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 <me@ttaylorr.com>
---
 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) [<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.
+
 -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", &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,
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
-- 
2.17.0

[PATCH v4 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   | 5 +++++
 Documentation/git-grep.txt | 3 +++
 grep.c                     | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `lineNumber`;;
 	line number prefix (when using `-n`)
+`column`;;
+	column number prefix (when using `--column`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 5409a24399..d451cd8883 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index f3fe416791..37bb39a4a8 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.column")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.column"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/README   | 6 +++---
 contrib/git-jump/git-jump | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -65,7 +65,7 @@ git jump grep foo_bar
 git jump grep -i foo_bar
 
 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag --column"
+git config jump.grepCmd "ag"
 --------------------------------------------------
 
 
@@ -82,7 +82,7 @@ which does something similar to `git jump grep`. However, it is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n --column"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

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 <me@ttaylorr.com> 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

Re: [PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Martin Ågren]

On 5 May 2018 at 04:43, Taylor Blau <me@ttaylorr.com> wrote:
> Take advantage of 'git-grep(1)''s new option, '--column' in order to
> teach Peff's 'git-jump' script how to jump to the correct column for any
> given match.
>
> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> no additional cleanup is necessary.

> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> index 4484bda410..7630e16854 100644

>  # use the silver searcher for git jump grep
> -git config jump.grepCmd "ag --column"
> +git config jump.grepCmd "ag"

I think this change originates from Ævar's comment that it "also makes
sense to update the example added in 007d06aa57 [...] which seems to
have added jump.grepCmd as a workaround for not having this" [1].

Somehow though, this approach seems a bit backwards to me. I do believe
that your series reduces the reasons for using `jump.grepCmd`, but
regressing this example usage of `jump.grepCmd` seems a bit hostile. If
someone wants to use `ag`, wouldn't we want to hint that they will
probably want to use `--column`?

Is there some other `ag --column --foo` that we can give, where `--foo`
is not yet in `git grep`? ;-)

Martin

[1] https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com/

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 <me@ttaylorr.com>
> ---
>  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) [<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", &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,
> 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)'

[Æ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 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Ævar Arnfjörð Bjarmason]

On Sun, May 06 2018, Martin Ågren wrote:

> On 5 May 2018 at 04:43, Taylor Blau <me@ttaylorr.com> wrote:
>> Take advantage of 'git-grep(1)''s new option, '--column' in order to
>> teach Peff's 'git-jump' script how to jump to the correct column for any
>> given match.
>>
>> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
>> no additional cleanup is necessary.
>
>> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
>> index 4484bda410..7630e16854 100644
>
>>  # use the silver searcher for git jump grep
>> -git config jump.grepCmd "ag --column"
>> +git config jump.grepCmd "ag"
>
> I think this change originates from Ævar's comment that it "also makes
> sense to update the example added in 007d06aa57 [...] which seems to
> have added jump.grepCmd as a workaround for not having this" [1].
>
> Somehow though, this approach seems a bit backwards to me. I do believe
> that your series reduces the reasons for using `jump.grepCmd`, but
> regressing this example usage of `jump.grepCmd` seems a bit hostile. If
> someone wants to use `ag`, wouldn't we want to hint that they will
> probably want to use `--column`?
>
> Is there some other `ag --column --foo` that we can give, where `--foo`
> is not yet in `git grep`? ;-)
>
> Martin
>
> [1] https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com/

Yeah it doesn't make sense to drop --column here, FWIW what I had in
mind was something closer to:

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..357f79371a 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 -----------------------------------

+Or, when running 'git jump grep' column numbers will also be emitted,
+e.g. `git jump grep "hello"' would return:
+
+-----------------------------------
+foo.c:2:10: printf("hello word!\n");
+-----------------------------------
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.

I.e. let's note what the output format is now like for 'grep', and no
need to change the jump.grepCmd.

The above patch may be incorrect when it comes to the line numbe /
column number / format, I just wrote that by hand.

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

[Junio C Hamano]

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?

 - 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 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

On Sun, May 06, 2018 at 08:03:01PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, May 06 2018, Martin Ågren wrote:
>
> > On 5 May 2018 at 04:43, Taylor Blau <me@ttaylorr.com> wrote:
> >> Take advantage of 'git-grep(1)''s new option, '--column' in order to
> >> teach Peff's 'git-jump' script how to jump to the correct column for any
> >> given match.
> >>
> >> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> >> no additional cleanup is necessary.
> >
> >> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> >> index 4484bda410..7630e16854 100644
> >
> >>  # use the silver searcher for git jump grep
> >> -git config jump.grepCmd "ag --column"
> >> +git config jump.grepCmd "ag"
> >
> > I think this change originates from Ævar's comment that it "also makes
> > sense to update the example added in 007d06aa57 [...] which seems to
> > have added jump.grepCmd as a workaround for not having this" [1].
> >
> > Somehow though, this approach seems a bit backwards to me. I do believe
> > that your series reduces the reasons for using `jump.grepCmd`, but
> > regressing this example usage of `jump.grepCmd` seems a bit hostile. If
> > someone wants to use `ag`, wouldn't we want to hint that they will
> > probably want to use `--column`?
> >
> > Is there some other `ag --column --foo` that we can give, where `--foo`
> > is not yet in `git grep`? ;-)
> >
> > Martin
> >
> > [1] https://public-inbox.org/git/874lk2e4he.fsf@evledraar.gmail.com/
>
> Yeah it doesn't make sense to drop --column here, FWIW what I had in
> mind was something closer to:

Thanks; I wasn't quite clear on what you had suggested in [1], so the
attached diff is very helpful.

> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> index 4484bda410..357f79371a 100644
> --- a/contrib/git-jump/README
> +++ b/contrib/git-jump/README
> @@ -25,6 +25,13 @@ git-jump will feed this to the editor:
>  foo.c:2: printf("hello word!\n");
>  -----------------------------------
>
> +Or, when running 'git jump grep' column numbers will also be emitted,
> +e.g. `git jump grep "hello"' would return:
> +
> +-----------------------------------
> +foo.c:2:10: printf("hello word!\n");
> +-----------------------------------
> +
>  Obviously this trivial case isn't that interesting; you could just open
>  `foo.c` yourself. But when you have many changes scattered across a
>  project, you can use the editor's support to "jump" from point to point.
>
> I.e. let's note what the output format is now like for 'grep', and no
> need to change the jump.grepCmd.

Applied (mostly) the above patch to my copy, and will attach as part of
v5.

> The above patch may be incorrect when it comes to the line numbe /
> column number / format, I just wrote that by hand.

Yes; the only thing that was wrong was the column number. The "w" is in
the 10th 1-indexed column, and 'git grep --column' uses 1-indexed
columns.

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 <me@ttaylorr.com> 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)'

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

Re: [PATCH v4 2/7] grep.c: expose matched column in match_line()

[René Scharfe]

Am 05.05.2018 um 04:42 schrieb Taylor Blau:
> When calling match_line(), callers presently cannot determine the
> relative offset of the match because match_line() discards the
> 'regmatch_t' that contains this information.
> 
> Instead, teach match_line() to take in a 'regmatch_t *' so that callers
> can inspect the match's starting and ending offset from the beginning of
> the line. This additional argument has no effect when opt->extended is
> non-zero.
> 
> We will later pass the starting offset from 'regmatch_t *' to
> show_line() in order to display the column number of the first match.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>   grep.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/grep.c b/grep.c
> index 65b90c10a3..1c25782355 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
>   }
>   
>   static int match_line(struct grep_opt *opt, char *bol, char *eol,
> -		      enum grep_context ctx, int collect_hits)
> +		      regmatch_t *match, enum grep_context ctx,
> +		      int collect_hits)
>   {
>   	struct grep_pat *p;
> -	regmatch_t match;
>   
>   	if (opt->extended)
>   		return match_expr(opt, bol, eol, ctx, collect_hits);

If ->extended is set then match won't be touched...

>   
>   	/* we do not call with collect_hits without being extended */
>   	for (p = opt->pattern_list; p; p = p->next) {
> -		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
> +		if (match_one_pattern(p, bol, eol, ctx, match, 0))
>   			return 1;
>   	}
>   	return 0;
> @@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>   	int try_lookahead = 0;
>   	int show_function = 0;
>   	struct userdiff_driver *textconv = NULL;
> +	regmatch_t match;
>   	enum grep_context ctx = GREP_CONTEXT_HEAD;
>   	xdemitconf_t xecfg;
>   
> @@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>   		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
>   			ctx = GREP_CONTEXT_BODY;
>   
> -		hit = match_line(opt, bol, eol, ctx, collect_hits);
> +		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
>   		*eol = ch;
>   
>   		if (collect_hits)
> 

... which leaves it uninitialized.

So at least the combination of extended matches and --column should error
out.  Supporting it would be better, of course.  That could get tricky for
negations, though (e.g. git grep --not -e foo).

René

[PATCH v5 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my fifth re-roll of the series to add '--column' to
'git-grep(1)'.

The main changes are from a René's concerns in [1]. He points out that
'--column' with certain extended expressions can produce nonsense
(particularly because we leave the regmatch_t uninitialized).

> So at least the combination of extended matches and --column should error
> out.  Supporting it would be better, of course.  That could get tricky for
> negations, though (e.g. git grep --not -e foo).

I have opted for the die() option, instead of supporting extended
matches with --column. The problem with extended matches in this case is
that _sometimes_ --column can produce sensible results, and other times
it cannot.

For instance, an extended expression containing a single atom
has a known answer for --column, but one containing a negative atom does
only sometimes. Specifically: if an NOT atom is at the top-level, we
_never_ have a sensible answer for --column, but only sometimes do when
it is not at the top-level.

So, this leaves us two choices: (1) don't support --column and extended
expressions, or (2) support --column with extended expressions by not
printing columnar information when we don't have an answer. Option (2)
requires callers to perform deep inspection of their extended
expressions, and determine whether or not there is a answer that Git
could produce. This is too much to ask a caller to reasonably consider
when scripting. On the other hand, option (1) does not allow the caller
to do as much under certain circumstances, but simplifies their lives
when scripting, etc. For those reasons, let's pick (1).

Beyond that, here is a summary of what has changed since last time:

  * die() when given --extended, or compiling to an extended grammar,
    like in the case of 'git grep --column --not -e foo' [1].

  * Clarify patch 5/7 and indicate the intended purpose of supporting
    '--column' [2].

  * Clarify that '--column' gives a 1-indexed _byte_ offset, nothing
    else [3,5].

  * Remove a dangling reference to '--column-number' [4].

  * Clean up contrib/git-jump/README to Ævar's suggestion [6].


Thanks as always for your kind review.


Thanks,
Taylor

[1]: https://public-inbox.org/git/d030b4ee-5a92-4863-a29c-de2642bfae8d@web.de
[2]: https://public-inbox.org/git/CACsJy8BdJ0=gWZQVfSqy-vjtZVT4uZNzRaPYxRYxx2WNzaLodw@mail.gmail.com
[3]: https://public-inbox.org/git/20bc9baf-a85e-f00e-859e-e796ab4324f6@talktalk.net
[4]: https://public-inbox.org/git/87efioy8f9.fsf@evledraar.gmail.com
[5]: https://public-inbox.org/git/xmqqk1sfpn9j.fsf@gitster-ct.c.googlers.com
[6]: https://public-inbox.org/git/87d0y8y84q.fsf@evledraar.gmail.com/

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt |  9 +++++++-
 builtin/grep.c             |  4 ++++
 contrib/git-jump/README    | 12 +++++++++--
 contrib/git-jump/git-jump  |  2 +-
 grep.c                     | 42 ++++++++++++++++++++++++++++++--------
 grep.h                     |  2 ++
 t/t7810-grep.sh            | 32 +++++++++++++++++++++++++++++
 8 files changed, 96 insertions(+), 14 deletions(-)

Inter-diff (since v5):

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d451cd8883..dc8f76ce99 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -173,7 +173,8 @@ providing this option will cause it to die.
 	Prefix the line number to matching lines.

 --column::
-	Prefix the 1-indexed column number of the first match on non-context lines.
+	Prefix the 1-indexed byte-offset of the first match on non-context lines. This
+	option is incompatible with '--invert-match', and extended expressions.

 -l::
 --files-with-matches::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c83f17759..f9f516dfc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1112,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/contrib/git-jump/README b/contrib/git-jump/README
index 7630e16854..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 -----------------------------------

+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+-----------------------------------
+foo.c:2:9: printf("hello word!\n");
+-----------------------------------
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:

   2. The beginning of any merge conflict markers.

-  3. Any grep matches, including the column of the first match on a line.
+  3. Any grep matches, including the column of the first match on a
+     line.

   4. Any whitespace errors detected by `git diff --check`.

@@ -65,7 +73,7 @@ git jump grep foo_bar
 git jump grep -i foo_bar

 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag"
+git config jump.grepCmd "ag --column"
 --------------------------------------------------


diff --git a/grep.c b/grep.c
index 37bb39a4a8..5d904810ad 100644
--- a/grep.c
+++ b/grep.c
@@ -1001,6 +1001,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 a03c3416e7..aa56b21ed9 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -110,7 +110,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep -w $L (with --{line,column}-number)" '
+	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
@@ -1612,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

--
2.17.0

[PATCH v5 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v5 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH v5 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
 	color_set(opt->color_lineno, "");
+	color_set(opt->color_columnno, "");
 	color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v5 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	/*
+	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
+	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
+	 * being called with a context line.
+	 */
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

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

[Taylor Blau]

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.
+
 -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
-- 
2.17.0

[PATCH v5 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   | 5 +++++
 Documentation/git-grep.txt | 3 +++
 grep.c                     | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `lineNumber`;;
 	line number prefix (when using `-n`)
+`column`;;
+	column number prefix (when using `--column`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 75f1561112..dc8f76ce99 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index f4228c23ac..5d904810ad 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.column")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.column"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v5 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/README   | 12 ++++++++++--
 contrib/git-jump/git-jump |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 -----------------------------------
 
+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+-----------------------------------
+foo.c:2:9: printf("hello word!\n");
+-----------------------------------
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a
+     line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -82,7 +90,7 @@ which does something similar to `git jump grep`. However, it is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n --column"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

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

[Phillip Wood]

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
> 

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

[Duy Nguyen]

On Wed, May 9, 2018 at 4:13 AM, Taylor Blau <me@ttaylorr.com> wrote:
> 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")),

Two things to consider:

- do we ever want columnar output in git-grep? Something like "git
grep --column -l" could make sense (if you don't have very large
worktree). --column is currently used for column output in git-branch,
git-tag and git-status, which makes me think maybe we should reserve
"--column" for that purpose and use another name here, even if we
don't ever want column output in git-grep, for consistency.

- If this is to help git-jump only and rarely manually specified on
command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
from "git grep --<tab>" completion. You would need to use OPT_BOOL_F()
instead of OPT_BOOL() in order to add extra flags.
-- 
Duy

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

[Martin Ågren]

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?

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

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

[Taylor Blau]

On Wed, May 09, 2018 at 06:17:20PM +0200, Duy Nguyen wrote:
> On Wed, May 9, 2018 at 4:13 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > 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")),
>
> Two things to consider:
>
> - do we ever want columnar output in git-grep? Something like "git
> grep --column -l" could make sense (if you don't have very large
> worktree). --column is currently used for column output in git-branch,
> git-tag and git-status, which makes me think maybe we should reserve
> "--column" for that purpose and use another name here, even if we
> don't ever want column output in git-grep, for consistency.

I think that this is a valid concern. I had a similar thought when
adding 'git config --color' (as a new type specifier) that we might be
squatting on '--color' and instead opted for '[--type=<type>]'.

I don't feel that the tradeoff between '--column' as a good name and the
concern that we _might_ want to output in a columnar format in 'git
grep' someday warrants the change.

> - If this is to help git-jump only and rarely manually specified on
> command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
> from "git grep --<tab>" completion. You would need to use OPT_BOOL_F()
> instead of OPT_BOOL() in order to add extra flags.

I believe that this option is worth auto-completing. Its primarily
motivated for use within 'git-jump', but I feel as if it would be
useful for other callers, as well.

Thanks,
Taylor

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

[Taylor Blau]

On Wed, May 09, 2018 at 11:41:02AM +0100, Phillip Wood wrote:
> 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

I agree, and would be happy to change it as-such. I think that there is
some pending discussion about regressing 'git-jump' no longer supporting
'--not', so I'll wait for that to resolve before resending this patch.

Thanks,
Taylor

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

Why are these problematic in the first place?  If I said

    $ git grep -e first --and -e these
    $ git grep -e first --and --not -e those
    $ git grep -e first --or -e those

I'd expect that the first line of this paragraph will hit, and the
first hit for these three are "these", "first" and "first",
respectively.  Most importantly, in the last one, "--or" can be
omitted and the whole thing stops being "extended", so rejecting
extended as a whole does not make much sense.

    $ git grep -v second
    $ git grep --not -e second

may hit all lines in this message (except for the obvious two
lines), but we cannot say which column we found a hit.  I am
wondering if it is too grave a sin to report "the whole line is what
satisfied the criteria given" and say the match lies at column #1.

By doing so, obviously we can sidestep the whole "this mode is
sometimes incompatible" and "I need to compute a lot to see if the
given expression is compatible or not" issues.

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

[René Scharfe]

Am 10.05.2018 um 02:04 schrieb Junio C Hamano:
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> This check we should retain and change the wording to mention '--and',
>> '--or', and '--not' specifically.
> 
> Why are these problematic in the first place?  If I said
> 
>      $ git grep -e first --and -e these
>      $ git grep -e first --and --not -e those
>      $ git grep -e first --or -e those
> 
> I'd expect that the first line of this paragraph will hit, and the
> first hit for these three are "these", "first" and "first",
> respectively.  Most importantly, in the last one, "--or" can be
> omitted and the whole thing stops being "extended", so rejecting
> extended as a whole does not make much sense.
> 
>      $ git grep -v second
>      $ git grep --not -e second
> 
> may hit all lines in this message (except for the obvious two
> lines), but we cannot say which column we found a hit.  I am
> wondering if it is too grave a sin to report "the whole line is what
> satisfied the criteria given" and say the match lies at column #1.
> 
> By doing so, obviously we can sidestep the whole "this mode is
> sometimes incompatible" and "I need to compute a lot to see if the
> given expression is compatible or not" issues.

FWIW, Silver Searcher 2.1.0 does just that:

	$ echo a | ag --column -v b
	1:a

ripgrep 0.8.1 as well:

	$ echo a | rg --column -v b
	1:1:a

Side note: This example also shows that --column implies --line-number
for ripgrep because column numbers are mostly useless without line
numbers (https://github.com/BurntSushi/ripgrep/issues/243).  I'm not
sure I'm buying that reasoning.

ack-grep 2.22 seems to have problems with that combination:

	$ echo a | ack --column -v b
	a

	$ echo a | ack -H --column -v b
	-
	Use of uninitialized value $line_parts[1] in join or string at /usr/bin/ack line 653, <STDIN> line 1.
	1::a

René

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 10.05.2018 um 02:04 schrieb Junio C Hamano:
> ...
>>      $ git grep -v second
>>      $ git grep --not -e second
>> 
>> may hit all lines in this message (except for the obvious two
>> lines), but we cannot say which column we found a hit.  I am
>> wondering if it is too grave a sin to report "the whole line is what
>> satisfied the criteria given" and say the match lies at column #1.

And if we are planning to use this to implement '-o', then I'd
suggest that we'd say the matched part of the line is the whole
thing (i.e. so is column #1, eo is at the eol).

>> By doing so, obviously we can sidestep the whole "this mode is
>> sometimes incompatible" and "I need to compute a lot to see if the
>> given expression is compatible or not" issues.
>
> FWIW, Silver Searcher 2.1.0 does just that:
>
> 	$ echo a | ag --column -v b
> 	1:a
>
> ripgrep 0.8.1 as well:
>
> 	$ echo a | rg --column -v b
> 	1:1:a

Thanks for additional datapoints.

[PATCH v6 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my sixth re-roll of a series to add '--column' to 'git
grep'.

The main change since v5 is supporting --column with queries containing
--and, --or, or --not. Previously, I had chosen to die() in this case
since there isn't always a good answer to "what is the first column of
<complicated expression>?" but have gone back on this for two reasons:

  1. It is important not to regress calls to git-jump/contrib/git-jump
  that contain --and, --or, or --not.

  2. It is not that hard to detect the absence of column data in scripts.
     Likewise, git-jump will happily accept lines with or without
     columnar information, and Vim will accept it as-is.

So, let's support --column and only die() when also given
--invert-match. When we don't have a good answer, print nothing.

Thanks,
Taylor

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++++++-
 Documentation/git-grep.txt | 10 +++++++++-
 builtin/grep.c             |  4 ++++
 contrib/git-jump/README    | 12 ++++++++++--
 contrib/git-jump/git-jump  |  2 +-
 grep.c                     | 40 +++++++++++++++++++++++++++++---------
 grep.h                     |  2 ++
 t/t7810-grep.sh            | 39 +++++++++++++++++++++++++++++++++++++
 8 files changed, 102 insertions(+), 14 deletions(-)

Inter-diff (since v5):

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index dc8f76ce99..c48a578cb1 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -173,8 +173,9 @@ providing this option will cause it to die.
 	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.
+	Prefix the 1-indexed byte-offset of the first match from the start of the
+	matching line. This option is incompatible with '--invert-match', and
+	ignored with expressions using '--and', '--or', '--not'.

 -l::
 --files-with-matches::
diff --git a/grep.c b/grep.c
index 5d904810ad..5ba1b05526 100644
--- a/grep.c
+++ b/grep.c
@@ -1001,9 +1001,6 @@ 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);
@@ -1411,9 +1408,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	/*
 	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
 	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
-	 * being called with a context line.
+	 * being called with a context line, or we are --extended, and cannot
+	 * always show an answer.
 	 */
-	if (opt->columnnum && cno) {
+	if (opt->columnnum && sign == ':' && !opt->extended) {
 		char buf[32];
 		xsnprintf(buf, sizeof(buf), "%d", cno);
 		output_color(opt, buf, strlen(buf), opt->color_columnno);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index aa56b21ed9..491b2e044a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -110,6 +110,18 @@ do
 		test_cmp expected actual
 	'

+	test_expect_success "grep -w $L (with --column, -C)" '
+		{
+			echo ${HC}file:5:foo mmap bar
+			echo ${HC}file-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 -C1 -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
@@ -1617,9 +1629,4 @@ test_expect_success 'grep does not allow --column, --invert-match' '
 	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

--
2.17.0

[PATCH v6 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.<slot>::
 	filename prefix (when not using `-h`)
 `function`;;
 	function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
 	line number prefix (when using `-n`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v6 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
-		      enum grep_context ctx, int collect_hits)
+		      regmatch_t *match, enum grep_context ctx,
+		      int collect_hits)
 {
 	struct grep_pat *p;
-	regmatch_t match;
 
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
+		if (match_one_pattern(p, bol, eol, ctx, match, 0))
 			return 1;
 	}
 	return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	int try_lookahead = 0;
 	int show_function = 0;
 	struct userdiff_driver *textconv = NULL;
+	regmatch_t match;
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
 			ctx = GREP_CONTEXT_BODY;
 
-		hit = match_line(opt, bol, eol, ctx, collect_hits);
+		hit = match_line(opt, bol, eol, &match, ctx, collect_hits);
 		*eol = ch;
 
 		if (collect_hits)
-- 
2.17.0

[PATCH v6 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_filename, "");
 	color_set(opt->color_function, "");
 	color_set(opt->color_lineno, "");
+	color_set(opt->color_columnno, "");
 	color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->extended_regexp_option = def->extended_regexp_option;
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
+	opt->columnnum = def->columnnum;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	color_set(opt->color_filename, def->color_filename);
 	color_set(opt->color_function, def->color_function);
 	color_set(opt->color_lineno, def->color_lineno);
+	color_set(opt->color_columnno, def->color_columnno);
 	color_set(opt->color_match_context, def->color_match_context);
 	color_set(opt->color_match_selected, def->color_match_selected);
 	color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
 	int prefix_length;
 	regex_t regexp;
 	int linenum;
+	int columnnum;
 	int invert;
 	int ignore_case;
 	int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
+	char color_columnno[COLOR_MAXLEN];
 	char color_match_context[COLOR_MAXLEN];
 	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v6 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 grep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
-		      const char *name, unsigned lno, char sign)
+		      const char *name, unsigned lno, unsigned cno, char sign)
 {
 	int rest = eol - bol;
 	const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		output_color(opt, buf, strlen(buf), opt->color_lineno);
 		output_sep(opt, sign);
 	}
+	/*
+	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
+	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
+	 * being called with a context line.
+	 */
+	if (opt->columnnum && cno) {
+		char buf[32];
+		xsnprintf(buf, sizeof(buf), "%d", cno);
+		output_color(opt, buf, strlen(buf), opt->color_columnno);
+		output_sep(opt, sign);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 			break;
 
 		if (match_funcname(opt, gs, bol, eol)) {
-			show_line(opt, bol, eol, gs->name, lno, '=');
+			show_line(opt, bol, eol, gs->name, lno, 0, '=');
 			break;
 		}
 	}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, gs->name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, 0, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
 				show_funcname_line(opt, gs, bol, lno);
-			show_line(opt, bol, eol, gs->name, lno, ':');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, gs->name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, '-');
 		}
 
 	next_line:
-- 
2.17.0

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

[Taylor Blau]

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 |  7 ++++++-
 builtin/grep.c             |  4 ++++
 grep.c                     |  5 +++--
 t/t7810-grep.sh            | 39 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..cec4665df5 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,11 @@ 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 from the start of the
+	matching line. This option is incompatible with '--invert-match', and
+	ignored with expressions using '--and', '--or', '--not'.
+
 -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..7396b49a2d 100644
--- a/grep.c
+++ b/grep.c
@@ -1402,9 +1402,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	/*
 	 * Treat 'cno' as the 1-indexed offset from the start of a non-context
 	 * line to its first match. Otherwise, 'cno' is 0 indicating that we are
-	 * being called with a context line.
+	 * being called with a context line, or we are --extended, and cannot
+	 * always show an answer.
 	 */
-	if (opt->columnnum && cno) {
+	if (opt->columnnum && sign == ':' && !opt->extended) {
 		char buf[32];
 		xsnprintf(buf, sizeof(buf), "%d", cno);
 		output_color(opt, buf, strlen(buf), opt->color_columnno);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..491b2e044a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,40 @@ 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 --column, -C)" '
+		{
+			echo ${HC}file:5:foo mmap bar
+			echo ${HC}file-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 -C1 -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 +1624,9 @@ 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_done
-- 
2.17.0

[PATCH v6 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt   | 5 +++++
 Documentation/git-grep.txt | 3 +++
 grep.c                     | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.<slot>::
 	function name lines (when using `-p`)
 `lineNumber`;;
 	line number prefix (when using `-n`)
+`column`;;
+	column number prefix (when using `--column`)
 `match`;;
 	matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index cec4665df5..c48a578cb1 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
 	If set to true, enable `-n` option by default.
 
+grep.column::
+	If set to true, enable the `--column` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index 7396b49a2d..5ba1b05526 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->linenum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.column")) {
+		opt->columnnum = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
+	else if (!strcmp(var, "color.grep.column"))
+		color = opt->color_columnno;
 	else if (!strcmp(var, "color.grep.matchcontext"))
 		color = opt->color_match_context;
 	else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v6 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/git-jump/README   | 12 ++++++++++--
 contrib/git-jump/git-jump |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
 foo.c:2: printf("hello word!\n");
 -----------------------------------
 
+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+-----------------------------------
+foo.c:2:9: printf("hello word!\n");
+-----------------------------------
+
 Obviously this trivial case isn't that interesting; you could just open
 `foo.c` yourself. But when you have many changes scattered across a
 project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a
+     line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -82,7 +90,7 @@ which does something similar to `git jump grep`. However, it is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
 	cmd=$(git config jump.grepCmd)
-	test -n "$cmd" || cmd="git grep -n"
+	test -n "$cmd" || cmd="git grep -n --column"
 	$cmd "$@" |
 	perl -pe '
 	s/[ \t]+/ /g;
-- 
2.17.0

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

[Taylor Blau]

On Thu, May 10, 2018 at 09:04:34AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > This check we should retain and change the wording to mention '--and',
> > '--or', and '--not' specifically.
>
> Why are these problematic in the first place?  If I said
>
>     $ git grep -e first --and -e these
>     $ git grep -e first --and --not -e those
>     $ git grep -e first --or -e those
>
> I'd expect that the first line of this paragraph will hit, and the
> first hit for these three are "these", "first" and "first",
> respectively.  Most importantly, in the last one, "--or" can be
> omitted and the whole thing stops being "extended", so rejecting
> extended as a whole does not make much sense.

Agreed that this is what I would expect, too. The trouble is that we
never do a compilation step from containing --and, --or, --not to a
POSIX regexp. So, if we're extended, we have to assume that there might
not be an answer.

Given this, I don't think we should categorically die() when we
encounter this (more below in the next hunk of this mail). I think this
thread has established that there are certainly cases where we cannot
provide a meaningful answer, (--not at the top-level, for instance) but
there are cases where we can (as you indicate above).

One day, I would like to support --column with --and, --or, or --not in
cases where there _is_ a definite answer. That said, omitting this
information for now and at least not die()-ing I think is worth taking
this patch earlier, and leaving some #leftoverbits.

>     $ git grep -v second
>     $ git grep --not -e second
>
> may hit all lines in this message (except for the obvious two
> lines), but we cannot say which column we found a hit.  I am
> wondering if it is too grave a sin to report "the whole line is what
> satisfied the criteria given" and say the match lies at column #1.

I think that is sensible. I previously was opposed to this because I
thought that it would be too difficult to script around the 'sometimes
we have columns but other times not' and 'I gave --column' but have to
check whether or not they are really there.

I no longer believe that my above argument is sound. It simplifies the
matter greatly to simply not share columns when we don't have a good
answer, and do when we do.

In other terms:

  * not giving '--column' will _never_ give a column,
  * '--column --invert' will _always_ die(), and
  * '--column --[and | or | not]' will _never_ give a column.

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>>     $ git grep -v second
>>     $ git grep --not -e second
>>
>> may hit all lines in this message (except for the obvious two
>> lines), but we cannot say which column we found a hit.  I am
>> wondering if it is too grave a sin to report "the whole line is what
>> satisfied the criteria given" and say the match lies at column #1.
>
> I think that is sensible. I previously was opposed to this because I
> thought that it would be too difficult to script around the 'sometimes
> we have columns but other times not' and 'I gave --column' but have to
> check whether or not they are really there.

I am not sure if you really got what I meant.  I am suggesting that
"git grep -v --column second" should report that the entire line has
hit for each and every line that does not have "second" on it, which
is a good answer and eliminate "sometimes there is column answer (or
answer to -o query) and sometimes not" at the same time.

> In other terms:
>
>   * not giving '--column' will _never_ give a column,
>   * '--column --invert' will _always_ die(), and
>   * '--column --[and | or | not]' will _never_ give a column.

So this is completely opposite from what I would have expected. to
somebody who said "I think that is sensible." over there.

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

[Taylor Blau]

On Sat, May 12, 2018 at 02:08:48PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >>     $ git grep -v second
> >>     $ git grep --not -e second
> >>
> >> may hit all lines in this message (except for the obvious two
> >> lines), but we cannot say which column we found a hit.  I am
> >> wondering if it is too grave a sin to report "the whole line is what
> >> satisfied the criteria given" and say the match lies at column #1.
> >
> > I think that is sensible. I previously was opposed to this because I
> > thought that it would be too difficult to script around the 'sometimes
> > we have columns but other times not' and 'I gave --column' but have to
> > check whether or not they are really there.
>
> I am not sure if you really got what I meant.  I am suggesting that
> "git grep -v --column second" should report that the entire line has
> hit for each and every line that does not have "second" on it, which
> is a good answer and eliminate "sometimes there is column answer (or
> answer to -o query) and sometimes not" at the same time.

I re-read your note and understand more clearly now what your suggestion
is. To ensure that we're in agreement, do you mean:

  1. '--column -v' will _never_ give a column, but will never die(),
      either

  2. '--column --[and | or | not]' will never give a column, but will
      also never die(), either.

I think that _those_ semantics are sensible, and I apologize for
misinterpreting your original note.

> > In other terms:
> >
> >   * not giving '--column' will _never_ give a column,
> >   * '--column --invert' will _always_ die(), and
> >   * '--column --[and | or | not]' will _never_ give a column.
>
> So this is completely opposite from what I would have expected. to
> somebody who said "I think that is sensible." over there.

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> I re-read your note and understand more clearly now what your suggestion
> is. To ensure that we're in agreement, do you mean:
>
>   1. '--column -v' will _never_ give a column, but will never die(),
>       either

No, I don't.

>   2. '--column --[and | or | not]' will never give a column, but will
>       also never die(), either.

No, I don't.

If a file does not have substring "foo", then

	git grep -v -e foo file
	git grep --not -e foo file

would hit all lines, just like

	git grep -e '.*' file

does.

I would expect that all of these

	git grep --column/-o -v -e foo file
	git grep --column/-o --not -e foo file
	git grep --column/-o -e '.*' file

give the same output, which is what we would get if we consider the
hit from "choose lines that lack 'foo'" on a line without 'foo' is
caused by the entire contents on the line.  That is in line with
"choose lines that has anything (including nothing)" aka ".*" would
result in the entire line being reported via -o.  The byte offset of
the first hit on such a line reported by --column is also 1, and
that is a good and real answer to the question "git grep --column/-o"
can give.  

In an earlier message, you sounded like you do not think "we did not
have 'foo' on that line, and that is why we are emitting because we
are operating under -v" lack a definite answer for --column, but I
think you are wrong.  "On the entire line, we didn't find 'foo'
anywhere" is good enough reason for me to make the answer "the
entire line contributed to this hit" a definite one.

Exactly the same applies for "git grep --not -e foo".

When "git grep -e bar [--or] --not -e foo" shows a line because the
line has 'bar' on it, we have --column that points at 'b' of the
first 'bar' on the line.  When it shows a line because the line has
neither 'bar' or 'foo', then "--not -e foo" part would give a
definite "the entire line contributed to this decision that it does
not have 'foo'".

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

[Taylor Blau]

On Sat, May 12, 2018 at 03:07:04PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > I re-read your note and understand more clearly now what your suggestion
> > is. To ensure that we're in agreement, do you mean:
> >
> >   1. '--column -v' will _never_ give a column, but will never die(),
> >       either
>
> No, I don't.
>
> >   2. '--column --[and | or | not]' will never give a column, but will
> >       also never die(), either.
>
> No, I don't.
>
> If a file does not have substring "foo", then
>
> 	git grep -v -e foo file
> 	git grep --not -e foo file
>
> would hit all lines, just like
>
> 	git grep -e '.*' file
>
> does.
>
> I would expect that all of these
>
> 	git grep --column/-o -v -e foo file
> 	git grep --column/-o --not -e foo file
> 	git grep --column/-o -e '.*' file
>
> give the same output, which is what we would get if we consider the
> hit from "choose lines that lack 'foo'" on a line without 'foo' is
> caused by the entire contents on the line.  That is in line with
> "choose lines that has anything (including nothing)" aka ".*" would
> result in the entire line being reported via -o.  The byte offset of
> the first hit on such a line reported by --column is also 1, and
> that is a good and real answer to the question "git grep --column/-o"
> can give.

I agree with your message now and thank you for explaining what you
had written. I spoke with Peff off-list for a while to determine what I
think is essentially the answer to ``what are a set of semantics for
filling out a regmatch_t given an extended expression?''

It's helpful to recognize that the extended expressions are implemented
very much like a tree, so a reasonable semantics will lend itself well
to the way in which match_expr_eval() is implemented. Here's what we
came up with:

  * `git grep -e foo`. This is the case where the extended expression
    has a single atomic node in its tree. This falls into the "just call
    match_one_pattern()" case and has a simple answer: the starting
    offset and ending offset are that of whatever match_one_pattern
    gives.

  * `git grep --not -e foo`. This has the set of semantics that you
    describe above (the starting offset is 1), with the addition that
    the ending offset is the end of the line. This is similar to the
    fact that `--not foo` is very similar to `.$`.

  * `git grep --and -e foo -e bar`. This binary operation should recur
    on its sub-expressions and take the minimum of the starting offset
    and the maximum of the ending offset.

    For inputs of the form "foobar" and "foo bar", it will do the right
    thing (give the starting and ending offset for "foobar" and give no
    match, respectively).

  * `git grep --or -e foo -e bar`. This is the most complicated case, in
    my opinion. In going with the min/max idea in the and case above, I
    think that `--or` should also min/max its sub-expressions, but in
    fact we short-circuit evaluating the second sub-expression when we
    find a match for the first.

    So, in cases like matching `--or -e foo -e bar` with "foo baz bar",
    we'll do the right thing, since `foo` is the first sub-expression
    and happens to be the left-most match. In other words, we __adhere
    to our answer with the left-most match first__ semantics, but only
    because __the first sub-expression is the left-most match__.

    In the other case where we try and match the same expression against
    "bar baz foo", we'll return the starting offset of "foo", even
    though it isn't the left-most match, violating our semantics.

So, I propose we adopt the following: use the trivial answer for "foo",
the whole line for "--not", and min/max the starting/ending offsets for
binary operators, knowing that we will sometimes produce a weird answer
for --or.

I think that the semantics for --or are OK to go forward with, but would
be interested in the thoughts of others to figure out whether this is
sensible to everyone else.

Does this seem like an OK approach? Perhaps Peff can clarify some of
what's shared here, since we did speak elsewhere about it.

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>   * `git grep --and -e foo -e bar`. This binary operation should recur
>     on its sub-expressions and take the minimum of the starting offset
>     and the maximum of the ending offset.

We use infix notation, so the above is "git grep -e foo --and -e
bar" actually ;-).

But you raise an interesting point.  A line with "hello foo bar baz"
on it would match, so does a line with "goodbye bar baz foo", as
both of them hits pattern "foo" *and* pattern "bar".  It is not
quite clear what it means to "show the first hit on the line".  One
interpretation would be to take the minimum span that makes both
sides of "--and" happy (your "minimum of start, maximum of end").
Another might be to pick "foo" in the first and "bar" in the second
line, as that is the "first hit" on the line, which is consistent
with how "git grep -e foo" would say about "a foo b foo c foo" (I
expect that the leftmost "foo" would be the first hit).  So there
may be multiple, equally plausible answer to the question.

>     For inputs of the form "foobar" and "foo bar", it will do the right
>     thing (give the starting and ending offset for "foobar" and give no
>     match, respectively).

I think I agree with "foobar", but I do not understand why there is
no match for "foo bar".

>   * `git grep --or -e foo -e bar`. This is the most complicated case, in
>     my opinion. In going with the min/max idea in the and case above, I
>     think that `--or` should also min/max its sub-expressions, but in
>     fact we short-circuit evaluating the second sub-expression when we
>     find a match for the first.

I am not sure I follow.  "git grep -e foo --or -e bar" is just a
longhand for "git grep -e foo -e bar".  Shouldn't it highlight
whichever between foo and bar that appears leftmost on the line?

>     So, in cases like matching `--or -e foo -e bar` with "foo baz bar",
>     we'll do the right thing, since `foo` is the first sub-expression
>     and happens to be the left-most match. In other words, we __adhere
>     to our answer with the left-most match first__ semantics, but only
>     because __the first sub-expression is the left-most match__.
>
>     In the other case where we try and match the same expression against
>     "bar baz foo", we'll return the starting offset of "foo", even
>     though it isn't the left-most match, violating our semantics.

I am not sure why you think your min-starting/max-ending would lead
to such a conclusion.  'foo baz bar' would be covered in its
entirety, 'bar baz foo' would also, as starting of hits with pattern
'foo' and pattern 'bar' would be 'b' in 'bar' on that three-word
line, and ending of hits with these two patterns would be the last
'o' in 'foo' on the line.  

I'd expect that a line 'foo baz bar' matched against "-e foo --or -e
bar" would say "among three words on me, 'f' in foo is the first
location of the match", though.

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

[Taylor Blau]

Thanks for your thoughtful response. I answered in detail below, but I
think that we're in agreement about the semantics, with a few
corrections on my part. I'd like to push forward with this series,
including the proposed changes below, but feel that sending it as v7
would be asking too much of a reviewer. Would it be OK if I sent this a
new series entirely and we abandon this thread?

On Fri, May 18, 2018 at 03:27:44PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >   * `git grep --and -e foo -e bar`. This binary operation should recur
> >     on its sub-expressions and take the minimum of the starting offset
> >     and the maximum of the ending offset.
>
> We use infix notation, so the above is "git grep -e foo --and -e
> bar" actually ;-).

Thanks for catching that :-).

> But you raise an interesting point.  A line with "hello foo bar baz"
> on it would match, so does a line with "goodbye bar baz foo", as
> both of them hits pattern "foo" *and* pattern "bar".  It is not
> quite clear what it means to "show the first hit on the line".  One
> interpretation would be to take the minimum span that makes both
> sides of "--and" happy (your "minimum of start, maximum of end").

It's funny you should mention: this was nearly the exact phrase I used
when speaking with Peff.

> Another might be to pick "foo" in the first and "bar" in the second
> line, as that is the "first hit" on the line, which is consistent
> with how "git grep -e foo" would say about "a foo b foo c foo" (I
> expect that the leftmost "foo" would be the first hit).  So there
> may be multiple, equally plausible answer to the question.

This is the largest fact in my mind pertaining to this discussion: there
are probably many different interpretations of semantics for this, all
equally valid in their own way. I am partial to the minimum substring
interpretation (which follows naturally from the minimum-start,
maximum-end idea), accepting the shortcoming that `--or` sometimes
doesn't ``do the right thing.''

> >     For inputs of the form "foobar" and "foo bar", it will do the right
> >     thing (give the starting and ending offset for "foobar" and give no
> >     match, respectively).
>
> I think I agree with "foobar", but I do not understand why there is
> no match for "foo bar".

Ah, I think this is my mistake -- when I wrote this note last night. The
case of `-e foo --and -e bar` should clearly match both `foo bar` _and_
`foobar`.

> >   * `git grep --or -e foo -e bar`. This is the most complicated case, in
> >     my opinion. In going with the min/max idea in the and case above, I
> >     think that `--or` should also min/max its sub-expressions, but in
> >     fact we short-circuit evaluating the second sub-expression when we
> >     find a match for the first.
>
> I am not sure I follow.  "git grep -e foo --or -e bar" is just a
> longhand for "git grep -e foo -e bar".  Shouldn't it highlight
> whichever between foo and bar that appears leftmost on the line?

I don't believe that the two are treated the same, but I think that this
is another case where I was incorrect in my judgement of the
implementation last night. In fact, the only time when we _don't_ recur
on both sub-expressions of `--or` is when 'collect_hits' is zero. That's
fine, I believe.

> >     So, in cases like matching `--or -e foo -e bar` with "foo baz bar",
> >     we'll do the right thing, since `foo` is the first sub-expression
> >     and happens to be the left-most match. In other words, we __adhere
> >     to our answer with the left-most match first__ semantics, but only
> >     because __the first sub-expression is the left-most match__.
> >
> >     In the other case where we try and match the same expression against
> >     "bar baz foo", we'll return the starting offset of "foo", even
> >     though it isn't the left-most match, violating our semantics.
>
> I am not sure why you think your min-starting/max-ending would lead
> to such a conclusion.  'foo baz bar' would be covered in its
> entirety, 'bar baz foo' would also, as starting of hits with pattern
> 'foo' and pattern 'bar' would be 'b' in 'bar' on that three-word
> line, and ending of hits with these two patterns would be the last
> 'o' in 'foo' on the line.

Right, I think with the understanding in my last stanza of this response
("I don't believe that ...") this issue is resolved, and the
min-starting/max-ending _will_ do the right thing.

> I'd expect that a line 'foo baz bar' matched against "-e foo --or -e
> bar" would say "among three words on me, 'f' in foo is the first
> location of the match", though.

I would, too.

Thanks,
Taylor

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

[Taylor Blau]

On Fri, May 18, 2018 at 02:50:21PM -0700, Taylor Blau wrote:
> [...]
>
> > Another might be to pick "foo" in the first and "bar" in the second
> > line, as that is the "first hit" on the line, which is consistent
> > with how "git grep -e foo" would say about "a foo b foo c foo" (I
> > expect that the leftmost "foo" would be the first hit).  So there
> > may be multiple, equally plausible answer to the question.
>
> This is the largest fact in my mind pertaining to this discussion: there
> are probably many different interpretations of semantics for this, all
> equally valid in their own way. I am partial to the minimum substring
> interpretation (which follows naturally from the minimum-start,
> maximum-end idea), accepting the shortcoming that `--or` sometimes
> doesn't ``do the right thing.''

Ignore this last part. `--or` _does_ do the right thing, as noted below.

Thanks,
Taylor