{"thread":{"id":"64586","subject":"[PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","startedAt":"2025-12-06T20:51:32Z","lastAt":"2026-03-19T23:30:46Z","messageCount":17,"participants":["Yee Cheng Chin via GitGitGadget","Junio C Hamano","Phillip Wood","Yee Cheng Chin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"531782","messageId":"pull.2120.git.git.1765054287938.gitgitgadget@gmail.com","threadId":"64586","inReplyTo":null,"subject":"[PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-12-06T20:51:27Z","receivedAt":"2025-12-06T20:51:32Z","isPatch":true,"sender":{"key":"ychin.macvim@gmail.com","avatar":null},"body":"From: Yee Cheng Chin \n\nAfter a diff algorithm has been run, the compaction phase\n(xdl_change_compact()) shifts and merges change groups to produce a\ncleaner output. However, this shifting could create a new matched group\nwhere both sides now have matching lines. This results in a\nwrong-looking diff output which contains redundant lines that are the\nsame on both files.\n\nFix this by detecting this situation, and re-diff the texts on each side\nto find similar lines, using the fall-back Myer's diff. Only do this for\nhistogram diff as it's the only algorithm where this is relevant. Below\ncontains an example, and more details.\n\nFor an example, consider two files below:\n\n file1:\n A\n\n A\n A\n A\n\n A\n A\n A\n\n file2:\n A\n\n A\n x\n A\n\n A\n A\n A\n\nWhen using Myer's diff, the algorithm finds that only the \"x\" has been\nchanged, and produces a final diff result (these are line diffs, but\nusing word-diff syntax for ease of presentation):\n\n A A[-A-]{+x+}A AAA\n\nWhen using histogram diff, the algorithm first discovers the LCS \"A\nAAA\", which it uses as anchor, then produces an intermediate diff:\n\n {+A Ax+}A AAA[- AAA-].\n\nThis is a longer diff than Myer's, but it's still self-consistent.\nHowever, the compaction phase attempts to shift the first file's diff\ngroup upwards (note that this shift crosses the anchor that histogram\nhad used), leading to the final results for histogram diff:\n\n [-A AA-]{+A Ax+}A AAA\n\nThis is a technically correct patch but looks clearly redundant to a\nhuman as the first 3 lines should not be in the diff.\n\nThe fix would detect that a shift has caused matching to a new group,\nand re-diff the \"A AA\" and \"A Ax\" parts, which results in \"A A\"\ncorrectly re-marked as unchanged. This creates the now correct histogram\ndiff:\n\n A A[-A-]{+x+}A AAA\n\nThis issue is not applicable to Myer's diff algorithm as it already\ngenerates a minimal diff, which means a shift cannot result in a smaller\ndiff output (the default Myer's diff in xdiff is not guaranteed to be\nminimal for performance reasons, but it typically does a good enough\njob).\n\nIt's also not applicable to patience diff, because it uses only unique\nlines as anchor for its splits, and falls back to Myer's diff within\neach split. Shifting requires both ends having the same lines, and\ntherefore cannot cross the unique line boundaries established by the\npatience algorithm. In contrast histogram diff uses non-unique lines as\nanchors, and therefore shifting can cross over them.\n\nThis issue is rare in a normal repository. Below is a table of\nrepositories (`git log --no-merges -p --histogram -1000`), showing how\nmany times a re-diff was done and how many times it resulted in finding\nmatching lines (therefore addressing this issue) with the fix. In\ngeneral it is fewer than 1% of diff's that exhibit this offending\nbehavior:\n\n| Repo (1k commits) | Re-diff | Found matching lines |\n|--------------------|---------|----------------------|\n| llvm-project | 45 | 11 |\n| vim | 110 | 9 |\n| git | 18 | 2 |\n| WebKit | 168 | 1 |\n| ripgrep | 22 | 1 |\n| cpython | 32 | 0 |\n| vscode | 13 | 0 |\n\nSigned-off-by: Yee Cheng Chin \n---\n xdiff: re-diff shifted change groups when using histogram algorithm\n \n This is a somewhat rare issue when using histogram to diff files, as the\n algorithm will generate a diff output that looks redundant and wrong to\n a human. I provided a synthetic example in the commit message, but for\n one from the real world, do the following command in the Git repo:\n \n git show -U0 --diff-algorithm=histogram 2c8999027c -- po/ga.po\n \n \n Scroll to the line \"@@ -7239,3 +5831,5 @@\", and we can see the following\n diff hunk:\n \n -#: builtin/diff.c\n -msgid \"Not a git repository\"\n -msgstr \"Ní stór git\"\n +msgid \"cannot come back to cwd\"\n +msgstr \"ní féidir teacht ar ais chuig cwd\"\n +\n +msgid \"Not a git repository\"\n +msgstr \"Ní stór git é\"\n \n \n We can see that the \"Not a git repository\" line is identical on both\n sides, which means it should not have been in the diff results to begin\n with. Under other diff algorithms (or histogram diff with this fix),\n said line is not considered to be part of the diff.\n \n Also, when I was implementing this, an alternative I was considering was\n to add a bespoke linear-time algorithm to remove matching lines on both\n sides. Just calling the fall-back diff seems the easiest and cleanest\n and so I went with that.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2120%2Fychin%2Fxdiff-fix-compact-remove-redundant-lines-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2120/ychin/xdiff-fix-compact-remove-redundant-lines-v1\nPull-Request: https://github.com/git/git/pull/2120\n\n t/meson.build | 1 +\n t/t4073-diff-shifted-matched-group.sh | 137 ++++++++++++++++++++++++++\n xdiff/xdiffi.c | 43 ++++++++\n 3 files changed, 181 insertions(+)\n create mode 100755 t/t4073-diff-shifted-matched-group.sh\n\ndiff --git a/t/meson.build b/t/meson.build\nindex 7c994d4643..ee233e80da 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -497,6 +497,7 @@ integration_tests = [\n 't4070-diff-pairs.sh',\n 't4071-diff-minimal.sh',\n 't4072-diff-max-depth.sh',\n+ 't4073-diff-shifted-matched-group.sh',\n 't4100-apply-stat.sh',\n 't4101-apply-nonl.sh',\n 't4102-apply-rename.sh',\ndiff --git a/t/t4073-diff-shifted-matched-group.sh b/t/t4073-diff-shifted-matched-group.sh\nnew file mode 100755\nindex 0000000000..0e915b78a6\n--- /dev/null\n+++ b/t/t4073-diff-shifted-matched-group.sh\n@@ -0,0 +1,137 @@\n+#!/bin/sh\n+\n+test_description='shifted diff groups re-diffing during histogram diff'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'shifted diff group should re-diff to minimize patch' '\n+\ttest_write_lines A x A A A x A A A >file1 &&\n+\ttest_write_lines A x A Z A x A A A >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,7 +1,7 @@\n+\t A\n+\t x\n+\t A\n+\t-A\n+\t+Z\n+\t A\n+\t x\n+\t A\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 're-diff should preserve diff flags' '\n+\ttest_write_lines a b c a b c >file1 &&\n+\ttest_write_lines x \" b\" z a b c >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,6 +1,6 @@\n+\t-a\n+\t-b\n+\t-c\n+\t+x\n+\t+ b\n+\t+z\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect_iwhite <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,6 +1,6 @@\n+\t-a\n+\t+x\n+\t b\n+\t-c\n+\t+z\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram --ignore-all-space file1 file2 >output_iwhite &&\n+\ttest_cmp expect_iwhite output_iwhite\n+'\n+\n+test_expect_success 'shifting on either side should trigger re-diff properly' '\n+\ttest_write_lines a b c a b c a b c >file1 &&\n+\ttest_write_lines a b c a1 a2 a3 b c1 a b c >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect1 <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,9 +1,11 @@\n+\t a\n+\t b\n+\t c\n+\t-a\n+\t+a1\n+\t+a2\n+\t+a3\n+\t b\n+\t-c\n+\t+c1\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output1 &&\n+\ttest_cmp expect1 output1 &&\n+\n+\tcat >expect2 <<-EOF &&\n+\tdiff --git a/file2 b/file1\n+\tindex $file2_h..$file1_h 100644\n+\t--- a/file2\n+\t+++ b/file1\n+\t@@ -1,11 +1,9 @@\n+\t a\n+\t b\n+\t c\n+\t-a1\n+\t-a2\n+\t-a3\n+\t+a\n+\t b\n+\t-c1\n+\t+c\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file2 file1 >output2 &&\n+\ttest_cmp expect2 output2\n+'\n+\n+test_done\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 6f3998ee54..5d9c7b5434 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -793,6 +793,7 @@ static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)\n */\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tstruct xdlgroup g, go;\n+\tstruct xdlgroup g_orig, go_orig;\n \tlong earliest_end, end_matching_other;\n \tlong groupsize;\n \n@@ -806,6 +807,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\tif (g.end == g.start)\n \t\t\tgoto next;\n \n+\t\tg_orig = g;\n+\t\tgo_orig = go;\n+\n \t\t/*\n \t\t * Now shift the change up and then down as far as possible in\n \t\t * each direction. If it bumps into any other changes, merge\n@@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t}\n \t\t}\n \n+\t\t/*\n+\t\t * If this has a matching group from the other file, it could\n+\t\t * either be the original match from the diff algorithm, or\n+\t\t * arrived at by shifting and joining groups. When it's the\n+\t\t * latter, it's possible for the two newly joined sides to have\n+\t\t * matching lines. Re-diff the group to mark these matching\n+\t\t * lines as unchanged and remove from the diff output.\n+\t\t *\n+\t\t * Only do this for histogram diff as its LCS algorithm makes\n+\t\t * this scenario possible. In contrast, patience diff finds LCS\n+\t\t * of unique lines that groups cannot be shifted across.\n+\t\t * Myer's diff (standalone or used as fall-back in patience\n+\t\t * diff) already finds minimal edits so it is not possible for\n+\t\t * shifted groups to result in a smaller diff. (Without\n+\t\t * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be\n+\t\t * minimal, but it should be so most of the time)\n+\t\t */\n+\t\tif (end_matching_other != -1 &&\n+\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n+\t\t\t\t(g.start != g_orig.start ||\n+\t\t\t\t g.end != g_orig.end ||\n+\t\t\t\t go.start != go_orig.start ||\n+\t\t\t\t go.end != go_orig.end)) {\n+\t\t\txpparam_t xpp;\n+\t\t\txdfenv_t xe;\n+\n+\t\t\tmemset(&xpp, 0, sizeof(xpp));\n+\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n+\n+\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n+\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n+\n+\t\t\tif (xdl_fall_back_diff(&xe, &xpp,\n+\t\t\t\t\t g.start + 1, g.end - g.start,\n+\t\t\t\t\t go.start + 1, go.end - go.start)) {\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n+\n \tnext:\n \t\t/* Move past the just-processed group: */\n \t\tif (group_next(xdf, &g))\n\nbase-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9\n-- \ngitgitgadget\n"},{"id":"534383","messageId":"xmqqikcusn8p.fsf@gitster.g","threadId":"64586","inReplyTo":"pull.2120.git.git.1765054287938.gitgitgadget@gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-21T20:51:34Z","receivedAt":"2026-01-21T20:51:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Yee Cheng Chin via GitGitGadget\" writes:\n\n> When using Myer's diff, the algorithm finds that only the \"x\" has been\n> changed, and produces a final diff result (these are line diffs, but\n> using word-diff syntax for ease of presentation):\n>\n> A A[-A-]{+x+}A AAA\n\nAnd patience gives the same result; as you noted, it uses a unique\nline as the anchoring point.\n\n> When using histogram diff, the algorithm first discovers the LCS \"A\n> AAA\", which it uses as anchor, then produces an intermediate diff:\n>\n> {+A Ax+}A AAA[- AAA-].\n>\n> This is a longer diff than Myer's, but it's still self-consistent.\n> However, the compaction phase attempts to shift the first file's diff\n> group upwards (note that this shift crosses the anchor that histogram\n> had used), leading to the final results for histogram diff:\n>\n> [-A AA-]{+A Ax+}A AAA\n>\n> This is a technically correct patch but looks clearly redundant to a\n> human as the first 3 lines should not be in the diff.\n\nSo true.\n\n> The fix would detect that a shift has caused matching to a new group,\n> and re-diff the \"A AA\" and \"A Ax\" parts, which results in \"A A\"\n> correctly re-marked as unchanged. This creates the now correct histogram\n> diff:\n>\n> A A[-A-]{+x+}A AAA\n\nOK.\n\n> This issue is rare in a normal repository. Below is a table of\n> repositories (`git log --no-merges -p --histogram -1000`), showing how\n> many times a re-diff was done and how many times it resulted in finding\n> matching lines (therefore addressing this issue) with the fix. In\n> general it is fewer than 1% of diff's that exhibit this offending\n> behavior:\n\nIn other words, without the fix, we'd see 1% or so commits with\nsuboptimal (or \"funny looking\") diff that will trigger bug reports,\nwhich sounds like an unacceptably high failure rate.\n\n> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> \t\t\t}\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * If this has a matching group from the other file, it could\n> +\t\t * either be the original match from the diff algorithm, or\n> +\t\t * arrived at by shifting and joining groups. When it's the\n> +\t\t * latter, it's possible for the two newly joined sides to have\n> +\t\t * matching lines. Re-diff the group to mark these matching\n> +\t\t * lines as unchanged and remove from the diff output.\n> +\t\t *\n> +\t\t * Only do this for histogram diff as its LCS algorithm makes\n> +\t\t * this scenario possible. In contrast, patience diff finds LCS\n> +\t\t * of unique lines that groups cannot be shifted across.\n> +\t\t * Myer's diff (standalone or used as fall-back in patience\n> +\t\t * diff) already finds minimal edits so it is not possible for\n> +\t\t * shifted groups to result in a smaller diff. (Without\n> +\t\t * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be\n> +\t\t * minimal, but it should be so most of the time)\n> +\t\t */\n> +\t\tif (end_matching_other != -1 &&\n> +\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n> +\t\t\t\t(g.start != g_orig.start ||\n> +\t\t\t\t g.end != g_orig.end ||\n> +\t\t\t\t go.start != go_orig.start ||\n> +\t\t\t\t go.end != go_orig.end)) {\n\nSo the idea is to remember the original values in g and go (the\nlocation of the group in the file and the other file) and if\nshifting up and down changed any one of the four ends from the\noriginal locations, we always take the fall-back route (if we are\ndoing histogram)?\n\nBy the way, this appears after the if/else if/ cascade that has:\n\n\tif (g.end == earliest_end) {\n\t\t... do nothing case (case #1)\n\t} else if (end_matching_other != -1) {\n\t\t... do the slide-up thing (case #2)\n\t} else if (flags & XDF_INDENT_HEIRISTIC) {\n\t\t... do the indent heuristic thing (case #3)\n\t}\n\nAm I reading the code correctly that, even though this new block\nappears as if it is a post-clean-up phase that is independent from\nwhich one of the three choices are taken in the previous if/elseif\ncascade, it only is relevant to the second case? I am wondering if\nit would make it easier to follow if the new code were made into a\nsmall helper function that is called from the (case #2) arm of the\nexisting if/else if cascade.\n\nThanks.\n\n\n> +\t\t\txpparam_t xpp;\n> +\t\t\txdfenv_t xe;\n> +\n> +\t\t\tmemset(&xpp, 0, sizeof(xpp));\n> +\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n> +\n> +\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n> +\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n> +\n> +\t\t\tif (xdl_fall_back_diff(&xe, &xpp,\n> +\t\t\t\t\t g.start + 1, g.end - g.start,\n> +\t\t\t\t\t go.start + 1, go.end - go.start)) {\n> +\t\t\t\treturn -1;\n> +\t\t\t}\n> +\t\t}\n> +\n> \tnext:\n> \t\t/* Move past the just-processed group: */\n> \t\tif (group_next(xdf, &g))\n>\n> base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9\n"},{"id":"534595","messageId":"4fa413ae-f2a4-4de2-a2fb-0b1db379750b@gmail.com","threadId":"64586","inReplyTo":"xmqqikcusn8p.fsf@gitster.g","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-01-24T10:54:14Z","receivedAt":"2026-01-24T10:54:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/01/2026 20:51, Junio C Hamano wrote:\n> \"Yee Cheng Chin via GitGitGadget\" writes:\n> \n>> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> \t\t\t}\n>> \t\t}\n>> \n>> +\t\t/*\n>> +\t\t * If this has a matching group from the other file, it could\n>> +\t\t * either be the original match from the diff algorithm, or\n>> +\t\t * arrived at by shifting and joining groups. When it's the\n>> +\t\t * latter, it's possible for the two newly joined sides to have\n>> +\t\t * matching lines. Re-diff the group to mark these matching\n>> +\t\t * lines as unchanged and remove from the diff output.\n>> +\t\t *\n>> +\t\t * Only do this for histogram diff as its LCS algorithm makes\n>> +\t\t * this scenario possible. In contrast, patience diff finds LCS\n>> +\t\t * of unique lines that groups cannot be shifted across.\n>> +\t\t * Myer's diff (standalone or used as fall-back in patience\n>> +\t\t * diff) already finds minimal edits so it is not possible for\n>> +\t\t * shifted groups to result in a smaller diff. (Without\n>> +\t\t * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be\n>> +\t\t * minimal, but it should be so most of the time)\n>> +\t\t */\n>> +\t\tif (end_matching_other != -1 &&\n>> +\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n>> +\t\t\t\t(g.start != g_orig.start ||\n>> +\t\t\t\t g.end != g_orig.end ||\n>> +\t\t\t\t go.start != go_orig.start ||\n>> +\t\t\t\t go.end != go_orig.end)) {\n> \n> So the idea is to remember the original values in g and go (the\n> location of the group in the file and the other file) and if\n> shifting up and down changed any one of the four ends from the\n> original locations, we always take the fall-back route (if we are\n> doing histogram)?\n\nI'm a bit confused why we need to check both groups. I think they're \nsupposed to move together (if we move \"g\" by n context lines we also \nmove \"go\" by n context lines) so I can't see how we can have\n\n\tg.start == g_orig.start && g.end == g_orig.end\n\nwhen\n\n\tgo.start != go.orig.start || go.end != go_orig.end\n\n> By the way, this appears after the if/else if/ cascade that has:\n> \n> \tif (g.end == earliest_end) {\n> \t\t... do nothing case (case #1)\n> \t} else if (end_matching_other != -1) {\n> \t\t... do the slide-up thing (case #2)\n> \t} else if (flags & XDF_INDENT_HEIRISTIC) {\n> \t\t... do the indent heuristic thing (case #3)\n> \t}\n> \n> Am I reading the code correctly that, even though this new block\n> appears as if it is a post-clean-up phase that is independent from\n> which one of the three choices are taken in the previous if/elseif\n> cascade, it only is relevant to the second case? I am wondering if\n> it would make it easier to follow if the new code were made into a\n> small helper function that is called from the (case #2) arm of the\n> existing if/else if cascade.\n\nThat's a good point\n\n>> +\t\t\txpparam_t xpp;\n>> +\t\t\txdfenv_t xe;\n>> +\n>> +\t\t\tmemset(&xpp, 0, sizeof(xpp));\n>> +\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n>> +\n>> +\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n>> +\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n\nThese would be safer as \"xe.xdf1 = *xdf\" so we don't have to worry about \ngetting the size correct (sizeof(*xdf) would also be safer but there is \nno need for memcpy() here).\n\nI also wondered if we need to do a diff or if we can just mark the \ncommon prefix and suffix as unchanged but I suspect that wont will work \nfor more complicated examples.\n\nThanks\n\nPhillip\n\n>> +\n>> +\t\t\tif (xdl_fall_back_diff(&xe, &xpp,\n>> +\t\t\t\t\t g.start + 1, g.end - g.start,\n>> +\t\t\t\t\t go.start + 1, go.end - go.start)) {\n>> +\t\t\t\treturn -1;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> \tnext:\n>> \t\t/* Move past the just-processed group: */\n>> \t\tif (group_next(xdf, &g))\n>>\n>> base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9\n> \n\n"},{"id":"534616","messageId":"xmqqy0llk33y.fsf@gitster.g","threadId":"64586","inReplyTo":"4fa413ae-f2a4-4de2-a2fb-0b1db379750b@gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-25T17:34:57Z","receivedAt":"2026-01-25T17:34:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 21/01/2026 20:51, Junio C Hamano wrote:\n>> \"Yee Cheng Chin via GitGitGadget\" writes:\n>> \n>>> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>> \t\t\t}\n>>> \t\t}\n>>> \n>>> +\t\t/*\n>>> +\t\t * If this has a matching group from the other file, it could\n>>> +\t\t * either be the original match from the diff algorithm, or\n>>> +\t\t * arrived at by shifting and joining groups. When it's the\n>>> +\t\t * latter, it's possible for the two newly joined sides to have\n>>> +\t\t * matching lines. Re-diff the group to mark these matching\n>>> +\t\t * lines as unchanged and remove from the diff output.\n\nAlso, after reading the first paragraph of the big comment again, it\nmakes me wonder if it is saying the same thing as \"When histogram is\nbeing used, we shouldn't bother shifting up and down to join groups,\nas the result will always worse than the fallback\", but is it that\nbad?\n\n>>> +\t\tif (end_matching_other != -1 &&\n>>> +\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n>>> +\t\t\t\t(g.start != g_orig.start ||\n>>> +\t\t\t\t g.end != g_orig.end ||\n>>> +\t\t\t\t go.start != go_orig.start ||\n>>> +\t\t\t\t go.end != go_orig.end)) {\n>> \n>> So the idea is to remember the original values in g and go (the\n>> location of the group in the file and the other file) and if\n>> shifting up and down changed any one of the four ends from the\n>> original locations, we always take the fall-back route (if we are\n>> doing histogram)?\n>\n> I'm a bit confused why we need to check both groups. I think they're \n> supposed to move together (if we move \"g\" by n context lines we also \n> move \"go\" by n context lines) so I can't see how we can have\n>\n> \tg.start == g_orig.start && g.end == g_orig.end\n>\n> when\n>\n> \tgo.start != go.orig.start || go.end != go_orig.end\n\nInteresting.\n\n>> By the way, this appears after the if/else if/ cascade that has:\n>> \n>> \tif (g.end == earliest_end) {\n>> \t\t... do nothing case (case #1)\n>> \t} else if (end_matching_other != -1) {\n>> \t\t... do the slide-up thing (case #2)\n>> \t} else if (flags & XDF_INDENT_HEIRISTIC) {\n>> \t\t... do the indent heuristic thing (case #3)\n>> \t}\n>> \n>> Am I reading the code correctly that, even though this new block\n>> appears as if it is a post-clean-up phase that is independent from\n>> which one of the three choices are taken in the previous if/elseif\n>> cascade, it only is relevant to the second case? I am wondering if\n>> it would make it easier to follow if the new code were made into a\n>> small helper function that is called from the (case #2) arm of the\n>> existing if/else if cascade.\n>\n> That's a good point\n>\n>>> +\t\t\txpparam_t xpp;\n>>> +\t\t\txdfenv_t xe;\n>>> +\n>>> +\t\t\tmemset(&xpp, 0, sizeof(xpp));\n>>> +\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n>>> +\n>>> +\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n>>> +\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n>\n> These would be safer as \"xe.xdf1 = *xdf\" so we don't have to worry about \n> getting the size correct (sizeof(*xdf) would also be safer but there is \n> no need for memcpy() here).\n\nVery good readability enhancement suggestion.\n\nThanks.\n"},{"id":"534638","messageId":"3aeb49dd-8618-42e0-b9f9-6a4fb8065793@gmail.com","threadId":"64586","inReplyTo":"xmqqy0llk33y.fsf@gitster.g","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-01-26T09:37:03Z","receivedAt":"2026-01-26T09:37:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/01/2026 17:34, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 21/01/2026 20:51, Junio C Hamano wrote:\n>>> \"Yee Cheng Chin via GitGitGadget\" writes:\n>>>\n>>>> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>>> \t\t\t}\n>>>> \t\t}\n>>>> \n>>>> +\t\t/*\n>>>> +\t\t * If this has a matching group from the other file, it could\n>>>> +\t\t * either be the original match from the diff algorithm, or\n>>>> +\t\t * arrived at by shifting and joining groups. When it's the\n>>>> +\t\t * latter, it's possible for the two newly joined sides to have\n>>>> +\t\t * matching lines. Re-diff the group to mark these matching\n>>>> +\t\t * lines as unchanged and remove from the diff output.\n> \n> Also, after reading the first paragraph of the big comment again, it\n> makes me wonder if it is saying the same thing as \"When histogram is\n> being used, we shouldn't bother shifting up and down to join groups,\n> as the result will always worse than the fallback\", but is it that\n> bad?\n\nLooking at the example in the commit message the result of shifting up \nand down and then calling the fallback is better than either the \nunshifted diff or shifting without the fallback, so I don't think just \ndisabling shifting improves things. It would also stop us coalescing \nchanged lines, for example\n\n-A A\n A -> -A\n-B -B\n\nThe indent heuristic seems to assume that we've shifted down as far as \npossible before trying it so that would probably get messed up as well. \nTo me the problem is that the histogram diff does not always generate \nparticularly good diffs (maybe I'm biased - whenever I've tried \nswitching the default to \"histogram\" I've always switched back \n\"patience\" fairly quickly after being presented with a diff that I found \nhard to comprehend)\n\nThanks\n\nPhillip\n\n>>>> +\t\tif (end_matching_other != -1 &&\n>>>> +\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n>>>> +\t\t\t\t(g.start != g_orig.start ||\n>>>> +\t\t\t\t g.end != g_orig.end ||\n>>>> +\t\t\t\t go.start != go_orig.start ||\n>>>> +\t\t\t\t go.end != go_orig.end)) {\n>>>\n>>> So the idea is to remember the original values in g and go (the\n>>> location of the group in the file and the other file) and if\n>>> shifting up and down changed any one of the four ends from the\n>>> original locations, we always take the fall-back route (if we are\n>>> doing histogram)?\n>>\n>> I'm a bit confused why we need to check both groups. I think they're\n>> supposed to move together (if we move \"g\" by n context lines we also\n>> move \"go\" by n context lines) so I can't see how we can have\n>>\n>> \tg.start == g_orig.start && g.end == g_orig.end\n>>\n>> when\n>>\n>> \tgo.start != go.orig.start || go.end != go_orig.end\n> \n> Interesting.\n> \n>>> By the way, this appears after the if/else if/ cascade that has:\n>>>\n>>> \tif (g.end == earliest_end) {\n>>> \t\t... do nothing case (case #1)\n>>> \t} else if (end_matching_other != -1) {\n>>> \t\t... do the slide-up thing (case #2)\n>>> \t} else if (flags & XDF_INDENT_HEIRISTIC) {\n>>> \t\t... do the indent heuristic thing (case #3)\n>>> \t}\n>>>\n>>> Am I reading the code correctly that, even though this new block\n>>> appears as if it is a post-clean-up phase that is independent from\n>>> which one of the three choices are taken in the previous if/elseif\n>>> cascade, it only is relevant to the second case? I am wondering if\n>>> it would make it easier to follow if the new code were made into a\n>>> small helper function that is called from the (case #2) arm of the\n>>> existing if/else if cascade.\n>>\n>> That's a good point\n>>\n>>>> +\t\t\txpparam_t xpp;\n>>>> +\t\t\txdfenv_t xe;\n>>>> +\n>>>> +\t\t\tmemset(&xpp, 0, sizeof(xpp));\n>>>> +\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n>>>> +\n>>>> +\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n>>>> +\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n>>\n>> These would be safer as \"xe.xdf1 = *xdf\" so we don't have to worry about\n>> getting the size correct (sizeof(*xdf) would also be safer but there is\n>> no need for memcpy() here).\n> \n> Very good readability enhancement suggestion.\n> \n> Thanks.\n\n"},{"id":"534677","messageId":"xmqq343sjn4x.fsf@gitster.g","threadId":"64586","inReplyTo":"3aeb49dd-8618-42e0-b9f9-6a4fb8065793@gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-26T17:32:14Z","receivedAt":"2026-01-26T17:32:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 25/01/2026 17:34, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>>> On 21/01/2026 20:51, Junio C Hamano wrote:\n>>>> \"Yee Cheng Chin via GitGitGadget\" writes:\n>>>>\n>>>>> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>>>> \t\t\t}\n>>>>> \t\t}\n>>>>> \n>>>>> +\t\t/*\n>>>>> +\t\t * If this has a matching group from the other file, it could\n>>>>> +\t\t * either be the original match from the diff algorithm, or\n>>>>> +\t\t * arrived at by shifting and joining groups. When it's the\n>>>>> +\t\t * latter, it's possible for the two newly joined sides to have\n>>>>> +\t\t * matching lines. Re-diff the group to mark these matching\n>>>>> +\t\t * lines as unchanged and remove from the diff output.\n>> \n>> Also, after reading the first paragraph of the big comment again, it\n>> makes me wonder if it is saying the same thing as \"When histogram is\n>> being used, we shouldn't bother shifting up and down to join groups,\n>> as the result will always worse than the fallback\", but is it that\n>> bad?\n>\n> Looking at the example in the commit message the result of shifting up \n> and down and then calling the fallback is better than either the \n> unshifted diff or shifting without the fallback, so I don't think just \n> disabling shifting improves things. It would also stop us coalescing \n> changed lines, for example\n>\n> -A A\n> A -> -A\n> -B -B\n>\n> The indent heuristic seems to assume that we've shifted down as far as \n> possible before trying it so that would probably get messed up as well. \n\nI see. Thanks for a good explanation.\n\n> To me the problem is that the histogram diff does not always generate \n> particularly good diffs (maybe I'm biased - whenever I've tried \n> switching the default to \"histogram\" I've always switched back \n> \"patience\" fairly quickly after being presented with a diff that I found \n> hard to comprehend)\n>\n> Thanks\n>\n> Phillip\n\nThanks.\n"},{"id":"534821","messageId":"CAHTeOx8SOZmqvi0pkcheSjFpbEALmOwaUiX0tKLmNP7fqvjMXA@mail.gmail.com","threadId":"64586","inReplyTo":"xmqq343sjn4x.fsf@gitster.g","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin","fromEmail":"ychin.git@gmail.com","sentAt":"2026-01-29T16:53:04Z","receivedAt":"2026-01-29T16:53:42Z","isPatch":true,"sender":{"key":"ychin.git@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1217449?v=4"},"body":"Thanks for the review and sorry for being a little late in replying.\nAggregating all my inline replies in one email if that's ok.\n\nOn Wed, Jan 21, 2026 at 12:51 PM Junio C Hamano wrote:\n> So the idea is to remember the original values in g and go (the\n> location of the group in the file and the other file) and if\n> shifting up and down changed any one of the four ends from the\n> original locations, we always take the fall-back route (if we are\n> doing histogram)?\n>\n> By the way, this appears after the if/else if/ cascade that has:\n>\n> if (g.end == earliest_end) {\n> ... do nothing case (case #1)\n> } else if (end_matching_other != -1) {\n> ... do the slide-up thing (case #2)\n> } else if (flags & XDF_INDENT_HEIRISTIC) {\n> ... do the indent heuristic thing (case #3)\n> }\n>\n> Am I reading the code correctly that, even though this new block\n> appears as if it is a post-clean-up phase that is independent from\n> which one of the three choices are taken in the previous if/elseif\n> cascade, it only is relevant to the second case? I am wondering if\n> it would make it easier to follow if the new code were made into a\n> small helper function that is called from the (case #2) arm of the\n> existing if/else if cascade.\n\nThat's correct. This condition happens only in the 2nd case. The\nproblematic scenario here only happens when the opposite side is\nnon-empty. If the opposite is empty (case #3, where we run the indent\nheuristic algorithm), there's simply no need to re-diff anything\nbecause diff'ing against an empty hunk is pointless.\n\nYou made a good point about placing it in the if block itself. The\nexisting code was a little confusing and took me re-reading the code\nbefore I remember the condition. I'll fix it in v2.\n\nOn Sat, Jan 24, 2026 at 2:54 AM Phillip Wood wrote:\n> I'm a bit confused why we need to check both groups. I think they're\n> supposed to move together (if we move \"g\" by n context lines we also\n> move \"go\" by n context lines) so I can't see how we can have\n>\n> g.start == g_orig.start && g.end == g_orig.end\n>\n> when\n>\n> go.start != go.orig.start || go.end != go_orig.end\n>\n\nYou are right. It was an over-specification. Looking through the code\nwe should be able to just use \"g\" and there is no need to test for\n\"g_orig\". Will fix in v2.\n\n> >> + xpparam_t xpp;\n> >> + xdfenv_t xe;\n> >> +\n> >> + memset(&xpp, 0, sizeof(xpp));\n> >> + xpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n> >> +\n> >> + memcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n> >> + memcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n>\n> These would be safer as \"xe.xdf1 = *xdf\" so we don't have to worry about\n> getting the size correct (sizeof(*xdf) would also be safer but there is\n> no need for memcpy() here).\n\nWill fix in v2.\n\n> I also wondered if we need to do a diff or if we can just mark the\n> common prefix and suffix as unchanged but I suspect that wont will work\n> for more complicated examples.\n\nCommon prefix/suffix would not work for more complicated examples.\nHere's an example (imagine each character to be its own line):\n\nFile 1:\nA AAyz AAA\nFile 2:\nA xAA AAA\n\nThe current Git histogram diff generates the following:\nA [-AAyz -]{+xAA +}AAA\n\nAfter the fix, we have:\nA {+x+}AA[-yz-] AA\n\nNote that there is no common prefix here, and we need a real diff\nalgorithm if we want to solve this issue in a generic fashion. As I\nmentioned in the cover letter, I thought about implementing a \"bespoke\nlinear-time algorithm\" but decided against it. What I meant was we\ncould implement a simple diff algorithm that finds the common lines in\nboth hunks that would run faster than Myer's, but isn't guaranteed to\nbe a optimal minimal diff. I decided that it is unnecessary to\novercomplicate things given that we can just call the fallback diff.\n\nOn Mon, Jan 26, 2026 at 1:37 AM Phillip Wood wrote:\n>\n> On 25/01/2026 17:34, Junio C Hamano wrote:\n> > Also, after reading the first paragraph of the big comment again, it\n> > makes me wonder if it is saying the same thing as \"When histogram is\n> > being used, we shouldn't bother shifting up and down to join groups,\n> > as the result will always worse than the fallback\", but is it that\n> > bad?\n>\n> Looking at the example in the commit message the result of shifting up\n> and down and then calling the fallback is better than either the\n> unshifted diff or shifting without the fallback, so I don't think just\n> disabling shifting improves things. It would also stop us coalescing\n> changed lines, for example\n>\n> -A A\n> A -> -A\n> -B -B\n>\n\nI agree with you, but I think it is actually a nuanced decision. The\nhistogram diff algorithm explicitly chose the specific\nalignment/anchor points to align both files due to the frequency of\nthe lines. When we do the sliding / compaction step, we are\nessentially ignoring and overriding the algorithmic decision made by\nhistogram, for the sake of other metrics that we value (compaction\nvalues fewer diff hunks, and indent heuristics values aligning by\nsemantics approximated by indentation). I think those metrics do help\nwhich is why we added them, but there's a bit of design tension\nbetween the underlying algorithm and the cleanup step.\n\n> To me the problem is that the histogram diff does not always generate\n> particularly good diffs (maybe I'm biased - whenever I've tried\n> switching the default to \"histogram\" I've always switched back\n> \"patience\" fairly quickly after being presented with a diff that I found\n> hard to comprehend)\n\nFWIW I personally feel that way as well. I think the documentation and\nnarrative that histogram diff is a \"more advanced/extended version\" of\npatience diff is sometimes problematic, as both algorithms are fairly\ndifferent and have their own weaknesses. The Longest Common\nSubsequence (LCS) used for alignment in patience diff is global for\nthe file and allows gaps, whereas the LCS in histogram diff requires\nconsecutive lines. This means even if the diff has unique lines across\nboth files the diff results could be quite different between histogram\nand patience. This consecutive requirement for a subsequence is why\nhistogram diff runs faster than patience diff most of the time, but it\ndoes mean the patience algorithm is better at discovering a global\n\"spine\" across a file.\n"},{"id":"534838","messageId":"xmqqsebo9lv6.fsf@gitster.g","threadId":"64586","inReplyTo":"CAHTeOx8SOZmqvi0pkcheSjFpbEALmOwaUiX0tKLmNP7fqvjMXA@mail.gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-29T20:58:53Z","receivedAt":"2026-01-29T20:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yee Cheng Chin writes:\n\n> Thanks for the review and sorry for being a little late in replying.\n> Aggregating all my inline replies in one email if that's ok.\n>\n> On Wed, Jan 21, 2026 at 12:51 PM Junio C Hamano wrote:\n>> So the idea is to remember the original values in g and go (the\n>> location of the group in the file and the other file) and if\n>> shifting up and down changed any one of the four ends from the\n>> original locations, we always take the fall-back route (if we are\n>> doing histogram)?\n>>\n>> By the way, this appears after the if/else if/ cascade that has:\n>>\n>> if (g.end == earliest_end) {\n>> ... do nothing case (case #1)\n>> } else if (end_matching_other != -1) {\n>> ... do the slide-up thing (case #2)\n>> } else if (flags & XDF_INDENT_HEIRISTIC) {\n>> ... do the indent heuristic thing (case #3)\n>> }\n>>\n>> Am I reading the code correctly that, even though this new block\n>> appears as if it is a post-clean-up phase that is independent from\n>> which one of the three choices are taken in the previous if/elseif\n>> cascade, it only is relevant to the second case? I am wondering if\n>> it would make it easier to follow if the new code were made into a\n>> small helper function that is called from the (case #2) arm of the\n>> existing if/else if cascade.\n>\n> That's correct. This condition happens only in the 2nd case. The\n> problematic scenario here only happens when the opposite side is\n> non-empty. If the opposite is empty (case #3, where we run the indent\n> heuristic algorithm), there's simply no need to re-diff anything\n> because diff'ing against an empty hunk is pointless.\n\nOK. In the version posted, it appeard that it is possible, after\nnot doing the slide-up thing but using indent heuristic thing, to\nfall into this compensation codepath because the new code was placed\nafter the above if-else-if cascade as if it is an independent\nclean-up phase. Encapsulating that new code in a helper function\nand calling it at the end of \"do the slide-up thing\" block will make\nthe intent clearer.\n\nThanks.\n"},{"id":"534850","messageId":"CAHTeOx-TLwqbcdGcb2drD4vE6D3M93EPMjcAeTNR+XNTbmTVZg@mail.gmail.com","threadId":"64586","inReplyTo":"xmqqsebo9lv6.fsf@gitster.g","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin","fromEmail":"ychin.git@gmail.com","sentAt":"2026-01-30T01:58:18Z","receivedAt":"2026-01-30T01:58:57Z","isPatch":true,"sender":{"key":"ychin.git@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1217449?v=4"},"body":"On Thu, Jan 29, 2026 at 12:58 PM Junio C Hamano wrote:\n>\n> Yee Cheng Chin writes:\n> >\n> > On Wed, Jan 21, 2026 at 12:51 PM Junio C Hamano wrote:\n> >> By the way, this appears after the if/else if/ cascade that has:\n> >>\n> >> if (g.end == earliest_end) {\n> >> ... do nothing case (case #1)\n> >> } else if (end_matching_other != -1) {\n> >> ... do the slide-up thing (case #2)\n> >> } else if (flags & XDF_INDENT_HEIRISTIC) {\n> >> ... do the indent heuristic thing (case #3)\n> >> }\n> >>\n> >> Am I reading the code correctly that, even though this new block\n> >> appears as if it is a post-clean-up phase that is independent from\n> >> which one of the three choices are taken in the previous if/elseif\n> >> cascade, it only is relevant to the second case? I am wondering if\n> >> it would make it easier to follow if the new code were made into a\n> >> small helper function that is called from the (case #2) arm of the\n> >> existing if/else if cascade.\n> >\n> > That's correct. This condition happens only in the 2nd case. The\n> > problematic scenario here only happens when the opposite side is\n> > non-empty. If the opposite is empty (case #3, where we run the indent\n> > heuristic algorithm), there's simply no need to re-diff anything\n> > because diff'ing against an empty hunk is pointless.\n>\n> OK. In the version posted, it appeard that it is possible, after\n> not doing the slide-up thing but using indent heuristic thing, to\n> fall into this compensation codepath because the new code was placed\n> after the above if-else-if cascade as if it is an independent\n> clean-up phase. Encapsulating that new code in a helper function\n> and calling it at the end of \"do the slide-up thing\" block will make\n> the intent clearer.\n\nSorry, I actually misspoke. I forgot that re-diff is actually needed\nin both case #1 and #2. Note that even in #1, it's possible for\n`end_matching_other != -1` to be true. In case #3, it only cannot\nhappen because `end_matching_other` has to be -1 by then (meaning that\nthis diff hunk only has content on this side and is empty on the\nother).\n\nCase #1 happens when no *remaining* shifting was necessary, but note\nthat this happens after the do/while loop above, where previous loops\ncould have shifted and compacted the diff blocks already. Case #2 just\nmeans there's some remaining clean up work to be done.\n\nJust for a concrete test case that will illustrate this in case\nsomeone is running the code and want a demonstration:\n\nFile 1:\nAXB*\n\nFile 2:\nCD*XE*\n\nThe first \"*\" is used as the histogram alignment anchor, which will be\nshifted resulting in a compaction, and therefore needs to trigger a\nre-diff. The correct output is as follows (which will only happen if\nwe also run the re-diff in case #1):\n\n{-A-}[+CD*+]X{-B-}[+E+]*\n\nOtherwise we will get the wrong output (note how the \"X\" is\nerroneuously included on both sides):\n\n{-AXB-}[+CD*XE+]*\n\nBecause of that, I'm leaning on keeping the current code structure,\nbecause it *is* indeed a cleanup step to be run after the previous\none. I could still refactor it into a separate function and put it\ninto the the case #1/#2 if blocks if you think that's cleaner.\n\nI will also add the above to the test case in v2.\n"},{"id":"534851","messageId":"xmqqsebn8xlk.fsf@gitster.g","threadId":"64586","inReplyTo":"CAHTeOx-TLwqbcdGcb2drD4vE6D3M93EPMjcAeTNR+XNTbmTVZg@mail.gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-30T05:43:03Z","receivedAt":"2026-01-30T05:43:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yee Cheng Chin writes:\n\n> Because of that, I'm leaning on keeping the current code structure,\n> because it *is* indeed a cleanup step to be run after the previous\n> one. I could still refactor it into a separate function and put it\n> into the the case #1/#2 if blocks if you think that's cleaner.\n>\n> I will also add the above to the test case in v2.\n\nAs long as the resulting code is explained (perhaps in the comment\nand/or with the code structure) well enough so that when read by\nsomebody else in two months, it won't have to invite the same\nquestion as I asked in this thread, it would be OK. I do not know\noffhand if a comment with the current code structure is good enough,\nor calling the same helper function from two out of three arms of\nif/else-if cascade would make it even clearer.\n\nI agree that the case you gave is tricky enough that it would be a\ngood idea to add it as a test.\n\nThanks.\n"},{"id":"534886","messageId":"b66b8781-a826-44e0-9a2b-2c3a57547f06@gmail.com","threadId":"64586","inReplyTo":"CAHTeOx-TLwqbcdGcb2drD4vE6D3M93EPMjcAeTNR+XNTbmTVZg@mail.gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-01-30T16:06:10Z","receivedAt":"2026-01-30T16:06:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 30/01/2026 01:58, Yee Cheng Chin wrote:\n> \n> Case #1 happens when no *remaining* shifting was necessary, but note\n> that this happens after the do/while loop above, where previous loops\n> could have shifted and compacted the diff blocks already. Case #2 just\n> means there's some remaining clean up work to be done.\n\nThat's a good point - as well as commenting the new code, it would be \nhelpful to update the comment in case #1 to make it clear that we don't \nneed to shift back up to align with a matching block, not there there \nwas no shift possible. I agree with Junio that it would be useful to add \nthe example below as a test\n\nThanks\n\nPhillip\n\n> Just for a concrete test case that will illustrate this in case\n> someone is running the code and want a demonstration:\n> \n> File 1:\n> AXB*\n> \n> File 2:\n> CD*XE*\n> \n> The first \"*\" is used as the histogram alignment anchor, which will be\n> shifted resulting in a compaction, and therefore needs to trigger a\n> re-diff. The correct output is as follows (which will only happen if\n> we also run the re-diff in case #1):\n> \n> {-A-}[+CD*+]X{-B-}[+E+]*\n> \n> Otherwise we will get the wrong output (note how the \"X\" is\n> erroneuously included on both sides):\n> \n> {-AXB-}[+CD*XE+]*\n> \n> Because of that, I'm leaning on keeping the current code structure,\n> because it *is* indeed a cleanup step to be run after the previous\n> one. I could still refactor it into a separate function and put it\n> into the the case #1/#2 if blocks if you think that's cleaner.\n> \n> I will also add the above to the test case in v2.\n> \n\n"},{"id":"536562","messageId":"xmqq7bs7ui95.fsf@gitster.g","threadId":"64586","inReplyTo":"CAHTeOx-TLwqbcdGcb2drD4vE6D3M93EPMjcAeTNR+XNTbmTVZg@mail.gmail.com","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-20T23:07:02Z","receivedAt":"2026-02-20T23:07:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yee Cheng Chin writes:\n\n> ...\n> {-AXB-}[+CD*XE+]*\n>\n> Because of that, I'm leaning on keeping the current code structure,\n> because it *is* indeed a cleanup step to be run after the previous\n> one. I could still refactor it into a separate function and put it\n> into the the case #1/#2 if blocks if you think that's cleaner.\n>\n> I will also add the above to the test case in v2.\n\nOK, it has been a few weeks since we had this message. Will we see\nan update sometime soon? No rush, but just pinging.\n\nThanks.\n\n"},{"id":"536581","messageId":"CAHTeOx9WehtwSMie53xzZUU7iK3JTrgUbVK48WM7S+LBi=jpkQ@mail.gmail.com","threadId":"64586","inReplyTo":"xmqq7bs7ui95.fsf@gitster.g","subject":"Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin","fromEmail":"ychin.git@gmail.com","sentAt":"2026-02-21T09:56:27Z","receivedAt":"2026-02-21T09:57:05Z","isPatch":true,"sender":{"key":"ychin.git@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1217449?v=4"},"body":"Hi, yes, I'm still on it. Sorry for the delay. I will push an update\nout this weekend.\n\nOn Fri, Feb 20, 2026 at 3:07 PM Junio C Hamano wrote:\n>\n> Yee Cheng Chin writes:\n>\n> > ...\n> > {-AXB-}[+CD*XE+]*\n> >\n> > Because of that, I'm leaning on keeping the current code structure,\n> > because it *is* indeed a cleanup step to be run after the previous\n> > one. I could still refactor it into a separate function and put it\n> > into the the case #1/#2 if blocks if you think that's cleaner.\n> >\n> > I will also add the above to the test case in v2.\n>\n> OK, it has been a few weeks since we had this message. Will we see\n> an update sometime soon? No rush, but just pinging.\n>\n> Thanks.\n>\n"},{"id":"537527","messageId":"pull.2120.v2.git.git.1772463265865.gitgitgadget@gmail.com","threadId":"64586","inReplyTo":"pull.2120.git.git.1765054287938.gitgitgadget@gmail.com","subject":"[PATCH v2] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-03-02T14:54:25Z","receivedAt":"2026-03-02T14:54:28Z","isPatch":true,"sender":{"key":"ychin.macvim@gmail.com","avatar":null},"body":"From: Yee Cheng Chin \n\nAfter a diff algorithm has been run, the compaction phase\n(xdl_change_compact()) shifts and merges change groups to produce a\ncleaner output. However, this shifting could create a new matched group\nwhere both sides now have matching lines. This results in a\nwrong-looking diff output which contains redundant lines that are the\nsame on both files.\n\nFix this by detecting this situation, and re-diff the texts on each side\nto find similar lines, using the fall-back Myer's diff. Only do this for\nhistogram diff as it's the only algorithm where this is relevant. Below\ncontains an example, and more details.\n\nFor an example, consider two files below:\n\n file1:\n A\n\n A\n A\n A\n\n A\n A\n A\n\n file2:\n A\n\n A\n x\n A\n\n A\n A\n A\n\nWhen using Myer's diff, the algorithm finds that only the \"x\" has been\nchanged, and produces a final diff result (these are line diffs, but\nusing word-diff syntax for ease of presentation):\n\n A A[-A-]{+x+}A AAA\n\nWhen using histogram diff, the algorithm first discovers the LCS \"A\nAAA\", which it uses as anchor, then produces an intermediate diff:\n\n {+A Ax+}A AAA[- AAA-].\n\nThis is a longer diff than Myer's, but it's still self-consistent.\nHowever, the compaction phase attempts to shift the first file's diff\ngroup upwards (note that this shift crosses the anchor that histogram\nhad used), leading to the final results for histogram diff:\n\n [-A AA-]{+A Ax+}A AAA\n\nThis is a technically correct patch but looks clearly redundant to a\nhuman as the first 3 lines should not be in the diff.\n\nThe fix would detect that a shift has caused matching to a new group,\nand re-diff the \"A AA\" and \"A Ax\" parts, which results in \"A A\"\ncorrectly re-marked as unchanged. This creates the now correct histogram\ndiff:\n\n A A[-A-]{+x+}A AAA\n\nThis issue is not applicable to Myer's diff algorithm as it already\ngenerates a minimal diff, which means a shift cannot result in a smaller\ndiff output (the default Myer's diff in xdiff is not guaranteed to be\nminimal for performance reasons, but it typically does a good enough\njob).\n\nIt's also not applicable to patience diff, because it uses only unique\nlines as anchor for its splits, and falls back to Myer's diff within\neach split. Shifting requires both ends having the same lines, and\ntherefore cannot cross the unique line boundaries established by the\npatience algorithm. In contrast histogram diff uses non-unique lines as\nanchors, and therefore shifting can cross over them.\n\nThis issue is rare in a normal repository. Below is a table of\nrepositories (`git log --no-merges -p --histogram -1000`), showing how\nmany times a re-diff was done and how many times it resulted in finding\nmatching lines (therefore addressing this issue) with the fix. In\ngeneral it is fewer than 1% of diff's that exhibit this offending\nbehavior:\n\n| Repo (1k commits) | Re-diff | Found matching lines |\n|--------------------|---------|----------------------|\n| llvm-project | 45 | 11 |\n| vim | 110 | 9 |\n| git | 18 | 2 |\n| WebKit | 168 | 1 |\n| ripgrep | 22 | 1 |\n| cpython | 32 | 0 |\n| vscode | 13 | 0 |\n\nSigned-off-by: Yee Cheng Chin \n---\n xdiff: re-diff shifted change groups when using histogram algorithm\n \n Changes since v1:\n \n * Fix the entry condition to be easier to understand by checking for\n go.end!=go.start, which makes it clear that this is just a triviality\n test (if one side is empty there is no point in diff'ing anything)\n * Remove go_orig, which was redundant as it was tracking the same thing\n as g_orig.\n * Use assignment instead of memcpy()\n * Clean up comments\n * Per discussed, add test to show that we need to re-diff even if we\n entere the first condition \"no shifting was possible\".\n \n This is a somewhat rare issue when using histogram to diff files, as the\n algorithm will generate a diff output that looks redundant and wrong to\n a human. I provided a synthetic example in the commit message, but for\n one from the real world, do the following command in the Git repo:\n \n git show -U0 --diff-algorithm=histogram 2c8999027c -- po/ga.po\n \n \n Scroll to the line \"@@ -7239,3 +5831,5 @@\", and we can see the following\n diff hunk:\n \n -#: builtin/diff.c\n -msgid \"Not a git repository\"\n -msgstr \"Ní stór git\"\n +msgid \"cannot come back to cwd\"\n +msgstr \"ní féidir teacht ar ais chuig cwd\"\n +\n +msgid \"Not a git repository\"\n +msgstr \"Ní stór git é\"\n \n \n We can see that the \"Not a git repository\" line is identical on both\n sides, which means it should not have been in the diff results to begin\n with. Under other diff algorithms (or histogram diff with this fix),\n said line is not considered to be part of the diff.\n \n Also, when I was implementing this, an alternative I was considering was\n to add a bespoke linear-time algorithm to remove matching lines on both\n sides. Just calling the fall-back diff seems the easiest and cleanest\n and so I went with that.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2120%2Fychin%2Fxdiff-fix-compact-remove-redundant-lines-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2120/ychin/xdiff-fix-compact-remove-redundant-lines-v2\nPull-Request: https://github.com/git/git/pull/2120\n\nRange-diff vs v1:\n\n 1: 34a370e59e ! 1: bd63c6866b xdiff: re-diff shifted change groups when using histogram algorithm\n @@ Commit message\n \n ## t/meson.build ##\n @@ t/meson.build: integration_tests = [\n - 't4070-diff-pairs.sh',\n 't4071-diff-minimal.sh',\n 't4072-diff-max-depth.sh',\n -+ 't4073-diff-shifted-matched-group.sh',\n + 't4073-diff-stat-name-width.sh',\n ++ 't4074-diff-shifted-matched-group.sh',\n 't4100-apply-stat.sh',\n 't4101-apply-nonl.sh',\n 't4102-apply-rename.sh',\n \n - ## t/t4073-diff-shifted-matched-group.sh (new) ##\n + ## t/t4074-diff-shifted-matched-group.sh (new) ##\n @@\n +#!/bin/sh\n +\n @@ t/t4073-diff-shifted-matched-group.sh (new)\n +\n +. ./test-lib.sh\n +\n -+test_expect_success 'shifted diff group should re-diff to minimize patch' '\n ++test_expect_success 'shifted/merged diff group should re-diff to minimize patch' '\n +\ttest_write_lines A x A A A x A A A >file1 &&\n +\ttest_write_lines A x A Z A x A A A >file2 &&\n +\n @@ t/t4073-diff-shifted-matched-group.sh (new)\n +\ttest_cmp expect output\n +'\n +\n ++test_expect_success 'merged diff group with no shift' '\n ++\ttest_write_lines A Z B x >file1 &&\n ++\ttest_write_lines C D x Z E x >file2 &&\n ++\n ++\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n ++\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n ++\n ++\tcat >expect <<-EOF &&\n ++\tdiff --git a/file1 b/file2\n ++\tindex $file1_h..$file2_h 100644\n ++\t--- a/file1\n ++\t+++ b/file2\n ++\t@@ -1,4 +1,6 @@\n ++\t-A\n ++\t+C\n ++\t+D\n ++\t+x\n ++\t Z\n ++\t-B\n ++\t+E\n ++\t x\n ++\tEOF\n ++\n ++\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n ++\ttest_cmp expect output\n ++'\n ++\n +test_expect_success 're-diff should preserve diff flags' '\n +\ttest_write_lines a b c a b c >file1 &&\n +\ttest_write_lines x \" b\" z a b c >file2 &&\n @@ xdiff/xdiffi.c: static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)\n */\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tstruct xdlgroup g, go;\n -+\tstruct xdlgroup g_orig, go_orig;\n ++\tstruct xdlgroup g_orig;\n \tlong earliest_end, end_matching_other;\n \tlong groupsize;\n \n @@ xdiff/xdiffi.c: int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags\n \t\t\tgoto next;\n \n +\t\tg_orig = g;\n -+\t\tgo_orig = go;\n +\n \t\t/*\n \t\t * Now shift the change up and then down as far as possible in\n \t\t * each direction. If it bumps into any other changes, merge\n +-\t\t * them.\n ++\t\t * them and restart the process.\n + \t\t */\n + \t\tdo {\n + \t\t\tgroupsize = g.end - g.start;\n +@@ xdiff/xdiffi.c: int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n + \t\t\t/*\n + \t\t\t * Move the possibly merged group of changes back to\n + \t\t\t * line up with the last group of changes from the\n +-\t\t\t * other file that it can align with.\n ++\t\t\t * other file that it can align with. This avoids breaking\n ++\t\t\t * a single change into a separate addition/deletion.\n + \t\t\t */\n + \t\t\twhile (go.end == go.start) {\n + \t\t\t\tif (group_slide_up(xdf, &g))\n @@ xdiff/xdiffi.c: int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t}\n \t\t}\n \n +\t\t/*\n -+\t\t * If this has a matching group from the other file, it could\n -+\t\t * either be the original match from the diff algorithm, or\n -+\t\t * arrived at by shifting and joining groups. When it's the\n -+\t\t * latter, it's possible for the two newly joined sides to have\n -+\t\t * matching lines. Re-diff the group to mark these matching\n -+\t\t * lines as unchanged and remove from the diff output.\n ++\t\t * If we merged change groups during shifting, the new\n ++\t\t * combined group could now have matching lines in both files,\n ++\t\t * even if the original separate groups did not. Re-diff the\n ++\t\t * new group to find these matching lines to mark them as\n ++\t\t * unchanged.\n ++\t\t *\n ++\t\t * Only do this if the corresponding group in the other file is\n ++\t\t * non-empty, as it's trivial otherwise.\n +\t\t *\n -+\t\t * Only do this for histogram diff as its LCS algorithm makes\n -+\t\t * this scenario possible. In contrast, patience diff finds LCS\n ++\t\t * Only do this for histogram diff as its LCS algorithm allows\n ++\t\t * for this scenario. In contrast, patience diff finds LCS\n +\t\t * of unique lines that groups cannot be shifted across.\n +\t\t * Myer's diff (standalone or used as fall-back in patience\n +\t\t * diff) already finds minimal edits so it is not possible for\n @@ xdiff/xdiffi.c: int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags\n +\t\t * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be\n +\t\t * minimal, but it should be so most of the time)\n +\t\t */\n -+\t\tif (end_matching_other != -1 &&\n ++\t\tif (go.end != go.start &&\n +\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n +\t\t\t\t(g.start != g_orig.start ||\n -+\t\t\t\t g.end != g_orig.end ||\n -+\t\t\t\t go.start != go_orig.start ||\n -+\t\t\t\t go.end != go_orig.end)) {\n ++\t\t\t\t g.end != g_orig.end)) {\n +\t\t\txpparam_t xpp;\n +\t\t\txdfenv_t xe;\n +\n +\t\t\tmemset(&xpp, 0, sizeof(xpp));\n +\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n +\n -+\t\t\tmemcpy(&xe.xdf1, xdf, sizeof(xdfile_t));\n -+\t\t\tmemcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));\n ++\t\t\txe.xdf1 = *xdf;\n ++\t\t\txe.xdf2 = *xdfo;\n +\n +\t\t\tif (xdl_fall_back_diff(&xe, &xpp,\n +\t\t\t\t\t g.start + 1, g.end - g.start,\n\n\n t/meson.build | 1 +\n t/t4074-diff-shifted-matched-group.sh | 164 ++++++++++++++++++++++++++\n xdiff/xdiffi.c | 47 +++++++-\n 3 files changed, 210 insertions(+), 2 deletions(-)\n create mode 100755 t/t4074-diff-shifted-matched-group.sh\n\ndiff --git a/t/meson.build b/t/meson.build\nindex 6d91470ebc..dfd0a5a7d9 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -504,6 +504,7 @@ integration_tests = [\n 't4071-diff-minimal.sh',\n 't4072-diff-max-depth.sh',\n 't4073-diff-stat-name-width.sh',\n+ 't4074-diff-shifted-matched-group.sh',\n 't4100-apply-stat.sh',\n 't4101-apply-nonl.sh',\n 't4102-apply-rename.sh',\ndiff --git a/t/t4074-diff-shifted-matched-group.sh b/t/t4074-diff-shifted-matched-group.sh\nnew file mode 100755\nindex 0000000000..d77fa3b79d\n--- /dev/null\n+++ b/t/t4074-diff-shifted-matched-group.sh\n@@ -0,0 +1,164 @@\n+#!/bin/sh\n+\n+test_description='shifted diff groups re-diffing during histogram diff'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'shifted/merged diff group should re-diff to minimize patch' '\n+\ttest_write_lines A x A A A x A A A >file1 &&\n+\ttest_write_lines A x A Z A x A A A >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,7 +1,7 @@\n+\t A\n+\t x\n+\t A\n+\t-A\n+\t+Z\n+\t A\n+\t x\n+\t A\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 'merged diff group with no shift' '\n+\ttest_write_lines A Z B x >file1 &&\n+\ttest_write_lines C D x Z E x >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,4 +1,6 @@\n+\t-A\n+\t+C\n+\t+D\n+\t+x\n+\t Z\n+\t-B\n+\t+E\n+\t x\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success 're-diff should preserve diff flags' '\n+\ttest_write_lines a b c a b c >file1 &&\n+\ttest_write_lines x \" b\" z a b c >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,6 +1,6 @@\n+\t-a\n+\t-b\n+\t-c\n+\t+x\n+\t+ b\n+\t+z\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output &&\n+\ttest_cmp expect output &&\n+\n+\tcat >expect_iwhite <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,6 +1,6 @@\n+\t-a\n+\t+x\n+\t b\n+\t-c\n+\t+z\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram --ignore-all-space file1 file2 >output_iwhite &&\n+\ttest_cmp expect_iwhite output_iwhite\n+'\n+\n+test_expect_success 'shifting on either side should trigger re-diff properly' '\n+\ttest_write_lines a b c a b c a b c >file1 &&\n+\ttest_write_lines a b c a1 a2 a3 b c1 a b c >file2 &&\n+\n+\tfile1_h=$(git rev-parse --short $(git hash-object file1)) &&\n+\tfile2_h=$(git rev-parse --short $(git hash-object file2)) &&\n+\n+\tcat >expect1 <<-EOF &&\n+\tdiff --git a/file1 b/file2\n+\tindex $file1_h..$file2_h 100644\n+\t--- a/file1\n+\t+++ b/file2\n+\t@@ -1,9 +1,11 @@\n+\t a\n+\t b\n+\t c\n+\t-a\n+\t+a1\n+\t+a2\n+\t+a3\n+\t b\n+\t-c\n+\t+c1\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file1 file2 >output1 &&\n+\ttest_cmp expect1 output1 &&\n+\n+\tcat >expect2 <<-EOF &&\n+\tdiff --git a/file2 b/file1\n+\tindex $file2_h..$file1_h 100644\n+\t--- a/file2\n+\t+++ b/file1\n+\t@@ -1,11 +1,9 @@\n+\t a\n+\t b\n+\t c\n+\t-a1\n+\t-a2\n+\t-a3\n+\t+a\n+\t b\n+\t-c1\n+\t+c\n+\t a\n+\t b\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index --histogram file2 file1 >output2 &&\n+\ttest_cmp expect2 output2\n+'\n+\n+test_done\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 4376f943db..5455b4690d 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -792,6 +792,7 @@ static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)\n */\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tstruct xdlgroup g, go;\n+\tstruct xdlgroup g_orig;\n \tlong earliest_end, end_matching_other;\n \tlong groupsize;\n \n@@ -805,10 +806,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\tif (g.end == g.start)\n \t\t\tgoto next;\n \n+\t\tg_orig = g;\n+\n \t\t/*\n \t\t * Now shift the change up and then down as far as possible in\n \t\t * each direction. If it bumps into any other changes, merge\n-\t\t * them.\n+\t\t * them and restart the process.\n \t\t */\n \t\tdo {\n \t\t\tgroupsize = g.end - g.start;\n@@ -861,7 +864,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t/*\n \t\t\t * Move the possibly merged group of changes back to\n \t\t\t * line up with the last group of changes from the\n-\t\t\t * other file that it can align with.\n+\t\t\t * other file that it can align with. This avoids breaking\n+\t\t\t * a single change into a separate addition/deletion.\n \t\t\t */\n \t\t\twhile (go.end == go.start) {\n \t\t\t\tif (group_slide_up(xdf, &g))\n@@ -914,6 +918,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t}\n \t\t}\n \n+\t\t/*\n+\t\t * If we merged change groups during shifting, the new\n+\t\t * combined group could now have matching lines in both files,\n+\t\t * even if the original separate groups did not. Re-diff the\n+\t\t * new group to find these matching lines to mark them as\n+\t\t * unchanged.\n+\t\t *\n+\t\t * Only do this if the corresponding group in the other file is\n+\t\t * non-empty, as it's trivial otherwise.\n+\t\t *\n+\t\t * Only do this for histogram diff as its LCS algorithm allows\n+\t\t * for this scenario. In contrast, patience diff finds LCS\n+\t\t * of unique lines that groups cannot be shifted across.\n+\t\t * Myer's diff (standalone or used as fall-back in patience\n+\t\t * diff) already finds minimal edits so it is not possible for\n+\t\t * shifted groups to result in a smaller diff. (Without\n+\t\t * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be\n+\t\t * minimal, but it should be so most of the time)\n+\t\t */\n+\t\tif (go.end != go.start &&\n+\t\t\t\tXDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&\n+\t\t\t\t(g.start != g_orig.start ||\n+\t\t\t\t g.end != g_orig.end)) {\n+\t\t\txpparam_t xpp;\n+\t\t\txdfenv_t xe;\n+\n+\t\t\tmemset(&xpp, 0, sizeof(xpp));\n+\t\t\txpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;\n+\n+\t\t\txe.xdf1 = *xdf;\n+\t\t\txe.xdf2 = *xdfo;\n+\n+\t\t\tif (xdl_fall_back_diff(&xe, &xpp,\n+\t\t\t\t\t g.start + 1, g.end - g.start,\n+\t\t\t\t\t go.start + 1, go.end - go.start)) {\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n+\n \tnext:\n \t\t/* Move past the just-processed group: */\n \t\tif (group_next(xdf, &g))\n\nbase-commit: 2cc71917514657b93014134350864f4849edfc83\n-- \ngitgitgadget\n"},{"id":"538866","messageId":"xmqqikb08ax3.fsf@gitster.g","threadId":"64586","inReplyTo":"pull.2120.v2.git.git.1772463265865.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T07:07:36Z","receivedAt":"2026-03-13T07:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Yee Cheng Chin via GitGitGadget\" writes:\n\n> From: Yee Cheng Chin \n>\n> After a diff algorithm has been run, the compaction phase\n> (xdl_change_compact()) shifts and merges change groups to produce a\n> cleaner output. However, this shifting could create a new matched group\n> where both sides now have matching lines. This results in a\n> wrong-looking diff output which contains redundant lines that are the\n> same on both files.\n>\n> Fix this by detecting this situation, and re-diff the texts on each side\n> to find similar lines, using the fall-back Myer's diff. Only do this for\n> histogram diff as it's the only algorithm where this is relevant. Below\n> contains an example, and more details.\n> ...\n> This issue is rare in a normal repository. Below is a table of\n> repositories (`git log --no-merges -p --histogram -1000`), showing how\n> many times a re-diff was done and how many times it resulted in finding\n> matching lines (therefore addressing this issue) with the fix. In\n> general it is fewer than 1% of diff's that exhibit this offending\n> behavior:\n>\n> | Repo (1k commits) | Re-diff | Found matching lines |\n> |--------------------|---------|----------------------|\n> | llvm-project | 45 | 11 |\n> | vim | 110 | 9 |\n> | git | 18 | 2 |\n> | WebKit | 168 | 1 |\n> | ripgrep | 22 | 1 |\n> | cpython | 32 | 0 |\n> | vscode | 13 | 0 |\n>\n> Signed-off-by: Yee Cheng Chin \n> ---\n\nThanks for the updated patch, and sorry for nobody responding to the\npatch for over a week.\n\nThe detailed explanation of the issue and the inclusion of the\nrepository analysis results are very helpful; they clearly show that\nwhile this is a rare edge case, it significantly improves the\nquality of histogram diffs when it does occur.\n\n - The removal of go_orig is correct since g and go are kept in sync \n throughout the slide loops.\n\n - Clearing the algorithm mask while preserving other flags ensures that \n user-provided options like --ignore-all-space are correctly applied \n during the re-diff.\n\n - While ignore_regex and anchors are not passed to the sub-diff, they \n aren't currently available to xdl_change_compact anyway. Given that \n compaction happens before regex filtering in the main pipeline, this\n is OK, I guess.\n\nLet me mark the topic for 'next'.\n\n"},{"id":"538870","messageId":"016df393-a36f-4e5e-ab6a-eb661f5c84cc@gmail.com","threadId":"64586","inReplyTo":"xmqqikb08ax3.fsf@gitster.g","subject":"Re: [PATCH v2] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-03-13T10:23:12Z","receivedAt":"2026-03-13T10:23:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/03/2026 07:07, Junio C Hamano wrote:\n> \"Yee Cheng Chin via GitGitGadget\" writes:\n> \n>> From: Yee Cheng Chin \n>>\n>> After a diff algorithm has been run, the compaction phase\n>> (xdl_change_compact()) shifts and merges change groups to produce a\n>> cleaner output. However, this shifting could create a new matched group\n>> where both sides now have matching lines. This results in a\n>> wrong-looking diff output which contains redundant lines that are the\n>> same on both files.\n>>\n>> Fix this by detecting this situation, and re-diff the texts on each side\n>> to find similar lines, using the fall-back Myer's diff. Only do this for\n>> histogram diff as it's the only algorithm where this is relevant. Below\n>> contains an example, and more details.\n>> ...\n>> This issue is rare in a normal repository. Below is a table of\n>> repositories (`git log --no-merges -p --histogram -1000`), showing how\n>> many times a re-diff was done and how many times it resulted in finding\n>> matching lines (therefore addressing this issue) with the fix. In\n>> general it is fewer than 1% of diff's that exhibit this offending\n>> behavior:\n>>\n>> | Repo (1k commits) | Re-diff | Found matching lines |\n>> |--------------------|---------|----------------------|\n>> | llvm-project | 45 | 11 |\n>> | vim | 110 | 9 |\n>> | git | 18 | 2 |\n>> | WebKit | 168 | 1 |\n>> | ripgrep | 22 | 1 |\n>> | cpython | 32 | 0 |\n>> | vscode | 13 | 0 |\n>>\n>> Signed-off-by: Yee Cheng Chin \n>> ---\n> \n> Thanks for the updated patch, and sorry for nobody responding to the\n> patch for over a week.\n\nYes, sorry for the slow response. I agree with Junio that this is \nexplained well and looks good\n\nThanks\n\nPhillip\n\n> The detailed explanation of the issue and the inclusion of the\n> repository analysis results are very helpful; they clearly show that\n> while this is a rare edge case, it significantly improves the\n> quality of histogram diffs when it does occur.\n> \n> - The removal of go_orig is correct since g and go are kept in sync\n> throughout the slide loops.\n> \n> - Clearing the algorithm mask while preserving other flags ensures that\n> user-provided options like --ignore-all-space are correctly applied\n> during the re-diff.\n> \n> - While ignore_regex and anchors are not passed to the sub-diff, they\n> aren't currently available to xdl_change_compact anyway. Given that\n> compaction happens before regex filtering in the main pipeline, this\n> is OK, I guess.\n> \n> Let me mark the topic for 'next'.\n> \n\n"},{"id":"539446","messageId":"CAHTeOx_edyC_nvXd7cU5o1498K6K9FVky1PG3ArDrWKrZ87pjQ@mail.gmail.com","threadId":"64586","inReplyTo":"016df393-a36f-4e5e-ab6a-eb661f5c84cc@gmail.com","subject":"Re: [PATCH v2] xdiff: re-diff shifted change groups when using histogram algorithm","fromName":"Yee Cheng Chin","fromEmail":"ychin.git@gmail.com","sentAt":"2026-03-19T23:30:08Z","receivedAt":"2026-03-19T23:30:46Z","isPatch":true,"sender":{"key":"ychin.git@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1217449?v=4"},"body":"Great. Sounds good, thanks!\n\n\nOn Fri, Mar 13, 2026 at 3:23 AM Phillip Wood wrote:\n>\n> On 13/03/2026 07:07, Junio C Hamano wrote:\n> > \"Yee Cheng Chin via GitGitGadget\" writes:\n> >\n> >> From: Yee Cheng Chin \n> >>\n> >> After a diff algorithm has been run, the compaction phase\n> >> (xdl_change_compact()) shifts and merges change groups to produce a\n> >> cleaner output. However, this shifting could create a new matched group\n> >> where both sides now have matching lines. This results in a\n> >> wrong-looking diff output which contains redundant lines that are the\n> >> same on both files.\n> >>\n> >> Fix this by detecting this situation, and re-diff the texts on each side\n> >> to find similar lines, using the fall-back Myer's diff. Only do this for\n> >> histogram diff as it's the only algorithm where this is relevant. Below\n> >> contains an example, and more details.\n> >> ...\n> >> This issue is rare in a normal repository. Below is a table of\n> >> repositories (`git log --no-merges -p --histogram -1000`), showing how\n> >> many times a re-diff was done and how many times it resulted in finding\n> >> matching lines (therefore addressing this issue) with the fix. In\n> >> general it is fewer than 1% of diff's that exhibit this offending\n> >> behavior:\n> >>\n> >> | Repo (1k commits) | Re-diff | Found matching lines |\n> >> |--------------------|---------|----------------------|\n> >> | llvm-project | 45 | 11 |\n> >> | vim | 110 | 9 |\n> >> | git | 18 | 2 |\n> >> | WebKit | 168 | 1 |\n> >> | ripgrep | 22 | 1 |\n> >> | cpython | 32 | 0 |\n> >> | vscode | 13 | 0 |\n> >>\n> >> Signed-off-by: Yee Cheng Chin \n> >> ---\n> >\n> > Thanks for the updated patch, and sorry for nobody responding to the\n> > patch for over a week.\n>\n> Yes, sorry for the slow response. I agree with Junio that this is\n> explained well and looks good\n>\n> Thanks\n>\n> Phillip\n>\n> > The detailed explanation of the issue and the inclusion of the\n> > repository analysis results are very helpful; they clearly show that\n> > while this is a rare edge case, it significantly improves the\n> > quality of histogram diffs when it does occur.\n> >\n> > - The removal of go_orig is correct since g and go are kept in sync\n> > throughout the slide loops.\n> >\n> > - Clearing the algorithm mask while preserving other flags ensures that\n> > user-provided options like --ignore-all-space are correctly applied\n> > during the re-diff.\n> >\n> > - While ignore_regex and anchors are not passed to the sub-diff, they\n> > aren't currently available to xdl_change_compact anyway. Given that\n> > compaction happens before regex filtering in the main pipeline, this\n> > is OK, I guess.\n> >\n> > Let me mark the topic for 'next'.\n> >\n>\n"}]}