{"thread":{"id":"63075","subject":"[PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","startedAt":"2025-03-07T15:48:45Z","lastAt":"2025-03-17T21:25:53Z","messageCount":25,"participants":["Elijah Newren via GitGitGadget","Patrick Steinhardt","Taylor Blau","Elijah Newren","Johannes Schindelin via GitGitGadget","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"513777","messageId":"pull.1875.git.1741362522.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":null,"subject":"[PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-07T15:48:39Z","receivedAt":"2025-03-07T15:48:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I've got 19 patches covering the work needed to prep for and allow us to\ndelete merge-recursive.[ch], and remap 'recursive' to 'ort', including some\nclean-up along the way. I've tried to divide it up into five smaller patch\nseries.\n\nThese 3 patches are the first of those series, and each of these 3 patches\nprovide a small new feature that together will be used to allow us to\nconvert some callers over from recursive to ort. If the third patch,\nintroducing merge_ort_generic(), doesn't make sense to submit without one of\nits new callers, I can extend this series to 6 patches and include the\nconversion of git-am.sh.\n\nElijah Newren (3):\n merge-ort: add new merge_ort_generic() function\n merge-ort: allow rename detection to be disabled\n merge-ort: support having merge verbosity be set to 0\n\n Documentation/merge-strategies.adoc | 12 +++---\n merge-ort-wrappers.c | 64 +++++++++++++++++++++++++++++\n merge-ort-wrappers.h | 12 ++++++\n merge-ort.c | 24 ++++++++---\n merge-ort.h | 5 +++\n 5 files changed, 106 insertions(+), 11 deletions(-)\n\n\nbase-commit: a36e024e989f4d35f35987a60e3af8022cac3420\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1875%2Fnewren%2Fendit-new-features-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1875/newren/endit-new-features-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1875\n-- \ngitgitgadget\n"},{"id":"513778","messageId":"9f73e54224d55b40faeb5d68ebd7ff0c13d69c7b.1741362522.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.git.1741362522.gitgitgadget@gmail.com","subject":"[PATCH 1/3] merge-ort: add new merge_ort_generic() function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-07T15:48:40Z","receivedAt":"2025-03-07T15:48:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-recursive.[ch] have three entry points:\n * merge_trees()\n * merge_recursive()\n * merge_recursive_generic()\nmerge-ort*.[ch] only has equivalents for the first two. Add an\nequivalent for the final entry point, so we can switch callers to\nuse it and remove merge-recursive.[ch].\n\nSigned-off-by: Elijah Newren \n---\n merge-ort-wrappers.c | 64 ++++++++++++++++++++++++++++++++++++++++++++\n merge-ort-wrappers.h | 12 +++++++++\n merge-ort.c | 17 ++++++++----\n merge-ort.h | 5 ++++\n 4 files changed, 93 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\nindex d6f61359965..62834c30e9e 100644\n--- a/merge-ort-wrappers.c\n+++ b/merge-ort-wrappers.c\n@@ -1,9 +1,13 @@\n #include \"git-compat-util.h\"\n #include \"gettext.h\"\n #include \"hash.h\"\n+#include \"hex.h\"\n+#include \"lockfile.h\"\n #include \"merge-ort.h\"\n #include \"merge-ort-wrappers.h\"\n #include \"read-cache-ll.h\"\n+#include \"repository.h\"\n+#include \"tag.h\"\n #include \"tree.h\"\n \n #include \"commit.h\"\n@@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt,\n \n \treturn tmp.clean;\n }\n+\n+static struct commit *get_ref(struct repository *repo,\n+\t\t\t const struct object_id *oid,\n+\t\t\t const char *name)\n+{\n+\tstruct object *object;\n+\n+\tobject = deref_tag(repo, parse_object(repo, oid),\n+\t\t\t name, strlen(name));\n+\tif (!object)\n+\t\treturn NULL;\n+\tif (object->type == OBJ_TREE)\n+\t\treturn make_virtual_commit(repo, (struct tree*)object, name);\n+\tif (object->type != OBJ_COMMIT)\n+\t\treturn NULL;\n+\tif (repo_parse_commit(repo, (struct commit *)object))\n+\t\treturn NULL;\n+\treturn (struct commit *)object;\n+}\n+\n+int merge_ort_generic(struct merge_options *opt,\n+\t\t const struct object_id *head,\n+\t\t const struct object_id *merge,\n+\t\t int num_merge_bases,\n+\t\t const struct object_id *merge_bases,\n+\t\t struct commit **result)\n+{\n+\tint clean;\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct commit *head_commit = get_ref(opt->repo, head, opt->branch1);\n+\tstruct commit *next_commit = get_ref(opt->repo, merge, opt->branch2);\n+\tstruct commit_list *ca = NULL;\n+\n+\tif (merge_bases) {\n+\t\tint i;\n+\t\tfor (i = 0; i < num_merge_bases; ++i) {\n+\t\t\tstruct commit *base;\n+\t\t\tif (!(base = get_ref(opt->repo, &merge_bases[i],\n+\t\t\t\t\t oid_to_hex(&merge_bases[i]))))\n+\t\t\t\treturn error(_(\"Could not parse object '%s'\"),\n+\t\t\t\t\t oid_to_hex(&merge_bases[i]));\n+\t\t\tcommit_list_insert(base, &ca);\n+\t\t}\n+\t}\n+\n+\trepo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n+\tclean = merge_ort_recursive(opt, head_commit, next_commit, ca,\n+\t\t\t\t result);\n+\tfree_commit_list(ca);\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&lock);\n+\t\treturn clean;\n+\t}\n+\n+\tif (write_locked_index(opt->repo->index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\treturn error(_(\"Unable to write index.\"));\n+\n+\treturn clean ? 0 : 1;\n+}\ndiff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h\nindex 90af1f69c55..aeffa1c87b4 100644\n--- a/merge-ort-wrappers.h\n+++ b/merge-ort-wrappers.h\n@@ -22,4 +22,16 @@ int merge_ort_recursive(struct merge_options *opt,\n \t\t\tconst struct commit_list *ancestors,\n \t\t\tstruct commit **result);\n \n+/*\n+ * rename-detecting three-way merge. num_merge_bases must be at least 1.\n+ * Recursive ancestor consolidation will be performed if num_merge_bases > 1.\n+ * Wrapper mimicking the old merge_recursive_generic() function.\n+ */\n+int merge_ort_generic(struct merge_options *opt,\n+\t\t const struct object_id *head,\n+\t\t const struct object_id *merge,\n+\t\t int num_merge_bases,\n+\t\t const struct object_id *merge_bases,\n+\t\t struct commit **result);\n+\n #endif\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..b4ff24403a1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4878,9 +4878,9 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)\n \tc->maybe_tree = t;\n }\n \n-static struct commit *make_virtual_commit(struct repository *repo,\n-\t\t\t\t\t struct tree *tree,\n-\t\t\t\t\t const char *comment)\n+struct commit *make_virtual_commit(struct repository *repo,\n+\t\t\t\t struct tree *tree,\n+\t\t\t\t const char *comment)\n {\n \tstruct commit *commit = alloc_commit_node(repo);\n \n@@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\tancestor_name = \"empty tree\";\n \t} else if (merge_bases) {\n \t\tancestor_name = \"merged common ancestors\";\n+\t} else if (opt->ancestor) {\n+\t\tancestor_name = opt->ancestor;\n \t} else {\n \t\tstrbuf_add_unique_abbrev(&merge_base_abbrev,\n \t\t\t\t\t &merged_merge_bases->object.oid,\n@@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt,\n {\n \ttrace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n \n-\t/* We set the ancestor label based on the merge_bases */\n-\tassert(opt->ancestor == NULL);\n+\t/*\n+\t * We set the ancestor label based on the merge_bases...but we\n+\t * allow one exception through so that builtin/am can override\n+\t * with its constructed fake ancestor.\n+\t */\n+\tassert(opt->ancestor == NULL ||\n+\t (merge_bases && !merge_bases->next));\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n \tmerge_start(opt, result);\ndiff --git a/merge-ort.h b/merge-ort.h\nindex 82f2b3222d2..b63bc5424e7 100644\n--- a/merge-ort.h\n+++ b/merge-ort.h\n@@ -44,6 +44,11 @@ struct merge_result {\n \tunsigned _properly_initialized;\n };\n \n+/* Mostly internal function also used by merge-ort-wrappers.c */\n+struct commit *make_virtual_commit(struct repository *repo,\n+\t\t\t\t struct tree *tree,\n+\t\t\t\t const char *comment);\n+\n /*\n * rename-detecting three-way merge with recursive ancestor consolidation.\n * working tree and index are untouched.\n-- \ngitgitgadget\n\n"},{"id":"513779","messageId":"4292b22723f759c3e0f84ac1000992187a9c7f7c.1741362522.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.git.1741362522.gitgitgadget@gmail.com","subject":"[PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-07T15:48:41Z","receivedAt":"2025-03-07T15:48:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen merge-ort was written, I did not at first allow rename detection to\nbe disabled, because I suspected that most folks disabling rename\ndetection were doing so solely for performance reasons. Since I put a\nlot of working into providing dramatic speedups for rename detection\nperformance as used by the merge machinery, I wanted to know if there\nwere still real world repositories where rename detection was\nproblematic from a performance perspective. We have had years now to\ncollect such information, and while we never received one, waiting\nlonger with the option disabled seems unlikely to help surface such\nissues at this point. Also, there has been at least one request to\nallow rename detection to be disabled for behavioral rather than\nperformance reasons, so let's start heeding the config and command line\nsettings.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/merge-strategies.adoc | 12 ++++++------\n merge-ort.c | 5 +++++\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/merge-strategies.adoc b/Documentation/merge-strategies.adoc\nindex 93822ebc4e8..59f5ae36ccb 100644\n--- a/Documentation/merge-strategies.adoc\n+++ b/Documentation/merge-strategies.adoc\n@@ -82,6 +82,11 @@ find-renames[=];;\n rename-threshold=;;\n \tDeprecated synonym for `find-renames=`.\n \n+no-renames;;\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n subtree[=];;\n \tThis option is a more advanced form of 'subtree' strategy, where\n \tthe strategy makes a guess on how two trees must be shifted to\n@@ -107,7 +112,7 @@ For a path that is a submodule, the same caution as 'ort' applies to this\n strategy.\n +\n The 'recursive' strategy takes the same options as 'ort'. However,\n-there are three additional options that 'ort' ignores (not documented\n+there are two additional options that 'ort' ignores (not documented\n above) that are potentially useful with the 'recursive' strategy:\n \n patience;;\n@@ -121,11 +126,6 @@ diff-algorithm=[patience|minimal|histogram|myers];;\n \tspecifically uses `diff-algorithm=histogram`, while `recursive`\n \tdefaults to the `diff.algorithm` config setting.\n \n-no-renames;;\n-\tTurn off rename detection. This overrides the `merge.renames`\n-\tconfiguration variable.\n-\tSee also linkgit:git-diff[1] `--no-renames`.\n-\n resolve::\n \tThis can only resolve two heads (i.e. the current branch\n \tand another branch you pulled from) using a 3-way merge\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b4ff24403a1..a6960b6a1b4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)\n \n \tif (!possible_renames(renames))\n \t\tgoto cleanup;\n+\tif (opt->detect_renames == 0) {\n+\t\trenames->redo_after_renames = 0;\n+\t\trenames->cached_pairs_valid_side = 0;\n+\t\tgoto cleanup;\n+\t}\n \n \ttrace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n \tdetection_run |= detect_regular_renames(opt, MERGE_SIDE1);\n-- \ngitgitgadget\n\n"},{"id":"513780","messageId":"c2a2be336e0ed7966b6ab0ef004f150537167b55.1741362522.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.git.1741362522.gitgitgadget@gmail.com","subject":"[PATCH 3/3] merge-ort: support having merge verbosity be set to 0","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-07T15:48:42Z","receivedAt":"2025-03-07T15:48:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nVarious callers such as am & checkout set the merge verbosity to 0 to\navoid having conflict messages printed. While this could be achieved by\navoiding the wrappers from merge-ort-wrappers and instead passing 0 for\ndisplay_update_msgs to merge_switch_to_result(), for simplicity of\nconverting callers simply allow them to also achieve this with the\nmerge-ort-wrappers by setting verbosity to 0.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a6960b6a1b4..8021083c112 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -799,6 +799,8 @@ static void path_msg(struct merge_options *opt,\n \t\treturn; /* Do not record mere hints in headers */\n \tif (opt->priv->call_depth && opt->verbosity < 5)\n \t\treturn; /* Ignore messages from inner merges */\n+\tif (!opt->verbosity)\n+\t\treturn;\n \n \t/* Ensure path_conflicts (ptr to array of logical_conflict) allocated */\n \tpath_conflicts = strmap_get(&opt->priv->conflicts, primary_path);\n-- \ngitgitgadget\n"},{"id":"514029","messageId":"Z9FAgCSCZSJnzv9d@pks.im","threadId":"63075","inReplyTo":"9f73e54224d55b40faeb5d68ebd7ff0c13d69c7b.1741362522.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] merge-ort: add new merge_ort_generic() function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-12T08:06:24Z","receivedAt":"2025-03-12T08:06:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 07, 2025 at 03:48:40PM +0000, Elijah Newren via GitGitGadget wrote:\n> diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\n> index d6f61359965..62834c30e9e 100644\n> --- a/merge-ort-wrappers.c\n> +++ b/merge-ort-wrappers.c\n> @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt,\n> \n> \treturn tmp.clean;\n> }\n> +\n> +static struct commit *get_ref(struct repository *repo,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t const char *name)\n> +{\n> +\tstruct object *object;\n> +\n> +\tobject = deref_tag(repo, parse_object(repo, oid),\n> +\t\t\t name, strlen(name));\n> +\tif (!object)\n> +\t\treturn NULL;\n> +\tif (object->type == OBJ_TREE)\n> +\t\treturn make_virtual_commit(repo, (struct tree*)object, name);\n> +\tif (object->type != OBJ_COMMIT)\n> +\t\treturn NULL;\n> +\tif (repo_parse_commit(repo, (struct commit *)object))\n> +\t\treturn NULL;\n> +\treturn (struct commit *)object;\n> +}\n\nThis is an exact copy of the same function in \"merge-recursive.c\".\n\n> +int merge_ort_generic(struct merge_options *opt,\n> +\t\t const struct object_id *head,\n> +\t\t const struct object_id *merge,\n> +\t\t int num_merge_bases,\n> +\t\t const struct object_id *merge_bases,\n> +\t\t struct commit **result)\n> +{\n> +\tint clean;\n> +\tstruct lock_file lock = LOCK_INIT;\n> +\tstruct commit *head_commit = get_ref(opt->repo, head, opt->branch1);\n> +\tstruct commit *next_commit = get_ref(opt->repo, merge, opt->branch2);\n> +\tstruct commit_list *ca = NULL;\n> +\n> +\tif (merge_bases) {\n> +\t\tint i;\n> +\t\tfor (i = 0; i < num_merge_bases; ++i) {\n> +\t\t\tstruct commit *base;\n> +\t\t\tif (!(base = get_ref(opt->repo, &merge_bases[i],\n> +\t\t\t\t\t oid_to_hex(&merge_bases[i]))))\n> +\t\t\t\treturn error(_(\"Could not parse object '%s'\"),\n> +\t\t\t\t\t oid_to_hex(&merge_bases[i]));\n> +\t\t\tcommit_list_insert(base, &ca);\n> +\t\t}\n> +\t}\n> +\n> +\trepo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n> +\tclean = merge_ort_recursive(opt, head_commit, next_commit, ca,\n> +\t\t\t\t result);\n> +\tfree_commit_list(ca);\n> +\tif (clean < 0) {\n> +\t\trollback_lock_file(&lock);\n> +\t\treturn clean;\n> +\t}\n> +\n> +\tif (write_locked_index(opt->repo->index, &lock,\n> +\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> +\t\treturn error(_(\"Unable to write index.\"));\n> +\n> +\treturn clean ? 0 : 1;\n> +}\n\nThere are two differences here:\n\n - We use `error()` instead of the custom `err()` function that\n \"merge-recursive.c\" uses. I'm happy to see us using standard error\n reporting.\n\n - We don't have the check for `num_merge_bases == 1`. I have no idea\n why we don't have it, and it's likely that other readers may be\n puzzled in the same way. So this is something I'd expect to see\n explained in the commit message.\n\nOther than that this function looks identical.\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 46e78c3ffa6..b4ff24403a1 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt,\n> \t\tancestor_name = \"empty tree\";\n> \t} else if (merge_bases) {\n> \t\tancestor_name = \"merged common ancestors\";\n> +\t} else if (opt->ancestor) {\n> +\t\tancestor_name = opt->ancestor;\n> \t} else {\n> \t\tstrbuf_add_unique_abbrev(&merge_base_abbrev,\n> \t\t\t\t\t &merged_merge_bases->object.oid,\n> @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt,\n> {\n> \ttrace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n> \n> -\t/* We set the ancestor label based on the merge_bases */\n> -\tassert(opt->ancestor == NULL);\n> +\t/*\n> +\t * We set the ancestor label based on the merge_bases...but we\n> +\t * allow one exception through so that builtin/am can override\n> +\t * with its constructed fake ancestor.\n> +\t */\n> +\tassert(opt->ancestor == NULL ||\n> +\t (merge_bases && !merge_bases->next));\n> \n> \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n> \tmerge_start(opt, result);\n\nThese two hunks look related to my above observation that we don't have\nthe check for `num_merge_bases == 1`, as in \"merge-recursive.c\" we used\nto set `opt->ancestor = \"constructed merge base\" if so.\n\nPatrick\n"},{"id":"514030","messageId":"Z9FAh5p2h55_4mki@pks.im","threadId":"63075","inReplyTo":"pull.1875.git.1741362522.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-12T08:06:31Z","receivedAt":"2025-03-12T08:06:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 07, 2025 at 03:48:39PM +0000, Elijah Newren via GitGitGadget wrote:\n> I've got 19 patches covering the work needed to prep for and allow us to\n> delete merge-recursive.[ch], and remap 'recursive' to 'ort', including some\n> clean-up along the way. I've tried to divide it up into five smaller patch\n> series.\n> \n> These 3 patches are the first of those series, and each of these 3 patches\n> provide a small new feature that together will be used to allow us to\n> convert some callers over from recursive to ort. If the third patch,\n> introducing merge_ort_generic(), doesn't make sense to submit without one of\n> its new callers, I can extend this series to 6 patches and include the\n> conversion of git-am.sh.\n\nI think extending it to 6 patches would make sense as it's somewhat\nunfortunate that this version introduces the function, but has no\ncallers at all.\n\nPatrick\n"},{"id":"514031","messageId":"Z9FAix-VKGte8UKk@pks.im","threadId":"63075","inReplyTo":"4292b22723f759c3e0f84ac1000992187a9c7f7c.1741362522.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-12T08:06:35Z","receivedAt":"2025-03-12T08:06:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> When merge-ort was written, I did not at first allow rename detection to\n> be disabled, because I suspected that most folks disabling rename\n> detection were doing so solely for performance reasons. Since I put a\n> lot of working into providing dramatic speedups for rename detection\n> performance as used by the merge machinery, I wanted to know if there\n> were still real world repositories where rename detection was\n> problematic from a performance perspective. We have had years now to\n> collect such information, and while we never received one, waiting\n> longer with the option disabled seems unlikely to help surface such\n> issues at this point. Also, there has been at least one request to\n> allow rename detection to be disabled for behavioral rather than\n> performance reasons, so let's start heeding the config and command line\n> settings.\n\nIt might be nice to provide a link to that request for more context.\n\n> diff --git a/Documentation/merge-strategies.adoc b/Documentation/merge-strategies.adoc\n> index 93822ebc4e8..59f5ae36ccb 100644\n> --- a/Documentation/merge-strategies.adoc\n> +++ b/Documentation/merge-strategies.adoc\n> @@ -82,6 +82,11 @@ find-renames[=];;\n> rename-threshold=;;\n> \tDeprecated synonym for `find-renames=`.\n> \n> +no-renames;;\n> +\tTurn off rename detection. This overrides the `merge.renames`\n> +\tconfiguration variable.\n> +\tSee also linkgit:git-diff[1] `--no-renames`.\n> +\n> subtree[=];;\n> \tThis option is a more advanced form of 'subtree' strategy, where\n> \tthe strategy makes a guess on how two trees must be shifted to\n> @@ -107,7 +112,7 @@ For a path that is a submodule, the same caution as 'ort' applies to this\n> strategy.\n> +\n> The 'recursive' strategy takes the same options as 'ort'. However,\n> -there are three additional options that 'ort' ignores (not documented\n> +there are two additional options that 'ort' ignores (not documented\n> above) that are potentially useful with the 'recursive' strategy:\n> \n> patience;;\n> @@ -121,11 +126,6 @@ diff-algorithm=[patience|minimal|histogram|myers];;\n> \tspecifically uses `diff-algorithm=histogram`, while `recursive`\n> \tdefaults to the `diff.algorithm` config setting.\n> \n> -no-renames;;\n> -\tTurn off rename detection. This overrides the `merge.renames`\n> -\tconfiguration variable.\n> -\tSee also linkgit:git-diff[1] `--no-renames`.\n> -\n> resolve::\n> \tThis can only resolve two heads (i.e. the current branch\n> \tand another branch you pulled from) using a 3-way merge\n\nMakes sense.\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index b4ff24403a1..a6960b6a1b4 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)\n> \n> \tif (!possible_renames(renames))\n> \t\tgoto cleanup;\n> +\tif (opt->detect_renames == 0) {\n> +\t\trenames->redo_after_renames = 0;\n> +\t\trenames->cached_pairs_valid_side = 0;\n> +\t\tgoto cleanup;\n> +\t}\n> \n> \ttrace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n> \tdetection_run |= detect_regular_renames(opt, MERGE_SIDE1);\n\nDo we want to add a test that demonstrates that the option works as\nexpected?\n\nPatrick\n"},{"id":"514101","messageId":"Z9HnysAkF6P9uUg6@nand.local","threadId":"63075","inReplyTo":"Z9FAgCSCZSJnzv9d@pks.im","subject":"Re: [PATCH 1/3] merge-ort: add new merge_ort_generic() function","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T20:00:10Z","receivedAt":"2025-03-12T20:00:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 09:06:24AM +0100, Patrick Steinhardt wrote:\n> These two hunks look related to my above observation that we don't have\n> the check for `num_merge_bases == 1`, as in \"merge-recursive.c\" we used\n> to set `opt->ancestor = \"constructed merge base\" if so.\n\nYeah, I noticed the same thing and agree that it would be helpful to see\nthat spelled out in the commit message.\n\nThanks,\nTaylor\n"},{"id":"514102","messageId":"Z9Hobtp+9esKkY/O@nand.local","threadId":"63075","inReplyTo":"Z9FAix-VKGte8UKk@pks.im","subject":"Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T20:02:54Z","receivedAt":"2025-03-12T20:02:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 09:06:35AM +0100, Patrick Steinhardt wrote:\n> On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > When merge-ort was written, I did not at first allow rename detection to\n> > be disabled, because I suspected that most folks disabling rename\n> > detection were doing so solely for performance reasons. Since I put a\n> > lot of working into providing dramatic speedups for rename detection\n> > performance as used by the merge machinery, I wanted to know if there\n> > were still real world repositories where rename detection was\n> > problematic from a performance perspective. We have had years now to\n> > collect such information, and while we never received one, waiting\n> > longer with the option disabled seems unlikely to help surface such\n> > issues at this point. Also, there has been at least one request to\n> > allow rename detection to be disabled for behavioral rather than\n> > performance reasons, so let's start heeding the config and command line\n> > settings.\n>\n> It might be nice to provide a link to that request for more context.\n\nI don't know if a link exists; I suspect the request referred to here is\nan email that Johannes Schindelin wrote to Elijah privately).\n\nBut I am almost certain that the behavior requested here is to disable\nrename detection to match the behavior of GitHub's prior use of libgit2\nto perform merges, where we also had rename detection disabled (for\nreasons that are unclear to me, but Peff might know).\n\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index b4ff24403a1..a6960b6a1b4 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)\n> >\n> > \tif (!possible_renames(renames))\n> > \t\tgoto cleanup;\n> > +\tif (opt->detect_renames == 0) {\n\n if (!opt->detect_renames)\n\n?\n\n> > +\t\trenames->redo_after_renames = 0;\n> > +\t\trenames->cached_pairs_valid_side = 0;\n> > +\t\tgoto cleanup;\n> > +\t}\n> >\n> > \ttrace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n> > \tdetection_run |= detect_regular_renames(opt, MERGE_SIDE1);\n>\n> Do we want to add a test that demonstrates that the option works as\n> expected?\n\nYeah, having a test here would be nice.\n\nThanks,\nTaylor\n"},{"id":"514103","messageId":"Z9HopHJKPv7CXI3R@nand.local","threadId":"63075","inReplyTo":"c2a2be336e0ed7966b6ab0ef004f150537167b55.1741362522.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] merge-ort: support having merge verbosity be set to 0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T20:03:48Z","receivedAt":"2025-03-12T20:03:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 07, 2025 at 03:48:42PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> Various callers such as am & checkout set the merge verbosity to 0 to\n> avoid having conflict messages printed. While this could be achieved by\n> avoiding the wrappers from merge-ort-wrappers and instead passing 0 for\n> display_update_msgs to merge_switch_to_result(), for simplicity of\n> converting callers simply allow them to also achieve this with the\n> merge-ort-wrappers by setting verbosity to 0.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index a6960b6a1b4..8021083c112 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -799,6 +799,8 @@ static void path_msg(struct merge_options *opt,\n> \t\treturn; /* Do not record mere hints in headers */\n> \tif (opt->priv->call_depth && opt->verbosity < 5)\n> \t\treturn; /* Ignore messages from inner merges */\n> +\tif (!opt->verbosity)\n> +\t\treturn;\n\nLooks trivially correct ;-). Should we add a test to ensure that we\ndon't regress this behavior in the future?\n\nThanks,\nTaylor\n"},{"id":"514104","messageId":"Z9HpAgGq0FSOheLj@nand.local","threadId":"63075","inReplyTo":"Z9FAh5p2h55_4mki@pks.im","subject":"Re: [PATCH 0/3] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T20:05:22Z","receivedAt":"2025-03-12T20:05:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 09:06:31AM +0100, Patrick Steinhardt wrote:\n> On Fri, Mar 07, 2025 at 03:48:39PM +0000, Elijah Newren via GitGitGadget wrote:\n> > I've got 19 patches covering the work needed to prep for and allow us to\n> > delete merge-recursive.[ch], and remap 'recursive' to 'ort', including some\n> > clean-up along the way. I've tried to divide it up into five smaller patch\n> > series.\n> >\n> > These 3 patches are the first of those series, and each of these 3 patches\n> > provide a small new feature that together will be used to allow us to\n> > convert some callers over from recursive to ort. If the third patch,\n> > introducing merge_ort_generic(), doesn't make sense to submit without one of\n> > its new callers, I can extend this series to 6 patches and include the\n> > conversion of git-am.sh.\n>\n> I think extending it to 6 patches would make sense as it's somewhat\n> unfortunate that this version introduces the function, but has no\n> callers at all.\n\nEh. I have gone back and forth about that over the years. I think in\ncases where the either caller(s) or implementation is sufficiently\ncomplicated, it's OK to introduce a (non-static) function without any\ncallers.\n\nI don't feel strongly about it, so I think the multi-series structure is\nfine as it is (especially since we know that more are coming in the\nfuture). But I'm also not opposed to seeing this series extended out to\ninclude the three additional patches.\n\nThanks,\nTaylor\n"},{"id":"514119","messageId":"CABPp-BGUjLm+G1_LsEOp3P2b962rADxmgoWxn1MgREHWiTvOQA@mail.gmail.com","threadId":"63075","inReplyTo":"Z9FAgCSCZSJnzv9d@pks.im","subject":"Re: [PATCH 1/3] merge-ort: add new merge_ort_generic() function","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T21:39:31Z","receivedAt":"2025-03-12T21:39:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 1:06 AM Patrick Steinhardt wrote:\n>\n> On Fri, Mar 07, 2025 at 03:48:40PM +0000, Elijah Newren via GitGitGadget wrote:\n> > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\n> > index d6f61359965..62834c30e9e 100644\n> > --- a/merge-ort-wrappers.c\n> > +++ b/merge-ort-wrappers.c\n> > @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt,\n> >\n> > return tmp.clean;\n> > }\n> > +\n> > +static struct commit *get_ref(struct repository *repo,\n> > + const struct object_id *oid,\n> > + const char *name)\n> > +{\n> > + struct object *object;\n> > +\n> > + object = deref_tag(repo, parse_object(repo, oid),\n> > + name, strlen(name));\n> > + if (!object)\n> > + return NULL;\n> > + if (object->type == OBJ_TREE)\n> > + return make_virtual_commit(repo, (struct tree*)object, name);\n> > + if (object->type != OBJ_COMMIT)\n> > + return NULL;\n> > + if (repo_parse_commit(repo, (struct commit *)object))\n> > + return NULL;\n> > + return (struct commit *)object;\n> > +}\n>\n> This is an exact copy of the same function in \"merge-recursive.c\".\n>\n> > +int merge_ort_generic(struct merge_options *opt,\n> > + const struct object_id *head,\n> > + const struct object_id *merge,\n> > + int num_merge_bases,\n> > + const struct object_id *merge_bases,\n> > + struct commit **result)\n> > +{\n> > + int clean;\n> > + struct lock_file lock = LOCK_INIT;\n> > + struct commit *head_commit = get_ref(opt->repo, head, opt->branch1);\n> > + struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2);\n> > + struct commit_list *ca = NULL;\n> > +\n> > + if (merge_bases) {\n> > + int i;\n> > + for (i = 0; i < num_merge_bases; ++i) {\n> > + struct commit *base;\n> > + if (!(base = get_ref(opt->repo, &merge_bases[i],\n> > + oid_to_hex(&merge_bases[i]))))\n> > + return error(_(\"Could not parse object '%s'\"),\n> > + oid_to_hex(&merge_bases[i]));\n> > + commit_list_insert(base, &ca);\n> > + }\n> > + }\n> > +\n> > + repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n> > + clean = merge_ort_recursive(opt, head_commit, next_commit, ca,\n> > + result);\n> > + free_commit_list(ca);\n> > + if (clean < 0) {\n> > + rollback_lock_file(&lock);\n> > + return clean;\n> > + }\n> > +\n> > + if (write_locked_index(opt->repo->index, &lock,\n> > + COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> > + return error(_(\"Unable to write index.\"));\n> > +\n> > + return clean ? 0 : 1;\n> > +}\n>\n> There are two differences here:\n>\n> - We use `error()` instead of the custom `err()` function that\n> \"merge-recursive.c\" uses. I'm happy to see us using standard error\n> reporting.\n>\n> - We don't have the check for `num_merge_bases == 1`. I have no idea\n> why we don't have it, and it's likely that other readers may be\n> puzzled in the same way. So this is something I'd expect to see\n> explained in the commit message.\n\nYeah, it looks like when I was splitting commits, I had more of this\nexplanation in a commit which was explaining differences in testcases.\nI'll copy the relevant bits here. Thanks for reading carefully.\n\n>\n> Other than that this function looks identical.\n>\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 46e78c3ffa6..b4ff24403a1 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt,\n> > ancestor_name = \"empty tree\";\n> > } else if (merge_bases) {\n> > ancestor_name = \"merged common ancestors\";\n> > + } else if (opt->ancestor) {\n> > + ancestor_name = opt->ancestor;\n> > } else {\n> > strbuf_add_unique_abbrev(&merge_base_abbrev,\n> > &merged_merge_bases->object.oid,\n> > @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt,\n> > {\n> > trace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n> >\n> > - /* We set the ancestor label based on the merge_bases */\n> > - assert(opt->ancestor == NULL);\n> > + /*\n> > + * We set the ancestor label based on the merge_bases...but we\n> > + * allow one exception through so that builtin/am can override\n> > + * with its constructed fake ancestor.\n> > + */\n> > + assert(opt->ancestor == NULL ||\n> > + (merge_bases && !merge_bases->next));\n> >\n> > trace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n> > merge_start(opt, result);\n>\n> These two hunks look related to my above observation that we don't have\n> the check for `num_merge_bases == 1`, as in \"merge-recursive.c\" we used\n> to set `opt->ancestor = \"constructed merge base\" if so.\n>\n> Patrick\n"},{"id":"514120","messageId":"CABPp-BFj08mpXGiSoZ3xZ4KamwTJ4k5wPLTCJh6hUtKSxWE52Q@mail.gmail.com","threadId":"63075","inReplyTo":"Z9Hobtp+9esKkY/O@nand.local","subject":"Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T21:40:35Z","receivedAt":"2025-03-12T21:40:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 1:02 PM Taylor Blau wrote:\n>\n> On Wed, Mar 12, 2025 at 09:06:35AM +0100, Patrick Steinhardt wrote:\n> > On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:\n> > > From: Elijah Newren \n> > >\n> > > [...] Also, there has been at least one request to\n> > > allow rename detection to be disabled for behavioral rather than\n> > > performance reasons, so let's start heeding the config and command line\n> > > settings.\n> >\n> > It might be nice to provide a link to that request for more context.\n\nWill add.\n\n> I don't know if a link exists; I suspect the request referred to here is\n> an email that Johannes Schindelin wrote to Elijah privately).\n\nIt exists: https://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/\n\n...which wasn't Johannes' request.\n\n> But I am almost certain that the behavior requested here is to disable\n> rename detection to match the behavior of GitHub's prior use of libgit2\n> to perform merges, where we also had rename detection disabled (for\n> reasons that are unclear to me, but Peff might know).\n\nNo, if that were the sole reason, I'd say it probably only belongs in\nour internal fork. Disabling of rename detection within GitHub was a\ntemporary internal migration measure, not a desired end state -- at\nleast that's the way Johannes portrayed it to me. I know that\n\"temporary\" sometimes lasts longer than we want, but now that I've\nbecome internal to GitHub, one of the things I want to do is add some\nweight to that \"temporary\" modifier.\n\n> > > diff --git a/merge-ort.c b/merge-ort.c\n> > > index b4ff24403a1..a6960b6a1b4 100644\n> > > --- a/merge-ort.c\n> > > +++ b/merge-ort.c\n> > > @@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)\n> > >\n> > > if (!possible_renames(renames))\n> > > goto cleanup;\n> > > + if (opt->detect_renames == 0) {\n>\n> if (!opt->detect_renames)\n>\n> ?\n\nYeah, I wanted an opt->detect_renames == DIFF_DETECT_NONE, but we\nnever defined that and only defined DIFF_DETECT_RENAME and\nDIFF_DETECT_COPY. I'll switch it over.\n\n> > > + renames->redo_after_renames = 0;\n> > > + renames->cached_pairs_valid_side = 0;\n> > > + goto cleanup;\n> > > + }\n> > >\n> > > trace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n> > > detection_run |= detect_regular_renames(opt, MERGE_SIDE1);\n> >\n> > Do we want to add a test that demonstrates that the option works as\n> > expected?\n>\n> Yeah, having a test here would be nice.\n\nWill add.\n"},{"id":"514124","messageId":"CABPp-BHoHsc8Pov+d3B6i9T4sRg2g4-tt+hLYg1VHT07XLyncw@mail.gmail.com","threadId":"63075","inReplyTo":"Z9HopHJKPv7CXI3R@nand.local","subject":"Re: [PATCH 3/3] merge-ort: support having merge verbosity be set to 0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T21:44:24Z","receivedAt":"2025-03-12T21:44:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 1:03 PM Taylor Blau wrote:\n>\n> On Fri, Mar 07, 2025 at 03:48:42PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Various callers such as am & checkout set the merge verbosity to 0 to\n> > avoid having conflict messages printed. While this could be achieved by\n> > avoiding the wrappers from merge-ort-wrappers and instead passing 0 for\n> > display_update_msgs to merge_switch_to_result(), for simplicity of\n> > converting callers simply allow them to also achieve this with the\n> > merge-ort-wrappers by setting verbosity to 0.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 2 ++\n> > 1 file changed, 2 insertions(+)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index a6960b6a1b4..8021083c112 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -799,6 +799,8 @@ static void path_msg(struct merge_options *opt,\n> > return; /* Do not record mere hints in headers */\n> > if (opt->priv->call_depth && opt->verbosity < 5)\n> > return; /* Ignore messages from inner merges */\n> > + if (!opt->verbosity)\n> > + return;\n>\n> Looks trivially correct ;-).\n\nYeah, but after thinking about it more I don't like it here. I don't\nreally like the opt->verbosity thing from merge-recursive, it simply\nsurvived because it was a semi-public API, but I'd rather minimize it.\nSo, in the next round I think I'm going to stick this in the\nmerge-ort-wrappers rather than have it appear here.\n\n> Should we add a test to ensure that we don't regress this behavior in the future?\n\nI'd rather reuse `git {switch,checkout} -m`'s tests for this purpose\nrather than adding new ones (and perhaps the ones from git-am; can't\nremember if those also caught this). If you feel strongly about this,\nI'll just squash this into the later patch and make it bigger so we\ndon't need more redundant tests.\n"},{"id":"514129","messageId":"Z9IBql7M3UuVMotw@nand.local","threadId":"63075","inReplyTo":"CABPp-BFj08mpXGiSoZ3xZ4KamwTJ4k5wPLTCJh6hUtKSxWE52Q@mail.gmail.com","subject":"Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T21:50:34Z","receivedAt":"2025-03-12T21:50:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 02:40:35PM -0700, Elijah Newren wrote:\n> > I don't know if a link exists; I suspect the request referred to here is\n> > an email that Johannes Schindelin wrote to Elijah privately).\n>\n> It exists: https://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/\n>\n> ...which wasn't Johannes' request.\n\nAh, thanks for the link!\n\n> > But I am almost certain that the behavior requested here is to disable\n> > rename detection to match the behavior of GitHub's prior use of libgit2\n> > to perform merges, where we also had rename detection disabled (for\n> > reasons that are unclear to me, but Peff might know).\n>\n> No, if that were the sole reason, I'd say it probably only belongs in\n> our internal fork. Disabling of rename detection within GitHub was a\n> temporary internal migration measure, not a desired end state -- at\n> least that's the way Johannes portrayed it to me. I know that\n> \"temporary\" sometimes lasts longer than we want, but now that I've\n> become internal to GitHub, one of the things I want to do is add some\n> weight to that \"temporary\" modifier.\n\n:-).\n\nThanks,\nTaylor\n"},{"id":"514130","messageId":"Z9IBv6ia3VHNmF1p@nand.local","threadId":"63075","inReplyTo":"CABPp-BHoHsc8Pov+d3B6i9T4sRg2g4-tt+hLYg1VHT07XLyncw@mail.gmail.com","subject":"Re: [PATCH 3/3] merge-ort: support having merge verbosity be set to 0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T21:50:55Z","receivedAt":"2025-03-12T21:50:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 02:44:24PM -0700, Elijah Newren wrote:\n> > Should we add a test to ensure that we don't regress this behavior in the future?\n>\n> I'd rather reuse `git {switch,checkout} -m`'s tests for this purpose\n> rather than adding new ones (and perhaps the ones from git-am; can't\n> remember if those also caught this). If you feel strongly about this,\n> I'll just squash this into the later patch and make it bigger so we\n> don't need more redundant tests.\n\nNo, I don't feel strongly.\n\nThanks,\nTaylor\n"},{"id":"514145","messageId":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.git.1741362522.gitgitgadget@gmail.com","subject":"[PATCH v2 0/6] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:35Z","receivedAt":"2025-03-13T02:46:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I've got 19 patches covering the work needed to prep for and allow us to\ndelete merge-recursive.[ch], and remap 'recursive' to 'ort', including some\nclean-up along the way. I've tried to divide it up into some smaller patch\nseries.\n\nThese 6 patches are the first of those series. Breakdown:\n\n * The first 3 patches provide small new features to allow us to convert\n later callers\n * Patches 4-5 document and fix a bug with directory rename detection being\n turned off (since git am always turned off directory rename detection,\n this is a prerequisite for converting git am)\n * Patch 6 converts git am from using merge_recursive_generic() to use the\n new merge_ort_generic().\n\nChanges since v1:\n\n * Patch 1: Added an explanation in the commit message about the one\n difference between merge_recursive_generic() and merge_ort_generic() --\n the label for the ancestor commit\n * Patch 2: Linked the relevant discussion in the commit message, fixed a\n style issue, and added a testcase\n * Patch 3: Since the opt->verbosity stuff was an unwanted carryover (due to\n being partially public API), move the tweak to the merge-ort-wrappers to\n avoid promoting it\n * Added 3 more patches, so folks can see one of the callers of\n merge_ort_generic().\n\nElijah Newren (5):\n merge-ort: add new merge_ort_generic() function\n merge-ort: allow rename detection to be disabled\n merge-ort: support having merge verbosity be set to 0\n merge-ort: fix merge.directoryRenames=false\n am: switch from merge_recursive_generic() to merge_ort_generic()\n\nJohannes Schindelin (1):\n t3650: document bug when directory renames are turned off\n\n Documentation/merge-strategies.adoc | 12 ++---\n builtin/am.c | 5 +-\n merge-ort-wrappers.c | 72 ++++++++++++++++++++++++++++-\n merge-ort-wrappers.h | 12 +++++\n merge-ort.c | 53 ++++++++++++++++++---\n merge-ort.h | 5 ++\n t/t3650-replay-basics.sh | 22 +++++++++\n t/t4151-am-abort.sh | 2 +-\n t/t4255-am-submodule.sh | 1 -\n t/t4301-merge-tree-write-tree.sh | 6 +++\n t/t6427-diff3-conflict-markers.sh | 2 +-\n 11 files changed, 172 insertions(+), 20 deletions(-)\n\n\nbase-commit: a36e024e989f4d35f35987a60e3af8022cac3420\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1875%2Fnewren%2Fendit-new-features-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1875/newren/endit-new-features-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1875\n\nRange-diff vs v1:\n\n 1: 9f73e54224d ! 1: c2f2e3e0cd6 merge-ort: add new merge_ort_generic() function\n @@ Commit message\n equivalent for the final entry point, so we can switch callers to\n use it and remove merge-recursive.[ch].\n \n + While porting it over, finally fix the issue with the label for the\n + ancestor (used when merge.conflictStyle=diff3 as a conflict label).\n + merge-recursive.c has traditionally not allowed callers to set that\n + label, but I have found that problematic for years.\n +\n + (Side note: This function was initially part of the merge-ort rewrite,\n + but reviewers questioned the ancestor label funnyness which I was\n + never really happy with anyway. It resulted in me jettisoning it and\n + hoping at the time that I would eventually be able to force the existing\n + callers to use some other API. That worked with `git stash`, as per\n + 874cf2a60444 (stash: apply stash using 'merge_ort_nonrecursive()',\n + 2022-05-10), but this API is the most reasonable one for `git am` and\n + `git merge-recursive`, if we can just allow them some freedom over the\n + ancestor label.)\n +\n + The merge_recursive_generic() function did not know whether it was being\n + invoked by `git stash`, `git merge-recursive`, or `git am`, and the\n + choice of meaningful ancestor label, when there is a unique ancestor,\n + varies for these different callers:\n +\n + * git am: ancestor is a constructed \"fake ancestor\" that user knows\n + nothing about and has no access to. (And is different than\n + the normal thing we mean by a \"virtual merge base\" which is\n + the merging of merge bases.)\n + * git merge-recursive: ancestor might be a tree, but at least it\n + was one specified by the user (if they invoked\n + merge-recursive directly)\n + * git stash: ancestor was the commit serving as the stash base\n +\n + Thus, using a label like \"constructed merge base\" (as\n + merge_recursive_generic() does) presupposes that `git am` is the only\n + caller; it is incorrect for other callers. This label has thrown me off\n + more than once. Allow the caller to override when there is a unique\n + merge base.\n +\n Signed-off-by: Elijah Newren \n \n ## merge-ort-wrappers.c ##\n 2: 4292b22723f ! 2: f401a8e0967 merge-ort: allow rename detection to be disabled\n @@ Commit message\n longer with the option disabled seems unlikely to help surface such\n issues at this point. Also, there has been at least one request to\n allow rename detection to be disabled for behavioral rather than\n - performance reasons, so let's start heeding the config and command line\n - settings.\n + performance reasons (see the thread including\n + https://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/\n + ), so let's start heeding the config and command line settings.\n \n Signed-off-by: Elijah Newren \n \n @@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt)\n \n \tif (!possible_renames(renames))\n \t\tgoto cleanup;\n -+\tif (opt->detect_renames == 0) {\n ++\tif (!opt->detect_renames) {\n +\t\trenames->redo_after_renames = 0;\n +\t\trenames->cached_pairs_valid_side = 0;\n +\t\tgoto cleanup;\n @@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt)\n \n \ttrace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n \tdetection_run |= detect_regular_renames(opt, MERGE_SIDE1);\n +\n + ## t/t4301-merge-tree-write-tree.sh ##\n +@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Clean merge' '\n + \ttest_cmp expect actual\n + '\n + \n ++# Repeat the previous test, but turn off rename detection\n ++test_expect_success 'Failed merge without rename detection' '\n ++\ttest_must_fail git -c diff.renames=false merge-tree --write-tree side1 side3 >out &&\n ++\tgrep \"CONFLICT (modify/delete): numbers deleted\" out\n ++'\n ++\n + test_expect_success 'Content merge and a few conflicts' '\n + \tgit checkout side1^0 &&\n + \ttest_must_fail git merge side2 &&\n 3: c2a2be336e0 < -: ----------- merge-ort: support having merge verbosity be set to 0\n -: ----------- > 3: a508b0a0fe2 merge-ort: support having merge verbosity be set to 0\n -: ----------- > 4: fefda4add11 t3650: document bug when directory renames are turned off\n -: ----------- > 5: b25225c3cab merge-ort: fix merge.directoryRenames=false\n -: ----------- > 6: 3f4b74eb3b9 am: switch from merge_recursive_generic() to merge_ort_generic()\n\n-- \ngitgitgadget\n"},{"id":"514146","messageId":"c2f2e3e0cd646eae2801bf8fb079266f43219921.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 1/6] merge-ort: add new merge_ort_generic() function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:36Z","receivedAt":"2025-03-13T02:46:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-recursive.[ch] have three entry points:\n * merge_trees()\n * merge_recursive()\n * merge_recursive_generic()\nmerge-ort*.[ch] only has equivalents for the first two. Add an\nequivalent for the final entry point, so we can switch callers to\nuse it and remove merge-recursive.[ch].\n\nWhile porting it over, finally fix the issue with the label for the\nancestor (used when merge.conflictStyle=diff3 as a conflict label).\nmerge-recursive.c has traditionally not allowed callers to set that\nlabel, but I have found that problematic for years.\n\n(Side note: This function was initially part of the merge-ort rewrite,\nbut reviewers questioned the ancestor label funnyness which I was\nnever really happy with anyway. It resulted in me jettisoning it and\nhoping at the time that I would eventually be able to force the existing\ncallers to use some other API. That worked with `git stash`, as per\n874cf2a60444 (stash: apply stash using 'merge_ort_nonrecursive()',\n2022-05-10), but this API is the most reasonable one for `git am` and\n`git merge-recursive`, if we can just allow them some freedom over the\nancestor label.)\n\nThe merge_recursive_generic() function did not know whether it was being\ninvoked by `git stash`, `git merge-recursive`, or `git am`, and the\nchoice of meaningful ancestor label, when there is a unique ancestor,\nvaries for these different callers:\n\n * git am: ancestor is a constructed \"fake ancestor\" that user knows\n nothing about and has no access to. (And is different than\n the normal thing we mean by a \"virtual merge base\" which is\n the merging of merge bases.)\n * git merge-recursive: ancestor might be a tree, but at least it\n was one specified by the user (if they invoked\n merge-recursive directly)\n * git stash: ancestor was the commit serving as the stash base\n\nThus, using a label like \"constructed merge base\" (as\nmerge_recursive_generic() does) presupposes that `git am` is the only\ncaller; it is incorrect for other callers. This label has thrown me off\nmore than once. Allow the caller to override when there is a unique\nmerge base.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort-wrappers.c | 64 ++++++++++++++++++++++++++++++++++++++++++++\n merge-ort-wrappers.h | 12 +++++++++\n merge-ort.c | 17 ++++++++----\n merge-ort.h | 5 ++++\n 4 files changed, 93 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\nindex d6f61359965..62834c30e9e 100644\n--- a/merge-ort-wrappers.c\n+++ b/merge-ort-wrappers.c\n@@ -1,9 +1,13 @@\n #include \"git-compat-util.h\"\n #include \"gettext.h\"\n #include \"hash.h\"\n+#include \"hex.h\"\n+#include \"lockfile.h\"\n #include \"merge-ort.h\"\n #include \"merge-ort-wrappers.h\"\n #include \"read-cache-ll.h\"\n+#include \"repository.h\"\n+#include \"tag.h\"\n #include \"tree.h\"\n \n #include \"commit.h\"\n@@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt,\n \n \treturn tmp.clean;\n }\n+\n+static struct commit *get_ref(struct repository *repo,\n+\t\t\t const struct object_id *oid,\n+\t\t\t const char *name)\n+{\n+\tstruct object *object;\n+\n+\tobject = deref_tag(repo, parse_object(repo, oid),\n+\t\t\t name, strlen(name));\n+\tif (!object)\n+\t\treturn NULL;\n+\tif (object->type == OBJ_TREE)\n+\t\treturn make_virtual_commit(repo, (struct tree*)object, name);\n+\tif (object->type != OBJ_COMMIT)\n+\t\treturn NULL;\n+\tif (repo_parse_commit(repo, (struct commit *)object))\n+\t\treturn NULL;\n+\treturn (struct commit *)object;\n+}\n+\n+int merge_ort_generic(struct merge_options *opt,\n+\t\t const struct object_id *head,\n+\t\t const struct object_id *merge,\n+\t\t int num_merge_bases,\n+\t\t const struct object_id *merge_bases,\n+\t\t struct commit **result)\n+{\n+\tint clean;\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct commit *head_commit = get_ref(opt->repo, head, opt->branch1);\n+\tstruct commit *next_commit = get_ref(opt->repo, merge, opt->branch2);\n+\tstruct commit_list *ca = NULL;\n+\n+\tif (merge_bases) {\n+\t\tint i;\n+\t\tfor (i = 0; i < num_merge_bases; ++i) {\n+\t\t\tstruct commit *base;\n+\t\t\tif (!(base = get_ref(opt->repo, &merge_bases[i],\n+\t\t\t\t\t oid_to_hex(&merge_bases[i]))))\n+\t\t\t\treturn error(_(\"Could not parse object '%s'\"),\n+\t\t\t\t\t oid_to_hex(&merge_bases[i]));\n+\t\t\tcommit_list_insert(base, &ca);\n+\t\t}\n+\t}\n+\n+\trepo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n+\tclean = merge_ort_recursive(opt, head_commit, next_commit, ca,\n+\t\t\t\t result);\n+\tfree_commit_list(ca);\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&lock);\n+\t\treturn clean;\n+\t}\n+\n+\tif (write_locked_index(opt->repo->index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\treturn error(_(\"Unable to write index.\"));\n+\n+\treturn clean ? 0 : 1;\n+}\ndiff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h\nindex 90af1f69c55..aeffa1c87b4 100644\n--- a/merge-ort-wrappers.h\n+++ b/merge-ort-wrappers.h\n@@ -22,4 +22,16 @@ int merge_ort_recursive(struct merge_options *opt,\n \t\t\tconst struct commit_list *ancestors,\n \t\t\tstruct commit **result);\n \n+/*\n+ * rename-detecting three-way merge. num_merge_bases must be at least 1.\n+ * Recursive ancestor consolidation will be performed if num_merge_bases > 1.\n+ * Wrapper mimicking the old merge_recursive_generic() function.\n+ */\n+int merge_ort_generic(struct merge_options *opt,\n+\t\t const struct object_id *head,\n+\t\t const struct object_id *merge,\n+\t\t int num_merge_bases,\n+\t\t const struct object_id *merge_bases,\n+\t\t struct commit **result);\n+\n #endif\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..b4ff24403a1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4878,9 +4878,9 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)\n \tc->maybe_tree = t;\n }\n \n-static struct commit *make_virtual_commit(struct repository *repo,\n-\t\t\t\t\t struct tree *tree,\n-\t\t\t\t\t const char *comment)\n+struct commit *make_virtual_commit(struct repository *repo,\n+\t\t\t\t struct tree *tree,\n+\t\t\t\t const char *comment)\n {\n \tstruct commit *commit = alloc_commit_node(repo);\n \n@@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\tancestor_name = \"empty tree\";\n \t} else if (merge_bases) {\n \t\tancestor_name = \"merged common ancestors\";\n+\t} else if (opt->ancestor) {\n+\t\tancestor_name = opt->ancestor;\n \t} else {\n \t\tstrbuf_add_unique_abbrev(&merge_base_abbrev,\n \t\t\t\t\t &merged_merge_bases->object.oid,\n@@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt,\n {\n \ttrace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n \n-\t/* We set the ancestor label based on the merge_bases */\n-\tassert(opt->ancestor == NULL);\n+\t/*\n+\t * We set the ancestor label based on the merge_bases...but we\n+\t * allow one exception through so that builtin/am can override\n+\t * with its constructed fake ancestor.\n+\t */\n+\tassert(opt->ancestor == NULL ||\n+\t (merge_bases && !merge_bases->next));\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n \tmerge_start(opt, result);\ndiff --git a/merge-ort.h b/merge-ort.h\nindex 82f2b3222d2..b63bc5424e7 100644\n--- a/merge-ort.h\n+++ b/merge-ort.h\n@@ -44,6 +44,11 @@ struct merge_result {\n \tunsigned _properly_initialized;\n };\n \n+/* Mostly internal function also used by merge-ort-wrappers.c */\n+struct commit *make_virtual_commit(struct repository *repo,\n+\t\t\t\t struct tree *tree,\n+\t\t\t\t const char *comment);\n+\n /*\n * rename-detecting three-way merge with recursive ancestor consolidation.\n * working tree and index are untouched.\n-- \ngitgitgadget\n\n"},{"id":"514147","messageId":"a508b0a0fe21199ff8cc6a1c36fe237b35a84deb.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 3/6] merge-ort: support having merge verbosity be set to 0","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:38Z","receivedAt":"2025-03-13T02:46:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nVarious callers such as am & checkout set the merge verbosity to 0 to\navoid having conflict messages printed. While this could be achieved by\navoiding the wrappers from merge-ort-wrappers and instead passing 0 for\ndisplay_update_msgs to merge_switch_to_result(), for simplicity of\nconverting callers simply allow them to also achieve this with the\nmerge-ort-wrappers by setting verbosity to 0.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort-wrappers.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\nindex 62834c30e9e..c54d56b3446 100644\n--- a/merge-ort-wrappers.c\n+++ b/merge-ort-wrappers.c\n@@ -33,6 +33,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,\n \t\t\t struct tree *merge_base)\n {\n \tstruct merge_result result;\n+\tint show_msgs;\n \n \tif (unclean(opt, head))\n \t\treturn -1;\n@@ -42,9 +43,10 @@ int merge_ort_nonrecursive(struct merge_options *opt,\n \t\treturn 1;\n \t}\n \n+\tshow_msgs = !!opt->verbosity;\n \tmemset(&result, 0, sizeof(result));\n \tmerge_incore_nonrecursive(opt, merge_base, head, merge, &result);\n-\tmerge_switch_to_result(opt, head, &result, 1, 1);\n+\tmerge_switch_to_result(opt, head, &result, 1, show_msgs);\n \n \treturn result.clean;\n }\n@@ -57,13 +59,15 @@ int merge_ort_recursive(struct merge_options *opt,\n {\n \tstruct tree *head = repo_get_commit_tree(opt->repo, side1);\n \tstruct merge_result tmp;\n+\tint show_msgs;\n \n \tif (unclean(opt, head))\n \t\treturn -1;\n \n+\tshow_msgs = !!opt->verbosity;\n \tmemset(&tmp, 0, sizeof(tmp));\n \tmerge_incore_recursive(opt, merge_bases, side1, side2, &tmp);\n-\tmerge_switch_to_result(opt, head, &tmp, 1, 1);\n+\tmerge_switch_to_result(opt, head, &tmp, 1, show_msgs);\n \t*result = NULL;\n \n \treturn tmp.clean;\n-- \ngitgitgadget\n\n"},{"id":"514148","messageId":"f401a8e09676c9c808792665c3ffadd9d2672485.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 2/6] merge-ort: allow rename detection to be disabled","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:37Z","receivedAt":"2025-03-13T02:46:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen merge-ort was written, I did not at first allow rename detection to\nbe disabled, because I suspected that most folks disabling rename\ndetection were doing so solely for performance reasons. Since I put a\nlot of working into providing dramatic speedups for rename detection\nperformance as used by the merge machinery, I wanted to know if there\nwere still real world repositories where rename detection was\nproblematic from a performance perspective. We have had years now to\ncollect such information, and while we never received one, waiting\nlonger with the option disabled seems unlikely to help surface such\nissues at this point. Also, there has been at least one request to\nallow rename detection to be disabled for behavioral rather than\nperformance reasons (see the thread including\nhttps://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/\n), so let's start heeding the config and command line settings.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/merge-strategies.adoc | 12 ++++++------\n merge-ort.c | 5 +++++\n t/t4301-merge-tree-write-tree.sh | 6 ++++++\n 3 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/merge-strategies.adoc b/Documentation/merge-strategies.adoc\nindex 93822ebc4e8..59f5ae36ccb 100644\n--- a/Documentation/merge-strategies.adoc\n+++ b/Documentation/merge-strategies.adoc\n@@ -82,6 +82,11 @@ find-renames[=];;\n rename-threshold=;;\n \tDeprecated synonym for `find-renames=`.\n \n+no-renames;;\n+\tTurn off rename detection. This overrides the `merge.renames`\n+\tconfiguration variable.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n subtree[=];;\n \tThis option is a more advanced form of 'subtree' strategy, where\n \tthe strategy makes a guess on how two trees must be shifted to\n@@ -107,7 +112,7 @@ For a path that is a submodule, the same caution as 'ort' applies to this\n strategy.\n +\n The 'recursive' strategy takes the same options as 'ort'. However,\n-there are three additional options that 'ort' ignores (not documented\n+there are two additional options that 'ort' ignores (not documented\n above) that are potentially useful with the 'recursive' strategy:\n \n patience;;\n@@ -121,11 +126,6 @@ diff-algorithm=[patience|minimal|histogram|myers];;\n \tspecifically uses `diff-algorithm=histogram`, while `recursive`\n \tdefaults to the `diff.algorithm` config setting.\n \n-no-renames;;\n-\tTurn off rename detection. This overrides the `merge.renames`\n-\tconfiguration variable.\n-\tSee also linkgit:git-diff[1] `--no-renames`.\n-\n resolve::\n \tThis can only resolve two heads (i.e. the current branch\n \tand another branch you pulled from) using a 3-way merge\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b4ff24403a1..1d3b690224e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)\n \n \tif (!possible_renames(renames))\n \t\tgoto cleanup;\n+\tif (!opt->detect_renames) {\n+\t\trenames->redo_after_renames = 0;\n+\t\trenames->cached_pairs_valid_side = 0;\n+\t\tgoto cleanup;\n+\t}\n \n \ttrace2_region_enter(\"merge\", \"regular renames\", opt->repo);\n \tdetection_run |= detect_regular_renames(opt, MERGE_SIDE1);\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex eea19907b55..44f7d077593 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -73,6 +73,12 @@ test_expect_success 'Clean merge' '\n \ttest_cmp expect actual\n '\n \n+# Repeat the previous test, but turn off rename detection\n+test_expect_success 'Failed merge without rename detection' '\n+\ttest_must_fail git -c diff.renames=false merge-tree --write-tree side1 side3 >out &&\n+\tgrep \"CONFLICT (modify/delete): numbers deleted\" out\n+'\n+\n test_expect_success 'Content merge and a few conflicts' '\n \tgit checkout side1^0 &&\n \ttest_must_fail git merge side2 &&\n-- \ngitgitgadget\n\n"},{"id":"514149","messageId":"fefda4add11074288811752f922c309963c57eea.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 4/6] t3650: document bug when directory renames are turned off","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:39Z","receivedAt":"2025-03-13T02:46:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is a bug in the way renames are cached that rears its head when\n`merge.directoryRenames` is set to false; it results in the following\nmessage:\n\n merge-ort.c:3002: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.\n Aborted\n\nIt is quite a curious bug: the same test case will succeed, without any\nassertion, if instead run with `merge.directoryRenames=true`.\n\nFurther, the assertion does not manifest while replaying the first\ncommit, it manifests while replaying the _second_ commit of the commit\nrange. But it does _not_ manifest when the second commit is replayed\nindividually.\n\nThis would indicate that there is an incomplete rename cache left-over\nfrom the first replayed commit which is being reused for the second\ncommit, and if directory rename detection is enabled, the missing paths\nare somehow regenerated.\n\nIncidentally, the same bug can by triggered by modifying t6429 to switch\nfrom merge.directoryRenames=true to merge.directoryRenames=false.\n\nSigned-off-by: Johannes Schindelin \n[en: tweaked the commit message slightly, including adjusting the\n line number of the assertion to the latest version, and the much\n later discovery that a simple t6429 tweak would also display the\n issue.]\nSigned-off-by: Elijah Newren \n---\n t/t3650-replay-basics.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex 389670262e4..cade7930765 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -195,4 +195,26 @@ test_expect_success 'using replay on bare repo to rebase multiple divergent bran\n \tdone\n '\n \n+test_expect_failure 'merge.directoryRenames=false' '\n+\t# create a test case that stress-tests the rename caching\n+\tgit switch -c rename-onto &&\n+\n+\tmkdir -p to-rename &&\n+\ttest_commit to-rename/move &&\n+\n+\tmkdir -p renamed-directory &&\n+\tgit mv to-rename/move* renamed-directory/ &&\n+\ttest_tick &&\n+\tgit commit -m renamed-directory &&\n+\n+\tgit switch -c rename-from HEAD^ &&\n+\ttest_commit to-rename/add-a-file &&\n+\techo modified >to-rename/add-a-file.t &&\n+\ttest_tick &&\n+\tgit commit -m modified to-rename/add-a-file.t &&\n+\n+\tgit -c merge.directoryRenames=false replay \\\n+\t\t--onto rename-onto rename-onto..rename-from\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"514150","messageId":"b25225c3cabf7aafc659489e302193fd57829d5e.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 5/6] merge-ort: fix merge.directoryRenames=false","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:40Z","receivedAt":"2025-03-13T02:46:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are two issues here.\n\nFirst, when merge.directoryRenames is set to false, there are a few code\npaths that should be turned off. I missed one; collect_renames() was\nstill doing some directory rename detection logic unconditionally. It\nended up not having much effect because\nget_provisional_directory_renames() was skipped earlier and not setting\nup renames->dir_renames, but the code should still be skipped.\n\nSecond, the larger issue is that sometimes we get a cached_pair rename\nfrom a previous commit being replayed mapping A->B, but in a subsequent\ncommit but collect_merge_info() doesn't even recurse into the\ndirectory containing B because there are no source pairings for that\nrename that are relevant; we can merge that commit fine without knowing\nthe rename. But since the cached renames are added to the normal\nrenames, when we go to process it and find that B is not part of\nopt->priv->paths, we hit the assertion error\n process_renames: Assertion `newinfo && ~newinfo->merged.clean` failed.\nI think we could fix this at the beginning of detect_regular_renames() by\npruning from cached_pairs any entry whose destination isn't in\nopt->priv->paths, but it's suboptimal in that we'd kind of like the\ncached_pair to be restored afterwards so that it can help the subsequent\ncommit, but more importantly since it sits at the intersection of\nthe caching renames optimization and the relevant renames optimization,\nand the trivial directory resolution optimization, and I don't currently\nhave Documentation/technical/remembering-renames.txt fully paged in, I'm\nnot sure if that's a full solution or a bandaid for the current\ntestcase. However, since the remembering renames optimization was the\nweakest of the set, and the optimization is far less important when\ndirectory rename detection is off (as that implies far fewer potential\nrenames), let's just use a bigger hammer to ensure this special case is\nfixed: turn off the rename caching. We do the same thing already when\nwe encounter rename/rename(1to1) cases (as per `git grep -3\ndisabling.the.optimization`, though it uses a slightly different\ntriggering mechanism since it's trying to affect the next time that\nmerge_check_renames_reusable() is called), and I think it makes sense\nto do the same here.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 31 +++++++++++++++++++++++++++++--\n t/t3650-replay-basics.sh | 2 +-\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 1d3b690224e..785e5c6f24a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3404,6 +3404,11 @@ static int collect_renames(struct merge_options *opt,\n \t\t\tpool_diff_free_filepair(&opt->priv->pool, p);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE &&\n+\t\t p->status == 'R' && 1) {\n+\t\t\tpossibly_cache_new_pair(renames, p, side_index, NULL);\n+\t\t\tgoto skip_directory_renames;\n+\t\t}\n \n \t\tnew_path = check_for_directory_rename(opt, p->two->path,\n \t\t\t\t\t\t side_index,\n@@ -3421,6 +3426,7 @@ static int collect_renames(struct merge_options *opt,\n \t\tif (new_path)\n \t\t\tapply_directory_rename_modifications(opt, p, new_path);\n \n+skip_directory_renames:\n \t\t/*\n \t\t * p->score comes back from diffcore_rename_extended() with\n \t\t * the similarity of the renamed file. The similarity is\n@@ -5025,7 +5031,8 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \ttrace2_region_leave(\"merge\", \"allocate/init\", opt->repo);\n }\n \n-static void merge_check_renames_reusable(struct merge_result *result,\n+static void merge_check_renames_reusable(struct merge_options *opt,\n+\t\t\t\t\t struct merge_result *result,\n \t\t\t\t\t struct tree *merge_base,\n \t\t\t\t\t struct tree *side1,\n \t\t\t\t\t struct tree *side2)\n@@ -5050,6 +5057,26 @@ static void merge_check_renames_reusable(struct merge_result *result,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Avoid using cached renames when directory rename detection is\n+\t * turned off. Cached renames are far less important in that case,\n+\t * and they lead to testcases with an interesting intersection of\n+\t * effects from relevant renames optimization, trivial directory\n+\t * resolution optimization, and cached renames all converging when\n+\t * the target of a cached rename is in a directory that\n+\t * collect_merge_info() does not recurse into. To avoid such\n+\t * problems, simply disable cached renames for this case (similar\n+\t * to the rename/rename(1to1) case; see the \"disabling the\n+\t * optimization\" comment near that case).\n+\t *\n+\t * This could be revisited in the future; see the commit message\n+\t * where this comment was added for some possible pointers.\n+\t */\n+\tif (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE) {\n+\t\trenames->cached_pairs_valid_side = 0; /* neither side valid */\n+\t\treturn;\n+\t}\n+\n \t/*\n \t * Handle other cases; note that merge_trees[0..2] will only\n \t * be NULL if opti is, or if all three were manually set to\n@@ -5258,7 +5285,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n \tassert(opt->ancestor != NULL);\n-\tmerge_check_renames_reusable(result, merge_base, side1, side2);\n+\tmerge_check_renames_reusable(opt, result, merge_base, side1, side2);\n \tmerge_start(opt, result);\n \t/*\n \t * Record the trees used in this merge, so if there's a next merge in\ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex cade7930765..58b37599357 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -195,7 +195,7 @@ test_expect_success 'using replay on bare repo to rebase multiple divergent bran\n \tdone\n '\n \n-test_expect_failure 'merge.directoryRenames=false' '\n+test_expect_success 'merge.directoryRenames=false' '\n \t# create a test case that stress-tests the rename caching\n \tgit switch -c rename-onto &&\n \n-- \ngitgitgadget\n\n"},{"id":"514151","messageId":"3f4b74eb3b934c62edb23b69db7460e3e0b44877.1741834001.git.gitgitgadget@gmail.com","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"[PATCH v2 6/6] am: switch from merge_recursive_generic() to merge_ort_generic()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-13T02:46:41Z","receivedAt":"2025-03-13T02:46:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSwitch from merge-recursive to merge-ort. Adjust the following\ntestcases due to the switch:\n\n* t4151: This test left an untracked file in the way of the merge.\n merge-recursive could only sometimes tell when untracked files were\n in the way, and by the time it discovers others, it has already made\n too many changes to back out of the merge. So, instead of writing the\n results to e.g. 'file1' it would instead write them to\n 'file1~branch1'. This is confusing for users, because they might not\n notice 'file1~branch1' and accidentally add and commit 'file1'.\n In contrast, merge-ort correctly notices the file in the way before\n making any changes and aborts. Since this test didn't care about the\n file in the way, just remove it before calling git-am.\n\n* t4255: Usage of merge-ort allows us to change two known failures into\n successes.\n\n* t6427: As noted a few commits ago, the choice of conflict label for\n diff3 markers for the ancestor commit was previously handled by\n merge-recursive.c rather than by callers. Since that has now changed,\n `git am` needs to specify that label. Although the previous conflict\n label (\"constructed merge base\") was already fairly somewhat slanted\n towards `git am`, let's use wording more along the lines of the\n related command-line flag from `git apply` and function involved to\n tie it more closely to `git am`.\n\nSigned-off-by: Elijah Newren \n---\n builtin/am.c | 5 +++--\n t/t4151-am-abort.sh | 2 +-\n t/t4255-am-submodule.sh | 1 -\n t/t6427-diff3-conflict-markers.sh | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 2921bb89ef1..3b61bd4c333 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -31,7 +31,7 @@\n #include \"preload-index.h\"\n #include \"sequencer.h\"\n #include \"revision.h\"\n-#include \"merge-recursive.h\"\n+#include \"merge-ort-wrappers.h\"\n #include \"log-tree.h\"\n #include \"notes-utils.h\"\n #include \"rerere.h\"\n@@ -1638,12 +1638,13 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \to.branch1 = \"HEAD\";\n \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n \to.branch2 = their_tree_name;\n+\to.ancestor = \"constructed fake ancestor\";\n \to.detect_directory_renames = MERGE_DIRECTORY_RENAMES_NONE;\n \n \tif (state->quiet)\n \t\to.verbosity = 0;\n \n-\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n+\tif (merge_ort_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n \t\trepo_rerere(the_repository, state->allow_rerere_autoupdate);\n \t\tfree(their_tree_name);\n \t\treturn error(_(\"Failed to merge in the changes.\"));\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex edb38da7010..8e1ecf8a685 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -112,7 +112,7 @@ test_expect_success 'am --abort will keep dirty index intact' '\n test_expect_success 'am -3 stops on conflict on unborn branch' '\n \tgit checkout -f --orphan orphan &&\n \tgit reset &&\n-\trm -f otherfile-4 &&\n+\trm -f file-1 otherfile-4 &&\n \ttest_must_fail git am -3 0003-*.patch &&\n \ttest 2 -eq $(git ls-files -u | wc -l) &&\n \ttest 4 = \"$(cat otherfile-4)\"\ndiff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh\nindex a7ba08f728c..e6679a01b44 100755\n--- a/t/t4255-am-submodule.sh\n+++ b/t/t4255-am-submodule.sh\n@@ -19,7 +19,6 @@ am_3way () {\n \t$2 git am --3way patch\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n test_submodule_switch_func \"am_3way\"\n \n test_expect_success 'setup diff.submodule' '\ndiff --git a/t/t6427-diff3-conflict-markers.sh b/t/t6427-diff3-conflict-markers.sh\nindex dd5fe6a4021..57569c4f4bd 100755\n--- a/t/t6427-diff3-conflict-markers.sh\n+++ b/t/t6427-diff3-conflict-markers.sh\n@@ -207,7 +207,7 @@ test_expect_success 'rebase --apply describes fake ancestor base' '\n \t\tcd rebase &&\n \t\tgit rebase --abort &&\n \t\ttest_must_fail git -c merge.conflictstyle=diff3 rebase --apply main &&\n-\t\tgrep \"||||||| constructed merge base\" file\n+\t\tgrep \"||||||| constructed fake ancestor\" file\n \t)\n '\n \n-- \ngitgitgadget\n"},{"id":"514155","messageId":"20250313052500.GB94015@coredump.intra.peff.net","threadId":"63075","inReplyTo":"CABPp-BFj08mpXGiSoZ3xZ4KamwTJ4k5wPLTCJh6hUtKSxWE52Q@mail.gmail.com","subject":"Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-13T05:25:00Z","receivedAt":"2025-03-13T05:25:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2025 at 02:40:35PM -0700, Elijah Newren wrote:\n\n> > But I am almost certain that the behavior requested here is to disable\n> > rename detection to match the behavior of GitHub's prior use of libgit2\n> > to perform merges, where we also had rename detection disabled (for\n> > reasons that are unclear to me, but Peff might know).\n> \n> No, if that were the sole reason, I'd say it probably only belongs in\n> our internal fork. Disabling of rename detection within GitHub was a\n> temporary internal migration measure, not a desired end state -- at\n> least that's the way Johannes portrayed it to me. I know that\n> \"temporary\" sometimes lasts longer than we want, but now that I've\n> become internal to GitHub, one of the things I want to do is add some\n> weight to that \"temporary\" modifier.\n\nYes, I think it was a series of hysterical raisins. The original PR\nmerge test at GitHub was done using a shell script around git-merge-file\n(because git-merge insisted on a working tree). And naturally that did\nnot support renames. (I think I probably wrote that script, but it's so\nlong ago I could be wrong, and I don't have access to the repo anymore).\n\nAnd then we switched from that to libgit2, after Ed Thomson implemented\nmerge support there (mostly for performance). And the decision was made\nto disable renames there at first, to confirm that it otherwise\nperformed identically to the existing shell script (to confirm the\nresults, but also because it was unclear if rename detection for\nautomated merges would always produce what the user wanted, or have bad\ncorner cases). So it was mostly temporary, with the idea that somebody\nwould explore turning it on later. But I don't think that ever happened.\nThose with access to the correct repositories can probably find the\narguments in the issue tracker. ;)\n\nI don't think I was around for switching from libgit2 to merge-tree, but\nI'd guess the same \"only change one thing at a time\" logic applied.\n\nSo yes, mostly temporary-but-never-revisited, with a dash of\nconservatism.\n\nI don't have any real opinion on what should happen in the future,\nexcept that renames on GitHub are probably reasonable, and having an\noption to disable renames for everyone is probably also a reasonable\nfeature. ;)\n\n-Peff\n"},{"id":"514435","messageId":"Z9iTX5D/eiFcPAhZ@nand.local","threadId":"63075","inReplyTo":"pull.1875.v2.git.1741834001.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/6] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-17T21:25:51Z","receivedAt":"2025-03-17T21:25:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 13, 2025 at 02:46:35AM +0000, Elijah Newren via GitGitGadget wrote:\n> Range-diff vs v1:\n\nThis is already marked as ready to merge to 'next' in the last WC, but I\ntook a look at the range-diff and everything LGTM:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}