Merge branch 'yc/histogram-hunk-shift-fix' into jch

The final clean-up phase of the diff output could turn the result of
histogram diff algorithm suboptimal, which has been corrected.

* yc/histogram-hunk-shift-fix:
  xdiff: re-diff shifted change groups when using histogram algorithm

t/meson.build

@@ -506,6 +506,7 @@ integration_tests = [
   't4071-diff-minimal.sh',
   't4072-diff-max-depth.sh',
   't4073-diff-stat-name-width.sh',
+  't4074-diff-shifted-matched-group.sh',
   't4100-apply-stat.sh',
   't4101-apply-nonl.sh',
   't4102-apply-rename.sh',

t/t4074-diff-shifted-matched-group.sh

@@ -0,0 +1,164 @@
+#!/bin/sh
+
+test_description='shifted diff groups re-diffing during histogram diff'
+
+. ./test-lib.sh
+
+test_expect_success 'shifted/merged diff group should re-diff to minimize patch' '
+	test_write_lines A x A A A x A A A >file1 &&
+	test_write_lines A x A Z A x A A A >file2 &&
+
+	file1_h=$(git rev-parse --short $(git hash-object file1)) &&
+	file2_h=$(git rev-parse --short $(git hash-object file2)) &&
+
+	cat >expect <<-EOF &&
+	diff --git a/file1 b/file2
+	index $file1_h..$file2_h 100644
+	--- a/file1
+	+++ b/file2
+	@@ -1,7 +1,7 @@
+	 A
+	 x
+	 A
+	-A
+	+Z
+	 A
+	 x
+	 A
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram file1 file2 >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'merged diff group with no shift' '
+	test_write_lines A Z B x >file1 &&
+	test_write_lines C D x Z E x >file2 &&
+
+	file1_h=$(git rev-parse --short $(git hash-object file1)) &&
+	file2_h=$(git rev-parse --short $(git hash-object file2)) &&
+
+	cat >expect <<-EOF &&
+	diff --git a/file1 b/file2
+	index $file1_h..$file2_h 100644
+	--- a/file1
+	+++ b/file2
+	@@ -1,4 +1,6 @@
+	-A
+	+C
+	+D
+	+x
+	 Z
+	-B
+	+E
+	 x
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram file1 file2 >output &&
+	test_cmp expect output
+'
+
+test_expect_success 're-diff should preserve diff flags' '
+	test_write_lines a b c a b c >file1 &&
+	test_write_lines x " b" z a b c >file2 &&
+
+	file1_h=$(git rev-parse --short $(git hash-object file1)) &&
+	file2_h=$(git rev-parse --short $(git hash-object file2)) &&
+
+	cat >expect <<-EOF &&
+	diff --git a/file1 b/file2
+	index $file1_h..$file2_h 100644
+	--- a/file1
+	+++ b/file2
+	@@ -1,6 +1,6 @@
+	-a
+	-b
+	-c
+	+x
+	+ b
+	+z
+	 a
+	 b
+	 c
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram file1 file2 >output &&
+	test_cmp expect output &&
+
+	cat >expect_iwhite <<-EOF &&
+	diff --git a/file1 b/file2
+	index $file1_h..$file2_h 100644
+	--- a/file1
+	+++ b/file2
+	@@ -1,6 +1,6 @@
+	-a
+	+x
+	  b
+	-c
+	+z
+	 a
+	 b
+	 c
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram --ignore-all-space file1 file2 >output_iwhite &&
+	test_cmp expect_iwhite output_iwhite
+'
+
+test_expect_success 'shifting on either side should trigger re-diff properly' '
+	test_write_lines a b c a b c a b c >file1 &&
+	test_write_lines a b c a1 a2 a3 b c1 a b c >file2 &&
+
+	file1_h=$(git rev-parse --short $(git hash-object file1)) &&
+	file2_h=$(git rev-parse --short $(git hash-object file2)) &&
+
+	cat >expect1 <<-EOF &&
+	diff --git a/file1 b/file2
+	index $file1_h..$file2_h 100644
+	--- a/file1
+	+++ b/file2
+	@@ -1,9 +1,11 @@
+	 a
+	 b
+	 c
+	-a
+	+a1
+	+a2
+	+a3
+	 b
+	-c
+	+c1
+	 a
+	 b
+	 c
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram file1 file2 >output1 &&
+	test_cmp expect1 output1 &&
+
+	cat >expect2 <<-EOF &&
+	diff --git a/file2 b/file1
+	index $file2_h..$file1_h 100644
+	--- a/file2
+	+++ b/file1
+	@@ -1,11 +1,9 @@
+	 a
+	 b
+	 c
+	-a1
+	-a2
+	-a3
+	+a
+	 b
+	-c1
+	+c
+	 a
+	 b
+	 c
+	EOF
+
+	test_expect_code 1 git diff --no-index --histogram file2 file1 >output2 &&
+	test_cmp expect2 output2
+'
+
+test_done

xdiff/xdiffi.c

@@ -792,6 +792,7 @@ static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)
  */
 int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 	struct xdlgroup g, go;
+	struct xdlgroup g_orig;
 	long earliest_end, end_matching_other;
 	long groupsize;
 
@@ -805,10 +806,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 		if (g.end == g.start)
 			goto next;
 
+		g_orig = g;
+
 		/*
 		 * Now shift the change up and then down as far as possible in
 		 * each direction. If it bumps into any other changes, merge
-		 * them.
+		 * them and restart the process.
 		 */
 		do {
 			groupsize = g.end - g.start;
@@ -861,7 +864,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			/*
 			 * Move the possibly merged group of changes back to
 			 * line up with the last group of changes from the
-			 * other file that it can align with.
+			 * other file that it can align with. This avoids breaking
+			 * a single change into a separate addition/deletion.
 			 */
 			while (go.end == go.start) {
 				if (group_slide_up(xdf, &g))
@@ -914,6 +918,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			}
 		}
 
+		/*
+		 * If we merged change groups during shifting, the new
+		 * combined group could now have matching lines in both files,
+		 * even if the original separate groups did not. Re-diff the
+		 * new group to find these matching lines to mark them as
+		 * unchanged.
+		 *
+		 * Only do this if the corresponding group in the other file is
+		 * non-empty, as it's trivial otherwise.
+		 *
+		 * Only do this for histogram diff as its LCS algorithm allows
+		 * for this scenario. In contrast, patience diff finds LCS
+		 * of unique lines that groups cannot be shifted across.
+		 * Myer's diff (standalone or used as fall-back in patience
+		 * diff) already finds minimal edits so it is not possible for
+		 * shifted groups to result in a smaller diff. (Without
+		 * XDF_NEED_MINIMAL, Myer's isn't technically guaranteed to be
+		 * minimal, but it should be so most of the time)
+		 */
+		if (go.end != go.start &&
+				XDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&
+				(g.start != g_orig.start ||
+				 g.end != g_orig.end)) {
+			xpparam_t xpp;
+			xdfenv_t xe;
+
+			memset(&xpp, 0, sizeof(xpp));
+			xpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;
+
+			xe.xdf1 = *xdf;
+			xe.xdf2 = *xdfo;
+
+			if (xdl_fall_back_diff(&xe, &xpp,
+					       g.start + 1, g.end - g.start,
+					       go.start + 1, go.end - go.start)) {
+				return -1;
+			}
+		}
+
 	next:
 		/* Move past the just-processed group: */
 		if (group_next(xdf, &g))