[PATCH 0/6] show non-empty lines before functions with diff/grep -W

[PATCH 0/6] show non-empty lines before functions with diff/grep -W

[René Scharfe]

The option -W/--function-context lets git diff and git grep show the
whole surrounding function as context.  For the sake of simplicity and
performance they don't fully parse the files, but as a heuristic show
all lines from the preceding function line to the next one.

This series refines that heuristic and extends the context to include
any non-empty lines before the preceding function line as well.  They
most likely contain comments related to that function and are thus
relevant for reviewing diffs and search results.

Idea and original implementation for git diff by Vegard Nossum:
https://public-inbox.org/git/1484324112-17773-2-git-send-email-vegard.nossum@oracle.com/

  t4051: add test for comments preceeding function lines
  xdiff: factor out is_func_rec()
  xdiff: show non-empty lines before functions with -W
  t7810: improve check of -W with user-defined function lines
  grep: update boundary variable for pre-context
  grep: show non-empty lines before functions with -W

 grep.c                           | 35 +++++++++++++++++++++++++++-------
 t/t4051-diff-function-context.sh |  4 ++++
 t/t4051/hello.c                  |  3 +++
 t/t7810-grep.sh                  | 41 ++++++++++++++++++++++++++++++----------
 xdiff/xemit.c                    | 13 ++++++++++---
 5 files changed, 76 insertions(+), 20 deletions(-)

-- 
2.15.0

[PATCH 1/6] t4051: add test for comments preceding function lines

[René Scharfe]

When showing function context it would be helpful to show comments
immediately before declarations, as they are most likely relevant.  Add
a test for that.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh | 4 ++++
 t/t4051/hello.c                  | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 3e6b485ecb..30fc5bf2b3 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -85,6 +85,10 @@ test_expect_success 'setup' '
 
 check_diff changed_hello 'changed function'
 
+test_expect_failure ' context includes comment' '
+	grep "^ .*Hello comment" changed_hello.diff
+'
+
 test_expect_success ' context includes begin' '
 	grep "^ .*Begin of hello" changed_hello.diff
 '
diff --git a/t/t4051/hello.c b/t/t4051/hello.c
index 63b1a1e4ef..73e767e178 100644
--- a/t/t4051/hello.c
+++ b/t/t4051/hello.c
@@ -1,4 +1,7 @@
 
+/*
+ * Hello comment.
+ */
 static void hello(void)	// Begin of hello
 {
 	/*
-- 
2.15.0

Re: [PATCH 1/6] t4051: add test for comments preceding function lines

[Eric Sunshine]

On Sat, Nov 18, 2017 at 1:04 PM, René Scharfe <l.s.r@web.de> wrote:
> When showing function context it would be helpful to show comments
> immediately before declarations, as they are most likely relevant.  Add
> a test for that.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
> @@ -85,6 +85,10 @@ test_expect_success 'setup' '
>> +test_expect_failure ' context includes comment' '
> +       grep "^ .*Hello comment" changed_hello.diff
> +'

This shows only that the the "* Hello comment." line is in the context
but says nothing about the entire comment block being included (which
patch 3/6 implements). Perhaps the test could be more thorough?

> diff --git a/t/t4051/hello.c b/t/t4051/hello.c
> index 63b1a1e4ef..73e767e178 100644
> --- a/t/t4051/hello.c
> +++ b/t/t4051/hello.c
> @@ -1,4 +1,7 @@
>
> +/*
> + * Hello comment.
> + */
>  static void hello(void)        // Begin of hello
>  {

Re: [PATCH 1/6] t4051: add test for comments preceding function lines

[René Scharfe]

Am 19.11.2017 um 02:18 schrieb Eric Sunshine:
> On Sat, Nov 18, 2017 at 1:04 PM, René Scharfe <l.s.r@web.de> wrote:
>> When showing function context it would be helpful to show comments
>> immediately before declarations, as they are most likely relevant.  Add
>> a test for that.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
>> @@ -85,6 +85,10 @@ test_expect_success 'setup' '
>>> +test_expect_failure ' context includes comment' '
>> +       grep "^ .*Hello comment" changed_hello.diff
>> +'
> 
> This shows only that the the "* Hello comment." line is in the context
> but says nothing about the entire comment block being included (which
> patch 3/6 implements). Perhaps the test could be more thorough?

You say that as if it was a bad thing.  That line represents 33% of
your suggested full-comment metric, i.e. more than nothing.  But more
importantly it's the actual comment payload.  The leading "/*" line is
included as a consequence of the employed heuristic, but a more
refined one might omit it as it doesn't actually contain any comment.

Not sure if we actually want to tweak our heuristic further, but the
test is meant to check only if the interesting part is present and
give leeway regarding other implementation details.

> 
>> diff --git a/t/t4051/hello.c b/t/t4051/hello.c
>> index 63b1a1e4ef..73e767e178 100644
>> --- a/t/t4051/hello.c
>> +++ b/t/t4051/hello.c
>> @@ -1,4 +1,7 @@
>>
>> +/*
>> + * Hello comment.
>> + */
>>   static void hello(void)        // Begin of hello
>>   {

Re: [PATCH 1/6] t4051: add test for comments preceding function lines

[Junio C Hamano]

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

> your suggested full-comment metric, i.e. more than nothing.  But more
> importantly it's the actual comment payload.  The leading "/*" line is
> included as a consequence of the employed heuristic, but a more
> refined one might omit it as it doesn't actually contain any comment.

I am slightly in favor of than against the above reasoning, but it
probably deserves to be recorded somewhere more readily accessible
than the mailing list archive.  The title of the test "context
*includes* comment" can be read to hint it by not saying that the
precontext shows the *entire* comment, but that is a very weak hint
that will be missed by anybody unaware of the reasoning behind this
decision.

    When showing function context it would be helpful to show comments
    immediately before declarations, as they are most likely relevant.  Add
    a test for that.

... but without specifying the choice of lines too rigidly in the
test---we may want to stop before and not include "/*" in the
future, for example.

perhaps?

Re: [PATCH 1/6] t4051: add test for comments preceding function lines

[René Scharfe]

Am 20.11.2017 um 01:36 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> your suggested full-comment metric, i.e. more than nothing.  But more
>> importantly it's the actual comment payload.  The leading "/*" line is
>> included as a consequence of the employed heuristic, but a more
>> refined one might omit it as it doesn't actually contain any comment.
> 
> I am slightly in favor of than against the above reasoning, but it
> probably deserves to be recorded somewhere more readily accessible
> than the mailing list archive.  The title of the test "context
> *includes* comment" can be read to hint it by not saying that the
> precontext shows the *entire* comment, but that is a very weak hint
> that will be missed by anybody unaware of the reasoning behind this
> decision.
> 
>      When showing function context it would be helpful to show comments
>      immediately before declarations, as they are most likely relevant.  Add
>      a test for that.
> 
> ... but without specifying the choice of lines too rigidly in the
> test---we may want to stop before and not include "/*" in the
> future, for example.
> 
> perhaps?

That's fine with me.

Thanks,
René

Re: [PATCH 1/6] t4051: add test for comments preceding function lines

[Junio C Hamano]

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

>> I am slightly in favor of than against the above reasoning, but it
>> probably deserves to be recorded somewhere more readily accessible
>> than the mailing list archive...
>> 
>> perhaps?
>
> That's fine with me.

As there seems to be no other reason for updating the series, I'll
just amend it in locally.

Thanks.

[PATCH 2/6] xdiff: factor out is_func_rec()

[René Scharfe]

Add a helper for checking if a given record is a function line.  It
frees callers from having to deal with the buffer arguments of
match_func_rec().

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 xdiff/xemit.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 6881445e4a..c2d5bd004c 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -121,6 +121,12 @@ static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,
 	return xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);
 }
 
+static int is_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)
+{
+	char dummy[1];
+	return match_func_rec(xdf, xecfg, ri, dummy, sizeof(dummy)) >= 0;
+}
+
 struct func_line {
 	long len;
 	char buf[80];
@@ -178,7 +184,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 
 			/* Appended chunk? */
 			if (i1 >= xe->xdf1.nrec) {
-				char dummy[1];
 				long i2 = xch->i2;
 
 				/*
@@ -186,8 +191,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 				 * a whole function was added.
 				 */
 				while (i2 < xe->xdf2.nrec) {
-					if (match_func_rec(&xe->xdf2, xecfg, i2,
-						dummy, sizeof(dummy)) >= 0)
+					if (is_func_rec(&xe->xdf2, xecfg, i2))
 						goto post_context_calculation;
 					i2++;
 				}
-- 
2.15.0

[PATCH 3/6] xdiff: show non-empty lines before functions with -W

[René Scharfe]

Non-empty lines before a function definition are most likely comments
for that function and thus relevant.  Include them in function context.

Such a non-empty line might also belong to the preceeding function if
there is no separating blank line.  Stop extending the context upwards
also at the next function line to make sure only one extra function body
is shown at most.

Original-patch-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh | 2 +-
 xdiff/xemit.c                    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 30fc5bf2b3..2d76a971c4 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -85,7 +85,7 @@ test_expect_success 'setup' '
 
 check_diff changed_hello 'changed function'
 
-test_expect_failure ' context includes comment' '
+test_expect_success ' context includes comment' '
 	grep "^ .*Hello comment" changed_hello.diff
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index c2d5bd004c..7778dc2b19 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -204,6 +204,9 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			}
 
 			fs1 = get_func_line(xe, xecfg, NULL, i1, -1);
+			while (fs1 > 0 && !is_empty_rec(&xe->xdf1, fs1 - 1) &&
+			       !is_func_rec(&xe->xdf1, xecfg, fs1 - 1))
+				fs1--;
 			if (fs1 < 0)
 				fs1 = 0;
 			if (fs1 < s1) {
-- 
2.15.0

[PATCH 4/6] t7810: improve check of -W with user-defined function lines

[René Scharfe]

The check for function context (-W) together with user-defined function
line patterns reuses hello.c and pretends it's written in a language in
which function lines contain either "printf" or a trailing curly brace.
That's a bit obscure.

Make the test easier to read by adding a small PowerShell script, using
a simple, but meaningful expression, and separating out checks for
different aspects into dedicated tests instead of simply matching the
whole output byte for byte.

Also include a test for showing comments before function lines like git
diff -W does, which is currently failing because that functionality is
not implemented for git grep, yet.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t7810-grep.sh | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2a6679c2f5..632b905611 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -60,6 +60,18 @@ test_expect_success setup '
 		echo " line with leading space3"
 		echo "line without leading space2"
 	} >space &&
+	cat >hello.ps1 <<-\EOF &&
+	# No-op.
+	function dummy() {}
+
+	# Say hello.
+	function hello() {
+	  echo "Hello world."
+	} # hello
+
+	# Still a no-op.
+	function dummy() {}
+	EOF
 	git add . &&
 	test_tick &&
 	git commit -m initial
@@ -766,18 +778,27 @@ test_expect_success 'grep -W shows no trailing empty lines' '
 	test_cmp expected actual
 '
 
-cat >expected <<EOF
-hello.c=	printf("Hello world.\n");
-hello.c:	return 0;
-hello.c-	/* char ?? */
-EOF
-
 test_expect_success 'grep -W with userdiff' '
 	test_when_finished "rm -f .gitattributes" &&
-	git config diff.custom.xfuncname "(printf.*|})$" &&
-	echo "hello.c diff=custom" >.gitattributes &&
-	git grep -W return >actual &&
-	test_cmp expected actual
+	git config diff.custom.xfuncname "^function .*$" &&
+	echo "hello.ps1 diff=custom" >.gitattributes &&
+	git grep -W echo >function-context-userdiff-actual
+'
+
+test_expect_failure ' includes preceding comment' '
+	grep "# Say hello" function-context-userdiff-actual
+'
+
+test_expect_success ' includes function line' '
+	grep "=function hello" function-context-userdiff-actual
+'
+
+test_expect_success ' includes matching line' '
+	grep ":  echo" function-context-userdiff-actual
+'
+
+test_expect_success ' includes last line of the function' '
+	grep "} # hello" function-context-userdiff-actual
 '
 
 for threads in $(test_seq 0 10)
-- 
2.15.0

[PATCH 5/6] grep: update boundary variable for pre-context

[René Scharfe]

Function context can be bigger than -A/-B/-C context.  To find the
beginning of the combined context we search backwards.  Currently we
check at each loop iteration what we're looking for and determine the
effective upper boundary based on that.

Simplify this a bit by setting the variable "from" to the lowest unshown
line number up front if we're looking for a function line and set it
back to the required -B/-C context line number when we find one.  This
prepares the ground for the next patch; no functional change intended.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 grep.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/grep.c b/grep.c
index d0b9b6cdfa..2c55d10c55 100644
--- a/grep.c
+++ b/grep.c
@@ -1479,20 +1479,21 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 			     char *bol, char *end, unsigned lno)
 {
-	unsigned cur = lno, from = 1, funcname_lno = 0;
+	unsigned cur = lno, from = 1, funcname_lno = 0, orig_from;
 	int funcname_needed = !!opt->funcname;
 
-	if (opt->funcbody && !match_funcname(opt, gs, bol, end))
-		funcname_needed = 2;
-
 	if (opt->pre_context < lno)
 		from = lno - opt->pre_context;
 	if (from <= opt->last_shown)
 		from = opt->last_shown + 1;
+	orig_from = from;
+	if (opt->funcbody && !match_funcname(opt, gs, bol, end)) {
+		funcname_needed = 1;
+		from = opt->last_shown + 1;
+	}
 
 	/* Rewind. */
-	while (bol > gs->buf &&
-	       cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) {
+	while (bol > gs->buf && cur > from) {
 		char *eol = --bol;
 
 		while (bol > gs->buf && bol[-1] != '\n')
@@ -1501,6 +1502,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 		if (funcname_needed && match_funcname(opt, gs, bol, eol)) {
 			funcname_lno = cur;
 			funcname_needed = 0;
+			from = orig_from;
 		}
 	}
 
-- 
2.15.0

[PATCH 6/6] grep: show non-empty lines before functions with -W

[René Scharfe]

Non-empty lines before a function definition are most likely comments
for that function and thus relevant.  Include them in function context.

Such a non-empty line might also belong to the preceding function if
there is no separating blank line.  Stop extending the context upwards
also at the next function line to make sure only one extra function body
is shown at most.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 grep.c          | 27 +++++++++++++++++++++++----
 t/t7810-grep.sh |  2 +-
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index 2c55d10c55..a69c05edc2 100644
--- a/grep.c
+++ b/grep.c
@@ -1476,33 +1476,52 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
 	}
 }
 
+static int is_empty_line(const char *bol, const char *eol);
+
 static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 			     char *bol, char *end, unsigned lno)
 {
 	unsigned cur = lno, from = 1, funcname_lno = 0, orig_from;
-	int funcname_needed = !!opt->funcname;
+	int funcname_needed = !!opt->funcname, comment_needed = 0;
 
 	if (opt->pre_context < lno)
 		from = lno - opt->pre_context;
 	if (from <= opt->last_shown)
 		from = opt->last_shown + 1;
 	orig_from = from;
-	if (opt->funcbody && !match_funcname(opt, gs, bol, end)) {
-		funcname_needed = 1;
+	if (opt->funcbody) {
+		if (match_funcname(opt, gs, bol, end))
+			comment_needed = 1;
+		else
+			funcname_needed = 1;
 		from = opt->last_shown + 1;
 	}
 
 	/* Rewind. */
 	while (bol > gs->buf && cur > from) {
+		char *next_bol = bol;
 		char *eol = --bol;
 
 		while (bol > gs->buf && bol[-1] != '\n')
 			bol--;
 		cur--;
+		if (comment_needed && (is_empty_line(bol, eol) ||
+				       match_funcname(opt, gs, bol, eol))) {
+			comment_needed = 0;
+			from = orig_from;
+			if (cur < from) {
+				cur++;
+				bol = next_bol;
+				break;
+			}
+		}
 		if (funcname_needed && match_funcname(opt, gs, bol, eol)) {
 			funcname_lno = cur;
 			funcname_needed = 0;
-			from = orig_from;
+			if (opt->funcbody)
+				comment_needed = 1;
+			else
+				from = orig_from;
 		}
 	}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 632b905611..c02ca735b9 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -785,7 +785,7 @@ test_expect_success 'grep -W with userdiff' '
 	git grep -W echo >function-context-userdiff-actual
 '
 
-test_expect_failure ' includes preceding comment' '
+test_expect_success ' includes preceding comment' '
 	grep "# Say hello" function-context-userdiff-actual
 '
 
-- 
2.15.0

Re: [PATCH 6/6] grep: show non-empty lines before functions with -W

[Stefan Beller]

On Sat, Nov 18, 2017 at 10:08 AM, René Scharfe <l.s.r@web.de> wrote:
> Non-empty lines before a function definition are most likely comments
> for that function and thus relevant.  Include them in function context.
>
> Such a non-empty line might also belong to the preceding function if
> there is no separating blank line.  Stop extending the context upwards
> also at the next function line to make sure only one extra function body
> is shown at most.

Can we add another heuristic, that checks for common function body ends, e.g.
if the preceding line contains '}' but is not commented (the line doesn't
contain '*/' '//', '#'), we have a strong hint that it is a function, not an
additional comment.

Re: [PATCH 6/6] grep: show non-empty lines before functions with -W

[René Scharfe]

Am 20.11.2017 um 21:39 schrieb Stefan Beller:
> On Sat, Nov 18, 2017 at 10:08 AM, René Scharfe <l.s.r@web.de> wrote:
>> Non-empty lines before a function definition are most likely comments
>> for that function and thus relevant.  Include them in function context.
>>
>> Such a non-empty line might also belong to the preceding function if
>> there is no separating blank line.  Stop extending the context upwards
>> also at the next function line to make sure only one extra function body
>> is shown at most.
> 
> Can we add another heuristic, that checks for common function body ends, e.g.
> if the preceding line contains '}' but is not commented (the line doesn't
> contain '*/' '//', '#'), we have a strong hint that it is a function, not an
> additional comment.

C comments containing "}" as part of the text would only be shown
partially, e.g:

	/*
	 * Not shown because of the curly closing brace in ${PATH}.
	 * Shown.
	 */

Two examples in git's repo are in refs.h and sha1-lookup.c.

Before diving deeper: Is it worth it?  Does the heuristic in this series
produce excessive context often?  Enough to be annoying?

Thanks,
René

Re: [PATCH 6/6] grep: show non-empty lines before functions with -W

[Stefan Beller]

On Mon, Nov 20, 2017 at 2:07 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 20.11.2017 um 21:39 schrieb Stefan Beller:
>> On Sat, Nov 18, 2017 at 10:08 AM, René Scharfe <l.s.r@web.de> wrote:
>>> Non-empty lines before a function definition are most likely comments
>>> for that function and thus relevant.  Include them in function context.
>>>
>>> Such a non-empty line might also belong to the preceding function if
>>> there is no separating blank line.  Stop extending the context upwards
>>> also at the next function line to make sure only one extra function body
>>> is shown at most.
>>
>> Can we add another heuristic, that checks for common function body ends, e.g.
>> if the preceding line contains '}' but is not commented (the line doesn't
>> contain '*/' '//', '#'), we have a strong hint that it is a function, not an
>> additional comment.
>
> C comments containing "}" as part of the text would only be shown
> partially, e.g:
>
>         /*
>          * Not shown because of the curly closing brace in ${PATH}.
>          * Shown.
>          */
>
> Two examples in git's repo are in refs.h and sha1-lookup.c.
>
> Before diving deeper: Is it worth it?  Does the heuristic in this series
> produce excessive context often?  Enough to be annoying?

We'll find out... I was just spurting out my thought of the day.
Sorry for the noise.