[PATCH] xdiff: unignore changes in function context

[PATCH] xdiff: unignore changes in function context

[René Scharfe]

Changes involving only blank lines are hidden with --ignore-blank-lines,
unless they appear in the context lines of other changes.  This is
handled by xdl_get_hunk() for context added by --inter-hunk-context, -u
and -U.

Function context for -W and --function-context added by xdl_emit_diff()
doesn't pay attention to such ignored changes; it relies fully on
xdl_get_hunk() and shows just the post-image of ignored changes
appearing in function context.  That's inconsistent and confusing.

Improve the result of using --ignore-blank-lines and --function-context
together by fully showing ignored changes if they happen to fall within
function context.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t4015-diff-whitespace.sh |  6 +-----
 xdiff/xemit.c              | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index eadaf57262..5888ae5ed3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -2025,11 +2025,6 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
 	test_cmp expected actual
 '

-# Note that the "6" in the expected hunk header below is funny, since we only
-# show 5 lines (the missing one was blank and thus ignored). This is how
-# --ignore-blank-lines behaves even without --function-context, and this test
-# is just checking the interaction of the two features. Don't take it as an
-# endorsement of that output.
 test_expect_success 'combine --ignore-blank-lines with --function-context' '
 	test_write_lines 1 "" 2 3 4 5 >a &&
 	test_write_lines 1    2 3 4   >b &&
@@ -2039,6 +2034,7 @@ test_expect_success 'combine --ignore-blank-lines with --function-context' '
 	cat <<-\EOF >expect &&
 	@@ -1,6 +1,4 @@
 	 1
+	-
 	 2
 	 3
 	 4
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 30713ae9a9..9d7d6c5087 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -172,10 +172,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 	struct func_line func_line = { 0 };

 	for (xch = xscr; xch; xch = xche->next) {
+		xdchange_t *xchp = xch;
 		xche = xdl_get_hunk(&xch, xecfg);
 		if (!xch)
 			break;

+pre_context_calculation:
 		s1 = XDL_MAX(xch->i1 - xecfg->ctxlen, 0);
 		s2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);

@@ -212,6 +214,21 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			if (fs1 < s1) {
 				s2 = XDL_MAX(s2 - (s1 - fs1), 0);
 				s1 = fs1;
+
+				/*
+				 * Did we extend context upwards into an
+				 * ignored change?
+				 */
+				while (xchp != xch &&
+				       xchp->i1 + xchp->chg1 <= s1 &&
+				       xchp->i2 + xchp->chg2 <= s2)
+					xchp = xchp->next;
+
+				/* If so, show it after all. */
+				if (xchp != xch) {
+					xch = xchp;
+					goto pre_context_calculation;
+				}
 			}
 		}

--
2.24.0

Re: [PATCH] xdiff: unignore changes in function context

[Junio C Hamano]

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

> Changes involving only blank lines are hidden with --ignore-blank-lines,
> unless they appear in the context lines of other changes.  This is
> handled by xdl_get_hunk() for context added by --inter-hunk-context, -u
> and -U.
>
> Function context for -W and --function-context added by xdl_emit_diff()
> doesn't pay attention to such ignored changes; it relies fully on
> xdl_get_hunk() and shows just the post-image of ignored changes
> appearing in function context.  That's inconsistent and confusing.
>
> Improve the result of using --ignore-blank-lines and --function-context
> together by fully showing ignored changes if they happen to fall within
> function context.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  t/t4015-diff-whitespace.sh |  6 +-----
>  xdiff/xemit.c              | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index eadaf57262..5888ae5ed3 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -2025,11 +2025,6 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
>  	test_cmp expected actual
>  '
>
> -# Note that the "6" in the expected hunk header below is funny, since we only
> -# show 5 lines (the missing one was blank and thus ignored). This is how
> -# --ignore-blank-lines behaves even without --function-context, and this test
> -# is just checking the interaction of the two features. Don't take it as an
> -# endorsement of that output.

Nice to see that somebody anticipated that we may fix this some day.

> @@ -2039,6 +2034,7 @@ test_expect_success 'combine --ignore-blank-lines with --function-context' '
>  	cat <<-\EOF >expect &&
>  	@@ -1,6 +1,4 @@
>  	 1
> +	-
>  	 2
>  	 3
>  	 4

And I think this is a more intuitive outcome people would expect.

> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> index 30713ae9a9..9d7d6c5087 100644
> --- a/xdiff/xemit.c
> +++ b/xdiff/xemit.c
> @@ -172,10 +172,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
>  	struct func_line func_line = { 0 };
>
>  	for (xch = xscr; xch; xch = xche->next) {
> +		xdchange_t *xchp = xch;
>  		xche = xdl_get_hunk(&xch, xecfg);
>  		if (!xch)
>  			break;
>
> +pre_context_calculation:
>  		s1 = XDL_MAX(xch->i1 - xecfg->ctxlen, 0);
>  		s2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);
>
> @@ -212,6 +214,21 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
>  			if (fs1 < s1) {
>  				s2 = XDL_MAX(s2 - (s1 - fs1), 0);
>  				s1 = fs1;
> +
> +				/*
> +				 * Did we extend context upwards into an
> +				 * ignored change?
> +				 */
> +				while (xchp != xch &&
> +				       xchp->i1 + xchp->chg1 <= s1 &&
> +				       xchp->i2 + xchp->chg2 <= s2)
> +					xchp = xchp->next;
> +
> +				/* If so, show it after all. */
> +				if (xchp != xch) {
> +					xch = xchp;
> +					goto pre_context_calculation;
> +				}
>  			}
>  		}
>
> --
> 2.24.0

Re: [PATCH] xdiff: unignore changes in function context

[Jeff King]

On Thu, Dec 05, 2019 at 09:29:46AM -0800, Junio C Hamano wrote:

> > -# Note that the "6" in the expected hunk header below is funny, since we only
> > -# show 5 lines (the missing one was blank and thus ignored). This is how
> > -# --ignore-blank-lines behaves even without --function-context, and this test
> > -# is just checking the interaction of the two features. Don't take it as an
> > -# endorsement of that output.
> 
> Nice to see that somebody anticipated that we may fix this some day.

Or that somebody just didn't want to be embarrassed by introducing such
obvious nonsense into the test suite. :)

I was curious, though, whether there was still a lurking bug in
"--ignore-blank-lines", based on what that comment says. But I don't
think so. It reports the correct numbers for this test case, but that's
because the blank line drops off the context. If we add -U4, then it
does mention 6 lines in the preimage, and includes the line.

Which matches what René claimed in the commit message: "Changes
involving only blank lines are hidden with --ignore-blank-lines, unless
they appear in the context lines of other changes." But now I've
double-checked. :)

(And I agree that the output after this patch is way better).

-Peff