{"thread":{"id":"46239","subject":"[PATCH] xdiff: trim common tail with -U0 after diff","startedAt":"2017-06-23T10:44:37Z","lastAt":"2017-06-23T23:04:51Z","messageCount":6,"participants":["Daniel Hahler","Junio C Hamano","Stefan Beller","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323093","messageId":"20170623103612.4694-1-git@thequod.de","threadId":"46239","inReplyTo":null,"subject":"[PATCH] xdiff: trim common tail with -U0 after diff","fromName":"Daniel Hahler","fromEmail":"git@thequod.de","sentAt":"2017-06-23T10:36:12Z","receivedAt":"2017-06-23T10:44:37Z","isPatch":true,"sender":{"key":"git@thequod.de","avatar":"https://gravatar.com/avatar/7ca36ccd9ac57f02a433e4ecf2e28e6e23abce8b005a408498f516b74232da67?d=mp&s=160"},"body":"When -U0 is used, trim_common_tail should be called after `xdl_diff`, so\nthat `--indent-heuristic` (and other diff processing) works as expected.\n\nIt also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`\nadded in e0876bca4, which does not appear to be necessary anymore after\nmoving the trimming down.\n\nThis only adds a test to t4061-diff-indent.sh, but should also have one for\nnormal (i.e. non-experimental) diff mode probably?!\n\nRef: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460\n---\n t/t4061-diff-indent.sh | 15 +++++++++++++++\n xdiff-interface.c | 7 ++++---\n 2 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 2affd7a10..df3151393 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -116,6 +116,16 @@ test_expect_success 'prepare' '\n \t 4\n \tEOF\n \n+\tcat <<-\\EOF >spaces-compacted-U0-expect &&\n+\tdiff --git a/spaces.txt b/spaces.txt\n+\t--- a/spaces.txt\n+\t+++ b/spaces.txt\n+\t@@ -4,0 +5,3 @@ a\n+\t+b\n+\t+a\n+\t+\n+\tEOF\n+\n \ttr \"_\" \" \" <<-\\EOF >functions-expect &&\n \tdiff --git a/functions.c b/functions.c\n \t--- a/functions.c\n@@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '\n \tcompare_diff spaces-compacted-expect out-compacted4\n '\n \n+test_expect_success 'diff: --indent-heuristic with -U0' '\n+\tgit diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&\n+\tcompare_diff spaces-compacted-U0-expect out-compacted5\n+'\n+\n test_expect_success 'diff: ugly functions' '\n \tgit diff --no-indent-heuristic old new -- functions.c >out &&\n \tcompare_diff functions-expect out\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex d3f78ca2a..a7e0e3583 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n \n int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n {\n+\tint ret;\n \tmmfile_t a = *mf1;\n \tmmfile_t b = *mf2;\n \n \tif (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)\n \t\treturn -1;\n \n-\tif (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))\n+\tret = xdl_diff(&a, &b, xpp, xecfg, xecb);\n+\tif (ret && !xecfg->ctxlen)\n \t\ttrim_common_tail(&a, &b);\n-\n-\treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n+\treturn ret;\n }\n \n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n-- \n2.13.1\n\n"},{"id":"323131","messageId":"xmqqinjmecuk.fsf@gitster.mtv.corp.google.com","threadId":"46239","inReplyTo":"20170623103612.4694-1-git@thequod.de","subject":"Re: [PATCH] xdiff: trim common tail with -U0 after diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T19:13:55Z","receivedAt":"2017-06-23T19:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Hahler writes:\n\n> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so\n> that `--indent-heuristic` (and other diff processing) works as expected.\n\nI thought common-tail trimming was a hack/optimization to avoid\nhaving to pass the whole thing to have xdl_diff() work on it? What\nwould we be gaining by unconditionally passing the whole thing first\nand then still trimming?\n\n> It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`\n> added in e0876bca4, which does not appear to be necessary anymore after\n> moving the trimming down.\n\nThe code was conditionally disabling the hack/optimization; with it\nunconditionally disabled, of course it wouldn't be needed, no?\n\nI could understand if the motivation and the proposed change were\n\"tail-trimming outlived its usefulness, so remove it altogether\", or\n\"trail-trimming negatively affects the output by robbing useful\ninformation that could be used by indent-heuristic, so disable it\nwhen the heuristic is in use\".\n\nBut neither is what this patch does, so I am sort of at loss.\n\n\n> --- a/t/t4061-diff-indent.sh\n> +++ b/t/t4061-diff-indent.sh\n> @@ -116,6 +116,16 @@ test_expect_success 'prepare' '\n> \t 4\n> \tEOF\n> \n> +\tcat <<-\\EOF >spaces-compacted-U0-expect &&\n> +\tdiff --git a/spaces.txt b/spaces.txt\n> +\t--- a/spaces.txt\n> +\t+++ b/spaces.txt\n> +\t@@ -4,0 +5,3 @@ a\n> +\t+b\n> +\t+a\n> +\t+\n> +\tEOF\n> +\n> \ttr \"_\" \" \" <<-\\EOF >functions-expect &&\n> \tdiff --git a/functions.c b/functions.c\n> \t--- a/functions.c\n> @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '\n> \tcompare_diff spaces-compacted-expect out-compacted4\n> '\n> \n> +test_expect_success 'diff: --indent-heuristic with -U0' '\n> +\tgit diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&\n> +\tcompare_diff spaces-compacted-U0-expect out-compacted5\n> +'\n> +\n> test_expect_success 'diff: ugly functions' '\n> \tgit diff --no-indent-heuristic old new -- functions.c >out &&\n> \tcompare_diff functions-expect out\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index d3f78ca2a..a7e0e3583 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n> \n> int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n> {\n> +\tint ret;\n> \tmmfile_t a = *mf1;\n> \tmmfile_t b = *mf2;\n> \n> \tif (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)\n> \t\treturn -1;\n> \n> -\tif (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))\n> +\tret = xdl_diff(&a, &b, xpp, xecfg, xecb);\n> +\tif (ret && !xecfg->ctxlen)\n> \t\ttrim_common_tail(&a, &b);\n> -\n> -\treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n> +\treturn ret;\n> }\n> \n> int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n\n\n"},{"id":"323133","messageId":"CAGZ79kZb+ziJGx6GvSVwX85Szce0zT=2gaF1GpPtz8ft1uhdPw@mail.gmail.com","threadId":"46239","inReplyTo":"xmqqinjmecuk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] xdiff: trim common tail with -U0 after diff","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T19:37:05Z","receivedAt":"2017-06-23T19:37:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 23, 2017 at 12:13 PM, Junio C Hamano wrote:\n> Daniel Hahler writes:\n>\n>> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so\n>> that `--indent-heuristic` (and other diff processing) works as expected.\n>\n> I thought common-tail trimming was a hack/optimization to avoid\n> having to pass the whole thing to have xdl_diff() work on it? What\n> would we be gaining by unconditionally passing the whole thing first\n> and then still trimming?\n\nSo I tried finding out more about this hack,\nand found the patch that introduced the common tail trimming at\n https://public-inbox.org/git/7vmysez0oa.fsf@gitster.siamese.dyndns.org/\n 913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13)\n\nIn the early days there were much larger email threads apparently.\nI haven't seen such a big one in a while. I did not find the email that\nthe commit message referenced to unfortunately.\n\nAFAICT we have 2 things going on:\n* swapping the order of xdl_diff and the tail trimming\n* change the condition under which the tail is trimmed\n (as a logical follow up/ cleanup because of the first point)\n\nI do not understand the first one, yet.\n"},{"id":"323135","messageId":"xmqqefuaebfr.fsf@gitster.mtv.corp.google.com","threadId":"46239","inReplyTo":"CAGZ79kZb+ziJGx6GvSVwX85Szce0zT=2gaF1GpPtz8ft1uhdPw@mail.gmail.com","subject":"Re: [PATCH] xdiff: trim common tail with -U0 after diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T19:44:24Z","receivedAt":"2017-06-23T19:44:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So I tried finding out more about this hack,\n> and found the patch that introduced the common tail trimming at\n> https://public-inbox.org/git/7vmysez0oa.fsf@gitster.siamese.dyndns.org/\n> 913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13)\n\nA relevant one that makes me hesitate to take this kind of change is\nthis:\n\nhttps://public-inbox.org/git/alpine.LFD.0.9999.0712202009290.21557@woody.linux-foundation.org/#t\n\nthat resulted in this change:\n\ncommit d2f82950a9226ae1102a7a97f03440a4bf8c6c09\nAuthor: Linus Torvalds \nDate: Thu Dec 20 20:22:46 2007 -0800\n\n Re(-re)*fix trim_common_tail()\n \n The tar-ball and the git archive itself is fine, but yes, the diff from\n 2.6.23 to 2.6.24-rc6 is bad. It's the \"trim_common_tail()\" optimization\n that has caused way too much pain.\n \n Very interesting breakage. The patch was actually \"correct\" in a (rather\n limited) technical sense, but the context at the end was missing because\n while the trim_common_tail() code made sure to keep enough common context\n to allow a valid diff to be generated, the diff machinery itself could\n decide that it could generate the diff differently than the \"obvious\"\n solution.\n \n Thee sad fact is that the git optimization (which is very important for\n \"git blame\", which needs no context), is only really valid for that one\n case where we really don't need any context.\n\n"},{"id":"323152","messageId":"fa350688-1831-f979-b984-3b6d54e32b8c@web.de","threadId":"46239","inReplyTo":"20170623103612.4694-1-git@thequod.de","subject":"Re: [PATCH] xdiff: trim common tail with -U0 after diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-23T20:39:36Z","receivedAt":"2017-06-23T20:40:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.06.2017 um 12:36 schrieb Daniel Hahler:\n> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so\n> that `--indent-heuristic` (and other diff processing) works as expected.\n> \n> It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`\n> added in e0876bca4, which does not appear to be necessary anymore after\n> moving the trimming down.\n> \n> This only adds a test to t4061-diff-indent.sh, but should also have one for\n> normal (i.e. non-experimental) diff mode probably?!\n> \n> Ref: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460\n> ---\n> t/t4061-diff-indent.sh | 15 +++++++++++++++\n> xdiff-interface.c | 7 ++++---\n> 2 files changed, 19 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\n> index 2affd7a10..df3151393 100755\n> --- a/t/t4061-diff-indent.sh\n> +++ b/t/t4061-diff-indent.sh\n> @@ -116,6 +116,16 @@ test_expect_success 'prepare' '\n> \t 4\n> \tEOF\n> \n> +\tcat <<-\\EOF >spaces-compacted-U0-expect &&\n> +\tdiff --git a/spaces.txt b/spaces.txt\n> +\t--- a/spaces.txt\n> +\t+++ b/spaces.txt\n> +\t@@ -4,0 +5,3 @@ a\n> +\t+b\n> +\t+a\n> +\t+\n> +\tEOF\n> +\n> \ttr \"_\" \" \" <<-\\EOF >functions-expect &&\n> \tdiff --git a/functions.c b/functions.c\n> \t--- a/functions.c\n> @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '\n> \tcompare_diff spaces-compacted-expect out-compacted4\n> '\n> \n> +test_expect_success 'diff: --indent-heuristic with -U0' '\n> +\tgit diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&\n> +\tcompare_diff spaces-compacted-U0-expect out-compacted5\n> +'\n> +\n> test_expect_success 'diff: ugly functions' '\n> \tgit diff --no-indent-heuristic old new -- functions.c >out &&\n> \tcompare_diff functions-expect out\n\nThe changed test script passes just fine for me even without your change\nto xdiff-interface.c, which is odd. Do you have another way to\ndemonstrate the unexpected behavior? And can someone replicate the\nfailure?\n\nRené\n"},{"id":"323168","messageId":"436f31fb-3a06-e6ca-094e-8e92aa1ee1d1@thequod.de","threadId":"46239","inReplyTo":"fa350688-1831-f979-b984-3b6d54e32b8c@web.de","subject":"Re: [PATCH] xdiff: trim common tail with -U0 after diff","fromName":"Daniel Hahler","fromEmail":"git@thequod.de","sentAt":"2017-06-23T22:57:57Z","receivedAt":"2017-06-23T23:04:51Z","isPatch":true,"sender":{"key":"git@thequod.de","avatar":"https://gravatar.com/avatar/7ca36ccd9ac57f02a433e4ecf2e28e6e23abce8b005a408498f516b74232da67?d=mp&s=160"},"body":"On 23.06.2017 22:39, René Scharfe wrote:\n\n> The changed test script passes just fine for me even without your change\n> to xdiff-interface.c, which is odd.\n\nSorry, I've apparently messed this up - it seems to be the case for me, too.\n\nI would assume that using the functions.c context/diff in this test file might prove it, but when I wrote this test I was already unsure to base it on an experimental feature, that might just get removed again (with its tests).\n\n> Do you have another way to demonstrate the unexpected behavior?\n\nIt was clearly reproducible with a local test case, based on \"copying an existing test case, and modifying the header for it only\".\n\nGiven the other replies, it seems like it is more a question now if the trimming should get moved down, or removed completely it seems.\nSo I will not update the patch/test for now.\n\n\n-- \nhttp://daniel.hahler.de/\n\n"}]}