{"thread":{"id":"54844","subject":"[PATCH 00/10] merge-ort: add more handling of basic conflict types","startedAt":"2020-12-18T05:52:12Z","lastAt":"2021-03-08T14:50:03Z","messageCount":37,"participants":["Elijah Newren via GitGitGadget","Elijah Newren","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"412523","messageId":"pull.815.git.1608270687.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":null,"subject":"[PATCH 00/10] merge-ort: add more handling of basic conflict types","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:17Z","receivedAt":"2020-12-18T05:52:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3\nor en/merge-ort-recursive).\n\nThis series adds handling of additional basic conflict types (directory/file\nconflicts, three-way content merging, very basic submodule divergence\nreconciliation, and different filetypes). This series drops the number of\ntest failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).\n\nFurther, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this\nseries are all merged down (in any order), then collectively they drop the\nnumber of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to\n60.\n\nElijah Newren (10):\n merge-ort: handle D/F conflict where directory disappears due to merge\n merge-ort: handle directory/file conflicts that remain\n merge-ort: implement unique_path() helper\n merge-ort: handle book-keeping around two- and three-way content merge\n merge-ort: flesh out implementation of handle_content_merge()\n merge-ort: copy and adapt merge_3way() from merge-recursive.c\n merge-ort: copy and adapt merge_submodule() from merge-recursive.c\n merge-ort: implement format_commit()\n merge-ort: copy find_first_merges() implementation from\n merge-recursive.c\n merge-ort: add handling for different types of files at same path\n\n merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 653 insertions(+), 18 deletions(-)\n\n\nbase-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/815\n-- \ngitgitgadget\n"},{"id":"412524","messageId":"382a009c18efc8a46a9c0210754f2266c3116ee4.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:18Z","receivedAt":"2020-12-18T05:52:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen one side has a directory at a given path and the other side of\nhistory has a file at the path, but the merge resolves the directory\naway (e.g. because no path within that directory was modified and the\nother side deleted it, or because renaming moved all the files\nelsewhere), then we don't actually have a conflict anymore. We just\nneed to clear away any information related to the relevant directory,\nand then the subsequent process_entry() handling can handle the given\npath.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 414e7b7eeac..f9a79eb25e6 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -976,14 +976,35 @@ static void process_entry(struct merge_options *opt,\n \t\tassert(ci->df_conflict);\n \t}\n \n-\tif (ci->df_conflict) {\n+\tif (ci->df_conflict && ci->merged.result.mode == 0) {\n+\t\tint i;\n+\n+\t\t/*\n+\t\t * directory no longer in the way, but we do have a file we\n+\t\t * need to place here so we need to clean away the \"directory\n+\t\t * merges to nothing\" result.\n+\t\t */\n+\t\tci->df_conflict = 0;\n+\t\tassert(ci->filemask != 0);\n+\t\tci->merged.clean = 0;\n+\t\tci->merged.is_null = 0;\n+\t\t/* and we want to zero out any directory-related entries */\n+\t\tci->match_mask = (ci->match_mask & ~ci->dirmask);\n+\t\tci->dirmask = 0;\n+\t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n+\t\t\tif (ci->filemask & (1 << i))\n+\t\t\t\tcontinue;\n+\t\t\tci->stages[i].mode = 0;\n+\t\t\toidcpy(&ci->stages[i].oid, &null_oid);\n+\t\t}\n+\t} else if (ci->df_conflict && ci->merged.result.mode != 0) {\n \t\tdie(\"Not yet implemented.\");\n \t}\n \n \t/*\n \t * NOTE: Below there is a long switch-like if-elseif-elseif... block\n \t * which the code goes through even for the df_conflict cases\n-\t * above. Well, it will once we don't die-not-implemented above.\n+\t * above.\n \t */\n \tif (ci->match_mask) {\n \t\tci->merged.clean = 1;\n-- \ngitgitgadget\n\n"},{"id":"412525","messageId":"6ac555b3c0fe605fbbe6e304482c2e3aef321865.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 03/10] merge-ort: implement unique_path() helper","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:20Z","receivedAt":"2020-12-18T05:52:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nImplement unique_path(), based on the one from merge-recursive.c. It is\nsimplified, however, due to: (1) using strmaps, and (2) the fact that\nmerge-ort lets the checkout codepath handle possible collisions with the\nworking tree means that other code locations don't have to.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d300a02810e..1adc27a11bc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/* add a string to a strbuf, but converting \"/\" to \"_\" */\n+static void add_flattened_path(struct strbuf *out, const char *s)\n+{\n+\tsize_t i = out->len;\n+\tstrbuf_addstr(out, s);\n+\tfor (; i < out->len; i++)\n+\t\tif (out->buf[i] == '/')\n+\t\t\tout->buf[i] = '_';\n+}\n+\n static char *unique_path(struct strmap *existing_paths,\n \t\t\t const char *path,\n \t\t\t const char *branch)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct strbuf newpath = STRBUF_INIT;\n+\tint suffix = 0;\n+\tsize_t base_len;\n+\n+\tstrbuf_addf(&newpath, \"%s~\", path);\n+\tadd_flattened_path(&newpath, branch);\n+\n+\tbase_len = newpath.len;\n+\twhile (strmap_contains(existing_paths, newpath.buf)) {\n+\t\tstrbuf_setlen(&newpath, base_len);\n+\t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n+\t}\n+\n+\treturn strbuf_detach(&newpath, NULL);\n }\n \n /*** Function Grouping: functions related to collect_merge_info() ***/\n-- \ngitgitgadget\n\n"},{"id":"412526","messageId":"4c641ec19d578a4014b4d3b568d18606a164ae9a.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 04/10] merge-ort: handle book-keeping around two- and three-way content merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:21Z","receivedAt":"2020-12-18T05:52:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn addition to the content merge (which will go in a subsequent commit),\nwe need to worry about conflict messages, placing results in higher\norder stages in case of a df_conflict, and making sure the results are\nplaced in ci->merged.result so that they will show up in the working\ntree. Take care of all that external book-keeping, moving the\nsimplistic just-take-HEAD code into the barebones handle_content_merge()\nfunction for now. Subsequent commits will flesh out\nhandle_content_merge().\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 41 insertions(+), 11 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 1adc27a11bc..47e230fe341 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -640,7 +640,15 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst int extra_marker_size,\n \t\t\t\tstruct version_info *result)\n {\n-\tdie(\"Not yet implemented\");\n+\tint clean = 0;\n+\t/*\n+\t * TODO: Needs a two-way or three-way content merge, but we're\n+\t * just being lazy and copying the version from HEAD and\n+\t * leaving it as conflicted.\n+\t */\n+\tresult->mode = a->mode;\n+\toidcpy(&result->oid, &a->oid);\n+\treturn clean;\n }\n \n /*** Function Grouping: functions related to detect_and_process_renames(), ***\n@@ -1138,16 +1146,38 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tdie(\"Not yet implemented.\");\n \t} else if (ci->filemask >= 6) {\n-\t\t/*\n-\t\t * TODO: Needs a two-way or three-way content merge, but we're\n-\t\t * just being lazy and copying the version from HEAD and\n-\t\t * leaving it as conflicted.\n-\t\t */\n-\t\tci->merged.clean = 0;\n-\t\tci->merged.result.mode = ci->stages[1].mode;\n-\t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n-\t\t/* When we fix above, we'll call handle_content_merge() */\n-\t\t(void)handle_content_merge;\n+\t\t/* Need a two-way or three-way content merge */\n+\t\tstruct version_info merged_file;\n+\t\tunsigned clean_merge;\n+\t\tstruct version_info *o = &ci->stages[0];\n+\t\tstruct version_info *a = &ci->stages[1];\n+\t\tstruct version_info *b = &ci->stages[2];\n+\n+\t\tclean_merge = handle_content_merge(opt, path, o, a, b,\n+\t\t\t\t\t\t ci->pathnames,\n+\t\t\t\t\t\t opt->priv->call_depth * 2,\n+\t\t\t\t\t\t &merged_file);\n+\t\tci->merged.clean = clean_merge &&\n+\t\t\t\t !ci->df_conflict && !ci->path_conflict;\n+\t\tci->merged.result.mode = merged_file.mode;\n+\t\tci->merged.is_null = (merged_file.mode == 0);\n+\t\toidcpy(&ci->merged.result.oid, &merged_file.oid);\n+\t\tif (clean_merge && ci->df_conflict) {\n+\t\t\tassert(df_file_index == 1 || df_file_index == 2);\n+\t\t\tci->filemask = 1 << df_file_index;\n+\t\t\tci->stages[df_file_index].mode = merged_file.mode;\n+\t\t\toidcpy(&ci->stages[df_file_index].oid, &merged_file.oid);\n+\t\t}\n+\t\tif (!clean_merge) {\n+\t\t\tconst char *reason = _(\"content\");\n+\t\t\tif (ci->filemask == 6)\n+\t\t\t\treason = _(\"add/add\");\n+\t\t\tif (S_ISGITLINK(merged_file.mode))\n+\t\t\t\treason = _(\"submodule\");\n+\t\t\tpath_msg(opt, path, 0,\n+\t\t\t\t _(\"CONFLICT (%s): Merge conflict in %s\"),\n+\t\t\t\t reason, path);\n+\t\t}\n \t} else if (ci->filemask == 3 || ci->filemask == 5) {\n \t\t/* Modify/delete */\n \t\tconst char *modify_branch, *delete_branch;\n-- \ngitgitgadget\n\n"},{"id":"412527","messageId":"46953226ba86fa159180262adb068b3218c74a9b.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 02/10] merge-ort: handle directory/file conflicts that remain","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:19Z","receivedAt":"2020-12-18T05:52:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen a directory/file conflict remains, we can leave the directory where\nit is, but need to move the information about the file to a different\npathname. After moving the file to a different pathname, we allow\nsubsequent process_entry() logic to handle any additional details that\nmight be relevant.\n\nThis depends on a new helper function, unique_path(), that dies with an\nunimplemented error currently but will be implemented in a subsequent\ncommit.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 83 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex f9a79eb25e6..d300a02810e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -343,6 +343,13 @@ static void path_msg(struct merge_options *opt,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+static char *unique_path(struct strmap *existing_paths,\n+\t\t\t const char *path,\n+\t\t\t const char *branch)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n /*** Function Grouping: functions related to collect_merge_info() ***/\n \n static void setup_path_info(struct merge_options *opt,\n@@ -962,6 +969,8 @@ static void process_entry(struct merge_options *opt,\n \t\t\t struct conflict_info *ci,\n \t\t\t struct directory_versions *dir_metadata)\n {\n+\tint df_file_index = 0;\n+\n \tVERIFY_CI(ci);\n \tassert(ci->filemask >= 0 && ci->filemask <= 7);\n \t/* ci->match_mask == 7 was handled in collect_merge_info_callback() */\n@@ -998,7 +1007,80 @@ static void process_entry(struct merge_options *opt,\n \t\t\toidcpy(&ci->stages[i].oid, &null_oid);\n \t\t}\n \t} else if (ci->df_conflict && ci->merged.result.mode != 0) {\n-\t\tdie(\"Not yet implemented.\");\n+\t\t/*\n+\t\t * This started out as a D/F conflict, and the entries in\n+\t\t * the competing directory were not removed by the merge as\n+\t\t * evidenced by write_completed_directory() writing a value\n+\t\t * to ci->merged.result.mode.\n+\t\t */\n+\t\tstruct conflict_info *new_ci;\n+\t\tconst char *branch;\n+\t\tconst char *old_path = path;\n+\t\tint i;\n+\n+\t\tassert(ci->merged.result.mode == S_IFDIR);\n+\n+\t\t/*\n+\t\t * If filemask is 1, we can just ignore the file as having\n+\t\t * been deleted on both sides. We do not want to overwrite\n+\t\t * ci->merged.result, since it stores the tree for all the\n+\t\t * files under it.\n+\t\t */\n+\t\tif (ci->filemask == 1) {\n+\t\t\tci->filemask = 0;\n+\t\t\treturn;\n+\t\t}\n+\n+\t\t/*\n+\t\t * This file still exists on at least one side, and we want\n+\t\t * the directory to remain here, so we need to move this\n+\t\t * path to some new location.\n+\t\t */\n+\t\tnew_ci = xcalloc(1, sizeof(*new_ci));\n+\t\t/* We don't really want new_ci->merged.result copied, but it'll\n+\t\t * be overwritten below so it doesn't matter. We also don't\n+\t\t * want any directory mode/oid values copied, but we'll zero\n+\t\t * those out immediately. We do want the rest of ci copied.\n+\t\t */\n+\t\tmemcpy(new_ci, ci, sizeof(*ci));\n+\t\tnew_ci->match_mask = (new_ci->match_mask & ~new_ci->dirmask);\n+\t\tnew_ci->dirmask = 0;\n+\t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n+\t\t\tif (new_ci->filemask & (1 << i))\n+\t\t\t\tcontinue;\n+\t\t\t/* zero out any entries related to directories */\n+\t\t\tnew_ci->stages[i].mode = 0;\n+\t\t\toidcpy(&new_ci->stages[i].oid, &null_oid);\n+\t\t}\n+\n+\t\t/*\n+\t\t * Find out which side this file came from; note that we\n+\t\t * cannot just use ci->filemask, because renames could cause\n+\t\t * the filemask to go back to 7. So we use dirmask, then\n+\t\t * pick the opposite side's index.\n+\t\t */\n+\t\tdf_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;\n+\t\tbranch = (df_file_index == 1) ? opt->branch1 : opt->branch2;\n+\t\tpath = unique_path(&opt->priv->paths, path, branch);\n+\t\tstrmap_put(&opt->priv->paths, path, new_ci);\n+\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"CONFLICT (file/directory): directory in the way \"\n+\t\t\t \"of %s from %s; moving it to %s instead.\"),\n+\t\t\t old_path, branch, path);\n+\n+\t\t/*\n+\t\t * Zero out the filemask for the old ci. At this point, ci\n+\t\t * was just an entry for a directory, so we don't need to\n+\t\t * do anything more with it.\n+\t\t */\n+\t\tci->filemask = 0;\n+\n+\t\t/*\n+\t\t * Now note that we're working on the new entry (path was\n+\t\t * updated above.\n+\t\t */\n+\t\tci = new_ci;\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"412528","messageId":"0e7321e67f83c197e84ad7b281ab2ce79c90b784.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 05/10] merge-ort: flesh out implementation of handle_content_merge()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:22Z","receivedAt":"2020-12-18T05:52:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis implementation is based heavily on merge_mode_and_contents() from\nmerge-recursive.c, though it has some fixes for recursive merges (i.e.\nwhen call_depth > 0), and has a number of changes throughout based on\nslight differences in data structures and in how the functions are\ncalled.\n\nIt is, however, based on two new helper functions -- merge_3way() and\nmerge_submodule -- for which we only provide die-not-implemented stubs\nat this point. Future commits will add implementations of these\nfunctions.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 143 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 47e230fe341..2cfb7ffa3b0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -631,6 +631,28 @@ static int collect_merge_info(struct merge_options *opt,\n \n /*** Function Grouping: functions related to threeway content merges ***/\n \n+static int merge_submodule(struct merge_options *opt,\n+\t\t\t const char *path,\n+\t\t\t const struct object_id *o,\n+\t\t\t const struct object_id *a,\n+\t\t\t const struct object_id *b,\n+\t\t\t struct object_id *result)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n+static int merge_3way(struct merge_options *opt,\n+\t\t const char *path,\n+\t\t const struct object_id *o,\n+\t\t const struct object_id *a,\n+\t\t const struct object_id *b,\n+\t\t const char *pathnames[3],\n+\t\t const int extra_marker_size,\n+\t\t mmbuffer_t *result_buf)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst char *path,\n \t\t\t\tconst struct version_info *o,\n@@ -640,14 +662,129 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst int extra_marker_size,\n \t\t\t\tstruct version_info *result)\n {\n-\tint clean = 0;\n \t/*\n-\t * TODO: Needs a two-way or three-way content merge, but we're\n-\t * just being lazy and copying the version from HEAD and\n-\t * leaving it as conflicted.\n+\t * path is the target location where we want to put the file, and\n+\t * is used to determine any normalization rules in ll_merge.\n+\t *\n+\t * The normal case is that path and all entries in pathnames are\n+\t * identical, though renames can affect which path we got one of\n+\t * the three blobs to merge on various sides of history.\n+\t *\n+\t * extra_marker_size is the amount to extend conflict markers in\n+\t * ll_merge; this is neeed if we have content merges of content\n+\t * merges, which happens for example with rename/rename(2to1) and\n+\t * rename/add conflicts.\n \t */\n-\tresult->mode = a->mode;\n-\toidcpy(&result->oid, &a->oid);\n+\tunsigned clean = 1;\n+\n+\t/*\n+\t * handle_content_merge() needs both files to be of the same type, i.e.\n+\t * both files OR both submodules OR both symlinks. Conflicting types\n+\t * needs to be handled elsewhere.\n+\t */\n+\tassert((S_IFMT & a->mode) == (S_IFMT & b->mode));\n+\n+\t/* Merge modes */\n+\tif (a->mode == b->mode || a->mode == o->mode)\n+\t\tresult->mode = b->mode;\n+\telse {\n+\t\t/* must be the 100644/100755 case */\n+\t\tassert(S_ISREG(a->mode));\n+\t\tresult->mode = a->mode;\n+\t\tclean = (b->mode == o->mode);\n+\t\t/*\n+\t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n+\t\t * should not make result->mode match either a->mode or\n+\t\t * b->mode; that causes t6036 \"check conflicting mode for\n+\t\t * regular file\" to fail. It would be best to use some other\n+\t\t * mode, but we'll confuse all kinds of stuff if we use one\n+\t\t * where S_ISREG(result->mode) isn't true, and if we use\n+\t\t * something like 0100666, then tree-walk.c's calls to\n+\t\t * canon_mode() will just normalize that to 100644 for us and\n+\t\t * thus not solve anything.\n+\t\t *\n+\t\t * Figure out if there's some kind of way we can work around\n+\t\t * this...\n+\t\t */\n+\t}\n+\n+\t/*\n+\t * Trivial oid merge.\n+\t *\n+\t * Note: While one might assume that the next four lines would\n+\t * be unnecessary due to the fact that match_mask is often\n+\t * setup and already handled, renames don't always take care\n+\t * of that.\n+\t */\n+\tif (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid))\n+\t\toidcpy(&result->oid, &b->oid);\n+\telse if (oideq(&b->oid, &o->oid))\n+\t\toidcpy(&result->oid, &a->oid);\n+\n+\t/* Remaining rules depend on file vs. submodule vs. symlink. */\n+\telse if (S_ISREG(a->mode)) {\n+\t\tmmbuffer_t result_buf;\n+\t\tint ret = 0, merge_status;\n+\t\tint two_way;\n+\n+\t\t/*\n+\t\t * If 'o' is different type, treat it as null so we do a\n+\t\t * two-way merge.\n+\t\t */\n+\t\ttwo_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));\n+\n+\t\tmerge_status = merge_3way(opt, path,\n+\t\t\t\t\t two_way ? &null_oid : &o->oid,\n+\t\t\t\t\t &a->oid, &b->oid,\n+\t\t\t\t\t pathnames, extra_marker_size,\n+\t\t\t\t\t &result_buf);\n+\n+\t\tif ((merge_status < 0) || !result_buf.ptr)\n+\t\t\tret = err(opt, _(\"Failed to execute internal merge\"));\n+\n+\t\tif (!ret &&\n+\t\t write_object_file(result_buf.ptr, result_buf.size,\n+\t\t\t\t blob_type, &result->oid))\n+\t\t\tret = err(opt, _(\"Unable to add %s to database\"),\n+\t\t\t\t path);\n+\n+\t\tfree(result_buf.ptr);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\t\tclean &= (merge_status == 0);\n+\t\tpath_msg(opt, path, 1, _(\"Auto-merging %s\"), path);\n+\t} else if (S_ISGITLINK(a->mode)) {\n+\t\tint two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));\n+\t\tclean = merge_submodule(opt, pathnames[0],\n+\t\t\t\t\ttwo_way ? &null_oid : &o->oid,\n+\t\t\t\t\t&a->oid, &b->oid, &result->oid);\n+\t\tif (opt->priv->call_depth && two_way && !clean) {\n+\t\t\tresult->mode = o->mode;\n+\t\t\toidcpy(&result->oid, &o->oid);\n+\t\t}\n+\t} else if (S_ISLNK(a->mode)) {\n+\t\tif (opt->priv->call_depth) {\n+\t\t\tclean = 0;\n+\t\t\tresult->mode = o->mode;\n+\t\t\toidcpy(&result->oid, &o->oid);\n+\t\t} else {\n+\t\t\tswitch (opt->recursive_variant) {\n+\t\t\tcase MERGE_VARIANT_NORMAL:\n+\t\t\t\tclean = 0;\n+\t\t\t\toidcpy(&result->oid, &a->oid);\n+\t\t\t\tbreak;\n+\t\t\tcase MERGE_VARIANT_OURS:\n+\t\t\t\toidcpy(&result->oid, &a->oid);\n+\t\t\t\tbreak;\n+\t\t\tcase MERGE_VARIANT_THEIRS:\n+\t\t\t\toidcpy(&result->oid, &b->oid);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t} else\n+\t\tBUG(\"unsupported object type in the tree: %06o for %s\",\n+\t\t a->mode, path);\n+\n \treturn clean;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"412529","messageId":"4696f6c2d95e5080486cf6775ed0fccee59f150e.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 07/10] merge-ort: copy and adapt merge_submodule() from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:24Z","receivedAt":"2020-12-18T05:52:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nTake merge_submodule() from merge-recursive.c and make slight\nadjustments, predominantly around deferring output using path_msg()\ninstead of using merge-recursive's output() and show() functions.\nThere's also a fix for recursive cases (when call_depth > 0) and a\nslight change to argument order for find_first_merges().\n\nfind_first_merges() and format_commit() are left unimplemented for\nnow, but will be added by subsequent commits.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 125 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a59adb42aa6..2dfab1858fc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -26,6 +26,7 @@\n #include \"ll-merge.h\"\n #include \"object-store.h\"\n #include \"strmap.h\"\n+#include \"submodule.h\"\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n #include \"xdiff-interface.h\"\n@@ -323,6 +324,13 @@ static int err(struct merge_options *opt, const char *err, ...)\n \treturn -1;\n }\n \n+static void format_commit(struct strbuf *sb,\n+\t\t\t int indent,\n+\t\t\t struct commit *commit)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n __attribute__((format (printf, 4, 5)))\n static void path_msg(struct merge_options *opt,\n \t\t const char *path,\n@@ -632,6 +640,15 @@ static int collect_merge_info(struct merge_options *opt,\n \n /*** Function Grouping: functions related to threeway content merges ***/\n \n+static int find_first_merges(struct repository *repo,\n+\t\t\t const char *path,\n+\t\t\t struct commit *a,\n+\t\t\t struct commit *b,\n+\t\t\t struct object_array *result)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n static int merge_submodule(struct merge_options *opt,\n \t\t\t const char *path,\n \t\t\t const struct object_id *o,\n@@ -639,7 +656,114 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *b,\n \t\t\t struct object_id *result)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct commit *commit_o, *commit_a, *commit_b;\n+\tint parent_count;\n+\tstruct object_array merges;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tint i;\n+\tint search = !opt->priv->call_depth;\n+\n+\t/* store fallback answer in result in case we fail */\n+\toidcpy(result, opt->priv->call_depth ? o : a);\n+\n+\t/* we can not handle deletion conflicts */\n+\tif (is_null_oid(o))\n+\t\treturn 0;\n+\tif (is_null_oid(a))\n+\t\treturn 0;\n+\tif (is_null_oid(b))\n+\t\treturn 0;\n+\n+\tif (add_submodule_odb(path)) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s (not checked out)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!(commit_o = lookup_commit_reference(opt->repo, o)) ||\n+\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n+\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s (commits not present)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\t/* check whether both changes are forward */\n+\tif (!in_merge_bases(commit_o, commit_a) ||\n+\t !in_merge_bases(commit_o, commit_b)) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t \"(commits don't follow merge-base)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\t/* Case #1: a is contained in b or vice versa */\n+\tif (in_merge_bases(commit_a, commit_b)) {\n+\t\toidcpy(result, b);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n+\t\t\t path, oid_to_hex(b));\n+\t\treturn 1;\n+\t}\n+\tif (in_merge_bases(commit_b, commit_a)) {\n+\t\toidcpy(result, a);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n+\t\t\t path, oid_to_hex(a));\n+\t\treturn 1;\n+\t}\n+\n+\t/*\n+\t * Case #2: There are one or more merges that contain a and b in\n+\t * the submodule. If there is only one, then present it as a\n+\t * suggestion to the user, but leave it marked unmerged so the\n+\t * user needs to confirm the resolution.\n+\t */\n+\n+\t/* Skip the search if makes no sense to the calling context. */\n+\tif (!search)\n+\t\treturn 0;\n+\n+\t/* find commit which merges them */\n+\tparent_count = find_first_merges(opt->repo, path, commit_a, commit_b,\n+\t\t\t\t\t &merges);\n+\tswitch (parent_count) {\n+\tcase 0:\n+\t\tpath_msg(opt, path, 0, _(\"Failed to merge submodule %s\"), path);\n+\t\tbreak;\n+\n+\tcase 1:\n+\t\tformat_commit(&sb, 4,\n+\t\t\t (struct commit *)merges.objects[0].item);\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s, but a possible merge \"\n+\t\t\t \"resolution exists:\\n%s\\n\"),\n+\t\t\t path, sb.buf);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"If this is correct simply add it to the index \"\n+\t\t\t \"for example\\n\"\n+\t\t\t \"by using:\\n\\n\"\n+\t\t\t \" git update-index --cacheinfo 160000 %s \\\"%s\\\"\\n\\n\"\n+\t\t\t \"which will accept this suggestion.\\n\"),\n+\t\t\t oid_to_hex(&merges.objects[0].item->oid), path);\n+\t\tstrbuf_release(&sb);\n+\t\tbreak;\n+\tdefault:\n+\t\tfor (i = 0; i < merges.nr; i++)\n+\t\t\tformat_commit(&sb, 4,\n+\t\t\t\t (struct commit *)merges.objects[i].item);\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s, but multiple \"\n+\t\t\t \"possible merges exist:\\n%s\"), path, sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tobject_array_clear(&merges);\n+\treturn 0;\n }\n \n static int merge_3way(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"412530","messageId":"b898876b119c669ac84a5197bba1750a6f7f1e52.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:26Z","receivedAt":"2020-12-18T05:52:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCode is identical for the function body in the two files, the call\nsignature is just slightly different in merge-ort than merge-recursive\nas noted a couple commits ago.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex bf704bcd34d..203fa987e43 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -25,6 +25,7 @@\n #include \"dir.h\"\n #include \"ll-merge.h\"\n #include \"object-store.h\"\n+#include \"revision.h\"\n #include \"strmap.h\"\n #include \"submodule.h\"\n #include \"tree.h\"\n@@ -658,7 +659,61 @@ static int find_first_merges(struct repository *repo,\n \t\t\t struct commit *b,\n \t\t\t struct object_array *result)\n {\n-\tdie(\"Not yet implemented.\");\n+\tint i, j;\n+\tstruct object_array merges = OBJECT_ARRAY_INIT;\n+\tstruct commit *commit;\n+\tint contains_another;\n+\n+\tchar merged_revision[GIT_MAX_HEXSZ + 2];\n+\tconst char *rev_args[] = { \"rev-list\", \"--merges\", \"--ancestry-path\",\n+\t\t\t\t \"--all\", merged_revision, NULL };\n+\tstruct rev_info revs;\n+\tstruct setup_revision_opt rev_opts;\n+\n+\tmemset(result, 0, sizeof(struct object_array));\n+\tmemset(&rev_opts, 0, sizeof(rev_opts));\n+\n+\t/* get all revisions that merge commit a */\n+\txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n+\t\t oid_to_hex(&a->object.oid));\n+\trepo_init_revisions(repo, &revs, NULL);\n+\trev_opts.submodule = path;\n+\t/* FIXME: can't handle linked worktrees in submodules yet */\n+\trevs.single_worktree = path != NULL;\n+\tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n+\n+\t/* save all revisions from the above list that contain b */\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(\"revision walk setup failed\");\n+\twhile ((commit = get_revision(&revs)) != NULL) {\n+\t\tstruct object *o = &(commit->object);\n+\t\tif (in_merge_bases(b, commit))\n+\t\t\tadd_object_array(o, NULL, &merges);\n+\t}\n+\treset_revision_walk();\n+\n+\t/* Now we've got all merges that contain a and b. Prune all\n+\t * merges that contain another found merge and save them in\n+\t * result.\n+\t */\n+\tfor (i = 0; i < merges.nr; i++) {\n+\t\tstruct commit *m1 = (struct commit *) merges.objects[i].item;\n+\n+\t\tcontains_another = 0;\n+\t\tfor (j = 0; j < merges.nr; j++) {\n+\t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n+\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\t\tcontains_another = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!contains_another)\n+\t\t\tadd_object_array(merges.objects[i].item, NULL, result);\n+\t}\n+\n+\tobject_array_clear(&merges);\n+\treturn result->nr;\n }\n \n static int merge_submodule(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"412531","messageId":"0a5778df2535c12b1ecb1484878e62d37e69d7cb.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 10/10] merge-ort: add handling for different types of files at same path","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:27Z","receivedAt":"2020-12-18T05:52:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAdd some handling that explicitly considers collisions of the following\ntypes:\n * file/submodule\n * file/symlink\n * submodule/symlink\nLeaving them as conflicts at the same path are hard for users to\nresolve, so move one or both of them aside so that they each get their\nown path.\n\nNote that in the case of recursive handling (i.e. call_depth > 0), we\ncan just use the merge base of the two merge bases as the merge result\nmuch like we do with modify/delete conflicts, binary files, conflicting\nsubmodule values, and so on.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 103 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 203fa987e43..afe721182e2 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1521,10 +1521,109 @@ static void process_entry(struct merge_options *opt,\n \t} else if (ci->filemask >= 6 &&\n \t\t (S_IFMT & ci->stages[1].mode) !=\n \t\t (S_IFMT & ci->stages[2].mode)) {\n-\t\t/*\n-\t\t * Two different items from (file/submodule/symlink)\n-\t\t */\n-\t\tdie(\"Not yet implemented.\");\n+\t\t/* Two different items from (file/submodule/symlink) */\n+\t\tif (opt->priv->call_depth) {\n+\t\t\t/* Just use the version from the merge base */\n+\t\t\tci->merged.clean = 0;\n+\t\t\toidcpy(&ci->merged.result.oid, &ci->stages[0].oid);\n+\t\t\tci->merged.result.mode = ci->stages[0].mode;\n+\t\t\tci->merged.is_null = (ci->merged.result.mode == 0);\n+\t\t} else {\n+\t\t\t/* Handle by renaming one or both to separate paths. */\n+\t\t\tunsigned o_mode = ci->stages[0].mode;\n+\t\t\tunsigned a_mode = ci->stages[1].mode;\n+\t\t\tunsigned b_mode = ci->stages[2].mode;\n+\t\t\tstruct conflict_info *new_ci;\n+\t\t\tconst char *a_path = NULL, *b_path = NULL;\n+\t\t\tint rename_a = 0, rename_b = 0;\n+\n+\t\t\tnew_ci = xmalloc(sizeof(*new_ci));\n+\n+\t\t\tif (S_ISREG(a_mode))\n+\t\t\t\trename_a = 1;\n+\t\t\telse if (S_ISREG(b_mode))\n+\t\t\t\trename_b = 1;\n+\t\t\telse {\n+\t\t\t\trename_a = 1;\n+\t\t\t\trename_b = 1;\n+\t\t\t}\n+\n+\t\t\tpath_msg(opt, path, 0,\n+\t\t\t\t _(\"CONFLICT (distinct types): %s had different \"\n+\t\t\t\t \"types on each side; renamed %s of them so \"\n+\t\t\t\t \"each can be recorded somewhere.\"),\n+\t\t\t\t path,\n+\t\t\t\t (rename_a && rename_b) ? _(\"both\") : _(\"one\"));\n+\n+\t\t\tci->merged.clean = 0;\n+\t\t\tmemcpy(new_ci, ci, sizeof(*new_ci));\n+\n+\t\t\t/* Put b into new_ci, removing a from stages */\n+\t\t\tnew_ci->merged.result.mode = ci->stages[2].mode;\n+\t\t\toidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid);\n+\t\t\tnew_ci->stages[1].mode = 0;\n+\t\t\toidcpy(&new_ci->stages[1].oid, &null_oid);\n+\t\t\tnew_ci->filemask = 5;\n+\t\t\tif ((S_IFMT & b_mode) != (S_IFMT & o_mode)) {\n+\t\t\t\tnew_ci->stages[0].mode = 0;\n+\t\t\t\toidcpy(&new_ci->stages[0].oid, &null_oid);\n+\t\t\t\tnew_ci->filemask = 4;\n+\t\t\t}\n+\n+\t\t\t/* Leave only a in ci, fixing stages. */\n+\t\t\tci->merged.result.mode = ci->stages[1].mode;\n+\t\t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n+\t\t\tci->stages[2].mode = 0;\n+\t\t\toidcpy(&ci->stages[2].oid, &null_oid);\n+\t\t\tci->filemask = 3;\n+\t\t\tif ((S_IFMT & a_mode) != (S_IFMT & o_mode)) {\n+\t\t\t\tci->stages[0].mode = 0;\n+\t\t\t\toidcpy(&ci->stages[0].oid, &null_oid);\n+\t\t\t\tci->filemask = 2;\n+\t\t\t}\n+\n+\t\t\t/* Insert entries into opt->priv_paths */\n+\t\t\tassert(rename_a || rename_b);\n+\t\t\tif (rename_a) {\n+\t\t\t\ta_path = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t path, opt->branch1);\n+\t\t\t\tstrmap_put(&opt->priv->paths, a_path, ci);\n+\t\t\t}\n+\n+\t\t\tif (rename_b)\n+\t\t\t\tb_path = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t path, opt->branch2);\n+\t\t\telse\n+\t\t\t\tb_path = path;\n+\t\t\tstrmap_put(&opt->priv->paths, b_path, new_ci);\n+\n+\t\t\tif (rename_a && rename_b) {\n+\t\t\t\tstrmap_remove(&opt->priv->paths, path, 0);\n+\t\t\t\t/*\n+\t\t\t\t * We removed path from opt->priv->paths. path\n+\t\t\t\t * will also eventually need to be freed, but\n+\t\t\t\t * it may still be used by e.g. ci->pathnames.\n+\t\t\t\t * So, store it in another string-list for now.\n+\t\t\t\t */\n+\t\t\t\tstring_list_append(&opt->priv->paths_to_free,\n+\t\t\t\t\t\t path);\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * Do special handling for b_path since process_entry()\n+\t\t\t * won't be called on it specially.\n+\t\t\t */\n+\t\t\tstrmap_put(&opt->priv->conflicted, b_path, new_ci);\n+\t\t\trecord_entry_for_tree(dir_metadata, b_path,\n+\t\t\t\t\t &new_ci->merged);\n+\n+\t\t\t/*\n+\t\t\t * Remaining code for processing this entry should\n+\t\t\t * think in terms of processing a_path.\n+\t\t\t */\n+\t\t\tif (a_path)\n+\t\t\t\tpath = a_path;\n+\t\t}\n \t} else if (ci->filemask >= 6) {\n \t\t/* Need a two-way or three-way content merge */\n \t\tstruct version_info merged_file;\n-- \ngitgitgadget\n"},{"id":"412533","messageId":"611141f24af2c8d706fbf2dc6ef776ca61391e41.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:23Z","receivedAt":"2020-12-18T05:53:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nTake merge_3way() from merge-recursive.c and make slight adjustments\nbased on different data structures (direct usage of object_id\nrather diff_filespec, separate pathnames which based on our careful\ninterning of pathnames in opt->priv->paths can be compared with '!='\nrather than 'strcmp').\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 2cfb7ffa3b0..a59adb42aa6 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -23,6 +23,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"dir.h\"\n+#include \"ll-merge.h\"\n #include \"object-store.h\"\n #include \"strmap.h\"\n #include \"tree.h\"\n@@ -650,7 +651,58 @@ static int merge_3way(struct merge_options *opt,\n \t\t const int extra_marker_size,\n \t\t mmbuffer_t *result_buf)\n {\n-\tdie(\"Not yet implemented.\");\n+\tmmfile_t orig, src1, src2;\n+\tstruct ll_merge_options ll_opts = {0};\n+\tchar *base, *name1, *name2;\n+\tint merge_status;\n+\n+\tll_opts.renormalize = opt->renormalize;\n+\tll_opts.extra_marker_size = extra_marker_size;\n+\tll_opts.xdl_opts = opt->xdl_opts;\n+\n+\tif (opt->priv->call_depth) {\n+\t\tll_opts.virtual_ancestor = 1;\n+\t\tll_opts.variant = 0;\n+\t} else {\n+\t\tswitch (opt->recursive_variant) {\n+\t\tcase MERGE_VARIANT_OURS:\n+\t\t\tll_opts.variant = XDL_MERGE_FAVOR_OURS;\n+\t\t\tbreak;\n+\t\tcase MERGE_VARIANT_THEIRS:\n+\t\t\tll_opts.variant = XDL_MERGE_FAVOR_THEIRS;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tll_opts.variant = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tassert(pathnames[0] && pathnames[1] && pathnames[2] && opt->ancestor);\n+\tif (pathnames[0] == pathnames[1] && pathnames[1] == pathnames[2]) {\n+\t\tbase = mkpathdup(\"%s\", opt->ancestor);\n+\t\tname1 = mkpathdup(\"%s\", opt->branch1);\n+\t\tname2 = mkpathdup(\"%s\", opt->branch2);\n+\t} else {\n+\t\tbase = mkpathdup(\"%s:%s\", opt->ancestor, pathnames[0]);\n+\t\tname1 = mkpathdup(\"%s:%s\", opt->branch1, pathnames[1]);\n+\t\tname2 = mkpathdup(\"%s:%s\", opt->branch2, pathnames[2]);\n+\t}\n+\n+\tread_mmblob(&orig, o);\n+\tread_mmblob(&src1, a);\n+\tread_mmblob(&src2, b);\n+\n+\tmerge_status = ll_merge(result_buf, path, &orig, base,\n+\t\t\t\t&src1, name1, &src2, name2,\n+\t\t\t\topt->repo->index, &ll_opts);\n+\n+\tfree(base);\n+\tfree(name1);\n+\tfree(name2);\n+\tfree(orig.ptr);\n+\tfree(src1.ptr);\n+\tfree(src2.ptr);\n+\treturn merge_status;\n }\n \n static int handle_content_merge(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"412532","messageId":"a640cc0effc0bf692938940df8cc9f6fdc647cf1.1608270687.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH 08/10] merge-ort: implement format_commit()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-18T05:51:25Z","receivedAt":"2020-12-18T05:53:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis implementation is based on a mixture of print_commit() and\noutput_commit_title() from merge-recursive.c so that it can be used to\ntake over both functions.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 2dfab1858fc..bf704bcd34d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -328,7 +328,19 @@ static void format_commit(struct strbuf *sb,\n \t\t\t int indent,\n \t\t\t struct commit *commit)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct merge_remote_desc *desc;\n+\tstruct pretty_print_context ctx = {0};\n+\tctx.abbrev = DEFAULT_ABBREV;\n+\n+\tstrbuf_addchars(sb, ' ', indent);\n+\tdesc = merge_remote_util(commit);\n+\tif (desc) {\n+\t\tstrbuf_addf(sb, \"virtual %s\\n\", desc->name);\n+\t\treturn;\n+\t}\n+\n+\tformat_commit_message(commit, \"%h %s\", sb, &ctx);\n+\tstrbuf_addch(sb, '\\n');\n }\n \n __attribute__((format (printf, 4, 5)))\n-- \ngitgitgadget\n\n"},{"id":"413115","messageId":"CABPp-BFpgTEuAkNTc2y6mzwJivEw5YAn1deAgScR9ATwaX9+3w@mail.gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/10] merge-ort: add more handling of basic conflict types","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-29T00:44:29Z","receivedAt":"2020-12-29T00:45:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Junio,\n\nOn Thu, Dec 17, 2020 at 9:51 PM Elijah Newren via GitGitGadget\n wrote:\n>\n> This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3\n> or en/merge-ort-recursive).\n>\n> This series adds handling of additional basic conflict types (directory/file\n\nI submitted this about a week and a half ago, and figured you might\nnot have picked it up for 'seen' because of the -rc freeze and focus\non the release. With 2.30.0 released now, would it be easier for you\nif I resent the series (with no changes) or is it easier to just pick\nthis series up as it is?\n\nThanks,\nElijah\n\n> conflicts, three-way content merging, very basic submodule divergence\n> reconciliation, and different filetypes). This series drops the number of\n> test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).\n>\n> Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this\n> series are all merged down (in any order), then collectively they drop the\n> number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to\n> 60.\n>\n> Elijah Newren (10):\n> merge-ort: handle D/F conflict where directory disappears due to merge\n> merge-ort: handle directory/file conflicts that remain\n> merge-ort: implement unique_path() helper\n> merge-ort: handle book-keeping around two- and three-way content merge\n> merge-ort: flesh out implementation of handle_content_merge()\n> merge-ort: copy and adapt merge_3way() from merge-recursive.c\n> merge-ort: copy and adapt merge_submodule() from merge-recursive.c\n> merge-ort: implement format_commit()\n> merge-ort: copy find_first_merges() implementation from\n> merge-recursive.c\n> merge-ort: add handling for different types of files at same path\n>\n> merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 653 insertions(+), 18 deletions(-)\n>\n>\n> base-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/815\n> --\n> gitgitgadget\n"},{"id":"413170","messageId":"cb08fd8a-b4e1-77d6-7d8d-9f0431db2404@gmail.com","threadId":"54844","inReplyTo":"382a009c18efc8a46a9c0210754f2266c3116ee4.1608270687.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-30T14:06:15Z","receivedAt":"2020-12-30T14:06:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n> +\t} else if (ci->df_conflict && ci->merged.result.mode != 0) {\n> \t\tdie(\"Not yet implemented.\");\n> \t}\n> \n> \t/*\n> \t * NOTE: Below there is a long switch-like if-elseif-elseif... block\n> \t * which the code goes through even for the df_conflict cases\n> -\t * above. Well, it will once we don't die-not-implemented above.\n> +\t * above.\n> \t */\n\nThis comment change might be a bit premature.\n\nThanks,\n-Stolee\n\n"},{"id":"413171","messageId":"3a07f700-18fc-9f4e-3117-41e534d737b0@gmail.com","threadId":"54844","inReplyTo":"6ac555b3c0fe605fbbe6e304482c2e3aef321865.1608270687.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/10] merge-ort: implement unique_path() helper","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-30T14:16:28Z","receivedAt":"2020-12-30T14:17:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Implement unique_path(), based on the one from merge-recursive.c. It is\n> simplified, however, due to: (1) using strmaps, and (2) the fact that\n> merge-ort lets the checkout codepath handle possible collisions with the\n> working tree means that other code locations don't have to.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 25 ++++++++++++++++++++++++-\n> 1 file changed, 24 insertions(+), 1 deletion(-)\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index d300a02810e..1adc27a11bc 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,\n> \tstrbuf_addch(sb, '\\n');\n> }\n> \n> +/* add a string to a strbuf, but converting \"/\" to \"_\" */\n> +static void add_flattened_path(struct strbuf *out, const char *s)\n> +{\n> +\tsize_t i = out->len;\n> +\tstrbuf_addstr(out, s);\n> +\tfor (; i < out->len; i++)\n> +\t\tif (out->buf[i] == '/')\n> +\t\t\tout->buf[i] = '_';\n> +}\n> +\n\nThank you for pointing out that you based your code on merge-recursive.c.\nI see that this implementation is identical to the one there. I question\nwhether this causes collisions in a problematic way, when \"a/b/c\" and\n\"a_b_c\" both exist in a tree.\n\nTo avoid such a problem, we'd likely need to also expand \"_\" to \"__\" or\nsimilar. This might never actually affect any users because of the\nstrange filename matches _and_ we need to be in a directory/file conflict.\n\nAnd maybe it's not a hole at all? If it is, we can consider patching or\nat least documenting the problem.\n\nThanks,\n-Stolee\n"},{"id":"413173","messageId":"CABPp-BF37UeBdF4T68rvOaFRtPyZ2uoUSe48V4K8CUigy77CfA@mail.gmail.com","threadId":"54844","inReplyTo":"3a07f700-18fc-9f4e-3117-41e534d737b0@gmail.com","subject":"Re: [PATCH 03/10] merge-ort: implement unique_path() helper","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-30T15:10:17Z","receivedAt":"2020-12-30T15:11:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Dec 30, 2020 at 6:16 AM Derrick Stolee wrote:\n>\n> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Implement unique_path(), based on the one from merge-recursive.c. It is\n> > simplified, however, due to: (1) using strmaps, and (2) the fact that\n> > merge-ort lets the checkout codepath handle possible collisions with the\n> > working tree means that other code locations don't have to.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 25 ++++++++++++++++++++++++-\n> > 1 file changed, 24 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index d300a02810e..1adc27a11bc 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,\n> > strbuf_addch(sb, '\\n');\n> > }\n> >\n> > +/* add a string to a strbuf, but converting \"/\" to \"_\" */\n> > +static void add_flattened_path(struct strbuf *out, const char *s)\n> > +{\n> > + size_t i = out->len;\n> > + strbuf_addstr(out, s);\n> > + for (; i < out->len; i++)\n> > + if (out->buf[i] == '/')\n> > + out->buf[i] = '_';\n> > +}\n> > +\n>\n> Thank you for pointing out that you based your code on merge-recursive.c.\n> I see that this implementation is identical to the one there. I question\n> whether this causes collisions in a problematic way, when \"a/b/c\" and\n> \"a_b_c\" both exist in a tree.\n>\n> To avoid such a problem, we'd likely need to also expand \"_\" to \"__\" or\n> similar. This might never actually affect any users because of the\n> strange filename matches _and_ we need to be in a directory/file conflict.\n>\n> And maybe it's not a hole at all? If it is, we can consider patching or\n> at least documenting the problem.\n\nadd_flattened_path() can certainly result in a collision, regardless\nof whether the char *s parameter has any '/' characters in it. For\nexample, if you are trying to get a unique path associated with\nbuiltin/commit-graph.c due to changes from the 'next' branch side of\nthe merge, and builtin/commit-graph.c~next already exists, then you\nhave a collision. It's actually pretty rare that the parameter would\nhave any '/' characters at all, since it's pretty rare for me to see\nfolks (other than Junio) use hierarchical branch names. But if the\nbranch name were ds/line-log-on-bloom, then the provisional filename\nwould be builtin/commit-graph.c~ds_line-log-on-bloom. The '/' to '_'\nconversion exists just to make sure our new file remains in the same\ndirectory as where the conflict that caused us to need a new unique\npath occurred.\n\nBut unique_path() does NOT end immediately after calling\nadd_flattened_path() and there is no collision possible in the return\nfrom unique_path(), because it ends with a\n while (strmap_contains(existing_paths, newpath.buf)) {\nloop that modifies the resulting path until it finds one that doesn't\ncollide with an existing path.\n"},{"id":"413174","messageId":"CABPp-BFrct6hZW8bh5hhRpGSPhjeGO9viCjpO02gUyTqNnrAgg@mail.gmail.com","threadId":"54844","inReplyTo":"cb08fd8a-b4e1-77d6-7d8d-9f0431db2404@gmail.com","subject":"Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-30T15:13:26Z","receivedAt":"2020-12-30T15:14:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee wrote:\n>\n> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n> > + } else if (ci->df_conflict && ci->merged.result.mode != 0) {\n> > die(\"Not yet implemented.\");\n> > }\n> >\n> > /*\n> > * NOTE: Below there is a long switch-like if-elseif-elseif... block\n> > * which the code goes through even for the df_conflict cases\n> > - * above. Well, it will once we don't die-not-implemented above.\n> > + * above.\n> > */\n>\n> This comment change might be a bit premature.\n\nOr perhaps it should have been squashed into an earlier series that\nwas already merged to next.\n"},{"id":"413211","messageId":"41ac7d44-e32b-99ed-c928-6325a02ba62c@gmail.com","threadId":"54844","inReplyTo":"CABPp-BFrct6hZW8bh5hhRpGSPhjeGO9viCjpO02gUyTqNnrAgg@mail.gmail.com","subject":"Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-31T11:17:44Z","receivedAt":"2020-12-31T11:20:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/30/2020 10:13 AM, Elijah Newren wrote:\n> On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee wrote:\n>>\n>> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n>>> + } else if (ci->df_conflict && ci->merged.result.mode != 0) {\n>>> die(\"Not yet implemented.\");\n>>> }\n>>>\n>>> /*\n>>> * NOTE: Below there is a long switch-like if-elseif-elseif... block\n>>> * which the code goes through even for the df_conflict cases\n>>> - * above. Well, it will once we don't die-not-implemented above.\n>>> + * above.\n>>> */\n>>\n>> This comment change might be a bit premature.\n> \n> Or perhaps it should have been squashed into an earlier series that\n> was already merged to next.\n \nI think it works with the next patch, which removes the die() from the\nif-elseif-elseif from immediately before the comment.\n\n-Stolee\n"},{"id":"413212","messageId":"0636b843-5dbd-2855-5c43-8f65a528bbc9@gmail.com","threadId":"54844","inReplyTo":"CABPp-BF37UeBdF4T68rvOaFRtPyZ2uoUSe48V4K8CUigy77CfA@mail.gmail.com","subject":"Re: [PATCH 03/10] merge-ort: implement unique_path() helper","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-31T11:19:55Z","receivedAt":"2020-12-31T11:21:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/30/2020 10:10 AM, Elijah Newren wrote:\n> On Wed, Dec 30, 2020 at 6:16 AM Derrick Stolee wrote:\n>> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>>\n>>> Implement unique_path(), based on the one from merge-recursive.c. It is\n>>> simplified, however, due to: (1) using strmaps, and (2) the fact that\n>>> merge-ort lets the checkout codepath handle possible collisions with the\n>>> working tree means that other code locations don't have to.\n>>>\n>>> Signed-off-by: Elijah Newren \n>>> ---\n>>> merge-ort.c | 25 ++++++++++++++++++++++++-\n>>> 1 file changed, 24 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/merge-ort.c b/merge-ort.c\n>>> index d300a02810e..1adc27a11bc 100644\n>>> --- a/merge-ort.c\n>>> +++ b/merge-ort.c\n>>> @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,\n>>> strbuf_addch(sb, '\\n');\n>>> }\n>>>\n>>> +/* add a string to a strbuf, but converting \"/\" to \"_\" */\n>>> +static void add_flattened_path(struct strbuf *out, const char *s)\n>>> +{\n>>> + size_t i = out->len;\n>>> + strbuf_addstr(out, s);\n>>> + for (; i < out->len; i++)\n>>> + if (out->buf[i] == '/')\n>>> + out->buf[i] = '_';\n>>> +}\n>>> +\n>>\n>> Thank you for pointing out that you based your code on merge-recursive.c.\n>> I see that this implementation is identical to the one there. I question\n>> whether this causes collisions in a problematic way, when \"a/b/c\" and\n>> \"a_b_c\" both exist in a tree.\n>>\n>> To avoid such a problem, we'd likely need to also expand \"_\" to \"__\" or\n>> similar. This might never actually affect any users because of the\n>> strange filename matches _and_ we need to be in a directory/file conflict.\n>>\n>> And maybe it's not a hole at all? If it is, we can consider patching or\n>> at least documenting the problem.\n> \n> add_flattened_path() can certainly result in a collision, regardless\n> of whether the char *s parameter has any '/' characters in it. For\n> example, if you are trying to get a unique path associated with\n> builtin/commit-graph.c due to changes from the 'next' branch side of\n> the merge, and builtin/commit-graph.c~next already exists, then you\n> have a collision. It's actually pretty rare that the parameter would\n> have any '/' characters at all, since it's pretty rare for me to see\n> folks (other than Junio) use hierarchical branch names. But if the\n> branch name were ds/line-log-on-bloom, then the provisional filename\n> would be builtin/commit-graph.c~ds_line-log-on-bloom. The '/' to '_'\n> conversion exists just to make sure our new file remains in the same\n> directory as where the conflict that caused us to need a new unique\n> path occurred.\n\nAh, I am misinterpreting which '/' characters are getting squashed.\nThank you for fixing my confusion.\n\n> But unique_path() does NOT end immediately after calling\n> add_flattened_path() and there is no collision possible in the return\n> from unique_path(), because it ends with a\n> while (strmap_contains(existing_paths, newpath.buf)) {\n> loop that modifies the resulting path until it finds one that doesn't\n> collide with an existing path.\n\nAnd this extra care here is helpful. Thanks!\n\n-Stolee\n\n"},{"id":"413224","messageId":"CABPp-BH6RPiTzmVM50HrOyq_Myyyuh_yQO3xwHv0vi-yZbw1Zg@mail.gmail.com","threadId":"54844","inReplyTo":"41ac7d44-e32b-99ed-c928-6325a02ba62c@gmail.com","subject":"Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-31T17:13:04Z","receivedAt":"2020-12-31T17:14:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 31, 2020 at 3:17 AM Derrick Stolee wrote:\n>\n> On 12/30/2020 10:13 AM, Elijah Newren wrote:\n> > On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee wrote:\n> >>\n> >> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:\n> >>> + } else if (ci->df_conflict && ci->merged.result.mode != 0) {\n> >>> die(\"Not yet implemented.\");\n> >>> }\n> >>>\n> >>> /*\n> >>> * NOTE: Below there is a long switch-like if-elseif-elseif... block\n> >>> * which the code goes through even for the df_conflict cases\n> >>> - * above. Well, it will once we don't die-not-implemented above.\n> >>> + * above.\n> >>> */\n> >>\n> >> This comment change might be a bit premature.\n> >\n> > Or perhaps it should have been squashed into an earlier series that\n> > was already merged to next.\n>\n> I think it works with the next patch, which removes the die() from the\n> if-elseif-elseif from immediately before the comment.\n\nOh, right, it's been long enough that I forgot the details and then I\nread the patch backwards thinking it was adding the message. Yeah, it\nshould go with the next patch. I'll fix it up.\n"},{"id":"413277","messageId":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.git.1608270687.gitgitgadget@gmail.com","subject":"[PATCH v2 00/10] merge-ort: add more handling of basic conflict types","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:38Z","receivedAt":"2021-01-01T02:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3\nor en/merge-ort-recursive).\n\nThis series adds handling of additional basic conflict types (directory/file\nconflicts, three-way content merging, very basic submodule divergence\nreconciliation, and different filetypes). This series drops the number of\ntest failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).\n\nFurther, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this\nseries are all merged down (in any order), then collectively they drop the\nnumber of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to\n60.\n\nChanges since v1:\n\n * Wait to remove comment about a die-not-implemented code block until the\n commit where we actually remove it (spotted by Stollee)\n\nElijah Newren (10):\n merge-ort: handle D/F conflict where directory disappears due to merge\n merge-ort: handle directory/file conflicts that remain\n merge-ort: implement unique_path() helper\n merge-ort: handle book-keeping around two- and three-way content merge\n merge-ort: flesh out implementation of handle_content_merge()\n merge-ort: copy and adapt merge_3way() from merge-recursive.c\n merge-ort: copy and adapt merge_submodule() from merge-recursive.c\n merge-ort: implement format_commit()\n merge-ort: copy find_first_merges() implementation from\n merge-recursive.c\n merge-ort: add handling for different types of files at same path\n\n merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 653 insertions(+), 18 deletions(-)\n\n\nbase-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/815\n\nRange-diff vs v1:\n\n 1: 382a009c18e ! 1: 1869e497482 merge-ort: handle D/F conflict where directory disappears due to merge\n @@ merge-ort.c: static void process_entry(struct merge_options *opt,\n \t\tdie(\"Not yet implemented.\");\n \t}\n \n - \t/*\n - \t * NOTE: Below there is a long switch-like if-elseif-elseif... block\n - \t * which the code goes through even for the df_conflict cases\n --\t * above. Well, it will once we don't die-not-implemented above.\n -+\t * above.\n - \t */\n - \tif (ci->match_mask) {\n - \t\tci->merged.clean = 1;\n 2: 46953226ba8 ! 2: 54f9be41a8a merge-ort: handle directory/file conflicts that remain\n @@ merge-ort.c: static void process_entry(struct merge_options *opt,\n \t}\n \n \t/*\n + \t * NOTE: Below there is a long switch-like if-elseif-elseif... block\n + \t * which the code goes through even for the df_conflict cases\n +-\t * above. Well, it will once we don't die-not-implemented above.\n ++\t * above.\n + \t */\n + \tif (ci->match_mask) {\n + \t\tci->merged.clean = 1;\n 3: 6ac555b3c0f = 3: 63fed5e49a7 merge-ort: implement unique_path() helper\n 4: 4c641ec19d5 = 4: d0fab13c78a merge-ort: handle book-keeping around two- and three-way content merge\n 5: 0e7321e67f8 = 5: 69129a20edc merge-ort: flesh out implementation of handle_content_merge()\n 6: 611141f24af = 6: d1cc76ac620 merge-ort: copy and adapt merge_3way() from merge-recursive.c\n 7: 4696f6c2d95 = 7: 2ddf6ece9d0 merge-ort: copy and adapt merge_submodule() from merge-recursive.c\n 8: a640cc0effc = 8: b0bfada5d81 merge-ort: implement format_commit()\n 9: b898876b119 = 9: 334cc7c65a6 merge-ort: copy find_first_merges() implementation from merge-recursive.c\n 10: 0a5778df253 = 10: 34eb647df40 merge-ort: add handling for different types of files at same path\n\n-- \ngitgitgadget\n"},{"id":"413278","messageId":"1869e497482a8ee7e1027d81dd900abf5f8be085.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:39Z","receivedAt":"2021-01-01T02:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen one side has a directory at a given path and the other side of\nhistory has a file at the path, but the merge resolves the directory\naway (e.g. because no path within that directory was modified and the\nother side deleted it, or because renaming moved all the files\nelsewhere), then we don't actually have a conflict anymore. We just\nneed to clear away any information related to the relevant directory,\nand then the subsequent process_entry() handling can handle the given\npath.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 23 ++++++++++++++++++++++-\n 1 file changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 414e7b7eeac..dd90987ae3d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -976,7 +976,28 @@ static void process_entry(struct merge_options *opt,\n \t\tassert(ci->df_conflict);\n \t}\n \n-\tif (ci->df_conflict) {\n+\tif (ci->df_conflict && ci->merged.result.mode == 0) {\n+\t\tint i;\n+\n+\t\t/*\n+\t\t * directory no longer in the way, but we do have a file we\n+\t\t * need to place here so we need to clean away the \"directory\n+\t\t * merges to nothing\" result.\n+\t\t */\n+\t\tci->df_conflict = 0;\n+\t\tassert(ci->filemask != 0);\n+\t\tci->merged.clean = 0;\n+\t\tci->merged.is_null = 0;\n+\t\t/* and we want to zero out any directory-related entries */\n+\t\tci->match_mask = (ci->match_mask & ~ci->dirmask);\n+\t\tci->dirmask = 0;\n+\t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n+\t\t\tif (ci->filemask & (1 << i))\n+\t\t\t\tcontinue;\n+\t\t\tci->stages[i].mode = 0;\n+\t\t\toidcpy(&ci->stages[i].oid, &null_oid);\n+\t\t}\n+\t} else if (ci->df_conflict && ci->merged.result.mode != 0) {\n \t\tdie(\"Not yet implemented.\");\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"413279","messageId":"69129a20edcfb57a3278285f31584ffcc2b3bad7.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:43Z","receivedAt":"2021-01-01T02:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis implementation is based heavily on merge_mode_and_contents() from\nmerge-recursive.c, though it has some fixes for recursive merges (i.e.\nwhen call_depth > 0), and has a number of changes throughout based on\nslight differences in data structures and in how the functions are\ncalled.\n\nIt is, however, based on two new helper functions -- merge_3way() and\nmerge_submodule -- for which we only provide die-not-implemented stubs\nat this point. Future commits will add implementations of these\nfunctions.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 143 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 47e230fe341..2cfb7ffa3b0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -631,6 +631,28 @@ static int collect_merge_info(struct merge_options *opt,\n \n /*** Function Grouping: functions related to threeway content merges ***/\n \n+static int merge_submodule(struct merge_options *opt,\n+\t\t\t const char *path,\n+\t\t\t const struct object_id *o,\n+\t\t\t const struct object_id *a,\n+\t\t\t const struct object_id *b,\n+\t\t\t struct object_id *result)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n+static int merge_3way(struct merge_options *opt,\n+\t\t const char *path,\n+\t\t const struct object_id *o,\n+\t\t const struct object_id *a,\n+\t\t const struct object_id *b,\n+\t\t const char *pathnames[3],\n+\t\t const int extra_marker_size,\n+\t\t mmbuffer_t *result_buf)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst char *path,\n \t\t\t\tconst struct version_info *o,\n@@ -640,14 +662,129 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst int extra_marker_size,\n \t\t\t\tstruct version_info *result)\n {\n-\tint clean = 0;\n \t/*\n-\t * TODO: Needs a two-way or three-way content merge, but we're\n-\t * just being lazy and copying the version from HEAD and\n-\t * leaving it as conflicted.\n+\t * path is the target location where we want to put the file, and\n+\t * is used to determine any normalization rules in ll_merge.\n+\t *\n+\t * The normal case is that path and all entries in pathnames are\n+\t * identical, though renames can affect which path we got one of\n+\t * the three blobs to merge on various sides of history.\n+\t *\n+\t * extra_marker_size is the amount to extend conflict markers in\n+\t * ll_merge; this is neeed if we have content merges of content\n+\t * merges, which happens for example with rename/rename(2to1) and\n+\t * rename/add conflicts.\n \t */\n-\tresult->mode = a->mode;\n-\toidcpy(&result->oid, &a->oid);\n+\tunsigned clean = 1;\n+\n+\t/*\n+\t * handle_content_merge() needs both files to be of the same type, i.e.\n+\t * both files OR both submodules OR both symlinks. Conflicting types\n+\t * needs to be handled elsewhere.\n+\t */\n+\tassert((S_IFMT & a->mode) == (S_IFMT & b->mode));\n+\n+\t/* Merge modes */\n+\tif (a->mode == b->mode || a->mode == o->mode)\n+\t\tresult->mode = b->mode;\n+\telse {\n+\t\t/* must be the 100644/100755 case */\n+\t\tassert(S_ISREG(a->mode));\n+\t\tresult->mode = a->mode;\n+\t\tclean = (b->mode == o->mode);\n+\t\t/*\n+\t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n+\t\t * should not make result->mode match either a->mode or\n+\t\t * b->mode; that causes t6036 \"check conflicting mode for\n+\t\t * regular file\" to fail. It would be best to use some other\n+\t\t * mode, but we'll confuse all kinds of stuff if we use one\n+\t\t * where S_ISREG(result->mode) isn't true, and if we use\n+\t\t * something like 0100666, then tree-walk.c's calls to\n+\t\t * canon_mode() will just normalize that to 100644 for us and\n+\t\t * thus not solve anything.\n+\t\t *\n+\t\t * Figure out if there's some kind of way we can work around\n+\t\t * this...\n+\t\t */\n+\t}\n+\n+\t/*\n+\t * Trivial oid merge.\n+\t *\n+\t * Note: While one might assume that the next four lines would\n+\t * be unnecessary due to the fact that match_mask is often\n+\t * setup and already handled, renames don't always take care\n+\t * of that.\n+\t */\n+\tif (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid))\n+\t\toidcpy(&result->oid, &b->oid);\n+\telse if (oideq(&b->oid, &o->oid))\n+\t\toidcpy(&result->oid, &a->oid);\n+\n+\t/* Remaining rules depend on file vs. submodule vs. symlink. */\n+\telse if (S_ISREG(a->mode)) {\n+\t\tmmbuffer_t result_buf;\n+\t\tint ret = 0, merge_status;\n+\t\tint two_way;\n+\n+\t\t/*\n+\t\t * If 'o' is different type, treat it as null so we do a\n+\t\t * two-way merge.\n+\t\t */\n+\t\ttwo_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));\n+\n+\t\tmerge_status = merge_3way(opt, path,\n+\t\t\t\t\t two_way ? &null_oid : &o->oid,\n+\t\t\t\t\t &a->oid, &b->oid,\n+\t\t\t\t\t pathnames, extra_marker_size,\n+\t\t\t\t\t &result_buf);\n+\n+\t\tif ((merge_status < 0) || !result_buf.ptr)\n+\t\t\tret = err(opt, _(\"Failed to execute internal merge\"));\n+\n+\t\tif (!ret &&\n+\t\t write_object_file(result_buf.ptr, result_buf.size,\n+\t\t\t\t blob_type, &result->oid))\n+\t\t\tret = err(opt, _(\"Unable to add %s to database\"),\n+\t\t\t\t path);\n+\n+\t\tfree(result_buf.ptr);\n+\t\tif (ret)\n+\t\t\treturn -1;\n+\t\tclean &= (merge_status == 0);\n+\t\tpath_msg(opt, path, 1, _(\"Auto-merging %s\"), path);\n+\t} else if (S_ISGITLINK(a->mode)) {\n+\t\tint two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));\n+\t\tclean = merge_submodule(opt, pathnames[0],\n+\t\t\t\t\ttwo_way ? &null_oid : &o->oid,\n+\t\t\t\t\t&a->oid, &b->oid, &result->oid);\n+\t\tif (opt->priv->call_depth && two_way && !clean) {\n+\t\t\tresult->mode = o->mode;\n+\t\t\toidcpy(&result->oid, &o->oid);\n+\t\t}\n+\t} else if (S_ISLNK(a->mode)) {\n+\t\tif (opt->priv->call_depth) {\n+\t\t\tclean = 0;\n+\t\t\tresult->mode = o->mode;\n+\t\t\toidcpy(&result->oid, &o->oid);\n+\t\t} else {\n+\t\t\tswitch (opt->recursive_variant) {\n+\t\t\tcase MERGE_VARIANT_NORMAL:\n+\t\t\t\tclean = 0;\n+\t\t\t\toidcpy(&result->oid, &a->oid);\n+\t\t\t\tbreak;\n+\t\t\tcase MERGE_VARIANT_OURS:\n+\t\t\t\toidcpy(&result->oid, &a->oid);\n+\t\t\t\tbreak;\n+\t\t\tcase MERGE_VARIANT_THEIRS:\n+\t\t\t\toidcpy(&result->oid, &b->oid);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t} else\n+\t\tBUG(\"unsupported object type in the tree: %06o for %s\",\n+\t\t a->mode, path);\n+\n \treturn clean;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"413280","messageId":"63fed5e49a734b11246f91d480ff4a27cc98db14.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 03/10] merge-ort: implement unique_path() helper","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:41Z","receivedAt":"2021-01-01T02:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nImplement unique_path(), based on the one from merge-recursive.c. It is\nsimplified, however, due to: (1) using strmaps, and (2) the fact that\nmerge-ort lets the checkout codepath handle possible collisions with the\nworking tree means that other code locations don't have to.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d300a02810e..1adc27a11bc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/* add a string to a strbuf, but converting \"/\" to \"_\" */\n+static void add_flattened_path(struct strbuf *out, const char *s)\n+{\n+\tsize_t i = out->len;\n+\tstrbuf_addstr(out, s);\n+\tfor (; i < out->len; i++)\n+\t\tif (out->buf[i] == '/')\n+\t\t\tout->buf[i] = '_';\n+}\n+\n static char *unique_path(struct strmap *existing_paths,\n \t\t\t const char *path,\n \t\t\t const char *branch)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct strbuf newpath = STRBUF_INIT;\n+\tint suffix = 0;\n+\tsize_t base_len;\n+\n+\tstrbuf_addf(&newpath, \"%s~\", path);\n+\tadd_flattened_path(&newpath, branch);\n+\n+\tbase_len = newpath.len;\n+\twhile (strmap_contains(existing_paths, newpath.buf)) {\n+\t\tstrbuf_setlen(&newpath, base_len);\n+\t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n+\t}\n+\n+\treturn strbuf_detach(&newpath, NULL);\n }\n \n /*** Function Grouping: functions related to collect_merge_info() ***/\n-- \ngitgitgadget\n\n"},{"id":"413281","messageId":"d0fab13c78aee3a7232f58625500acb8b16a3e34.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 04/10] merge-ort: handle book-keeping around two- and three-way content merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:42Z","receivedAt":"2021-01-01T02:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn addition to the content merge (which will go in a subsequent commit),\nwe need to worry about conflict messages, placing results in higher\norder stages in case of a df_conflict, and making sure the results are\nplaced in ci->merged.result so that they will show up in the working\ntree. Take care of all that external book-keeping, moving the\nsimplistic just-take-HEAD code into the barebones handle_content_merge()\nfunction for now. Subsequent commits will flesh out\nhandle_content_merge().\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 41 insertions(+), 11 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 1adc27a11bc..47e230fe341 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -640,7 +640,15 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\tconst int extra_marker_size,\n \t\t\t\tstruct version_info *result)\n {\n-\tdie(\"Not yet implemented\");\n+\tint clean = 0;\n+\t/*\n+\t * TODO: Needs a two-way or three-way content merge, but we're\n+\t * just being lazy and copying the version from HEAD and\n+\t * leaving it as conflicted.\n+\t */\n+\tresult->mode = a->mode;\n+\toidcpy(&result->oid, &a->oid);\n+\treturn clean;\n }\n \n /*** Function Grouping: functions related to detect_and_process_renames(), ***\n@@ -1138,16 +1146,38 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tdie(\"Not yet implemented.\");\n \t} else if (ci->filemask >= 6) {\n-\t\t/*\n-\t\t * TODO: Needs a two-way or three-way content merge, but we're\n-\t\t * just being lazy and copying the version from HEAD and\n-\t\t * leaving it as conflicted.\n-\t\t */\n-\t\tci->merged.clean = 0;\n-\t\tci->merged.result.mode = ci->stages[1].mode;\n-\t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n-\t\t/* When we fix above, we'll call handle_content_merge() */\n-\t\t(void)handle_content_merge;\n+\t\t/* Need a two-way or three-way content merge */\n+\t\tstruct version_info merged_file;\n+\t\tunsigned clean_merge;\n+\t\tstruct version_info *o = &ci->stages[0];\n+\t\tstruct version_info *a = &ci->stages[1];\n+\t\tstruct version_info *b = &ci->stages[2];\n+\n+\t\tclean_merge = handle_content_merge(opt, path, o, a, b,\n+\t\t\t\t\t\t ci->pathnames,\n+\t\t\t\t\t\t opt->priv->call_depth * 2,\n+\t\t\t\t\t\t &merged_file);\n+\t\tci->merged.clean = clean_merge &&\n+\t\t\t\t !ci->df_conflict && !ci->path_conflict;\n+\t\tci->merged.result.mode = merged_file.mode;\n+\t\tci->merged.is_null = (merged_file.mode == 0);\n+\t\toidcpy(&ci->merged.result.oid, &merged_file.oid);\n+\t\tif (clean_merge && ci->df_conflict) {\n+\t\t\tassert(df_file_index == 1 || df_file_index == 2);\n+\t\t\tci->filemask = 1 << df_file_index;\n+\t\t\tci->stages[df_file_index].mode = merged_file.mode;\n+\t\t\toidcpy(&ci->stages[df_file_index].oid, &merged_file.oid);\n+\t\t}\n+\t\tif (!clean_merge) {\n+\t\t\tconst char *reason = _(\"content\");\n+\t\t\tif (ci->filemask == 6)\n+\t\t\t\treason = _(\"add/add\");\n+\t\t\tif (S_ISGITLINK(merged_file.mode))\n+\t\t\t\treason = _(\"submodule\");\n+\t\t\tpath_msg(opt, path, 0,\n+\t\t\t\t _(\"CONFLICT (%s): Merge conflict in %s\"),\n+\t\t\t\t reason, path);\n+\t\t}\n \t} else if (ci->filemask == 3 || ci->filemask == 5) {\n \t\t/* Modify/delete */\n \t\tconst char *modify_branch, *delete_branch;\n-- \ngitgitgadget\n\n"},{"id":"413282","messageId":"2ddf6ece9d0e3ec4d98d33f56e168466439ae421.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 07/10] merge-ort: copy and adapt merge_submodule() from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:45Z","receivedAt":"2021-01-01T02:36:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nTake merge_submodule() from merge-recursive.c and make slight\nadjustments, predominantly around deferring output using path_msg()\ninstead of using merge-recursive's output() and show() functions.\nThere's also a fix for recursive cases (when call_depth > 0) and a\nslight change to argument order for find_first_merges().\n\nfind_first_merges() and format_commit() are left unimplemented for\nnow, but will be added by subsequent commits.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 125 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a59adb42aa6..2dfab1858fc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -26,6 +26,7 @@\n #include \"ll-merge.h\"\n #include \"object-store.h\"\n #include \"strmap.h\"\n+#include \"submodule.h\"\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n #include \"xdiff-interface.h\"\n@@ -323,6 +324,13 @@ static int err(struct merge_options *opt, const char *err, ...)\n \treturn -1;\n }\n \n+static void format_commit(struct strbuf *sb,\n+\t\t\t int indent,\n+\t\t\t struct commit *commit)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n __attribute__((format (printf, 4, 5)))\n static void path_msg(struct merge_options *opt,\n \t\t const char *path,\n@@ -632,6 +640,15 @@ static int collect_merge_info(struct merge_options *opt,\n \n /*** Function Grouping: functions related to threeway content merges ***/\n \n+static int find_first_merges(struct repository *repo,\n+\t\t\t const char *path,\n+\t\t\t struct commit *a,\n+\t\t\t struct commit *b,\n+\t\t\t struct object_array *result)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n static int merge_submodule(struct merge_options *opt,\n \t\t\t const char *path,\n \t\t\t const struct object_id *o,\n@@ -639,7 +656,114 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t const struct object_id *b,\n \t\t\t struct object_id *result)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct commit *commit_o, *commit_a, *commit_b;\n+\tint parent_count;\n+\tstruct object_array merges;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tint i;\n+\tint search = !opt->priv->call_depth;\n+\n+\t/* store fallback answer in result in case we fail */\n+\toidcpy(result, opt->priv->call_depth ? o : a);\n+\n+\t/* we can not handle deletion conflicts */\n+\tif (is_null_oid(o))\n+\t\treturn 0;\n+\tif (is_null_oid(a))\n+\t\treturn 0;\n+\tif (is_null_oid(b))\n+\t\treturn 0;\n+\n+\tif (add_submodule_odb(path)) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s (not checked out)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!(commit_o = lookup_commit_reference(opt->repo, o)) ||\n+\t !(commit_a = lookup_commit_reference(opt->repo, a)) ||\n+\t !(commit_b = lookup_commit_reference(opt->repo, b))) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s (commits not present)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\t/* check whether both changes are forward */\n+\tif (!in_merge_bases(commit_o, commit_a) ||\n+\t !in_merge_bases(commit_o, commit_b)) {\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t \"(commits don't follow merge-base)\"),\n+\t\t\t path);\n+\t\treturn 0;\n+\t}\n+\n+\t/* Case #1: a is contained in b or vice versa */\n+\tif (in_merge_bases(commit_a, commit_b)) {\n+\t\toidcpy(result, b);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n+\t\t\t path, oid_to_hex(b));\n+\t\treturn 1;\n+\t}\n+\tif (in_merge_bases(commit_b, commit_a)) {\n+\t\toidcpy(result, a);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"Note: Fast-forwarding submodule %s to %s\"),\n+\t\t\t path, oid_to_hex(a));\n+\t\treturn 1;\n+\t}\n+\n+\t/*\n+\t * Case #2: There are one or more merges that contain a and b in\n+\t * the submodule. If there is only one, then present it as a\n+\t * suggestion to the user, but leave it marked unmerged so the\n+\t * user needs to confirm the resolution.\n+\t */\n+\n+\t/* Skip the search if makes no sense to the calling context. */\n+\tif (!search)\n+\t\treturn 0;\n+\n+\t/* find commit which merges them */\n+\tparent_count = find_first_merges(opt->repo, path, commit_a, commit_b,\n+\t\t\t\t\t &merges);\n+\tswitch (parent_count) {\n+\tcase 0:\n+\t\tpath_msg(opt, path, 0, _(\"Failed to merge submodule %s\"), path);\n+\t\tbreak;\n+\n+\tcase 1:\n+\t\tformat_commit(&sb, 4,\n+\t\t\t (struct commit *)merges.objects[0].item);\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s, but a possible merge \"\n+\t\t\t \"resolution exists:\\n%s\\n\"),\n+\t\t\t path, sb.buf);\n+\t\tpath_msg(opt, path, 1,\n+\t\t\t _(\"If this is correct simply add it to the index \"\n+\t\t\t \"for example\\n\"\n+\t\t\t \"by using:\\n\\n\"\n+\t\t\t \" git update-index --cacheinfo 160000 %s \\\"%s\\\"\\n\\n\"\n+\t\t\t \"which will accept this suggestion.\\n\"),\n+\t\t\t oid_to_hex(&merges.objects[0].item->oid), path);\n+\t\tstrbuf_release(&sb);\n+\t\tbreak;\n+\tdefault:\n+\t\tfor (i = 0; i < merges.nr; i++)\n+\t\t\tformat_commit(&sb, 4,\n+\t\t\t\t (struct commit *)merges.objects[i].item);\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"Failed to merge submodule %s, but multiple \"\n+\t\t\t \"possible merges exist:\\n%s\"), path, sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tobject_array_clear(&merges);\n+\treturn 0;\n }\n \n static int merge_3way(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"413283","messageId":"54f9be41a8a053985533247325013fe45c5c2635.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 02/10] merge-ort: handle directory/file conflicts that remain","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:40Z","receivedAt":"2021-01-01T02:36:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen a directory/file conflict remains, we can leave the directory where\nit is, but need to move the information about the file to a different\npathname. After moving the file to a different pathname, we allow\nsubsequent process_entry() logic to handle any additional details that\nmight be relevant.\n\nThis depends on a new helper function, unique_path(), that dies with an\nunimplemented error currently but will be implemented in a subsequent\ncommit.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 84 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex dd90987ae3d..d300a02810e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -343,6 +343,13 @@ static void path_msg(struct merge_options *opt,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+static char *unique_path(struct strmap *existing_paths,\n+\t\t\t const char *path,\n+\t\t\t const char *branch)\n+{\n+\tdie(\"Not yet implemented.\");\n+}\n+\n /*** Function Grouping: functions related to collect_merge_info() ***/\n \n static void setup_path_info(struct merge_options *opt,\n@@ -962,6 +969,8 @@ static void process_entry(struct merge_options *opt,\n \t\t\t struct conflict_info *ci,\n \t\t\t struct directory_versions *dir_metadata)\n {\n+\tint df_file_index = 0;\n+\n \tVERIFY_CI(ci);\n \tassert(ci->filemask >= 0 && ci->filemask <= 7);\n \t/* ci->match_mask == 7 was handled in collect_merge_info_callback() */\n@@ -998,13 +1007,86 @@ static void process_entry(struct merge_options *opt,\n \t\t\toidcpy(&ci->stages[i].oid, &null_oid);\n \t\t}\n \t} else if (ci->df_conflict && ci->merged.result.mode != 0) {\n-\t\tdie(\"Not yet implemented.\");\n+\t\t/*\n+\t\t * This started out as a D/F conflict, and the entries in\n+\t\t * the competing directory were not removed by the merge as\n+\t\t * evidenced by write_completed_directory() writing a value\n+\t\t * to ci->merged.result.mode.\n+\t\t */\n+\t\tstruct conflict_info *new_ci;\n+\t\tconst char *branch;\n+\t\tconst char *old_path = path;\n+\t\tint i;\n+\n+\t\tassert(ci->merged.result.mode == S_IFDIR);\n+\n+\t\t/*\n+\t\t * If filemask is 1, we can just ignore the file as having\n+\t\t * been deleted on both sides. We do not want to overwrite\n+\t\t * ci->merged.result, since it stores the tree for all the\n+\t\t * files under it.\n+\t\t */\n+\t\tif (ci->filemask == 1) {\n+\t\t\tci->filemask = 0;\n+\t\t\treturn;\n+\t\t}\n+\n+\t\t/*\n+\t\t * This file still exists on at least one side, and we want\n+\t\t * the directory to remain here, so we need to move this\n+\t\t * path to some new location.\n+\t\t */\n+\t\tnew_ci = xcalloc(1, sizeof(*new_ci));\n+\t\t/* We don't really want new_ci->merged.result copied, but it'll\n+\t\t * be overwritten below so it doesn't matter. We also don't\n+\t\t * want any directory mode/oid values copied, but we'll zero\n+\t\t * those out immediately. We do want the rest of ci copied.\n+\t\t */\n+\t\tmemcpy(new_ci, ci, sizeof(*ci));\n+\t\tnew_ci->match_mask = (new_ci->match_mask & ~new_ci->dirmask);\n+\t\tnew_ci->dirmask = 0;\n+\t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n+\t\t\tif (new_ci->filemask & (1 << i))\n+\t\t\t\tcontinue;\n+\t\t\t/* zero out any entries related to directories */\n+\t\t\tnew_ci->stages[i].mode = 0;\n+\t\t\toidcpy(&new_ci->stages[i].oid, &null_oid);\n+\t\t}\n+\n+\t\t/*\n+\t\t * Find out which side this file came from; note that we\n+\t\t * cannot just use ci->filemask, because renames could cause\n+\t\t * the filemask to go back to 7. So we use dirmask, then\n+\t\t * pick the opposite side's index.\n+\t\t */\n+\t\tdf_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;\n+\t\tbranch = (df_file_index == 1) ? opt->branch1 : opt->branch2;\n+\t\tpath = unique_path(&opt->priv->paths, path, branch);\n+\t\tstrmap_put(&opt->priv->paths, path, new_ci);\n+\n+\t\tpath_msg(opt, path, 0,\n+\t\t\t _(\"CONFLICT (file/directory): directory in the way \"\n+\t\t\t \"of %s from %s; moving it to %s instead.\"),\n+\t\t\t old_path, branch, path);\n+\n+\t\t/*\n+\t\t * Zero out the filemask for the old ci. At this point, ci\n+\t\t * was just an entry for a directory, so we don't need to\n+\t\t * do anything more with it.\n+\t\t */\n+\t\tci->filemask = 0;\n+\n+\t\t/*\n+\t\t * Now note that we're working on the new entry (path was\n+\t\t * updated above.\n+\t\t */\n+\t\tci = new_ci;\n \t}\n \n \t/*\n \t * NOTE: Below there is a long switch-like if-elseif-elseif... block\n \t * which the code goes through even for the df_conflict cases\n-\t * above. Well, it will once we don't die-not-implemented above.\n+\t * above.\n \t */\n \tif (ci->match_mask) {\n \t\tci->merged.clean = 1;\n-- \ngitgitgadget\n\n"},{"id":"413284","messageId":"334cc7c65a60fa45b2c0e418d6b22d4dd53d3e71.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:47Z","receivedAt":"2021-01-01T02:36:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCode is identical for the function body in the two files, the call\nsignature is just slightly different in merge-ort than merge-recursive\nas noted a couple commits ago.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex bf704bcd34d..203fa987e43 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -25,6 +25,7 @@\n #include \"dir.h\"\n #include \"ll-merge.h\"\n #include \"object-store.h\"\n+#include \"revision.h\"\n #include \"strmap.h\"\n #include \"submodule.h\"\n #include \"tree.h\"\n@@ -658,7 +659,61 @@ static int find_first_merges(struct repository *repo,\n \t\t\t struct commit *b,\n \t\t\t struct object_array *result)\n {\n-\tdie(\"Not yet implemented.\");\n+\tint i, j;\n+\tstruct object_array merges = OBJECT_ARRAY_INIT;\n+\tstruct commit *commit;\n+\tint contains_another;\n+\n+\tchar merged_revision[GIT_MAX_HEXSZ + 2];\n+\tconst char *rev_args[] = { \"rev-list\", \"--merges\", \"--ancestry-path\",\n+\t\t\t\t \"--all\", merged_revision, NULL };\n+\tstruct rev_info revs;\n+\tstruct setup_revision_opt rev_opts;\n+\n+\tmemset(result, 0, sizeof(struct object_array));\n+\tmemset(&rev_opts, 0, sizeof(rev_opts));\n+\n+\t/* get all revisions that merge commit a */\n+\txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n+\t\t oid_to_hex(&a->object.oid));\n+\trepo_init_revisions(repo, &revs, NULL);\n+\trev_opts.submodule = path;\n+\t/* FIXME: can't handle linked worktrees in submodules yet */\n+\trevs.single_worktree = path != NULL;\n+\tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n+\n+\t/* save all revisions from the above list that contain b */\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(\"revision walk setup failed\");\n+\twhile ((commit = get_revision(&revs)) != NULL) {\n+\t\tstruct object *o = &(commit->object);\n+\t\tif (in_merge_bases(b, commit))\n+\t\t\tadd_object_array(o, NULL, &merges);\n+\t}\n+\treset_revision_walk();\n+\n+\t/* Now we've got all merges that contain a and b. Prune all\n+\t * merges that contain another found merge and save them in\n+\t * result.\n+\t */\n+\tfor (i = 0; i < merges.nr; i++) {\n+\t\tstruct commit *m1 = (struct commit *) merges.objects[i].item;\n+\n+\t\tcontains_another = 0;\n+\t\tfor (j = 0; j < merges.nr; j++) {\n+\t\t\tstruct commit *m2 = (struct commit *) merges.objects[j].item;\n+\t\t\tif (i != j && in_merge_bases(m2, m1)) {\n+\t\t\t\tcontains_another = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!contains_another)\n+\t\t\tadd_object_array(merges.objects[i].item, NULL, result);\n+\t}\n+\n+\tobject_array_clear(&merges);\n+\treturn result->nr;\n }\n \n static int merge_submodule(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"413286","messageId":"b0bfada5d8151429513821b4bd692ba28b3f9689.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 08/10] merge-ort: implement format_commit()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:46Z","receivedAt":"2021-01-01T02:36:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis implementation is based on a mixture of print_commit() and\noutput_commit_title() from merge-recursive.c so that it can be used to\ntake over both functions.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 2dfab1858fc..bf704bcd34d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -328,7 +328,19 @@ static void format_commit(struct strbuf *sb,\n \t\t\t int indent,\n \t\t\t struct commit *commit)\n {\n-\tdie(\"Not yet implemented.\");\n+\tstruct merge_remote_desc *desc;\n+\tstruct pretty_print_context ctx = {0};\n+\tctx.abbrev = DEFAULT_ABBREV;\n+\n+\tstrbuf_addchars(sb, ' ', indent);\n+\tdesc = merge_remote_util(commit);\n+\tif (desc) {\n+\t\tstrbuf_addf(sb, \"virtual %s\\n\", desc->name);\n+\t\treturn;\n+\t}\n+\n+\tformat_commit_message(commit, \"%h %s\", sb, &ctx);\n+\tstrbuf_addch(sb, '\\n');\n }\n \n __attribute__((format (printf, 4, 5)))\n-- \ngitgitgadget\n\n"},{"id":"413285","messageId":"34eb647df40aa61ac961753118112ca2695eab4c.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 10/10] merge-ort: add handling for different types of files at same path","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:48Z","receivedAt":"2021-01-01T02:36:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAdd some handling that explicitly considers collisions of the following\ntypes:\n * file/submodule\n * file/symlink\n * submodule/symlink\nLeaving them as conflicts at the same path are hard for users to\nresolve, so move one or both of them aside so that they each get their\nown path.\n\nNote that in the case of recursive handling (i.e. call_depth > 0), we\ncan just use the merge base of the two merge bases as the merge result\nmuch like we do with modify/delete conflicts, binary files, conflicting\nsubmodule values, and so on.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 103 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 203fa987e43..afe721182e2 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1521,10 +1521,109 @@ static void process_entry(struct merge_options *opt,\n \t} else if (ci->filemask >= 6 &&\n \t\t (S_IFMT & ci->stages[1].mode) !=\n \t\t (S_IFMT & ci->stages[2].mode)) {\n-\t\t/*\n-\t\t * Two different items from (file/submodule/symlink)\n-\t\t */\n-\t\tdie(\"Not yet implemented.\");\n+\t\t/* Two different items from (file/submodule/symlink) */\n+\t\tif (opt->priv->call_depth) {\n+\t\t\t/* Just use the version from the merge base */\n+\t\t\tci->merged.clean = 0;\n+\t\t\toidcpy(&ci->merged.result.oid, &ci->stages[0].oid);\n+\t\t\tci->merged.result.mode = ci->stages[0].mode;\n+\t\t\tci->merged.is_null = (ci->merged.result.mode == 0);\n+\t\t} else {\n+\t\t\t/* Handle by renaming one or both to separate paths. */\n+\t\t\tunsigned o_mode = ci->stages[0].mode;\n+\t\t\tunsigned a_mode = ci->stages[1].mode;\n+\t\t\tunsigned b_mode = ci->stages[2].mode;\n+\t\t\tstruct conflict_info *new_ci;\n+\t\t\tconst char *a_path = NULL, *b_path = NULL;\n+\t\t\tint rename_a = 0, rename_b = 0;\n+\n+\t\t\tnew_ci = xmalloc(sizeof(*new_ci));\n+\n+\t\t\tif (S_ISREG(a_mode))\n+\t\t\t\trename_a = 1;\n+\t\t\telse if (S_ISREG(b_mode))\n+\t\t\t\trename_b = 1;\n+\t\t\telse {\n+\t\t\t\trename_a = 1;\n+\t\t\t\trename_b = 1;\n+\t\t\t}\n+\n+\t\t\tpath_msg(opt, path, 0,\n+\t\t\t\t _(\"CONFLICT (distinct types): %s had different \"\n+\t\t\t\t \"types on each side; renamed %s of them so \"\n+\t\t\t\t \"each can be recorded somewhere.\"),\n+\t\t\t\t path,\n+\t\t\t\t (rename_a && rename_b) ? _(\"both\") : _(\"one\"));\n+\n+\t\t\tci->merged.clean = 0;\n+\t\t\tmemcpy(new_ci, ci, sizeof(*new_ci));\n+\n+\t\t\t/* Put b into new_ci, removing a from stages */\n+\t\t\tnew_ci->merged.result.mode = ci->stages[2].mode;\n+\t\t\toidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid);\n+\t\t\tnew_ci->stages[1].mode = 0;\n+\t\t\toidcpy(&new_ci->stages[1].oid, &null_oid);\n+\t\t\tnew_ci->filemask = 5;\n+\t\t\tif ((S_IFMT & b_mode) != (S_IFMT & o_mode)) {\n+\t\t\t\tnew_ci->stages[0].mode = 0;\n+\t\t\t\toidcpy(&new_ci->stages[0].oid, &null_oid);\n+\t\t\t\tnew_ci->filemask = 4;\n+\t\t\t}\n+\n+\t\t\t/* Leave only a in ci, fixing stages. */\n+\t\t\tci->merged.result.mode = ci->stages[1].mode;\n+\t\t\toidcpy(&ci->merged.result.oid, &ci->stages[1].oid);\n+\t\t\tci->stages[2].mode = 0;\n+\t\t\toidcpy(&ci->stages[2].oid, &null_oid);\n+\t\t\tci->filemask = 3;\n+\t\t\tif ((S_IFMT & a_mode) != (S_IFMT & o_mode)) {\n+\t\t\t\tci->stages[0].mode = 0;\n+\t\t\t\toidcpy(&ci->stages[0].oid, &null_oid);\n+\t\t\t\tci->filemask = 2;\n+\t\t\t}\n+\n+\t\t\t/* Insert entries into opt->priv_paths */\n+\t\t\tassert(rename_a || rename_b);\n+\t\t\tif (rename_a) {\n+\t\t\t\ta_path = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t path, opt->branch1);\n+\t\t\t\tstrmap_put(&opt->priv->paths, a_path, ci);\n+\t\t\t}\n+\n+\t\t\tif (rename_b)\n+\t\t\t\tb_path = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t path, opt->branch2);\n+\t\t\telse\n+\t\t\t\tb_path = path;\n+\t\t\tstrmap_put(&opt->priv->paths, b_path, new_ci);\n+\n+\t\t\tif (rename_a && rename_b) {\n+\t\t\t\tstrmap_remove(&opt->priv->paths, path, 0);\n+\t\t\t\t/*\n+\t\t\t\t * We removed path from opt->priv->paths. path\n+\t\t\t\t * will also eventually need to be freed, but\n+\t\t\t\t * it may still be used by e.g. ci->pathnames.\n+\t\t\t\t * So, store it in another string-list for now.\n+\t\t\t\t */\n+\t\t\t\tstring_list_append(&opt->priv->paths_to_free,\n+\t\t\t\t\t\t path);\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * Do special handling for b_path since process_entry()\n+\t\t\t * won't be called on it specially.\n+\t\t\t */\n+\t\t\tstrmap_put(&opt->priv->conflicted, b_path, new_ci);\n+\t\t\trecord_entry_for_tree(dir_metadata, b_path,\n+\t\t\t\t\t &new_ci->merged);\n+\n+\t\t\t/*\n+\t\t\t * Remaining code for processing this entry should\n+\t\t\t * think in terms of processing a_path.\n+\t\t\t */\n+\t\t\tif (a_path)\n+\t\t\t\tpath = a_path;\n+\t\t}\n \t} else if (ci->filemask >= 6) {\n \t\t/* Need a two-way or three-way content merge */\n \t\tstruct version_info merged_file;\n-- \ngitgitgadget\n"},{"id":"413287","messageId":"d1cc76ac620151e3d8c71b6b0b5fe6ed65c170c3.1609468488.git.gitgitgadget@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"[PATCH v2 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-01T02:34:44Z","receivedAt":"2021-01-01T02:36:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nTake merge_3way() from merge-recursive.c and make slight adjustments\nbased on different data structures (direct usage of object_id\nrather diff_filespec, separate pathnames which based on our careful\ninterning of pathnames in opt->priv->paths can be compared with '!='\nrather than 'strcmp').\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 2cfb7ffa3b0..a59adb42aa6 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -23,6 +23,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"dir.h\"\n+#include \"ll-merge.h\"\n #include \"object-store.h\"\n #include \"strmap.h\"\n #include \"tree.h\"\n@@ -650,7 +651,58 @@ static int merge_3way(struct merge_options *opt,\n \t\t const int extra_marker_size,\n \t\t mmbuffer_t *result_buf)\n {\n-\tdie(\"Not yet implemented.\");\n+\tmmfile_t orig, src1, src2;\n+\tstruct ll_merge_options ll_opts = {0};\n+\tchar *base, *name1, *name2;\n+\tint merge_status;\n+\n+\tll_opts.renormalize = opt->renormalize;\n+\tll_opts.extra_marker_size = extra_marker_size;\n+\tll_opts.xdl_opts = opt->xdl_opts;\n+\n+\tif (opt->priv->call_depth) {\n+\t\tll_opts.virtual_ancestor = 1;\n+\t\tll_opts.variant = 0;\n+\t} else {\n+\t\tswitch (opt->recursive_variant) {\n+\t\tcase MERGE_VARIANT_OURS:\n+\t\t\tll_opts.variant = XDL_MERGE_FAVOR_OURS;\n+\t\t\tbreak;\n+\t\tcase MERGE_VARIANT_THEIRS:\n+\t\t\tll_opts.variant = XDL_MERGE_FAVOR_THEIRS;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tll_opts.variant = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tassert(pathnames[0] && pathnames[1] && pathnames[2] && opt->ancestor);\n+\tif (pathnames[0] == pathnames[1] && pathnames[1] == pathnames[2]) {\n+\t\tbase = mkpathdup(\"%s\", opt->ancestor);\n+\t\tname1 = mkpathdup(\"%s\", opt->branch1);\n+\t\tname2 = mkpathdup(\"%s\", opt->branch2);\n+\t} else {\n+\t\tbase = mkpathdup(\"%s:%s\", opt->ancestor, pathnames[0]);\n+\t\tname1 = mkpathdup(\"%s:%s\", opt->branch1, pathnames[1]);\n+\t\tname2 = mkpathdup(\"%s:%s\", opt->branch2, pathnames[2]);\n+\t}\n+\n+\tread_mmblob(&orig, o);\n+\tread_mmblob(&src1, a);\n+\tread_mmblob(&src2, b);\n+\n+\tmerge_status = ll_merge(result_buf, path, &orig, base,\n+\t\t\t\t&src1, name1, &src2, name2,\n+\t\t\t\topt->repo->index, &ll_opts);\n+\n+\tfree(base);\n+\tfree(name1);\n+\tfree(name2);\n+\tfree(orig.ptr);\n+\tfree(src1.ptr);\n+\tfree(src2.ptr);\n+\treturn merge_status;\n }\n \n static int handle_content_merge(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"413457","messageId":"5f6d5428-36ce-3e91-4916-8968ac1b8686@gmail.com","threadId":"54844","inReplyTo":"pull.815.v2.git.1609468488.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/10] merge-ort: add more handling of basic conflict types","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-05T14:23:42Z","receivedAt":"2021-01-05T14:24:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/31/2020 9:34 PM, Elijah Newren via GitGitGadget wrote:\n> This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3\n> or en/merge-ort-recursive).\n> \n> This series adds handling of additional basic conflict types (directory/file\n> conflicts, three-way content merging, very basic submodule divergence\n> reconciliation, and different filetypes). This series drops the number of\n> test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).\n> \n> Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this\n> series are all merged down (in any order), then collectively they drop the\n> number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to\n> 60.\n\nI finished reading the rest of the patches. They are well-organized to\npresent the merging logic in small chunks.\n\nWhile this is a very dense series, the proof is in the test cases. I\nlook forward to testing the ort algorithm in CI builds and start enabling\nit in my repositories.\n\nYour patch organization will help if there are bugs that we won't catch\nuntil we can enable this merge algorithm more widely, as we can blame to\nthese well-crafted patches to assist with debugging.\n\nThanks,\n-Stolee\n"},{"id":"413595","messageId":"CABPp-BH0wN8UD91-dnTMdkE1qnbJ5eP=X526bAn=kFpQKKxYDw@mail.gmail.com","threadId":"54844","inReplyTo":"5f6d5428-36ce-3e91-4916-8968ac1b8686@gmail.com","subject":"Re: [PATCH v2 00/10] merge-ort: add more handling of basic conflict types","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-06T19:20:37Z","receivedAt":"2021-01-06T19:21:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 5, 2021 at 6:23 AM Derrick Stolee wrote:\n>\n> On 12/31/2020 9:34 PM, Elijah Newren via GitGitGadget wrote:\n> > This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3\n> > or en/merge-ort-recursive).\n> >\n> > This series adds handling of additional basic conflict types (directory/file\n> > conflicts, three-way content merging, very basic submodule divergence\n> > reconciliation, and different filetypes). This series drops the number of\n> > test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).\n> >\n> > Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this\n> > series are all merged down (in any order), then collectively they drop the\n> > number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to\n> > 60.\n>\n> I finished reading the rest of the patches. They are well-organized to\n> present the merging logic in small chunks.\n>\n> While this is a very dense series, the proof is in the test cases. I\n> look forward to testing the ort algorithm in CI builds and start enabling\n> it in my repositories.\n>\n> Your patch organization will help if there are bugs that we won't catch\n> until we can enable this merge algorithm more widely, as we can blame to\n> these well-crafted patches to assist with debugging.\n\nThanks for taking a look, as always.\n\nYour comments reminded me that I intended to dig into your code\ncoverage reports and try to figure out if there are parts of\nmerge-ort.[ch] (and diffcore-rename.c) that are uncovered by any\ntestcases. I bet there are some. There were certainly many in\nmerge-recursive, and while I've extended the testsuite coverage over\nthe years, I never did it with the aid of a code coverage tool...\n"},{"id":"418240","messageId":"877dmmkhnt.fsf@evledraar.gmail.com","threadId":"54844","inReplyTo":"69129a20edcfb57a3278285f31584ffcc2b3bad7.1609468488.git.gitgitgadget@gmail.com","subject":"A merge-ort TODO comment, and how to test merge-ort?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-04T16:28:06Z","receivedAt":"2021-03-04T16:29:40Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:\n\n> +\telse {\n> +\t\t/* must be the 100644/100755 case */\n> +\t\tassert(S_ISREG(a->mode));\n> +\t\tresult->mode = a->mode;\n> +\t\tclean = (b->mode == o->mode);\n> +\t\t/*\n> +\t\t * FIXME: If opt->priv->call_depth && !clean, then we really\n> +\t\t * should not make result->mode match either a->mode or\n> +\t\t * b->mode; that causes t6036 \"check conflicting mode for\n> +\t\t * regular file\" to fail. It would be best to use some other\n> +\t\t * mode, but we'll confuse all kinds of stuff if we use one\n> +\t\t * where S_ISREG(result->mode) isn't true, and if we use\n> +\t\t * something like 0100666, then tree-walk.c's calls to\n> +\t\t * canon_mode() will just normalize that to 100644 for us and\n> +\t\t * thus not solve anything.\n> +\t\t *\n> +\t\t * Figure out if there's some kind of way we can work around\n> +\t\t * this...\n> +\t\t */\n\nSo if tree-walk.c didn't call canon_mode() you would do:\n\n if (opt->priv->call_depth && !clean)\n result->mode = 0100666;\n else\n result->mode = a->mode;\n\nI haven't looked at this bit closer, but that doesn't make the test\nreferenced here pass.\n\nI'm refactoring tree-walk.h to do that in a WIP series, and ran into\nthis.\n\nAs an aside, how does one run the merge-ort tests in such a way as\nthey'll pass on master now? There's now a bunch of failures with\nGIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:\n \n t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)\n Failed test: 12\n Non-zero exit status: 1\n t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)\n Failed tests: 4-5, 10\n Non-zero exit status: 1\n t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)\n TODO passed: 13, 17\n t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n Failed tests: 7, 53, 55, 59\n Non-zero exit status: 1\n\nAnd both test_expect_merge_algorithm and what seems to be a common\npattern of e.g.:\n \n t6400-merge-df.sh: if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n t6400-merge-df.sh- then\n t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)\n t6400-merge-df.sh- else\n t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)\n t6400-merge-df.sh- fi &&\n\nWill not run tests on both backends, I was expecting to find something\nso we can the test N times for the backends, and declared if things were\nto be skipped on ort or whatever.\n\nI understand that this is still WIP code, but it would be nice to have\nit in a state where one can confidently touch merge-ort.c when changing\nsome API or whatever and have it be tested by default.\n\nOr maybe that's the case, and I've missed how it's happening...\n"},{"id":"418245","messageId":"CABPp-BFdN4k=LpAymggw96PPg8dFrzF2FVWLacH2hkrPakhhxg@mail.gmail.com","threadId":"54844","inReplyTo":"877dmmkhnt.fsf@evledraar.gmail.com","subject":"Re: A merge-ort TODO comment, and how to test merge-ort?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-04T19:43:07Z","receivedAt":"2021-03-04T19:45:06Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ævar,\n\nOn Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > + else {\n> > + /* must be the 100644/100755 case */\n> > + assert(S_ISREG(a->mode));\n> > + result->mode = a->mode;\n> > + clean = (b->mode == o->mode);\n> > + /*\n> > + * FIXME: If opt->priv->call_depth && !clean, then we really\n> > + * should not make result->mode match either a->mode or\n> > + * b->mode; that causes t6036 \"check conflicting mode for\n> > + * regular file\" to fail. It would be best to use some other\n> > + * mode, but we'll confuse all kinds of stuff if we use one\n> > + * where S_ISREG(result->mode) isn't true, and if we use\n> > + * something like 0100666, then tree-walk.c's calls to\n> > + * canon_mode() will just normalize that to 100644 for us and\n> > + * thus not solve anything.\n> > + *\n> > + * Figure out if there's some kind of way we can work around\n> > + * this...\n> > + */\n>\n> So if tree-walk.c didn't call canon_mode() you would do:\n>\n> if (opt->priv->call_depth && !clean)\n> result->mode = 0100666;\n> else\n> result->mode = a->mode;\n>\n> I haven't looked at this bit closer, but that doesn't make the test\n> referenced here pass.\n>\n> I'm refactoring tree-walk.h to do that in a WIP series, and ran into\n> this.\n\nInteresting. Yeah, there might be more steps to make that particular\ntest work, but I couldn't go any further due to canon_mode(). It's a\ntestcase that has always failed under merge-recursive, and which I was\nresigned to always have fail under merge-ort too; I suspect it's\nenough of a corner case that no one but me ever really cared before.\n(And I didn't hit it in the wild or know anyone that did, I just\nlearned of it by trying to clean up merge-recursive.)\n\n> As an aside, how does one run the merge-ort tests in such a way as\n> they'll pass on master now? There's now a bunch of failures with\n> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:\n>\n> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)\n> Failed test: 12\n> Non-zero exit status: 1\n> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)\n> Failed tests: 4-5, 10\n> Non-zero exit status: 1\n> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)\n> TODO passed: 13, 17\n> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n> Failed tests: 7, 53, 55, 59\n> Non-zero exit status: 1\n\nRight, I've been sending merge-ort upstream as fast as possible since\nlast September or so, but there's only so much reviewer bandwidth so\nI've been forced to hold back on dozens of patches.\n\nCurrently there are 8 test failures (all shown in your output here --\n1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two\nof which you show here). I was forced to switch my ordering of\nsending patches upstream late last year due to an intern project that\nwas planned to do significant work within diffcore-rename; I was\nworried about major conflicts, so I needed to get the diffcore-rename\nchanges upstream earlier. That's still in-process.\n\nBy the way, if you'd like to help accelerate the merge-ort work; it's\nalmost entirely review bound.\nhttps://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\nstill has no comments, then I have optimization series 10-14 to send\n(viewable up at\nhttps://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch),\nthen I have other fixes -- mostly for the testsuite (viewable at\nhttps://github.com/newren/git/tree/ort-remainder), then I need to fix\nup the TODO passed submodule tests. Due to how the submodule testing\nframework functions, I can't just make a simple\ns/test_expect_failure/test_expect_success/ -- the tests are structured\na bit funny and the tests are themselves buggy in some cases. I\ntalked with jrnieder about it a little bit, just need to spend more\ntime on it. But it hasn't been critical because the rest of the code\nwas so far away from finally landing anyway. Finally, and optionally,\ncomes the --remerge-diff and --remerge-diff-only options to log/show\n(viewable at https://github.com/newren/git/tree/remerge-diff, but\nthese patches need to both be cleaned up and rebased on\nort-remainder).\n\n> And both test_expect_merge_algorithm and what seems to be a common\n> pattern of e.g.:\n>\n> t6400-merge-df.sh: if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> t6400-merge-df.sh- then\n> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)\n> t6400-merge-df.sh- else\n> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)\n> t6400-merge-df.sh- fi &&\n>\n> Will not run tests on both backends, I was expecting to find something\n> so we can the test N times for the backends, and declared if things were\n> to be skipped on ort or whatever.\n\nYeah, multiple ways of testing were discussed mid last year. There\nwere lots of tradeoffs. I think the thing that pushed in this\ndirection is that we're not just aiming to add another optional merge\nbackend, we're aiming to replace merge-recursive entirely. Since\nmerge tests appear all throughout the code base, many as rebase or\ncherry-pick or revert or stash tests...or just as simple setup tests,\nwe want all of those tested with the new backend. Trying to duplicate\nall those tests in any way other than just re-running the testsuite\nwith a different knob would require huge changes to hundreds\n(thousands?) of testfiles and conflict with nearly every other topic.\nSo I made an environment variable that would choose which backend to\nuse, but with the downside of having to re-run the testsuite again.\n\n> I understand that this is still WIP code, but it would be nice to have\n> it in a state where one can confidently touch merge-ort.c when changing\n> some API or whatever and have it be tested by default.\n\nThanks for proactively checking. To make it easier for you, I'll see\nif I can submit a series later today that mostly completes the\nmerge-ort implementation; the t6423 testcases won't be fixed until\n\"Optimization batch 12\" lands, and I might not be able to fix the\n\"TODO passed\" submodule tests in this series, but the rest of the\nstuff can be fixed with about 10-12 patches. I had been worried about\noverloading the list with too many patches at once, but since it\nsounds like you're willing to review these particular patches... :-)\n\n> Or maybe that's the case, and I've missed how it's happening...\n"},{"id":"418262","messageId":"874khqk3ou.fsf@evledraar.gmail.com","threadId":"54844","inReplyTo":"CABPp-BFdN4k=LpAymggw96PPg8dFrzF2FVWLacH2hkrPakhhxg@mail.gmail.com","subject":"Re: A merge-ort TODO comment, and how to test merge-ort?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-04T21:29:53Z","receivedAt":"2021-03-04T21:31:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 04 2021, Elijah Newren wrote:\n\n> Hi Ævar,\n>\n> On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:\n>>\n>> > + else {\n>> > + /* must be the 100644/100755 case */\n>> > + assert(S_ISREG(a->mode));\n>> > + result->mode = a->mode;\n>> > + clean = (b->mode == o->mode);\n>> > + /*\n>> > + * FIXME: If opt->priv->call_depth && !clean, then we really\n>> > + * should not make result->mode match either a->mode or\n>> > + * b->mode; that causes t6036 \"check conflicting mode for\n>> > + * regular file\" to fail. It would be best to use some other\n>> > + * mode, but we'll confuse all kinds of stuff if we use one\n>> > + * where S_ISREG(result->mode) isn't true, and if we use\n>> > + * something like 0100666, then tree-walk.c's calls to\n>> > + * canon_mode() will just normalize that to 100644 for us and\n>> > + * thus not solve anything.\n>> > + *\n>> > + * Figure out if there's some kind of way we can work around\n>> > + * this...\n>> > + */\n>>\n>> So if tree-walk.c didn't call canon_mode() you would do:\n>>\n>> if (opt->priv->call_depth && !clean)\n>> result->mode = 0100666;\n>> else\n>> result->mode = a->mode;\n>>\n>> I haven't looked at this bit closer, but that doesn't make the test\n>> referenced here pass.\n>>\n>> I'm refactoring tree-walk.h to do that in a WIP series, and ran into\n>> this.\n>\n> Interesting. Yeah, there might be more steps to make that particular\n> test work, but I couldn't go any further due to canon_mode(). It's a\n> testcase that has always failed under merge-recursive, and which I was\n> resigned to always have fail under merge-ort too; I suspect it's\n> enough of a corner case that no one but me ever really cared before.\n> (And I didn't hit it in the wild or know anyone that did, I just\n> learned of it by trying to clean up merge-recursive.)\n\nI'll send those patches out sooner than later, but as a quick question,\nfor merges / writing new files to the index/trees etc. we basically:\n\n 1. sanitize the mode with canon_mode()\n 2. write it to a new object, either index or TREE object\n\nI've been trying to refactor things so those things have canon_mode() as\nclose to the time of writing as possible.\n\nWell, mostly to replace the whole S_*(mode) macros all over the place\nwith checks against \"enum object_type\", which is what most of it wants\nanyway.\n\nDo you think the merge logic generally wants to operate on the \"raw\"\nmode bits (including what may not even pass fsck checks), or the\nsanitized canon_mode()?\n\n>> As an aside, how does one run the merge-ort tests in such a way as\n>> they'll pass on master now? There's now a bunch of failures with\n>> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:\n>>\n>> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)\n>> Failed test: 12\n>> Non-zero exit status: 1\n>> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)\n>> Failed tests: 4-5, 10\n>> Non-zero exit status: 1\n>> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)\n>> TODO passed: 13, 17\n>> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n>> Failed tests: 7, 53, 55, 59\n>> Non-zero exit status: 1\n>\n> Right, I've been sending merge-ort upstream as fast as possible since\n> last September or so, but there's only so much reviewer bandwidth so\n> I've been forced to hold back on dozens of patches.\n>\n> Currently there are 8 test failures (all shown in your output here --\n> 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two\n> of which you show here). I was forced to switch my ordering of\n> sending patches upstream late last year due to an intern project that\n> was planned to do significant work within diffcore-rename; I was\n> worried about major conflicts, so I needed to get the diffcore-rename\n> changes upstream earlier. That's still in-process.\n>\n> By the way, if you'd like to help accelerate the merge-ort work; it's\n> almost entirely review bound.\n> https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\n> still has no comments, then I have optimization series 10-14 to send\n> (viewable up at\n> https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch),\n> then I have other fixes -- mostly for the testsuite (viewable at\n> https://github.com/newren/git/tree/ort-remainder), then I need to fix\n> up the TODO passed submodule tests. Due to how the submodule testing\n> framework functions, I can't just make a simple\n> s/test_expect_failure/test_expect_success/ -- the tests are structured\n> a bit funny and the tests are themselves buggy in some cases. I\n> talked with jrnieder about it a little bit, just need to spend more\n> time on it. But it hasn't been critical because the rest of the code\n> was so far away from finally landing anyway. Finally, and optionally,\n> comes the --remerge-diff and --remerge-diff-only options to log/show\n> (viewable at https://github.com/newren/git/tree/remerge-diff, but\n> these patches need to both be cleaned up and rebased on\n> ort-remainder).\n\nMaybe something like this with a bit of test prereq sprinkle on top? :)\n\n diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh\n new file mode 120000\n index 00000000000..6bf750c4036\n --- /dev/null\n +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh\n @@ -0,0 +1 @@\n +t6423-merge-rename-directories.sh\n \\ No newline at end of file\n diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n index 5d3b711fe68..1e571384223 100755\n --- a/t/t6423-merge-rename-directories.sh\n +++ b/t/t6423-merge-rename-directories.sh\n @@ -3,2 +3,4 @@\n test_description=\"recursive merge with directory renames\"\n +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort\n +\n # includes checking of many corner cases, with a similar methodology to:\n diff --git a/t/test-lib.sh b/t/test-lib.sh\n index d3f6af6a654..8d5da7e0ba9 100644\n --- a/t/test-lib.sh\n +++ b/t/test-lib.sh\n @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n TEST_NAME=\"$(basename \"$0\" .sh)\"\n +if test -L \"$0\"\n +then\n + target=$(echo \"$0\" | grep -o \"TARGET-[^.]*\")\n + if test -n \"$target\"\n + then\n + to_eval=$(grep \"^# $target: \" \"$0\" | sed 's/.*://')\n + eval $to_eval\n + fi\n +fi\n TEST_NUMBER=\"${TEST_NAME%%-*}\"\n\nThat implementation's a bit of a hack, and requires SYMLINK (could be\nchanged), but now I can:\n\n ./t6423-merge-rename-directories-TARGET-ort.sh\n ./t6423-merge-rename-directories.sh\n\nAnd run the whole thing with/without ort in one go.\n\nThen with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM)\nloop on top I've got ort and non-ort merge tests all in one go on a WIP\ntopic.\n\n>> And both test_expect_merge_algorithm and what seems to be a common\n>> pattern of e.g.:\n>>\n>> t6400-merge-df.sh: if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n>> t6400-merge-df.sh- then\n>> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)\n>> t6400-merge-df.sh- else\n>> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)\n>> t6400-merge-df.sh- fi &&\n>>\n>> Will not run tests on both backends, I was expecting to find something\n>> so we can the test N times for the backends, and declared if things were\n>> to be skipped on ort or whatever.\n>\n> Yeah, multiple ways of testing were discussed mid last year. There\n> were lots of tradeoffs. I think the thing that pushed in this\n> direction is that we're not just aiming to add another optional merge\n> backend, we're aiming to replace merge-recursive entirely. Since\n> merge tests appear all throughout the code base, many as rebase or\n> cherry-pick or revert or stash tests...or just as simple setup tests,\n> we want all of those tested with the new backend. Trying to duplicate\n> all those tests in any way other than just re-running the testsuite\n> with a different knob would require huge changes to hundreds\n> (thousands?) of testfiles and conflict with nearly every other topic.\n> So I made an environment variable that would choose which backend to\n> use, but with the downside of having to re-run the testsuite again.\n>\n>> I understand that this is still WIP code, but it would be nice to have\n>> it in a state where one can confidently touch merge-ort.c when changing\n>> some API or whatever and have it be tested by default.\n>\n> Thanks for proactively checking. To make it easier for you, I'll see\n> if I can submit a series later today that mostly completes the\n> merge-ort implementation; the t6423 testcases won't be fixed until\n> \"Optimization batch 12\" lands, and I might not be able to fix the\n> \"TODO passed\" submodule tests in this series, but the rest of the\n> stuff can be fixed with about 10-12 patches. I had been worried about\n> overloading the list with too many patches at once, but since it\n> sounds like you're willing to review these particular patches... :-)\n\n*nod*\n\nAlso, I'll try to get to reviewing some of it, thanks for all your work.\n\nB.t.w., if the original E-Mail sounded like complaining that wasn't the\nintent. I just thought I'd shoot off a quick message about if I missed\nsomething about the in-flight status / testing of the ort work...\n"},{"id":"418266","messageId":"CABPp-BEdu1PqV5W=FuL0f08iFhGzvzV8oSUybNj4eF0aAwTnAw@mail.gmail.com","threadId":"54844","inReplyTo":"874khqk3ou.fsf@evledraar.gmail.com","subject":"Re: A merge-ort TODO comment, and how to test merge-ort?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-04T22:45:10Z","receivedAt":"2021-03-04T22:45:24Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 4, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Thu, Mar 04 2021, Elijah Newren wrote:\n>\n> > Hi Ævar,\n> >\n> > On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason wrote:\n> >>\n> >> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:\n> >>\n> >> > + else {\n> >> > + /* must be the 100644/100755 case */\n> >> > + assert(S_ISREG(a->mode));\n> >> > + result->mode = a->mode;\n> >> > + clean = (b->mode == o->mode);\n> >> > + /*\n> >> > + * FIXME: If opt->priv->call_depth && !clean, then we really\n> >> > + * should not make result->mode match either a->mode or\n> >> > + * b->mode; that causes t6036 \"check conflicting mode for\n> >> > + * regular file\" to fail. It would be best to use some other\n> >> > + * mode, but we'll confuse all kinds of stuff if we use one\n> >> > + * where S_ISREG(result->mode) isn't true, and if we use\n> >> > + * something like 0100666, then tree-walk.c's calls to\n> >> > + * canon_mode() will just normalize that to 100644 for us and\n> >> > + * thus not solve anything.\n> >> > + *\n> >> > + * Figure out if there's some kind of way we can work around\n> >> > + * this...\n> >> > + */\n> >>\n> >> So if tree-walk.c didn't call canon_mode() you would do:\n> >>\n> >> if (opt->priv->call_depth && !clean)\n> >> result->mode = 0100666;\n> >> else\n> >> result->mode = a->mode;\n> >>\n> >> I haven't looked at this bit closer, but that doesn't make the test\n> >> referenced here pass.\n> >>\n> >> I'm refactoring tree-walk.h to do that in a WIP series, and ran into\n> >> this.\n> >\n> > Interesting. Yeah, there might be more steps to make that particular\n> > test work, but I couldn't go any further due to canon_mode(). It's a\n> > testcase that has always failed under merge-recursive, and which I was\n> > resigned to always have fail under merge-ort too; I suspect it's\n> > enough of a corner case that no one but me ever really cared before.\n> > (And I didn't hit it in the wild or know anyone that did, I just\n> > learned of it by trying to clean up merge-recursive.)\n>\n> I'll send those patches out sooner than later, but as a quick question,\n> for merges / writing new files to the index/trees etc. we basically:\n>\n> 1. sanitize the mode with canon_mode()\n> 2. write it to a new object, either index or TREE object\n>\n> I've been trying to refactor things so those things have canon_mode() as\n> close to the time of writing as possible.\n>\n> Well, mostly to replace the whole S_*(mode) macros all over the place\n> with checks against \"enum object_type\", which is what most of it wants\n> anyway.\n>\n> Do you think the merge logic generally wants to operate on the \"raw\"\n> mode bits (including what may not even pass fsck checks), or the\n> sanitized canon_mode()?\n\nThis one little special case is the only one when it'd care about the\nraw mode bits. I'm worried that making the code work on raw mode bits\nwouldn't be trivial. In general mode differences between different\nsides or the mode base is a conflict as well, so I'd need to add code\naround it's-not-really-a-conflict if canon_mode() on both modes map to\nthe same value.\n\n> >> As an aside, how does one run the merge-ort tests in such a way as\n> >> they'll pass on master now? There's now a bunch of failures with\n> >> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:\n> >>\n> >> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)\n> >> Failed test: 12\n> >> Non-zero exit status: 1\n> >> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)\n> >> Failed tests: 4-5, 10\n> >> Non-zero exit status: 1\n> >> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)\n> >> TODO passed: 13, 17\n> >> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n> >> Failed tests: 7, 53, 55, 59\n> >> Non-zero exit status: 1\n> >\n> > Right, I've been sending merge-ort upstream as fast as possible since\n> > last September or so, but there's only so much reviewer bandwidth so\n> > I've been forced to hold back on dozens of patches.\n> >\n> > Currently there are 8 test failures (all shown in your output here --\n> > 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two\n> > of which you show here). I was forced to switch my ordering of\n> > sending patches upstream late last year due to an intern project that\n> > was planned to do significant work within diffcore-rename; I was\n> > worried about major conflicts, so I needed to get the diffcore-rename\n> > changes upstream earlier. That's still in-process.\n> >\n> > By the way, if you'd like to help accelerate the merge-ort work; it's\n> > almost entirely review bound.\n> > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\n> > still has no comments, then I have optimization series 10-14 to send\n> > (viewable up at\n> > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch),\n> > then I have other fixes -- mostly for the testsuite (viewable at\n> > https://github.com/newren/git/tree/ort-remainder), then I need to fix\n> > up the TODO passed submodule tests. Due to how the submodule testing\n> > framework functions, I can't just make a simple\n> > s/test_expect_failure/test_expect_success/ -- the tests are structured\n> > a bit funny and the tests are themselves buggy in some cases. I\n> > talked with jrnieder about it a little bit, just need to spend more\n> > time on it. But it hasn't been critical because the rest of the code\n> > was so far away from finally landing anyway. Finally, and optionally,\n> > comes the --remerge-diff and --remerge-diff-only options to log/show\n> > (viewable at https://github.com/newren/git/tree/remerge-diff, but\n> > these patches need to both be cleaned up and rebased on\n> > ort-remainder).\n>\n> Maybe something like this with a bit of test prereq sprinkle on top? :)\n>\n> diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh\n> new file mode 120000\n> index 00000000000..6bf750c4036\n> --- /dev/null\n> +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh\n> @@ -0,0 +1 @@\n> +t6423-merge-rename-directories.sh\n> \\ No newline at end of file\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 5d3b711fe68..1e571384223 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -3,2 +3,4 @@\n> test_description=\"recursive merge with directory renames\"\n> +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort\n\nTo test my understanding of your proposal, would you need to add this\nline to hundreds of files?\n\n> +\n> # includes checking of many corner cases, with a similar methodology to:\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index d3f6af6a654..8d5da7e0ba9 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n> TEST_NAME=\"$(basename \"$0\" .sh)\"\n> +if test -L \"$0\"\n> +then\n> + target=$(echo \"$0\" | grep -o \"TARGET-[^.]*\")\n> + if test -n \"$target\"\n> + then\n> + to_eval=$(grep \"^# $target: \" \"$0\" | sed 's/.*://')\n> + eval $to_eval\n> + fi\n> +fi\n> TEST_NUMBER=\"${TEST_NAME%%-*}\"\n>\n> That implementation's a bit of a hack, and requires SYMLINK (could be\n> changed), but now I can:\n>\n> ./t6423-merge-rename-directories-TARGET-ort.sh\n> ./t6423-merge-rename-directories.sh\n>\n> And run the whole thing with/without ort in one go.\n>\n> Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM)\n> loop on top I've got ort and non-ort merge tests all in one go on a WIP\n> topic.\n\nDo you need a symlink for each file as well, thus hundreds of symlinks?\n\nYour idea is a quick way to get testing, that's much better than\nduplicating all the files. I'm still a bit worried that it'd\nencourage people to only add it to the \"most important\" or \"most\nobvious\" test files, which goes somewhat counter to testing merge-ort\nas a full replacement of merge-recursive. While developing merge-ort,\nI'd sometimes see failures outside t6*, even when everything under t6*\npassed. For example, t3[45]* and t76*.\n\n> >> And both test_expect_merge_algorithm and what seems to be a common\n> >> pattern of e.g.:\n> >>\n> >> t6400-merge-df.sh: if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> >> t6400-merge-df.sh- then\n> >> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)\n> >> t6400-merge-df.sh- else\n> >> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)\n> >> t6400-merge-df.sh- fi &&\n> >>\n> >> Will not run tests on both backends, I was expecting to find something\n> >> so we can the test N times for the backends, and declared if things were\n> >> to be skipped on ort or whatever.\n> >\n> > Yeah, multiple ways of testing were discussed mid last year. There\n> > were lots of tradeoffs. I think the thing that pushed in this\n> > direction is that we're not just aiming to add another optional merge\n> > backend, we're aiming to replace merge-recursive entirely. Since\n> > merge tests appear all throughout the code base, many as rebase or\n> > cherry-pick or revert or stash tests...or just as simple setup tests,\n> > we want all of those tested with the new backend. Trying to duplicate\n> > all those tests in any way other than just re-running the testsuite\n> > with a different knob would require huge changes to hundreds\n> > (thousands?) of testfiles and conflict with nearly every other topic.\n> > So I made an environment variable that would choose which backend to\n> > use, but with the downside of having to re-run the testsuite again.\n> >\n> >> I understand that this is still WIP code, but it would be nice to have\n> >> it in a state where one can confidently touch merge-ort.c when changing\n> >> some API or whatever and have it be tested by default.\n> >\n> > Thanks for proactively checking. To make it easier for you, I'll see\n> > if I can submit a series later today that mostly completes the\n> > merge-ort implementation; the t6423 testcases won't be fixed until\n> > \"Optimization batch 12\" lands, and I might not be able to fix the\n> > \"TODO passed\" submodule tests in this series, but the rest of the\n> > stuff can be fixed with about 10-12 patches. I had been worried about\n> > overloading the list with too many patches at once, but since it\n> > sounds like you're willing to review these particular patches... :-)\n>\n> *nod*\n>\n> Also, I'll try to get to reviewing some of it, thanks for all your work.\n>\n> B.t.w., if the original E-Mail sounded like complaining that wasn't the\n> intent. I just thought I'd shoot off a quick message about if I missed\n> something about the in-flight status / testing of the ort work...\n\nNope, didn't sound like complaining; thanks again for checking.\n"},{"id":"418447","messageId":"87mtvditul.fsf@evledraar.gmail.com","threadId":"54844","inReplyTo":"CABPp-BEdu1PqV5W=FuL0f08iFhGzvzV8oSUybNj4eF0aAwTnAw@mail.gmail.com","subject":"Re: A merge-ort TODO comment, and how to test merge-ort?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T14:49:06Z","receivedAt":"2021-03-08T14:50:03Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 04 2021, Elijah Newren wrote:\n\n> On Thu, Mar 4, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason wrote:\n>>\n>>\n>> On Thu, Mar 04 2021, Elijah Newren wrote:\n>>\n>> > Hi Ævar,\n>> >\n>> > On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason wrote:\n>> >>\n>> >> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:\n>> >>\n>> >> > + else {\n>> >> > + /* must be the 100644/100755 case */\n>> >> > + assert(S_ISREG(a->mode));\n>> >> > + result->mode = a->mode;\n>> >> > + clean = (b->mode == o->mode);\n>> >> > + /*\n>> >> > + * FIXME: If opt->priv->call_depth && !clean, then we really\n>> >> > + * should not make result->mode match either a->mode or\n>> >> > + * b->mode; that causes t6036 \"check conflicting mode for\n>> >> > + * regular file\" to fail. It would be best to use some other\n>> >> > + * mode, but we'll confuse all kinds of stuff if we use one\n>> >> > + * where S_ISREG(result->mode) isn't true, and if we use\n>> >> > + * something like 0100666, then tree-walk.c's calls to\n>> >> > + * canon_mode() will just normalize that to 100644 for us and\n>> >> > + * thus not solve anything.\n>> >> > + *\n>> >> > + * Figure out if there's some kind of way we can work around\n>> >> > + * this...\n>> >> > + */\n>> >>\n>> >> So if tree-walk.c didn't call canon_mode() you would do:\n>> >>\n>> >> if (opt->priv->call_depth && !clean)\n>> >> result->mode = 0100666;\n>> >> else\n>> >> result->mode = a->mode;\n>> >>\n>> >> I haven't looked at this bit closer, but that doesn't make the test\n>> >> referenced here pass.\n>> >>\n>> >> I'm refactoring tree-walk.h to do that in a WIP series, and ran into\n>> >> this.\n>> >\n>> > Interesting. Yeah, there might be more steps to make that particular\n>> > test work, but I couldn't go any further due to canon_mode(). It's a\n>> > testcase that has always failed under merge-recursive, and which I was\n>> > resigned to always have fail under merge-ort too; I suspect it's\n>> > enough of a corner case that no one but me ever really cared before.\n>> > (And I didn't hit it in the wild or know anyone that did, I just\n>> > learned of it by trying to clean up merge-recursive.)\n>>\n>> I'll send those patches out sooner than later, but as a quick question,\n>> for merges / writing new files to the index/trees etc. we basically:\n>>\n>> 1. sanitize the mode with canon_mode()\n>> 2. write it to a new object, either index or TREE object\n>>\n>> I've been trying to refactor things so those things have canon_mode() as\n>> close to the time of writing as possible.\n>>\n>> Well, mostly to replace the whole S_*(mode) macros all over the place\n>> with checks against \"enum object_type\", which is what most of it wants\n>> anyway.\n>>\n>> Do you think the merge logic generally wants to operate on the \"raw\"\n>> mode bits (including what may not even pass fsck checks), or the\n>> sanitized canon_mode()?\n>\n> This one little special case is the only one when it'd care about the\n> raw mode bits. I'm worried that making the code work on raw mode bits\n> wouldn't be trivial. In general mode differences between different\n> sides or the mode base is a conflict as well, so I'd need to add code\n> around it's-not-really-a-conflict if canon_mode() on both modes map to\n> the same value.\n\nIt wasn't trivial, but let's see what you think of the end result, soon\non-list.\n\n>> >> As an aside, how does one run the merge-ort tests in such a way as\n>> >> they'll pass on master now? There's now a bunch of failures with\n>> >> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:\n>> >>\n>> >> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)\n>> >> Failed test: 12\n>> >> Non-zero exit status: 1\n>> >> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)\n>> >> Failed tests: 4-5, 10\n>> >> Non-zero exit status: 1\n>> >> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)\n>> >> TODO passed: 13, 17\n>> >> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n>> >> Failed tests: 7, 53, 55, 59\n>> >> Non-zero exit status: 1\n>> >\n>> > Right, I've been sending merge-ort upstream as fast as possible since\n>> > last September or so, but there's only so much reviewer bandwidth so\n>> > I've been forced to hold back on dozens of patches.\n>> >\n>> > Currently there are 8 test failures (all shown in your output here --\n>> > 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two\n>> > of which you show here). I was forced to switch my ordering of\n>> > sending patches upstream late last year due to an intern project that\n>> > was planned to do significant work within diffcore-rename; I was\n>> > worried about major conflicts, so I needed to get the diffcore-rename\n>> > changes upstream earlier. That's still in-process.\n>> >\n>> > By the way, if you'd like to help accelerate the merge-ort work; it's\n>> > almost entirely review bound.\n>> > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\n>> > still has no comments, then I have optimization series 10-14 to send\n>> > (viewable up at\n>> > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch),\n>> > then I have other fixes -- mostly for the testsuite (viewable at\n>> > https://github.com/newren/git/tree/ort-remainder), then I need to fix\n>> > up the TODO passed submodule tests. Due to how the submodule testing\n>> > framework functions, I can't just make a simple\n>> > s/test_expect_failure/test_expect_success/ -- the tests are structured\n>> > a bit funny and the tests are themselves buggy in some cases. I\n>> > talked with jrnieder about it a little bit, just need to spend more\n>> > time on it. But it hasn't been critical because the rest of the code\n>> > was so far away from finally landing anyway. Finally, and optionally,\n>> > comes the --remerge-diff and --remerge-diff-only options to log/show\n>> > (viewable at https://github.com/newren/git/tree/remerge-diff, but\n>> > these patches need to both be cleaned up and rebased on\n>> > ort-remainder).\n>>\n>> Maybe something like this with a bit of test prereq sprinkle on top? :)\n>>\n>> diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh\n>> new file mode 120000\n>> index 00000000000..6bf750c4036\n>> --- /dev/null\n>> +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh\n>> @@ -0,0 +1 @@\n>> +t6423-merge-rename-directories.sh\n>> \\ No newline at end of file\n>> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n>> index 5d3b711fe68..1e571384223 100755\n>> --- a/t/t6423-merge-rename-directories.sh\n>> +++ b/t/t6423-merge-rename-directories.sh\n>> @@ -3,2 +3,4 @@\n>> test_description=\"recursive merge with directory renames\"\n>> +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort\n>\n> To test my understanding of your proposal, would you need to add this\n> line to hundreds of files?\n>\n>> +\n>> # includes checking of many corner cases, with a similar methodology to:\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index d3f6af6a654..8d5da7e0ba9 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n>> TEST_NAME=\"$(basename \"$0\" .sh)\"\n>> +if test -L \"$0\"\n>> +then\n>> + target=$(echo \"$0\" | grep -o \"TARGET-[^.]*\")\n>> + if test -n \"$target\"\n>> + then\n>> + to_eval=$(grep \"^# $target: \" \"$0\" | sed 's/.*://')\n>> + eval $to_eval\n>> + fi\n>> +fi\n>> TEST_NUMBER=\"${TEST_NAME%%-*}\"\n>>\n>> That implementation's a bit of a hack, and requires SYMLINK (could be\n>> changed), but now I can:\n>>\n>> ./t6423-merge-rename-directories-TARGET-ort.sh\n>> ./t6423-merge-rename-directories.sh\n>>\n>> And run the whole thing with/without ort in one go.\n>>\n>> Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM)\n>> loop on top I've got ort and non-ort merge tests all in one go on a WIP\n>> topic.\n>\n> Do you need a symlink for each file as well, thus hundreds of symlinks?\n>\n> Your idea is a quick way to get testing, that's much better than\n> duplicating all the files. I'm still a bit worried that it'd\n> encourage people to only add it to the \"most important\" or \"most\n> obvious\" test files, which goes somewhat counter to testing merge-ort\n> as a full replacement of merge-recursive. While developing merge-ort,\n> I'd sometimes see failures outside t6*, even when everything under t6*\n> passed. For example, t3[45]* and t76*.\n\nYes, this wouldn't make sense for merge-ort then. I was assuming that it\nwas fairly isolated (at least mostly) to a few test files.\n\nThat's mostly me not having read the ort traffic carefully, I'm\nembarrased to say that I managed to miss that it was a full \"recursive\"\nreplacement, I thought it was (mostly) a new merge driver mode and we'd\nkeep both.\n\nSo nevermind :)\n\nI do think it's interesting to have something like this, but it's way\nout of scope for merge-ort work.\n\nE.g. we could start by making sure for all N tests in a file, we can run\nrun each N times in a loop, i.e. individual --stress tests.\n\nThat in itself would be a big undertaking, and would require e.g. having\na \"test_expect_success_setup\" for tests that do one-off setup, which\nwe'd skip.\n\nThen we could instrument the git_env_bool(\"GIT_TEST_*\" with some\nreplacement which logged if we ended up deciding something on whether\nthat was true/false.\n\nAnd finally, log that with trace2, then for each test that encountered\ndiffering modes we'd run them for the N modes, or all combinations of\nmodes (would quickly get expensive for things that touch a lot of\nthings).\n\nAnyway, just take the above as rambling :)\n\n>> >> And both test_expect_merge_algorithm and what seems to be a common\n>> >> pattern of e.g.:\n>> >>\n>> >> t6400-merge-df.sh: if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n>> >> t6400-merge-df.sh- then\n>> >> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)\n>> >> t6400-merge-df.sh- else\n>> >> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)\n>> >> t6400-merge-df.sh- fi &&\n>> >>\n>> >> Will not run tests on both backends, I was expecting to find something\n>> >> so we can the test N times for the backends, and declared if things were\n>> >> to be skipped on ort or whatever.\n>> >\n>> > Yeah, multiple ways of testing were discussed mid last year. There\n>> > were lots of tradeoffs. I think the thing that pushed in this\n>> > direction is that we're not just aiming to add another optional merge\n>> > backend, we're aiming to replace merge-recursive entirely. Since\n>> > merge tests appear all throughout the code base, many as rebase or\n>> > cherry-pick or revert or stash tests...or just as simple setup tests,\n>> > we want all of those tested with the new backend. Trying to duplicate\n>> > all those tests in any way other than just re-running the testsuite\n>> > with a different knob would require huge changes to hundreds\n>> > (thousands?) of testfiles and conflict with nearly every other topic.\n>> > So I made an environment variable that would choose which backend to\n>> > use, but with the downside of having to re-run the testsuite again.\n>> >\n>> >> I understand that this is still WIP code, but it would be nice to have\n>> >> it in a state where one can confidently touch merge-ort.c when changing\n>> >> some API or whatever and have it be tested by default.\n>> >\n>> > Thanks for proactively checking. To make it easier for you, I'll see\n>> > if I can submit a series later today that mostly completes the\n>> > merge-ort implementation; the t6423 testcases won't be fixed until\n>> > \"Optimization batch 12\" lands, and I might not be able to fix the\n>> > \"TODO passed\" submodule tests in this series, but the rest of the\n>> > stuff can be fixed with about 10-12 patches. I had been worried about\n>> > overloading the list with too many patches at once, but since it\n>> > sounds like you're willing to review these particular patches... :-)\n>>\n>> *nod*\n>>\n>> Also, I'll try to get to reviewing some of it, thanks for all your work.\n>>\n>> B.t.w., if the original E-Mail sounded like complaining that wasn't the\n>> intent. I just thought I'd shoot off a quick message about if I missed\n>> something about the in-flight status / testing of the ort work...\n>\n> Nope, didn't sound like complaining; thanks again for checking.\n\nAnd thanks a lot for your good work on merge-ort & other things :)\n"}]}