{"thread":{"id":"61751","subject":"[PATCH] merge-recursive: honor diff.algorithm","startedAt":"2024-07-08T09:34:52Z","lastAt":"2024-07-13T17:04:01Z","messageCount":7,"participants":["Antonin Delpeuch via GitGitGadget","Antonin Delpeuch","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498229","messageId":"pull.1743.git.git.1720431288496.gitgitgadget@gmail.com","threadId":"61751","inReplyTo":null,"subject":"[PATCH] merge-recursive: honor diff.algorithm","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-08T09:34:48Z","receivedAt":"2024-07-08T09:34:52Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nThe documentation claims that \"recursive defaults to the diff.algorithm\nconfig setting\", but this is currently not the case. This fixes it,\nensuring that diff.algorithm is used when -Xdiff-algorithm is not\nsupplied. This affects the following porcelain commands: \"merge\",\n\"rebase\", \"cherry-pick\", \"pull\", \"stash\", \"log\", \"am\" and \"checkout\".\nIt also affects the \"merge-tree\" ancillary interrogator.\n\nThis change also affects the \"replay\" and \"merge-recursive\" plumbing\ncommands, which happen to call 'merge_recursive_config' and therefore\nare also affected by other configuration variables read in this\nfunction. For instance theay read \"diff.renames\", classified in diff.c\nas a diff \"UI\" config variable. Removing the reliance of those\ncommands on this set of configuration variables feels like a bigger\nchange and introducing an argument to 'merge_recursive_config' to\nprevent only the newly added diff.algorithm to be read by plumbing\ncommands feels like muddying the architecture, as this function\nshould likely not be called at all by plumbing commands.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-recursive: honor diff.algorithm\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1743%2Fwetneb%2Frecursive_respects_diff.algorithm-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1743/wetneb/recursive_respects_diff.algorithm-v1\nPull-Request: https://github.com/git/git/pull/1743\n\n builtin/merge-recursive.c   |  4 ++++\n builtin/replay.c            |  4 ++++\n merge-recursive.c           |  7 ++++++\n t/t3515-cherry-pick-diff.sh | 41 +++++++++++++++++++++++++++++++++++\n t/t3515/base.c              | 17 +++++++++++++++\n t/t3515/ours.c              | 17 +++++++++++++++\n t/t3515/theirs.c            | 17 +++++++++++++++\n t/t7615-merge-diff.sh       | 43 +++++++++++++++++++++++++++++++++++++\n 8 files changed, 150 insertions(+)\n create mode 100755 t/t3515-cherry-pick-diff.sh\n create mode 100644 t/t3515/base.c\n create mode 100644 t/t3515/ours.c\n create mode 100644 t/t3515/theirs.c\n create mode 100755 t/t7615-merge-diff.sh\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex c2ce044a201..c14158fd1db 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -31,6 +31,10 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)\n \tchar *better1, *better2;\n \tstruct commit *result;\n \n+\t/*\n+\t * FIXME: This reads various config variables,\n+\t * which 'merge-recursive' should ignore as a plumbing command\n+\t */\n \tinit_merge_options(&o, the_repository);\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\ndiff --git a/builtin/replay.c b/builtin/replay.c\nindex 6bf0691f15d..98feb6f6320 100644\n--- a/builtin/replay.c\n+++ b/builtin/replay.c\n@@ -373,6 +373,10 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \t}\n \n+\t/*\n+\t * FIXME: This reads various config variables,\n+\t * which 'replay' should ignore as a plumbing command\n+\t */\n \tinit_merge_options(&merge_opt, the_repository);\n \tmemset(&result, 0, sizeof(result));\n \tmerge_opt.show_rename_progress = 0;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 46ee364af73..205fb8aa72d 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3930,6 +3930,13 @@ static void merge_recursive_config(struct merge_options *opt)\n \t\t} /* avoid erroring on values from future versions of git */\n \t\tfree(value);\n \t}\n+\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n+\t\tlong diff_algorithm = parse_algorithm_value(value);\n+\t\tif (diff_algorithm < 0)\n+\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n+\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n+\t\tfree(value);\n+\t}\n \tgit_config(git_xmerge_config, NULL);\n }\n \ndiff --git a/t/t3515-cherry-pick-diff.sh b/t/t3515-cherry-pick-diff.sh\nnew file mode 100755\nindex 00000000000..caeaa01c590\n--- /dev/null\n+++ b/t/t3515-cherry-pick-diff.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='git cherry-pick\n+\n+Testing the influence of the diff algorithm on the merge output.'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git cherry-pick -s recursive c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit cherry-pick --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit cherry-pick --strategy recursive c2\n+'\n+test_done\ndiff --git a/t/t3515/base.c b/t/t3515/base.c\nnew file mode 100644\nindex 00000000000..c64abc59366\n--- /dev/null\n+++ b/t/t3515/base.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\ndiff --git a/t/t3515/ours.c b/t/t3515/ours.c\nnew file mode 100644\nindex 00000000000..44d82513970\n--- /dev/null\n+++ b/t/t3515/ours.c\n@@ -0,0 +1,17 @@\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\n+\n+int h(int x, int y, int z)\n+{\n+        if (z == 0)\n+        {\n+                return x;\n+        }\n+        return y;\n+}\ndiff --git a/t/t3515/theirs.c b/t/t3515/theirs.c\nnew file mode 100644\nindex 00000000000..85f02146fee\n--- /dev/null\n+++ b/t/t3515/theirs.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u > 34)\n+        {\n+                u--;\n+        }\n+        return u;\n+}\ndiff --git a/t/t7615-merge-diff.sh b/t/t7615-merge-diff.sh\nnew file mode 100755\nindex 00000000000..be335c7c3d1\n--- /dev/null\n+++ b/t/t7615-merge-diff.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='git merge\n+\n+Testing the influence of the diff algorithm on the merge output.'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+GIT_TEST_MERGE_ALGORITHM=recursive\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git merge -s recursive c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit merge --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit merge --strategy recursive c2\n+'\n+test_done\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498370","messageId":"198a4c00-291f-456d-84ac-082e142bd4fe@delpeuch.eu","threadId":"61751","inReplyTo":"pull.1743.git.git.1720431288496.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-recursive: honor diff.algorithm","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-07-09T17:16:52Z","receivedAt":"2024-07-09T17:29:58Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"On 08/07/2024 11:34, Antonin Delpeuch via GitGitGadget wrote:\n> introducing an argument to 'merge_recursive_config' to\n> prevent only the newly added diff.algorithm to be read by plumbing\n> commands feels like muddying the architecture, as this function\n> should likely not be called at all by plumbing commands.\n\nI have second thoughts about this, perhaps it is possible to refactor\nthings a bit further, imitating diff.c which has \"git_diff_ui_config\"\nand \"git_diff_basic_config\". In a similar way, we could have\n\"init_merge_ui_options\" and \"init_merge_basic_options\" which the\ncommands could call depending on whether they are porcelain or plumbing.\nThis would make it easier to remove the current dependencies of plumbing\ncommands on some config variables classified as UI. I'll have a try.\n\nBest,\n\nAntonin\n\n"},{"id":"498372","messageId":"xmqqy16ae6vj.fsf@gitster.g","threadId":"61751","inReplyTo":"198a4c00-291f-456d-84ac-082e142bd4fe@delpeuch.eu","subject":"Re: [PATCH] merge-recursive: honor diff.algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-09T18:51:28Z","receivedAt":"2024-07-09T18:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonin Delpeuch <antonin@delpeuch.eu> writes:\n\n> I have second thoughts about this, perhaps it is possible to refactor\n> things a bit further, imitating diff.c which has \"git_diff_ui_config\"\n> and \"git_diff_basic_config\". In a similar way, we could have\n> \"init_merge_ui_options\" and \"init_merge_basic_options\" which the\n> commands could call depending on whether they are porcelain or plumbing.\n\nIt does make sense to treat the internal merge_recursive() function\nas robust and reliable building block whose behaviour does not get\naffected by configuration beyond the control of the caller, just\nlike we treat a plumbing command.\n\nThanks.\n"},{"id":"498374","messageId":"pull.1743.v2.git.git.1720551701648.gitgitgadget@gmail.com","threadId":"61751","inReplyTo":"pull.1743.git.git.1720431288496.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-recursive: honor diff.algorithm","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-09T19:01:41Z","receivedAt":"2024-07-09T19:01:45Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nThe documentation claims that \"recursive defaults to the diff.algorithm\nconfig setting\", but this is currently not the case. This fixes it,\nensuring that diff.algorithm is used when -Xdiff-algorithm is not\nsupplied. This affects the following porcelain commands: \"merge\",\n\"rebase\", \"cherry-pick\", \"pull\", \"stash\", \"log\", \"am\" and \"checkout\".\nIt also affects the \"merge-tree\" ancillary interrogator.\n\nThis change refactors the initialization of merge options to introduce\ntwo functions, \"init_merge_ui_options\" and \"init_merge_basic_options\"\ninstead of just one \"init_merge_options\". This design follows the\napproach used in diff.c, providing initialization methods for\nporcelain and plumbing commands respectively. Thanks to that, the\n\"replay\" and \"merge-recursive\" plumbing commands remain unaffected by\ndiff.algorithm.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-recursive: honor diff.algorithm\n    \n    Changes since v1:\n    \n     * introduce separate initialization methods for porcelain and plumbing\n       commands\n     * adapt commit message accordingly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1743%2Fwetneb%2Frecursive_respects_diff.algorithm-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1743/wetneb/recursive_respects_diff.algorithm-v2\nPull-Request: https://github.com/git/git/pull/1743\n\nRange-diff vs v1:\n\n 1:  798b1612189 ! 1:  9c1907fad43 merge-recursive: honor diff.algorithm\n     @@ Commit message\n          \"rebase\", \"cherry-pick\", \"pull\", \"stash\", \"log\", \"am\" and \"checkout\".\n          It also affects the \"merge-tree\" ancillary interrogator.\n      \n     -    This change also affects the \"replay\" and \"merge-recursive\" plumbing\n     -    commands, which happen to call 'merge_recursive_config' and therefore\n     -    are also affected by other configuration variables read in this\n     -    function. For instance theay read \"diff.renames\", classified in diff.c\n     -    as a diff \"UI\" config variable. Removing the reliance of those\n     -    commands on this set of configuration variables feels like a bigger\n     -    change and introducing an argument to 'merge_recursive_config' to\n     -    prevent only the newly added diff.algorithm to be read by plumbing\n     -    commands feels like muddying the architecture, as this function\n     -    should likely not be called at all by plumbing commands.\n     +    This change refactors the initialization of merge options to introduce\n     +    two functions, \"init_merge_ui_options\" and \"init_merge_basic_options\"\n     +    instead of just one \"init_merge_options\". This design follows the\n     +    approach used in diff.c, providing initialization methods for\n     +    porcelain and plumbing commands respectively. Thanks to that, the\n     +    \"replay\" and \"merge-recursive\" plumbing commands remain unaffected by\n     +    diff.algorithm.\n      \n          Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n      \n     + ## builtin/am.c ##\n     +@@ builtin/am.c: static int fall_back_threeway(const struct am_state *state, const char *index_pa\n     + \t * changes.\n     + \t */\n     + \n     +-\tinit_merge_options(&o, the_repository);\n     ++\tinit_ui_merge_options(&o, the_repository);\n     + \n     + \to.branch1 = \"HEAD\";\n     + \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n     +\n     + ## builtin/checkout.c ##\n     +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n     + \n     + \t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n     + \t\t\t\t\t   0);\n     +-\t\t\tinit_merge_options(&o, the_repository);\n     ++\t\t\tinit_ui_merge_options(&o, the_repository);\n     + \t\t\to.verbosity = 0;\n     + \t\t\twork = write_in_core_index_as_tree(the_repository);\n     + \n     +\n       ## builtin/merge-recursive.c ##\n      @@ builtin/merge-recursive.c: int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)\n       \tchar *better1, *better2;\n       \tstruct commit *result;\n       \n     -+\t/*\n     -+\t * FIXME: This reads various config variables,\n     -+\t * which 'merge-recursive' should ignore as a plumbing command\n     -+\t */\n     - \tinit_merge_options(&o, the_repository);\n     +-\tinit_merge_options(&o, the_repository);\n     ++\tinit_basic_merge_options(&o, the_repository);\n       \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n       \t\to.subtree_shift = \"\";\n     + \n     +\n     + ## builtin/merge-tree.c ##\n     +@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n     + \t};\n     + \n     + \t/* Init merge options */\n     +-\tinit_merge_options(&o.merge_options, the_repository);\n     ++\tinit_ui_merge_options(&o.merge_options, the_repository);\n     + \n     + \t/* Parse arguments */\n     + \toriginal_argc = argc - 1; /* ignoring argv[0] */\n     +\n     + ## builtin/merge.c ##\n     +@@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct commit_list *common,\n     + \t\t\treturn 2;\n     + \t\t}\n     + \n     +-\t\tinit_merge_options(&o, the_repository);\n     ++\t\tinit_ui_merge_options(&o, the_repository);\n     + \t\tif (!strcmp(strategy, \"subtree\"))\n     + \t\t\to.subtree_shift = \"\";\n     + \n      \n       ## builtin/replay.c ##\n      @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)\n       \t\tgoto cleanup;\n       \t}\n       \n     -+\t/*\n     -+\t * FIXME: This reads various config variables,\n     -+\t * which 'replay' should ignore as a plumbing command\n     -+\t */\n     - \tinit_merge_options(&merge_opt, the_repository);\n     +-\tinit_merge_options(&merge_opt, the_repository);\n     ++\tinit_basic_merge_options(&merge_opt, the_repository);\n       \tmemset(&result, 0, sizeof(result));\n       \tmerge_opt.show_rename_progress = 0;\n     + \tlast_commit = onto;\n     +\n     + ## builtin/stash.c ##\n     +@@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,\n     + \t\t}\n     + \t}\n     + \n     +-\tinit_merge_options(&o, the_repository);\n     ++\tinit_ui_merge_options(&o, the_repository);\n     + \n     + \to.branch1 = \"Updated upstream\";\n     + \to.branch2 = \"Stashed changes\";\n     +\n     + ## log-tree.c ##\n     +@@ log-tree.c: static int do_remerge_diff(struct rev_info *opt,\n     + \tstruct strbuf parent2_desc = STRBUF_INIT;\n     + \n     + \t/* Setup merge options */\n     +-\tinit_merge_options(&o, the_repository);\n     ++\tinit_ui_merge_options(&o, the_repository);\n     + \to.show_rename_progress = 0;\n     + \to.record_conflict_msgs_as_headers = 1;\n     + \to.msg_header_prefix = \"remerge\";\n      \n       ## merge-recursive.c ##\n     +@@ merge-recursive.c: int merge_recursive_generic(struct merge_options *opt,\n     + \treturn clean ? 0 : 1;\n     + }\n     + \n     +-static void merge_recursive_config(struct merge_options *opt)\n     ++static void merge_recursive_config(struct merge_options *opt, int ui)\n     + {\n     + \tchar *value = NULL;\n     + \tint renormalize = 0;\n      @@ merge-recursive.c: static void merge_recursive_config(struct merge_options *opt)\n       \t\t} /* avoid erroring on values from future versions of git */\n       \t\tfree(value);\n       \t}\n     -+\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n     -+\t\tlong diff_algorithm = parse_algorithm_value(value);\n     -+\t\tif (diff_algorithm < 0)\n     -+\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n     -+\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n     -+\t\tfree(value);\n     ++\tif (ui) {\n     ++\t\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n     ++\t\t\tlong diff_algorithm = parse_algorithm_value(value);\n     ++\t\t\tif (diff_algorithm < 0)\n     ++\t\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n     ++\t\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n     ++\t\t\tfree(value);\n     ++\t\t}\n      +\t}\n       \tgit_config(git_xmerge_config, NULL);\n       }\n       \n     +-void init_merge_options(struct merge_options *opt,\n     +-\t\t\tstruct repository *repo)\n     ++static void init_merge_options(struct merge_options *opt,\n     ++\t\t\tstruct repository *repo, int ui)\n     + {\n     + \tconst char *merge_verbosity;\n     + \tmemset(opt, 0, sizeof(struct merge_options));\n     +@@ merge-recursive.c: void init_merge_options(struct merge_options *opt,\n     + \n     + \topt->conflict_style = -1;\n     + \n     +-\tmerge_recursive_config(opt);\n     ++\tmerge_recursive_config(opt, ui);\n     + \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n     + \tif (merge_verbosity)\n     + \t\topt->verbosity = strtol(merge_verbosity, NULL, 10);\n     +@@ merge-recursive.c: void init_merge_options(struct merge_options *opt,\n     + \t\topt->buffer_output = 0;\n     + }\n     + \n     ++void init_ui_merge_options(struct merge_options *opt,\n     ++\t\t\tstruct repository *repo)\n     ++{\n     ++\tinit_merge_options(opt, repo, 1);\n     ++}\n     ++\n     ++void init_basic_merge_options(struct merge_options *opt,\n     ++\t\t\tstruct repository *repo)\n     ++{\n     ++\tinit_merge_options(opt, repo, 0);\n     ++}\n     ++\n     + /*\n     +  * For now, members of merge_options do not need deep copying, but\n     +  * it may change in the future, in which case we would need to update\n     +\n     + ## merge-recursive.h ##\n     +@@ merge-recursive.h: struct merge_options {\n     + \tstruct merge_options_internal *priv;\n     + };\n     + \n     +-void init_merge_options(struct merge_options *opt, struct repository *repo);\n     ++/* for use by porcelain commands */\n     ++void init_ui_merge_options(struct merge_options *opt, struct repository *repo);\n     ++/* for use by plumbing commands */\n     ++void init_basic_merge_options(struct merge_options *opt, struct repository *repo);\n     + \n     + void copy_merge_options(struct merge_options *dst, struct merge_options *src);\n     + void clear_merge_options(struct merge_options *opt);\n     +\n     + ## sequencer.c ##\n     +@@ sequencer.c: static int do_recursive_merge(struct repository *r,\n     + \n     + \trepo_read_index(r);\n     + \n     +-\tinit_merge_options(&o, r);\n     ++\tinit_ui_merge_options(&o, r);\n     + \to.ancestor = base ? base_label : \"(empty tree)\";\n     + \to.branch1 = \"HEAD\";\n     + \to.branch2 = next ? next_label : \"(empty tree)\";\n     +@@ sequencer.c: static int do_merge(struct repository *r,\n     + \tbases = reverse_commit_list(bases);\n     + \n     + \trepo_read_index(r);\n     +-\tinit_merge_options(&o, r);\n     ++\tinit_ui_merge_options(&o, r);\n     + \to.branch1 = \"HEAD\";\n     + \to.branch2 = ref_name.buf;\n     + \to.buffer_output = 2;\n      \n       ## t/t3515-cherry-pick-diff.sh (new) ##\n      @@\n\n\n builtin/am.c                |  2 +-\n builtin/checkout.c          |  2 +-\n builtin/merge-recursive.c   |  2 +-\n builtin/merge-tree.c        |  2 +-\n builtin/merge.c             |  2 +-\n builtin/replay.c            |  2 +-\n builtin/stash.c             |  2 +-\n log-tree.c                  |  2 +-\n merge-recursive.c           | 29 +++++++++++++++++++++----\n merge-recursive.h           |  5 ++++-\n sequencer.c                 |  4 ++--\n t/t3515-cherry-pick-diff.sh | 41 +++++++++++++++++++++++++++++++++++\n t/t3515/base.c              | 17 +++++++++++++++\n t/t3515/ours.c              | 17 +++++++++++++++\n t/t3515/theirs.c            | 17 +++++++++++++++\n t/t7615-merge-diff.sh       | 43 +++++++++++++++++++++++++++++++++++++\n 16 files changed, 174 insertions(+), 15 deletions(-)\n create mode 100755 t/t3515-cherry-pick-diff.sh\n create mode 100644 t/t3515/base.c\n create mode 100644 t/t3515/ours.c\n create mode 100644 t/t3515/theirs.c\n create mode 100755 t/t7615-merge-diff.sh\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8f9619ea3a3..b821561b15a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1630,7 +1630,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \t * changes.\n \t */\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \n \to.branch1 = \"HEAD\";\n \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3cf44b4683a..5769efaca00 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -884,7 +884,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \n \t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n \t\t\t\t\t   0);\n-\t\t\tinit_merge_options(&o, the_repository);\n+\t\t\tinit_ui_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\n \ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex c2ce044a201..9e9d0b57158 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -31,7 +31,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)\n \tchar *better1, *better2;\n \tstruct commit *result;\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_basic_merge_options(&o, the_repository);\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 1082d919fd1..aab0843ff5a 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -570,7 +570,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \t};\n \n \t/* Init merge options */\n-\tinit_merge_options(&o.merge_options, the_repository);\n+\tinit_ui_merge_options(&o.merge_options, the_repository);\n \n \t/* Parse arguments */\n \toriginal_argc = argc - 1; /* ignoring argv[0] */\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 66a4fa72e1c..686326bc1d3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -720,7 +720,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\treturn 2;\n \t\t}\n \n-\t\tinit_merge_options(&o, the_repository);\n+\t\tinit_ui_merge_options(&o, the_repository);\n \t\tif (!strcmp(strategy, \"subtree\"))\n \t\t\to.subtree_shift = \"\";\n \ndiff --git a/builtin/replay.c b/builtin/replay.c\nindex 6bf0691f15d..d90ddd0837d 100644\n--- a/builtin/replay.c\n+++ b/builtin/replay.c\n@@ -373,7 +373,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \t}\n \n-\tinit_merge_options(&merge_opt, the_repository);\n+\tinit_basic_merge_options(&merge_opt, the_repository);\n \tmemset(&result, 0, sizeof(result));\n \tmerge_opt.show_rename_progress = 0;\n \tlast_commit = onto;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7859bc0866a..86803755f03 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -574,7 +574,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t}\n \t}\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \n \to.branch1 = \"Updated upstream\";\n \to.branch2 = \"Stashed changes\";\ndiff --git a/log-tree.c b/log-tree.c\nindex 101079e8200..5d8fb6ff8df 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -1025,7 +1025,7 @@ static int do_remerge_diff(struct rev_info *opt,\n \tstruct strbuf parent2_desc = STRBUF_INIT;\n \n \t/* Setup merge options */\n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \to.show_rename_progress = 0;\n \to.record_conflict_msgs_as_headers = 1;\n \to.msg_header_prefix = \"remerge\";\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 46ee364af73..cd9bd3c03ef 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3901,7 +3901,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \treturn clean ? 0 : 1;\n }\n \n-static void merge_recursive_config(struct merge_options *opt)\n+static void merge_recursive_config(struct merge_options *opt, int ui)\n {\n \tchar *value = NULL;\n \tint renormalize = 0;\n@@ -3930,11 +3930,20 @@ static void merge_recursive_config(struct merge_options *opt)\n \t\t} /* avoid erroring on values from future versions of git */\n \t\tfree(value);\n \t}\n+\tif (ui) {\n+\t\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n+\t\t\tlong diff_algorithm = parse_algorithm_value(value);\n+\t\t\tif (diff_algorithm < 0)\n+\t\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n+\t\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n+\t\t\tfree(value);\n+\t\t}\n+\t}\n \tgit_config(git_xmerge_config, NULL);\n }\n \n-void init_merge_options(struct merge_options *opt,\n-\t\t\tstruct repository *repo)\n+static void init_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo, int ui)\n {\n \tconst char *merge_verbosity;\n \tmemset(opt, 0, sizeof(struct merge_options));\n@@ -3953,7 +3962,7 @@ void init_merge_options(struct merge_options *opt,\n \n \topt->conflict_style = -1;\n \n-\tmerge_recursive_config(opt);\n+\tmerge_recursive_config(opt, ui);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\n \t\topt->verbosity = strtol(merge_verbosity, NULL, 10);\n@@ -3961,6 +3970,18 @@ void init_merge_options(struct merge_options *opt,\n \t\topt->buffer_output = 0;\n }\n \n+void init_ui_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo)\n+{\n+\tinit_merge_options(opt, repo, 1);\n+}\n+\n+void init_basic_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo)\n+{\n+\tinit_merge_options(opt, repo, 0);\n+}\n+\n /*\n  * For now, members of merge_options do not need deep copying, but\n  * it may change in the future, in which case we would need to update\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex e67d38c3030..85a5c332bbb 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -54,7 +54,10 @@ struct merge_options {\n \tstruct merge_options_internal *priv;\n };\n \n-void init_merge_options(struct merge_options *opt, struct repository *repo);\n+/* for use by porcelain commands */\n+void init_ui_merge_options(struct merge_options *opt, struct repository *repo);\n+/* for use by plumbing commands */\n+void init_basic_merge_options(struct merge_options *opt, struct repository *repo);\n \n void copy_merge_options(struct merge_options *dst, struct merge_options *src);\n void clear_merge_options(struct merge_options *opt);\ndiff --git a/sequencer.c b/sequencer.c\nindex b4f055e5a85..3608374166a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -762,7 +762,7 @@ static int do_recursive_merge(struct repository *r,\n \n \trepo_read_index(r);\n \n-\tinit_merge_options(&o, r);\n+\tinit_ui_merge_options(&o, r);\n \to.ancestor = base ? base_label : \"(empty tree)\";\n \to.branch1 = \"HEAD\";\n \to.branch2 = next ? next_label : \"(empty tree)\";\n@@ -4308,7 +4308,7 @@ static int do_merge(struct repository *r,\n \tbases = reverse_commit_list(bases);\n \n \trepo_read_index(r);\n-\tinit_merge_options(&o, r);\n+\tinit_ui_merge_options(&o, r);\n \to.branch1 = \"HEAD\";\n \to.branch2 = ref_name.buf;\n \to.buffer_output = 2;\ndiff --git a/t/t3515-cherry-pick-diff.sh b/t/t3515-cherry-pick-diff.sh\nnew file mode 100755\nindex 00000000000..caeaa01c590\n--- /dev/null\n+++ b/t/t3515-cherry-pick-diff.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='git cherry-pick\n+\n+Testing the influence of the diff algorithm on the merge output.'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git cherry-pick -s recursive c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit cherry-pick --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit cherry-pick --strategy recursive c2\n+'\n+test_done\ndiff --git a/t/t3515/base.c b/t/t3515/base.c\nnew file mode 100644\nindex 00000000000..c64abc59366\n--- /dev/null\n+++ b/t/t3515/base.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\ndiff --git a/t/t3515/ours.c b/t/t3515/ours.c\nnew file mode 100644\nindex 00000000000..44d82513970\n--- /dev/null\n+++ b/t/t3515/ours.c\n@@ -0,0 +1,17 @@\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\n+\n+int h(int x, int y, int z)\n+{\n+        if (z == 0)\n+        {\n+                return x;\n+        }\n+        return y;\n+}\ndiff --git a/t/t3515/theirs.c b/t/t3515/theirs.c\nnew file mode 100644\nindex 00000000000..85f02146fee\n--- /dev/null\n+++ b/t/t3515/theirs.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u > 34)\n+        {\n+                u--;\n+        }\n+        return u;\n+}\ndiff --git a/t/t7615-merge-diff.sh b/t/t7615-merge-diff.sh\nnew file mode 100755\nindex 00000000000..be335c7c3d1\n--- /dev/null\n+++ b/t/t7615-merge-diff.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='git merge\n+\n+Testing the influence of the diff algorithm on the merge output.'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+GIT_TEST_MERGE_ALGORITHM=recursive\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git merge -s recursive c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit merge --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit merge --strategy recursive c2\n+'\n+test_done\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498473","messageId":"xmqqmsmpw2mp.fsf@gitster.g","threadId":"61751","inReplyTo":"pull.1743.v2.git.git.1720551701648.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-recursive: honor diff.algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-10T17:58:06Z","receivedAt":"2024-07-10T17:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Antonin Delpeuch via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Antonin Delpeuch <antonin@delpeuch.eu>\n>\n> The documentation claims that \"recursive defaults to the diff.algorithm\n> config setting\", but this is currently not the case. This fixes it,\n> ensuring that diff.algorithm is used when -Xdiff-algorithm is not\n> supplied. This affects the following porcelain commands: \"merge\",\n> \"rebase\", \"cherry-pick\", \"pull\", \"stash\", \"log\", \"am\" and \"checkout\".\n> It also affects the \"merge-tree\" ancillary interrogator.\n\nUnfortunate.\n\nSince be733e12 (Merge branch 'en/merge-tree', 2022-07-14),\nmerge-tree is no longer an interrogator but works as an manipulator.\nAs it is meant to be used as a building block that gives a reliable\nand repeatable output, I am tempted to say it should be categorized\nas a plumbing, but second opinions do count.  Elijah Cc'ed as it was\nhis \"fault\" to add \"--write-tree\" mode to the command and forgetting\nto update command-list.txt ;-)\n\nBut I agree with the direction of this patch and the structure of\nthe solution (i.e. have two variants of init_*_options()).\n\n> This change refactors the initialization of merge options to introduce\n> two functions, \"init_merge_ui_options\" and \"init_merge_basic_options\"\n> instead of just one \"init_merge_options\". This design follows the\n> approach used in diff.c, providing initialization methods for\n> porcelain and plumbing commands respectively. Thanks to that, the\n> \"replay\" and \"merge-recursive\" plumbing commands remain unaffected by\n> diff.algorithm.\n\nIn other words, these two are the only ones that use the _basic\nvariant.\n\nI am unsure (read: do not take this as my recommendation to change\nyour patch) which one merge-tree should use, but other than that,\nnicely done.\n\n> diff --git a/log-tree.c b/log-tree.c\n> index 101079e8200..5d8fb6ff8df 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -1025,7 +1025,7 @@ static int do_remerge_diff(struct rev_info *opt,\n>  \tstruct strbuf parent2_desc = STRBUF_INIT;\n>  \n>  \t/* Setup merge options */\n> -\tinit_merge_options(&o, the_repository);\n> +\tinit_ui_merge_options(&o, the_repository);\n>  \to.show_rename_progress = 0;\n>  \to.record_conflict_msgs_as_headers = 1;\n>  \to.msg_header_prefix = \"remerge\";\n\nIsn't log-tree shared with things like \"git diff-tree\" porcelain?\n\n> -static void merge_recursive_config(struct merge_options *opt)\n> +static void merge_recursive_config(struct merge_options *opt, int ui)\n>  {\n>  \tchar *value = NULL;\n>  \tint renormalize = 0;\n> @@ -3930,11 +3930,20 @@ static void merge_recursive_config(struct merge_options *opt)\n>  \t\t} /* avoid erroring on values from future versions of git */\n>  \t\tfree(value);\n>  \t}\n> +\tif (ui) {\n> +\t\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n> +\t\t\tlong diff_algorithm = parse_algorithm_value(value);\n> +\t\t\tif (diff_algorithm < 0)\n> +\t\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n> +\t\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n> +\t\t\tfree(value);\n> +\t\t}\n> +\t}\n>  \tgit_config(git_xmerge_config, NULL);\n>  }\n\nThis looks sensible.  Even though we have a single merge_recursive()\nthat is internally callable, depending on the callers, they may or\nmay not want to be affected by configuration.\n\nAs to the tests, it felt a bit unnatural and error prone to make\nt7615 depend on material that appears to be made only for t3515 (by\nnaming the directory as such).\n\nWe have not done \"a test-material directory that is shared among\nmultiple tests\" in t/, but we have plenty of \"test helpers that are\nshared across multiple tests\" named lib-foo.sh.  I wonder if\ndoing something like\n\n\t... in t/lib-histogram-merge-history.sh ...\n\t# prepare history for merges that depend on diff.algorithm\n\tsetup_history_for_histogram () {\n\t\tcat >file.c <<\\EOF &&\n\t\t... contents of base.c ...\n\t\tEOF\n\t\tgit add file.c &&\n\t\tgit commit -m c0 &&\n\t\tgit tag c0 &&\n\n\t\tcat >file.c <<\\EOF &&\n\t\t... contents of ours.c ...\n\t\tEOF\n\t\t...\n                git tag c2\n\t}\t\t\n\nand make the setup step in t3515 (and t7615) use that shared set-up\nfunction like so:\n\n\t. ./test-lib.sh\n\t. \"$TEST_DIRECTORY/test-lib-histogram-merge/history.sh\"\n\n\ttest_expect_success setup '\n\t\tsetup_history_for_histogram\n\t'\n\nmay be cleaner?  I am mostly afraid of mistakes like \"now we are\ndone with the area 3515 covered let's remove all the traces of it,\nlike t3515-cherry-pick-diff.sh and t3515/ directory\", breaking an\nseemingly unrelated t7615.\n\nEven better.  Can't we save the scarce resource that is test number\nand make these not about \"I test cherry-pick\" and \"I test merge\"?\nYou are testing how mergy operations are affected by the choice of\ndiff.algorithm, so perhaps create a single test file and name it\nafter that single shared aspect of the tests you are adding?\nPerhaps t/t7615-diff-algo-with-mergy-operations.sh that has all\nthree of these:\n\n * the setup_history_for_histogram() helper function as described\n   above;\n\n * the test for cherry-pick in this patch;\n\n * the test for merge in this patch.\n\nThanks.\n"},{"id":"498638","messageId":"pull.1743.v3.git.git.1720889507066.gitgitgadget@gmail.com","threadId":"61751","inReplyTo":"pull.1743.v2.git.git.1720551701648.gitgitgadget@gmail.com","subject":"[PATCH v3] merge-recursive: honor diff.algorithm","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T16:51:46Z","receivedAt":"2024-07-13T16:51:50Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nThe documentation claims that \"recursive defaults to the diff.algorithm\nconfig setting\", but this is currently not the case. This fixes it,\nensuring that diff.algorithm is used when -Xdiff-algorithm is not\nsupplied. This affects the following porcelain commands: \"merge\",\n\"rebase\", \"cherry-pick\", \"pull\", \"stash\", \"log\", \"am\" and \"checkout\".\nIt also affects the \"merge-tree\" ancillary interrogator.\n\nThis change refactors the initialization of merge options to introduce\ntwo functions, \"init_merge_ui_options\" and \"init_merge_basic_options\"\ninstead of just one \"init_merge_options\". This design follows the\napproach used in diff.c, providing initialization methods for\nporcelain and plumbing commands respectively. Thanks to that, the\n\"replay\" and \"merge-recursive\" plumbing commands remain unaffected by\ndiff.algorithm.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-recursive: honor diff.algorithm\n    \n    Changes since v2:\n    \n     * merge test cases of \"merge\" and \"cherry-pick\" together to keep things\n       simple\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1743%2Fwetneb%2Frecursive_respects_diff.algorithm-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1743/wetneb/recursive_respects_diff.algorithm-v3\nPull-Request: https://github.com/git/git/pull/1743\n\nRange-diff vs v2:\n\n 1:  9c1907fad43 ! 1:  abba5c3c0cf merge-recursive: honor diff.algorithm\n     @@ sequencer.c: static int do_merge(struct repository *r,\n       \to.branch2 = ref_name.buf;\n       \to.buffer_output = 2;\n      \n     - ## t/t3515-cherry-pick-diff.sh (new) ##\n     + ## t/t7615-diff-algo-with-mergy-operations.sh (new) ##\n      @@\n      +#!/bin/sh\n      +\n     -+test_description='git cherry-pick\n     ++test_description='git merge and other operations that rely on merge\n      +\n      +Testing the influence of the diff algorithm on the merge output.'\n      +\n     @@ t/t3515-cherry-pick-diff.sh (new)\n      +. ./test-lib.sh\n      +\n      +test_expect_success 'setup' '\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n     ++\tcp \"$TEST_DIRECTORY\"/t7615/base.c file.c &&\n      +\tgit add file.c &&\n      +\tgit commit -m c0 &&\n      +\tgit tag c0 &&\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n     ++\tcp \"$TEST_DIRECTORY\"/t7615/ours.c file.c &&\n      +\tgit add file.c &&\n      +\tgit commit -m c1 &&\n      +\tgit tag c1 &&\n      +\tgit reset --hard c0 &&\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n     ++\tcp \"$TEST_DIRECTORY\"/t7615/theirs.c file.c &&\n      +\tgit add file.c &&\n      +\tgit commit -m c2 &&\n      +\tgit tag c2\n      +'\n      +\n     ++GIT_TEST_MERGE_ALGORITHM=recursive\n     ++\n     ++test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n     ++\tgit reset --hard c1 &&\n     ++\ttest_must_fail git merge -s recursive c2\n     ++'\n     ++\n     ++test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n     ++\tgit reset --hard c1 &&\n     ++\tgit merge --strategy recursive -Xdiff-algorithm=histogram c2\n     ++'\n     ++\n     ++test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n     ++\tgit reset --hard c1 &&\n     ++\tgit config diff.algorithm histogram &&\n     ++\tgit merge --strategy recursive c2\n     ++'\n     ++\n      +test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n      +\tgit reset --hard c1 &&\n      +\ttest_must_fail git cherry-pick -s recursive c2\n     @@ t/t3515-cherry-pick-diff.sh (new)\n      +\tgit config diff.algorithm histogram &&\n      +\tgit cherry-pick --strategy recursive c2\n      +'\n     ++\n      +test_done\n      \n     - ## t/t3515/base.c (new) ##\n     + ## t/t7615/base.c (new) ##\n      @@\n      +int f(int x, int y)\n      +{\n     @@ t/t3515/base.c (new)\n      +        return u;\n      +}\n      \n     - ## t/t3515/ours.c (new) ##\n     + ## t/t7615/ours.c (new) ##\n      @@\n      +int g(size_t u)\n      +{\n     @@ t/t3515/ours.c (new)\n      +        return y;\n      +}\n      \n     - ## t/t3515/theirs.c (new) ##\n     + ## t/t7615/theirs.c (new) ##\n      @@\n      +int f(int x, int y)\n      +{\n     @@ t/t3515/theirs.c (new)\n      +        }\n      +        return u;\n      +}\n     -\n     - ## t/t7615-merge-diff.sh (new) ##\n     -@@\n     -+#!/bin/sh\n     -+\n     -+test_description='git merge\n     -+\n     -+Testing the influence of the diff algorithm on the merge output.'\n     -+\n     -+TEST_PASSES_SANITIZE_LEAK=true\n     -+. ./test-lib.sh\n     -+\n     -+test_expect_success 'setup' '\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/base.c file.c &&\n     -+\tgit add file.c &&\n     -+\tgit commit -m c0 &&\n     -+\tgit tag c0 &&\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/ours.c file.c &&\n     -+\tgit add file.c &&\n     -+\tgit commit -m c1 &&\n     -+\tgit tag c1 &&\n     -+\tgit reset --hard c0 &&\n     -+\tcp \"$TEST_DIRECTORY\"/t3515/theirs.c file.c &&\n     -+\tgit add file.c &&\n     -+\tgit commit -m c2 &&\n     -+\tgit tag c2\n     -+'\n     -+\n     -+GIT_TEST_MERGE_ALGORITHM=recursive\n     -+\n     -+test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n     -+\tgit reset --hard c1 &&\n     -+\ttest_must_fail git merge -s recursive c2\n     -+'\n     -+\n     -+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n     -+\tgit reset --hard c1 &&\n     -+\tgit merge --strategy recursive -Xdiff-algorithm=histogram c2\n     -+'\n     -+\n     -+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n     -+\tgit reset --hard c1 &&\n     -+\tgit config diff.algorithm histogram &&\n     -+\tgit merge --strategy recursive c2\n     -+'\n     -+test_done\n\n\n builtin/am.c                               |  2 +-\n builtin/checkout.c                         |  2 +-\n builtin/merge-recursive.c                  |  2 +-\n builtin/merge-tree.c                       |  2 +-\n builtin/merge.c                            |  2 +-\n builtin/replay.c                           |  2 +-\n builtin/stash.c                            |  2 +-\n log-tree.c                                 |  2 +-\n merge-recursive.c                          | 29 +++++++++--\n merge-recursive.h                          |  5 +-\n sequencer.c                                |  4 +-\n t/t7615-diff-algo-with-mergy-operations.sh | 60 ++++++++++++++++++++++\n t/t7615/base.c                             | 17 ++++++\n t/t7615/ours.c                             | 17 ++++++\n t/t7615/theirs.c                           | 17 ++++++\n 15 files changed, 150 insertions(+), 15 deletions(-)\n create mode 100755 t/t7615-diff-algo-with-mergy-operations.sh\n create mode 100644 t/t7615/base.c\n create mode 100644 t/t7615/ours.c\n create mode 100644 t/t7615/theirs.c\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8f9619ea3a3..b821561b15a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1630,7 +1630,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \t * changes.\n \t */\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \n \to.branch1 = \"HEAD\";\n \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3cf44b4683a..5769efaca00 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -884,7 +884,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \n \t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n \t\t\t\t\t   0);\n-\t\t\tinit_merge_options(&o, the_repository);\n+\t\t\tinit_ui_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\n \ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex c2ce044a201..9e9d0b57158 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -31,7 +31,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix UNUSED)\n \tchar *better1, *better2;\n \tstruct commit *result;\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_basic_merge_options(&o, the_repository);\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 1082d919fd1..aab0843ff5a 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -570,7 +570,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \t};\n \n \t/* Init merge options */\n-\tinit_merge_options(&o.merge_options, the_repository);\n+\tinit_ui_merge_options(&o.merge_options, the_repository);\n \n \t/* Parse arguments */\n \toriginal_argc = argc - 1; /* ignoring argv[0] */\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 66a4fa72e1c..686326bc1d3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -720,7 +720,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\treturn 2;\n \t\t}\n \n-\t\tinit_merge_options(&o, the_repository);\n+\t\tinit_ui_merge_options(&o, the_repository);\n \t\tif (!strcmp(strategy, \"subtree\"))\n \t\t\to.subtree_shift = \"\";\n \ndiff --git a/builtin/replay.c b/builtin/replay.c\nindex 6bf0691f15d..d90ddd0837d 100644\n--- a/builtin/replay.c\n+++ b/builtin/replay.c\n@@ -373,7 +373,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \t}\n \n-\tinit_merge_options(&merge_opt, the_repository);\n+\tinit_basic_merge_options(&merge_opt, the_repository);\n \tmemset(&result, 0, sizeof(result));\n \tmerge_opt.show_rename_progress = 0;\n \tlast_commit = onto;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7859bc0866a..86803755f03 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -574,7 +574,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t}\n \t}\n \n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \n \to.branch1 = \"Updated upstream\";\n \to.branch2 = \"Stashed changes\";\ndiff --git a/log-tree.c b/log-tree.c\nindex 101079e8200..5d8fb6ff8df 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -1025,7 +1025,7 @@ static int do_remerge_diff(struct rev_info *opt,\n \tstruct strbuf parent2_desc = STRBUF_INIT;\n \n \t/* Setup merge options */\n-\tinit_merge_options(&o, the_repository);\n+\tinit_ui_merge_options(&o, the_repository);\n \to.show_rename_progress = 0;\n \to.record_conflict_msgs_as_headers = 1;\n \to.msg_header_prefix = \"remerge\";\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 46ee364af73..cd9bd3c03ef 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3901,7 +3901,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \treturn clean ? 0 : 1;\n }\n \n-static void merge_recursive_config(struct merge_options *opt)\n+static void merge_recursive_config(struct merge_options *opt, int ui)\n {\n \tchar *value = NULL;\n \tint renormalize = 0;\n@@ -3930,11 +3930,20 @@ static void merge_recursive_config(struct merge_options *opt)\n \t\t} /* avoid erroring on values from future versions of git */\n \t\tfree(value);\n \t}\n+\tif (ui) {\n+\t\tif (!git_config_get_string(\"diff.algorithm\", &value)) {\n+\t\t\tlong diff_algorithm = parse_algorithm_value(value);\n+\t\t\tif (diff_algorithm < 0)\n+\t\t\t\tdie(_(\"unknown value for config '%s': %s\"), \"diff.algorithm\", value);\n+\t\t\topt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;\n+\t\t\tfree(value);\n+\t\t}\n+\t}\n \tgit_config(git_xmerge_config, NULL);\n }\n \n-void init_merge_options(struct merge_options *opt,\n-\t\t\tstruct repository *repo)\n+static void init_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo, int ui)\n {\n \tconst char *merge_verbosity;\n \tmemset(opt, 0, sizeof(struct merge_options));\n@@ -3953,7 +3962,7 @@ void init_merge_options(struct merge_options *opt,\n \n \topt->conflict_style = -1;\n \n-\tmerge_recursive_config(opt);\n+\tmerge_recursive_config(opt, ui);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\n \t\topt->verbosity = strtol(merge_verbosity, NULL, 10);\n@@ -3961,6 +3970,18 @@ void init_merge_options(struct merge_options *opt,\n \t\topt->buffer_output = 0;\n }\n \n+void init_ui_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo)\n+{\n+\tinit_merge_options(opt, repo, 1);\n+}\n+\n+void init_basic_merge_options(struct merge_options *opt,\n+\t\t\tstruct repository *repo)\n+{\n+\tinit_merge_options(opt, repo, 0);\n+}\n+\n /*\n  * For now, members of merge_options do not need deep copying, but\n  * it may change in the future, in which case we would need to update\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex e67d38c3030..85a5c332bbb 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -54,7 +54,10 @@ struct merge_options {\n \tstruct merge_options_internal *priv;\n };\n \n-void init_merge_options(struct merge_options *opt, struct repository *repo);\n+/* for use by porcelain commands */\n+void init_ui_merge_options(struct merge_options *opt, struct repository *repo);\n+/* for use by plumbing commands */\n+void init_basic_merge_options(struct merge_options *opt, struct repository *repo);\n \n void copy_merge_options(struct merge_options *dst, struct merge_options *src);\n void clear_merge_options(struct merge_options *opt);\ndiff --git a/sequencer.c b/sequencer.c\nindex b4f055e5a85..3608374166a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -762,7 +762,7 @@ static int do_recursive_merge(struct repository *r,\n \n \trepo_read_index(r);\n \n-\tinit_merge_options(&o, r);\n+\tinit_ui_merge_options(&o, r);\n \to.ancestor = base ? base_label : \"(empty tree)\";\n \to.branch1 = \"HEAD\";\n \to.branch2 = next ? next_label : \"(empty tree)\";\n@@ -4308,7 +4308,7 @@ static int do_merge(struct repository *r,\n \tbases = reverse_commit_list(bases);\n \n \trepo_read_index(r);\n-\tinit_merge_options(&o, r);\n+\tinit_ui_merge_options(&o, r);\n \to.branch1 = \"HEAD\";\n \to.branch2 = ref_name.buf;\n \to.buffer_output = 2;\ndiff --git a/t/t7615-diff-algo-with-mergy-operations.sh b/t/t7615-diff-algo-with-mergy-operations.sh\nnew file mode 100755\nindex 00000000000..9a83be518cb\n--- /dev/null\n+++ b/t/t7615-diff-algo-with-mergy-operations.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='git merge and other operations that rely on merge\n+\n+Testing the influence of the diff algorithm on the merge output.'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcp \"$TEST_DIRECTORY\"/t7615/base.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t7615/ours.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\tcp \"$TEST_DIRECTORY\"/t7615/theirs.c file.c &&\n+\tgit add file.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2\n+'\n+\n+GIT_TEST_MERGE_ALGORITHM=recursive\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git merge -s recursive c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit merge --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'merge c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit merge --strategy recursive c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy fails with the current default myers diff algorithm' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git cherry-pick -s recursive c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with -Xdiff-algorithm=histogram' '\n+\tgit reset --hard c1 &&\n+\tgit cherry-pick --strategy recursive -Xdiff-algorithm=histogram c2\n+'\n+\n+test_expect_success 'cherry-pick c2 to c1 with recursive merge strategy succeeds with diff.algorithm = histogram' '\n+\tgit reset --hard c1 &&\n+\tgit config diff.algorithm histogram &&\n+\tgit cherry-pick --strategy recursive c2\n+'\n+\n+test_done\ndiff --git a/t/t7615/base.c b/t/t7615/base.c\nnew file mode 100644\nindex 00000000000..c64abc59366\n--- /dev/null\n+++ b/t/t7615/base.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\ndiff --git a/t/t7615/ours.c b/t/t7615/ours.c\nnew file mode 100644\nindex 00000000000..44d82513970\n--- /dev/null\n+++ b/t/t7615/ours.c\n@@ -0,0 +1,17 @@\n+int g(size_t u)\n+{\n+        while (u < 30)\n+        {\n+                u++;\n+        }\n+        return u;\n+}\n+\n+int h(int x, int y, int z)\n+{\n+        if (z == 0)\n+        {\n+                return x;\n+        }\n+        return y;\n+}\ndiff --git a/t/t7615/theirs.c b/t/t7615/theirs.c\nnew file mode 100644\nindex 00000000000..85f02146fee\n--- /dev/null\n+++ b/t/t7615/theirs.c\n@@ -0,0 +1,17 @@\n+int f(int x, int y)\n+{\n+        if (x == 0)\n+        {\n+                return y;\n+        }\n+        return x;\n+}\n+\n+int g(size_t u)\n+{\n+        while (u > 34)\n+        {\n+                u--;\n+        }\n+        return u;\n+}\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498639","messageId":"9ff7c0e6-af29-48a2-bec7-be4554681671@delpeuch.eu","threadId":"61751","inReplyTo":"xmqqmsmpw2mp.fsf@gitster.g","subject":"Re: [PATCH v2] merge-recursive: honor diff.algorithm","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-07-13T16:50:42Z","receivedAt":"2024-07-13T17:04:01Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"On 10/07/2024 19:58, Junio C Hamano wrote:\n> Since be733e12 (Merge branch 'en/merge-tree', 2022-07-14),\n> merge-tree is no longer an interrogator but works as an manipulator.\n> As it is meant to be used as a building block that gives a reliable\n> and repeatable output, I am tempted to say it should be categorized\n> as a plumbing, but second opinions do count.  Elijah Cc'ed as it was\n> his \"fault\" to add \"--write-tree\" mode to the command and forgetting\n> to update command-list.txt ;-)\nI'm happy to change it to use the \"basic\" config if you prefer. I have\nto admit I don't have a good overview of what's porcelain or plumbing.\n> Isn't log-tree shared with things like \"git diff-tree\" porcelain?\nI'm happy to also change this, but looking at the call hierarchy we seem\nto have a complicated entanglement of porcelain and plumbing commands\nhere, so to separate them is probably more involved.\n> Can't we save the scarce resource that is test number\n> and make these not about \"I test cherry-pick\" and \"I test merge\"?\n\nYes I agree, let me submit a new patch which does that already. I wasn't\nsure if it was worth including tests for different commands anyway.\n\nAntonin\n\n"}]}