From 9c93ba4d0aee1bc8c663a13552afd2b2c22863a9 Mon Sep 17 00:00:00 2001 From: Antonin Delpeuch Date: Sat, 13 Jul 2024 16:51:46 +0000 Subject: [PATCH] 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 Signed-off-by: Junio C Hamano --- builtin/am.c | 2 +- builtin/checkout.c | 2 +- builtin/merge-recursive.c | 2 +- builtin/merge-tree.c | 2 +- builtin/merge.c | 2 +- builtin/replay.c | 2 +- builtin/stash.c | 2 +- log-tree.c | 2 +- merge-recursive.c | 29 +++++++++++++-- merge-recursive.h | 5 ++- sequencer.c | 4 +- t/t7615-diff-algo-with-mergy-operations.sh | 60 ++++++++++++++++++++++++++++++ t/t7615/base.c | 17 +++++++++ t/t7615/ours.c | 17 +++++++++ t/t7615/theirs.c | 17 +++++++++ 15 files changed, 150 insertions(+), 15 deletions(-) create mode 100755 t/t7615-diff-algo-with-mergy-operations.sh create mode 100644 t/t7615/base.c create mode 100644 t/t7615/ours.c create mode 100644 t/t7615/theirs.c diff --git a/builtin/am.c b/builtin/am.c index 370f5593f2..a12be088f7 100644 --- 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 3cf44b4683..5769efaca0 100644 --- 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 82bebea15b..e951b09805 100644 --- 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 dab2fdc2a6..9bca9b5f33 100644 --- 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 9fba27d85d..c896b18d1a 100644 --- 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 0448326636..9acf51c32b 100644 --- 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 46b981c4dd..fb75ca219e 100644 --- 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 52feec4356..576ef30d90 100644 --- 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 5cc638066a..ed64a4c537 100644 --- 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 3136c7cc2d..0b91f28f90 100644 --- 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 a2284ac9e9..0291920f0b 100644 --- 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 index 0000000000..9a83be518c --- /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 index 0000000000..c64abc5936 --- /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 index 0000000000..44d8251397 --- /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 index 0000000000..85f02146fe --- /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; +} -- 2.11.4.GIT