[PATCH 1/3] xdiff: -W: relax end-of-file function detection

When adding a new function to the end of a file, it's enough to know
that 1) the addition is at the end of the file; and 2) there is a
function _somewhere_ in there.

If we had simply been changing the end of an existing function, then we
would also be deleting something from the old version.

This fixes the case where we add e.g.

	// Begin of dummy
	static int dummy(void)
	{
	}

to the end of the file.

Cc: René Scharfe <redacted>
Signed-off-by: Vegard Nossum <redacted>
---
 xdiff/xemit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 7389ce4..8c88dbd 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -183,16 +183,14 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 
 				/*
 				 * We don't need additional context if
-				 * a whole function was added, possibly
-				 * starting with empty lines.
+				 * a whole function was added.
 				 */
-				while (i2 < xe->xdf2.nrec &&
-				       is_empty_rec(&xe->xdf2, i2))
+				while (i2 < xe->xdf2.nrec) {
+					if (match_func_rec(&xe->xdf2, xecfg, i2,
+						dummy, sizeof(dummy)) >= 0)
+						goto post_context_calculation;
 					i2++;
-				if (i2 < xe->xdf2.nrec &&
-				    match_func_rec(&xe->xdf2, xecfg, i2,
-						   dummy, sizeof(dummy)) >= 0)
-					goto post_context_calculation;
+				}
 
 				/*
 				 * Otherwise get more context from the

-- 
2.7.4

When using -W to include the whole function in the diff context, you
are typically doing this to be able to review the change in its entirety
within the context of the function. It is therefore almost always
desirable to include any comments that immediately precede the function.

This also the fixes the case for C where the declaration is split across
multiple lines (where the first line of the declaration would not be
included in the output), e.g.:

	void
	dummy(void)
	{
		...
	}

We can include these lines by simply scanning upwards from the place of
the detected function start until we hit the first non-blank line.

Cc: René Scharfe <redacted>
Signed-off-by: Vegard Nossum <redacted>
---
 xdiff/xemit.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 8c88dbd..3a3060d 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -200,6 +200,8 @@ 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))
+				fs1--;
 			if (fs1 < 0)
 				fs1 = 0;
 			if (fs1 < s1) {

@@ -220,6 +222,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			long fe1 = get_func_line(xe, xecfg, NULL,
 						 xche->i1 + xche->chg1,
 						 xe->xdf1.nrec);
+			while (fe1 > 0 && !is_empty_rec(&xe->xdf1, fe1 - 1))
+				fe1--;
 			while (fe1 > 0 && is_empty_rec(&xe->xdf1, fe1 - 1))
 				fe1--;
 			if (fe1 < 0)

-- 
2.7.4

Am 13.01.2017 um 17:15 schrieb Vegard Nossum:

When using -W to include the whole function in the diff context, you
are typically doing this to be able to review the change in its entirety
within the context of the function. It is therefore almost always
desirable to include any comments that immediately precede the function.

This also the fixes the case for C where the declaration is split across
multiple lines (where the first line of the declaration would not be
included in the output), e.g.:

	void
	dummy(void)
	{
		...
	}

That's true, but I'm not sure "non-empty line before function line" is 
good enough a definition for desirable lines.  It wouldn't work for 
people who don't believe in empty lines.  Or for those that put a blank 
line between comment and function.  (I have an opinion on such habits, 
but git diff should probably stay neutral.)  And that's just for C code; 
I have no idea how this heuristic would hold up for other file types 
like HTML.

We can identify function lines with arbitrary precision (with a 
xfuncname regex, if needed), but there is no accurate way to classify 
lines as comments, or as the end of functions.  Adding optional regexes 
for single- and multi-line comments would help, at least for C.

René

On Fri, Jan 13, 2017 at 10:19 AM, René Scharfe [off-list ref] wrote:

Am 13.01.2017 um 17:15 schrieb Vegard Nossum:

quoted

When using -W to include the whole function in the diff context, you
are typically doing this to be able to review the change in its entirety
within the context of the function. It is therefore almost always
desirable to include any comments that immediately precede the function.

Do we need a small comment in the actual code to hint at why we count
upwards there?

quoted

This also the fixes the case for C where the declaration is split across
multiple lines (where the first line of the declaration would not be
included in the output), e.g.:

        void
        dummy(void)
        {
                ...
        }

That's true, but I'm not sure "non-empty line before function line" is good
enough a definition for desirable lines.  It wouldn't work for people who
don't believe in empty lines.  Or for those that put a blank line between
comment and function.  (I have an opinion on such habits, but git diff
should probably stay neutral.)  And that's just for C code; I have no idea
how this heuristic would hold up for other file types like HTML.

I think empty lines are "good as a first approach", see e.g.
433860f3d0beb0c6 the "compaction" heuristic for a similar
thing (the compaction was introduced at d634d61ed), and then
we can build a more elaborate thing on top.

We can identify function lines with arbitrary precision (with a xfuncname
regex, if needed), but there is no accurate way to classify lines as
comments, or as the end of functions.  Adding optional regexes for single-
and multi-line comments would help, at least for C.

That would cover Java and whole lot of other C like languages. So a good
start as well IMHO.

René

We now include non-empty lines immediately before (and after) a function
as belonging to the function.

We can test this new functionality by moving the "// Begin" markers on
each function to the previous line.

This commit is intentionally not part of the previous commits in order
to show that the tests do not break even when changing the behaviour of
'diff -W' in the previous commits.

Cc: René Scharfe <redacted>
Signed-off-by: Vegard Nossum <redacted>
---
 t/t4051-diff-function-context.sh | 2 +-
 t/t4051/appended1.c              | 3 ++-
 t/t4051/dummy.c                  | 3 ++-
 t/t4051/hello.c                  | 3 ++-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 6154acb..d1a646f 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh

@@ -72,7 +72,7 @@ test_expect_success 'setup' '
 
 	# overlap function context of 1st change and -u context of 2nd change
 	grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
-	sed 2p <"$dir/dummy.c" >>file.c &&
+	sed 3p <"$dir/dummy.c" >>file.c &&
 	commit_and_tag changed_hello_dummy file.c &&
 
 	git checkout initial &&

diff --git a/t/t4051/appended1.c b/t/t4051/appended1.c
index a9f56f1..8683983 100644
--- a/t/t4051/appended1.c
+++ b/t/t4051/appended1.c

@@ -1,5 +1,6 @@
 
-int appended(void) // Begin of first part
+// Begin of first part
+int appended(void)
 {
 	int i;
 	char *s = "a string";

diff --git a/t/t4051/dummy.c b/t/t4051/dummy.c
index a43016e..db227a8 100644
--- a/t/t4051/dummy.c
+++ b/t/t4051/dummy.c

@@ -1,5 +1,6 @@
 
-static int dummy(void)	// Begin of dummy
+// Begin of dummy
+static int dummy(void)
 {
 	int rc = 0;
 

diff --git a/t/t4051/hello.c b/t/t4051/hello.c
index 63b1a1e..75eac1d 100644
--- a/t/t4051/hello.c
+++ b/t/t4051/hello.c

@@ -1,5 +1,6 @@
 
-static void hello(void)	// Begin of hello
+// Begin of hello
+static void hello(void)
 {
 	/*
 	 * Classic.

-- 
2.7.4

Am 13.01.2017 um 17:15 schrieb Vegard Nossum:

When adding a new function to the end of a file, it's enough to know
that 1) the addition is at the end of the file; and 2) there is a
function _somewhere_ in there.

If we had simply been changing the end of an existing function, then we
would also be deleting something from the old version.

That makes sense, thanks.

This fixes the case where we add e.g.

	// Begin of dummy
	static int dummy(void)
	{
	}

to the end of the file.

Without this patch the unchanged function before the added lines is 
shown in its entirety as (uncalled for) context.

Nit: I don't like the indentation here.  Giving "dummy" its own line is 
also not exactly pretty, but at least would allow the parameters to be 
aligned on the opening parenthesis.

+						goto post_context_calculation;
 					i2++;
-				if (i2 < xe->xdf2.nrec &&
-				    match_func_rec(&xe->xdf2, xecfg, i2,
-						   dummy, sizeof(dummy)) >= 0)
-					goto post_context_calculation;
+				}

 				/*
 				 * Otherwise get more context from the