threads / patch / 52388

patchxdiff: unignore changes in function context

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

## tl;dr

3 messages between Dec 5, 2019 and Dec 5, 2019. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

René Scharfe· Dec 5, 2019, 16:15 UTC · lore

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(-)
Show changes to 2 files +18 −6

t/t4015-diff-whitespace.sh, xdiff/xemit.c

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
Junio C Hamano· Dec 5, 2019, 17:29 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 33 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
Show 41 quoted lines
> 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
Jeff King· Dec 5, 2019, 21:09 UTC · re: Junio C Hamano · lore

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

On Thu, Dec 05, 2019 at 09:29:46AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> > -# 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

← back to recent threads