{"thread":{"id":"52388","subject":"[PATCH] xdiff: unignore changes in function context","startedAt":"2019-12-05T16:15:52Z","lastAt":"2019-12-05T21:09:47Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387562","messageId":"3053f7a8-0723-aaa7-fe43-9b8b13b2e259@web.de","threadId":"52388","inReplyTo":null,"subject":"[PATCH] xdiff: unignore changes in function context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-12-05T16:15:31Z","receivedAt":"2019-12-05T16:15:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes involving only blank lines are hidden with --ignore-blank-lines,\nunless they appear in the context lines of other changes.  This is\nhandled by xdl_get_hunk() for context added by --inter-hunk-context, -u\nand -U.\n\nFunction context for -W and --function-context added by xdl_emit_diff()\ndoesn't pay attention to such ignored changes; it relies fully on\nxdl_get_hunk() and shows just the post-image of ignored changes\nappearing in function context.  That's inconsistent and confusing.\n\nImprove the result of using --ignore-blank-lines and --function-context\ntogether by fully showing ignored changes if they happen to fall within\nfunction context.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t4015-diff-whitespace.sh |  6 +-----\n xdiff/xemit.c              | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex eadaf57262..5888ae5ed3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -2025,11 +2025,6 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \ttest_cmp expected actual\n '\n\n-# Note that the \"6\" in the expected hunk header below is funny, since we only\n-# show 5 lines (the missing one was blank and thus ignored). This is how\n-# --ignore-blank-lines behaves even without --function-context, and this test\n-# is just checking the interaction of the two features. Don't take it as an\n-# endorsement of that output.\n test_expect_success 'combine --ignore-blank-lines with --function-context' '\n \ttest_write_lines 1 \"\" 2 3 4 5 >a &&\n \ttest_write_lines 1    2 3 4   >b &&\n@@ -2039,6 +2034,7 @@ test_expect_success 'combine --ignore-blank-lines with --function-context' '\n \tcat <<-\\EOF >expect &&\n \t@@ -1,6 +1,4 @@\n \t 1\n+\t-\n \t 2\n \t 3\n \t 4\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 30713ae9a9..9d7d6c5087 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -172,10 +172,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \tstruct func_line func_line = { 0 };\n\n \tfor (xch = xscr; xch; xch = xche->next) {\n+\t\txdchange_t *xchp = xch;\n \t\txche = xdl_get_hunk(&xch, xecfg);\n \t\tif (!xch)\n \t\t\tbreak;\n\n+pre_context_calculation:\n \t\ts1 = XDL_MAX(xch->i1 - xecfg->ctxlen, 0);\n \t\ts2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);\n\n@@ -212,6 +214,21 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\tif (fs1 < s1) {\n \t\t\t\ts2 = XDL_MAX(s2 - (s1 - fs1), 0);\n \t\t\t\ts1 = fs1;\n+\n+\t\t\t\t/*\n+\t\t\t\t * Did we extend context upwards into an\n+\t\t\t\t * ignored change?\n+\t\t\t\t */\n+\t\t\t\twhile (xchp != xch &&\n+\t\t\t\t       xchp->i1 + xchp->chg1 <= s1 &&\n+\t\t\t\t       xchp->i2 + xchp->chg2 <= s2)\n+\t\t\t\t\txchp = xchp->next;\n+\n+\t\t\t\t/* If so, show it after all. */\n+\t\t\t\tif (xchp != xch) {\n+\t\t\t\t\txch = xchp;\n+\t\t\t\t\tgoto pre_context_calculation;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n\n--\n2.24.0\n"},{"id":"387567","messageId":"xmqq5ziu1y0l.fsf@gitster-ct.c.googlers.com","threadId":"52388","inReplyTo":"3053f7a8-0723-aaa7-fe43-9b8b13b2e259@web.de","subject":"Re: [PATCH] xdiff: unignore changes in function context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-05T17:29:46Z","receivedAt":"2019-12-05T17:29:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Changes involving only blank lines are hidden with --ignore-blank-lines,\n> unless they appear in the context lines of other changes.  This is\n> handled by xdl_get_hunk() for context added by --inter-hunk-context, -u\n> and -U.\n>\n> Function context for -W and --function-context added by xdl_emit_diff()\n> doesn't pay attention to such ignored changes; it relies fully on\n> xdl_get_hunk() and shows just the post-image of ignored changes\n> appearing in function context.  That's inconsistent and confusing.\n>\n> Improve the result of using --ignore-blank-lines and --function-context\n> together by fully showing ignored changes if they happen to fall within\n> function context.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  t/t4015-diff-whitespace.sh |  6 +-----\n>  xdiff/xemit.c              | 17 +++++++++++++++++\n>  2 files changed, 18 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index eadaf57262..5888ae5ed3 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -2025,11 +2025,6 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n>  \ttest_cmp expected actual\n>  '\n>\n> -# Note that the \"6\" in the expected hunk header below is funny, since we only\n> -# show 5 lines (the missing one was blank and thus ignored). This is how\n> -# --ignore-blank-lines behaves even without --function-context, and this test\n> -# is just checking the interaction of the two features. Don't take it as an\n> -# endorsement of that output.\n\nNice to see that somebody anticipated that we may fix this some day.\n\n> @@ -2039,6 +2034,7 @@ test_expect_success 'combine --ignore-blank-lines with --function-context' '\n>  \tcat <<-\\EOF >expect &&\n>  \t@@ -1,6 +1,4 @@\n>  \t 1\n> +\t-\n>  \t 2\n>  \t 3\n>  \t 4\n\nAnd I think this is a more intuitive outcome people would expect.\n\n> diff --git a/xdiff/xemit.c b/xdiff/xemit.c\n> index 30713ae9a9..9d7d6c5087 100644\n> --- a/xdiff/xemit.c\n> +++ b/xdiff/xemit.c\n> @@ -172,10 +172,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n>  \tstruct func_line func_line = { 0 };\n>\n>  \tfor (xch = xscr; xch; xch = xche->next) {\n> +\t\txdchange_t *xchp = xch;\n>  \t\txche = xdl_get_hunk(&xch, xecfg);\n>  \t\tif (!xch)\n>  \t\t\tbreak;\n>\n> +pre_context_calculation:\n>  \t\ts1 = XDL_MAX(xch->i1 - xecfg->ctxlen, 0);\n>  \t\ts2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);\n>\n> @@ -212,6 +214,21 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n>  \t\t\tif (fs1 < s1) {\n>  \t\t\t\ts2 = XDL_MAX(s2 - (s1 - fs1), 0);\n>  \t\t\t\ts1 = fs1;\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t * Did we extend context upwards into an\n> +\t\t\t\t * ignored change?\n> +\t\t\t\t */\n> +\t\t\t\twhile (xchp != xch &&\n> +\t\t\t\t       xchp->i1 + xchp->chg1 <= s1 &&\n> +\t\t\t\t       xchp->i2 + xchp->chg2 <= s2)\n> +\t\t\t\t\txchp = xchp->next;\n> +\n> +\t\t\t\t/* If so, show it after all. */\n> +\t\t\t\tif (xchp != xch) {\n> +\t\t\t\t\txch = xchp;\n> +\t\t\t\t\tgoto pre_context_calculation;\n> +\t\t\t\t}\n>  \t\t\t}\n>  \t\t}\n>\n> --\n> 2.24.0\n"},{"id":"387584","messageId":"20191205210945.GA49397@coredump.intra.peff.net","threadId":"52388","inReplyTo":"xmqq5ziu1y0l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] xdiff: unignore changes in function context","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-05T21:09:45Z","receivedAt":"2019-12-05T21:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 05, 2019 at 09:29:46AM -0800, Junio C Hamano wrote:\n\n> > -# Note that the \"6\" in the expected hunk header below is funny, since we only\n> > -# show 5 lines (the missing one was blank and thus ignored). This is how\n> > -# --ignore-blank-lines behaves even without --function-context, and this test\n> > -# is just checking the interaction of the two features. Don't take it as an\n> > -# endorsement of that output.\n> \n> Nice to see that somebody anticipated that we may fix this some day.\n\nOr that somebody just didn't want to be embarrassed by introducing such\nobvious nonsense into the test suite. :)\n\nI was curious, though, whether there was still a lurking bug in\n\"--ignore-blank-lines\", based on what that comment says. But I don't\nthink so. It reports the correct numbers for this test case, but that's\nbecause the blank line drops off the context. If we add -U4, then it\ndoes mention 6 lines in the preimage, and includes the line.\n\nWhich matches what René claimed in the commit message: \"Changes\ninvolving only blank lines are hidden with --ignore-blank-lines, unless\nthey appear in the context lines of other changes.\" But now I've\ndouble-checked. :)\n\n(And I agree that the output after this patch is way better).\n\n-Peff\n"}]}