| Message ID | pull.1743.v2.git.git.1720551701648.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 6db73d03df19a1faa655d6a0a31098fe9e36802b |
| Headers | show |
| Series | [v2] merge-recursive: honor diff.algorithm | expand |
"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Antonin Delpeuch <antonin@delpeuch.eu> > > 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. Unfortunate. Since be733e12 (Merge branch 'en/merge-tree', 2022-07-14), merge-tree is no longer an interrogator but works as an manipulator. As it is meant to be used as a building block that gives a reliable and repeatable output, I am tempted to say it should be categorized as a plumbing, but second opinions do count. Elijah Cc'ed as it was his "fault" to add "--write-tree" mode to the command and forgetting to update command-list.txt ;-) But I agree with the direction of this patch and the structure of the solution (i.e. have two variants of init_*_options()). > 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. In other words, these two are the only ones that use the _basic variant. I am unsure (read: do not take this as my recommendation to change your patch) which one merge-tree should use, but other than that, nicely done. > diff --git a/log-tree.c b/log-tree.c > index 101079e8200..5d8fb6ff8df 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"; Isn't log-tree shared with things like "git diff-tree" porcelain? > -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; > @@ -3930,11 +3930,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); > } This looks sensible. Even though we have a single merge_recursive() that is internally callable, depending on the callers, they may or may not want to be affected by configuration. As to the tests, it felt a bit unnatural and error prone to make t7615 depend on material that appears to be made only for t3515 (by naming the directory as such). We have not done "a test-material directory that is shared among multiple tests" in t/, but we have plenty of "test helpers that are shared across multiple tests" named lib-foo.sh. I wonder if doing something like ... in t/lib-histogram-merge-history.sh ... # prepare history for merges that depend on diff.algorithm setup_history_for_histogram () { cat >file.c <<\EOF && ... contents of base.c ... EOF git add file.c && git commit -m c0 && git tag c0 && cat >file.c <<\EOF && ... contents of ours.c ... EOF ... git tag c2 } and make the setup step in t3515 (and t7615) use that shared set-up function like so: . ./test-lib.sh . "$TEST_DIRECTORY/test-lib-histogram-merge/history.sh" test_expect_success setup ' setup_history_for_histogram ' may be cleaner? I am mostly afraid of mistakes like "now we are done with the area 3515 covered let's remove all the traces of it, like t3515-cherry-pick-diff.sh and t3515/ directory", breaking an seemingly unrelated t7615. Even better. Can't we save the scarce resource that is test number and make these not about "I test cherry-pick" and "I test merge"? You are testing how mergy operations are affected by the choice of diff.algorithm, so perhaps create a single test file and name it after that single shared aspect of the tests you are adding? Perhaps t/t7615-diff-algo-with-mergy-operations.sh that has all three of these: * the setup_history_for_histogram() helper function as described above; * the test for cherry-pick in this patch; * the test for merge in this patch. Thanks.
On 10/07/2024 19:58, Junio C Hamano wrote: > Since be733e12 (Merge branch 'en/merge-tree', 2022-07-14), > merge-tree is no longer an interrogator but works as an manipulator. > As it is meant to be used as a building block that gives a reliable > and repeatable output, I am tempted to say it should be categorized > as a plumbing, but second opinions do count. Elijah Cc'ed as it was > his "fault" to add "--write-tree" mode to the command and forgetting > to update command-list.txt ;-) I'm happy to change it to use the "basic" config if you prefer. I have to admit I don't have a good overview of what's porcelain or plumbing. > Isn't log-tree shared with things like "git diff-tree" porcelain? I'm happy to also change this, but looking at the call hierarchy we seem to have a complicated entanglement of porcelain and plumbing commands here, so to separate them is probably more involved. > Can't we save the scarce resource that is test number > and make these not about "I test cherry-pick" and "I test merge"? Yes I agree, let me submit a new patch which does that already. I wasn't sure if it was worth including tests for different commands anyway. Antonin
diff --git a/builtin/am.c b/builtin/am.c index 8f9619ea3a3..b821561b15a 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 3cf44b4683a..5769efaca00 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 c2ce044a201..9e9d0b57158 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 1082d919fd1..aab0843ff5a 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -570,7 +570,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 66a4fa72e1c..686326bc1d3 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -720,7 +720,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 6bf0691f15d..d90ddd0837d 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -373,7 +373,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 7859bc0866a..86803755f03 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 101079e8200..5d8fb6ff8df 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 46ee364af73..cd9bd3c03ef 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3901,7 +3901,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; @@ -3930,11 +3930,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)); @@ -3953,7 +3962,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); @@ -3961,6 +3970,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 e67d38c3030..85a5c332bbb 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 b4f055e5a85..3608374166a 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)"; @@ -4308,7 +4308,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/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