{"thread":{"id":"57191","subject":"[RFC PATCH 1/2] merge-ort: add new merge-tree-ort command","startedAt":"2022-01-05T16:33:59Z","lastAt":"2022-02-22T13:08:14Z","messageCount":28,"participants":["Christian Couder","Elijah Newren","Johannes Schindelin","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"445531","messageId":"20220105163324.73369-2-chriscool@tuxfamily.org","threadId":"57191","inReplyTo":"20220105163324.73369-1-chriscool@tuxfamily.org","subject":"[RFC PATCH 1/2] merge-ort: add new merge-tree-ort command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-01-05T16:33:23Z","receivedAt":"2022-01-05T16:33:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This new command is similar as merge-tree, but uses\nmerge-ort code and features, especially\nmerge_incore_nonrecursive(), instead of those from\nresursive merge, to perform the merge.\n\nThe output from this new command is very different from\nmerge-tree's custom output, as we are only using code and\nfeatures from diff.{h,c}, especially diff_tree_oid(). This\nshould make it easy to customize and standardize the output\nusing regular diff options in the future.\n\nThis command will be extended to support new features\nwith the long-term goal of enabling merges and rebases\nwithout a worktree.\n\nSigned-off-by: Christian Couder \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge-tree-ort.c | 93 ++++++++++++++++++++++++++++++++++++++++\n git.c | 1 +\n 5 files changed, 97 insertions(+)\n create mode 100644 builtin/merge-tree-ort.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 054249b20a..2dfcb1a589 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -98,6 +98,7 @@\n /git-merge-index\n /git-merge-file\n /git-merge-tree\n+/git-merge-tree-ort\n /git-merge-octopus\n /git-merge-one-file\n /git-merge-ours\ndiff --git a/Makefile b/Makefile\nindex 75ed168adb..915e500b06 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1124,6 +1124,7 @@ 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-tree.o\n+BUILTIN_OBJS += builtin/merge-tree-ort.o\n BUILTIN_OBJS += builtin/merge.o\n BUILTIN_OBJS += builtin/mktag.o\n BUILTIN_OBJS += builtin/mktree.o\ndiff --git a/builtin.h b/builtin.h\nindex 8a58743ed6..c68f46b118 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -182,6 +182,7 @@ 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_tree(int argc, const char **argv, const char *prefix);\n+int cmd_merge_tree_ort(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 int cmd_multi_pack_index(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/merge-tree-ort.c b/builtin/merge-tree-ort.c\nnew file mode 100644\nindex 0000000000..1c8ecd16ec\n--- /dev/null\n+++ b/builtin/merge-tree-ort.c\n@@ -0,0 +1,93 @@\n+#include \"builtin.h\"\n+#include \"merge-ort.h\"\n+#include \"diff.h\"\n+\n+static const char merge_tree_ort_usage[] = \"git merge-tree-ort \";\n+\n+static void show_result(struct tree *base_tree,\n+\t\t\tstruct tree *head_tree,\n+\t\t\tstruct tree *merge_tree,\n+\t\t\tstruct merge_result *result)\n+{\n+\tconst struct object_id *base_oid = &(base_tree->object.oid);\n+\tconst struct object_id *head_oid = &(head_tree->object.oid);\n+\tconst struct object_id *merge_oid = &(merge_tree->object.oid);\n+\tconst struct object_id *result_oid = &(result->tree->object.oid);\n+\tstruct diff_options opts;\n+\n+\trepo_diff_setup(the_repository, &opts);\n+\topts.stat_width = -1; /* use full terminal width */\n+\topts.output_format |= DIFF_FORMAT_RAW | DIFF_FORMAT_PATCH;\n+\topts.detect_rename = DIFF_DETECT_RENAME;\n+\tdiff_setup_done(&opts);\n+\n+\tprintf(\"result tree: %s\\n\", oid_to_hex(result_oid));\n+\tprintf(\"clean: %d\\n\", result->clean);\n+\n+\tprintf(\"diff with branch1:\\n\");\n+\tdiff_tree_oid(head_oid, result_oid, \"\", &opts);\n+\tdiffcore_std(&opts);\n+\tdiff_flush(&opts);\n+\n+\tprintf(\"diff with branch2:\\n\");\n+\tdiff_tree_oid(merge_oid, result_oid, \"\", &opts);\n+\tdiffcore_std(&opts);\n+\tdiff_flush(&opts);\n+\n+\tprintf(\"diff with base:\\n\");\n+\tdiff_tree_oid(base_oid, result_oid, \"\", &opts);\n+\tdiffcore_std(&opts);\n+\tdiff_flush(&opts);\n+}\n+\n+static struct commit *get_commit_by_name_or_die(const char *name)\n+{\n+\tstruct commit *c = lookup_commit_reference_by_name(name);\n+\tif (!c)\n+\t\tdie(_(\"not a valid commit name '%s'\"), name);\n+\treturn c;\n+}\n+\n+static void merge_trees_ort(const char *base_name,\n+\t\t\t const char *branch1,\n+\t\t\t const char *branch2)\n+{\n+\tstruct merge_result result;\n+\tstruct merge_options merge_opt;\n+\n+\tstruct commit *base = get_commit_by_name_or_die(base_name);\n+\tstruct commit *head = get_commit_by_name_or_die(branch1);\n+\tstruct commit *merge = get_commit_by_name_or_die(branch2);\n+\n+\tstruct tree *base_tree = get_commit_tree(base);\n+\tstruct tree *head_tree = get_commit_tree(head);\n+\tstruct tree *merge_tree = get_commit_tree(merge);\n+\n+\tmemset(&result, 0, sizeof(result));\n+\tinit_merge_options(&merge_opt, the_repository);\n+\n+\tmerge_opt.show_rename_progress = 1;\n+\tmerge_opt.branch1 = branch1;\n+\tmerge_opt.branch2 = branch2;\n+\tmerge_opt.ancestor = base_name;\n+\n+\tresult.tree = head_tree;\n+\n+\tmerge_incore_nonrecursive(&merge_opt,\n+\t\t\t\t base_tree,\n+\t\t\t\t result.tree,\n+\t\t\t\t merge_tree,\n+\t\t\t\t &result);\n+\n+\tshow_result(base_tree, head_tree, merge_tree, &result);\n+}\n+\n+int cmd_merge_tree_ort(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc != 4)\n+\t\tusage(merge_tree_ort_usage);\n+\n+\tmerge_trees_ort(argv[1], argv[2], argv[3]);\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex 7edafd8ecf..90b8a4984c 100644\n--- a/git.c\n+++ b/git.c\n@@ -562,6 +562,7 @@ static struct cmd_struct commands[] = {\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-tree\", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },\n+\t{ \"merge-tree-ort\", cmd_merge_tree_ort, RUN_SETUP | NO_PARSEOPT },\n \t{ \"mktag\", cmd_mktag, RUN_SETUP | NO_PARSEOPT },\n \t{ \"mktree\", cmd_mktree, RUN_SETUP },\n \t{ \"multi-pack-index\", cmd_multi_pack_index, RUN_SETUP },\n-- \n2.34.1.433.g7bc349372a.dirty\n\n"},{"id":"445532","messageId":"20220105163324.73369-1-chriscool@tuxfamily.org","threadId":"57191","inReplyTo":null,"subject":"[RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-01-05T16:33:22Z","receivedAt":"2022-01-05T16:34:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"During the 2nd Virtual Git Contributors’ Summit last October, and even\nbefore, the subject of performing server side merges and rebases came\nup, as platforms like GitHub and GitLab would like to support many\nfeatures and data formats that libgit2 doesn't support, like for\nexample SHA256 hashes and partial clone.\n\nIt's hard for them to get rid of libgit2 though, because Git itself\ndoesn't have a good way to support server side merges and rebases,\nwhile libgit2 has ways to perform them. Without server side merges and\nrebases, those platforms would have to launch some kind of checkout,\nwhich can be very expensive, before any merge or rebase.\n\nThe latest discussions on this topic following the 2nd Virtual\nSummit[1] ended with some proposals around a `git merge-tree` on\nsteroids that could be a good solution to this issue.\n\nThe current `git merge-tree` command though seems to have a number of\nissues, especially:\n\n - it's too much related to the old merge recursive strategy which is\n not the default anymore since v2.34.0 and is likely to be\n deprecated over time,\n\n - it seems to output things in its own special format, which is not\n easy to customize, and which needs special code and logic to parse\n\nTo move forward on this, this small RFC patch series introduces a new\n`git merge-tree-ort` command with the following design:\n\n - it uses merge-ort's API as is to perform the merge\n \n - it gets back a tree oid and a cleanliness status from merge-ort's\n API and prints them out first\n\n - it uses diff's API as is to output changed paths and code\n\n - the diff API, actually diff_tree_oid() is called 3 times: once for\n the diff versus branch1 (\"ours\"), once for the diff versus branch2\n (\"theirs\"), and once for the diff versus the base.\n\nTherefore:\n\n - its code is very simple and very easy to extend and customize, for\n example by passing diff or merge-ort options that the code would\n just pass on to the merge-ort and diff APIs respectively\n\n - its output can easily be parsed using simple code and existing\n diff parsers\n\nThis of course means that merge-tree-ort's output is not backward\ncompatible with merge-tree's output, but it doesn't seem that there is\nmuch value in keeping the same output anyway. On the contrary\nmerge-tree's output is likely to hold us back already.\n\nThe first patch in the series adds the new command without any test\nand documentation.\n\nThe second patch in the series adds a few tests that let us see how\nthe command's output looks like in different very simple cases.\n\nOf course if this approach is considered valuable, I plan to add some\ndocumentation, more tests and very likely a number of options before\nsubmitting the next iteration.\n\nI am not sure that it's worth showing the 3 diffs (versus branch1,\nbranch2 and base) by default. Maybe by default no diff at all should\nbe shown and the command should have --branch1 (or --ours), --branch2\n(or --theirs) and --base options to ask for such output, but for an\nRFC patch I thought it would be better to output the 3 diffs so that\npeople get a better idea of the approach this patch series is taking.\n\n[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211147490.56@tvgsbejvaqbjf.bet/\n\n\nChristian Couder (2):\n merge-ort: add new merge-tree-ort command\n merge-ort: add t/t4310-merge-tree-ort.sh\n\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge-tree-ort.c | 93 ++++++++++++++++++++++\n git.c | 1 +\n t/t4310-merge-tree-ort.sh | 162 ++++++++++++++++++++++++++++++++++++++\n 6 files changed, 259 insertions(+)\n create mode 100644 builtin/merge-tree-ort.c\n create mode 100755 t/t4310-merge-tree-ort.sh\n\n-- \n2.34.1.433.g7bc349372a.dirty\n\n"},{"id":"445533","messageId":"20220105163324.73369-3-chriscool@tuxfamily.org","threadId":"57191","inReplyTo":"20220105163324.73369-1-chriscool@tuxfamily.org","subject":"[RFC PATCH 2/2] merge-ort: add t/t4310-merge-tree-ort.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-01-05T16:33:24Z","receivedAt":"2022-01-05T16:34:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This adds a few tests for the new merge-tree-ort command. They have\nbeen copy-pasted from t4300-merge-tree.sh, and then the expected\noutput has been adjusted.\n\nSigned-off-by: Christian Couder \n---\n t/t4310-merge-tree-ort.sh | 162 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 162 insertions(+)\n create mode 100755 t/t4310-merge-tree-ort.sh\n\ndiff --git a/t/t4310-merge-tree-ort.sh b/t/t4310-merge-tree-ort.sh\nnew file mode 100755\nindex 0000000000..9a54508e82\n--- /dev/null\n+++ b/t/t4310-merge-tree-ort.sh\n@@ -0,0 +1,162 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Will Palmer\n+# Copyright (c) 2021 Christian Couder\n+#\n+\n+test_description='git merge-tree-ort'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit \"initial\" \"initial-file\" \"initial\"\n+'\n+\n+test_expect_success 'file add A, !B' '\n+\tgit reset --hard initial &&\n+\ttest_commit \"add-a-not-b\" \"ONE\" \"AAA\" &&\n+\tgit merge-tree-ort initial initial add-a-not-b >actual &&\n+\tcat >expected <actual &&\n+\tcat >expected <actual &&\n+\tcat >expected <actual &&\n+\tcat >expected <>>>>>> add-a-b-diff-B\n+diff with branch2:\n+:100644 100644 ba62923 1e462bc M\tONE\n+\n+diff --git a/ONE b/ONE\n+index ba62923..1e462bc 100644\n+--- a/ONE\n++++ b/ONE\n+@@ -1 +1,5 @@\n++<<<<<<< add-a-b-diff-A\n++AAA\n++=======\n+ BBB\n++>>>>>>> add-a-b-diff-B\n+diff with base:\n+:000000 100644 0000000 1e462bc A\tONE\n+\n+diff --git a/ONE b/ONE\n+new file mode 100644\n+index 0000000..1e462bc\n+--- /dev/null\n++++ b/ONE\n+@@ -0,0 +1,5 @@\n++<<<<<<< add-a-b-diff-A\n++AAA\n++=======\n++BBB\n++>>>>>>> add-a-b-diff-B\n+EXPECTED\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n2.34.1.433.g7bc349372a.dirty\n\n"},{"id":"445537","messageId":"CABPp-BFh7UnQtPM=tO8rfp5bPK4-7esouv5KCx1sUSESwEA=Rw@mail.gmail.com","threadId":"57191","inReplyTo":"20220105163324.73369-1-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-05T16:53:48Z","receivedAt":"2022-01-05T16:54:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n wrote:\n>\n> During the 2nd Virtual Git Contributors’ Summit last October, and even\n> before, the subject of performing server side merges and rebases came\n> up, as platforms like GitHub and GitLab would like to support many\n> features and data formats that libgit2 doesn't support, like for\n> example SHA256 hashes and partial clone.\n>\n> It's hard for them to get rid of libgit2 though, because Git itself\n> doesn't have a good way to support server side merges and rebases,\n> while libgit2 has ways to perform them. Without server side merges and\n> rebases, those platforms would have to launch some kind of checkout,\n> which can be very expensive, before any merge or rebase.\n>\n> The latest discussions on this topic following the 2nd Virtual\n> Summit[1] ended with some proposals around a `git merge-tree` on\n> steroids that could be a good solution to this issue.\n>\n> The current `git merge-tree` command though seems to have a number of\n> issues, especially:\n>\n> - it's too much related to the old merge recursive strategy which is\n> not the default anymore since v2.34.0 and is likely to be\n> deprecated over time,\n>\n> - it seems to output things in its own special format, which is not\n> easy to customize, and which needs special code and logic to parse\n\nI agree we don't want those...but why would new merge-tree options\nhave to use the old merge strategy or the old output format?\n\n> To move forward on this, this small RFC patch series introduces a new\n> `git merge-tree-ort` command with the following design:\n\nSlightly dislike the command name. `ort` was meant as a temporary,\ninternal name. I don't think it's very meaningful to users, so I was\nhoping to just make `recursive` mean `ort` after we had enough\ntesting, and to delete merge-recursive.[ch] at that time. Then `ort`\nmerely becomes a historical footnote (...and perhaps part of the name\nof the file where the `recursive` algorithm is implemented).\n\n> - it uses merge-ort's API as is to perform the merge\n>\n> - it gets back a tree oid and a cleanliness status from merge-ort's\n> API and prints them out first\n\nGood so far.\n\n>\n> - it uses diff's API as is to output changed paths and code\n>\n> - the diff API, actually diff_tree_oid() is called 3 times: once for\n> the diff versus branch1 (\"ours\"), once for the diff versus branch2\n> (\"theirs\"), and once for the diff versus the base.\n\nWhy? That seems to be a performance penalty for anyone that doesn't\nwant/need the diffs, and since we return a tree, a caller can go and\nget whatever diffs they like.\n\n> Therefore:\n>\n> - its code is very simple and very easy to extend and customize, for\n> example by passing diff or merge-ort options that the code would\n> just pass on to the merge-ort and diff APIs respectively\n>\n> - its output can easily be parsed using simple code\n\nThese points are good.\n\n> and existing diff parsers\n>\n> This of course means that merge-tree-ort's output is not backward\n> compatible with merge-tree's output, but it doesn't seem that there is\n> much value in keeping the same output anyway. On the contrary\n> merge-tree's output is likely to hold us back already.\n>\n> The first patch in the series adds the new command without any test\n> and documentation.\n>\n> The second patch in the series adds a few tests that let us see how\n> the command's output looks like in different very simple cases.\n>\n> Of course if this approach is considered valuable, I plan to add some\n> documentation, more tests and very likely a number of options before\n> submitting the next iteration.\n\nWas there something you didn't like about\nhttps://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/?\n\n> I am not sure that it's worth showing the 3 diffs (versus branch1,\n> branch2 and base) by default. Maybe by default no diff at all should\n> be shown and the command should have --branch1 (or --ours), --branch2\n> (or --theirs) and --base options to ask for such output, but for an\n> RFC patch I thought it would be better to output the 3 diffs so that\n> people get a better idea of the approach this patch series is taking.\n\nI think not showing, neither by default or at all would be better.\nAll three of these are things users could easily generate for\nthemselves with the tree we return. I'm curious, though, what's the\nusecase for wanting these specific diffs?\n\nTwo things you didn't return that users cannot get any other way: (1)\nconflict and warning messages, (2) list of conflicted paths.\n\n> [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211147490.56@tvgsbejvaqbjf.bet/\n>\n>\n> Christian Couder (2):\n> merge-ort: add new merge-tree-ort command\n> merge-ort: add t/t4310-merge-tree-ort.sh\n>\n> .gitignore | 1 +\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/merge-tree-ort.c | 93 ++++++++++++++++++++++\n> git.c | 1 +\n> t/t4310-merge-tree-ort.sh | 162 ++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 259 insertions(+)\n> create mode 100644 builtin/merge-tree-ort.c\n> create mode 100755 t/t4310-merge-tree-ort.sh\n>\n> --\n> 2.34.1.433.g7bc349372a.dirty\n"},{"id":"445538","messageId":"CABPp-BFQv+mrWj8iH0Vo5Pr5L922v=ZsVthFjofy5pm1Sx8x5Q@mail.gmail.com","threadId":"57191","inReplyTo":"20220105163324.73369-2-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 1/2] merge-ort: add new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-05T17:08:21Z","receivedAt":"2022-01-05T17:08:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n wrote:\n>\n> This new command is similar as merge-tree, but uses\n> merge-ort code and features, especially\n> merge_incore_nonrecursive(), instead of those from\n> resursive merge, to perform the merge.\n\ns/resursive/recursive/\n\n> The output from this new command is very different from\n> merge-tree's custom output, as we are only using code and\n> features from diff.{h,c}, especially diff_tree_oid(). This\n> should make it easy to customize and standardize the output\n> using regular diff options in the future.\n>\n> This command will be extended to support new features\n> with the long-term goal of enabling merges and rebases\n> without a worktree.\n\nAnd cherry-picks? And reverts?\n\nI think the cherry-pick/rebase replacement actually deserves a\nseparate command from what merges should use. Replaying a sequence of\ncommits just has a number of UI differences and abilities that I think\npull it in a different direction. And I don't want replaying of a\nsequence of commits via a script like the old days (even if one that\ncalls something that doesn't update the working tree and index would\nbe better than the old rebases built on top of `git merge-recursive`\nand `git cherry-pick`). I'm working on such a thing, though it's\nstill kind of early.\n\n>\n> Signed-off-by: Christian Couder \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/merge-tree-ort.c | 93 ++++++++++++++++++++++++++++++++++++++++\n> git.c | 1 +\n> 5 files changed, 97 insertions(+)\n> create mode 100644 builtin/merge-tree-ort.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index 054249b20a..2dfcb1a589 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -98,6 +98,7 @@\n> /git-merge-index\n> /git-merge-file\n> /git-merge-tree\n> +/git-merge-tree-ort\n> /git-merge-octopus\n> /git-merge-one-file\n> /git-merge-ours\n> diff --git a/Makefile b/Makefile\n> index 75ed168adb..915e500b06 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1124,6 +1124,7 @@ 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-tree.o\n> +BUILTIN_OBJS += builtin/merge-tree-ort.o\n> BUILTIN_OBJS += builtin/merge.o\n> BUILTIN_OBJS += builtin/mktag.o\n> BUILTIN_OBJS += builtin/mktree.o\n> diff --git a/builtin.h b/builtin.h\n> index 8a58743ed6..c68f46b118 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -182,6 +182,7 @@ 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_tree(int argc, const char **argv, const char *prefix);\n> +int cmd_merge_tree_ort(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> int cmd_multi_pack_index(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/merge-tree-ort.c b/builtin/merge-tree-ort.c\n> new file mode 100644\n> index 0000000000..1c8ecd16ec\n> --- /dev/null\n> +++ b/builtin/merge-tree-ort.c\n> @@ -0,0 +1,93 @@\n> +#include \"builtin.h\"\n> +#include \"merge-ort.h\"\n> +#include \"diff.h\"\n> +\n> +static const char merge_tree_ort_usage[] = \"git merge-tree-ort \";\n\nI think this is somewhat limiting to use for merges in general; it'll\ngenerate trees that do not match what users get from `git merge ...`\nat the command line, because specifying a single base-tree is\ndifferent than having the algorithm determine the merge base,\nparticularly when it needs to construct a virtual merge base.\n\n> +\n> +static void show_result(struct tree *base_tree,\n> + struct tree *head_tree,\n> + struct tree *merge_tree,\n> + struct merge_result *result)\n> +{\n> + const struct object_id *base_oid = &(base_tree->object.oid);\n> + const struct object_id *head_oid = &(head_tree->object.oid);\n> + const struct object_id *merge_oid = &(merge_tree->object.oid);\n> + const struct object_id *result_oid = &(result->tree->object.oid);\n> + struct diff_options opts;\n> +\n> + repo_diff_setup(the_repository, &opts);\n> + opts.stat_width = -1; /* use full terminal width */\n> + opts.output_format |= DIFF_FORMAT_RAW | DIFF_FORMAT_PATCH;\n> + opts.detect_rename = DIFF_DETECT_RENAME;\n> + diff_setup_done(&opts);\n> +\n> + printf(\"result tree: %s\\n\", oid_to_hex(result_oid));\n> + printf(\"clean: %d\\n\", result->clean);\n> +\n> + printf(\"diff with branch1:\\n\");\n> + diff_tree_oid(head_oid, result_oid, \"\", &opts);\n> + diffcore_std(&opts);\n> + diff_flush(&opts);\n> +\n> + printf(\"diff with branch2:\\n\");\n> + diff_tree_oid(merge_oid, result_oid, \"\", &opts);\n> + diffcore_std(&opts);\n> + diff_flush(&opts);\n> +\n> + printf(\"diff with base:\\n\");\n> + diff_tree_oid(base_oid, result_oid, \"\", &opts);\n> + diffcore_std(&opts);\n> + diff_flush(&opts);\n\nI commented on the diffs in my response on the cover letter.\n\n> +}\n> +\n> +static struct commit *get_commit_by_name_or_die(const char *name)\n> +{\n> + struct commit *c = lookup_commit_reference_by_name(name);\n> + if (!c)\n> + die(_(\"not a valid commit name '%s'\"), name);\n> + return c;\n> +}\n> +\n> +static void merge_trees_ort(const char *base_name,\n> + const char *branch1,\n> + const char *branch2)\n> +{\n> + struct merge_result result;\n> + struct merge_options merge_opt;\n> +\n> + struct commit *base = get_commit_by_name_or_die(base_name);\n> + struct commit *head = get_commit_by_name_or_die(branch1);\n> + struct commit *merge = get_commit_by_name_or_die(branch2);\n> +\n> + struct tree *base_tree = get_commit_tree(base);\n> + struct tree *head_tree = get_commit_tree(head);\n> + struct tree *merge_tree = get_commit_tree(merge);\n> +\n> + memset(&result, 0, sizeof(result));\n> + init_merge_options(&merge_opt, the_repository);\n> +\n> + merge_opt.show_rename_progress = 1;\n> + merge_opt.branch1 = branch1;\n> + merge_opt.branch2 = branch2;\n> + merge_opt.ancestor = base_name;\n> +\n> + result.tree = head_tree;\n> +\n> + merge_incore_nonrecursive(&merge_opt,\n> + base_tree,\n> + result.tree,\n> + merge_tree,\n> + &result);\n\nI think for server side merges, merge_incore_recursive() should be\nused, so that they match what a `git merge ...` would provide. I\nthink merge_incore_nonrecursive() is better used for replaying a\nsequence of commits.\n\nSo, I view this as more suitable for usage with the server side\nrebase, except that it seems to be serving as a building block and\nexpecting the rebase to be some script on top. I think server-side\nrebase/cherry-pick should be a builtin of some form.\n\n> +\n> + show_result(base_tree, head_tree, merge_tree, &result);\n\nAs noted in the cover letter, this current form doesn't provide 2\nthings that users can't get some other way: (1) a list of conflicted\npaths, and (2) the various conflict and warning messages generated\nduring the merge. I agree that many usecases might not want those,\nbut since they can't be obtained another way I think it'd be prudent\nto provide them.\n\nAnd this show_result provides 3 things that users could generate on\ntheir own with the information already provided -- namely the three\ndiffs that you do. I'm curious what the usecase is for those diffs.\n\n> +}\n> +\n> +int cmd_merge_tree_ort(int argc, const char **argv, const char *prefix)\n> +{\n> + if (argc != 4)\n> + usage(merge_tree_ort_usage);\n> +\n> + merge_trees_ort(argv[1], argv[2], argv[3]);\n> +\n> + return 0;\n> +}\n> diff --git a/git.c b/git.c\n> index 7edafd8ecf..90b8a4984c 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -562,6 +562,7 @@ static struct cmd_struct commands[] = {\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-tree\", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },\n> + { \"merge-tree-ort\", cmd_merge_tree_ort, RUN_SETUP | NO_PARSEOPT },\n> { \"mktag\", cmd_mktag, RUN_SETUP | NO_PARSEOPT },\n> { \"mktree\", cmd_mktree, RUN_SETUP },\n> { \"multi-pack-index\", cmd_multi_pack_index, RUN_SETUP },\n> --\n> 2.34.1.433.g7bc349372a.dirty\n"},{"id":"445548","messageId":"CABPp-BFGh=aNbtz7PDfH+JK13Hazb+EggLy8XDWqu0wAP=hmuw@mail.gmail.com","threadId":"57191","inReplyTo":"20220105163324.73369-3-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH 2/2] merge-ort: add t/t4310-merge-tree-ort.sh","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-05T17:29:35Z","receivedAt":"2022-01-05T17:30:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n wrote:\n>\n> This adds a few tests for the new merge-tree-ort command. They have\n> been copy-pasted from t4300-merge-tree.sh, and then the expected\n> output has been adjusted.\n>\n> Signed-off-by: Christian Couder \n> ---\n> t/t4310-merge-tree-ort.sh | 162 ++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 162 insertions(+)\n> create mode 100755 t/t4310-merge-tree-ort.sh\n>\n> diff --git a/t/t4310-merge-tree-ort.sh b/t/t4310-merge-tree-ort.sh\n> new file mode 100755\n> index 0000000000..9a54508e82\n> --- /dev/null\n> +++ b/t/t4310-merge-tree-ort.sh\n> @@ -0,0 +1,162 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2010 Will Palmer\n> +# Copyright (c) 2021 Christian Couder\n> +#\n> +\n> +test_description='git merge-tree-ort'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> + test_commit \"initial\" \"initial-file\" \"initial\"\n> +'\n> +\n> +test_expect_success 'file add A, !B' '\n> + git reset --hard initial &&\n> + test_commit \"add-a-not-b\" \"ONE\" \"AAA\" &&\n> + git merge-tree-ort initial initial add-a-not-b >actual &&\n> + cat >expected < +result tree: ee38e20a5c0e1698539ac99d55616079a04fce26\n\nSo the tests only work on sha1? My tests in\nhttps://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/\nare sha256 compatible.\n\n> +clean: 1\n> +diff with branch1:\n> +:000000 100644 0000000 43d5a8e A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..43d5a8e\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n\nOh, this isn't just a --raw diff, but both a raw and full diff? I\nmissed that reading over the previous patch. This seems potentially\n*extremely* expensive for big repos; dramatically more so than the\nmerge portion of the operation. (Any files modified on just one side\ncan be trivially merged without looking at the contents. In fact,\ndirectories only modified on one side can usually be trivially merged\nwithout looking at the contents. But merges are going to modify lots\nof files relative to either of the two sides and especially relative\nto the merge base, and doing a full diff is going to have to crack\nopen every one of those files -- multiple times since you do it\nagainst the base as well -- to show this output). I don't think this\nis what you want.\n\n> +diff with branch2:\n> +diff with base:\n> +:000000 100644 0000000 43d5a8e A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..43d5a8e\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n> +EXPECTED\n> +\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'file add !A, B' '\n> + git reset --hard initial &&\n> + test_commit \"add-not-a-b\" \"ONE\" \"AAA\" &&\n> + git merge-tree-ort initial add-not-a-b initial >actual &&\n> + cat >expected < +result tree: ee38e20a5c0e1698539ac99d55616079a04fce26\n> +clean: 1\n> +diff with branch1:\n> +diff with branch2:\n> +:000000 100644 0000000 43d5a8e A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..43d5a8e\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n> +diff with base:\n> +:000000 100644 0000000 43d5a8e A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..43d5a8e\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n> +EXPECTED\n> +\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'file add A, B (same)' '\n> + git reset --hard initial &&\n> + test_commit \"add-a-b-same-A\" \"ONE\" \"AAA\" &&\n> + git reset --hard initial &&\n> + test_commit \"add-a-b-same-B\" \"ONE\" \"AAA\" &&\n> + git merge-tree-ort initial add-a-b-same-A add-a-b-same-B >actual &&\n> + cat >expected < +result tree: ee38e20a5c0e1698539ac99d55616079a04fce26\n> +clean: 1\n> +diff with branch1:\n> +diff with branch2:\n> +diff with base:\n> +:000000 100644 0000000 43d5a8e A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..43d5a8e\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n> +EXPECTED\n> +\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'file add A, B (different)' '\n> + git reset --hard initial &&\n> + test_commit \"add-a-b-diff-A\" \"ONE\" \"AAA\" &&\n> + git reset --hard initial &&\n> + test_commit \"add-a-b-diff-B\" \"ONE\" \"BBB\" &&\n> + git merge-tree-ort initial add-a-b-diff-A add-a-b-diff-B >actual &&\n> + cat >expected < +result tree: 3aa938e8cc8be73c81cbaf629ea55a16e7c39319\n> +clean: 0\n> +diff with branch1:\n> +:100644 100644 43d5a8e 1e462bc M ONE\n> +\n> +diff --git a/ONE b/ONE\n> +index 43d5a8e..1e462bc 100644\n> +--- a/ONE\n> ++++ b/ONE\n> +@@ -1 +1,5 @@\n> ++<<<<<<< add-a-b-diff-A\n> + AAA\n> ++=======\n> ++BBB\n> ++>>>>>>> add-a-b-diff-B\n> +diff with branch2:\n> +:100644 100644 ba62923 1e462bc M ONE\n> +\n> +diff --git a/ONE b/ONE\n> +index ba62923..1e462bc 100644\n> +--- a/ONE\n> ++++ b/ONE\n> +@@ -1 +1,5 @@\n> ++<<<<<<< add-a-b-diff-A\n> ++AAA\n> ++=======\n> + BBB\n> ++>>>>>>> add-a-b-diff-B\n> +diff with base:\n> +:000000 100644 0000000 1e462bc A ONE\n> +\n> +diff --git a/ONE b/ONE\n> +new file mode 100644\n> +index 0000000..1e462bc\n> +--- /dev/null\n> ++++ b/ONE\n> +@@ -0,0 +1,5 @@\n> ++<<<<<<< add-a-b-diff-A\n> ++AAA\n> ++=======\n> ++BBB\n> ++>>>>>>> add-a-b-diff-B\n> +EXPECTED\n> +\n> + test_cmp expected actual\n> +'\n> +\n> +test_done\n> --\n\nI've focused a bit on the things that I didn't care as much for, but\nthe usage of the merge-ort API was solid and there are pieces here\nthat look quite simliar to what I'd expect...and in fact, to what I\nalso implemented. Perhaps there are multiple things I also overlooked\nin my implementation of this idea; would be great to get your comments\non that, over at [1].\n\nAnd, as a heads up, as noted at [2], I'm also working on the\nserver-side cherry-pick/rebase.\n\n\n[1] https://lore.kernel.org/git/pull.1114.v2.git.git.1641403655.gitgitgadget@gmail.com/\n[2] https://lore.kernel.org/git/CABPp-BHpK8hPsiuHoYsf5D_rjcGLSW-_faL3ODoh56pG_2Luwg@mail.gmail.com/\n"},{"id":"445550","messageId":"CABPp-BEeBpJoU4yXdfA6vRAYVAUbd2gRhEV6j4VEqoqcu=FGSw@mail.gmail.com","threadId":"57191","inReplyTo":"CABPp-BFh7UnQtPM=tO8rfp5bPK4-7esouv5KCx1sUSESwEA=Rw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-05T17:32:17Z","receivedAt":"2022-01-05T17:32:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 5, 2022 at 8:53 AM Elijah Newren wrote:\n>\n> On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> wrote:\n> >\n> > During the 2nd Virtual Git Contributors’ Summit last October, and even\n> > before, the subject of performing server side merges and rebases came\n> > up, as platforms like GitHub and GitLab would like to support many\n> > features and data formats that libgit2 doesn't support, like for\n> > example SHA256 hashes and partial clone.\n> >\n...\n> > The first patch in the series adds the new command without any test\n> > and documentation.\n> >\n> > The second patch in the series adds a few tests that let us see how\n> > the command's output looks like in different very simple cases.\n> >\n> > Of course if this approach is considered valuable, I plan to add some\n> > documentation, more tests and very likely a number of options before\n> > submitting the next iteration.\n>\n> Was there something you didn't like about\n> https://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/?\n\nSince I already had some fixes queued up for that series based on\nfeedback from Johannes Altmanninger and Fabian Stelzer, I decided to\nsubmit those so you wouldn't have to stumble over the same problems.\nSo see https://lore.kernel.org/git/pull.1114.v2.git.git.1641403655.gitgitgadget@gmail.com/\ninstead (which also now has a pointer to this series in the cover\nletter).\n"},{"id":"445728","messageId":"CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com","threadId":"57191","inReplyTo":"CABPp-BFh7UnQtPM=tO8rfp5bPK4-7esouv5KCx1sUSESwEA=Rw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-01-07T17:58:00Z","receivedAt":"2022-01-07T17:58:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren wrote:\n>\n> On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> wrote:\n\n> > The current `git merge-tree` command though seems to have a number of\n> > issues, especially:\n> >\n> > - it's too much related to the old merge recursive strategy which is\n> > not the default anymore since v2.34.0 and is likely to be\n> > deprecated over time,\n> >\n> > - it seems to output things in its own special format, which is not\n> > easy to customize, and which needs special code and logic to parse\n>\n> I agree we don't want those...but why would new merge-tree options\n> have to use the old merge strategy or the old output format?\n\nYeah, it's not necessary if there are 2 separate modes, a \"real\" mode\n(like what you implemented with --real in your recent patch series)\nand a \"trivial\" mode (which is the name you give to the old code).\n\nAdding modes like this to a command is likely to make the command and\nits documentation more difficult to understand though. For example I\nthink that we created `git switch` because the different modes of `git\ncheckout` made the command hard to learn.\n\nI gave other reasons in [1] why I prefer a separate command.\n\n[1] https://lore.kernel.org/git/CAP8UFD1LgfZ0MT9=cMvxCcox++_MBBhWG9Twf42cMiXL42AdpQ@mail.gmail.com/\n\n> > To move forward on this, this small RFC patch series introduces a new\n> > `git merge-tree-ort` command with the following design:\n>\n> Slightly dislike the command name.\n\nI am ok with changing the command name.\n\n> `ort` was meant as a temporary,\n> internal name. I don't think it's very meaningful to users, so I was\n> hoping to just make `recursive` mean `ort` after we had enough\n> testing, and to delete merge-recursive.[ch] at that time. Then `ort`\n> merely becomes a historical footnote (...and perhaps part of the name\n> of the file where the `recursive` algorithm is implemented).\n\nI think something similar could still be done with `git\nmerge-tree-ort` or whatever name we give to this command. For example\nwe could first add --ort to `git merge-tree` and make it call `git\nmerge-tree-ort`, then after some time make --ort the default, then\nafter some more time remove `git merge-tree-ort`. And whenever we make\nthose changes we could also rename the builtin/merge-tree*.{h,c}\naccordingly.\n\n> > - it uses merge-ort's API as is to perform the merge\n> >\n> > - it gets back a tree oid and a cleanliness status from merge-ort's\n> > API and prints them out first\n>\n> Good so far.\n>\n> > - it uses diff's API as is to output changed paths and code\n> >\n> > - the diff API, actually diff_tree_oid() is called 3 times: once for\n> > the diff versus branch1 (\"ours\"), once for the diff versus branch2\n> > (\"theirs\"), and once for the diff versus the base.\n>\n> Why? That seems to be a performance penalty for anyone that doesn't\n> want/need the diffs, and since we return a tree, a caller can go and\n> get whatever diffs they like.\n\nI say somewhere else that I am planning to add options to disable this\nor parts of this diff output.\n\nI think it's still interesting for the command to be able to output\ndiffs, especially diffs of conflicting parts. In [2] Ævar said:\n\n=> I.e. I'm not the first or last to have (not for anything serious)\n=> implement a dry-run bare-repo merge with something like:\n=>\n=> git merge-tree origin/master git-for-windows/main origin/seen >diff\n=> # Better regex needed, but basically this\n=> grep \"^\\+<<<<<<< \\.our$\" diff && conflict=t\n\nAlso `git merge-tree` currently outputs diffs, so I thought it would\nbe sad if the new command couldn't do it.\n\n[2] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/\n\n> > Therefore:\n> >\n> > - its code is very simple and very easy to extend and customize, for\n> > example by passing diff or merge-ort options that the code would\n> > just pass on to the merge-ort and diff APIs respectively\n> >\n> > - its output can easily be parsed using simple code\n>\n> These points are good.\n>\n> > and existing diff parsers\n> >\n> > This of course means that merge-tree-ort's output is not backward\n> > compatible with merge-tree's output, but it doesn't seem that there is\n> > much value in keeping the same output anyway. On the contrary\n> > merge-tree's output is likely to hold us back already.\n> >\n> > The first patch in the series adds the new command without any test\n> > and documentation.\n> >\n> > The second patch in the series adds a few tests that let us see how\n> > the command's output looks like in different very simple cases.\n> >\n> > Of course if this approach is considered valuable, I plan to add some\n> > documentation, more tests and very likely a number of options before\n> > submitting the next iteration.\n>\n> Was there something you didn't like about\n> https://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/?\n\nI was having a vacation at the time and even though I skimmed the\nmailing list, I missed it. Sorry.\n\nAlso I thought that you might not be interested in this anymore as you\ndidn't reply to [1] and [2] where Ævar and I both said that we were\ninterested in your opinion on a git merge-tree on steroids.\n\n> > I am not sure that it's worth showing the 3 diffs (versus branch1,\n> > branch2 and base) by default. Maybe by default no diff at all should\n> > be shown and the command should have --branch1 (or --ours), --branch2\n> > (or --theirs) and --base options to ask for such output, but for an\n> > RFC patch I thought it would be better to output the 3 diffs so that\n> > people get a better idea of the approach this patch series is taking.\n>\n> I think not showing, neither by default\n\nI am ok with not showing them by default.\n\n> or at all would be better.\n> All three of these are things users could easily generate for\n> themselves with the tree we return. I'm curious, though, what's the\n> usecase for wanting these specific diffs?\n\nI think I replied to this above.\n\n> Two things you didn't return that users cannot get any other way: (1)\n> conflict and warning messages, (2) list of conflicted paths.\n\nYeah, I wasn't sure how they could be returned with the merge-ort (or\nmaybe diff) API, and I thought that the current `git merge-tree`\ndoesn't report them, so I was aiming for something roughly just as\npowerful as the current `git merge-tree`.\n"},{"id":"445733","messageId":"CABPp-BHpuLjCYycBwweMW_NdQrS_r3ECmvLb8XjNW-_Rau5NTw@mail.gmail.com","threadId":"57191","inReplyTo":"CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-07T19:06:07Z","receivedAt":"2022-01-07T19:06:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 7, 2022 at 9:58 AM Christian Couder\n wrote:\n>\n> On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren wrote:\n> >\n> > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > wrote:\n>\n> > > The current `git merge-tree` command though seems to have a number of\n> > > issues, especially:\n> > >\n> > > - it's too much related to the old merge recursive strategy which is\n> > > not the default anymore since v2.34.0 and is likely to be\n> > > deprecated over time,\n> > >\n> > > - it seems to output things in its own special format, which is not\n> > > easy to customize, and which needs special code and logic to parse\n> >\n> > I agree we don't want those...but why would new merge-tree options\n> > have to use the old merge strategy or the old output format?\n>\n> Yeah, it's not necessary if there are 2 separate modes, a \"real\" mode\n> (like what you implemented with --real in your recent patch series)\n> and a \"trivial\" mode (which is the name you give to the old code).\n>\n> Adding modes like this to a command is likely to make the command and\n> its documentation more difficult to understand though. For example I\n> think that we created `git switch` because the different modes of `git\n> checkout` made the command hard to learn.\n>\n> I gave other reasons in [1] why I prefer a separate command.\n>\n> [1] https://lore.kernel.org/git/CAP8UFD1LgfZ0MT9=cMvxCcox++_MBBhWG9Twf42cMiXL42AdpQ@mail.gmail.com/\n\nI can see where you're coming from, but I think the particular\ncomparison you use isn't quite apples to apples. `git checkout` has a\n\"change branches\" mode and a \"revert specific paths\" mode. While\nthose have significant implementation overlap, they seem like\ndifferent concepts to users. For merge-tree, the implementation is\ncompletely orthogonal between the two modes, but the concept is\nbasically the same from the user viewpoint. Yes, the output differs\nin the two merge-tree modes, but command line arguments are often used\nto tweak the output (much like diff has different output styles based\non various flags). In fact, in [1] where you suggest a new command\nyou suggest that it should have \"roughly the same features as git\nmerge-tree and a similar interface\". To me, that suggests that the\ntwo may be good candidates for being similar commands.\n\nThat said, I'm not against a new command. Personally, my main reason\nfor using merge-tree wasn't because that's where I thought it was best\nto put it, but just that (IIRC) each of Dscho, Peff, and Junio\nsuggested that location.\n\nMy biggest gripe was just the command name...\n\n> > > To move forward on this, this small RFC patch series introduces a new\n> > > `git merge-tree-ort` command with the following design:\n> >\n> > Slightly dislike the command name.\n>\n> I am ok with changing the command name.\n\n:-)\n\n> > `ort` was meant as a temporary,\n> > internal name. I don't think it's very meaningful to users, so I was\n> > hoping to just make `recursive` mean `ort` after we had enough\n> > testing, and to delete merge-recursive.[ch] at that time. Then `ort`\n> > merely becomes a historical footnote (...and perhaps part of the name\n> > of the file where the `recursive` algorithm is implemented).\n>\n> I think something similar could still be done with `git\n> merge-tree-ort` or whatever name we give to this command. For example\n> we could first add --ort to `git merge-tree` and make it call `git\n> merge-tree-ort`, then after some time make --ort the default, then\n> after some more time remove `git merge-tree-ort`. And whenever we make\n> those changes we could also rename the builtin/merge-tree*.{h,c}\n> accordingly.\n\nUser-facing names tend to take a while to remove; I'd rather start\nwith user-facing command and option names that are meaningful to users\nin terms of what they want to accomplish. I don't think `ort`\nqualifies as such; I'd rather `ort` only be used by expert users (e.g.\nfolks wanting to test out a new merge algorithm they heard about\nbefore it became the default).\n\n> > > - it uses merge-ort's API as is to perform the merge\n> > >\n> > > - it gets back a tree oid and a cleanliness status from merge-ort's\n> > > API and prints them out first\n> >\n> > Good so far.\n> >\n> > > - it uses diff's API as is to output changed paths and code\n> > >\n> > > - the diff API, actually diff_tree_oid() is called 3 times: once for\n> > > the diff versus branch1 (\"ours\"), once for the diff versus branch2\n> > > (\"theirs\"), and once for the diff versus the base.\n> >\n> > Why? That seems to be a performance penalty for anyone that doesn't\n> > want/need the diffs, and since we return a tree, a caller can go and\n> > get whatever diffs they like.\n>\n> I say somewhere else that I am planning to add options to disable this\n> or parts of this diff output.\n>\n> I think it's still interesting for the command to be able to output\n> diffs, especially diffs of conflicting parts.\n\nAre you presuming that you can output a diff of conflicting parts? A\ndiff on a modify/delete or can't-merge-binary-files won't show\nanything, and won't provide any notification that there even was a\nconflict.\n\n> In [2] Ævar said:\n>\n> => I.e. I'm not the first or last to have (not for anything serious)\n> => implement a dry-run bare-repo merge with something like:\n> =>\n> => git merge-tree origin/master git-for-windows/main origin/seen >diff\n> => # Better regex needed, but basically this\n> => grep \"^\\+<<<<<<< \\.our$\" diff && conflict=t\n\nThis code is pretty hacky; it presumes that content conflicts are the\nonly type of conflict. Inability to merge binary files,\nfile/directory conflicts, mode conflicts, modify/delete conflicts,\nvarious rename conflicts, etc. are all other types. I'd rather not\nencourage such hacks.\n\nReturning whether there were conflicts in the exit status is far\ncleaner. Providing some mechanism for getting a list of files which\nhad conflicts may also be useful, but both of these are things where a\ndiff is the wrong tool.\n\n> Also `git merge-tree` currently outputs diffs, so I thought it would\n> be sad if the new command couldn't do it.\n>\n> [2] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/\n\nHmm, I had a totally different opinion. I felt the diffs in the\ncurrent merge-tree was a hack to deal with the fact that they didn't\nhave good tools to make use of the results, and ended up providing a\nRube-Goldberg scheme to do anything useful with it.\n\nProviding a tree is a concrete and easily usable object for end users.\nThey can feed that tree to other git commands to do additional things,\nand obviates the need for the Rube-Goldberg contraption.\n\n> > Was there something you didn't like about\n> > https://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/?\n>\n> I was having a vacation at the time and even though I skimmed the\n> mailing list, I missed it. Sorry.\n\nAh, gotcha. I was just curious if there was something you felt was\nwrong and you wanted to propose an alternative.\n\n> Also I thought that you might not be interested in this anymore as you\n> didn't reply to [1] and [2] where Ævar and I both said that we were\n> interested in your opinion on a git merge-tree on steroids.\n\nSorry, I saw Ævar's email and noticed various assumptions I felt were\nproblematic, but noticed he said he was just spitballing (which is\ntotally fair). I figured that rather than attempt to explain each\ndetail, it'd be better to update my code and provide it...and I then\nignored the rest of the thread and missed your question. Sorry. I\ncertainly meant to update this series and send it out earlier, but\nthere were multiple other series of mine that were also backed up and\nseveral series I needed to review, so it took me a while to clean out\nthe queue a bit.\n"},{"id":"445743","messageId":"nycvar.QRO.7.76.6.2201072044120.339@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-07T19:54:30Z","receivedAt":"2022-01-07T19:54:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Fri, 7 Jan 2022, Christian Couder wrote:\n\n> On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren wrote:\n> >\n> > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > wrote:\n>\n> > > To move forward on this, this small RFC patch series introduces a\n> > > new `git merge-tree-ort` command with the following design:\n> >\n> > Slightly dislike the command name.\n>\n> I am ok with changing the command name.\n\nOr just going with `merge-tree`?\n\nThat command name has the distinct advantage of _already_ being used by\nGit. So there is no chance for users to have a `git-merge-tree` script\nlying around that all of a sudden would stop working because a superseding\nbuilt-in using the same name is introduced.\n\nThat is a distinct advantage of using the existing command for the new\ncode flow, even if the command name could be interpreted as misleading. Of\ncourse, the name could also be construed to be on point: it merges, and it\nproduces a tree, hence \"merge-tree\".\n\n> > > - the diff API, actually diff_tree_oid() is called 3 times: once for\n> > > the diff versus branch1 (\"ours\"), once for the diff versus branch2\n> > > (\"theirs\"), and once for the diff versus the base.\n> >\n> > Why? That seems to be a performance penalty for anyone that doesn't\n> > want/need the diffs, and since we return a tree, a caller can go and\n> > get whatever diffs they like.\n>\n> I say somewhere else that I am planning to add options to disable this\n> or parts of this diff output.\n\nWell, now you got me really curious. Since you are listed as part of\nGitLab (https://about.gitlab.com/company/team/#chriscool), I assume that\nyou are pretty familiar with how merges are done on GitLab's server side,\nand plan on experimenting with your own work to use `merge-ort` on\nGitLab's servers.\n\nWhich makes me wonder where that idea comes from to use the diff API?\n\nWhat operations does GitLab need from that Git command, which code flows\nneed to be supported (and what inputs/outputs are there)?\n\n> I think it's still interesting for the command to be able to output\n> diffs, especially diffs of conflicting parts. In [2] Ævar said:\n>\n> => I.e. I'm not the first or last to have (not for anything serious)\n> => implement a dry-run bare-repo merge with something like:\n> =>\n> => git merge-tree origin/master git-for-windows/main origin/seen >diff\n> => # Better regex needed, but basically this\n> => grep \"^\\+<<<<<<< \\.our$\" diff && conflict=t\n>\n> Also `git merge-tree` currently outputs diffs, so I thought it would\n> be sad if the new command couldn't do it.\n>\n> [2] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/\n\nBut `git merge-tree` was only meant as a proof-of-concept, intended to\nentice other developers to turn it into something useful. As it took\n_this_ long, we can safely assume that the user interface of `merge-tree`\nleft a bit of a room for improvement. Wouldn't you agree?\n\n> Also I thought that you might not be interested in this anymore as you\n> didn't reply to [1] and [2] where Ævar and I both said that we were\n> interested in your opinion on a git merge-tree on steroids.\n\nElijah stated at the time that he was scarce on time for Git.\n\nBut back to the topic: have you played with using `merge-ort` in GitLab\nalready? What were the needs you saw that would be required of the Git\ncommand, what flows would it have to support?\n\nCiao,\nJohannes\n"},{"id":"445829","messageId":"nycvar.QRO.7.76.6.2201101427440.339@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"CABPp-BHpuLjCYycBwweMW_NdQrS_r3ECmvLb8XjNW-_Rau5NTw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-10T13:49:26Z","receivedAt":"2022-01-10T13:49:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 7 Jan 2022, Elijah Newren wrote:\n\n> On Fri, Jan 7, 2022 at 9:58 AM Christian Couder\n> wrote:\n> >\n> > On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren wrote:\n> > >\n> > > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > > wrote:\n> >\n> > > > The current `git merge-tree` command though seems to have a number of\n> > > > issues, especially:\n> > > >\n> > > > - it's too much related to the old merge recursive strategy which is\n> > > > not the default anymore since v2.34.0 and is likely to be\n> > > > deprecated over time,\n> > > >\n> > > > - it seems to output things in its own special format, which is not\n> > > > easy to customize, and which needs special code and logic to parse\n> > >\n> > > I agree we don't want those...but why would new merge-tree options\n> > > have to use the old merge strategy or the old output format?\n> >\n> > Yeah, it's not necessary if there are 2 separate modes, a \"real\" mode\n> > (like what you implemented with --real in your recent patch series)\n> > and a \"trivial\" mode (which is the name you give to the old code).\n> >\n> > Adding modes like this to a command is likely to make the command and\n> > its documentation more difficult to understand though. For example I\n> > think that we created `git switch` because the different modes of `git\n> > checkout` made the command hard to learn.\n> >\n> > I gave other reasons in [1] why I prefer a separate command.\n> >\n> > [1] https://lore.kernel.org/git/CAP8UFD1LgfZ0MT9=cMvxCcox++_MBBhWG9Twf42cMiXL42AdpQ@mail.gmail.com/\n>\n> I can see where you're coming from, but I think the particular\n> comparison you use isn't quite apples to apples. `git checkout` has a\n> \"change branches\" mode and a \"revert specific paths\" mode. While\n> those have significant implementation overlap, they seem like\n> different concepts to users. For merge-tree, the implementation is\n> completely orthogonal between the two modes, but the concept is\n> basically the same from the user viewpoint. Yes, the output differs\n> in the two merge-tree modes, but command line arguments are often used\n> to tweak the output (much like diff has different output styles based\n> on various flags). In fact, in [1] where you suggest a new command\n> you suggest that it should have \"roughly the same features as git\n> merge-tree and a similar interface\". To me, that suggests that the\n> two may be good candidates for being similar commands.\n>\n> That said, I'm not against a new command. Personally, my main reason\n> for using merge-tree wasn't because that's where I thought it was best\n> to put it, but just that (IIRC) each of Dscho, Peff, and Junio\n> suggested that location.\n>\n> My biggest gripe was just the command name...\n\nI am against a new command for what essentially serves the original\npurpose of `merge-tree`.\n\nThe fact that `merge-tree` has not seen any work in almost 12 years is\ntestament not only to how hard it was to disentangle the work-tree\nrequirement from the recursive merge (it is one of my favorite\ncounterexamples when anybody claims that you can easily prototype code in\na script and then convert it to C), but the fact that there is no user\nwithin Git itself (apart from t/t4300-merge-tree.sh, which does not count)\nspeaks volumes about the design of that `merge-tree` tool.\n\nSo it's only fair to breathe life into it by letting it do what it was\nmeant to do all along.\n\n> > Also `git merge-tree` currently outputs diffs, so I thought it would\n> > be sad if the new command couldn't do it.\n> >\n> > [2] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/\n>\n> Hmm, I had a totally different opinion. I felt the diffs in the\n> current merge-tree was a hack to deal with the fact that they didn't\n> have good tools to make use of the results, and ended up providing a\n> Rube-Goldberg scheme to do anything useful with it.\n\nIndeed. When I had a look how libgit2 is used to perform a server-side\nmerge, I saw how careful the code is not to produce anything unneeded. And\nsince the `merge` operation is performed a ton of times even without any\nuser interaction, producing a diff is the _least_ of said code's concerns.\n\n> Providing a tree is a concrete and easily usable object for end users.\n> They can feed that tree to other git commands to do additional things,\n> and obviates the need for the Rube-Goldberg contraption.\n\nI strongly concur.\n\nSo I would like to reiterate my challenge to you, Christian: could you\nhave a look at the server-side merge of GitLab, and see what it actually\nwould need of the `git merge-tree` command?\n\nI _bet_ it needs first and foremost the information \"is this mergeable?\".\n\nThat is by far the most common question the code I saw had to answer,\nwithout any follow-up questions asked.\n\nAn add-on question is then \"which files/directories conflicted?\". And\nthere, it really only wanted the file names, along with the OIDs of the\nrespective item in the base, the HEAD and the merge branch.\n\nIn my work in December, I also had to implement another thing that I think\nwe will have to implement in `merge-tree` in one form or another: when\nusers provided merge conflict resolutions via the web UI, we want the\nmerge to succeed. What I implemented was this (in a nutshell, a way to\nprovide file names with associated blob OIDs that resolve the content\nconflicts):\n\n-- snip --\nAuthor: Johannes Schindelin \nDate: Thu Dec 16 20:46:35 2021 +0100\nSubject: merge-ort: allow pre-resolving merge conflicts\n\nOne of merge-ort's particular strengths is that it works well even in a\nworktree-less setting, e.g. in a backend for a server-side merge.\n\nIn such a scenario, it is conceivable that the merge in question not\nonly results in a conflict, but that the caller figures out some sort of\nresolution before calling the merge again. The second call, of course,\nis meant to apply the merge conflict resolution.\n\nWith this commit, we allow just that. The new, optional\n`pre_resolved_conflicts` field of `struct merge_options` maps paths to\nthe blob OID of the resolution.\n\nSigned-off-by: Johannes Schindelin \n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a74d4251c3..fa59ce2f97 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3961,6 +3961,7 @@ static void process_entries(struct merge_options *opt,\n \tprefetch_for_content_merges(opt, &plist);\n \tfor (entry = &plist.items[plist.nr-1]; entry >= plist.items; --entry) {\n \t\tchar *path = entry->string;\n+\t\tstruct object_id *resolved_oid;\n \t\t/*\n \t\t * NOTE: mi may actually be a pointer to a conflict_info, but\n \t\t * we have to check mi->clean first to see if it's safe to\n@@ -3972,7 +3973,17 @@ static void process_entries(struct merge_options *opt,\n \t\t\t\t\t &dir_metadata);\n \t\tif (mi->clean)\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n-\t\telse {\n+\t\telse if (opt->pre_resolved_conflicts &&\n+\t\t\t (resolved_oid =\n+\t\t\t strmap_get(opt->pre_resolved_conflicts, path))) {\n+\t\t\tmi->clean = 1;\n+\t\t\tmi->is_null = 0;\n+\t\t\tmemcpy(&mi->result.oid, resolved_oid,\n+\t\t\t sizeof(*resolved_oid));\n+\t\t\tif (!mi->result.mode)\n+\t\t\t\tmi->result.mode = 0100644;\n+\t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n+\t\t} else {\n \t\t\tstruct conflict_info *ci = (struct conflict_info *)mi;\n \t\t\tprocess_entry(opt, path, ci, &dir_metadata);\n \t\t}\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 0795a1d3ec..1f45effdd0 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -47,6 +47,13 @@ struct merge_options {\n \tconst char *subtree_shift;\n \tunsigned renormalize : 1;\n\n+\t/*\n+\t * (merge-ORT only) If non-NULL, contains a map from `path` to OID. If\n+\t * the given `path would be marked as conflict, it is instead resolved\n+\t * to the specified blob.\n+\t */\n+\tstruct strmap *pre_resolved_conflicts;\n+\n \t/* internal fields used by the implementation */\n \tstruct merge_options_internal *priv;\n };\n-- snap --\n\nIt is a proof-of-concept, therefore it expects the resolved conflicts to\nbe in _files_. I don't think that there is a way to reasonably handle\nsymlink target conflicts or directory/file/symlink conflicts, but there\nmight be.\n\nA subsequent commit changed my hacky `merge-tree` hack to optionally\naccept NUL-terminated merge conflict resolutions in / pairs via\nstdin (again, avoiding files to be written where we do not _need_ spend\nI/O unnecessarily).\n\nThe biggest problem we faced at the Contributor Summit was that our\ndiscussion was not informed by the actual server-side needs. So I would\nlike to reiterate my challenge to make that the driver. Let's not use any\nhypothetical scenario as the basis for the design of `git merge-tree`, but\nlet's use the concrete requirements of actual server-side merges that are\ncurrently implemented using libgit2, when trying to figure out what\nfunctionality we need from `merge-tree`, and in what shape.\n\nHere is an initial list:\n\n- need to determine whether a merge will succeed, quickly\n\n- need the tree OID for a successful merge\n\n- need the list of items that conflict, along with OIDs and modes, if the\n merge failed\n\n- need a way to provide merge conflict resolutions\n\nAnd now that I wrote all this, I realize I should have sent it to the\n`merge-tree` thread, not the `merge-tree-ort` thread...\n\nCiao,\nDscho\n"},{"id":"445847","messageId":"xmqq8rvn1nkw.fsf@gitster.g","threadId":"57191","inReplyTo":"nycvar.QRO.7.76.6.2201101427440.339@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-10T17:56:47Z","receivedAt":"2022-01-10T17:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I am against a new command for what essentially serves the original\n> purpose of `merge-tree`.\n>\n> The fact that `merge-tree` has not seen any work in almost 12 years is\n> testament not only to how hard it was to disentangle the work-tree\n> requirement from the recursive merge (it is one of my favorite\n> counterexamples when anybody claims that you can easily prototype code in\n> a script and then convert it to C), but the fact that there is no user\n> within Git itself (apart from t/t4300-merge-tree.sh, which does not count)\n> speaks volumes about the design of that `merge-tree` tool.\n>\n> So it's only fair to breathe life into it by letting it do what it was\n> meant to do all along.\n\nMy \"Yup\" would not weigh as much as one that Linus (whose original\nmerge-tree survived this long without seeing much enhancements)\nmight give us, but he is busy elsewhere so you guys have to live\nwith mine ;-)\n\nAs to its output, I do agree that \"we give a tree when it is already\nusable to record in a new commit\" is a valuable option to have. The\noriginal behaviour should be made available somehow, for those who\nbuilt their workflow (including scripts) around it, though.\n"},{"id":"445848","messageId":"CABPp-BHJvFx0fxobYZ2vauK=KfCLF_7So8xABLjqr9rx4SVy-w@mail.gmail.com","threadId":"57191","inReplyTo":"nycvar.QRO.7.76.6.2201101427440.339@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-10T17:59:30Z","receivedAt":"2022-01-10T17:59:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 10, 2022 at 5:49 AM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Fri, 7 Jan 2022, Elijah Newren wrote:\n>\n> > On Fri, Jan 7, 2022 at 9:58 AM Christian Couder\n> > wrote:\n> > >\n> > > On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren wrote:\n> > > >\n> > > > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > > > wrote:\n> > >\n...\n>\n> I _bet_ it needs first and foremost the information \"is this mergeable?\".\n>\n> That is by far the most common question the code I saw had to answer,\n> without any follow-up questions asked.\n>\n> An add-on question is then \"which files/directories conflicted?\". And\n> there, it really only wanted the file names, along with the OIDs of the\n> respective item in the base, the HEAD and the merge branch.\n\nThis might be difficult to provide for some cases, e.g.\nrename/rename(1to2), especially if that conflict gets entangled with\nany others. Users would also have difficulty gaining these even using\nthe command line `git merge` (with either recursive or ort) for these\ncases.\n\nAlso, does this presume three OIDs? What about the cases where there\nare 4 or 6 for a given path?\n\nI'm a bit worried about the assumptions underlying this request, and\nthat it might not be satisfiable in general depending on what those\nassumptions are.\n\n> In my work in December, I also had to implement another thing that I think\n> we will have to implement in `merge-tree` in one form or another: when\n> users provided merge conflict resolutions via the web UI, we want the\n> merge to succeed. What I implemented was this (in a nutshell, a way to\n> provide file names with associated blob OIDs that resolve the content\n> conflicts):\n\nInteresting. I'm curious, though -- if they are providing conflict\nresolutions, then would they not have had a previous merge to find out\nwhat the conflicts were? If so, wouldn't they be able to provide the\ntree to avoid the need for a second merge?\n\n>\n> -- snip --\n> Author: Johannes Schindelin \n> Date: Thu Dec 16 20:46:35 2021 +0100\n> Subject: merge-ort: allow pre-resolving merge conflicts\n>\n> One of merge-ort's particular strengths is that it works well even in a\n> worktree-less setting, e.g. in a backend for a server-side merge.\n>\n> In such a scenario, it is conceivable that the merge in question not\n> only results in a conflict, but that the caller figures out some sort of\n> resolution before calling the merge again. The second call, of course,\n> is meant to apply the merge conflict resolution.\n>\n> With this commit, we allow just that. The new, optional\n> `pre_resolved_conflicts` field of `struct merge_options` maps paths to\n> the blob OID of the resolution.\n>\n> Signed-off-by: Johannes Schindelin \n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index a74d4251c3..fa59ce2f97 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3961,6 +3961,7 @@ static void process_entries(struct merge_options *opt,\n> prefetch_for_content_merges(opt, &plist);\n> for (entry = &plist.items[plist.nr-1]; entry >= plist.items; --entry) {\n> char *path = entry->string;\n> + struct object_id *resolved_oid;\n> /*\n> * NOTE: mi may actually be a pointer to a conflict_info, but\n> * we have to check mi->clean first to see if it's safe to\n> @@ -3972,7 +3973,17 @@ static void process_entries(struct merge_options *opt,\n> &dir_metadata);\n> if (mi->clean)\n> record_entry_for_tree(&dir_metadata, path, mi);\n> - else {\n> + else if (opt->pre_resolved_conflicts &&\n> + (resolved_oid =\n> + strmap_get(opt->pre_resolved_conflicts, path))) {\n\nYou have a couple problematic assumptions here:\n\n * What if the user's resolution required fixing a semantic conflict,\nmeaning they needed to modify a file that had no conflicts? Your code\nhere would ignore those, because the clean case is handled above.\n\n * What if the user's resolution required adding a totally new file\n(either because a rename is handled as a separate add/delete, or they\njust needed a new file)? The loop above isn't over items in\npre_resolved_conflicts, it just assumes that items in\npre_resolved_conflicts are also in the plist.items being looped over.\n\n\n> + mi->clean = 1;\n> + mi->is_null = 0;\n> + memcpy(&mi->result.oid, resolved_oid,\n> + sizeof(*resolved_oid));\n\nAnd here's another:\n\n * What if the provided resolution was a deletion of the file in\nquestion (especially after e.g. a modify/delete or rename/delete\nconflict)? Don't you need to have a special check for the zero_oid\nhere?\n\nAnd if I combine the things above:\n\n * What if the user wants to remove a file and add a directory of\nfiles in its place at some location in order to resolve things?\nGranted, you didn't handle either deletes or new files above, but if\nyou did add both, then you might have this issue. The code at this\npoint used some very carefully constructed logic and order of walking\nitems specifically to handle file/delete conflicts correctly. I'm\nworried that your conflict resolution stuff here might interact badly\nwith that.\n\n> + if (!mi->result.mode)\n> + mi->result.mode = 0100644;\n\nHow do you know it's not supposed to be 0100755?\n\n> + record_entry_for_tree(&dir_metadata, path, mi);\n> + } else {\n> struct conflict_info *ci = (struct conflict_info *)mi;\n> process_entry(opt, path, ci, &dir_metadata);\n> }\n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index 0795a1d3ec..1f45effdd0 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -47,6 +47,13 @@ struct merge_options {\n> const char *subtree_shift;\n> unsigned renormalize : 1;\n>\n> + /*\n> + * (merge-ORT only) If non-NULL, contains a map from `path` to OID. If\n> + * the given `path would be marked as conflict, it is instead resolved\n> + * to the specified blob.\n> + */\n> + struct strmap *pre_resolved_conflicts;\n> +\n> /* internal fields used by the implementation */\n> struct merge_options_internal *priv;\n> };\n> -- snap --\n>\n> It is a proof-of-concept\n\nFair enough.\n\n>, therefore it expects the resolved conflicts to\n> be in _files_. I don't think that there is a way to reasonably handle\n> symlink target conflicts or directory/file/symlink conflicts, but there\n> might be.\n\nYou really need (mode,oid) pairs to be provided by the user. That\nfixes the executable issue I mentioned above, and makes it clear how\nto handle symlinks and file/symlink conflicts.\n\ndirectory/file are still handled by providing individual files, but\nordering traversal becomes really tricky. The directory/file case\nmight even require the pre_resolved_conflicts to be pulled out of that\nloop somehow. It'd take some investigative work, or some deep\nthought, or both.\n\n> A subsequent commit changed my hacky `merge-tree` hack to optionally\n> accept NUL-terminated merge conflict resolutions in / pairs via\n> stdin (again, avoiding files to be written where we do not _need_ spend\n> I/O unnecessarily).\n>\n> The biggest problem we faced at the Contributor Summit was that our\n> discussion was not informed by the actual server-side needs. So I would\n> like to reiterate my challenge to make that the driver. Let's not use any\n> hypothetical scenario as the basis for the design of `git merge-tree`, but\n> let's use the concrete requirements of actual server-side merges that are\n> currently implemented using libgit2, when trying to figure out what\n> functionality we need from `merge-tree`, and in what shape.\n>\n> Here is an initial list:\n>\n> - need to determine whether a merge will succeed, quickly\n>\n> - need the tree OID for a successful merge\n>\n> - need the list of items that conflict...\n\nI think my version is good up to here.\n\n> , along with OIDs and modes, if the merge failed\n\nCould you clarify what you mean here by OIDs and modes? For a given\npath, are you just looking for a (pathname, OID, mode) tuple? (In\nwhich case, you'd get the OID of a file that potentially has embedded\nconflict markers)\n\nOr are you looking for multiple OIDs and modes for each file?\n\nIf you are looking for multiple OIDs and modes for each file\n(representing where the content came from on the different sides of\nthe merge), are you looking for:\n * the OID and mode of each file that played a part in the merge resolution\nOR\n * just the OIDs and modes that would have been recorded in the index\nhad the merge been done by `git merge`?\n\n(Those last two possibilities are usually the same answer, but no they\nare not always the same. The index cannot hold all the OIDs and modes\nin various cases and we have to squash them down to three, possibly\ntossing some information that might have been of interest to the user.\nThis can even occur when you have a unique merge base.)\n\n>\n> - need a way to provide merge conflict resolutions\n>\n> And now that I wrote all this, I realize I should have sent it to the\n> `merge-tree` thread, not the `merge-tree-ort` thread...\n\nMy submission was RFC and you're providing C, so it's all good in my\nbook. I'm reading both threads. :-)\n"},{"id":"445922","messageId":"nycvar.QRO.7.76.6.2201111439190.1081@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"xmqq8rvn1nkw.fsf@gitster.g","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-11T13:47:48Z","receivedAt":"2022-01-11T13:47:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 10 Jan 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I am against a new command for what essentially serves the original\n> > purpose of `merge-tree`.\n> >\n> > The fact that `merge-tree` has not seen any work in almost 12 years is\n> > testament not only to how hard it was to disentangle the work-tree\n> > requirement from the recursive merge (it is one of my favorite\n> > counterexamples when anybody claims that you can easily prototype code in\n> > a script and then convert it to C), but the fact that there is no user\n> > within Git itself (apart from t/t4300-merge-tree.sh, which does not count)\n> > speaks volumes about the design of that `merge-tree` tool.\n> >\n> > So it's only fair to breathe life into it by letting it do what it was\n> > meant to do all along.\n>\n> My \"Yup\" would not weigh as much as one that Linus (whose original\n> merge-tree survived this long without seeing much enhancements)\n> might give us, but he is busy elsewhere so you guys have to live\n> with mine ;-)\n>\n> As to its output, I do agree that \"we give a tree when it is already\n> usable to record in a new commit\" is a valuable option to have. The\n> original behaviour should be made available somehow, for those who\n> built their workflow (including scripts) around it, though.\n\nNo, I don't think it is a good idea to keep the original behavior around\nindefinitely, when it is totally unclear whether there actually _is_ any\nuser of this feature out there.\n\nWe intentionally broke any existing users of `git-parse-remote.sh` by\nremoving it, when that feature was much more likely to be used in scripts\nthan `git merge-tree`. We cannot say on the one hand that we will get rid\nof some useful script just because we don't want the maintenance burden\nwhen on the other hand trying to keep support for an operation that is\nunlikely to have any users. That does not compute. (And don't get hung up\non the specific example of `git-parse-remote.sh`, you know there we\nshuffled around _many_ more things recently that had a good chance of\nbreaking existing users.)\n\nBesides, the original `git remote-tree` behavior will be very easy to\nrecreate using the better UI, the one that outputs tree OIDs.\n\nCiao,\nDscho\n"},{"id":"445969","messageId":"220111.86mtk2xb1y.gmgdl@evledraar.gmail.com","threadId":"57191","inReplyTo":"nycvar.QRO.7.76.6.2201111439190.1081@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T17:00:51Z","receivedAt":"2022-01-11T20:38:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 11 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Mon, 10 Jan 2022, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > I am against a new command for what essentially serves the original\n>> > purpose of `merge-tree`.\n>> >\n>> > The fact that `merge-tree` has not seen any work in almost 12 years is\n>> > testament not only to how hard it was to disentangle the work-tree\n>> > requirement from the recursive merge (it is one of my favorite\n>> > counterexamples when anybody claims that you can easily prototype code in\n>> > a script and then convert it to C), but the fact that there is no user\n>> > within Git itself (apart from t/t4300-merge-tree.sh, which does not count)\n>> > speaks volumes about the design of that `merge-tree` tool.\n>> >\n>> > So it's only fair to breathe life into it by letting it do what it was\n>> > meant to do all along.\n>>\n>> My \"Yup\" would not weigh as much as one that Linus (whose original\n>> merge-tree survived this long without seeing much enhancements)\n>> might give us, but he is busy elsewhere so you guys have to live\n>> with mine ;-)\n>>\n>> As to its output, I do agree that \"we give a tree when it is already\n>> usable to record in a new commit\" is a valuable option to have. The\n>> original behaviour should be made available somehow, for those who\n>> built their workflow (including scripts) around it, though.\n>\n> No, I don't think it is a good idea to keep the original behavior around\n> indefinitely, when it is totally unclear whether there actually _is_ any\n> user of this feature out there.\n\nFor what it's worth I've used it for some automation at an ex-employer\nat least once. Grepping through my #*git* IRC logs there's a few\nmentions of it, and likewise for StackOverflow.\n\nI'm not opposed to replacing it, and I think that probably in-the-wild\nusers of it are almost certainly just grepping for the conflict markers\nto see if there's conflicts, or parsing which files have them etc.\n\nSo if we can provide a better interface that they can use (or even make\ngit merge-tree a thin wrapper...).\n\n> We intentionally broke any existing users of `git-parse-remote.sh` by\n> removing it, when that feature was much more likely to be used in scripts\n> than `git merge-tree`. We cannot say on the one hand that we will get rid\n> of some useful script just because we don't want the maintenance burden\n> when on the other hand trying to keep support for an operation that is\n> unlikely to have any users. That does not compute. (And don't get hung up\n> on the specific example of `git-parse-remote.sh`[...]\n\nThe specific example or case really matters. We don't have some generic\nflowchart for deprecating things that applies in all cases.\n\nI removed git-parse-remote.sh in a89a2fbfccd (parse-remote: remove this\nnow-unused library, 2020-11-14) and had the opposite impression you're\nexpressing here.\n\nI.e. searching around for in-the-wild uses via search engines, stack\noverflow etc. either found nothing interesting, or just a reflection of\nourselves (i.e. generated manpages etc.).\n\nThat along with its history, i.e. it wasn't *really* meant for\nout-of-tree exposure if you look at the history of how it came about, it\nwas just made at a time when similar libraries had similar manpages\netc. added as a template.\n\nMaybe that was the right thing to do, maybe not, but it went out with\nv2.30.0 and the lack of complaints since then would seem to suggest that\nI was right that removing it wouldn't be a big deal.\n\nOf course it may have broken someone's script somewhere.\n\nBut an important distinction is that they can get it working again by\njust copy/pasting that ~100 line shell library into their own script, or\ncalling the underlying commands it was invoking themselves.\n\nWhich doesn't apply to \"git merge-tree\" unless we come up with a\nreplacement, and even then a 1=1 port of some in-the-wild code might be\nmuch harder, i.e. if it's deeply coupled to parsing the specific output\nit emits now.\n\nSo I'd personally be much more hesitant to remove or change that, but of\ncourse we might still come up with good reasons for why it's worth it,\nespecially with some advertised deprecation period.\n\nAnd isn't any doubt around that even more reason to just go with\nElijah's plan of introducing new plumbing? I.e. is it really costing us\nto just leave these \"legacy merge\" plumbing built-ins and\nmerge-recursive.c etc. in place?\n"},{"id":"445972","messageId":"CABPp-BEQ5BBEgB21hUNBuioOfMry_x2nKOLgY8=v2w8eGXrsyw@mail.gmail.com","threadId":"57191","inReplyTo":"CABPp-BHJvFx0fxobYZ2vauK=KfCLF_7So8xABLjqr9rx4SVy-w@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-11T21:15:33Z","receivedAt":"2022-01-11T21:15:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 10, 2022 at 9:59 AM Elijah Newren wrote:\n>\n> On Mon, Jan 10, 2022 at 5:49 AM Johannes Schindelin\n> wrote:\n> >\n...\n> >, therefore it expects the resolved conflicts to\n> > be in _files_. I don't think that there is a way to reasonably handle\n> > symlink target conflicts or directory/file/symlink conflicts, but there\n> > might be.\n>\n> You really need (mode,oid) pairs to be provided by the user. That\n> fixes the executable issue I mentioned above, and makes it clear how\n> to handle symlinks and file/symlink conflicts.\n>\n> directory/file are still handled by providing individual files, but\n> ordering traversal becomes really tricky. The directory/file case\n> might even require the pre_resolved_conflicts to be pulled out of that\n> loop somehow. It'd take some investigative work, or some deep\n> thought, or both.\n\nI think I came up with a solution to this during my run yesterday,\nthough I haven't tried or tested it. Instead of modifying the loop\nover plist.items, you instead add a preliminary loop over\npre_resolved_conflicts that modifies opt->priv->paths (and add this\npreliminary loop just before the items from opt->priv->paths are added\nto plist.items). In that preliminary loop, you need to make sure that\n(a) any files in pre_resolved_conflicts corresponding to existing\n_files_ in opt->priv->path result in updating that item's clean &\nis_null & mode & oid state, (b) any files in pre_resolved_conflicts\nthat correspond to existing _directories_ in opt->priv->path need the\nvalue to be expanded to be a conflict_info instead of just a\nmerged_info, you need to set the df_conflict bit, and don't update the\nmerge_info fields but do update the extended conflict_info ones, (c)\nany new files in pre_resolved_conflicts result in new entries\nopt->priv->paths, (d) any leading directories of new files in\npre_resolved_conflicts are appropriately handled, meaning: (d1) new\nopt->priv->paths are created if the directory path wasn't in\nopt->priv->paths before, (d2) a tweak to df_conflict for the directory\nitem if it previously existed in opt->priv->paths but only as a file\n(possibly also necessitating expanding from a merged_info to a\nconflict_info), (d3) no-op if the directory already existed in\nopt->priv->paths and was just a directory (and in this case, you can\nstop walking the parent directories to the toplevel).\n\nThen, after this preliminary loop that modifies opt->priv->paths, the\nrest can just proceed as-is.\n\nThat should handle new files, new directories, and all directory/file\nconflicts. Yeah, it's a bunch to look at, but directory/file\nconflicts are always a bear.\n"},{"id":"445975","messageId":"CABPp-BFei07srZBgyKs6HCm+G+hmPR-3_EkKjRK8WwGL1Uf2oA@mail.gmail.com","threadId":"57191","inReplyTo":"220111.86mtk2xb1y.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-11T22:25:56Z","receivedAt":"2022-01-11T22:26:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 11, 2022 at 12:38 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Tue, Jan 11 2022, Johannes Schindelin wrote:\n>\n> > Hi Junio,\n> >\n> > On Mon, 10 Jan 2022, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >>\n...\n> > No, I don't think it is a good idea to keep the original behavior around\n> > indefinitely, when it is totally unclear whether there actually _is_ any\n> > user of this feature out there.\n>\n> For what it's worth I've used it for some automation at an ex-employer\n> at least once. Grepping through my #*git* IRC logs there's a few\n> mentions of it, and likewise for StackOverflow.\n>\n> I'm not opposed to replacing it, and I think that probably in-the-wild\n> users of it are almost certainly just grepping for the conflict markers\n> to see if there's conflicts, or parsing which files have them etc.\n>\n> So if we can provide a better interface that they can use (or even make\n> git merge-tree a thin wrapper...).\n...\n> So I'd personally be much more hesitant to remove or change that, but of\n> course we might still come up with good reasons for why it's worth it,\n> especially with some advertised deprecation period.\n>\n> And isn't any doubt around that even more reason to just go with\n> Elijah's plan of introducing new plumbing? I.e. is it really costing us\n> to just leave these \"legacy merge\" plumbing built-ins\n\nI definitely think it's worth guiding users away from the old `git\nmerge-tree` behavior in documentation (i.e. deprecating it). That may\nalso lead towards its eventual removal, but I'm not as worried about\nthat.\n\n`git merge-recursive` was actively used in various places, including\nin `git cherry-pick`. I had used it a few times myself in a script.\nI don't see a need to deprecate it currently, which naturally would\npush its removal (if anyone is pushing for it) even further away.\n\n> and merge-recursive.c etc. in place?\n\nThis, however, is different. I have much stronger opinions here and I\ndo want to eventually remove merge-recursive.c. Definitely not now,\nbecause:\n\n * `ort` has not been the default long enough. We had one bug\nreported this cycle where ort did the wrong thing and using\n`recursive` was a good workaround. There may be a long tail here, so\nI'd like to wait a couple years for reports to trickle in.\n * `merge-recursive.c` is still hard-coded in three places in the\ncode; you can't even set a configuration option to choose merge-ort.c\nin those places: builtin/am, builtin/merge-recursive, and\nbuiltin/stash.\n\nMore details on that second point: All three of these use\nmerge_recursive_generic() and need that usage to be replaced. It's on\nmy TODO list. Although it might look like a trivial job of just\ncopying merge_recursive_generic() and replace its merge_recursive()\ncall with merge_ort_recursive(), that would only get you something\nthat would die on assert()s in ort's setup. If you removed those\nassert()s, then you'd merely be copying the bugs in\nmerge_recursive_generic() usage into the new world (and adding one new\nbug). We should instead fix the problems identified with that usage\nand those callers during the review of ort. So don't try to do this\nsimplistic translation.\n"},{"id":"445976","messageId":"nycvar.QRO.7.76.6.2201111448140.1081@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"CABPp-BHJvFx0fxobYZ2vauK=KfCLF_7So8xABLjqr9rx4SVy-w@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-11T22:30:36Z","receivedAt":"2022-01-11T22:30:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Mon, 10 Jan 2022, Elijah Newren wrote:\n\n> On Mon, Jan 10, 2022 at 5:49 AM Johannes Schindelin\n> wrote:\n> >\n> > On Fri, 7 Jan 2022, Elijah Newren wrote:\n> >\n> > > On Fri, Jan 7, 2022 at 9:58 AM Christian Couder\n> > > wrote:\n> > > >\n> > > > On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren \n> > > > wrote:\n> > > > >\n> > > > > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > > > > wrote:\n> > > >\n> ...\n> >\n> > I _bet_ it needs first and foremost the information \"is this\n> > mergeable?\".\n> >\n> > That is by far the most common question the code I saw had to answer,\n> > without any follow-up questions asked.\n> >\n> > An add-on question is then \"which files/directories conflicted?\". And\n> > there, it really only wanted the file names, along with the OIDs of\n> > the respective item in the base, the HEAD and the merge branch.\n>\n> This might be difficult to provide for some cases, e.g.\n> rename/rename(1to2), especially if that conflict gets entangled with any\n> others. Users would also have difficulty gaining these even using the\n> command line `git merge` (with either recursive or ort) for these cases.\n>\n> Also, does this presume three OIDs? What about the cases where there\n> are 4 or 6 for a given path?\n>\n> I'm a bit worried about the assumptions underlying this request, and\n> that it might not be satisfiable in general depending on what those\n> assumptions are.\n\nWell, that request is driven by the reality of a web UI.\n\nYou cannot reasonably resolve just any merge conflict in a web UI. But you\ncan easily resolve a trivial content conflict in, say, a README file,\nwithout opening a console window, cloning the repository, starting an\neditor, then saving the result after resolving the textual conflict,\ncommitting it, then trying to force-push to the original PR branch (if the\ncontributor has given you permission to push).\n\nSo what I am talking about here really is the case where no rename\nconflict happened, no directory/file conflict, no type change. Just the\ngood ole' textual conflicts where the same lines were modified in\ndivergent ways.\n\nThis means that we need the base, HEAD and MERGE OIDs here (and modes, you\nare absolutely correct).\n\n> > In my work in December, I also had to implement another thing that I\n> > think we will have to implement in `merge-tree` in one form or\n> > another: when users provided merge conflict resolutions via the web\n> > UI, we want the merge to succeed. What I implemented was this (in a\n> > nutshell, a way to provide file names with associated blob OIDs that\n> > resolve the content conflicts):\n>\n> Interesting. I'm curious, though -- if they are providing conflict\n> resolutions, then would they not have had a previous merge to find out\n> what the conflicts were? If so, wouldn't they be able to provide the\n> tree to avoid the need for a second merge?\n\nNo, the conflict resolutions are apparently stored independently from the\ncommits. Probably to allow for the same resolutions to be applied in case\nthat the PR's target branch changes but leaves the same conflicts.\n\nAgain, we are dealing with the realities of a web UI ;-)\n\nBTW I am not saying that the way I implemented it is 100% the best way to\nachieve the intended goal. It is quite possible that there is a better\nway, or that we need to at least provide the (mode,OID) triplet\ncorresponding to the conflict, too, so that the merge machinery can verify\nthat the resolution is applied to the _correct_ conflict ;-)\n\n> > -- snip --\n> > Author: Johannes Schindelin \n> > Date: Thu Dec 16 20:46:35 2021 +0100\n> > Subject: merge-ort: allow pre-resolving merge conflicts\n> >\n> > One of merge-ort's particular strengths is that it works well even in a\n> > worktree-less setting, e.g. in a backend for a server-side merge.\n> >\n> > In such a scenario, it is conceivable that the merge in question not\n> > only results in a conflict, but that the caller figures out some sort of\n> > resolution before calling the merge again. The second call, of course,\n> > is meant to apply the merge conflict resolution.\n> >\n> > With this commit, we allow just that. The new, optional\n> > `pre_resolved_conflicts` field of `struct merge_options` maps paths to\n> > the blob OID of the resolution.\n> >\n> > Signed-off-by: Johannes Schindelin \n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index a74d4251c3..fa59ce2f97 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3961,6 +3961,7 @@ static void process_entries(struct merge_options *opt,\n> > prefetch_for_content_merges(opt, &plist);\n> > for (entry = &plist.items[plist.nr-1]; entry >= plist.items; --entry) {\n> > char *path = entry->string;\n> > + struct object_id *resolved_oid;\n> > /*\n> > * NOTE: mi may actually be a pointer to a conflict_info, but\n> > * we have to check mi->clean first to see if it's safe to\n> > @@ -3972,7 +3973,17 @@ static void process_entries(struct merge_options *opt,\n> > &dir_metadata);\n> > if (mi->clean)\n> > record_entry_for_tree(&dir_metadata, path, mi);\n> > - else {\n> > + else if (opt->pre_resolved_conflicts &&\n> > + (resolved_oid =\n> > + strmap_get(opt->pre_resolved_conflicts, path))) {\n>\n> You have a couple problematic assumptions here:\n>\n> * What if the user's resolution required fixing a semantic conflict,\n> meaning they needed to modify a file that had no conflicts? Your code\n> here would ignore those, because the clean case is handled above.\n>\n> * What if the user's resolution required adding a totally new file\n> (either because a rename is handled as a separate add/delete, or they\n> just needed a new file)? The loop above isn't over items in\n> pre_resolved_conflicts, it just assumes that items in\n> pre_resolved_conflicts are also in the plist.items being looped over.\n\nI can see how these assumptions may look ludicrous when coming from the\ncommand-line. But again, we are talking about the realities of a conflict\nresolution via a web UI.\n\nSo I think that it is out of the question to address non-textual conflicts\nin this scenario. And then the assumptions make much more sense.\n\n>\n> > + mi->clean = 1;\n> > + mi->is_null = 0;\n> > + memcpy(&mi->result.oid, resolved_oid,\n> > + sizeof(*resolved_oid));\n>\n> And here's another:\n>\n> * What if the provided resolution was a deletion of the file in\n> question (especially after e.g. a modify/delete or rename/delete\n> conflict)? Don't you need to have a special check for the zero_oid\n> here?\n>\n> And if I combine the things above:\n>\n> * What if the user wants to remove a file and add a directory of\n> files in its place at some location in order to resolve things?\n> Granted, you didn't handle either deletes or new files above, but if\n> you did add both, then you might have this issue. The code at this\n> point used some very carefully constructed logic and order of walking\n> items specifically to handle file/delete conflicts correctly. I'm\n> worried that your conflict resolution stuff here might interact badly\n> with that.\n>\n> > + if (!mi->result.mode)\n> > + mi->result.mode = 0100644;\n>\n> How do you know it's not supposed to be 0100755?\n\nI don't ;-)\n\nThis was a proof-of-concept, and I meant to look into this a bit further,\ntrying to figure out from where I could get this information, but I never\ngot to that.\n\nNow that I think about it, the best solution would probably be for the\nmode to be provided in addition to the blob OID, so that the caller has to\ndecide.\n\n> > + record_entry_for_tree(&dir_metadata, path, mi);\n> > + } else {\n> > struct conflict_info *ci = (struct conflict_info *)mi;\n> > process_entry(opt, path, ci, &dir_metadata);\n> > }\n> > diff --git a/merge-recursive.h b/merge-recursive.h\n> > index 0795a1d3ec..1f45effdd0 100644\n> > --- a/merge-recursive.h\n> > +++ b/merge-recursive.h\n> > @@ -47,6 +47,13 @@ struct merge_options {\n> > const char *subtree_shift;\n> > unsigned renormalize : 1;\n> >\n> > + /*\n> > + * (merge-ORT only) If non-NULL, contains a map from `path` to OID. If\n> > + * the given `path would be marked as conflict, it is instead resolved\n> > + * to the specified blob.\n> > + */\n> > + struct strmap *pre_resolved_conflicts;\n> > +\n> > /* internal fields used by the implementation */\n> > struct merge_options_internal *priv;\n> > };\n> > -- snap --\n> >\n> > It is a proof-of-concept\n>\n> Fair enough.\n>\n> >, therefore it expects the resolved conflicts to\n> > be in _files_. I don't think that there is a way to reasonably handle\n> > symlink target conflicts or directory/file/symlink conflicts, but there\n> > might be.\n>\n> You really need (mode,oid) pairs to be provided by the user. That\n> fixes the executable issue I mentioned above, and makes it clear how\n> to handle symlinks and file/symlink conflicts.\n\nIt's a really good point, too.\n\n> directory/file are still handled by providing individual files, but\n> ordering traversal becomes really tricky. The directory/file case\n> might even require the pre_resolved_conflicts to be pulled out of that\n> loop somehow. It'd take some investigative work, or some deep\n> thought, or both.\n\nMy idea for directory/file conflicts is to essentially teach the web UI to\nthrow its hands in the air upon encountering them, and telling the user\nthat it's not possible to resolve these types of conflicts via the UI.\n\nAnd since my principal driver here is the concrete need for such a\nserver-side merge with conflict resolution, that's as far as I want to\npush `merge-tree`, too, and leave any more complicated resolution to a\nfuture date, or never, whichever comes first.\n\n> > A subsequent commit changed my hacky `merge-tree` hack to optionally\n> > accept NUL-terminated merge conflict resolutions in / pairs via\n> > stdin (again, avoiding files to be written where we do not _need_ spend\n> > I/O unnecessarily).\n> >\n> > The biggest problem we faced at the Contributor Summit was that our\n> > discussion was not informed by the actual server-side needs. So I would\n> > like to reiterate my challenge to make that the driver. Let's not use any\n> > hypothetical scenario as the basis for the design of `git merge-tree`, but\n> > let's use the concrete requirements of actual server-side merges that are\n> > currently implemented using libgit2, when trying to figure out what\n> > functionality we need from `merge-tree`, and in what shape.\n> >\n> > Here is an initial list:\n> >\n> > - need to determine whether a merge will succeed, quickly\n> >\n> > - need the tree OID for a successful merge\n> >\n> > - need the list of items that conflict...\n>\n> I think my version is good up to here.\n\nYes!\n\nThe only thing I would change is to not even bother providing a tree\nobject in case of conflicts. Either we provide it, and expect the user to\nsomehow reconstruct the conflict types from there, or we simply don't say\nanything about the tree whose (transitive) file contents may or may not\ncontain conflict markers (or not, think: binary files). Providing a tree\nobject in case of a failed merge isn't helpful IMO.\n\n> > , along with OIDs and modes, if the merge failed\n>\n> Could you clarify what you mean here by OIDs and modes? For a given\n> path, are you just looking for a (pathname, OID, mode) tuple? (In\n> which case, you'd get the OID of a file that potentially has embedded\n> conflict markers)\n>\n> Or are you looking for multiple OIDs and modes for each file?\n\nThis. In case of a conflict, I am looking for (mode,OID) for the merge\nbase (which _can_ be a virtual one in case of recursive merges) as well as\nfor the two divergent revisions that were supposed to be merged.\n\nI do realize that other types of conflicts can occur, such as a\nrename/rename conflict, and we would need a way to represent these in the\noutput, too. But in such an instance, where no clear (mode,OID) triplet\ncan be provided that represents the merge conflicts for this file, the\ncurrent web UI cannot offer a way to resolve the conflict, so I am a bit\nless interested in that scenario.\n\n> If you are looking for multiple OIDs and modes for each file\n> (representing where the content came from on the different sides of\n> the merge), are you looking for:\n> * the OID and mode of each file that played a part in the merge resolution\n> OR\n> * just the OIDs and modes that would have been recorded in the index\n> had the merge been done by `git merge`?\n\nI think the latter was my idea.\n\nBut again, you made me think of rename/rename conflicts and friends, and\nwe would need a way to represent those, too. Even if my current use case\nwould need to only detect their presence in order to say \"nope, can't\nresolve that on the web\".\n\n> (Those last two possibilities are usually the same answer, but no they\n> are not always the same. The index cannot hold all the OIDs and modes\n> in various cases and we have to squash them down to three, possibly\n> tossing some information that might have been of interest to the user.\n> This can even occur when you have a unique merge base.)\n>\n> >\n> > - need a way to provide merge conflict resolutions\n> >\n> > And now that I wrote all this, I realize I should have sent it to the\n> > `merge-tree` thread, not the `merge-tree-ort` thread...\n>\n> My submission was RFC and you're providing C, so it's all good in my\n> book. I'm reading both threads. :-)\n\n:-)\n\nThank you so much!\nDscho\n"},{"id":"445981","messageId":"CABPp-BGdbh=HM7jA+_RTwSWVcMr_qvEY7RoNXooeBT2NB4Ubzw@mail.gmail.com","threadId":"57191","inReplyTo":"nycvar.QRO.7.76.6.2201111448140.1081@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-12T00:41:07Z","receivedAt":"2022-01-12T00:41:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Tue, Jan 11, 2022 at 2:30 PM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Mon, 10 Jan 2022, Elijah Newren wrote:\n>\n> > On Mon, Jan 10, 2022 at 5:49 AM Johannes Schindelin\n> > wrote:\n> > >\n> > > On Fri, 7 Jan 2022, Elijah Newren wrote:\n> > >\n> > > > On Fri, Jan 7, 2022 at 9:58 AM Christian Couder\n> > > > wrote:\n> > > > >\n> > > > > On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren \n> > > > > wrote:\n> > > > > >\n> > > > > > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder\n> > > > > > wrote:\n> > > > >\n> > ...\n> > >\n> > > I _bet_ it needs first and foremost the information \"is this\n> > > mergeable?\".\n> > >\n> > > That is by far the most common question the code I saw had to answer,\n> > > without any follow-up questions asked.\n> > >\n> > > An add-on question is then \"which files/directories conflicted?\". And\n> > > there, it really only wanted the file names, along with the OIDs of\n> > > the respective item in the base, the HEAD and the merge branch.\n> >\n> > This might be difficult to provide for some cases, e.g.\n> > rename/rename(1to2), especially if that conflict gets entangled with any\n> > others. Users would also have difficulty gaining these even using the\n> > command line `git merge` (with either recursive or ort) for these cases.\n> >\n> > Also, does this presume three OIDs? What about the cases where there\n> > are 4 or 6 for a given path?\n> >\n> > I'm a bit worried about the assumptions underlying this request, and\n> > that it might not be satisfiable in general depending on what those\n> > assumptions are.\n>\n> Well, that request is driven by the reality of a web UI.\n>\n> You cannot reasonably resolve just any merge conflict in a web UI. But you\n> can easily resolve a trivial content conflict in, say, a README file,\n> without opening a console window, cloning the repository, starting an\n> editor, then saving the result after resolving the textual conflict,\n> committing it, then trying to force-push to the original PR branch (if the\n> contributor has given you permission to push).\n>\n> So what I am talking about here really is the case where no rename\n> conflict happened, no directory/file conflict, no type change. Just the\n> good ole' textual conflicts where the same lines were modified in\n> divergent ways.\n\nSure, but we were talking about needs for `git merge-tree`. You are\nsaying we need more information out of it (OIDs and modes) to fullfill\nuser requests. That's totally reasonable. But stating that you don't\ncare about certain types of conflicts or cases where renames happen\ndoesn't really help answer my question; while that's a restriction you\ncan put on your internal implementation, other users of `merge-tree`\ndoing the exact same type of thing (even involving a web UI) might\ndecide they also want to handle the other types of conflicts. I don't\nwant merge-tree to have a design only capable of handling unrenamed\nfiles, or only textual conflicts, since the new mode for merge-tree\nwas all about handling the complexity of a real merge.\n\n> This means that we need the base, HEAD and MERGE OIDs here (and modes, you\n> are absolutely correct).\n\nNow you're back to requiring three again. What about modify/delete\nconflicts where there are only two available?\n\nYour stated requirements either lack precision about what you really\nwant (e.g. \"the OIDs and modes that would have appeared in the higher\norder stages in the index for the given path\") or aren't satisfiable.\nI'm against providing anything that knowingly paints us into a corner.\n\n...\n> > > @@ -3972,7 +3973,17 @@ static void process_entries(struct merge_options *opt,\n> > > &dir_metadata);\n> > > if (mi->clean)\n> > > record_entry_for_tree(&dir_metadata, path, mi);\n> > > - else {\n> > > + else if (opt->pre_resolved_conflicts &&\n> > > + (resolved_oid =\n> > > + strmap_get(opt->pre_resolved_conflicts, path))) {\n> >\n> > You have a couple problematic assumptions here:\n> >\n> > * What if the user's resolution required fixing a semantic conflict,\n> > meaning they needed to modify a file that had no conflicts? Your code\n> > here would ignore those, because the clean case is handled above.\n> >\n> > * What if the user's resolution required adding a totally new file\n> > (either because a rename is handled as a separate add/delete, or they\n> > just needed a new file)? The loop above isn't over items in\n> > pre_resolved_conflicts, it just assumes that items in\n> > pre_resolved_conflicts are also in the plist.items being looped over.\n>\n> I can see how these assumptions may look ludicrous when coming from the\n> command-line. But again, we are talking about the realities of a conflict\n> resolution via a web UI.\n>\n> So I think that it is out of the question to address non-textual conflicts\n> in this scenario. And then the assumptions make much more sense.\n\nWaving your hands and saying it came from a web UI doesn't reduce my\nworries or concerns at all. I could easily imagine a web UI that\nallows users to specify other modifications they want to make, even\nlimited to textual ones, to include in the merge. What happens when\nthey modify some file that had otherwise merged cleanly (e.g. a file\nthat gains a new call to some function, which after the merge needs an\nextra parameter passed to it)? Your solution doesn't handle it; it'd\nsend that user-specified change to /dev/null.\n\nTo be fair, you mentioned below that this is just a proof of concept,\nand that certainly reduces worries/concerns. It's totally fine to\nhave such things. Maybe you intend to keep this patch internal\nindefinitely. That's fine too. My commentary is just feedback on if\nwe want merge-ort/merge-tree extended more in this kind of fashion\n(and it might also serve as useful pointers on how to extend your\ninternal patch if you get requests to extend your web UI a bit to\nhandle more cases). :-)\n\n> > > + mi->clean = 1;\n> > > + mi->is_null = 0;\n> > > + memcpy(&mi->result.oid, resolved_oid,\n> > > + sizeof(*resolved_oid));\n> >\n> > And here's another:\n> >\n> > * What if the provided resolution was a deletion of the file in\n> > question (especially after e.g. a modify/delete or rename/delete\n> > conflict)? Don't you need to have a special check for the zero_oid\n> > here?\n> >\n> > And if I combine the things above:\n> >\n> > * What if the user wants to remove a file and add a directory of\n> > files in its place at some location in order to resolve things?\n> > Granted, you didn't handle either deletes or new files above, but if\n> > you did add both, then you might have this issue. The code at this\n> > point used some very carefully constructed logic and order of walking\n> > items specifically to handle file/delete conflicts correctly. I'm\n> > worried that your conflict resolution stuff here might interact badly\n> > with that.\n> >\n> > > + if (!mi->result.mode)\n> > > + mi->result.mode = 0100644;\n> >\n> > How do you know it's not supposed to be 0100755?\n>\n> I don't ;-)\n>\n> This was a proof-of-concept, and I meant to look into this a bit further,\n> trying to figure out from where I could get this information, but I never\n> got to that.\n>\n> Now that I think about it, the best solution would probably be for the\n> mode to be provided in addition to the blob OID, so that the caller has to\n> decide.\n\n+1.\n\n...\n> > directory/file are still handled by providing individual files, but\n> > ordering traversal becomes really tricky. The directory/file case\n> > might even require the pre_resolved_conflicts to be pulled out of that\n> > loop somehow. It'd take some investigative work, or some deep\n> > thought, or both.\n>\n> My idea for directory/file conflicts is to essentially teach the web UI to\n> throw its hands in the air upon encountering them, and telling the user\n> that it's not possible to resolve these types of conflicts via the UI.\n>\n> And since my principal driver here is the concrete need for such a\n> server-side merge with conflict resolution, that's as far as I want to\n> push `merge-tree`, too, and leave any more complicated resolution to a\n> future date, or never, whichever comes first.\n\n:-)\n\n> > > A subsequent commit changed my hacky `merge-tree` hack to optionally\n> > > accept NUL-terminated merge conflict resolutions in / pairs via\n> > > stdin (again, avoiding files to be written where we do not _need_ spend\n> > > I/O unnecessarily).\n> > >\n> > > The biggest problem we faced at the Contributor Summit was that our\n> > > discussion was not informed by the actual server-side needs. So I would\n> > > like to reiterate my challenge to make that the driver. Let's not use any\n> > > hypothetical scenario as the basis for the design of `git merge-tree`, but\n> > > let's use the concrete requirements of actual server-side merges that are\n> > > currently implemented using libgit2, when trying to figure out what\n> > > functionality we need from `merge-tree`, and in what shape.\n> > >\n> > > Here is an initial list:\n> > >\n> > > - need to determine whether a merge will succeed, quickly\n> > >\n> > > - need the tree OID for a successful merge\n> > >\n> > > - need the list of items that conflict...\n> >\n> > I think my version is good up to here.\n>\n> Yes!\n\n:-)\n\n> The only thing I would change is to not even bother providing a tree\n> object in case of conflicts. Either we provide it, and expect the user to\n> somehow reconstruct the conflict types from there, or we simply don't say\n> anything about the tree whose (transitive) file contents may or may not\n> contain conflict markers (or not, think: binary files). Providing a tree\n> object in case of a failed merge isn't helpful IMO.\n\nYou've got a false dichotomy here. Providing a tree object does NOT\nimply that we expect users to reconstruct a list of conflicted files\nor the types of conflicts from that tree. I pretty strongly argued\nagainst that, in fact (see [*], , starting at \"???\" -- and yes, I mean\na literal triple question mark).\n\nI also don't see why you are so bothered by the tree object being\nprinted. I already pointed out that we can't easily avoid computing\nthe tree object without risking hurting performance more than you're\ntrying to help it (again, see [*]).\n\nBut beyond performance, by trying to require that the tree object not\nbe printed, you are trying to prevent users from getting information\nthat they simply cannot obtain any other way. Even if `git\nmerge-tree` provided a list of up to three (stage, mode, oid) triplets\nfor each conflicted filename corresponding to what you'd find in the\nindex in the higher order stages (which sounds like a totally\nreasonable request, by the way), you could not reconstruct the\ninformation available in the tree for the same conflicted paths.\nSure, you could get \"close\", and for some uses, maybe that's good\nenough, but most users I've ever seen prefer the file in the working\ncopy after a merge for trying to resolve conflicts over the (stage,\nmode, oid) pairs found in the index. And since file in the tree\npotentially has some information not reconstructable from the index, I\ndon't see why we should avoid printing 41 characters to stdout to\nallow the user to make use of it. If it's not useful to you,\nwhatever, toss those 41 printed bytes.\n\n[*] https://lore.kernel.org/git/CABPp-BHvXrP0sTTmuTYfACoJTCcm9+wk_f441nj4TstrmQdqMQ@mail.gmail.com/\n\n> > > , along with OIDs and modes, if the merge failed\n> >\n> > Could you clarify what you mean here by OIDs and modes? For a given\n> > path, are you just looking for a (pathname, OID, mode) tuple? (In\n> > which case, you'd get the OID of a file that potentially has embedded\n> > conflict markers)\n> >\n> > Or are you looking for multiple OIDs and modes for each file?\n>\n> This. In case of a conflict, I am looking for (mode,OID) for the merge\n> base (which _can_ be a virtual one in case of recursive merges) as well as\n> for the two divergent revisions that were supposed to be merged.\n>\n> I do realize that other types of conflicts can occur, such as a\n> rename/rename conflict, and we would need a way to represent these in the\n> output, too. But in such an instance, where no clear (mode,OID) triplet\n> can be provided that represents the merge conflicts for this file, the\n> current web UI cannot offer a way to resolve the conflict, so I am a bit\n> less interested in that scenario.\n\nOkay, this is helping explain a bit better.\n\nOut of curiosity, does this mean the web UI only can resolve cases\nwhere all three modes & OIDs are present, and the files are text\nrather than binary? For example, does this mean the web UI cannot\nhandle cases like modify/delete or add/add?\n\n> > If you are looking for multiple OIDs and modes for each file\n> > (representing where the content came from on the different sides of\n> > the merge), are you looking for:\n> > * the OID and mode of each file that played a part in the merge resolution\n> > OR\n> > * just the OIDs and modes that would have been recorded in the index\n> > had the merge been done by `git merge`?\n>\n> I think the latter was my idea.\n\nAh, perfect; that's something we can provide!\n\n> But again, you made me think of rename/rename conflicts and friends, and\n> we would need a way to represent those, too. Even if my current use case\n> would need to only detect their presence in order to say \"nope, can't\n> resolve that on the web\".\n\nBut now you're making me worry whether I can satisfy your requests\nagain, or at minimum, whether I need to do a lot more work in\nmerge-ort to answer them. Do you need a representation other than the\nlist of (stage, mode, oid) triplets?\n\nThat might be hard to come by, especially for cases like mod6 of\nt6422. The code basically assumes there's going to be 0 or more\nconflict types at each path (and yes, you can absolutely have more\nthan 1), but doesn't store these conflict types other than in terms of\noutputting various \"CONFLICT (type): \" strings (though it\ndoes keep the output messages in memory and associated with each file\nbefore printing them at the end). Further, while some conflict types\nwhich could be considered to affect multiple paths (e.g. rename/rename\nwhere both sides rename the same file differently), they are only\nstored once relative to only one of the filenames to avoid printing\nthe same conflict notice three times. (For example, in the\nrename/rename case, the conflict message is associated with the base\nfilename).\n\nIf you do only need the list of (stage, mode, oid) triplets, then that\ncan be provided for every file. However, things like\nrename/rename(1to2) will only provide a single (stage, mode, oid)\ntriplet for the oldfilename, and also only provide one triplet for the\nnew filename on side1, and also only provide one triplet for the new\nfilename on side2. I don't know if it'll confuse users why they have\na conflict for a file with only a single triplet.\n\n> > > And now that I wrote all this, I realize I should have sent it to the\n> > > `merge-tree` thread, not the `merge-tree-ort` thread...\n> >\n> > My submission was RFC and you're providing C, so it's all good in my\n> > book. I'm reading both threads. :-)\n>\n> :-)\n\nI'm a little worried I might come across sounding a little picky since\nI tend to dive into details and really fixate on them. I apologize in\nadvance if it sounds that way at all; you provide lots of good points\nto think about and I can't help but dive into the quirks surrounding\neach. I really appreciate all the awesome feedback and food for\nthought.\n"},{"id":"446032","messageId":"xmqqy23kx2k5.fsf@gitster.g","threadId":"57191","inReplyTo":"220111.86mtk2xb1y.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-12T17:54:18Z","receivedAt":"2022-01-12T17:54:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Maybe that was the right thing to do, maybe not, but it went out with\n> v2.30.0 and the lack of complaints since then would seem to suggest that\n> I was right that removing it wouldn't be a big deal.\n>\n> Of course it may have broken someone's script somewhere.\n>\n> But an important distinction is that they can get it working again by\n> just copy/pasting that ~100 line shell library into their own script, or\n> calling the underlying commands it was invoking themselves.\n\nWas parse-remote a part of what promised our end-users? merge-tree\nhas been since it was written. \n\nThat distinction is even more important.\n"},{"id":"446033","messageId":"xmqqk0f4x20a.fsf@gitster.g","threadId":"57191","inReplyTo":"CABPp-BFei07srZBgyKs6HCm+G+hmPR-3_EkKjRK8WwGL1Uf2oA@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-12T18:06:13Z","receivedAt":"2022-01-12T18:06:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> And isn't any doubt around that even more reason to just go with\n>> Elijah's plan of introducing new plumbing? I.e. is it really costing us\n>> to just leave these \"legacy merge\" plumbing built-ins\n>\n> I definitely think it's worth guiding users away from the old `git\n> merge-tree` behavior in documentation (i.e. deprecating it). That may\n> also lead towards its eventual removal, but I'm not as worried about\n> that.\n\nYup, promising users that we will remove it and telling them that\nthey should migrate away from it is necessary. Doing anything else\nis simply irresponsible.\n\nI however suspect that Ævar didn't mean by \"legacy merge plumbing\nbuilt-in\" the strategy backends. IOW, I had an impression that what\nis on the chopping block is merge-tree and not merge-recursive.\n\nBut since you brought up deprecation of recursive, let's spend a few\nminutes on the topic.\n\n> `git merge-recursive` was actively used in various places, including\n> in `git cherry-pick`. I had used it a few times myself in a script.\n> I don't see a need to deprecate it currently, which naturally would\n> push its removal (if anyone is pushing for it) even further away.\n\nI suspect that we may be able to just invoke ort when recursive is\ninvoked, and such a wrapping may even be easier than wrapping \"git\nblame\" to replace \"git annotate\" (where a command line option or two\nneeds to change behaviour). I doubt there is -X\nthat affects recursive that ORT does not understand, so it may be\nquite simple to deprecate \"merge -s recursive\".\n\nAs you say, replacing the internal implementation is a different\nmatter.\n\n> * `merge-recursive.c` is still hard-coded in three places in the\n> code; you can't even set a configuration option to choose merge-ort.c\n> in those places: builtin/am, builtin/merge-recursive, and\n> builtin/stash.\n>\n> More details on that second point: All three of these use\n> merge_recursive_generic() and need that usage to be replaced. It's on\n> my TODO list.\n\nYes, I do recall mentioning that when we were reviewing the series\nthat added ort.\n"},{"id":"446068","messageId":"CABPp-BHQdkhAEmTrtc+XMgj5A5ASBVRw0_bXH10NSrMsyRK+oA@mail.gmail.com","threadId":"57191","inReplyTo":"xmqqk0f4x20a.fsf@gitster.g","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-12T20:06:38Z","receivedAt":"2022-01-12T20:07:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 12, 2022 at 10:06 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n...\n> I however suspect that Ævar didn't mean by \"legacy merge plumbing\n> built-in\" the strategy backends. IOW, I had an impression that what\n> is on the chopping block is merge-tree and not merge-recursive.\n>\n> But since you brought up deprecation of recursive, let's spend a few\n> minutes on the topic.\n\nNot sure it matters, but for reference, Ævar explicitly brought up\nmerge-recursive.c. The fuller quote was:\n\n> >> I.e. is it really costing us\n> >> to just leave these \"legacy merge\" plumbing built-ins and\n> >> merge-recursive.c etc. in place?\n\nBecause he brought it up, I decided to address it. It was unclear to\nme whether he meant builtin/merge-recursive.c or the toplevel\nmerge-recursive.c, so I just addressed both.\n\n> I suspect that we may be able to just invoke ort when recursive is\n> invoked, and such a wrapping may even be easier than wrapping \"git\n> blame\" to replace \"git annotate\" (where a command line option or two\n> needs to change behaviour).\n\nYes, that was the plan I had.\n\n> I doubt there is -X\n> that affects recursive that ORT does not understand,\n\nTechnically there are currently two. As documented in\nmerge-strategies.txt, ORT takes the same -X options as recursive, but\ncurrently ignores both -Xdiff-algorithm (instead passing\nHISTOGRAM_DIFF down to ll_merge()), and -Xno-renames (instead passing\nDIFF_DETECT_RENAME down to diffcore_rename_extended()). I guess there\nare three if you count -Xpatience separately from\n-Xdiff-algorithm=patience.\n\n> so it may be quite simple to deprecate \"merge -s recursive\".\n\nYes...but why deprecate? I thought the plan was to (eventually) make\nrequests for either `recursive` or `ort` be handled by running the\n`ort` backend. Making that kind of switch is much smaller than the\none we already made to switch the default backend from `recursive` to\n`ort`, so I'm not sure I see what we gain by doing such a switch in\nstages. Maybe I'm missing something?\n"},{"id":"446098","messageId":"xmqqk0f4jhh8.fsf@gitster.g","threadId":"57191","inReplyTo":"CABPp-BHQdkhAEmTrtc+XMgj5A5ASBVRw0_bXH10NSrMsyRK+oA@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T06:08:03Z","receivedAt":"2022-01-13T06:08:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> so it may be quite simple to deprecate \"merge -s recursive\".\n>\n> Yes...but why deprecate? I thought the plan was to (eventually) make\n> requests for either `recursive` or `ort` be handled by running the\n> `ort` backend. Making that kind of switch is much smaller than the\n> one we already made to switch the default backend from `recursive` to\n> `ort`, so I'm not sure I see what we gain by doing such a switch in\n> stages. Maybe I'm missing something?\n\nDidn't we \"deprecate\" but still indefinitely support \"annotate\"? I\nhave been assuming that recursive will be in that category after ort\nestablishes itself.\n"},{"id":"446109","messageId":"CABPp-BGZrcdAw369snSi3EOMcEFP77KUcfux1VNG5f2xorzGQA@mail.gmail.com","threadId":"57191","inReplyTo":"xmqqk0f4jhh8.fsf@gitster.g","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-13T08:01:47Z","receivedAt":"2022-01-13T08:02:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 12, 2022 at 10:08 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> >> so it may be quite simple to deprecate \"merge -s recursive\".\n> >\n> > Yes...but why deprecate? I thought the plan was to (eventually) make\n> > requests for either `recursive` or `ort` be handled by running the\n> > `ort` backend. Making that kind of switch is much smaller than the\n> > one we already made to switch the default backend from `recursive` to\n> > `ort`, so I'm not sure I see what we gain by doing such a switch in\n> > stages. Maybe I'm missing something?\n>\n> Didn't we \"deprecate\" but still indefinitely support \"annotate\"? I\n> have been assuming that recursive will be in that category after ort\n> establishes itself.\n\nBut in that case, we suggested folks use \"blame\" instead of \"annotate\", right?\n\nFrom a user point of view, my intention was that \"ort\" is just the\nnewer version of \"recursive\" (which happens to be an essentially total\nrewrite). ort only got a separate name because we wanted a special\ntesting period and a way for users to select the old version of\nrecursive or the new version of recursive. Of course, that period is\nmultiple release cycles, so both names will persist, but they'll just\nbe aliases.\n\nOnce we switch \"recursive\" to mean \"ort\" so they both run the exact\nsame code, I was intending to have the documentation prefer the term\n\"recursive\" over \"ort\" since it is a more meaningful user-facing name.\n"},{"id":"446112","messageId":"220113.86k0f4vuz5.gmgdl@evledraar.gmail.com","threadId":"57191","inReplyTo":"CABPp-BHQdkhAEmTrtc+XMgj5A5ASBVRw0_bXH10NSrMsyRK+oA@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T09:26:57Z","receivedAt":"2022-01-13T09:35:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 12 2022, Elijah Newren wrote:\n\n> On Wed, Jan 12, 2022 at 10:06 AM Junio C Hamano wrote:\n>>\n>> Elijah Newren writes:\n>>\n> ...\n>> I however suspect that Ævar didn't mean by \"legacy merge plumbing\n>> built-in\" the strategy backends. IOW, I had an impression that what\n>> is on the chopping block is merge-tree and not merge-recursive.\n>>\n>> But since you brought up deprecation of recursive, let's spend a few\n>> minutes on the topic.\n>\n> Not sure it matters, but for reference, Ævar explicitly brought up\n> merge-recursive.c. The fuller quote was:\n>\n>> >> I.e. is it really costing us\n>> >> to just leave these \"legacy merge\" plumbing built-ins and\n>> >> merge-recursive.c etc. in place?\n>\n> Because he brought it up, I decided to address it. It was unclear to\n> me whether he meant builtin/merge-recursive.c or the toplevel\n> merge-recursive.c, so I just addressed both.\n\nFWIW what I meant (but clearly didn't make clear enough) is whether we'd\ndeprecate the git-merge-tree(1) command, not whatever powers it under\nthe hood.\n\nI.e. I took the greater discussion here to mean (but may have\nmisunderstood it) that we were talking about the needs for a\nlibgit2-replacing merge plumbing.\n\nThe existing git-merge-tree command probably gets us 5% towards that,\nand I can see how being bug-for-bug compatible with it might be\ninconvenient in some future on-top-of-ort rewrite and extension of it.\n\nSo we probably SHOULD keep it, but I don't think it's a MUST. I.e. if\nyou/someone wrote some more powerful version of it, and keeping it\nbecame hard to support I think it would be OK to transition/deprecate\nit, as presumably its existing users wouldn't be too inconvenienced, or\nwould be happier with the more powerful plumbing tool.\n"},{"id":"446113","messageId":"220113.86fspsvu6v.gmgdl@evledraar.gmail.com","threadId":"57191","inReplyTo":"xmqqy23kx2k5.fsf@gitster.g","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T09:22:05Z","receivedAt":"2022-01-13T09:52:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 12 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Maybe that was the right thing to do, maybe not, but it went out with\n>> v2.30.0 and the lack of complaints since then would seem to suggest that\n>> I was right that removing it wouldn't be a big deal.\n>>\n>> Of course it may have broken someone's script somewhere.\n>>\n>> But an important distinction is that they can get it working again by\n>> just copy/pasting that ~100 line shell library into their own script, or\n>> calling the underlying commands it was invoking themselves.\n>\n> Was parse-remote a part of what promised our end-users? [...]\n\nIt was listed under \"LOW-LEVEL COMMANDS (PLUMBING)\" => \"Syncing\nrepositories\", which has git-daemon, git-{upload,receive}-pack,\ngit-shell etc. now.\n\nSo, pedantically we removed a removed a plumbing utility without much if\nany warning.\n\nBut as I argued when it was removed I think that realistically some of\nour plumbing tools were made for internal-only use, and as the *.sh->C\nrewriting of built-ins progressed they became orphaned.\n\nThey only had public-facing manpages due to plans that never came to\nfruition, or just in copy/pasting an existing template at the time.\n\nI don't think that would matter for git-parse-remote if it had\nsubsequently gotted widespread use in the wild (even if it were\nundocumented), but when I investigated that it really seemed to be used\nby approximately nobody.\n\nBut a while after it was removed you pushed back on some further similar\nchanges to git-sh-setup. I asked some follow-up questions in\nhttps://lore.kernel.org/git/87tuiwjfvi.fsf@evledraar.gmail.com/ about\nhow we should consider these that you didn't reply to.\n\nThe greater context being that I was removing git-parse-remote.sh so\nthat I could eventually get rid of the bridge of extending\nlibintl/gettext to *.sh-land. The current state of that on \"master\" in\nthat regard being:\n \n $ git grep '\\b(eval_)?gettext(ln)?\\b' -- ':!t/' ':!ci/' ':!git-gui' ':!git-sh-i18n.sh' '*.sh'\n git-merge-octopus.sh: gettextln \"Error: Your local changes to the following files would be overwritten by merge\"\n git-merge-octopus.sh: gettextln \"Automated merge did not work.\"\n git-merge-octopus.sh: gettextln \"Should not be doing an octopus.\"\n git-merge-octopus.sh: die \"$(eval_gettext \"Unable to find common commit with \\$pretty_name\")\"\n git-merge-octopus.sh: eval_gettextln \"Already up to date with \\$pretty_name\"\n git-merge-octopus.sh: eval_gettextln \"Fast-forwarding to: \\$pretty_name\"\n git-merge-octopus.sh: eval_gettextln \"Trying simple merge with \\$pretty_name\"\n git-merge-octopus.sh: gettextln \"Simple merge did not work, trying automatic merge.\"\n git-sh-setup.sh:# Source git-sh-i18n for gettext support.\n git-sh-setup.sh: die \"$(eval_gettext \"usage: \\$dashless \\$USAGE\")\"\n git-sh-setup.sh: LONG_USAGE=\"$(eval_gettext \"usage: \\$dashless \\$USAGE\")\"\n git-sh-setup.sh: LONG_USAGE=\"$(eval_gettext \"usage: \\$dashless \\$USAGE\n git-sh-setup.sh: gettextln \"Cannot chdir to \\$cdup, the toplevel of the working tree\" >&2\n git-sh-setup.sh: die \"$(eval_gettext \"fatal: \\$program_name cannot be used without a working tree.\")\"\n git-sh-setup.sh: die \"$(eval_gettext \"fatal: \\$program_name cannot be used without a working tree.\")\"\n git-sh-setup.sh: gettextln \"Cannot rewrite branches: You have unstaged changes.\" >&2\n git-sh-setup.sh: eval_gettextln \"Cannot \\$action: You have unstaged changes.\" >&2\n git-sh-setup.sh: eval_gettextln \"Cannot \\$action: Your index contains uncommitted changes.\" >&2\n git-sh-setup.sh: gettextln \"Additionally, your index contains uncommitted changes.\" >&2\n git-sh-setup.sh: gettextln \"You need to run this command from the toplevel of the working tree.\" >&2\n git-sh-setup.sh: gettextln \"Unable to determine absolute path of git directory\" >&2\n git-submodule.sh: die \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n git-submodule.sh: die \"fatal: $(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n git-submodule.sh: die \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n git-submodule.sh: die_msg=\"fatal: $(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n\nI.e. if we were able to get rid of that we could remove\nsh-i18n--envsubst.c and git-sh-i18n.sh itself.\n\nSome of that is dead code, others have pending *.sh->C rewrites. For the\nrest we could expose a trivial git-i18n.c helper to emit the <10\nmessages that remained pending further rewrites, which would be much\nsimpler than extending the generic libintl functionality to *.sh.\n\nBut since git-sh-i18n.sh latter is publicly documented as plumbing\n(which I'm responsible for, merely by copy/pasting an existing template)\nI stalled on that. Since you seemed to suggest in the linked-to thread\nthat removing any such publicly documented shellscripting functions was\na no-go, even if we'd previously removed git-parse-remote.sh.\n"},{"id":"449119","messageId":"nycvar.QRO.7.76.6.2202221331220.11118@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"CABPp-BGdbh=HM7jA+_RTwSWVcMr_qvEY7RoNXooeBT2NB4Ubzw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-22T12:44:17Z","receivedAt":"2022-02-22T12:44:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nto save every reader some time, I snipped the parts that were addressed\nelsewhere already.\n\nOn Tue, 11 Jan 2022, Elijah Newren wrote:\n\n> On Tue, Jan 11, 2022 at 2:30 PM Johannes Schindelin\n> wrote:\n> >\n> > On Mon, 10 Jan 2022, Elijah Newren wrote:\n> >\n> > > You have a couple problematic assumptions here:\n> > >\n> > > * What if the user's resolution required fixing a semantic\n> > > conflict, meaning they needed to modify a file that had no\n> > > conflicts? Your code here would ignore those, because the clean\n> > > case is handled above.\n> > >\n> > > * What if the user's resolution required adding a totally new file\n> > > (either because a rename is handled as a separate add/delete, or\n> > > they just needed a new file)? The loop above isn't over items in\n> > > pre_resolved_conflicts, it just assumes that items in\n> > > pre_resolved_conflicts are also in the plist.items being looped\n> > > over.\n> >\n> > I can see how these assumptions may look ludicrous when coming from\n> > the command-line. But again, we are talking about the realities of a\n> > conflict resolution via a web UI.\n> >\n> > So I think that it is out of the question to address non-textual\n> > conflicts in this scenario. And then the assumptions make much more\n> > sense.\n>\n> Waving your hands and saying it came from a web UI doesn't reduce my\n> worries or concerns at all. I could easily imagine a web UI that\n> allows users to specify other modifications they want to make, even\n> limited to textual ones, to include in the merge. What happens when\n> they modify some file that had otherwise merged cleanly (e.g. a file\n> that gains a new call to some function, which after the merge needs an\n> extra parameter passed to it)? Your solution doesn't handle it; it'd\n> send that user-specified change to /dev/null.\n>\n> To be fair, you mentioned below that this is just a proof of concept,\n> and that certainly reduces worries/concerns. It's totally fine to\n> have such things. Maybe you intend to keep this patch internal\n> indefinitely. That's fine too. My commentary is just feedback on if\n> we want merge-ort/merge-tree extended more in this kind of fashion\n> (and it might also serve as useful pointers on how to extend your\n> internal patch if you get requests to extend your web UI a bit to\n> handle more cases). :-)\n\nFair enough. I think I'll keep this patch internal-only for now, and\niterate with real scenarios to figure out what we need/not need.\n\n> > > Could you clarify what you mean here by OIDs and modes? For a given\n> > > path, are you just looking for a (pathname, OID, mode) tuple? (In\n> > > which case, you'd get the OID of a file that potentially has embedded\n> > > conflict markers)\n> > >\n> > > Or are you looking for multiple OIDs and modes for each file?\n> >\n> > This. In case of a conflict, I am looking for (mode,OID) for the merge\n> > base (which _can_ be a virtual one in case of recursive merges) as well as\n> > for the two divergent revisions that were supposed to be merged.\n> >\n> > I do realize that other types of conflicts can occur, such as a\n> > rename/rename conflict, and we would need a way to represent these in the\n> > output, too. But in such an instance, where no clear (mode,OID) triplet\n> > can be provided that represents the merge conflicts for this file, the\n> > current web UI cannot offer a way to resolve the conflict, so I am a bit\n> > less interested in that scenario.\n>\n> Okay, this is helping explain a bit better.\n>\n> Out of curiosity, does this mean the web UI only can resolve cases\n> where all three modes & OIDs are present, and the files are text\n> rather than binary? For example, does this mean the web UI cannot\n> handle cases like modify/delete or add/add?\n\nRight now, the UI is based on the assumption that the underlying merge\nmachinery does not even try to detect renames. Which simplifies the range\nof supported scenarios somewhat.\n\nI have not looked at the underlying code, but\nhttps://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github\nclearly says:\n\n\tYou can resolve simple merge conflicts that involve competing line\n\tchanges on GitHub, using the conflict editor.\n\nSo yes, the UI does not even try to support resolving modify/delete\nconflicts at this time.\n\n> > But again, you made me think of rename/rename conflicts and friends, and\n> > we would need a way to represent those, too. Even if my current use case\n> > would need to only detect their presence in order to say \"nope, can't\n> > resolve that on the web\".\n>\n> But now you're making me worry whether I can satisfy your requests\n> again, or at minimum, whether I need to do a lot more work in\n> merge-ort to answer them. Do you need a representation other than the\n> list of (stage, mode, oid) triplets?\n\nI guess we will have to accept that the (stage, mode, OID) list is the\nbest form, for now.\n\nWe will have to see what is possible to do on the UI side, and then extend\nthe `merge-ort` side as needed.\n\n> I'm a little worried I might come across sounding a little picky since I\n> tend to dive into details and really fixate on them. I apologize in\n> advance if it sounds that way at all; you provide lots of good points to\n> think about and I can't help but dive into the quirks surrounding each.\n> I really appreciate all the awesome feedback and food for thought.\n\nPersonally, I did not find your comments picky at all, but rather\nconstructive. I find this conversation thoroughly enjoyable and productive\neven at times when you prove me wrong. You set a really high bar as far as\ncollaboration style goes. Thank you very much for that!\n\nCiao,\nDscho\n"},{"id":"449124","messageId":"nycvar.QRO.7.76.6.2202221407150.11118@tvgsbejvaqbjf.bet","threadId":"57191","inReplyTo":"CABPp-BEQ5BBEgB21hUNBuioOfMry_x2nKOLgY8=v2w8eGXrsyw@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-22T13:08:03Z","receivedAt":"2022-02-22T13:08:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Tue, 11 Jan 2022, Elijah Newren wrote:\n\n> On Mon, Jan 10, 2022 at 9:59 AM Elijah Newren wrote:\n> >\n> > On Mon, Jan 10, 2022 at 5:49 AM Johannes Schindelin\n> > wrote:\n> > >\n> ...\n> > >, therefore it expects the resolved conflicts to\n> > > be in _files_. I don't think that there is a way to reasonably handle\n> > > symlink target conflicts or directory/file/symlink conflicts, but there\n> > > might be.\n> >\n> > You really need (mode,oid) pairs to be provided by the user. That\n> > fixes the executable issue I mentioned above, and makes it clear how\n> > to handle symlinks and file/symlink conflicts.\n> >\n> > directory/file are still handled by providing individual files, but\n> > ordering traversal becomes really tricky. The directory/file case\n> > might even require the pre_resolved_conflicts to be pulled out of that\n> > loop somehow. It'd take some investigative work, or some deep\n> > thought, or both.\n>\n> I think I came up with a solution to this during my run yesterday,\n> though I haven't tried or tested it. Instead of modifying the loop\n> over plist.items, you instead add a preliminary loop over\n> pre_resolved_conflicts that modifies opt->priv->paths (and add this\n> preliminary loop just before the items from opt->priv->paths are added\n> to plist.items). In that preliminary loop, you need to make sure that\n> (a) any files in pre_resolved_conflicts corresponding to existing\n> _files_ in opt->priv->path result in updating that item's clean &\n> is_null & mode & oid state, (b) any files in pre_resolved_conflicts\n> that correspond to existing _directories_ in opt->priv->path need the\n> value to be expanded to be a conflict_info instead of just a\n> merged_info, you need to set the df_conflict bit, and don't update the\n> merge_info fields but do update the extended conflict_info ones, (c)\n> any new files in pre_resolved_conflicts result in new entries\n> opt->priv->paths, (d) any leading directories of new files in\n> pre_resolved_conflicts are appropriately handled, meaning: (d1) new\n> opt->priv->paths are created if the directory path wasn't in\n> opt->priv->paths before, (d2) a tweak to df_conflict for the directory\n> item if it previously existed in opt->priv->paths but only as a file\n> (possibly also necessitating expanding from a merged_info to a\n> conflict_info), (d3) no-op if the directory already existed in\n> opt->priv->paths and was just a directory (and in this case, you can\n> stop walking the parent directories to the toplevel).\n>\n> Then, after this preliminary loop that modifies opt->priv->paths, the\n> rest can just proceed as-is.\n>\n> That should handle new files, new directories, and all directory/file\n> conflicts. Yeah, it's a bunch to look at, but directory/file\n> conflicts are always a bear.\n\nIndeed.\n\nWhat's even worse is the question how to represent that in a web UI, and I\nthink I'll wait for that to happen (if ever) to give that part of the\ndesign more thought.\n\nCiao,\nDscho\n"}]}