{"thread":{"id":"59336","subject":"[RFC PATCH] sequencer - tipped merge strategy","startedAt":"2023-03-03T14:55:10Z","lastAt":"2023-03-06T17:35:04Z","messageCount":9,"participants":["Edmundo Carmona Antoranz","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472959","messageId":"20230303145311.513960-1-eantoranz@gmail.com","threadId":"59336","inReplyTo":null,"subject":"[RFC PATCH] sequencer - tipped merge strategy","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2023-03-03T14:53:11Z","receivedAt":"2023-03-03T14:55:10Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"When rebasing merge commits and dealing with conflicts, having the\noriginal merge commit as a reference can help us avoid some of\nthem.\n\nWith this patch, we leverage the original merge commit to handle the most\nobvious case:\n- HEAD tree has to match the tree of the first parent of the original merge\n commit.\n- MERGE_HEAD tree has to match the tree of the second parent of the original\n merge commit.\n- At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n a tree in the merge bases of the parent commits of the original merge\n commit.\n\nIf all of those conditions are met, we can safely use the tree of the\noriginal merge commit as the resulting tree of this merge that is being\nattempted at the time.\n\nSigned-off-by: Edmundo Carmona Antoranz \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge-tms.c | 148 ++++++++++++++++++++++++++++++++++++++++++++\n git.c | 1 +\n sequencer.c | 36 ++++++++++-\n 6 files changed, 187 insertions(+), 1 deletion(-)\n create mode 100644 builtin/merge-tms.c\n\ndiff --git a/.gitignore b/.gitignore\nindex e875c59054..8b534f98e6 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -103,6 +103,7 @@\n /git-merge-recursive\n /git-merge-resolve\n /git-merge-subtree\n+/git-merge-tms\n /git-mergetool\n /git-mergetool--lib\n /git-mktag\ndiff --git a/Makefile b/Makefile\nindex 50ee51fde3..10a3167c50 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1264,6 +1264,7 @@ BUILTIN_OBJS += builtin/merge-file.o\n BUILTIN_OBJS += builtin/merge-index.o\n BUILTIN_OBJS += builtin/merge-ours.o\n BUILTIN_OBJS += builtin/merge-recursive.o\n+BUILTIN_OBJS += builtin/merge-tms.o\n BUILTIN_OBJS += builtin/merge-tree.o\n BUILTIN_OBJS += builtin/merge.o\n BUILTIN_OBJS += builtin/mktag.o\ndiff --git a/builtin.h b/builtin.h\nindex 46cc789789..94dcb73f85 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -180,6 +180,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix);\n int cmd_merge_ours(int argc, const char **argv, const char *prefix);\n int cmd_merge_file(int argc, const char **argv, const char *prefix);\n int cmd_merge_recursive(int argc, const char **argv, const char *prefix);\n+int cmd_merge_tms(int argc, const char **argv, const char *prefix);\n int cmd_merge_tree(int argc, const char **argv, const char *prefix);\n int cmd_mktag(int argc, const char **argv, const char *prefix);\n int cmd_mktree(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/merge-tms.c b/builtin/merge-tms.c\nnew file mode 100644\nindex 0000000000..37a2427757\n--- /dev/null\n+++ b/builtin/merge-tms.c\n@@ -0,0 +1,148 @@\n+/*\n+ * Copyright (c) 2023 Edmundo Carmona Antoranz\n+ * Released under the terms of GPL2\n+ *\n+ * Tipped merge strategy.... a.k.a. fortune-teller merge strategy\n+ *\n+ * In cases like rebases, merge commits offer us the advantage of knowing\n+ * _before hand_ what the previous result of the _original_ branches\n+ * involved was.\n+ *\n+ * This merge strategy tries to leverage this knowledge so that we can\n+ * avoid at least the most obvious conflicts that have been solved in the\n+ * original merge commit.\n+ *\n+ * In the current state, the strategy works based on exact matches of the trees\n+ * involved:\n+ * - HEAD tree has to match the tree of the first parent of the original merge\n+ * commit.\n+ * - MERGE_HEAD tree has to match the tree of the second parent of the original\n+ * merge commit.\n+ * - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n+ * a tree in the merge bases of the parent commits of the original merge\n+ * commit.\n+ * If all of those conditions are met, we can safely use the tree of the\n+ * original merge commit as the resulting tree of this merge that is being\n+ * attempted at the time.\n+ */\n+\n+#include \"builtin.h\"\n+#include \"commit-reach.h\"\n+#include \"oid-array.h\"\n+#include \"parse-options.h\"\n+#include \"run-command.h\"\n+\n+\n+struct tms_options {\n+\tconst char *tip;\n+\tconst char *merge_head;\n+} tms_options;\n+\n+static int restore(struct commit *commit)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_push(&cmd.args, \"restore\");\n+\tstrvec_push(&cmd.args, \"--worktree\");\n+\tstrvec_push(&cmd.args, \"--stage\");\n+\tstrvec_pushf(&cmd.args, \"--source=%s\",\n+\t\t oid_to_hex(&commit->object.oid));\n+\tstrvec_push(&cmd.args, \"--\");\n+\tstrvec_push(&cmd.args, \".\");\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n+}\n+\n+static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)\n+{\n+\tstruct commit_list *i;\n+\n+\tfor (i = bases; i; i = i->next)\n+\t\toid_array_append(oids, get_commit_tree_oid(i->item));\n+}\n+\n+static int find_oid(const struct object_id *oid,\n+\t\tvoid *data)\n+{\n+\tstruct oid_array *other_list = (struct oid_array *) data;\n+\tint pos = oid_array_lookup(other_list, oid);\n+\treturn pos >= 0 ? 1 : 0;\n+}\n+\n+static int base_match(struct commit *rebase_head,\n+\t\tstruct commit *head,\n+\t\tstruct commit *merge_head)\n+{\n+\tstruct commit_list *bases_current, *bases_tip;\n+\tstruct oid_array trees_current = OID_ARRAY_INIT;\n+\tstruct oid_array trees_tip = OID_ARRAY_INIT;\n+\tint oid_match;\n+\n+\tbases_current = get_merge_bases(head, merge_head);\n+\tbases_tip = get_merge_bases(rebase_head->parents->item,\n+\t\t\t\t rebase_head->parents->next->item);\n+\tload_tree_oids(&trees_current, bases_current);\n+\tload_tree_oids(&trees_tip, bases_tip);\n+\n+\toid_match = oid_array_for_each(&trees_current, find_oid, &trees_tip);\n+\n+\toid_array_clear(&trees_current);\n+\toid_array_clear(&trees_tip);\n+\n+\treturn oid_match;\n+}\n+\n+static int run_tms_merge(struct tms_options *options)\n+{\n+\tstruct commit *head, *merge_head, *tip;\n+\tstruct commit_list *i;\n+\n+\thead = lookup_commit_reference_by_name(\"HEAD\");\n+\tmerge_head = lookup_commit_reference_by_name(options->merge_head);\n+\ttip = lookup_commit_reference_by_name(options->tip);\n+\n+\tif (!(head && merge_head && tip)) {\n+\t\treturn 2;\n+\t}\n+\tif (commit_list_count(tip->parents) != 2)\n+\t\treturn 2;\n+\n+\tfor (i = tip->parents; i; i = i->next)\n+\t\tparse_commit(i->item);\n+\tif (!oideq(get_commit_tree_oid(head),\n+\t\t get_commit_tree_oid(tip->parents->item)))\n+\t\treturn 2;\n+\tif (!oideq(get_commit_tree_oid(merge_head),\n+\t\t get_commit_tree_oid(tip->parents->next->item)))\n+\t\treturn 2;\n+\n+\tif (!base_match(tip, head, merge_head))\n+\t\treturn 2;\n+\n+\tif (restore(tip))\n+\t\treturn 2;\n+\n+\treturn 0;\n+}\n+\n+int cmd_merge_tms(int argc, const char **argv, const char *prefix)\n+{\n+\n+\tstruct option mt_options[] = {\n+\t\tOPT_STRING(0, \"tip\", &tms_options.tip,\n+\t\t\t N_(\"tip-merge-commit\"),\n+\t\t\t N_(\"merge commit being rebased used as a tip for conflict resolution.\")),\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, NULL, mt_options,\n+\t\t\t NULL, 0);\n+\n+\tif (argc != 1)\n+\t\treturn 2;\n+\ttms_options.merge_head = argv[0];\n+\n+\tif (!tms_options.tip)\n+\t\treturn 2;\n+\n+\treturn run_tms_merge(&tms_options);\n+}\ndiff --git a/git.c b/git.c\nindex 96b0a2837d..2e843731f1 100644\n--- a/git.c\n+++ b/git.c\n@@ -544,6 +544,7 @@ static struct cmd_struct commands[] = {\n \t{ \"merge-recursive-ours\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n \t{ \"merge-recursive-theirs\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n \t{ \"merge-subtree\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n+\t{ \"merge-tms\", cmd_merge_tms, RUN_SETUP },\n \t{ \"merge-tree\", cmd_merge_tree, RUN_SETUP },\n \t{ \"mktag\", cmd_mktag, RUN_SETUP },\n \t{ \"mktree\", cmd_mktree, RUN_SETUP },\ndiff --git a/sequencer.c b/sequencer.c\nindex 65a34f9676..559169814b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3833,6 +3833,21 @@ static int do_reset(struct repository *r,\n \treturn ret;\n }\n \n+static int try_tms_merge(struct replay_opts *opts,\n+\t\t\t struct commit *rebase_head,\n+\t\t\t struct commit *merge_commit)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_push(&cmd.args, \"merge-tms\");\n+\tstrvec_push(&cmd.args, \"--tip\");\n+\tstrvec_pushf(&cmd.args, \"%s\", oid_to_hex(&rebase_head->object.oid));\n+\tstrvec_pushf(&cmd.args, \"%s\", oid_to_hex(&merge_commit->object.oid));\n+\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd) ? 0 : 1;\n+}\n+\n static int do_merge(struct repository *r,\n \t\t struct commit *commit,\n \t\t const char *arg, int arg_len,\n@@ -3846,7 +3861,8 @@ static int do_merge(struct repository *r,\n \tconst char *strategy = !opts->xopts_nr &&\n \t\t(!opts->strategy ||\n \t\t !strcmp(opts->strategy, \"recursive\") ||\n-\t\t !strcmp(opts->strategy, \"ort\")) ?\n+\t\t !strcmp(opts->strategy, \"ort\") ||\n+\t\t !strcmp(opts->strategy, \"tms\")) ?\n \t\tNULL : opts->strategy;\n \tstruct merge_options o;\n \tint merge_arg_len, oneline_offset, can_fast_forward, ret, k;\n@@ -4086,6 +4102,23 @@ static int do_merge(struct repository *r,\n \to.branch2 = ref_name.buf;\n \to.buffer_output = 2;\n \n+\tif (!opts->strategy || !strcmp(opts->strategy, \"tms\")) {\n+\t\trollback_lock_file(&lock);\n+\t\tret = try_tms_merge(opts, commit, to_merge->item);\n+\t\tif (ret) {\n+\t\t\tdiscard_index(r->index);\n+\t\t\tif (repo_read_index(r) < 0) {\n+\t\t\t\tret = error(_(\"could not read index\"));\n+\t\t\t\tgoto leave_merge;\n+\t\t\t}\n+\t\t\tgoto ran_merge;\n+\t\t}\n+\t\t// regain lock to go into recursive\n+\t\tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto leave_merge;\n+\t\t}\n+\t}\n \tif (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n \t\t/*\n \t\t * TODO: Should use merge_incore_recursive() and\n@@ -4100,6 +4133,7 @@ static int do_merge(struct repository *r,\n \t\tret = merge_recursive(&o, head_commit, merge_commit, bases,\n \t\t\t\t &i);\n \t}\n+ran_merge:\n \tif (ret <= 0)\n \t\tfputs(o.obuf.buf, stdout);\n \tstrbuf_release(&o.obuf);\n-- \n2.39.1\nI think it is ok to write things over here, right?\n\nI would like a little bit of coaching in terms of releasing/regaining\nlock before/after calling the merge strategy built-in. I am not so sure\ncurrent implementation is correct in that front but at least it is\nworking in my tests so I think it is a good starting point for an RFC.\n\nThanks in advance for any feedback you might provide.\n"},{"id":"472963","messageId":"xmqqcz5phjgz.fsf@gitster.g","threadId":"59336","inReplyTo":"20230303145311.513960-1-eantoranz@gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T16:45:32Z","receivedAt":"2023-03-03T16:46:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz writes:\n\n> When rebasing merge commits and dealing with conflicts, having the\n> original merge commit as a reference can help us avoid some of\n> them.\n>\n> With this patch, we leverage the original merge commit to handle the most\n> obvious case:\n> - HEAD tree has to match the tree of the first parent of the original merge\n> commit.\n> - MERGE_HEAD tree has to match the tree of the second parent of the original\n> merge commit.\n> - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n> a tree in the merge bases of the parent commits of the original merge\n> commit.\n\nThe first two conditions look a bit too restrictive for the purpose\nof reusing previous conflict resolution, while I am not sure ...\n\n> If all of those conditions are met, we can safely use the tree of the\n> original merge commit as the resulting tree of this merge that is being\n> attempted at the time.\n\n...if the \"at least one\" in the last condition is a safe and\nsensible loosening; if it introduces a mismerge by ignoring bases\nthat are different from the original, then it is a bit too bold to\ndeclare that we can safely use the tree of the original.\n\nWhat was the motivating usecase behind this new feature? Was it\nmore about reusing the structural merge conflict resolution, or\nabout the textual merge conflict resolution? For the latter, after\ndoing the usual three-way file-level merge and seeing a conflicted\ntextual merge, requiring the match of the blob objects for only these\nconflicted paths and taking the previous merge result may give you a\nsafe way to raising the chance to find more reusable merges.\n\n> +static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)\n> +{\n> +\tstruct commit_list *i;\n> +\tfor (i = bases; i; i = i->next)\n\nUsing 'i' for a pointer looks novel. Don't.\n\n> +static int find_oid(const struct object_id *oid,\n> +\t\tvoid *data)\n\nThe result of unfolding these two lines would not be overly long, I suspect?\n\n> +{\n> +\tstruct oid_array *other_list = (struct oid_array *) data;\n> +\tint pos = oid_array_lookup(other_list, oid);\n> +\treturn pos >= 0 ? 1 : 0;\n\nThat's an unusual way to say \n\n\treturn pos >= 0;\n\nor even\n\n\treturn 0 <= oid_array_lookup(other_list, oid);\n\nwithout otherwise unused variable.\n\n> +static int run_tms_merge(struct tms_options *options)\n> +{\n> +\tstruct commit *head, *merge_head, *tip;\n> +\tstruct commit_list *i;\n> +\n> +\thead = lookup_commit_reference_by_name(\"HEAD\");\n> +\tmerge_head = lookup_commit_reference_by_name(options->merge_head);\n> +\ttip = lookup_commit_reference_by_name(options->tip);\n> +\n> +\tif (!(head && merge_head && tip)) {\n> +\t\treturn 2;\n> +\t}\n\nUnnecessary {} around a single statement block.\n\n> +\tif (commit_list_count(tip->parents) != 2)\n> +\t\treturn 2;\n> +\n> +\tfor (i = tip->parents; i; i = i->next)\n> +\t\tparse_commit(i->item);\n> +\tif (!oideq(get_commit_tree_oid(head),\n> +\t\t get_commit_tree_oid(tip->parents->item)))\n> +\t\treturn 2;\n> +\tif (!oideq(get_commit_tree_oid(merge_head),\n> +\t\t get_commit_tree_oid(tip->parents->next->item)))\n> +\t\treturn 2;\n> +\n> +\tif (!base_match(tip, head, merge_head))\n> +\t\treturn 2;\n> +\n> +\tif (restore(tip))\n> +\t\treturn 2;\n\nI somehow thought that reverting the trashed working tree and the\nindex to their original state was not the responsibility for a merge\nstrategy but for the caller? Shouldn't this restoration be on the\ncaller's side?\n\nOh, has this code even touched anything in the working tree and the\nindex at this point? It looks more like everything we did above in\norder to punt by returning 2 was to see if the condition for us to\nreuse the resulting tree holds and nothing else.\n\nAh, \"restore()\" is misnamed, perhaps? I thought it was about \"oh,\nwe made a mess and need to go back to the state that was given to us\nbefore failing\", but is this the real \"ok, the condition holds and\nwe can just reuse the tree from the previous merge\"? Then it makes\nsense for the code to attempt to check out that tree and return 2\nwhen it fails. Only the function name was misleading.\n\n> +\tif (!opts->strategy || !strcmp(opts->strategy, \"tms\")) {\n> +\t\trollback_lock_file(&lock);\n> +\t\tret = try_tms_merge(opts, commit, to_merge->item);\n> +\t\tif (ret) {\n> +\t\t\tdiscard_index(r->index);\n> +\t\t\tif (repo_read_index(r) < 0) {\n> +\t\t\t\tret = error(_(\"could not read index\"));\n> +\t\t\t\tgoto leave_merge;\n> +\t\t\t}\n> +\t\t\tgoto ran_merge;\n> +\t\t}\n> +\t\t// regain lock to go into recursive\n\nNo // comments here.\n\n> +\t\tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n"},{"id":"473000","messageId":"CAOc6etaCz=JWPnk2Jv8KYG02xBN+jZG1EfbnwtKnq_A0UBUpEg@mail.gmail.com","threadId":"59336","inReplyTo":"CAOc6etb9pCXx9S5jxk8Yex++_iai21THU74qdXmOm1XxjHx8Lw@mail.gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2023-03-04T11:47:04Z","receivedAt":"2023-03-04T11:47:23Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sat, Mar 4, 2023 at 12:45 PM Edmundo Carmona Antoranz\n wrote:\n>\n> Usercase can be at the moment trying to rebase (with merges) on top of\n> an exact base copy. In cases like this, git just crashes on merge\n> commits. An easy example:\n\nI should have said _crashes on merge commits where there was a conflict_.\n"},{"id":"473001","messageId":"CAOc6etb9pCXx9S5jxk8Yex++_iai21THU74qdXmOm1XxjHx8Lw@mail.gmail.com","threadId":"59336","inReplyTo":"xmqqcz5phjgz.fsf@gitster.g","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2023-03-04T11:45:14Z","receivedAt":"2023-03-04T11:47:23Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Fri, Mar 3, 2023 at 5:45 PM Junio C Hamano wrote:\n> > With this patch, we leverage the original merge commit to handle the most\n> > obvious case:\n> > - HEAD tree has to match the tree of the first parent of the original merge\n> > commit.\n> > - MERGE_HEAD tree has to match the tree of the second parent of the original\n> > merge commit.\n> > - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n> > a tree in the merge bases of the parent commits of the original merge\n> > commit.\n>\n> The first two conditions look a bit too restrictive for the purpose\n> of reusing previous conflict resolution, while I am not sure ...\n>\n> > If all of those conditions are met, we can safely use the tree of the\n> > original merge commit as the resulting tree of this merge that is being\n> > attempted at the time.\n>\n> ...if the \"at least one\" in the last condition is a safe and\n> sensible loosening; if it introduces a mismerge by ignoring bases\n> that are different from the original, then it is a bit too bold to\n> declare that we can safely use the tree of the original.\n>\nI think the conditions hold _but_ I will think it through or perhaps\ncreate a few scenarios that we could talk about. Will come back to it\nin a few days.\n\nI agree that the current restrictions make it too narrow. Very\nrestricted scenarios would match at the moment. I will start working\non making this a little bit more accepting to widen the scope.\n\n> What was the motivating usecase behind this new feature? Was it\n> more about reusing the structural merge conflict resolution, or\n> about the textual merge conflict resolution? For the latter, after\n> doing the usual three-way file-level merge and seeing a conflicted\n> textual merge, requiring the match of the blob objects for only these\n> conflicted paths and taking the previous merge result may give you a\n> safe way to raising the chance to find more reusable merges.\n\nUsercase can be at the moment trying to rebase (with merges) on top of\nan exact base copy. In cases like this, git just crashes on merge\ncommits. An easy example:\n\ngit checkout v2.38.0\ngit commit --amend --no-edit\ngit rebase --rebase-merges --onto HEAD v2.38.0 v2.39.0\n\n>\n> > +static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)\n> > +{\n> > + struct commit_list *i;\n> > + for (i = bases; i; i = i->next)\n>\n> Using 'i' for a pointer looks novel. Don't.\n>\n\nThanks for the comments on code. At least it doesn't sound like I\nmessed up big time.... so far.\n\n>\n> I somehow thought that reverting the trashed working tree and the\n> index to their original state was not the responsibility for a merge\n> strategy but for the caller? Shouldn't this restoration be on the\n> caller's side?\n>\n> Oh, has this code even touched anything in the working tree and the\n> index at this point? It looks more like everything we did above in\n> order to punt by returning 2 was to see if the condition for us to\n> reuse the resulting tree holds and nothing else.\n>\n> Ah, \"restore()\" is misnamed, perhaps? I thought it was about \"oh,\n> we made a mess and need to go back to the state that was given to us\n> before failing\", but is this the real \"ok, the condition holds and\n> we can just reuse the tree from the previous merge\"? Then it makes\n> sense for the code to attempt to check out that tree and return 2\n> when it fails. Only the function name was misleading.\n>\n\ncalling _git restore_ to do that hence _restore_ :-) But it's ok. I\ncan give it a better name.\n"},{"id":"473008","messageId":"CABPp-BGOtsjgfN5f=dSb0ZSEx8nzFs6SKrUm=2TtPPH5cKa4cA@mail.gmail.com","threadId":"59336","inReplyTo":"20230303145311.513960-1-eantoranz@gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-03-04T20:31:50Z","receivedAt":"2023-03-04T20:32:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Edmundo,\n\nOn Fri, Mar 3, 2023 at 7:43 AM Edmundo Carmona Antoranz\n wrote:\n>\n> When rebasing merge commits and dealing with conflicts, having the\n> original merge commit as a reference can help us avoid some of\n> them.\n>\n>\n> With this patch, we leverage the original merge commit to handle the most\n> obvious case:\n> - HEAD tree has to match the tree of the first parent of the original merge\n> commit.\n> - MERGE_HEAD tree has to match the tree of the second parent of the original\n> merge commit.\n> - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n> a tree in the merge bases of the parent commits of the original merge\n> commit.\n>\n> If all of those conditions are met, we can safely use the tree of the\n> original merge commit as the resulting tree of this merge that is being\n> attempted at the time.\n\nThe conditions are quite specific, making one wonder what you are\ntrying to do, and yet also leave an obvious open hole that seems to be\ninviting bugs.\n\nHaving read the rest of this thread, I notice you pointed out to Junio\nthat you want to amend a commit in the history of the merge,\nsuggesting you are just modifying the commit message (or maybe\nauthor/committer info). More generally, I _think_ your usecase and\njustification for this patch could be worded something like:\n\n\"\"\"\nWe often rebase with `--rebase-merges`, `--interactive`, and\n`--keep-base` (or equivalent command line flags) and only modify\ncommit metadata during the rebase. Since we do not modify any files,\nwe would like the rebase to proceed without conflicts. However, since\n--rebase-merges currently does not rebase merges but recreates them\nfrom scratch (ignoring everything but the commit metadata of the old\nmerge), it forces users to redo conflict resolution. Since the trees\nof all relevant commits have not changed, this conflict resolution\nfeels unnecessary. In this patch we do not try to solve the general\nproblem of rebasing merges, but instead introduce a narrow hack\nspecific to this particular scenario: we check that the trees of all\nrelevant commits involved in the new merge are the same as for the old\nmerge, and when that holds, use the tree from the original merge as\nthe merge resolution. In more detail:\n\"\"\"\n\nwhich would be followed immediately by your text after \"handle the\nmost obvious case:\"\n\nAm I reading your motivation correctly?\n\nAlso, as Junio highlighted, I don't believe it's safe to only require\nthat one tree of the merge bases match. You should require having\nboth the same number of merge bases and that the set of trees for the\nmerge bases of both the old and new merge commits exactly match.\n\nAs a high level review, I personally tend to dislike piecemeal\nsolutions that only work for very specific cases. However, others on\nthe list may decide to include it, so long as it doesn't actively hurt\nother cases. One request I would make, if it is to be included, is\nthat we design it such that we can easily jettison this code (& any\ndocumentation it needs) later when we gain a more general solution for\nrebasing merges.\n\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/merge-tms.c | 148 ++++++++++++++++++++++++++++++++++++++++++++\n> git.c | 1 +\n> sequencer.c | 36 ++++++++++-\n> 6 files changed, 187 insertions(+), 1 deletion(-)\n\nI understand waiting to make documentation updates while your proposal\nis just an RFC, but I think tests might help showcase how the strategy\nis meant to be used and verify whether the behavior is sane when your\nnew strategy doesn't apply.\n\n> create mode 100644 builtin/merge-tms.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index e875c59054..8b534f98e6 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -103,6 +103,7 @@\n> /git-merge-recursive\n> /git-merge-resolve\n> /git-merge-subtree\n> +/git-merge-tms\n> /git-mergetool\n> /git-mergetool--lib\n> /git-mktag\n> diff --git a/Makefile b/Makefile\n> index 50ee51fde3..10a3167c50 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1264,6 +1264,7 @@ BUILTIN_OBJS += builtin/merge-file.o\n> BUILTIN_OBJS += builtin/merge-index.o\n> BUILTIN_OBJS += builtin/merge-ours.o\n> BUILTIN_OBJS += builtin/merge-recursive.o\n> +BUILTIN_OBJS += builtin/merge-tms.o\n> BUILTIN_OBJS += builtin/merge-tree.o\n> BUILTIN_OBJS += builtin/merge.o\n> BUILTIN_OBJS += builtin/mktag.o\n> diff --git a/builtin.h b/builtin.h\n> index 46cc789789..94dcb73f85 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -180,6 +180,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix);\n> int cmd_merge_ours(int argc, const char **argv, const char *prefix);\n> int cmd_merge_file(int argc, const char **argv, const char *prefix);\n> int cmd_merge_recursive(int argc, const char **argv, const char *prefix);\n> +int cmd_merge_tms(int argc, const char **argv, const char *prefix);\n> int cmd_merge_tree(int argc, const char **argv, const char *prefix);\n> int cmd_mktag(int argc, const char **argv, const char *prefix);\n> int cmd_mktree(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/merge-tms.c b/builtin/merge-tms.c\n> new file mode 100644\n> index 0000000000..37a2427757\n> --- /dev/null\n> +++ b/builtin/merge-tms.c\n> @@ -0,0 +1,148 @@\n> +/*\n> + * Copyright (c) 2023 Edmundo Carmona Antoranz\n\nI dislike having these in our files. Despite the file being modified\nlater, they are virtually never updated to list the new authors and\nnew years. Needing to always update these lines of code would\nobviously be onerous, and is why we don't do that, but that just means\nthese lines always eventually become inaccurate and perhaps even\nwildly misleading. People can look at log/blame/etc. to find out the\n_correct_ information, so what purpose does a line like this serve?\n\n(I'm not a lawyer or anything close; I previously suggested ripping\nthese lines out of our codebase but people pointed out we can't rip\nthem out once put in. But it'd be nice to avoid spreading the\nproblem. Or maybe there are still lawyer-ish reasons for including\nthese that I'm not aware of?)\n\n> + * Released under the terms of GPL2\n\nThat's automatic for anything in the project that doesn't explicitly\nstate otherwise; I'd rather not have this included for every file.\n\nTangential question for the list: If folks do want their contributions\nto also be available under an alternate license (e.g. as done for\nsha1dc or as done with\nhttps://github.com/libgit2/libgit2/blob/main/git.git-authors), is\nthere a scheme for doing so? Incidentally, my employer told me that\nany new entire files or subsystems that I contributed should be\nlicensed under MIT (like sha1dc is). I didn't want to have to deal\nwith that headache, but luckily I was copying a bunch of\nmerge-recursive.[ch] code into merge-ort.[ch], so I just did that in\nthe initial commits and side-stepped the whole question. :-) (But\nreally, if anyone wants any of the code I've contributed to Git under\nMIT during my time at Palantir, that's what my employer\nwanted/intended so you've got permission to do so.)\n\n...and with these two first lines out of the way, I can stop\ncommenting on things I have a tenuous grasp on (copyright & licenses)\nto things that I actually know something about...\n\n> + *\n> + * Tipped merge strategy.... a.k.a. fortune-teller merge strategy\n> + *\n> + * In cases like rebases, merge commits offer us the advantage of knowing\n> + * _before hand_ what the previous result of the _original_ branches\n> + * involved was.\n> + *\n> + * This merge strategy tries to leverage this knowledge so that we can\n> + * avoid at least the most obvious conflicts that have been solved in the\n> + * original merge commit.\n\nExcept it doesn't solve \"the most obvious conflicts\", it either solves\nall of the conflicts or none of them. Perhaps this wording can be fixed up?\n\n> + *\n> + * In the current state, the strategy works based on exact matches of the trees\n> + * involved:\n> + * - HEAD tree has to match the tree of the first parent of the original merge\n> + * commit.\n> + * - MERGE_HEAD tree has to match the tree of the second parent of the original\n> + * merge commit.\n> + * - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match\n> + * a tree in the merge bases of the parent commits of the original merge\n> + * commit.\n> + * If all of those conditions are met, we can safely use the tree of the\n> + * original merge commit as the resulting tree of this merge that is being\n> + * attempted at the time.\n> + */\n> +\n> +#include \"builtin.h\"\n> +#include \"commit-reach.h\"\n> +#include \"oid-array.h\"\n> +#include \"parse-options.h\"\n> +#include \"run-command.h\"\n> +\n> +\n> +struct tms_options {\n> + const char *tip;\n> + const char *merge_head;\n> +} tms_options;\n> +\n> +static int restore(struct commit *commit)\n> +{\n> + struct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> + strvec_push(&cmd.args, \"restore\");\n> + strvec_push(&cmd.args, \"--worktree\");\n> + strvec_push(&cmd.args, \"--stage\");\n> + strvec_pushf(&cmd.args, \"--source=%s\",\n> + oid_to_hex(&commit->object.oid));\n> + strvec_push(&cmd.args, \"--\");\n> + strvec_push(&cmd.args, \".\");\n> + cmd.git_cmd = 1;\n> + return run_command(&cmd);\n\nWe fork subprocesses a lot in git, but it was a horrible mistake. It\nhurts performance, it *really* hurts on Windows (or so I hear), it\nhurts debuggability/maintainability in general, and we should be\nremoving this from our codebase rather than adding more. As someone\nwho has put time into slowly eradicating this from our codebase,\nplease make library functions you can call instead. (Actually, first\nlook and see if there are relevant library functions.\nRebase/sequencer probably already needed this and already wrote such a\nfunction.)\n\n> +}\n> +\n> +static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)\n> +{\n> + struct commit_list *i;\n> +\n> + for (i = bases; i; i = i->next)\n> + oid_array_append(oids, get_commit_tree_oid(i->item));\n> +}\n> +\n> +static int find_oid(const struct object_id *oid,\n> + void *data)\n> +{\n> + struct oid_array *other_list = (struct oid_array *) data;\n> + int pos = oid_array_lookup(other_list, oid);\n> + return pos >= 0 ? 1 : 0;\n> +}\n> +\n> +static int base_match(struct commit *rebase_head,\n> + struct commit *head,\n> + struct commit *merge_head)\n> +{\n> + struct commit_list *bases_current, *bases_tip;\n> + struct oid_array trees_current = OID_ARRAY_INIT;\n> + struct oid_array trees_tip = OID_ARRAY_INIT;\n> + int oid_match;\n> +\n> + bases_current = get_merge_bases(head, merge_head);\n> + bases_tip = get_merge_bases(rebase_head->parents->item,\n> + rebase_head->parents->next->item);\n> + load_tree_oids(&trees_current, bases_current);\n> + load_tree_oids(&trees_tip, bases_tip);\n> +\n> + oid_match = oid_array_for_each(&trees_current, find_oid, &trees_tip);\n> +\n> + oid_array_clear(&trees_current);\n> + oid_array_clear(&trees_tip);\n> +\n> + return oid_match;\n> +}\n> +\n> +static int run_tms_merge(struct tms_options *options)\n> +{\n> + struct commit *head, *merge_head, *tip;\n> + struct commit_list *i;\n> +\n> + head = lookup_commit_reference_by_name(\"HEAD\");\n> + merge_head = lookup_commit_reference_by_name(options->merge_head);\n> + tip = lookup_commit_reference_by_name(options->tip);\n> +\n> + if (!(head && merge_head && tip)) {\n> + return 2;\n> + }\n> + if (commit_list_count(tip->parents) != 2)\n> + return 2;\n> +\n> + for (i = tip->parents; i; i = i->next)\n> + parse_commit(i->item);\n> + if (!oideq(get_commit_tree_oid(head),\n> + get_commit_tree_oid(tip->parents->item)))\n> + return 2;\n> + if (!oideq(get_commit_tree_oid(merge_head),\n> + get_commit_tree_oid(tip->parents->next->item)))\n> + return 2;\n> +\n> + if (!base_match(tip, head, merge_head))\n> + return 2;\n> +\n> + if (restore(tip))\n> + return 2;\n\nSo six different cases where this merge strategy can currently bail\nand do nothing; I'll discuss this more below.\n\n> +\n> + return 0;\n> +}\n> +\n> +int cmd_merge_tms(int argc, const char **argv, const char *prefix)\n> +{\n> +\n> + struct option mt_options[] = {\n> + OPT_STRING(0, \"tip\", &tms_options.tip,\n> + N_(\"tip-merge-commit\"),\n> + N_(\"merge commit being rebased used as a tip for conflict resolution.\")),\n> + OPT_END()\n> + };\n> + argc = parse_options(argc, argv, NULL, mt_options,\n> + NULL, 0);\n> +\n> + if (argc != 1)\n> + return 2;\n> + tms_options.merge_head = argv[0];\n> +\n> + if (!tms_options.tip)\n> + return 2;\n\nBy creating a builtin named \"merge-\", you implicitly create a\n`--strategy ` option for git-merge, not just something for\ngit-rebase. If we publish this new merge strategy in a released\nversion of Git, we'll have to support it for ~forever. For something\nthat is meant as a short-term hack, I'd rather avoid that. Further,\nyour merge strategy does not accept the normal arguments that a merge\nstrategy accepts, and will bail with an exit status of 2 if a \"--tip\"\nis not specified. Luckily, my comments elsewhere to make library\ncalls instead of forking subprocesses would also solve these issues\nhere.\n\nI will also point out that this main function adds two more cases\nwhere we can bail and do nothing, returning a status of 2, so now\nwe're up to eight cases. Again, I'll discuss these more below.\n\n> +\n> + return run_tms_merge(&tms_options);\n> +}\n> diff --git a/git.c b/git.c\n> index 96b0a2837d..2e843731f1 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -544,6 +544,7 @@ static struct cmd_struct commands[] = {\n> { \"merge-recursive-ours\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n> { \"merge-recursive-theirs\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n> { \"merge-subtree\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n> + { \"merge-tms\", cmd_merge_tms, RUN_SETUP },\n> { \"merge-tree\", cmd_merge_tree, RUN_SETUP },\n> { \"mktag\", cmd_mktag, RUN_SETUP },\n> { \"mktree\", cmd_mktree, RUN_SETUP },\n> diff --git a/sequencer.c b/sequencer.c\n> index 65a34f9676..559169814b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3833,6 +3833,21 @@ static int do_reset(struct repository *r,\n> return ret;\n> }\n>\n> +static int try_tms_merge(struct replay_opts *opts,\n> + struct commit *rebase_head,\n> + struct commit *merge_commit)\n> +{\n> + struct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> + strvec_push(&cmd.args, \"merge-tms\");\n> + strvec_push(&cmd.args, \"--tip\");\n> + strvec_pushf(&cmd.args, \"%s\", oid_to_hex(&rebase_head->object.oid));\n> + strvec_pushf(&cmd.args, \"%s\", oid_to_hex(&merge_commit->object.oid));\n> +\n> + cmd.git_cmd = 1;\n> + return run_command(&cmd) ? 0 : 1;\n\nAgain, let's please not fork subprocesses for builtin code.\n\n> +}\n> +\n> static int do_merge(struct repository *r,\n> struct commit *commit,\n> const char *arg, int arg_len,\n> @@ -3846,7 +3861,8 @@ static int do_merge(struct repository *r,\n> const char *strategy = !opts->xopts_nr &&\n> (!opts->strategy ||\n> !strcmp(opts->strategy, \"recursive\") ||\n> - !strcmp(opts->strategy, \"ort\")) ?\n> + !strcmp(opts->strategy, \"ort\") ||\n> + !strcmp(opts->strategy, \"tms\")) ?\n\nSo folks trigger this by passing `--strategy tms` to rebase.\nTypically, all the --strategy options to rebase are also ones that\nmerge accepts. So if we trigger via that method, we may need to\nexpose the strategy to merge as well...or update the documentation in\nvarious places that talks about merge strategies to be more specific\nabout which ones apply where.\n\nAlso, what if the merge strategy is inappropriate for the case in\nquestion? \"ort\" and \"recursive\" are appropriate for all non-octopus\nmerges, and the code only gets to this point in sequencer if we have a\nnon-octopus merge to do. But your strategy is inappropriate other\nthan in very specific circumstances that haven't yet been checked.\nWhat will the code do if we're under one of the many cases where \"tms\"\nisn't applicable? More on this question below.\n\n> NULL : opts->strategy;\n> struct merge_options o;\n> int merge_arg_len, oneline_offset, can_fast_forward, ret, k;\n> @@ -4086,6 +4102,23 @@ static int do_merge(struct repository *r,\n> o.branch2 = ref_name.buf;\n> o.buffer_output = 2;\n>\n> + if (!opts->strategy || !strcmp(opts->strategy, \"tms\")) {\n> + rollback_lock_file(&lock);\n> + ret = try_tms_merge(opts, commit, to_merge->item);\n> + if (ret) {\n> + discard_index(r->index);\n> + if (repo_read_index(r) < 0) {\n> + ret = error(_(\"could not read index\"));\n> + goto leave_merge;\n> + }\n> + goto ran_merge;\n\nWe \"goto ran_merge\", which could be very misleading. Under the 8\nconditions given, we did not run a merge; we bailed early saying the\nmerge strategy was inappropriate for the conditions at hand. For\nthose 8 cases, ret will be 2 and we will jump to...\n\n> + }\n> + // regain lock to go into recursive\n> + if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n> + ret = -1;\n> + goto leave_merge;\n> + }\n> + }\n> if (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n> /*\n> * TODO: Should use merge_incore_recursive() and\n> @@ -4100,6 +4133,7 @@ static int do_merge(struct repository *r,\n> ret = merge_recursive(&o, head_commit, merge_commit, bases,\n> &i);\n> }\n> +ran_merge:\n> if (ret <= 0)\n> fputs(o.obuf.buf, stdout);\n> strbuf_release(&o.obuf);\n\n...here. The code following this includes a check for ret < 0, and then runs:\n\n /*\n * The return value of merge_recursive() is 1 on clean, and 0 on\n * unclean merge.\n *\n * Let's reverse that, so that do_merge() returns 0 upon success and\n * 1 upon failed merge (keeping the return value -1 for the cases where\n * we will want to reschedule the `merge` command).\n */\n ret = !ret;\n\n if (r->index->cache_changed &&\n write_locked_index(r->index, &lock, COMMIT_LOCK)) {\n ret = error(_(\"merge: Unable to write new index file\"));\n goto leave_merge;\n }\n\nSo, if I'm reading correctly (please double check me), since your\nmerge strategy made no changes to the index or working tree, and\nreturned a status of 2, and since !2 == !1 == 0, we'll treat this the\nsame as a successful merge and commit the \"results\", i.e. the tree of\nthe first parent. Doesn't this tipped merge strategy thus behave the\nsame as a `--strategy ours` merge when its preconditions are not\nsatisfied? If it does, that would be horrifying.\n"},{"id":"473009","messageId":"CABPp-BFCNjMsxAcLOxr_9Rnu3n_KL5RMCfD_m7ytS+b_gbd3Xw@mail.gmail.com","threadId":"59336","inReplyTo":"CAOc6etaCz=JWPnk2Jv8KYG02xBN+jZG1EfbnwtKnq_A0UBUpEg@mail.gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-03-04T20:36:45Z","receivedAt":"2023-03-04T20:37:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 4, 2023 at 3:57 AM Edmundo Carmona Antoranz\n wrote:\n>\n> On Sat, Mar 4, 2023 at 12:45 PM Edmundo Carmona Antoranz\n> wrote:\n> >\n> > Usercase can be at the moment trying to rebase (with merges) on top of\n> > an exact base copy. In cases like this, git just crashes on merge\n> > commits. An easy example:\n>\n> I should have said _crashes on merge commits where there was a conflict_.\n\nCrash tends to mean the program has done something not allowed by the\noperating system. I don't think that's what's happening here. Do you\nmean Git stops with conflicts?\n"},{"id":"473027","messageId":"CAOc6etZeWpxLwkuJM-auhc0N-zu92zYtrMnSjHMGyxvvs2+vEw@mail.gmail.com","threadId":"59336","inReplyTo":"CABPp-BFCNjMsxAcLOxr_9Rnu3n_KL5RMCfD_m7ytS+b_gbd3Xw@mail.gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2023-03-05T13:40:39Z","receivedAt":"2023-03-05T13:40:54Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sat, Mar 4, 2023 at 9:36 PM Elijah Newren wrote:\n>\n> On Sat, Mar 4, 2023 at 3:57 AM Edmundo Carmona Antoranz\n> wrote:\n> >\n> > On Sat, Mar 4, 2023 at 12:45 PM Edmundo Carmona Antoranz\n> > wrote:\n> > >\n> > > Usercase can be at the moment trying to rebase (with merges) on top of\n> > > an exact base copy. In cases like this, git just crashes on merge\n> > > commits. An easy example:\n> >\n> > I should have said _crashes on merge commits where there was a conflict_.\n>\n> Crash tends to mean the program has done something not allowed by the\n> operating system. I don't think that's what's happening here. Do you\n> mean Git stops with conflicts?\n\nOk, sorry for the sloppy wording. Yes, it stops with conflicts.\n"},{"id":"473028","messageId":"CAOc6etawNSEjwt9QsJE90ok5pJjf4xOJ6ZaFr9HkHu3+Sw89+A@mail.gmail.com","threadId":"59336","inReplyTo":"CABPp-BGOtsjgfN5f=dSb0ZSEx8nzFs6SKrUm=2TtPPH5cKa4cA@mail.gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2023-03-05T14:00:39Z","receivedAt":"2023-03-05T14:02:51Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Hey, Elijah!\n\nThank you for all of that feedback. Really good Will need to go\nthrough it which will take me some time to digest but I did want to go\nover one of the subjects.\n\n\n> So, if I'm reading correctly (please double check me), since your\n> merge strategy made no changes to the index or working tree, and\n> returned a status of 2, and since !2 == !1 == 0, we'll treat this the\n> same as a successful merge and commit the \"results\", i.e. the tree of\n> the first parent. Doesn't this tipped merge strategy thus behave the\n> same as a `--strategy ours` merge when its preconditions are not\n> satisfied? If it does, that would be horrifying.\n\nI think that is not correct. The possible values that come out of\ntry_tms_merge are 0 if nothing happened and 1 if the merge was\nsuccessful and we changed the index and the working tree. Then I think\nI wrote this correctly following the call:\n\nif (ret) {\n discard_index(r->index);\n if (repo_read_index(r) < 0) {\n ret = error(_(\"could not read index\"));\n goto leave_merge;\n }\n goto ran_merge;\n}\n// regain lock to go into recursive\nif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n ret = -1;\n goto leave_merge;\n}\n\nActually, if we will be switching to using a library, then this won't\nbe that important because we might be able to pull it off without\nhaving to release the lock given that we would be running in-process,\nbut I wanted to clear up what the intended flow is there, just in\ncase.\n\nOk.... more questions or comments will be coming in the following\ndays. And thank you, again.\n\nBR!\n"},{"id":"473064","messageId":"xmqq8rg9bxal.fsf@gitster.g","threadId":"59336","inReplyTo":"CABPp-BGOtsjgfN5f=dSb0ZSEx8nzFs6SKrUm=2TtPPH5cKa4cA@mail.gmail.com","subject":"Re: [RFC PATCH] sequencer - tipped merge strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T17:32:34Z","receivedAt":"2023-03-06T17:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Having read the rest of this thread, I notice you pointed out to Junio\n> that you want to amend a commit in the history of the merge,\n> suggesting you are just modifying the commit message (or maybe\n> author/committer info). More generally, I _think_ your usecase and\n> justification for this patch could be worded something like:\n>\n> \"\"\"\n> We often rebase with `--rebase-merges`, `--interactive`, and\n> `--keep-base` (or equivalent command line flags) and only modify\n> commit metadata during the rebase. Since we do not modify any files,\n> we would like the rebase to proceed without conflicts.\n\nIt makes very much sense to focus on this narrow but useful use\ncase, and I view it a very natural extension to already existing \"if\nwe just pick without any user interaction a commit on top of its\ncurrent base, the we do not do anything, fast-forward and just\npretend we picked it\". IOW, shouldn't it something the sequencer\nmachinery should be able to do natively without forcing the user to\nspecify a new merge strategy?\n\n"}]}