merge-recursive: honor diff.algorithm

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 refactors the initialization of merge options to introduce
two functions, "init_merge_ui_options" and "init_merge_basic_options"
instead of just one "init_merge_options". This design follows the
approach used in diff.c, providing initialization methods for
porcelain and plumbing commands respectively. Thanks to that, the
"replay" and "merge-recursive" plumbing commands remain unaffected by
diff.algorithm.

Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/am.c b/builtin/am.c
index 370f5593f23ae53ba573cac342a02b9044d2328b..a12be088f7b7e566af6f62cf0d8ba8ac8076b554 100644 (file)
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1630,7 +1630,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
         * changes.
         */
 
-       init_merge_options(&o, the_repository);
+       init_ui_merge_options(&o, the_repository);
 
        o.branch1 = "HEAD";
        their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3cf44b4683a9ba48abaaa32ef20bc127920865bd..5769efaca0007a784e0e1acabb44eaff4315858d 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -884,7 +884,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 
                        add_files_to_cache(the_repository, NULL, NULL, NULL, 0,
                                           0);
-                       init_merge_options(&o, the_repository);
+                       init_ui_merge_options(&o, the_repository);
                        o.verbosity = 0;
                        work = write_in_core_index_as_tree(the_repository);
 

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 82bebea15b3fd034fc590b115d92418def143040..e951b09805dd872ea3a8eeadf056c4d069483bd5 100644 (file)
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -31,7 +31,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)
        char *better1, *better2;
        struct commit *result;
 
-       init_merge_options(&o, the_repository);
+       init_basic_merge_options(&o, the_repository);
        if (argv[0] && ends_with(argv[0], "-subtree"))
                o.subtree_shift = "";
 

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index dab2fdc2a6ba1eb2debfc6f797413c8474b9f4b4..9bca9b5f33c7c1bd40564b8936004224f17b482e 100644 (file)
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -571,7 +571,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
        };
 
        /* Init merge options */
-       init_merge_options(&o.merge_options, the_repository);
+       init_ui_merge_options(&o.merge_options, the_repository);
 
        /* Parse arguments */
        original_argc = argc - 1; /* ignoring argv[0] */

diff --git a/builtin/merge.c b/builtin/merge.c
index 9fba27d85dfdda2e84bae6257138c0b6f6f6ceed..c896b18d1a95713d04c6146736b75a669ef713e0 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -724,7 +724,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
                        return 2;
                }
 
-               init_merge_options(&o, the_repository);
+               init_ui_merge_options(&o, the_repository);
                if (!strcmp(strategy, "subtree"))
                        o.subtree_shift = "";
 

diff --git a/builtin/replay.c b/builtin/replay.c
index 04483266367bb677d8459e700e0de8a2d133c6c5..9acf51c32b60070f9ebc764d7199ed6987917b0c 100644 (file)
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -377,7 +377,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
                goto cleanup;
        }
 
-       init_merge_options(&merge_opt, the_repository);
+       init_basic_merge_options(&merge_opt, the_repository);
        memset(&result, 0, sizeof(result));
        merge_opt.show_rename_progress = 0;
        last_commit = onto;

diff --git a/builtin/stash.c b/builtin/stash.c
index 46b981c4dd3cd998aa443d5f7c7652aa9a628dd2..fb75ca219eaba5c5ac0f6654a24ad2e5e92dad7f 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -574,7 +574,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
                }
        }
 
-       init_merge_options(&o, the_repository);
+       init_ui_merge_options(&o, the_repository);
 
        o.branch1 = "Updated upstream";
        o.branch2 = "Stashed changes";

diff --git a/log-tree.c b/log-tree.c
index 52feec4356ab12e206dcf79d313c65c07c49f75f..576ef30d9098dcaa0b43eb8f4dae5be30b64ec98 100644 (file)
--- a/log-tree.c
+++ b/log-tree.c
@@ -1025,7 +1025,7 @@ static int do_remerge_diff(struct rev_info *opt,
        struct strbuf parent2_desc = STRBUF_INIT;
 
        /* Setup merge options */
-       init_merge_options(&o, the_repository);
+       init_ui_merge_options(&o, the_repository);
        o.show_rename_progress = 0;
        o.record_conflict_msgs_as_headers = 1;
        o.msg_header_prefix = "remerge";

diff --git a/merge-recursive.c b/merge-recursive.c
index 5cc638066af599f5598ab65d93307c534ca2f1a4..ed64a4c537ca8925c0336164a5ae30a4fd7cd6fe 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3921,7 +3921,7 @@ int merge_recursive_generic(struct merge_options *opt,
        return clean ? 0 : 1;
 }
 
-static void merge_recursive_config(struct merge_options *opt)
+static void merge_recursive_config(struct merge_options *opt, int ui)
 {
        char *value = NULL;
        int renormalize = 0;
@@ -3950,11 +3950,20 @@ static void merge_recursive_config(struct merge_options *opt)
                } /* avoid erroring on values from future versions of git */
                free(value);
        }
+       if (ui) {
+               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);
 }
 
-void init_merge_options(struct merge_options *opt,
-                       struct repository *repo)
+static void init_merge_options(struct merge_options *opt,
+                       struct repository *repo, int ui)
 {
        const char *merge_verbosity;
        memset(opt, 0, sizeof(struct merge_options));
@@ -3973,7 +3982,7 @@ void init_merge_options(struct merge_options *opt,
 
        opt->conflict_style = -1;
 
-       merge_recursive_config(opt);
+       merge_recursive_config(opt, ui);
        merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
        if (merge_verbosity)
                opt->verbosity = strtol(merge_verbosity, NULL, 10);
@@ -3981,6 +3990,18 @@ void init_merge_options(struct merge_options *opt,
                opt->buffer_output = 0;
 }
 
+void init_ui_merge_options(struct merge_options *opt,
+                       struct repository *repo)
+{
+       init_merge_options(opt, repo, 1);
+}
+
+void init_basic_merge_options(struct merge_options *opt,
+                       struct repository *repo)
+{
+       init_merge_options(opt, repo, 0);
+}
+
 /*
  * For now, members of merge_options do not need deep copying, but
  * it may change in the future, in which case we would need to update

diff --git a/merge-recursive.h b/merge-recursive.h
index 3136c7cc2dfb47435e0a34dedfb753ca008215a4..0b91f28f9022793920c01edd5bb2520fbdad7984 100644 (file)
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -54,7 +54,10 @@ struct merge_options {
        struct merge_options_internal *priv;
 };
 
-void init_merge_options(struct merge_options *opt, struct repository *repo);
+/* for use by porcelain commands */
+void init_ui_merge_options(struct merge_options *opt, struct repository *repo);
+/* for use by plumbing commands */
+void init_basic_merge_options(struct merge_options *opt, struct repository *repo);
 
 void copy_merge_options(struct merge_options *dst, struct merge_options *src);
 void clear_merge_options(struct merge_options *opt);

diff --git a/sequencer.c b/sequencer.c
index a2284ac9e953fca5bffb7addf6c9d7fbbedc3492..0291920f0b753fe30eb6c6e456017d4f9cc43607 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -762,7 +762,7 @@ static int do_recursive_merge(struct repository *r,
 
        repo_read_index(r);
 
-       init_merge_options(&o, r);
+       init_ui_merge_options(&o, r);
        o.ancestor = base ? base_label : "(empty tree)";
        o.branch1 = "HEAD";
        o.branch2 = next ? next_label : "(empty tree)";
@@ -4309,7 +4309,7 @@ static int do_merge(struct repository *r,
        bases = reverse_commit_list(bases);
 
        repo_read_index(r);
-       init_merge_options(&o, r);
+       init_ui_merge_options(&o, r);
        o.branch1 = "HEAD";
        o.branch2 = ref_name.buf;
        o.buffer_output = 2;

diff --git a/t/t7615-diff-algo-with-mergy-operations.sh b/t/t7615-diff-algo-with-mergy-operations.sh
new file mode 100755 (executable)
index 0000000..9a83be5
--- /dev/null
+++ b/t/t7615-diff-algo-with-mergy-operations.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='git merge and other operations that rely on 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"/t7615/base.c file.c &&
+       git add file.c &&
+       git commit -m c0 &&
+       git tag c0 &&
+       cp "$TEST_DIRECTORY"/t7615/ours.c file.c &&
+       git add file.c &&
+       git commit -m c1 &&
+       git tag c1 &&
+       git reset --hard c0 &&
+       cp "$TEST_DIRECTORY"/t7615/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_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/t7615/base.c b/t/t7615/base.c
new file mode 100644 (file)
index 0000000..c64abc5
--- /dev/null
+++ b/t/t7615/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/t7615/ours.c b/t/t7615/ours.c
new file mode 100644 (file)
index 0000000..44d8251
--- /dev/null
+++ b/t/t7615/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/t7615/theirs.c b/t/t7615/theirs.c
new file mode 100644 (file)
index 0000000..85f0214
--- /dev/null
+++ b/t/t7615/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;
+}