merge-recursive: honor diff.algorithm

Commit Message

Antonin Delpeuch July 8, 2024, 9:34 a.m. UTC

From: Antonin Delpeuch <[e-mail]>

The documentation claims that "recursive defaults to the diff.algorithm
config setting", but this is currently not the case. This fixes it,
ensuring that diff.algorithm is used when -Xdiff-algorithm is not
supplied. This affects the following porcelain commands: "merge",
"rebase", "cherry-pick", "pull", "stash", "log", "am" and "checkout".
It also affects the "merge-tree" ancillary interrogator.

This change also affects the "replay" and "merge-recursive" plumbing
commands, which happen to call 'merge_recursive_config' and therefore
are also affected by other configuration variables read in this
function. For instance theay read "diff.renames", classified in diff.c
as a diff "UI" config variable. Removing the reliance of those
commands on this set of configuration variables feels like a bigger
change and introducing an argument to 'merge_recursive_config' to
prevent only the newly added diff.algorithm to be read by plumbing
commands feels like muddying the architecture, as this function
should likely not be called at all by plumbing commands.

Signed-off-by: Antonin Delpeuch <[e-mail]>
---
    merge-recursive: honor diff.algorithm

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1743%2Fwetneb%2Frecursive_respects_diff.algorithm-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1743/wetneb/recursive_respects_diff.algorithm-v1
Pull-Request: https://github.com/git/git/pull/1743

 builtin/merge-recursive.c   |  4 ++++
 builtin/replay.c            |  4 ++++
 merge-recursive.c           |  7 ++++++
 t/t3515-cherry-pick-diff.sh | 41 +++++++++++++++++++++++++++++++++++
 t/t3515/base.c              | 17 +++++++++++++++
 t/t3515/ours.c              | 17 +++++++++++++++
 t/t3515/theirs.c            | 17 +++++++++++++++
 t/t7615-merge-diff.sh       | 43 +++++++++++++++++++++++++++++++++++++
 8 files changed, 150 insertions(+)
 create mode 100755 t/t3515-cherry-pick-diff.sh
 create mode 100644 t/t3515/base.c
 create mode 100644 t/t3515/ours.c
 create mode 100644 t/t3515/theirs.c
 create mode 100755 t/t7615-merge-diff.sh


base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558

Comments

Antonin Delpeuch July 9, 2024, 5:16 p.m. UTC | #1

On 08/07/2024 11:34, Antonin Delpeuch via GitGitGadget wrote:
> introducing an argument to 'merge_recursive_config' to
> prevent only the newly added diff.algorithm to be read by plumbing
> commands feels like muddying the architecture, as this function
> should likely not be called at all by plumbing commands.

I have second thoughts about this, perhaps it is possible to refactor
things a bit further, imitating diff.c which has "git_diff_ui_config"
and "git_diff_basic_config". In a similar way, we could have
"init_merge_ui_options" and "init_merge_basic_options" which the
commands could call depending on whether they are porcelain or plumbing.
This would make it easier to remove the current dependencies of plumbing
commands on some config variables classified as UI. I'll have a try.

Best,

Antonin

Junio C Hamano July 9, 2024, 6:51 p.m. UTC | #2

Antonin Delpeuch <[e-mail]> writes:

> I have second thoughts about this, perhaps it is possible to refactor
> things a bit further, imitating diff.c which has "git_diff_ui_config"
> and "git_diff_basic_config". In a similar way, we could have
> "init_merge_ui_options" and "init_merge_basic_options" which the
> commands could call depending on whether they are porcelain or plumbing.

It does make sense to treat the internal merge_recursive() function
as robust and reliable building block whose behaviour does not get
affected by configuration beyond the control of the caller, just
like we treat a plumbing command.

Thanks.

Patch

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index c2ce044a201..c14158fd1db 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -31,6 +31,10 @@  int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)
 	char *better1, *better2;
 	struct commit *result;
 
+	/*
+	 * FIXME: This reads various config variables,
+	 * which 'merge-recursive' should ignore as a plumbing command
+	 */
 	init_merge_options(&o, the_repository);
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
diff --git a/builtin/replay.c b/builtin/replay.c
index 6bf0691f15d..98feb6f6320 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -373,6 +373,10 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 
+	/*
+	 * FIXME: This reads various config variables,
+	 * which 'replay' should ignore as a plumbing command
+	 */
 	init_merge_options(&merge_opt, the_repository);
 	memset(&result, 0, sizeof(result));
 	merge_opt.show_rename_progress = 0;
diff --git a/merge-recursive.c b/merge-recursive.c
index 46ee364af73..205fb8aa72d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3930,6 +3930,13 @@  static void merge_recursive_config(struct merge_options *opt)
 		} /* avoid erroring on values from future versions of git */
 		free(value);
 	}
+	if (!git_config_get_string("diff.algorithm", &value)) {
+		long diff_algorithm = parse_algorithm_value(value);
+		if (diff_algorithm < 0)
+			die(_("unknown value for config '%s': %s"), "diff.algorithm", value);
+		opt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;
+		free(value);
+	}
 	git_config(git_xmerge_config, NULL);
 }
 
diff --git a/t/t3515-cherry-pick-diff.sh b/t/t3515-cherry-pick-diff.sh
new file mode 100755
index 00000000000..caeaa01c590
--- /dev/null
+++ b/t/t3515-cherry-pick-diff.sh
@@ -0,0 +1,41 @@ 
+#!/bin/sh
+
+test_description='git cherry-pick
+
+Testing the influence of the diff algorithm on the merge output.'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	cp "$TEST_DIRECTORY"/t3515/base.c file.c &&
+	git add file.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	cp "$TEST_DIRECTORY"/t3515/ours.c file.c &&
+	git add file.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	cp "$TEST_DIRECTORY"/t3515/theirs.c file.c &&
+	git add file.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '
+	git reset --hard c1 &&
+	test_must_fail git cherry-pick -s recursive c2
+'
+
+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '
+	git reset --hard c1 &&
+	git cherry-pick --strategy recursive -Xdiff-algorithm=histogram c2
+'
+
+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '
+	git reset --hard c1 &&
+	git config diff.algorithm histogram &&
+	git cherry-pick --strategy recursive c2
+'
+test_done
diff --git a/t/t3515/base.c b/t/t3515/base.c
new file mode 100644
index 00000000000..c64abc59366
--- /dev/null
+++ b/t/t3515/base.c
@@ -0,0 +1,17 @@ 
+int f(int x, int y)
+{
+        if (x == 0)
+        {
+                return y;
+        }
+        return x;
+}
+
+int g(size_t u)
+{
+        while (u < 30)
+        {
+                u++;
+        }
+        return u;
+}
diff --git a/t/t3515/ours.c b/t/t3515/ours.c
new file mode 100644
index 00000000000..44d82513970
--- /dev/null
+++ b/t/t3515/ours.c
@@ -0,0 +1,17 @@ 
+int g(size_t u)
+{
+        while (u < 30)
+        {
+                u++;
+        }
+        return u;
+}
+
+int h(int x, int y, int z)
+{
+        if (z == 0)
+        {
+                return x;
+        }
+        return y;
+}
diff --git a/t/t3515/theirs.c b/t/t3515/theirs.c
new file mode 100644
index 00000000000..85f02146fee
--- /dev/null
+++ b/t/t3515/theirs.c
@@ -0,0 +1,17 @@ 
+int f(int x, int y)
+{
+        if (x == 0)
+        {
+                return y;
+        }
+        return x;
+}
+
+int g(size_t u)
+{
+        while (u > 34)
+        {
+                u--;
+        }
+        return u;
+}
diff --git a/t/t7615-merge-diff.sh b/t/t7615-merge-diff.sh
new file mode 100755
index 00000000000..be335c7c3d1
--- /dev/null
+++ b/t/t7615-merge-diff.sh
@@ -0,0 +1,43 @@ 
+#!/bin/sh
+
+test_description='git merge
+
+Testing the influence of the diff algorithm on the merge output.'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	cp "$TEST_DIRECTORY"/t3515/base.c file.c &&
+	git add file.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	cp "$TEST_DIRECTORY"/t3515/ours.c file.c &&
+	git add file.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	cp "$TEST_DIRECTORY"/t3515/theirs.c file.c &&
+	git add file.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+GIT_TEST_MERGE_ALGORITHM=recursive
+
+test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '
+	git reset --hard c1 &&
+	test_must_fail git merge -s recursive c2
+'
+
+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '
+	git reset --hard c1 &&
+	git merge --strategy recursive -Xdiff-algorithm=histogram c2
+'
+
+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '
+	git reset --hard c1 &&
+	git config diff.algorithm histogram &&
+	git merge --strategy recursive c2
+'
+test_done