{"thread":{"id":"55259","subject":"[PATCH 00/11] Complete merge-ort implementation...almost","startedAt":"2021-03-05T00:55:41Z","lastAt":"2021-03-17T21:43:04Z","messageCount":44,"participants":["Elijah Newren via GitGitGadget","Ævar Arnfjörð Bjarmason","Elijah Newren","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"418281","messageId":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":null,"subject":"[PATCH 00/11] Complete merge-ort implementation...almost","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:26Z","receivedAt":"2021-03-05T00:55:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In order to help Ævar test his tree-walk changes against merge-ort[1], this\nseries completes the merge-ort implementation and cleans up testsuite\nfailures...EXCEPT for some t6423 failures. It also leaves out a lot of\nperformance work, which incidentally will fix the t6423 failures and is\nbeing reviewed independently[2].\n\nThis 11-patch series could be submitted as 7 independent series, 1-4 patches\nin length each, but it's probably easier for Ævar if we can merge just one\nmore thing and it's only 11 total patches. This series sub-divides as\nfollows:\n\n * Patch 1: Fix bug in already-merged portion of merge-ort affecting\n rename/rename conflicts on platforms where qsort isn't stable. (Could be\n considered for merging before 2.31 since it is a new bug in the 2.31\n cycle that I just learned of last night, but not sure it matters since\n merge-ort wasn't complete anyway and we're not even mentioning merge-ort\n in the release notes.)\n * Patches 2-5: Add support for renormalization\n * Patch 6: Add support for subtree shifting\n * Patch 7-8: Add test and support for conflicts affecting sparse-checkout\n entries\n * Patch 9: Update submodule related merge tests to note the ones that\n merge-ort fixes relative to merge-recursive\n * Patch 10: New feature -- allow \"git diff AUTO_MERGE\" during conflict\n resolution to let user review changes they made since\n merge/rebase/cherry-pick/revert stopped and informed them of conflicts\n * Patch 11: Add comments noting various bugs in merge-recursive\n\nThe last two patches aren't needed by Ævar, so they could be left out and\nsubmitted later. I just figured that it was only two more patches and they\nwere part of \"completing the merge-ort implementation\" in my view.\n\n[1] https://lore.kernel.org/git/877dmmkhnt.fsf@evledraar.gmail.com/ [2] See\nhttps://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/;\nthere are five more waiting after that -- viewable by the curious at\nhttps://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch\n\nElijah Newren (11):\n merge-ort: use STABLE_QSORT instead of QSORT where required\n merge-ort: add a special minimal index just for renormalization\n merge-ort: add a function for initializing our special attr_index\n merge-ort: have ll_merge() calls use the attr_index for\n renormalization\n merge-ort: let renormalization change modify/delete into clean delete\n merge-ort: support subtree shifting\n t6428: new test for SKIP_WORKTREE handling and conflicts\n merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries\n t: mark several submodule merging tests as fixed under merge-ort\n merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict\n merge-recursive: add a bunch of FIXME comments documenting known bugs\n\n branch.c | 1 +\n builtin/rebase.c | 1 +\n merge-ort.c | 230 ++++++++++++++++--\n merge-recursive.c | 37 +++\n path.c | 1 +\n path.h | 2 +\n sequencer.c | 5 +\n t/t3512-cherry-pick-submodule.sh | 9 +-\n t/t3513-revert-submodule.sh | 7 +-\n t/t5572-pull-submodule.sh | 9 +-\n t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++\n t/t6437-submodule-merge.sh | 5 +-\n t/t6438-submodule-directory-file-conflicts.sh | 9 +-\n 13 files changed, 449 insertions(+), 25 deletions(-)\n create mode 100755 t/t6428-merge-conflicts-sparse.sh\n\n\nbase-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-973%2Fnewren%2Fort-remainder-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-973/newren/ort-remainder-v1\nPull-Request: https://github.com/git/git/pull/973\n-- \ngitgitgadget\n"},{"id":"418282","messageId":"3ca16a5e3466914c1c7efa79843395a4dfc3c3f1.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 01/11] merge-ort: use STABLE_QSORT instead of QSORT where required","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:27Z","receivedAt":"2021-03-05T00:55:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nrename/rename conflict handling depends on the fact that if both sides\nrenamed the same path, that the one on the MERGE_SIDE1 will appear first\nin the combined diff_queue_struct passed to process_renames(). Since we\nadd all pairs from MERGE_SIDE1 to combined first, and then all pairs\nfrom MERGE_SIDE2, and then sort based on filename, this will only be\ntrue if the sort used is stable. This was found due to the fact that\nMac, unlike Linux, apparently has a system-defined qsort that is not\nstable.\n\nWhile we are at it, review the other callers of QSORT and add comments\nabout why they can remain as calls to QSORT instead of being modified\nto call STABLE_QSORT.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 603d30c52170..5309488fd9d8 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2289,7 +2289,7 @@ static int detect_and_process_renames(struct merge_options *opt,\n \tclean &= collect_renames(opt, &combined, MERGE_SIDE2,\n \t\t\t\t &renames->dir_renames[1],\n \t\t\t\t &renames->dir_renames[2]);\n-\tQSORT(combined.queue, combined.nr, compare_pairs);\n+\tSTABLE_QSORT(combined.queue, combined.nr, compare_pairs);\n \ttrace2_region_leave(\"merge\", \"directory renames\", opt->repo);\n \n \ttrace2_region_enter(\"merge\", \"process renames\", opt->repo);\n@@ -2415,6 +2415,7 @@ static void write_tree(struct object_id *result_oid,\n \t */\n \trelevant_entries.items = versions->items + offset;\n \trelevant_entries.nr = versions->nr - offset;\n+\t/* No need for STABLE_QSORT -- filenames must be unique */\n \tQSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);\n \n \t/* Pre-allocate some space in buf */\n@@ -3190,6 +3191,11 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n \t * entries we added to the end into their right locations.\n \t */\n \tremove_marked_cache_entries(index, 1);\n+\t/*\n+\t * No need for STABLE_QSORT -- cmp_cache_name_compare sorts primarily\n+\t * on filename and secondarily on stage, and (name, stage #) are a\n+\t * unique tuple.\n+\t */\n \tQSORT(index->cache, index->cache_nr, cmp_cache_name_compare);\n \n \treturn errs;\n-- \ngitgitgadget\n\n"},{"id":"418283","messageId":"24454e67b186992b9ef7a8ac6f220cbb4aaafd6b.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 02/11] merge-ort: add a special minimal index just for renormalization","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:28Z","receivedAt":"2021-03-05T00:55:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nrenormalize_buffer() requires an index_state, which is something that\nmerge-ort does not operate with. However, all the renormalization code\nneeds is an index with a .gitattributes file...plus a little bit of\nsetup. Create such an index, along with the deallocation and\nattr_direction handling.\n\nA subsequent commit will add a function to finish the initialization\nof this index.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 5309488fd9d8..d91b66a052b6 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -18,6 +18,7 @@\n #include \"merge-ort.h\"\n \n #include \"alloc.h\"\n+#include \"attr.h\"\n #include \"blob.h\"\n #include \"cache-tree.h\"\n #include \"commit.h\"\n@@ -170,6 +171,16 @@ struct merge_options_internal {\n \t */\n \tstruct rename_info renames;\n \n+\t/*\n+\t * attr_index: hacky minimal index used for renormalization\n+\t *\n+\t * renormalization code _requires_ an index, though it only needs to\n+\t * find a .gitattributes file within the index. So, when\n+\t * renormalization is important, we create a special index with just\n+\t * that one file.\n+\t */\n+\tstruct index_state attr_index;\n+\n \t/*\n \t * current_dir_name, toplevel_dir: temporary vars\n \t *\n@@ -349,6 +360,9 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \tstring_list_clear(&opti->paths_to_free, 0);\n \topti->paths_to_free.strdup_strings = 0;\n \n+\tif (opti->attr_index.cache_nr)\n+\t\tdiscard_index(&opti->attr_index);\n+\n \t/* Free memory used by various renames maps */\n \tfor (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {\n \t\tstruct hashmap_iter iter;\n@@ -3272,6 +3286,8 @@ void merge_finalize(struct merge_options *opt,\n {\n \tstruct merge_options_internal *opti = result->priv;\n \n+\tif (opt->renormalize)\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \tassert(opt->priv == NULL);\n \n \tclear_or_reinit_internal_opts(opti, 0);\n@@ -3347,6 +3363,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t/* Default to histogram diff. Actually, just hardcode it...for now. */\n \topt->xdl_opts = DIFF_WITH_ALG(opt, HISTOGRAM_DIFF);\n \n+\t/* Handle attr direction stuff for renormalization */\n+\tif (opt->renormalize)\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n+\n \t/* Initialization of opt->priv, our internal merge data */\n \ttrace2_region_enter(\"merge\", \"allocate/init\", opt->repo);\n \tif (opt->priv) {\n-- \ngitgitgadget\n\n"},{"id":"418284","messageId":"815af5d30ebd5e7f80aa42e4a54808af2e3781e0.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 03/11] merge-ort: add a function for initializing our special attr_index","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:29Z","receivedAt":"2021-03-05T00:55:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAdd a function which can be called to populate the attr_index with the\nappropriate .gitattributes contents when necessary. Make it return\nearly if the attr_index is already initialized or if we are not\nrenormalizing files.\n\nNOTE 1: Even if the user has a working copy or a real index (which is\nnot a given as merge-ort can be used in bare repositories), we\nexplicitly ignore any .gitattributes file from either of these\nlocations. merge-ort can be used to merge two branches that are\nunrelated to HEAD, so .gitattributes from the working copy and current\nindex should not be considered relevant.\n\nNOTE 2: Since we are in the middle of merging, there is a risk that\n.gitattributes itself is conflicted...leaving us with an ill-defined\nsituation about how to perform the rest of the merge. It could be that\nthe .gitattributes file does not even exist on one of the sides of the\nmerge, or that it has been modified on both sides. If it's been\nmodified on both sides, it's possible that it could itself be merged\ncleanly, though it's also possible that it only merges cleanly if you\nuse the right version of the .gitattributes file to drive the merge. It\ngets kind of complicated. The only test we ever had that attempted to\ntest behavior in this area was seemingly unaware of the undefined\nbehavior, but knew the test wouldn't work for lack of attribute handling\nsupport, marked it as test_expect_failure from the beginning, but\nmanaged to fail for several reasons unrelated to attribute handling.\nSee commit 6f6e7cfb52 (\"t6038: remove problematic test\", 2020-08-03) for\ndetails. So there are probably various ways to improve what\ninitialize_attr_index() picks in the case of a conflicted .gitattributes\nbut for now I just implemented something simple -- look for whatever\n.gitattributes file we can find in any of the higher order stages and\nuse it.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 61 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d91b66a052b6..028d1adcd2c9 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -988,6 +988,67 @@ static int merge_submodule(struct merge_options *opt,\n \treturn 0;\n }\n \n+MAYBE_UNUSED\n+static void initialize_attr_index(struct merge_options *opt)\n+{\n+\t/*\n+\t * The renormalize_buffer() functions require attributes, and\n+\t * annoyingly those can only be read from the working tree or from\n+\t * an index_state. merge-ort doesn't have an index_state, so we\n+\t * generate a fake one containing only attribute information.\n+\t */\n+\tstruct merged_info *mi;\n+\tstruct index_state *attr_index = &opt->priv->attr_index;\n+\tstruct cache_entry *ce;\n+\n+\tif (!opt->renormalize)\n+\t\treturn;\n+\n+\tif (attr_index->initialized)\n+\t\treturn;\n+\tattr_index->initialized = 1;\n+\n+\tmi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);\n+\tif (!mi)\n+\t\treturn;\n+\n+\tif (mi->clean) {\n+\t\tint len = strlen(GITATTRIBUTES_FILE);\n+\t\tce = make_empty_cache_entry(attr_index, len);\n+\t\tce->ce_mode = create_ce_mode(mi->result.mode);\n+\t\tce->ce_flags = create_ce_flags(0);\n+\t\tce->ce_namelen = len;\n+\t\toidcpy(&ce->oid, &mi->result.oid);\n+\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n+\t\tadd_index_entry(attr_index, ce,\n+\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE, &ce->oid);\n+\t}\n+\telse {\n+\t\tint stage, len;\n+\t\tstruct conflict_info *ci;\n+\n+\t\tASSIGN_AND_VERIFY_CI(ci, mi);\n+\t\tfor (stage=0; stage<3; ++stage) {\n+\t\t\tunsigned stage_mask = (1 << stage);\n+\n+\t\t\tif (!(ci->filemask & stage_mask))\n+\t\t\t\tcontinue;\n+\t\t\tlen = strlen(GITATTRIBUTES_FILE);\n+\t\t\tce = make_empty_cache_entry(attr_index, len);\n+\t\t\tce->ce_mode = create_ce_mode(ci->stages[stage].mode);\n+\t\t\tce->ce_flags = create_ce_flags(stage);\n+\t\t\tce->ce_namelen = len;\n+\t\t\toidcpy(&ce->oid, &ci->stages[stage].oid);\n+\t\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n+\t\t\tadd_index_entry(attr_index, ce,\n+\t\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+\t\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE,\n+\t\t\t\t\t &ce->oid);\n+\t\t}\n+\t}\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-- \ngitgitgadget\n\n"},{"id":"418285","messageId":"cb035ac5fe4ab18b697eff42afedcab62880ceec.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 04/11] merge-ort: have ll_merge() calls use the attr_index for renormalization","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:30Z","receivedAt":"2021-03-05T00:55:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nll_merge() needs an index when renormalization is requested. Give it\nthe special one we created exactly for that purpose. This fixes t6418.4\nand t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 028d1adcd2c9..87c553c0882c 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -360,7 +360,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \tstring_list_clear(&opti->paths_to_free, 0);\n \topti->paths_to_free.strdup_strings = 0;\n \n-\tif (opti->attr_index.cache_nr)\n+\tif (opti->attr_index.cache_nr) /* true iff opt->renormalize */\n \t\tdiscard_index(&opti->attr_index);\n \n \t/* Free memory used by various renames maps */\n@@ -988,7 +988,6 @@ static int merge_submodule(struct merge_options *opt,\n \treturn 0;\n }\n \n-MAYBE_UNUSED\n static void initialize_attr_index(struct merge_options *opt)\n {\n \t/*\n@@ -1063,6 +1062,8 @@ static int merge_3way(struct merge_options *opt,\n \tchar *base, *name1, *name2;\n \tint merge_status;\n \n+\tinitialize_attr_index(opt);\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@@ -1101,7 +1102,7 @@ static int merge_3way(struct merge_options *opt,\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+\t\t\t\t&opt->priv->attr_index, &ll_opts);\n \n \tfree(base);\n \tfree(name1);\n-- \ngitgitgadget\n\n"},{"id":"418286","messageId":"b70ef4d7000a1d83b1588a9be1e8f4273a382cc7.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 05/11] merge-ort: let renormalization change modify/delete into clean delete","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:31Z","receivedAt":"2021-03-05T00:55:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen we have a modify/delete conflict, but the only change to the\nmodification is e.g. change of line endings, then if renormalization is\nrequested then we should be able to recognize such a case as a\nnot-modified/delete and resolve the conflict automatically.\n\nThis fixes t6418.10 under GIT_TEST_MERGE_ALGORITHM=ort.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 61 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 87c553c0882c..c4bd88b9d3db 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2416,6 +2416,60 @@ static int string_list_df_name_compare(const char *one, const char *two)\n \treturn onelen - twolen;\n }\n \n+static int read_oid_strbuf(struct merge_options *opt,\n+\t\t\t const struct object_id *oid,\n+\t\t\t struct strbuf *dst)\n+{\n+\tvoid *buf;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tbuf = read_object_file(oid, &type, &size);\n+\tif (!buf)\n+\t\treturn err(opt, _(\"cannot read object %s\"), oid_to_hex(oid));\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn err(opt, _(\"object %s is not a blob\"), oid_to_hex(oid));\n+\t}\n+\tstrbuf_attach(dst, buf, size, size + 1);\n+\treturn 0;\n+}\n+\n+static int blob_unchanged(struct merge_options *opt,\n+\t\t\t const struct version_info *base,\n+\t\t\t const struct version_info *side,\n+\t\t\t const char *path)\n+{\n+\tstruct strbuf basebuf = STRBUF_INIT;\n+\tstruct strbuf sidebuf = STRBUF_INIT;\n+\tint ret = 0; /* assume changed for safety */\n+\tconst struct index_state *idx = &opt->priv->attr_index;\n+\n+\tinitialize_attr_index(opt);\n+\n+\tif (base->mode != side->mode)\n+\t\treturn 0;\n+\tif (oideq(&base->oid, &side->oid))\n+\t\treturn 1;\n+\n+\tif (read_oid_strbuf(opt, &base->oid, &basebuf) ||\n+\t read_oid_strbuf(opt, &side->oid, &sidebuf))\n+\t\tgoto error_return;\n+\t/*\n+\t * Note: binary | is used so that both renormalizations are\n+\t * performed. Comparison can be skipped if both files are\n+\t * unchanged since their sha1s have already been compared.\n+\t */\n+\tif (renormalize_buffer(idx, path, basebuf.buf, basebuf.len, &basebuf) |\n+\t renormalize_buffer(idx, path, sidebuf.buf, sidebuf.len, &sidebuf))\n+\t\tret = (basebuf.len == sidebuf.len &&\n+\t\t !memcmp(basebuf.buf, sidebuf.buf, basebuf.len));\n+\n+error_return:\n+\tstrbuf_release(&basebuf);\n+\tstrbuf_release(&sidebuf);\n+\treturn ret;\n+}\n+\n struct directory_versions {\n \t/*\n \t * versions: list of (basename -> version_info)\n@@ -3003,8 +3057,13 @@ static void process_entry(struct merge_options *opt,\n \t\tmodify_branch = (side == 1) ? opt->branch1 : opt->branch2;\n \t\tdelete_branch = (side == 1) ? opt->branch2 : opt->branch1;\n \n-\t\tif (ci->path_conflict &&\n-\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n+\t\tif (opt->renormalize &&\n+\t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n+\t\t\t\t path)) {\n+\t\t\tci->merged.is_null = 1;\n+\t\t\tci->merged.clean = 1;\n+\t\t} else if (ci->path_conflict &&\n+\t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\n \t\t\t * This came from a rename/delete; no action to take,\n \t\t\t * but avoid printing \"modify/delete\" conflict notice\n-- \ngitgitgadget\n\n"},{"id":"418287","messageId":"d04ddabde124c165a1096c1a1d72490b2e399138.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 06/11] merge-ort: support subtree shifting","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:32Z","receivedAt":"2021-03-05T00:55:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-recursive has some simple code to support subtree shifting; copy\nit over to merge-ort. This fixes t6409.12 under\nGIT_TEST_MERGE_ALGORITHM=ort.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c4bd88b9d3db..a998f843a1da 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3417,6 +3417,23 @@ void merge_finalize(struct merge_options *opt,\n \n /*** Function Grouping: helper functions for merge_incore_*() ***/\n \n+static struct tree *shift_tree_object(struct repository *repo,\n+\t\t\t\t struct tree *one, struct tree *two,\n+\t\t\t\t const char *subtree_shift)\n+{\n+\tstruct object_id shifted;\n+\n+\tif (!*subtree_shift) {\n+\t\tshift_tree(repo, &one->object.oid, &two->object.oid, &shifted, 0);\n+\t} else {\n+\t\tshift_tree_by(repo, &one->object.oid, &two->object.oid, &shifted,\n+\t\t\t subtree_shift);\n+\t}\n+\tif (oideq(&two->object.oid, &shifted))\n+\t\treturn two;\n+\treturn lookup_tree(repo, &shifted);\n+}\n+\n static inline void set_commit_tree(struct commit *c, struct tree *t)\n {\n \tc->maybe_tree = t;\n@@ -3544,6 +3561,13 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n {\n \tstruct object_id working_tree_oid;\n \n+\tif (opt->subtree_shift) {\n+\t\tside2 = shift_tree_object(opt->repo, side1, side2,\n+\t\t\t\t\t opt->subtree_shift);\n+\t\tmerge_base = shift_tree_object(opt->repo, side1, merge_base,\n+\t\t\t\t\t opt->subtree_shift);\n+\t}\n+\n \ttrace2_region_enter(\"merge\", \"collect_merge_info\", opt->repo);\n \tif (collect_merge_info(opt, merge_base, side1, side2) != 0) {\n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"418288","messageId":"6ccb24b557fc9c9d8e3d307d3e142d8393920414.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 07/11] t6428: new test for SKIP_WORKTREE handling and conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:33Z","receivedAt":"2021-03-05T00:55:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf there is a conflict during a merge for a SKIP_WORKTREE entry, we\nexpect that file to be written to the working copy and have the\nSKIP_WORKTREE bit cleared in the index. If the user had manually\ncreated a file in the working tree despite SKIP_WORKTREE being set, we\ndo not want to clobber their changes to that file, but want to move it\nout of the way. Add tests that check for these behaviors.\n\nSigned-off-by: Elijah Newren \n---\n t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++\n 1 file changed, 158 insertions(+)\n create mode 100755 t/t6428-merge-conflicts-sparse.sh\n\ndiff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\nnew file mode 100755\nindex 000000000000..1bb52ff6f38c\n--- /dev/null\n+++ b/t/t6428-merge-conflicts-sparse.sh\n@@ -0,0 +1,158 @@\n+#!/bin/sh\n+\n+test_description=\"merge cases\"\n+\n+# The setup for all of them, pictorially, is:\n+#\n+# A\n+# o\n+# / \\\n+# O o ?\n+# \\ /\n+# o\n+# B\n+#\n+# To help make it easier to follow the flow of tests, they have been\n+# divided into sections and each test will start with a quick explanation\n+# of what commits O, A, and B contain.\n+#\n+# Notation:\n+# z/{b,c} means files z/b and z/c both exist\n+# x/d_1 means file x/d exists with content d1. (Purpose of the\n+# underscore notation is to differentiate different\n+# files that might be renamed into each other's paths.)\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-merge.sh\n+\n+\n+# Testcase basic, conflicting changes in 'numerals'\n+\n+test_setup_numerals () {\n+\ttest_create_repo numerals_$1 &&\n+\t(\n+\t\tcd numerals_$1 &&\n+\n+\t\t>README &&\n+\t\ttest_write_lines I II III >numerals &&\n+\t\tgit add README numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_write_lines I II III IIII >numerals &&\n+\t\tgit add numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\ttest_write_lines I II III IV >numerals &&\n+\t\tgit add numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\" &&\n+\n+\t\tcat <<-EOF >expected-index &&\n+\t\tH README\n+\t\tM numerals\n+\t\tM numerals\n+\t\tM numerals\n+\t\tEOF\n+\n+\t\tcat <<-EOF >expected-merge\n+\t\tI\n+\t\tII\n+\t\tIII\n+\t\t<<<<<<< HEAD\n+\t\tIIII\n+\t\t=======\n+\t\tIV\n+\t\t>>>>>>> B^0\n+\t\tEOF\n+\n+\t)\n+}\n+\n+test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n+\ttest_setup_numerals plain &&\n+\t(\n+\t\tcd numerals_plain &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\tgit sparse-checkout init &&\n+\t\tgit sparse-checkout set README &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_missing numerals &&\n+\n+\t\ttest_must_fail git merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -t >index_files &&\n+\t\ttest_cmp expected-index index_files &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\ttest_cmp expected-merge numerals &&\n+\n+\t\t# 4 other files:\n+\t\t# * expected-merge\n+\t\t# * expected-index\n+\t\t# * index_files\n+\t\t# * others\n+\t\tgit ls-files -o >others &&\n+\t\ttest_line_count = 4 others\n+\t)\n+'\n+\n+test_expect_merge_algorithm failure failure 'present-despite-SKIP_WORKTREE handled reasonably' '\n+\ttest_setup_numerals in_the_way &&\n+\t(\n+\t\tcd numerals_in_the_way &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\tgit sparse-checkout init &&\n+\t\tgit sparse-checkout set README &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_missing numerals &&\n+\n+\t\techo foobar >numerals &&\n+\n+\t\ttest_must_fail git merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -t >index_files &&\n+\t\ttest_cmp expected-index index_files &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\ttest_cmp expected-merge numerals &&\n+\n+\t\t# There should still be a file with \"foobar\" in it\n+\t\tgrep foobar * &&\n+\n+\t\t# 5 other files:\n+\t\t# * expected-merge\n+\t\t# * expected-index\n+\t\t# * index_files\n+\t\t# * others\n+\t\t# * whatever name was given to the numerals file that had\n+\t\t# \"foobar\" in it\n+\t\tgit ls-files -o >others &&\n+\t\ttest_line_count = 5 others\n+\t)\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"418289","messageId":"100c0187bdfeef5c560ecd17160ed7c9a3032156.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 08/11] merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:34Z","receivedAt":"2021-03-05T00:55:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen merge conflicts occur in paths removed by a sparse-checkout, we\nneed to unsparsify those paths (clear the SKIP_WORKTREE bit), and write\nout the conflicted file to the working copy. In the very unlikely case\nthat someone manually put a file into the working copy at the location\nof the SKIP_WORKTREE file, we need to avoid overwriting whatever edits\nthey have made and move that file to a different location first.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 43 +++++++++++++++++++++----------\n t/t6428-merge-conflicts-sparse.sh | 4 +--\n 2 files changed, 32 insertions(+), 15 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a998f843a1da..37b69cbe0f9a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3235,23 +3235,27 @@ static int checkout(struct merge_options *opt,\n \treturn ret;\n }\n \n-static int record_conflicted_index_entries(struct merge_options *opt,\n-\t\t\t\t\t struct index_state *index,\n-\t\t\t\t\t struct strmap *paths,\n-\t\t\t\t\t struct strmap *conflicted)\n+static int record_conflicted_index_entries(struct merge_options *opt)\n {\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n+\tstruct index_state *index = opt->repo->index;\n+\tstruct checkout state = CHECKOUT_INIT;\n \tint errs = 0;\n \tint original_cache_nr;\n \n-\tif (strmap_empty(conflicted))\n+\tif (strmap_empty(&opt->priv->conflicted))\n \t\treturn 0;\n \n+\t/* If any entries have skip_worktree set, we'll have to check 'em out */\n+\tstate.force = 1;\n+\tstate.quiet = 1;\n+\tstate.refresh_cache = 1;\n+\tstate.istate = index;\n \toriginal_cache_nr = index->cache_nr;\n \n \t/* Put every entry from paths into plist, then sort */\n-\tstrmap_for_each_entry(conflicted, &iter, e) {\n+\tstrmap_for_each_entry(&opt->priv->conflicted, &iter, e) {\n \t\tconst char *path = e->key;\n \t\tstruct conflict_info *ci = e->value;\n \t\tint pos;\n@@ -3292,9 +3296,23 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n \t\t\t * the higher order stages. Thus, we need override\n \t\t\t * the CE_SKIP_WORKTREE bit and manually write those\n \t\t\t * files to the working disk here.\n-\t\t\t *\n-\t\t\t * TODO: Implement this CE_SKIP_WORKTREE fixup.\n \t\t\t */\n+\t\t\tif (ce_skip_worktree(ce)) {\n+\t\t\t\tstruct stat st;\n+\n+\t\t\t\tif (!lstat(path, &st)) {\n+\t\t\t\t\tchar *new_name = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t\t\t path,\n+\t\t\t\t\t\t\t\t \"cruft\");\n+\n+\t\t\t\t\tpath_msg(opt, path, 1,\n+\t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n+\t\t\t\t\t\t path, new_name);\n+\t\t\t\t\terrs |= rename(path, new_name);\n+\t\t\t\t\tfree(new_name);\n+\t\t\t\t}\n+\t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Mark this cache entry for removal and instead add\n@@ -3344,8 +3362,6 @@ void merge_switch_to_result(struct merge_options *opt,\n {\n \tassert(opt->priv == NULL);\n \tif (result->clean >= 0 && update_worktree_and_index) {\n-\t\tstruct merge_options_internal *opti = result->priv;\n-\n \t\ttrace2_region_enter(\"merge\", \"checkout\", opt->repo);\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n@@ -3355,13 +3371,14 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n \n \t\ttrace2_region_enter(\"merge\", \"record_conflicted\", opt->repo);\n-\t\tif (record_conflicted_index_entries(opt, opt->repo->index,\n-\t\t\t\t\t\t &opti->paths,\n-\t\t\t\t\t\t &opti->conflicted)) {\n+\t\topt->priv = result->priv;\n+\t\tif (record_conflicted_index_entries(opt)) {\n \t\t\t/* failure to function */\n+\t\t\topt->priv = NULL;\n \t\t\tresult->clean = -1;\n \t\t\treturn;\n \t\t}\n+\t\topt->priv = NULL;\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n \t}\n \ndiff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\nindex 1bb52ff6f38c..7e8bf497f821 100755\n--- a/t/t6428-merge-conflicts-sparse.sh\n+++ b/t/t6428-merge-conflicts-sparse.sh\n@@ -76,7 +76,7 @@ test_setup_numerals () {\n \t)\n }\n \n-test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n+test_expect_success 'conflicting entries written to worktree even if sparse' '\n \ttest_setup_numerals plain &&\n \t(\n \t\tcd numerals_plain &&\n@@ -112,7 +112,7 @@ test_expect_merge_algorithm success failure 'conflicting entries written to work\n \t)\n '\n \n-test_expect_merge_algorithm failure failure 'present-despite-SKIP_WORKTREE handled reasonably' '\n+test_expect_merge_algorithm failure success 'present-despite-SKIP_WORKTREE handled reasonably' '\n \ttest_setup_numerals in_the_way &&\n \t(\n \t\tcd numerals_in_the_way &&\n-- \ngitgitgadget\n\n"},{"id":"418290","messageId":"95a6c0abe558f6fe2c570602650b4511d33296b2.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 09/11] t: mark several submodule merging tests as fixed under merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:35Z","receivedAt":"2021-03-05T00:55:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-ort handles submodules (and directory/file conflicts in general)\ndifferently than merge-recursive does; it basically puts all the special\nhandling for different filetypes into one place in the codebase instead\nof needing special handling for different filetypes in many different\ncode paths. This one code path in merge-ort could perhaps use some work\nstill (there are still test_expect_failure cases in the testsuite), but\nit passes all the tests that merge-recursive does as well as 12\nadditional ones that merge-recursive fails. Mark those 12 tests as\ntest_expect_success under merge-ort.\n\nSigned-off-by: Elijah Newren \n---\n t/t3512-cherry-pick-submodule.sh | 9 +++++++--\n t/t3513-revert-submodule.sh | 7 ++++++-\n t/t5572-pull-submodule.sh | 9 +++++++--\n t/t6437-submodule-merge.sh | 5 +++--\n t/t6438-submodule-directory-file-conflicts.sh | 9 +++++++--\n 5 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh\nindex 822f2d4bfbd5..eb7a47b9d98a 100755\n--- a/t/t3512-cherry-pick-submodule.sh\n+++ b/t/t3512-cherry-pick-submodule.sh\n@@ -8,8 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+then\n+\t: # No special additional KNOWN_FAILURE knobs to set\n+else\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch \"cherry-pick\"\n \n test_expect_success 'unrelated submodule/file conflict is ignored' '\ndiff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh\nindex a759f12cbb1d..1f8157ad5ba5 100755\n--- a/t/t3513-revert-submodule.sh\n+++ b/t/t3513-revert-submodule.sh\n@@ -30,7 +30,12 @@ git_revert () {\n \tgit revert HEAD\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+then\n+\t: # No special additional KNOWN_FAILURE knobs to set\n+else\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+fi\n test_submodule_switch_func \"git_revert\"\n \n test_done\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 29537f4798ef..4b4495613d04 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -42,8 +42,13 @@ git_pull_noff () {\n \t$2 git pull --no-ff\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+then\n+\t: # No special additional KNOWN_FAILURE knobs to set\n+else\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch_func \"git_pull_noff\"\n \n test_expect_success 'pull --recurse-submodule setup' '\ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex 0f92bcf326c8..e5e89c2045e7 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-merge.sh\n \n #\n # history\n@@ -328,7 +329,7 @@ test_expect_success 'setup file/submodule conflict' '\n \t)\n '\n \n-test_expect_failure 'file/submodule conflict' '\n+test_expect_merge_algorithm failure success 'file/submodule conflict' '\n \ttest_when_finished \"git -C file-submodule reset --hard\" &&\n \t(\n \t\tcd file-submodule &&\n@@ -437,7 +438,7 @@ test_expect_failure 'directory/submodule conflict; keep submodule clean' '\n \t)\n '\n \n-test_expect_failure !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n+test_expect_merge_algorithm failure success !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n \ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n \ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n \t(\ndiff --git a/t/t6438-submodule-directory-file-conflicts.sh b/t/t6438-submodule-directory-file-conflicts.sh\nindex 04bf4be7d792..abfa59d3684c 100755\n--- a/t/t6438-submodule-directory-file-conflicts.sh\n+++ b/t/t6438-submodule-directory-file-conflicts.sh\n@@ -12,8 +12,13 @@ test_submodule_switch \"merge --ff\"\n \n test_submodule_switch \"merge --ff-only\"\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+then\n+\t: # No special additional KNOWN_FAILURE knobs to set\n+else\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch \"merge --no-ff\"\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"418291","messageId":"d8c6eb39aa7c39263b7f0333ec92a3cc65c367ec.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 10/11] merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:36Z","receivedAt":"2021-03-05T00:55:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are a variety of questions users might ask while resolving\nconflicts:\n * What changes have been made since the previous (first) parent?\n * What changes are staged?\n * What is still unstaged? (or what is still conflicted?)\n * What changes did I make to resolve conflicts so far?\nThe first three of these have simple answers:\n * git diff HEAD\n * git diff --cached\n * git diff\nThere was no way to answer the final question previously. Adding one\nis trivial in merge-ort, since it works by creating a tree representing\nwhat should be written to the working copy complete with conflict\nmarkers. Simply write that tree to .git/AUTO_MERGE, allowing users to\nanswer the fourth question with\n * git diff AUTO_MERGE\n\nI avoided using a name like \"MERGE_AUTO\", because that would be\nmerge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD,\nCHERRY_PICK_HEAD) and I wanted a name that didn't change depending on\nwhich type of operation the merge was part of.\n\nEnsure that paths which clean out other temporary operation-specific\nfiles (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory)\nalso clean out this AUTO_MERGE file.\n\nSigned-off-by: Elijah Newren \n---\n branch.c | 1 +\n builtin/rebase.c | 1 +\n merge-ort.c | 10 ++++++++++\n path.c | 1 +\n path.h | 2 ++\n sequencer.c | 5 +++++\n 6 files changed, 20 insertions(+)\n\ndiff --git a/branch.c b/branch.c\nindex 9c9dae1eae32..b71a2de29dbe 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)\n \tunlink(git_path_merge_rr(r));\n \tunlink(git_path_merge_msg(r));\n \tunlink(git_path_merge_mode(r));\n+\tunlink(git_path_auto_merge(r));\n \tsave_autostash(git_path_merge_autostash(r));\n }\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex de400f9a1973..7d9afe118fd4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -739,6 +739,7 @@ static int finish_rebase(struct rebase_options *opts)\n \tint ret = 0;\n \n \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n+\tunlink(git_path_auto_merge(the_repository));\n \tapply_autostash(state_dir_path(\"autostash\", opts));\n \tclose_object_store(the_repository->objects);\n \t/*\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 37b69cbe0f9a..cf927cd160e1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3362,6 +3362,9 @@ void merge_switch_to_result(struct merge_options *opt,\n {\n \tassert(opt->priv == NULL);\n \tif (result->clean >= 0 && update_worktree_and_index) {\n+\t\tconst char *filename;\n+\t\tFILE *fp;\n+\n \t\ttrace2_region_enter(\"merge\", \"checkout\", opt->repo);\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n@@ -3380,6 +3383,13 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t}\n \t\topt->priv = NULL;\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n+\n+\t\ttrace2_region_enter(\"merge\", \"write_auto_merge\", opt->repo);\n+\t\tfilename = git_path_auto_merge(opt->repo);\n+\t\tfp = xfopen(filename, \"w\");\n+\t\tfprintf(fp, \"%s\\n\", oid_to_hex(&result->tree->object.oid));\n+\t\tfclose(fp);\n+\t\ttrace2_region_leave(\"merge\", \"write_auto_merge\", opt->repo);\n \t}\n \n \tif (display_update_msgs) {\ndiff --git a/path.c b/path.c\nindex 7b385e5eb282..9e883eb52446 100644\n--- a/path.c\n+++ b/path.c\n@@ -1534,5 +1534,6 @@ REPO_GIT_PATH_FUNC(merge_rr, \"MERGE_RR\")\n REPO_GIT_PATH_FUNC(merge_mode, \"MERGE_MODE\")\n REPO_GIT_PATH_FUNC(merge_head, \"MERGE_HEAD\")\n REPO_GIT_PATH_FUNC(merge_autostash, \"MERGE_AUTOSTASH\")\n+REPO_GIT_PATH_FUNC(auto_merge, \"AUTO_MERGE\")\n REPO_GIT_PATH_FUNC(fetch_head, \"FETCH_HEAD\")\n REPO_GIT_PATH_FUNC(shallow, \"shallow\")\ndiff --git a/path.h b/path.h\nindex e7e77da6aaa5..251c78d98000 100644\n--- a/path.h\n+++ b/path.h\n@@ -176,6 +176,7 @@ struct path_cache {\n \tconst char *merge_mode;\n \tconst char *merge_head;\n \tconst char *merge_autostash;\n+\tconst char *auto_merge;\n \tconst char *fetch_head;\n \tconst char *shallow;\n };\n@@ -191,6 +192,7 @@ const char *git_path_merge_rr(struct repository *r);\n const char *git_path_merge_mode(struct repository *r);\n const char *git_path_merge_head(struct repository *r);\n const char *git_path_merge_autostash(struct repository *r);\n+const char *git_path_auto_merge(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d2332d3e1787..472cdd8c620d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2096,6 +2096,7 @@ static int do_pick_commit(struct repository *r,\n \t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n \t\t\t\tNULL, 0);\n \t\tunlink(git_path_merge_msg(r));\n+\t\tunlink(git_path_auto_merge(r));\n \t\tfprintf(stderr,\n \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n@@ -2451,6 +2452,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \t\tneed_cleanup = 1;\n \t}\n \n+\tunlink(git_path_auto_merge(r));\n+\n \tif (!need_cleanup)\n \t\treturn;\n \n@@ -4111,6 +4114,7 @@ static int pick_commits(struct repository *r,\n \t\t\tunlink(rebase_path_stopped_sha());\n \t\t\tunlink(rebase_path_amend());\n \t\t\tunlink(git_path_merge_head(r));\n+\t\t\tunlink(git_path_auto_merge(r));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n \t\t\tif (item->command == TODO_BREAK) {\n@@ -4505,6 +4509,7 @@ static int commit_staged_changes(struct repository *r,\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n \tunlink(git_path_merge_head(r));\n+\tunlink(git_path_auto_merge(r));\n \tif (final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n \t\tunlink(rebase_path_squash_msg());\n-- \ngitgitgadget\n\n"},{"id":"418292","messageId":"0409118d4ff76a8dc0285c436f89478176752955.1614905738.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH 11/11] merge-recursive: add a bunch of FIXME comments documenting known bugs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-05T00:55:37Z","receivedAt":"2021-03-05T00:55:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe plan is to just delete merge-recursive, but not until everyone is\ncomfortable with merge-ort as a replacement. Given that I haven't\nswitched all callers of merge-recursive over yet (e.g. git-am still uses\nmerge-recursive), maybe there's some value documenting known bugs in the\nalgorithm in case we end up keeping it or someone wants to dig it up in\nthe future.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b052974f191c..99a197597db5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1075,6 +1075,11 @@ static int merge_3way(struct merge_options *opt,\n \tread_mmblob(&src1, &a->oid);\n \tread_mmblob(&src2, &b->oid);\n \n+\t/*\n+\t * FIXME: Using a->path for normalization rules in ll_merge could be\n+\t * wrong if we renamed from a->path to b->path. We should use the\n+\t * target path for where the file will be written.\n+\t */\n \tmerge_status = ll_merge(result_buf, a->path, &orig, base,\n \t\t\t\t&src1, name1, &src2, name2,\n \t\t\t\topt->repo->index, &ll_opts);\n@@ -1154,6 +1159,8 @@ static void print_commit(struct commit *commit)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n \tctx.date_mode.type = DATE_NORMAL;\n+\t/* FIXME: Merge this with output_commit_title() */\n+\tassert(!merge_remote_util(commit));\n \tformat_commit_message(commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n@@ -1177,6 +1184,11 @@ static int merge_submodule(struct merge_options *opt,\n \tint search = !opt->priv->call_depth;\n \n \t/* store a in result in case we fail */\n+\t/* FIXME: This is the WRONG resolution for the recursive case when\n+\t * we need to be careful to avoid accidentally matching either side.\n+\t * Should probably use o instead there, much like we do for merging\n+\t * binaries.\n+\t */\n \toidcpy(result, a);\n \n \t/* we can not handle deletion conflicts */\n@@ -1301,6 +1313,13 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \n \tif ((S_IFMT & a->mode) != (S_IFMT & b->mode)) {\n \t\tresult->clean = 0;\n+\t\t/*\n+\t\t * FIXME: This is a bad resolution for recursive case; for\n+\t\t * the recursive case we want something that is unlikely to\n+\t\t * accidentally match either side. Also, while it makes\n+\t\t * sense to prefer regular files over symlinks, it doesn't\n+\t\t * make sense to prefer regular files over submodules.\n+\t\t */\n \t\tif (S_ISREG(a->mode)) {\n \t\t\tresult->blob.mode = a->mode;\n \t\t\toidcpy(&result->blob.oid, &a->oid);\n@@ -1349,6 +1368,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\tfree(result_buf.ptr);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n+\t\t\t/* FIXME: bug, what if modes didn't match? */\n \t\t\tresult->clean = (merge_status == 0);\n \t\t} else if (S_ISGITLINK(a->mode)) {\n \t\t\tresult->clean = merge_submodule(opt, &result->blob.oid,\n@@ -2664,6 +2684,14 @@ static int process_renames(struct merge_options *opt,\n \tstruct string_list b_by_dst = STRING_LIST_INIT_NODUP;\n \tconst struct rename *sre;\n \n+\t/*\n+\t * FIXME: As string-list.h notes, it's O(n^2) to build a sorted\n+\t * string_list one-by-one, but O(n log n) to build it unsorted and\n+\t * then sort it. Note that as we build the list, we do not need to\n+\t * check if the existing destination path is already in the list,\n+\t * because the structure of diffcore_rename guarantees we won't\n+\t * have duplicates.\n+\t */\n \tfor (i = 0; i < a_renames->nr; i++) {\n \t\tsre = a_renames->items[i].util;\n \t\tstring_list_insert(&a_by_dst, sre->pair->two->path)->util\n@@ -3602,6 +3630,15 @@ static int merge_recursive_internal(struct merge_options *opt,\n \t\t\treturn err(opt, _(\"merge returned no commit\"));\n \t}\n \n+\t/*\n+\t * FIXME: Since merge_recursive_internal() is only ever called by\n+\t * places that ensure the index is loaded first\n+\t * (e.g. builtin/merge.c, rebase/sequencer, etc.), in the common\n+\t * case where the merge base was unique that means when we get here\n+\t * we immediately discard the index and re-read it, which is a\n+\t * complete waste of time. We should only be discarding and\n+\t * re-reading if we were forced to recurse.\n+\t */\n \tdiscard_index(opt->repo->index);\n \tif (!opt->priv->call_depth)\n \t\trepo_read_index(opt->repo);\n-- \ngitgitgadget\n"},{"id":"418436","messageId":"87czw9ke2m.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"815af5d30ebd5e7f80aa42e4a54808af2e3781e0.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/11] merge-ort: add a function for initializing our special attr_index","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T12:46:57Z","receivedAt":"2021-03-08T12:47:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> Add a function which can be called to populate the attr_index with the\n> appropriate .gitattributes contents when necessary. Make it return\n> early if the attr_index is already initialized or if we are not\n> renormalizing files.\n>\n> NOTE 1: Even if the user has a working copy or a real index (which is\n> not a given as merge-ort can be used in bare repositories), we\n> explicitly ignore any .gitattributes file from either of these\n> locations. merge-ort can be used to merge two branches that are\n> unrelated to HEAD, so .gitattributes from the working copy and current\n> index should not be considered relevant.\n>\n> NOTE 2: Since we are in the middle of merging, there is a risk that\n> .gitattributes itself is conflicted...leaving us with an ill-defined\n> situation about how to perform the rest of the merge. It could be that\n> the .gitattributes file does not even exist on one of the sides of the\n> merge, or that it has been modified on both sides. If it's been\n> modified on both sides, it's possible that it could itself be merged\n> cleanly, though it's also possible that it only merges cleanly if you\n> use the right version of the .gitattributes file to drive the merge. It\n> gets kind of complicated. The only test we ever had that attempted to\n> test behavior in this area was seemingly unaware of the undefined\n> behavior, but knew the test wouldn't work for lack of attribute handling\n> support, marked it as test_expect_failure from the beginning, but\n> managed to fail for several reasons unrelated to attribute handling.\n> See commit 6f6e7cfb52 (\"t6038: remove problematic test\", 2020-08-03) for\n> details. So there are probably various ways to improve what\n> initialize_attr_index() picks in the case of a conflicted .gitattributes\n> but for now I just implemented something simple -- look for whatever\n> .gitattributes file we can find in any of the higher order stages and\n> use it.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 61 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index d91b66a052b6..028d1adcd2c9 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -988,6 +988,67 @@ static int merge_submodule(struct merge_options *opt,\n> \treturn 0;\n> }\n> \n> +MAYBE_UNUSED\n\nAs with the lst series you had I also think this is better squashed with\n04/11.\n\n> +static void initialize_attr_index(struct merge_options *opt)\n> +{\n> +\t/*\n> +\t * The renormalize_buffer() functions require attributes, and\n> +\t * annoyingly those can only be read from the working tree or from\n> +\t * an index_state. merge-ort doesn't have an index_state, so we\n> +\t * generate a fake one containing only attribute information.\n> +\t */\n> +\tstruct merged_info *mi;\n> +\tstruct index_state *attr_index = &opt->priv->attr_index;\n> +\tstruct cache_entry *ce;\n> +\n> +\tif (!opt->renormalize)\n> +\t\treturn;\n> +\n> +\tif (attr_index->initialized)\n> +\t\treturn;\n\nWill comment on this in 04/11.\n\n> +\tattr_index->initialized = 1;\n> +\n> +\tmi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);\n> +\tif (!mi)\n> +\t\treturn;\n> +\n> +\tif (mi->clean) {\n> +\t\tint len = strlen(GITATTRIBUTES_FILE);\n> +\t\tce = make_empty_cache_entry(attr_index, len);\n> +\t\tce->ce_mode = create_ce_mode(mi->result.mode);\n> +\t\tce->ce_flags = create_ce_flags(0);\n> +\t\tce->ce_namelen = len;\n> +\t\toidcpy(&ce->oid, &mi->result.oid);\n> +\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n> +\t\tadd_index_entry(attr_index, ce,\n> +\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n> +\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE, &ce->oid);\n> +\t}\n> +\telse {\n\nStyle nit: } else {\n\n> +\t\tint stage, len;\n> +\t\tstruct conflict_info *ci;\n> +\n> +\t\tASSIGN_AND_VERIFY_CI(ci, mi);\n> +\t\tfor (stage=0; stage<3; ++stage) {\n\nStyle nit: stage < 3\n\nStyle nit: I find just stage++ to be more readable in for-loops, makes\nno difference to the compiler, just more idiomatic.\n\n> +\t\t\tunsigned stage_mask = (1 << stage);\n> +\n> +\t\t\tif (!(ci->filemask & stage_mask))\n> +\t\t\t\tcontinue;\n> +\t\t\tlen = strlen(GITATTRIBUTES_FILE);\n> +\t\t\tce = make_empty_cache_entry(attr_index, len);\n> +\t\t\tce->ce_mode = create_ce_mode(ci->stages[stage].mode);\n> +\t\t\tce->ce_flags = create_ce_flags(stage);\n> +\t\t\tce->ce_namelen = len;\n> +\t\t\toidcpy(&ce->oid, &ci->stages[stage].oid);\n> +\t\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n> +\t\t\tadd_index_entry(attr_index, ce,\n> +\t\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n> +\t\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE,\n> +\t\t\t\t\t &ce->oid);\n> +\t\t}\n> +\t}\n> +}\n> +\n\n"},{"id":"418437","messageId":"87a6rdkdxq.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"cb035ac5fe4ab18b697eff42afedcab62880ceec.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/11] merge-ort: have ll_merge() calls use the attr_index for renormalization","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T12:49:53Z","receivedAt":"2021-03-08T12:50:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> ll_merge() needs an index when renormalization is requested. Give it\n> the special one we created exactly for that purpose. This fixes t6418.4\n> and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 028d1adcd2c9..87c553c0882c 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -360,7 +360,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n> \tstring_list_clear(&opti->paths_to_free, 0);\n> \topti->paths_to_free.strdup_strings = 0;\n> \n> -\tif (opti->attr_index.cache_nr)\n> +\tif (opti->attr_index.cache_nr) /* true iff opt->renormalize */\n> \t\tdiscard_index(&opti->attr_index);\n\nPerhaps instead of a comment, in that \"if\":\n\n assert(opt->renormalize);\n\n> \t/* Free memory used by various renames maps */\n> @@ -988,7 +988,6 @@ static int merge_submodule(struct merge_options *opt,\n> \treturn 0;\n> }\n> \n> -MAYBE_UNUSED\n> static void initialize_attr_index(struct merge_options *opt)\n> {\n> \t/*\n> @@ -1063,6 +1062,8 @@ static int merge_3way(struct merge_options *opt,\n> \tchar *base, *name1, *name2;\n> \tint merge_status;\n> \n> +\tinitialize_attr_index(opt);\n\nSubjective, but I think it's more readable to move the \"initialized\"\ncheck in initialize_attr_index() here, so:\n\n if (!attr_index->initialized)\n initialize_attr_index(opt);\n\nSaves the reader a trip to the function to see that it doesn't do\nanything except exit early on that flag.\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> @@ -1101,7 +1102,7 @@ static int merge_3way(struct merge_options *opt,\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> +\t\t\t\t&opt->priv->attr_index, &ll_opts);\n> \n> \tfree(base);\n> \tfree(name1);\n\n"},{"id":"418438","messageId":"877dmhkdo9.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"b70ef4d7000a1d83b1588a9be1e8f4273a382cc7.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 05/11] merge-ort: let renormalization change modify/delete into clean delete","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T12:55:34Z","receivedAt":"2021-03-08T12:56:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> When we have a modify/delete conflict, but the only change to the\n> modification is e.g. change of line endings, then if renormalization is\n> requested then we should be able to recognize such a case as a\n> not-modified/delete and resolve the conflict automatically.\n>\n> This fixes t6418.10 under GIT_TEST_MERGE_ALGORITHM=ort.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 61 insertions(+), 2 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 87c553c0882c..c4bd88b9d3db 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -2416,6 +2416,60 @@ static int string_list_df_name_compare(const char *one, const char *two)\n> \treturn onelen - twolen;\n> }\n> \n> +static int read_oid_strbuf(struct merge_options *opt,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t struct strbuf *dst)\n> +{\n> +\tvoid *buf;\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\tbuf = read_object_file(oid, &type, &size);\n> +\tif (!buf)\n> +\t\treturn err(opt, _(\"cannot read object %s\"), oid_to_hex(oid));\n> +\tif (type != OBJ_BLOB) {\n> +\t\tfree(buf);\n> +\t\treturn err(opt, _(\"object %s is not a blob\"), oid_to_hex(oid));\n\nAs an aside I've got another series I'll submit soon which refactors all\nthese \"object is not xyz\" calls to a utility function, so in this case\nwe'd also say what it was other than a blob.\n\nFine to keep this here, just a #leftoverbits note to myself to\neventually migrate this.\n\n> +\t}\n> +\tstrbuf_attach(dst, buf, size, size + 1);\n> +\treturn 0;\n> +}\n> +\n> +static int blob_unchanged(struct merge_options *opt,\n> +\t\t\t const struct version_info *base,\n> +\t\t\t const struct version_info *side,\n> +\t\t\t const char *path)\n> +{\n> +\tstruct strbuf basebuf = STRBUF_INIT;\n> +\tstruct strbuf sidebuf = STRBUF_INIT;\n> +\tint ret = 0; /* assume changed for safety */\n> +\tconst struct index_state *idx = &opt->priv->attr_index;\n> +\n> +\tinitialize_attr_index(opt);\n> +\n> +\tif (base->mode != side->mode)\n> +\t\treturn 0;\n> +\tif (oideq(&base->oid, &side->oid))\n> +\t\treturn 1;\n> +\n> +\tif (read_oid_strbuf(opt, &base->oid, &basebuf) ||\n> +\t read_oid_strbuf(opt, &side->oid, &sidebuf))\n> +\t\tgoto error_return;\n> +\t/*\n> +\t * Note: binary | is used so that both renormalizations are\n> +\t * performed. Comparison can be skipped if both files are\n> +\t * unchanged since their sha1s have already been compared.\n> +\t */\n> +\tif (renormalize_buffer(idx, path, basebuf.buf, basebuf.len, &basebuf) |\n> +\t renormalize_buffer(idx, path, sidebuf.buf, sidebuf.len, &sidebuf))\n> +\t\tret = (basebuf.len == sidebuf.len &&\n> +\t\t !memcmp(basebuf.buf, sidebuf.buf, basebuf.len));\n> +\n> +error_return:\n> +\tstrbuf_release(&basebuf);\n> +\tstrbuf_release(&sidebuf);\n> +\treturn ret;\n> +}\n> +\n>\n> struct directory_versions {\n> \t/*\n> \t * versions: list of (basename -> version_info)\n> @@ -3003,8 +3057,13 @@ static void process_entry(struct merge_options *opt,\n> \t\tmodify_branch = (side == 1) ? opt->branch1 : opt->branch2;\n> \t\tdelete_branch = (side == 1) ? opt->branch2 : opt->branch1;\n> \n> -\t\tif (ci->path_conflict &&\n> -\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n> +\t\tif (opt->renormalize &&\n> +\t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n> +\t\t\t\t path)) {\n> +\t\t\tci->merged.is_null = 1;\n> +\t\t\tci->merged.clean = 1;\n> +\t\t} else if (ci->path_conflict &&\n> +\t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n\nSmall note (no need for re-roll or whatever) on having read a bit of\nmerge-ort.c code recently: I'd find this thing a bit easier on the eyes\nif ci->stages[0] and ci->stages[side] were split into a variable before\nthe if/else, i.e. used as \"side_0.oid and side_n.oid\" and \"side_0 and\nside_n\" in this case..\n\nThat would also avoid the wrapping of at least one argument list here.\n"},{"id":"418439","messageId":"874khlkdb0.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"6ccb24b557fc9c9d8e3d307d3e142d8393920414.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 07/11] t6428: new test for SKIP_WORKTREE handling and conflicts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T13:03:31Z","receivedAt":"2021-03-08T13:04:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> If there is a conflict during a merge for a SKIP_WORKTREE entry, we\n> expect that file to be written to the working copy and have the\n> SKIP_WORKTREE bit cleared in the index. If the user had manually\n> created a file in the working tree despite SKIP_WORKTREE being set, we\n> do not want to clobber their changes to that file, but want to move it\n> out of the way. Add tests that check for these behaviors.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++\n> 1 file changed, 158 insertions(+)\n> create mode 100755 t/t6428-merge-conflicts-sparse.sh\n>\n> diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\n> new file mode 100755\n> index 000000000000..1bb52ff6f38c\n> --- /dev/null\n> +++ b/t/t6428-merge-conflicts-sparse.sh\n> @@ -0,0 +1,158 @@\n> +#!/bin/sh\n> +\n> +test_description=\"merge cases\"\n> +\n> +# The setup for all of them, pictorially, is:\n> +#\n> +# A\n> +# o\n> +# / \\\n> +# O o ?\n> +# \\ /\n> +# o\n> +# B\n> +#\n> +# To help make it easier to follow the flow of tests, they have been\n> +# divided into sections and each test will start with a quick explanation\n> +# of what commits O, A, and B contain.\n> +#\n> +# Notation:\n> +# z/{b,c} means files z/b and z/c both exist\n> +# x/d_1 means file x/d exists with content d1. (Purpose of the\n> +# underscore notation is to differentiate different\n> +# files that might be renamed into each other's paths.)\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-merge.sh\n> +\n> +\n> +# Testcase basic, conflicting changes in 'numerals'\n> +\n> +test_setup_numerals () {\n> +\ttest_create_repo numerals_$1 &&\n> +\t(\n> +\t\tcd numerals_$1 &&\n> +\n> +\t\t>README &&\n> +\t\ttest_write_lines I II III >numerals &&\n> +\t\tgit add README numerals &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"O\" &&\n\nAs an aside this could use the --printf option to test_commit I've got\nin next, but that's also a bit painful to use since you can't use\ntest_write_lines.\n\nI've wanted to just support something like this for this use-case of\nusing an existing file:\n\n test_write_lines A B C D >lines &&\n test_commit --add O lines &&\n\n\n> +\n> +\t\tgit branch O &&\n> +\t\tgit branch A &&\n> +\t\tgit branch B &&\n> +\n> +\t\tgit checkout A &&\n> +\t\ttest_write_lines I II III IIII >numerals &&\n> +\t\tgit add numerals &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"A\" &&\n> +\n> +\t\tgit checkout B &&\n> +\t\ttest_write_lines I II III IV >numerals &&\n> +\t\tgit add numerals &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"B\" &&\n> +\n> +\t\tcat <<-EOF >expected-index &&\n> +\t\tH README\n> +\t\tM numerals\n> +\t\tM numerals\n> +\t\tM numerals\n> +\t\tEOF\n> +\n> +\t\tcat <<-EOF >expected-merge\n> +\t\tI\n> +\t\tII\n> +\t\tIII\n> +\t\t<<<<<<< HEAD\n> +\t\tIIII\n> +\t\t=======\n> +\t\tIV\n> +\t\t>>>>>>> B^0\n> +\t\tEOF\n> +\n> +\t)\n> +}\n> +\n> +test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n> +\ttest_setup_numerals plain &&\n\nA small nit, but makes it easier to debug things: I think having what\nyou have in \"test_setup_numerals\" above in a test_expect_success is a\nbetter pattern, then if it fails we can see where exactly.\n\nThen instead of calling \"test_setup_numerals\" here you'd do:\n\n cp -R template plain &&\n\nTo just copy over that existing setup template, or re-use it and have\nhave the tests call a small helper to \"test_when_finish\" reset --hard\nback as appropriate.\n"},{"id":"418440","messageId":"871rcpkd6v.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"100c0187bdfeef5c560ecd17160ed7c9a3032156.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 08/11] merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T13:06:00Z","receivedAt":"2021-03-08T13:06:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> When merge conflicts occur in paths removed by a sparse-checkout, we\n> need to unsparsify those paths (clear the SKIP_WORKTREE bit), and write\n> out the conflicted file to the working copy. In the very unlikely case\n> that someone manually put a file into the working copy at the location\n> of the SKIP_WORKTREE file, we need to avoid overwriting whatever edits\n> they have made and move that file to a different location first.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 43 +++++++++++++++++++++----------\n> t/t6428-merge-conflicts-sparse.sh | 4 +--\n> 2 files changed, 32 insertions(+), 15 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index a998f843a1da..37b69cbe0f9a 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3235,23 +3235,27 @@ static int checkout(struct merge_options *opt,\n> \treturn ret;\n> }\n> \n> -static int record_conflicted_index_entries(struct merge_options *opt,\n> -\t\t\t\t\t struct index_state *index,\n> -\t\t\t\t\t struct strmap *paths,\n> -\t\t\t\t\t struct strmap *conflicted)\n> +static int record_conflicted_index_entries(struct merge_options *opt)\n> {\n> \tstruct hashmap_iter iter;\n> \tstruct strmap_entry *e;\n> +\tstruct index_state *index = opt->repo->index;\n> +\tstruct checkout state = CHECKOUT_INIT;\n> \tint errs = 0;\n> \tint original_cache_nr;\n> \n> -\tif (strmap_empty(conflicted))\n> +\tif (strmap_empty(&opt->priv->conflicted))\n> \t\treturn 0;\n> \n> +\t/* If any entries have skip_worktree set, we'll have to check 'em out */\n> +\tstate.force = 1;\n> +\tstate.quiet = 1;\n> +\tstate.refresh_cache = 1;\n> +\tstate.istate = index;\n> \toriginal_cache_nr = index->cache_nr;\n> \n> \t/* Put every entry from paths into plist, then sort */\n> -\tstrmap_for_each_entry(conflicted, &iter, e) {\n> +\tstrmap_for_each_entry(&opt->priv->conflicted, &iter, e) {\n> \t\tconst char *path = e->key;\n> \t\tstruct conflict_info *ci = e->value;\n> \t\tint pos;\n> @@ -3292,9 +3296,23 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n> \t\t\t * the higher order stages. Thus, we need override\n> \t\t\t * the CE_SKIP_WORKTREE bit and manually write those\n> \t\t\t * files to the working disk here.\n> -\t\t\t *\n> -\t\t\t * TODO: Implement this CE_SKIP_WORKTREE fixup.\n> \t\t\t */\n> +\t\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\t\tstruct stat st;\n> +\n> +\t\t\t\tif (!lstat(path, &st)) {\n> +\t\t\t\t\tchar *new_name = unique_path(&opt->priv->paths,\n> +\t\t\t\t\t\t\t\t path,\n> +\t\t\t\t\t\t\t\t \"cruft\");\n> +\n> +\t\t\t\t\tpath_msg(opt, path, 1,\n> +\t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n> +\t\t\t\t\t\t path, new_name);\n\nI see this follows existing uses in merge-ort.c, but I wonder if this\nwon't be quite unreadable on long paths, i.e.:\n\n renamed to \n\nAs opposed to:\n\n We had to rename your thing:\n from: \n to: \n"},{"id":"418441","messageId":"87y2exiyjy.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"95a6c0abe558f6fe2c570602650b4511d33296b2.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 09/11] t: mark several submodule merging tests as fixed under merge-ort","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T13:07:29Z","receivedAt":"2021-03-08T13:08:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> merge-ort handles submodules (and directory/file conflicts in general)\n> differently than merge-recursive does; it basically puts all the special\n> handling for different filetypes into one place in the codebase instead\n> of needing special handling for different filetypes in many different\n> code paths. This one code path in merge-ort could perhaps use some work\n> still (there are still test_expect_failure cases in the testsuite), but\n> it passes all the tests that merge-recursive does as well as 12\n> additional ones that merge-recursive fails. Mark those 12 tests as\n> test_expect_success under merge-ort.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> t/t3512-cherry-pick-submodule.sh | 9 +++++++--\n> t/t3513-revert-submodule.sh | 7 ++++++-\n> t/t5572-pull-submodule.sh | 9 +++++++--\n> t/t6437-submodule-merge.sh | 5 +++--\n> t/t6438-submodule-directory-file-conflicts.sh | 9 +++++++--\n> 5 files changed, 30 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh\n> index 822f2d4bfbd5..eb7a47b9d98a 100755\n> --- a/t/t3512-cherry-pick-submodule.sh\n> +++ b/t/t3512-cherry-pick-submodule.sh\n> @@ -8,8 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n> \n> -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n> -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n> +if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> +then\n> +\t: # No special additional KNOWN_FAILURE knobs to set\n> +else\n> +\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n> +\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n> +fi\n\nnit: I think this and the rest would be just as readable/obvious without\nthe comment as:\n\n if test \"$x\" != ort\n then\n some_vars=xyz\n fi\n"},{"id":"418442","messageId":"87v9a1iycx.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"d8c6eb39aa7c39263b7f0333ec92a3cc65c367ec.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T13:11:42Z","receivedAt":"2021-03-08T13:12:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> There are a variety of questions users might ask while resolving\n> conflicts:\n> * What changes have been made since the previous (first) parent?\n> * What changes are staged?\n> * What is still unstaged? (or what is still conflicted?)\n> * What changes did I make to resolve conflicts so far?\n> The first three of these have simple answers:\n> * git diff HEAD\n> * git diff --cached\n> * git diff\n> There was no way to answer the final question previously. Adding one\n> is trivial in merge-ort, since it works by creating a tree representing\n> what should be written to the working copy complete with conflict\n> markers. Simply write that tree to .git/AUTO_MERGE, allowing users to\n> answer the fourth question with\n> * git diff AUTO_MERGE\n>\n> I avoided using a name like \"MERGE_AUTO\", because that would be\n> merge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD,\n> CHERRY_PICK_HEAD) and I wanted a name that didn't change depending on\n> which type of operation the merge was part of.\n\nThat's a really cool feature. I'm starting to like this \"ort\" thing :)\n\n(After knowing almost nothing about it until a few days ago...)\n\n> Ensure that paths which clean out other temporary operation-specific\n> files (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory)\n> also clean out this AUTO_MERGE file.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> branch.c | 1 +\n> builtin/rebase.c | 1 +\n> merge-ort.c | 10 ++++++++++\n> path.c | 1 +\n> path.h | 2 ++\n> sequencer.c | 5 +++++\n> 6 files changed, 20 insertions(+)\n>\n> diff --git a/branch.c b/branch.c\n> index 9c9dae1eae32..b71a2de29dbe 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)\n> \tunlink(git_path_merge_rr(r));\n> \tunlink(git_path_merge_msg(r));\n> \tunlink(git_path_merge_mode(r));\n> +\tunlink(git_path_auto_merge(r));\n> \tsave_autostash(git_path_merge_autostash(r));\n> }\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index de400f9a1973..7d9afe118fd4 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -739,6 +739,7 @@ static int finish_rebase(struct rebase_options *opts)\n> \tint ret = 0;\n> \n> \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n> +\tunlink(git_path_auto_merge(the_repository));\n> \tapply_autostash(state_dir_path(\"autostash\", opts));\n> \tclose_object_store(the_repository->objects);\n> \t/*\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 37b69cbe0f9a..cf927cd160e1 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3362,6 +3362,9 @@ void merge_switch_to_result(struct merge_options *opt,\n> {\n> \tassert(opt->priv == NULL);\n> \tif (result->clean >= 0 && update_worktree_and_index) {\n> +\t\tconst char *filename;\n> +\t\tFILE *fp;\n> +\n> \t\ttrace2_region_enter(\"merge\", \"checkout\", opt->repo);\n> \t\tif (checkout(opt, head, result->tree)) {\n> \t\t\t/* failure to function */\n> @@ -3380,6 +3383,13 @@ void merge_switch_to_result(struct merge_options *opt,\n> \t\t}\n> \t\topt->priv = NULL;\n> \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n> +\n> +\t\ttrace2_region_enter(\"merge\", \"write_auto_merge\", opt->repo);\n> +\t\tfilename = git_path_auto_merge(opt->repo);\n> +\t\tfp = xfopen(filename, \"w\");\n> +\t\tfprintf(fp, \"%s\\n\", oid_to_hex(&result->tree->object.oid));\n> +\t\tfclose(fp);\n> +\t\ttrace2_region_leave(\"merge\", \"write_auto_merge\", opt->repo);\n\nThis isn't a new problem since you're just folling an existing pattern,\nbut here you (rightly) do xopen()< and the:n\n\n> \t}\n> \n> \tif (display_update_msgs) {\n> diff --git a/path.c b/path.c\n> index 7b385e5eb282..9e883eb52446 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -1534,5 +1534,6 @@ REPO_GIT_PATH_FUNC(merge_rr, \"MERGE_RR\")\n> REPO_GIT_PATH_FUNC(merge_mode, \"MERGE_MODE\")\n> REPO_GIT_PATH_FUNC(merge_head, \"MERGE_HEAD\")\n> REPO_GIT_PATH_FUNC(merge_autostash, \"MERGE_AUTOSTASH\")\n> +REPO_GIT_PATH_FUNC(auto_merge, \"AUTO_MERGE\")\n> REPO_GIT_PATH_FUNC(fetch_head, \"FETCH_HEAD\")\n> REPO_GIT_PATH_FUNC(shallow, \"shallow\")\n> diff --git a/path.h b/path.h\n> index e7e77da6aaa5..251c78d98000 100644\n> --- a/path.h\n> +++ b/path.h\n> @@ -176,6 +176,7 @@ struct path_cache {\n> \tconst char *merge_mode;\n> \tconst char *merge_head;\n> \tconst char *merge_autostash;\n> +\tconst char *auto_merge;\n> \tconst char *fetch_head;\n> \tconst char *shallow;\n> };\n> @@ -191,6 +192,7 @@ const char *git_path_merge_rr(struct repository *r);\n> const char *git_path_merge_mode(struct repository *r);\n> const char *git_path_merge_head(struct repository *r);\n> const char *git_path_merge_autostash(struct repository *r);\n> +const char *git_path_auto_merge(struct repository *r);\n> const char *git_path_fetch_head(struct repository *r);\n> const char *git_path_shallow(struct repository *r);\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index d2332d3e1787..472cdd8c620d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2096,6 +2096,7 @@ static int do_pick_commit(struct repository *r,\n> \t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> \t\t\t\tNULL, 0);\n> \t\tunlink(git_path_merge_msg(r));\n> +\t\tunlink(git_path_auto_merge(r));\n\nShouldn't this & the rest ideally be at least unlink_or_warn()?\n\n> \t\tfprintf(stderr,\n> \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n> \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n> @@ -2451,6 +2452,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n> \t\tneed_cleanup = 1;\n> \t}\n> \n> +\tunlink(git_path_auto_merge(r));\n> +\n> \tif (!need_cleanup)\n> \t\treturn;\n> \n> @@ -4111,6 +4114,7 @@ static int pick_commits(struct repository *r,\n> \t\t\tunlink(rebase_path_stopped_sha());\n> \t\t\tunlink(rebase_path_amend());\n> \t\t\tunlink(git_path_merge_head(r));\n> +\t\t\tunlink(git_path_auto_merge(r));\n> \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n> \n> \t\t\tif (item->command == TODO_BREAK) {\n> @@ -4505,6 +4509,7 @@ static int commit_staged_changes(struct repository *r,\n> \t\treturn error(_(\"could not commit staged changes.\"));\n> \tunlink(rebase_path_amend());\n> \tunlink(git_path_merge_head(r));\n> +\tunlink(git_path_auto_merge(r));\n> \tif (final_fixup) {\n> \t\tunlink(rebase_path_fixup_msg());\n> \t\tunlink(rebase_path_squash_msg());\n\n"},{"id":"418443","messageId":"87sg55iyc0.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"0409118d4ff76a8dc0285c436f89478176752955.1614905738.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] merge-recursive: add a bunch of FIXME comments documenting known bugs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T13:12:15Z","receivedAt":"2021-03-08T13:13:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> The plan is to just delete merge-recursive, but not until everyone is\n> comfortable with merge-ort as a replacement. Given that I haven't\n> switched all callers of merge-recursive over yet (e.g. git-am still uses\n> merge-recursive), maybe there's some value documenting known bugs in the\n> algorithm in case we end up keeping it or someone wants to dig it up in\n> the future.\n\nYes, I think that's very useful.\n"},{"id":"418446","messageId":"87pn09iu3j.fsf@evledraar.gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/11] Complete merge-ort implementation...almost","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-08T14:43:44Z","receivedAt":"2021-03-08T14:44:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n\n> In order to help Ævar test his tree-walk changes against merge-ort[1], this\n> series completes the merge-ort implementation and cleans up testsuite\n> failures...EXCEPT for some t6423 failures. It also leaves out a lot of\n> performance work, which incidentally will fix the t6423 failures and is\n> being reviewed independently[2].\n\nFor those testing this in combination with their work, the expected\nfailures are these specific tests:\n\n ./t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n Failed tests: 7, 53, 55, 59\n\nPerhaps I'm missing something but why not have [1] below on top of this\nseries? It makes that test pass in both modes, and means we could\ne.g. follow-up by running CI with \"ort\".\n\nThat CI step seems worth doing sooner than later, even if it needs some\nGIT_TEST_SKIP for now...\n\n> This 11-patch series could be submitted as 7 independent series, 1-4 patches\n> in length each, but it's probably easier for Ævar if we can merge just one\n> more thing and it's only 11 total patches. This series sub-divides as\n> follows:\n>\n> * Patch 1: Fix bug in already-merged portion of merge-ort affecting\n> rename/rename conflicts on platforms where qsort isn't stable. (Could be\n> considered for merging before 2.31 since it is a new bug in the 2.31\n> cycle that I just learned of last night, but not sure it matters since\n> merge-ort wasn't complete anyway and we're not even mentioning merge-ort\n> in the release notes.)\n> * Patches 2-5: Add support for renormalization\n> * Patch 6: Add support for subtree shifting\n> * Patch 7-8: Add test and support for conflicts affecting sparse-checkout\n> entries\n> * Patch 9: Update submodule related merge tests to note the ones that\n> merge-ort fixes relative to merge-recursive\n> * Patch 10: New feature -- allow \"git diff AUTO_MERGE\" during conflict\n> resolution to let user review changes they made since\n> merge/rebase/cherry-pick/revert stopped and informed them of conflicts\n> * Patch 11: Add comments noting various bugs in merge-recursive\n>\n> The last two patches aren't needed by Ævar, so they could be left out and\n> submitted later. I just figured that it was only two more patches and they\n> were part of \"completing the merge-ort implementation\" in my view.\n\nThis whole thing was a pleasant read, and helped me catch a subtle\nregression in my WIP \"mode\" work (which I'm now about to submit).\n\nReviewing this series suffered from the problem you have with writing\ncode that's clearly good enough: Mostly all I had were minor nits,\nsuggestions to re-arrange code differently etc.\n\nThat being said I'm not all that familiar with the guts of the merge\nlogic, so I may have missed other issues...\n\n> [1] https://lore.kernel.org/git/877dmmkhnt.fsf@evledraar.gmail.com/ [2] See\n> https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/;\n> there are five more waiting after that -- viewable by the curious at\n> https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch\n>\n> Elijah Newren (11):\n> merge-ort: use STABLE_QSORT instead of QSORT where required\n> merge-ort: add a special minimal index just for renormalization\n> merge-ort: add a function for initializing our special attr_index\n> merge-ort: have ll_merge() calls use the attr_index for\n> renormalization\n> merge-ort: let renormalization change modify/delete into clean delete\n> merge-ort: support subtree shifting\n> t6428: new test for SKIP_WORKTREE handling and conflicts\n> merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries\n> t: mark several submodule merging tests as fixed under merge-ort\n> merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict\n> merge-recursive: add a bunch of FIXME comments documenting known bugs\n>\n> branch.c | 1 +\n> builtin/rebase.c | 1 +\n> merge-ort.c | 230 ++++++++++++++++--\n> merge-recursive.c | 37 +++\n> path.c | 1 +\n> path.h | 2 +\n> sequencer.c | 5 +\n> t/t3512-cherry-pick-submodule.sh | 9 +-\n> t/t3513-revert-submodule.sh | 7 +-\n> t/t5572-pull-submodule.sh | 9 +-\n> t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++\n> t/t6437-submodule-merge.sh | 5 +-\n> t/t6438-submodule-directory-file-conflicts.sh | 9 +-\n> 13 files changed, 449 insertions(+), 25 deletions(-)\n> create mode 100755 t/t6428-merge-conflicts-sparse.sh\n>\n>\n> base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-973%2Fnewren%2Fort-remainder-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-973/newren/ort-remainder-v1\n> Pull-Request: https://github.com/git/git/pull/973\n\n1.:\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 5d3b711fe68..4f6ead11e51 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -507,7 +507,7 @@ test_setup_2a () {\n \t)\n }\n \n-test_expect_success '2a: Directory split into two on one side, with equal numbers of paths' '\n+test_expect_merge_algorithm success failure '2a: Directory split into two on one side, with equal numbers of paths' '\n \ttest_setup_2a &&\n \t(\n \t\tcd 2a &&\n@@ -3060,7 +3060,7 @@ test_setup_9g () {\n \t)\n }\n \n-test_expect_failure '9g: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '\n+test_expect_merge_algorithm failure failure '9g: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '\n \ttest_setup_9g &&\n \t(\n \t\tcd 9g &&\n@@ -4267,7 +4267,7 @@ test_setup_12b1 () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12b1: Moving two directory hierarchies into each other' '\n+test_expect_merge_algorithm failure failure '12b1: Moving two directory hierarchies into each other' '\n \ttest_setup_12b1 &&\n \t(\n \t\tcd 12b1 &&\n@@ -4435,7 +4435,7 @@ test_setup_12c1 () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12c1: Moving one directory hierarchy into another w/ content merge' '\n+test_expect_merge_algorithm failure failure '12c1: Moving one directory hierarchy into another w/ content merge' '\n \ttest_setup_12c1 &&\n \t(\n \t\tcd 12c1 &&\n@@ -4797,7 +4797,7 @@ test_setup_12f () {\n \t)\n }\n \n-test_expect_merge_algorithm failure success '12f: Trivial directory resolve, caching, all kinds of fun' '\n+test_expect_merge_algorithm failure failure '12f: Trivial directory resolve, caching, all kinds of fun' '\n \ttest_setup_12f &&\n \t(\n \t\tcd 12f &&\n"},{"id":"418530","messageId":"CABPp-BEZpq7yX0vOamXhDa3M6696YqrD_-UZE2GfFFJrORDdMQ@mail.gmail.com","threadId":"55259","inReplyTo":"874khlkdb0.fsf@evledraar.gmail.com","subject":"Re: [PATCH 07/11] t6428: new test for SKIP_WORKTREE handling and conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-08T20:52:10Z","receivedAt":"2021-03-08T20:53:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 5:03 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > If there is a conflict during a merge for a SKIP_WORKTREE entry, we\n> > expect that file to be written to the working copy and have the\n> > SKIP_WORKTREE bit cleared in the index. If the user had manually\n> > created a file in the working tree despite SKIP_WORKTREE being set, we\n> > do not want to clobber their changes to that file, but want to move it\n> > out of the way. Add tests that check for these behaviors.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++\n> > 1 file changed, 158 insertions(+)\n> > create mode 100755 t/t6428-merge-conflicts-sparse.sh\n> >\n> > diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\n> > new file mode 100755\n> > index 000000000000..1bb52ff6f38c\n> > --- /dev/null\n> > +++ b/t/t6428-merge-conflicts-sparse.sh\n> > @@ -0,0 +1,158 @@\n> > +#!/bin/sh\n> > +\n> > +test_description=\"merge cases\"\n> > +\n> > +# The setup for all of them, pictorially, is:\n> > +#\n> > +# A\n> > +# o\n> > +# / \\\n> > +# O o ?\n> > +# \\ /\n> > +# o\n> > +# B\n> > +#\n> > +# To help make it easier to follow the flow of tests, they have been\n> > +# divided into sections and each test will start with a quick explanation\n> > +# of what commits O, A, and B contain.\n> > +#\n> > +# Notation:\n> > +# z/{b,c} means files z/b and z/c both exist\n> > +# x/d_1 means file x/d exists with content d1. (Purpose of the\n> > +# underscore notation is to differentiate different\n> > +# files that might be renamed into each other's paths.)\n> > +\n> > +. ./test-lib.sh\n> > +. \"$TEST_DIRECTORY\"/lib-merge.sh\n> > +\n> > +\n> > +# Testcase basic, conflicting changes in 'numerals'\n> > +\n> > +test_setup_numerals () {\n> > + test_create_repo numerals_$1 &&\n> > + (\n> > + cd numerals_$1 &&\n> > +\n> > + >README &&\n> > + test_write_lines I II III >numerals &&\n> > + git add README numerals &&\n> > + test_tick &&\n> > + git commit -m \"O\" &&\n>\n> As an aside this could use the --printf option to test_commit I've got\n> in next, but that's also a bit painful to use since you can't use\n> test_write_lines.\n>\n> I've wanted to just support something like this for this use-case of\n> using an existing file:\n>\n> test_write_lines A B C D >lines &&\n> test_commit --add O lines &&\n>\n>\n> > +\n> > + git branch O &&\n> > + git branch A &&\n> > + git branch B &&\n> > +\n> > + git checkout A &&\n> > + test_write_lines I II III IIII >numerals &&\n> > + git add numerals &&\n> > + test_tick &&\n> > + git commit -m \"A\" &&\n> > +\n> > + git checkout B &&\n> > + test_write_lines I II III IV >numerals &&\n> > + git add numerals &&\n> > + test_tick &&\n> > + git commit -m \"B\" &&\n> > +\n> > + cat <<-EOF >expected-index &&\n> > + H README\n> > + M numerals\n> > + M numerals\n> > + M numerals\n> > + EOF\n> > +\n> > + cat <<-EOF >expected-merge\n> > + I\n> > + II\n> > + III\n> > + <<<<<<< HEAD\n> > + IIII\n> > + =======\n> > + IV\n> > + >>>>>>> B^0\n> > + EOF\n> > +\n> > + )\n> > +}\n> > +\n> > +test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n> > + test_setup_numerals plain &&\n>\n> A small nit, but makes it easier to debug things: I think having what\n> you have in \"test_setup_numerals\" above in a test_expect_success is a\n> better pattern, then if it fails we can see where exactly.\n>\n> Then instead of calling \"test_setup_numerals\" here you'd do:\n>\n> cp -R template plain &&\n>\n> To just copy over that existing setup template, or re-use it and have\n> have the tests call a small helper to \"test_when_finish\" reset --hard\n> back as appropriate.\n\nI used to have tests follow that pattern, but Dscho objected\nstrenuously to it; see e.g.\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.1910141211130.46@tvgsbejvaqbjf.bet/\n\nI went through and replaced most of the merge-recursive-related ones\nto the current style to help him out.\n"},{"id":"418531","messageId":"CABPp-BE1hyWsTyFDZTtHrsbCSx7TQcLb0WwfYVw2mHVYj7_jLQ@mail.gmail.com","threadId":"55259","inReplyTo":"871rcpkd6v.fsf@evledraar.gmail.com","subject":"Re: [PATCH 08/11] merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-08T20:54:57Z","receivedAt":"2021-03-08T20:55:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 5:06 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > When merge conflicts occur in paths removed by a sparse-checkout, we\n> > need to unsparsify those paths (clear the SKIP_WORKTREE bit), and write\n> > out the conflicted file to the working copy. In the very unlikely case\n> > that someone manually put a file into the working copy at the location\n> > of the SKIP_WORKTREE file, we need to avoid overwriting whatever edits\n> > they have made and move that file to a different location first.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 43 +++++++++++++++++++++----------\n> > t/t6428-merge-conflicts-sparse.sh | 4 +--\n> > 2 files changed, 32 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index a998f843a1da..37b69cbe0f9a 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3235,23 +3235,27 @@ static int checkout(struct merge_options *opt,\n> > return ret;\n> > }\n> >\n> > -static int record_conflicted_index_entries(struct merge_options *opt,\n> > - struct index_state *index,\n> > - struct strmap *paths,\n> > - struct strmap *conflicted)\n> > +static int record_conflicted_index_entries(struct merge_options *opt)\n> > {\n> > struct hashmap_iter iter;\n> > struct strmap_entry *e;\n> > + struct index_state *index = opt->repo->index;\n> > + struct checkout state = CHECKOUT_INIT;\n> > int errs = 0;\n> > int original_cache_nr;\n> >\n> > - if (strmap_empty(conflicted))\n> > + if (strmap_empty(&opt->priv->conflicted))\n> > return 0;\n> >\n> > + /* If any entries have skip_worktree set, we'll have to check 'em out */\n> > + state.force = 1;\n> > + state.quiet = 1;\n> > + state.refresh_cache = 1;\n> > + state.istate = index;\n> > original_cache_nr = index->cache_nr;\n> >\n> > /* Put every entry from paths into plist, then sort */\n> > - strmap_for_each_entry(conflicted, &iter, e) {\n> > + strmap_for_each_entry(&opt->priv->conflicted, &iter, e) {\n> > const char *path = e->key;\n> > struct conflict_info *ci = e->value;\n> > int pos;\n> > @@ -3292,9 +3296,23 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n> > * the higher order stages. Thus, we need override\n> > * the CE_SKIP_WORKTREE bit and manually write those\n> > * files to the working disk here.\n> > - *\n> > - * TODO: Implement this CE_SKIP_WORKTREE fixup.\n> > */\n> > + if (ce_skip_worktree(ce)) {\n> > + struct stat st;\n> > +\n> > + if (!lstat(path, &st)) {\n> > + char *new_name = unique_path(&opt->priv->paths,\n> > + path,\n> > + \"cruft\");\n> > +\n> > + path_msg(opt, path, 1,\n> > + _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n> > + path, new_name);\n>\n> I see this follows existing uses in merge-ort.c, but I wonder if this\n> won't be quite unreadable on long paths, i.e.:\n>\n> renamed to \n>\n> As opposed to:\n>\n> We had to rename your thing:\n> from: \n> to: \n\nMakes sense, but it seems like something we'd want to do to a lot of\nmessages rather than just this one. For now, especially given that I\nexpect this particular message to be *very* rare, I think I'll leave\nthis one as-is for now but we can address this idea in a subsequent\nseries or as #leftoverbits.\n"},{"id":"418539","messageId":"CABPp-BFr5_EurX2D1Yo7UAJNnLLFauqE5FFfULjGjtzzG9UJZQ@mail.gmail.com","threadId":"55259","inReplyTo":"87v9a1iycx.fsf@evledraar.gmail.com","subject":"Re: [PATCH 10/11] merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-08T21:51:42Z","receivedAt":"2021-03-08T21:52:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 5:11 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > There are a variety of questions users might ask while resolving\n> > conflicts:\n> > * What changes have been made since the previous (first) parent?\n> > * What changes are staged?\n> > * What is still unstaged? (or what is still conflicted?)\n> > * What changes did I make to resolve conflicts so far?\n> > The first three of these have simple answers:\n> > * git diff HEAD\n> > * git diff --cached\n> > * git diff\n> > There was no way to answer the final question previously. Adding one\n> > is trivial in merge-ort, since it works by creating a tree representing\n> > what should be written to the working copy complete with conflict\n> > markers. Simply write that tree to .git/AUTO_MERGE, allowing users to\n> > answer the fourth question with\n> > * git diff AUTO_MERGE\n> >\n> > I avoided using a name like \"MERGE_AUTO\", because that would be\n> > merge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD,\n> > CHERRY_PICK_HEAD) and I wanted a name that didn't change depending on\n> > which type of operation the merge was part of.\n>\n> That's a really cool feature. I'm starting to like this \"ort\" thing :)\n>\n> (After knowing almost nothing about it until a few days ago...)\n>\n> > Ensure that paths which clean out other temporary operation-specific\n> > files (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory)\n> > also clean out this AUTO_MERGE file.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > branch.c | 1 +\n> > builtin/rebase.c | 1 +\n> > merge-ort.c | 10 ++++++++++\n> > path.c | 1 +\n> > path.h | 2 ++\n> > sequencer.c | 5 +++++\n> > 6 files changed, 20 insertions(+)\n> >\n> > diff --git a/branch.c b/branch.c\n> > index 9c9dae1eae32..b71a2de29dbe 100644\n> > --- a/branch.c\n> > +++ b/branch.c\n> > @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)\n> > unlink(git_path_merge_rr(r));\n> > unlink(git_path_merge_msg(r));\n> > unlink(git_path_merge_mode(r));\n> > + unlink(git_path_auto_merge(r));\n> > save_autostash(git_path_merge_autostash(r));\n> > }\n> >\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index de400f9a1973..7d9afe118fd4 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -739,6 +739,7 @@ static int finish_rebase(struct rebase_options *opts)\n> > int ret = 0;\n> >\n> > delete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n> > + unlink(git_path_auto_merge(the_repository));\n> > apply_autostash(state_dir_path(\"autostash\", opts));\n> > close_object_store(the_repository->objects);\n> > /*\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 37b69cbe0f9a..cf927cd160e1 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3362,6 +3362,9 @@ void merge_switch_to_result(struct merge_options *opt,\n> > {\n> > assert(opt->priv == NULL);\n> > if (result->clean >= 0 && update_worktree_and_index) {\n> > + const char *filename;\n> > + FILE *fp;\n> > +\n> > trace2_region_enter(\"merge\", \"checkout\", opt->repo);\n> > if (checkout(opt, head, result->tree)) {\n> > /* failure to function */\n> > @@ -3380,6 +3383,13 @@ void merge_switch_to_result(struct merge_options *opt,\n> > }\n> > opt->priv = NULL;\n> > trace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n> > +\n> > + trace2_region_enter(\"merge\", \"write_auto_merge\", opt->repo);\n> > + filename = git_path_auto_merge(opt->repo);\n> > + fp = xfopen(filename, \"w\");\n> > + fprintf(fp, \"%s\\n\", oid_to_hex(&result->tree->object.oid));\n> > + fclose(fp);\n> > + trace2_region_leave(\"merge\", \"write_auto_merge\", opt->repo);\n>\n> This isn't a new problem since you're just folling an existing pattern,\n> but here you (rightly) do xopen()< and the:n\n\nLooks like your comment got garbled/truncated. Do you remember the\nrest of what you were going to say here?\n\n> > }\n> >\n> > if (display_update_msgs) {\n> > diff --git a/path.c b/path.c\n> > index 7b385e5eb282..9e883eb52446 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -1534,5 +1534,6 @@ REPO_GIT_PATH_FUNC(merge_rr, \"MERGE_RR\")\n> > REPO_GIT_PATH_FUNC(merge_mode, \"MERGE_MODE\")\n> > REPO_GIT_PATH_FUNC(merge_head, \"MERGE_HEAD\")\n> > REPO_GIT_PATH_FUNC(merge_autostash, \"MERGE_AUTOSTASH\")\n> > +REPO_GIT_PATH_FUNC(auto_merge, \"AUTO_MERGE\")\n> > REPO_GIT_PATH_FUNC(fetch_head, \"FETCH_HEAD\")\n> > REPO_GIT_PATH_FUNC(shallow, \"shallow\")\n> > diff --git a/path.h b/path.h\n> > index e7e77da6aaa5..251c78d98000 100644\n> > --- a/path.h\n> > +++ b/path.h\n> > @@ -176,6 +176,7 @@ struct path_cache {\n> > const char *merge_mode;\n> > const char *merge_head;\n> > const char *merge_autostash;\n> > + const char *auto_merge;\n> > const char *fetch_head;\n> > const char *shallow;\n> > };\n> > @@ -191,6 +192,7 @@ const char *git_path_merge_rr(struct repository *r);\n> > const char *git_path_merge_mode(struct repository *r);\n> > const char *git_path_merge_head(struct repository *r);\n> > const char *git_path_merge_autostash(struct repository *r);\n> > +const char *git_path_auto_merge(struct repository *r);\n> > const char *git_path_fetch_head(struct repository *r);\n> > const char *git_path_shallow(struct repository *r);\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index d2332d3e1787..472cdd8c620d 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -2096,6 +2096,7 @@ static int do_pick_commit(struct repository *r,\n> > refs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> > NULL, 0);\n> > unlink(git_path_merge_msg(r));\n> > + unlink(git_path_auto_merge(r));\n>\n> Shouldn't this & the rest ideally be at least unlink_or_warn()?\n\nPerhaps, but I think that should be a follow-on series or\n#leftoverbits. I'm having enough trouble getting reviews (I think I'm\nburning Stolee out after the last half year) without making my series\nlonger for tangential cleanups. :-)\n\nI'm starting to worry that despite having all the remaining patches\nready (and most have been ready for months), that we won't be able to\nget merge-ort done before git-2.32 -- the -rc0 for it is now just\nunder three months away.\n\n> > fprintf(stderr,\n> > _(\"dropping %s %s -- patch contents already upstream\\n\"),\n> > oid_to_hex(&commit->object.oid), msg.subject);\n> > @@ -2451,6 +2452,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n> > need_cleanup = 1;\n> > }\n> >\n> > + unlink(git_path_auto_merge(r));\n> > +\n> > if (!need_cleanup)\n> > return;\n> >\n> > @@ -4111,6 +4114,7 @@ static int pick_commits(struct repository *r,\n> > unlink(rebase_path_stopped_sha());\n> > unlink(rebase_path_amend());\n> > unlink(git_path_merge_head(r));\n> > + unlink(git_path_auto_merge(r));\n> > delete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n> >\n> > if (item->command == TODO_BREAK) {\n> > @@ -4505,6 +4509,7 @@ static int commit_staged_changes(struct repository *r,\n> > return error(_(\"could not commit staged changes.\"));\n> > unlink(rebase_path_amend());\n> > unlink(git_path_merge_head(r));\n> > + unlink(git_path_auto_merge(r));\n> > if (final_fixup) {\n> > unlink(rebase_path_fixup_msg());\n> > unlink(rebase_path_squash_msg());\n"},{"id":"418542","messageId":"CABPp-BHeR6m4-M=nSX5NZtA2js3E3EVbAyDSMtp3-rN2QnUjqw@mail.gmail.com","threadId":"55259","inReplyTo":"87pn09iu3j.fsf@evledraar.gmail.com","subject":"Re: [PATCH 00/11] Complete merge-ort implementation...almost","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-08T22:13:53Z","receivedAt":"2021-03-08T22:14:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 6:43 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > In order to help Ævar test his tree-walk changes against merge-ort[1], this\n> > series completes the merge-ort implementation and cleans up testsuite\n> > failures...EXCEPT for some t6423 failures. It also leaves out a lot of\n> > performance work, which incidentally will fix the t6423 failures and is\n> > being reviewed independently[2].\n>\n> For those testing this in combination with their work, the expected\n> failures are these specific tests:\n>\n> ./t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)\n> Failed tests: 7, 53, 55, 59\n>\n> Perhaps I'm missing something but why not have [1] below on top of this\n> series? It makes that test pass in both modes, and means we could\n> e.g. follow-up by running CI with \"ort\".\n>\n> That CI step seems worth doing sooner than later, even if it needs some\n> GIT_TEST_SKIP for now...\n\nOh, um, mostly it was history and the fact that I just never took a\nstep back. When I started adding merge-ort, there were 2200+ tests\nthat were failing, and marking them all as test_expect_fail was just a\nuseless code churn; we knew it'd fail when the implemenation basically\njust called die(). At some point I got it down to 8 tests failing,\nbut felt I needed to quickly switch tracks to upstream the\ndiffcore-rename work since we had an Outreachy intern that was going\nto be buildiing on it and/or making changes that might conflict with\nit. I was still in the mode of fixing them through completing the\nimplementation rather than stepping back and saying, \"Hey, we only\nhave a few tests left failing; let's mark them as such.\".\n\nHowever, all that said, making this change _now_ would semantically\nconflict with ort-perf-batch-9[1] (which was submitted first), since\nit fixes one of the four tests. It'd also mean a similar semantic\nconflict for ort-perf-batch-10 (which fixes another) and\nort-perf-batch-12 (which fixes the last 2), or those series will have\nto depend on a merge of ort-perf-batch-9 and this series. Relying on\nspecial merges for some of the earlier patches for merge-ort seems\nlike it was a bit of a pain and I'd rather not put Junio through that.\nAlso, if we do it, and don't have Junio mark the fixes as things are\nmerged, then we have to decide on a topic order. So, Junio: thoughts?\n\n[1] https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\n\n> > This 11-patch series could be submitted as 7 independent series, 1-4 patches\n> > in length each, but it's probably easier for Ævar if we can merge just one\n> > more thing and it's only 11 total patches. This series sub-divides as\n> > follows:\n> >\n> > * Patch 1: Fix bug in already-merged portion of merge-ort affecting\n> > rename/rename conflicts on platforms where qsort isn't stable. (Could be\n> > considered for merging before 2.31 since it is a new bug in the 2.31\n> > cycle that I just learned of last night, but not sure it matters since\n> > merge-ort wasn't complete anyway and we're not even mentioning merge-ort\n> > in the release notes.)\n> > * Patches 2-5: Add support for renormalization\n> > * Patch 6: Add support for subtree shifting\n> > * Patch 7-8: Add test and support for conflicts affecting sparse-checkout\n> > entries\n> > * Patch 9: Update submodule related merge tests to note the ones that\n> > merge-ort fixes relative to merge-recursive\n> > * Patch 10: New feature -- allow \"git diff AUTO_MERGE\" during conflict\n> > resolution to let user review changes they made since\n> > merge/rebase/cherry-pick/revert stopped and informed them of conflicts\n> > * Patch 11: Add comments noting various bugs in merge-recursive\n> >\n> > The last two patches aren't needed by Ævar, so they could be left out and\n> > submitted later. I just figured that it was only two more patches and they\n> > were part of \"completing the merge-ort implementation\" in my view.\n>\n> This whole thing was a pleasant read, and helped me catch a subtle\n> regression in my WIP \"mode\" work (which I'm now about to submit).\n\nWahoo for helping catch regressions! :-)\n\n> Reviewing this series suffered from the problem you have with writing\n> code that's clearly good enough: Mostly all I had were minor nits,\n> suggestions to re-arrange code differently etc.\n\nThanks for taking a look. You had lots of good small suggestions that\nI'll incorporate in a re-roll. There were a couple that sounded like\nthey should be split off into their own sets of cleanup patches that\nI'll defer for now.\n\n> That being said I'm not all that familiar with the guts of the merge\n> logic, so I may have missed other issues...\n\n>\n> > [1] https://lore.kernel.org/git/877dmmkhnt.fsf@evledraar.gmail.com/ [2] See\n> > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/;\n> > there are five more waiting after that -- viewable by the curious at\n> > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch\n> >\n> > Elijah Newren (11):\n> > merge-ort: use STABLE_QSORT instead of QSORT where required\n> > merge-ort: add a special minimal index just for renormalization\n> > merge-ort: add a function for initializing our special attr_index\n> > merge-ort: have ll_merge() calls use the attr_index for\n> > renormalization\n> > merge-ort: let renormalization change modify/delete into clean delete\n> > merge-ort: support subtree shifting\n> > t6428: new test for SKIP_WORKTREE handling and conflicts\n> > merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries\n> > t: mark several submodule merging tests as fixed under merge-ort\n> > merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict\n> > merge-recursive: add a bunch of FIXME comments documenting known bugs\n> >\n> > branch.c | 1 +\n> > builtin/rebase.c | 1 +\n> > merge-ort.c | 230 ++++++++++++++++--\n> > merge-recursive.c | 37 +++\n> > path.c | 1 +\n> > path.h | 2 +\n> > sequencer.c | 5 +\n> > t/t3512-cherry-pick-submodule.sh | 9 +-\n> > t/t3513-revert-submodule.sh | 7 +-\n> > t/t5572-pull-submodule.sh | 9 +-\n> > t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++\n> > t/t6437-submodule-merge.sh | 5 +-\n> > t/t6438-submodule-directory-file-conflicts.sh | 9 +-\n> > 13 files changed, 449 insertions(+), 25 deletions(-)\n> > create mode 100755 t/t6428-merge-conflicts-sparse.sh\n> >\n> >\n> > base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-973%2Fnewren%2Fort-remainder-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-973/newren/ort-remainder-v1\n> > Pull-Request: https://github.com/git/git/pull/973\n>\n> 1.:\n>\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 5d3b711fe68..4f6ead11e51 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -507,7 +507,7 @@ test_setup_2a () {\n> )\n> }\n>\n> -test_expect_success '2a: Directory split into two on one side, with equal numbers of paths' '\n> +test_expect_merge_algorithm success failure '2a: Directory split into two on one side, with equal numbers of paths' '\n> test_setup_2a &&\n> (\n> cd 2a &&\n> @@ -3060,7 +3060,7 @@ test_setup_9g () {\n> )\n> }\n>\n> -test_expect_failure '9g: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '\n> +test_expect_merge_algorithm failure failure '9g: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '\n> test_setup_9g &&\n> (\n> cd 9g &&\n> @@ -4267,7 +4267,7 @@ test_setup_12b1 () {\n> )\n> }\n>\n> -test_expect_merge_algorithm failure success '12b1: Moving two directory hierarchies into each other' '\n> +test_expect_merge_algorithm failure failure '12b1: Moving two directory hierarchies into each other' '\n> test_setup_12b1 &&\n> (\n> cd 12b1 &&\n> @@ -4435,7 +4435,7 @@ test_setup_12c1 () {\n> )\n> }\n>\n> -test_expect_merge_algorithm failure success '12c1: Moving one directory hierarchy into another w/ content merge' '\n> +test_expect_merge_algorithm failure failure '12c1: Moving one directory hierarchy into another w/ content merge' '\n> test_setup_12c1 &&\n> (\n> cd 12c1 &&\n> @@ -4797,7 +4797,7 @@ test_setup_12f () {\n> )\n> }\n>\n> -test_expect_merge_algorithm failure success '12f: Trivial directory resolve, caching, all kinds of fun' '\n> +test_expect_merge_algorithm failure failure '12f: Trivial directory resolve, caching, all kinds of fun' '\n> test_setup_12f &&\n> (\n> cd 12f &&\n"},{"id":"418566","messageId":"CABPp-BEMAbeQAcja=MFU3_N7dUOpKWCohtyNfnTj9npep8j1Lw@mail.gmail.com","threadId":"55259","inReplyTo":"87a6rdkdxq.fsf@evledraar.gmail.com","subject":"Re: [PATCH 04/11] merge-ort: have ll_merge() calls use the attr_index for renormalization","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-09T04:27:35Z","receivedAt":"2021-03-09T04:28:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 4:49 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > ll_merge() needs an index when renormalization is requested. Give it\n> > the special one we created exactly for that purpose. This fixes t6418.4\n> > and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 7 ++++---\n> > 1 file changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 028d1adcd2c9..87c553c0882c 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -360,7 +360,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n> > string_list_clear(&opti->paths_to_free, 0);\n> > opti->paths_to_free.strdup_strings = 0;\n> >\n> > - if (opti->attr_index.cache_nr)\n> > + if (opti->attr_index.cache_nr) /* true iff opt->renormalize */\n> > discard_index(&opti->attr_index);\n>\n> Perhaps instead of a comment, in that \"if\":\n>\n> assert(opt->renormalize);\n\nI would, but opt isn't defined here, and passing it in merely for an\nassertion seems overboard.\n\n> > /* Free memory used by various renames maps */\n> > @@ -988,7 +988,6 @@ static int merge_submodule(struct merge_options *opt,\n> > return 0;\n> > }\n> >\n> > -MAYBE_UNUSED\n> > static void initialize_attr_index(struct merge_options *opt)\n> > {\n> > /*\n> > @@ -1063,6 +1062,8 @@ static int merge_3way(struct merge_options *opt,\n> > char *base, *name1, *name2;\n> > int merge_status;\n> >\n> > + initialize_attr_index(opt);\n>\n> Subjective, but I think it's more readable to move the \"initialized\"\n> check in initialize_attr_index() here, so:\n>\n> if (!attr_index->initialized)\n> initialize_attr_index(opt);\n>\n> Saves the reader a trip to the function to see that it doesn't do\n> anything except exit early on that flag.\n\nMakes sense; I'll fix this up.\n\n>\n> > ll_opts.renormalize = opt->renormalize;\n> > ll_opts.extra_marker_size = extra_marker_size;\n> > ll_opts.xdl_opts = opt->xdl_opts;\n> > @@ -1101,7 +1102,7 @@ static int merge_3way(struct merge_options *opt,\n> >\n> > merge_status = ll_merge(result_buf, path, &orig, base,\n> > &src1, name1, &src2, name2,\n> > - opt->repo->index, &ll_opts);\n> > + &opt->priv->attr_index, &ll_opts);\n> >\n> > free(base);\n> > free(name1);\n>\n"},{"id":"418568","messageId":"3ca16a5e3466914c1c7efa79843395a4dfc3c3f1.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 01/10] merge-ort: use STABLE_QSORT instead of QSORT where required","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:36Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nrename/rename conflict handling depends on the fact that if both sides\nrenamed the same path, that the one on the MERGE_SIDE1 will appear first\nin the combined diff_queue_struct passed to process_renames(). Since we\nadd all pairs from MERGE_SIDE1 to combined first, and then all pairs\nfrom MERGE_SIDE2, and then sort based on filename, this will only be\ntrue if the sort used is stable. This was found due to the fact that\nMac, unlike Linux, apparently has a system-defined qsort that is not\nstable.\n\nWhile we are at it, review the other callers of QSORT and add comments\nabout why they can remain as calls to QSORT instead of being modified\nto call STABLE_QSORT.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 603d30c52170..5309488fd9d8 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2289,7 +2289,7 @@ static int detect_and_process_renames(struct merge_options *opt,\n \tclean &= collect_renames(opt, &combined, MERGE_SIDE2,\n \t\t\t\t &renames->dir_renames[1],\n \t\t\t\t &renames->dir_renames[2]);\n-\tQSORT(combined.queue, combined.nr, compare_pairs);\n+\tSTABLE_QSORT(combined.queue, combined.nr, compare_pairs);\n \ttrace2_region_leave(\"merge\", \"directory renames\", opt->repo);\n \n \ttrace2_region_enter(\"merge\", \"process renames\", opt->repo);\n@@ -2415,6 +2415,7 @@ static void write_tree(struct object_id *result_oid,\n \t */\n \trelevant_entries.items = versions->items + offset;\n \trelevant_entries.nr = versions->nr - offset;\n+\t/* No need for STABLE_QSORT -- filenames must be unique */\n \tQSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);\n \n \t/* Pre-allocate some space in buf */\n@@ -3190,6 +3191,11 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n \t * entries we added to the end into their right locations.\n \t */\n \tremove_marked_cache_entries(index, 1);\n+\t/*\n+\t * No need for STABLE_QSORT -- cmp_cache_name_compare sorts primarily\n+\t * on filename and secondarily on stage, and (name, stage #) are a\n+\t * unique tuple.\n+\t */\n \tQSORT(index->cache, index->cache_nr, cmp_cache_name_compare);\n \n \treturn errs;\n-- \ngitgitgadget\n\n"},{"id":"418569","messageId":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.git.git.1614905738.gitgitgadget@gmail.com","subject":"[PATCH v2 00/10] Complete merge-ort implementation...almost","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:35Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series (nearly) completes the merge-ort implementation, cleaning up\ntestsuite failures. The exceptions are some t6423 failures being addressed\nindependently[1].\n\nChanges since v1, to address feedback from Ævar:\n\n * Squashed patches 3 & 4\n * Various style fixes\n * Check attr_index->initialized before calling initialize_attr_index()\n instead of inside initialize_attr_index()\n * Simplify GIT_TEST_MERGE_ALGORITHM comparison to only have if-then block\n\nStuff not included in v2:\n\n * Ævar suggested patching the test expectation on the 4 known failing tests\n so that all tests passed under GIT_TEST_MERGE_ALGORITHM=ort. Seems\n reasonable, but the semantic conflicts with other series might make it\n more trouble than it's worth and other series will fix all 4 tests.\n Leaving as-is for now to avoid putting more burden on Junio; see\n https://lore.kernel.org/git/CABPp-BHeR6m4-M=nSX5NZtA2js3E3EVbAyDSMtp3-rN2QnUjqw@mail.gmail.com/\n * Ævar noted a few bigger cleanups to surrounding code that could also be\n done, even if orthogonal to this series. I'll leave those for other\n series to address.\n\n[1] See\nhttps://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/\nand batch 10 and 12 from\nhttps://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch\n\nElijah Newren (10):\n merge-ort: use STABLE_QSORT instead of QSORT where required\n merge-ort: add a special minimal index just for renormalization\n merge-ort: have ll_merge() use a special attr_index for\n renormalization\n merge-ort: let renormalization change modify/delete into clean delete\n merge-ort: support subtree shifting\n t6428: new test for SKIP_WORKTREE handling and conflicts\n merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries\n t: mark several submodule merging tests as fixed under merge-ort\n merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict\n merge-recursive: add a bunch of FIXME comments documenting known bugs\n\n branch.c | 1 +\n builtin/rebase.c | 1 +\n merge-ort.c | 229 ++++++++++++++++--\n merge-recursive.c | 37 +++\n path.c | 1 +\n path.h | 2 +\n sequencer.c | 5 +\n t/t3512-cherry-pick-submodule.sh | 7 +-\n t/t3513-revert-submodule.sh | 5 +-\n t/t5572-pull-submodule.sh | 7 +-\n t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++\n t/t6437-submodule-merge.sh | 5 +-\n t/t6438-submodule-directory-file-conflicts.sh | 7 +-\n 13 files changed, 440 insertions(+), 25 deletions(-)\n create mode 100755 t/t6428-merge-conflicts-sparse.sh\n\n\nbase-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-973%2Fnewren%2Fort-remainder-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-973/newren/ort-remainder-v2\nPull-Request: https://github.com/git/git/pull/973\n\nRange-diff vs v1:\n\n 1: 3ca16a5e3466 = 1: 3ca16a5e3466 merge-ort: use STABLE_QSORT instead of QSORT where required\n 2: 24454e67b186 = 2: 24454e67b186 merge-ort: add a special minimal index just for renormalization\n 3: 815af5d30ebd ! 3: 386cb3230b67 merge-ort: add a function for initializing our special attr_index\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - merge-ort: add a function for initializing our special attr_index\n + merge-ort: have ll_merge() use a special attr_index for renormalization\n \n - Add a function which can be called to populate the attr_index with the\n - appropriate .gitattributes contents when necessary. Make it return\n - early if the attr_index is already initialized or if we are not\n - renormalizing files.\n + ll_merge() needs an index when renormalization is requested. Create one\n + specifically for just this purpose with just the one needed entry. This\n + fixes t6418.4 and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n \n NOTE 1: Even if the user has a working copy or a real index (which is\n not a given as merge-ort can be used in bare repositories), we\n @@ Commit message\n Signed-off-by: Elijah Newren \n \n ## merge-ort.c ##\n +@@ merge-ort.c: static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n + \tstring_list_clear(&opti->paths_to_free, 0);\n + \topti->paths_to_free.strdup_strings = 0;\n + \n +-\tif (opti->attr_index.cache_nr)\n ++\tif (opti->attr_index.cache_nr) /* true iff opt->renormalize */\n + \t\tdiscard_index(&opti->attr_index);\n + \n + \t/* Free memory used by various renames maps */\n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n \treturn 0;\n }\n \n -+MAYBE_UNUSED\n +static void initialize_attr_index(struct merge_options *opt)\n +{\n +\t/*\n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n +\tstruct index_state *attr_index = &opt->priv->attr_index;\n +\tstruct cache_entry *ce;\n +\n -+\tif (!opt->renormalize)\n -+\t\treturn;\n ++\tattr_index->initialized = 1;\n +\n -+\tif (attr_index->initialized)\n ++\tif (!opt->renormalize)\n +\t\treturn;\n -+\tattr_index->initialized = 1;\n +\n +\tmi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);\n +\tif (!mi)\n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n +\t\tadd_index_entry(attr_index, ce,\n +\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n +\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE, &ce->oid);\n -+\t}\n -+\telse {\n ++\t} else {\n +\t\tint stage, len;\n +\t\tstruct conflict_info *ci;\n +\n +\t\tASSIGN_AND_VERIFY_CI(ci, mi);\n -+\t\tfor (stage=0; stage<3; ++stage) {\n ++\t\tfor (stage = 0; stage < 3; stage++) {\n +\t\t\tunsigned stage_mask = (1 << stage);\n +\n +\t\t\tif (!(ci->filemask & stage_mask))\n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n static int merge_3way(struct merge_options *opt,\n \t\t const char *path,\n \t\t const struct object_id *o,\n +@@ merge-ort.c: static int merge_3way(struct merge_options *opt,\n + \tchar *base, *name1, *name2;\n + \tint merge_status;\n + \n ++\tif (!opt->priv->attr_index.initialized)\n ++\t\tinitialize_attr_index(opt);\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 +@@ merge-ort.c: static int merge_3way(struct merge_options *opt,\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 ++\t\t\t\t&opt->priv->attr_index, &ll_opts);\n + \n + \tfree(base);\n + \tfree(name1);\n 4: cb035ac5fe4a < -: ------------ merge-ort: have ll_merge() calls use the attr_index for renormalization\n 5: b70ef4d7000a ! 4: 7fcabded5016 merge-ort: let renormalization change modify/delete into clean delete\n @@ merge-ort.c: static int string_list_df_name_compare(const char *one, const char\n +\tint ret = 0; /* assume changed for safety */\n +\tconst struct index_state *idx = &opt->priv->attr_index;\n +\n -+\tinitialize_attr_index(opt);\n ++\tif (!idx->initialized)\n ++\t\tinitialize_attr_index(opt);\n +\n +\tif (base->mode != side->mode)\n +\t\treturn 0;\n 6: d04ddabde124 = 5: e21eea71e707 merge-ort: support subtree shifting\n 7: 6ccb24b557fc = 6: d1d8c017b23f t6428: new test for SKIP_WORKTREE handling and conflicts\n 8: 100c0187bdfe = 7: 90a57ade629d merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries\n 9: 95a6c0abe558 ! 8: fcce88c5ac3d t: mark several submodule merging tests as fixed under merge-ort\n @@ t/t3512-cherry-pick-submodule.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n -+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n ++if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n +then\n -+\t: # No special additional KNOWN_FAILURE knobs to set\n -+else\n +\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n +\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n +fi\n @@ t/t3513-revert-submodule.sh: git_revert () {\n }\n \n -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n -+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n ++if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n +then\n -+\t: # No special additional KNOWN_FAILURE knobs to set\n -+else\n +\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n +fi\n test_submodule_switch_func \"git_revert\"\n @@ t/t5572-pull-submodule.sh: git_pull_noff () {\n \n -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n -+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n ++if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n +then\n -+\t: # No special additional KNOWN_FAILURE knobs to set\n -+else\n +\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n +\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n +fi\n @@ t/t6438-submodule-directory-file-conflicts.sh: test_submodule_switch \"merge --ff\n \n -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n -+if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n ++if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n +then\n -+\t: # No special additional KNOWN_FAILURE knobs to set\n -+else\n +\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n +\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n +fi\n 10: d8c6eb39aa7c = 9: 93b241c2f213 merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict\n 11: 0409118d4ff7 = 10: a9a95bb0391f merge-recursive: add a bunch of FIXME comments documenting known bugs\n\n-- \ngitgitgadget\n"},{"id":"418570","messageId":"24454e67b186992b9ef7a8ac6f220cbb4aaafd6b.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 02/10] merge-ort: add a special minimal index just for renormalization","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:37Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nrenormalize_buffer() requires an index_state, which is something that\nmerge-ort does not operate with. However, all the renormalization code\nneeds is an index with a .gitattributes file...plus a little bit of\nsetup. Create such an index, along with the deallocation and\nattr_direction handling.\n\nA subsequent commit will add a function to finish the initialization\nof this index.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 5309488fd9d8..d91b66a052b6 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -18,6 +18,7 @@\n #include \"merge-ort.h\"\n \n #include \"alloc.h\"\n+#include \"attr.h\"\n #include \"blob.h\"\n #include \"cache-tree.h\"\n #include \"commit.h\"\n@@ -170,6 +171,16 @@ struct merge_options_internal {\n \t */\n \tstruct rename_info renames;\n \n+\t/*\n+\t * attr_index: hacky minimal index used for renormalization\n+\t *\n+\t * renormalization code _requires_ an index, though it only needs to\n+\t * find a .gitattributes file within the index. So, when\n+\t * renormalization is important, we create a special index with just\n+\t * that one file.\n+\t */\n+\tstruct index_state attr_index;\n+\n \t/*\n \t * current_dir_name, toplevel_dir: temporary vars\n \t *\n@@ -349,6 +360,9 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \tstring_list_clear(&opti->paths_to_free, 0);\n \topti->paths_to_free.strdup_strings = 0;\n \n+\tif (opti->attr_index.cache_nr)\n+\t\tdiscard_index(&opti->attr_index);\n+\n \t/* Free memory used by various renames maps */\n \tfor (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {\n \t\tstruct hashmap_iter iter;\n@@ -3272,6 +3286,8 @@ void merge_finalize(struct merge_options *opt,\n {\n \tstruct merge_options_internal *opti = result->priv;\n \n+\tif (opt->renormalize)\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \tassert(opt->priv == NULL);\n \n \tclear_or_reinit_internal_opts(opti, 0);\n@@ -3347,6 +3363,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t/* Default to histogram diff. Actually, just hardcode it...for now. */\n \topt->xdl_opts = DIFF_WITH_ALG(opt, HISTOGRAM_DIFF);\n \n+\t/* Handle attr direction stuff for renormalization */\n+\tif (opt->renormalize)\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n+\n \t/* Initialization of opt->priv, our internal merge data */\n \ttrace2_region_enter(\"merge\", \"allocate/init\", opt->repo);\n \tif (opt->priv) {\n-- \ngitgitgadget\n\n"},{"id":"418571","messageId":"e21eea71e70733a035031bd4e97957b08bc6b22a.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 05/10] merge-ort: support subtree shifting","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:40Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-recursive has some simple code to support subtree shifting; copy\nit over to merge-ort. This fixes t6409.12 under\nGIT_TEST_MERGE_ALGORITHM=ort.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex db46f496cb82..cfd956a3f435 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3416,6 +3416,23 @@ void merge_finalize(struct merge_options *opt,\n \n /*** Function Grouping: helper functions for merge_incore_*() ***/\n \n+static struct tree *shift_tree_object(struct repository *repo,\n+\t\t\t\t struct tree *one, struct tree *two,\n+\t\t\t\t const char *subtree_shift)\n+{\n+\tstruct object_id shifted;\n+\n+\tif (!*subtree_shift) {\n+\t\tshift_tree(repo, &one->object.oid, &two->object.oid, &shifted, 0);\n+\t} else {\n+\t\tshift_tree_by(repo, &one->object.oid, &two->object.oid, &shifted,\n+\t\t\t subtree_shift);\n+\t}\n+\tif (oideq(&two->object.oid, &shifted))\n+\t\treturn two;\n+\treturn lookup_tree(repo, &shifted);\n+}\n+\n static inline void set_commit_tree(struct commit *c, struct tree *t)\n {\n \tc->maybe_tree = t;\n@@ -3543,6 +3560,13 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n {\n \tstruct object_id working_tree_oid;\n \n+\tif (opt->subtree_shift) {\n+\t\tside2 = shift_tree_object(opt->repo, side1, side2,\n+\t\t\t\t\t opt->subtree_shift);\n+\t\tmerge_base = shift_tree_object(opt->repo, side1, merge_base,\n+\t\t\t\t\t opt->subtree_shift);\n+\t}\n+\n \ttrace2_region_enter(\"merge\", \"collect_merge_info\", opt->repo);\n \tif (collect_merge_info(opt, merge_base, side1, side2) != 0) {\n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"418572","messageId":"7fcabded50166523588177216ab6b8e8973d1905.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 04/10] merge-ort: let renormalization change modify/delete into clean delete","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:39Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen we have a modify/delete conflict, but the only change to the\nmodification is e.g. change of line endings, then if renormalization is\nrequested then we should be able to recognize such a case as a\nnot-modified/delete and resolve the conflict automatically.\n\nThis fixes t6418.10 under GIT_TEST_MERGE_ALGORITHM=ort.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex ea720eb51971..db46f496cb82 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2414,6 +2414,61 @@ static int string_list_df_name_compare(const char *one, const char *two)\n \treturn onelen - twolen;\n }\n \n+static int read_oid_strbuf(struct merge_options *opt,\n+\t\t\t const struct object_id *oid,\n+\t\t\t struct strbuf *dst)\n+{\n+\tvoid *buf;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tbuf = read_object_file(oid, &type, &size);\n+\tif (!buf)\n+\t\treturn err(opt, _(\"cannot read object %s\"), oid_to_hex(oid));\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn err(opt, _(\"object %s is not a blob\"), oid_to_hex(oid));\n+\t}\n+\tstrbuf_attach(dst, buf, size, size + 1);\n+\treturn 0;\n+}\n+\n+static int blob_unchanged(struct merge_options *opt,\n+\t\t\t const struct version_info *base,\n+\t\t\t const struct version_info *side,\n+\t\t\t const char *path)\n+{\n+\tstruct strbuf basebuf = STRBUF_INIT;\n+\tstruct strbuf sidebuf = STRBUF_INIT;\n+\tint ret = 0; /* assume changed for safety */\n+\tconst struct index_state *idx = &opt->priv->attr_index;\n+\n+\tif (!idx->initialized)\n+\t\tinitialize_attr_index(opt);\n+\n+\tif (base->mode != side->mode)\n+\t\treturn 0;\n+\tif (oideq(&base->oid, &side->oid))\n+\t\treturn 1;\n+\n+\tif (read_oid_strbuf(opt, &base->oid, &basebuf) ||\n+\t read_oid_strbuf(opt, &side->oid, &sidebuf))\n+\t\tgoto error_return;\n+\t/*\n+\t * Note: binary | is used so that both renormalizations are\n+\t * performed. Comparison can be skipped if both files are\n+\t * unchanged since their sha1s have already been compared.\n+\t */\n+\tif (renormalize_buffer(idx, path, basebuf.buf, basebuf.len, &basebuf) |\n+\t renormalize_buffer(idx, path, sidebuf.buf, sidebuf.len, &sidebuf))\n+\t\tret = (basebuf.len == sidebuf.len &&\n+\t\t !memcmp(basebuf.buf, sidebuf.buf, basebuf.len));\n+\n+error_return:\n+\tstrbuf_release(&basebuf);\n+\tstrbuf_release(&sidebuf);\n+\treturn ret;\n+}\n+\n struct directory_versions {\n \t/*\n \t * versions: list of (basename -> version_info)\n@@ -3001,8 +3056,13 @@ static void process_entry(struct merge_options *opt,\n \t\tmodify_branch = (side == 1) ? opt->branch1 : opt->branch2;\n \t\tdelete_branch = (side == 1) ? opt->branch2 : opt->branch1;\n \n-\t\tif (ci->path_conflict &&\n-\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n+\t\tif (opt->renormalize &&\n+\t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n+\t\t\t\t path)) {\n+\t\t\tci->merged.is_null = 1;\n+\t\t\tci->merged.clean = 1;\n+\t\t} else if (ci->path_conflict &&\n+\t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\n \t\t\t * This came from a rename/delete; no action to take,\n \t\t\t * but avoid printing \"modify/delete\" conflict notice\n-- \ngitgitgadget\n\n"},{"id":"418573","messageId":"d1d8c017b23ff227bbf4b8b500e8a5b1f2a0ebca.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 06/10] t6428: new test for SKIP_WORKTREE handling and conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:41Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf there is a conflict during a merge for a SKIP_WORKTREE entry, we\nexpect that file to be written to the working copy and have the\nSKIP_WORKTREE bit cleared in the index. If the user had manually\ncreated a file in the working tree despite SKIP_WORKTREE being set, we\ndo not want to clobber their changes to that file, but want to move it\nout of the way. Add tests that check for these behaviors.\n\nSigned-off-by: Elijah Newren \n---\n t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++++++++++++++++++++\n 1 file changed, 158 insertions(+)\n create mode 100755 t/t6428-merge-conflicts-sparse.sh\n\ndiff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\nnew file mode 100755\nindex 000000000000..1bb52ff6f38c\n--- /dev/null\n+++ b/t/t6428-merge-conflicts-sparse.sh\n@@ -0,0 +1,158 @@\n+#!/bin/sh\n+\n+test_description=\"merge cases\"\n+\n+# The setup for all of them, pictorially, is:\n+#\n+# A\n+# o\n+# / \\\n+# O o ?\n+# \\ /\n+# o\n+# B\n+#\n+# To help make it easier to follow the flow of tests, they have been\n+# divided into sections and each test will start with a quick explanation\n+# of what commits O, A, and B contain.\n+#\n+# Notation:\n+# z/{b,c} means files z/b and z/c both exist\n+# x/d_1 means file x/d exists with content d1. (Purpose of the\n+# underscore notation is to differentiate different\n+# files that might be renamed into each other's paths.)\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-merge.sh\n+\n+\n+# Testcase basic, conflicting changes in 'numerals'\n+\n+test_setup_numerals () {\n+\ttest_create_repo numerals_$1 &&\n+\t(\n+\t\tcd numerals_$1 &&\n+\n+\t\t>README &&\n+\t\ttest_write_lines I II III >numerals &&\n+\t\tgit add README numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_write_lines I II III IIII >numerals &&\n+\t\tgit add numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\ttest_write_lines I II III IV >numerals &&\n+\t\tgit add numerals &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\" &&\n+\n+\t\tcat <<-EOF >expected-index &&\n+\t\tH README\n+\t\tM numerals\n+\t\tM numerals\n+\t\tM numerals\n+\t\tEOF\n+\n+\t\tcat <<-EOF >expected-merge\n+\t\tI\n+\t\tII\n+\t\tIII\n+\t\t<<<<<<< HEAD\n+\t\tIIII\n+\t\t=======\n+\t\tIV\n+\t\t>>>>>>> B^0\n+\t\tEOF\n+\n+\t)\n+}\n+\n+test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n+\ttest_setup_numerals plain &&\n+\t(\n+\t\tcd numerals_plain &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\tgit sparse-checkout init &&\n+\t\tgit sparse-checkout set README &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_missing numerals &&\n+\n+\t\ttest_must_fail git merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -t >index_files &&\n+\t\ttest_cmp expected-index index_files &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\ttest_cmp expected-merge numerals &&\n+\n+\t\t# 4 other files:\n+\t\t# * expected-merge\n+\t\t# * expected-index\n+\t\t# * index_files\n+\t\t# * others\n+\t\tgit ls-files -o >others &&\n+\t\ttest_line_count = 4 others\n+\t)\n+'\n+\n+test_expect_merge_algorithm failure failure 'present-despite-SKIP_WORKTREE handled reasonably' '\n+\ttest_setup_numerals in_the_way &&\n+\t(\n+\t\tcd numerals_in_the_way &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\tgit sparse-checkout init &&\n+\t\tgit sparse-checkout set README &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_missing numerals &&\n+\n+\t\techo foobar >numerals &&\n+\n+\t\ttest_must_fail git merge -s recursive B^0 &&\n+\n+\t\tgit ls-files -t >index_files &&\n+\t\ttest_cmp expected-index index_files &&\n+\n+\t\ttest_path_is_file README &&\n+\t\ttest_path_is_file numerals &&\n+\n+\t\ttest_cmp expected-merge numerals &&\n+\n+\t\t# There should still be a file with \"foobar\" in it\n+\t\tgrep foobar * &&\n+\n+\t\t# 5 other files:\n+\t\t# * expected-merge\n+\t\t# * expected-index\n+\t\t# * index_files\n+\t\t# * others\n+\t\t# * whatever name was given to the numerals file that had\n+\t\t# \"foobar\" in it\n+\t\tgit ls-files -o >others &&\n+\t\ttest_line_count = 5 others\n+\t)\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"418574","messageId":"386cb3230b6780817120d83457796ccf6f64c84b.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 03/10] merge-ort: have ll_merge() use a special attr_index for renormalization","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:38Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nll_merge() needs an index when renormalization is requested. Create one\nspecifically for just this purpose with just the one needed entry. This\nfixes t6418.4 and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n\nNOTE 1: Even if the user has a working copy or a real index (which is\nnot a given as merge-ort can be used in bare repositories), we\nexplicitly ignore any .gitattributes file from either of these\nlocations. merge-ort can be used to merge two branches that are\nunrelated to HEAD, so .gitattributes from the working copy and current\nindex should not be considered relevant.\n\nNOTE 2: Since we are in the middle of merging, there is a risk that\n.gitattributes itself is conflicted...leaving us with an ill-defined\nsituation about how to perform the rest of the merge. It could be that\nthe .gitattributes file does not even exist on one of the sides of the\nmerge, or that it has been modified on both sides. If it's been\nmodified on both sides, it's possible that it could itself be merged\ncleanly, though it's also possible that it only merges cleanly if you\nuse the right version of the .gitattributes file to drive the merge. It\ngets kind of complicated. The only test we ever had that attempted to\ntest behavior in this area was seemingly unaware of the undefined\nbehavior, but knew the test wouldn't work for lack of attribute handling\nsupport, marked it as test_expect_failure from the beginning, but\nmanaged to fail for several reasons unrelated to attribute handling.\nSee commit 6f6e7cfb52 (\"t6038: remove problematic test\", 2020-08-03) for\ndetails. So there are probably various ways to improve what\ninitialize_attr_index() picks in the case of a conflicted .gitattributes\nbut for now I just implemented something simple -- look for whatever\n.gitattributes file we can find in any of the higher order stages and\nuse it.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d91b66a052b6..ea720eb51971 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -360,7 +360,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \tstring_list_clear(&opti->paths_to_free, 0);\n \topti->paths_to_free.strdup_strings = 0;\n \n-\tif (opti->attr_index.cache_nr)\n+\tif (opti->attr_index.cache_nr) /* true iff opt->renormalize */\n \t\tdiscard_index(&opti->attr_index);\n \n \t/* Free memory used by various renames maps */\n@@ -988,6 +988,63 @@ static int merge_submodule(struct merge_options *opt,\n \treturn 0;\n }\n \n+static void initialize_attr_index(struct merge_options *opt)\n+{\n+\t/*\n+\t * The renormalize_buffer() functions require attributes, and\n+\t * annoyingly those can only be read from the working tree or from\n+\t * an index_state. merge-ort doesn't have an index_state, so we\n+\t * generate a fake one containing only attribute information.\n+\t */\n+\tstruct merged_info *mi;\n+\tstruct index_state *attr_index = &opt->priv->attr_index;\n+\tstruct cache_entry *ce;\n+\n+\tattr_index->initialized = 1;\n+\n+\tif (!opt->renormalize)\n+\t\treturn;\n+\n+\tmi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);\n+\tif (!mi)\n+\t\treturn;\n+\n+\tif (mi->clean) {\n+\t\tint len = strlen(GITATTRIBUTES_FILE);\n+\t\tce = make_empty_cache_entry(attr_index, len);\n+\t\tce->ce_mode = create_ce_mode(mi->result.mode);\n+\t\tce->ce_flags = create_ce_flags(0);\n+\t\tce->ce_namelen = len;\n+\t\toidcpy(&ce->oid, &mi->result.oid);\n+\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n+\t\tadd_index_entry(attr_index, ce,\n+\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE, &ce->oid);\n+\t} else {\n+\t\tint stage, len;\n+\t\tstruct conflict_info *ci;\n+\n+\t\tASSIGN_AND_VERIFY_CI(ci, mi);\n+\t\tfor (stage = 0; stage < 3; stage++) {\n+\t\t\tunsigned stage_mask = (1 << stage);\n+\n+\t\t\tif (!(ci->filemask & stage_mask))\n+\t\t\t\tcontinue;\n+\t\t\tlen = strlen(GITATTRIBUTES_FILE);\n+\t\t\tce = make_empty_cache_entry(attr_index, len);\n+\t\t\tce->ce_mode = create_ce_mode(ci->stages[stage].mode);\n+\t\t\tce->ce_flags = create_ce_flags(stage);\n+\t\t\tce->ce_namelen = len;\n+\t\t\toidcpy(&ce->oid, &ci->stages[stage].oid);\n+\t\t\tmemcpy(ce->name, GITATTRIBUTES_FILE, len);\n+\t\t\tadd_index_entry(attr_index, ce,\n+\t\t\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+\t\t\tget_stream_filter(attr_index, GITATTRIBUTES_FILE,\n+\t\t\t\t\t &ce->oid);\n+\t\t}\n+\t}\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@@ -1002,6 +1059,9 @@ static int merge_3way(struct merge_options *opt,\n \tchar *base, *name1, *name2;\n \tint merge_status;\n \n+\tif (!opt->priv->attr_index.initialized)\n+\t\tinitialize_attr_index(opt);\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@@ -1040,7 +1100,7 @@ static int merge_3way(struct merge_options *opt,\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+\t\t\t\t&opt->priv->attr_index, &ll_opts);\n \n \tfree(base);\n \tfree(name1);\n-- \ngitgitgadget\n\n"},{"id":"418575","messageId":"90a57ade629d66e557f6f7b142e87cbeba8ca25e.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 07/10] merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:42Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen merge conflicts occur in paths removed by a sparse-checkout, we\nneed to unsparsify those paths (clear the SKIP_WORKTREE bit), and write\nout the conflicted file to the working copy. In the very unlikely case\nthat someone manually put a file into the working copy at the location\nof the SKIP_WORKTREE file, we need to avoid overwriting whatever edits\nthey have made and move that file to a different location first.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 43 +++++++++++++++++++++----------\n t/t6428-merge-conflicts-sparse.sh | 4 +--\n 2 files changed, 32 insertions(+), 15 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex cfd956a3f435..040287e3a998 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3234,23 +3234,27 @@ static int checkout(struct merge_options *opt,\n \treturn ret;\n }\n \n-static int record_conflicted_index_entries(struct merge_options *opt,\n-\t\t\t\t\t struct index_state *index,\n-\t\t\t\t\t struct strmap *paths,\n-\t\t\t\t\t struct strmap *conflicted)\n+static int record_conflicted_index_entries(struct merge_options *opt)\n {\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n+\tstruct index_state *index = opt->repo->index;\n+\tstruct checkout state = CHECKOUT_INIT;\n \tint errs = 0;\n \tint original_cache_nr;\n \n-\tif (strmap_empty(conflicted))\n+\tif (strmap_empty(&opt->priv->conflicted))\n \t\treturn 0;\n \n+\t/* If any entries have skip_worktree set, we'll have to check 'em out */\n+\tstate.force = 1;\n+\tstate.quiet = 1;\n+\tstate.refresh_cache = 1;\n+\tstate.istate = index;\n \toriginal_cache_nr = index->cache_nr;\n \n \t/* Put every entry from paths into plist, then sort */\n-\tstrmap_for_each_entry(conflicted, &iter, e) {\n+\tstrmap_for_each_entry(&opt->priv->conflicted, &iter, e) {\n \t\tconst char *path = e->key;\n \t\tstruct conflict_info *ci = e->value;\n \t\tint pos;\n@@ -3291,9 +3295,23 @@ static int record_conflicted_index_entries(struct merge_options *opt,\n \t\t\t * the higher order stages. Thus, we need override\n \t\t\t * the CE_SKIP_WORKTREE bit and manually write those\n \t\t\t * files to the working disk here.\n-\t\t\t *\n-\t\t\t * TODO: Implement this CE_SKIP_WORKTREE fixup.\n \t\t\t */\n+\t\t\tif (ce_skip_worktree(ce)) {\n+\t\t\t\tstruct stat st;\n+\n+\t\t\t\tif (!lstat(path, &st)) {\n+\t\t\t\t\tchar *new_name = unique_path(&opt->priv->paths,\n+\t\t\t\t\t\t\t\t path,\n+\t\t\t\t\t\t\t\t \"cruft\");\n+\n+\t\t\t\t\tpath_msg(opt, path, 1,\n+\t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n+\t\t\t\t\t\t path, new_name);\n+\t\t\t\t\terrs |= rename(path, new_name);\n+\t\t\t\t\tfree(new_name);\n+\t\t\t\t}\n+\t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Mark this cache entry for removal and instead add\n@@ -3343,8 +3361,6 @@ void merge_switch_to_result(struct merge_options *opt,\n {\n \tassert(opt->priv == NULL);\n \tif (result->clean >= 0 && update_worktree_and_index) {\n-\t\tstruct merge_options_internal *opti = result->priv;\n-\n \t\ttrace2_region_enter(\"merge\", \"checkout\", opt->repo);\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n@@ -3354,13 +3370,14 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n \n \t\ttrace2_region_enter(\"merge\", \"record_conflicted\", opt->repo);\n-\t\tif (record_conflicted_index_entries(opt, opt->repo->index,\n-\t\t\t\t\t\t &opti->paths,\n-\t\t\t\t\t\t &opti->conflicted)) {\n+\t\topt->priv = result->priv;\n+\t\tif (record_conflicted_index_entries(opt)) {\n \t\t\t/* failure to function */\n+\t\t\topt->priv = NULL;\n \t\t\tresult->clean = -1;\n \t\t\treturn;\n \t\t}\n+\t\topt->priv = NULL;\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n \t}\n \ndiff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\nindex 1bb52ff6f38c..7e8bf497f821 100755\n--- a/t/t6428-merge-conflicts-sparse.sh\n+++ b/t/t6428-merge-conflicts-sparse.sh\n@@ -76,7 +76,7 @@ test_setup_numerals () {\n \t)\n }\n \n-test_expect_merge_algorithm success failure 'conflicting entries written to worktree even if sparse' '\n+test_expect_success 'conflicting entries written to worktree even if sparse' '\n \ttest_setup_numerals plain &&\n \t(\n \t\tcd numerals_plain &&\n@@ -112,7 +112,7 @@ test_expect_merge_algorithm success failure 'conflicting entries written to work\n \t)\n '\n \n-test_expect_merge_algorithm failure failure 'present-despite-SKIP_WORKTREE handled reasonably' '\n+test_expect_merge_algorithm failure success 'present-despite-SKIP_WORKTREE handled reasonably' '\n \ttest_setup_numerals in_the_way &&\n \t(\n \t\tcd numerals_in_the_way &&\n-- \ngitgitgadget\n\n"},{"id":"418576","messageId":"93b241c2f21386bf40496fab6f2e21698835c508.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 09/10] merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:44Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are a variety of questions users might ask while resolving\nconflicts:\n * What changes have been made since the previous (first) parent?\n * What changes are staged?\n * What is still unstaged? (or what is still conflicted?)\n * What changes did I make to resolve conflicts so far?\nThe first three of these have simple answers:\n * git diff HEAD\n * git diff --cached\n * git diff\nThere was no way to answer the final question previously. Adding one\nis trivial in merge-ort, since it works by creating a tree representing\nwhat should be written to the working copy complete with conflict\nmarkers. Simply write that tree to .git/AUTO_MERGE, allowing users to\nanswer the fourth question with\n * git diff AUTO_MERGE\n\nI avoided using a name like \"MERGE_AUTO\", because that would be\nmerge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD,\nCHERRY_PICK_HEAD) and I wanted a name that didn't change depending on\nwhich type of operation the merge was part of.\n\nEnsure that paths which clean out other temporary operation-specific\nfiles (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory)\nalso clean out this AUTO_MERGE file.\n\nSigned-off-by: Elijah Newren \n---\n branch.c | 1 +\n builtin/rebase.c | 1 +\n merge-ort.c | 10 ++++++++++\n path.c | 1 +\n path.h | 2 ++\n sequencer.c | 5 +++++\n 6 files changed, 20 insertions(+)\n\ndiff --git a/branch.c b/branch.c\nindex 9c9dae1eae32..b71a2de29dbe 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)\n \tunlink(git_path_merge_rr(r));\n \tunlink(git_path_merge_msg(r));\n \tunlink(git_path_merge_mode(r));\n+\tunlink(git_path_auto_merge(r));\n \tsave_autostash(git_path_merge_autostash(r));\n }\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex de400f9a1973..7d9afe118fd4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -739,6 +739,7 @@ static int finish_rebase(struct rebase_options *opts)\n \tint ret = 0;\n \n \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n+\tunlink(git_path_auto_merge(the_repository));\n \tapply_autostash(state_dir_path(\"autostash\", opts));\n \tclose_object_store(the_repository->objects);\n \t/*\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 040287e3a998..1a07e39d2db4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3361,6 +3361,9 @@ void merge_switch_to_result(struct merge_options *opt,\n {\n \tassert(opt->priv == NULL);\n \tif (result->clean >= 0 && update_worktree_and_index) {\n+\t\tconst char *filename;\n+\t\tFILE *fp;\n+\n \t\ttrace2_region_enter(\"merge\", \"checkout\", opt->repo);\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n@@ -3379,6 +3382,13 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t}\n \t\topt->priv = NULL;\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n+\n+\t\ttrace2_region_enter(\"merge\", \"write_auto_merge\", opt->repo);\n+\t\tfilename = git_path_auto_merge(opt->repo);\n+\t\tfp = xfopen(filename, \"w\");\n+\t\tfprintf(fp, \"%s\\n\", oid_to_hex(&result->tree->object.oid));\n+\t\tfclose(fp);\n+\t\ttrace2_region_leave(\"merge\", \"write_auto_merge\", opt->repo);\n \t}\n \n \tif (display_update_msgs) {\ndiff --git a/path.c b/path.c\nindex 7b385e5eb282..9e883eb52446 100644\n--- a/path.c\n+++ b/path.c\n@@ -1534,5 +1534,6 @@ REPO_GIT_PATH_FUNC(merge_rr, \"MERGE_RR\")\n REPO_GIT_PATH_FUNC(merge_mode, \"MERGE_MODE\")\n REPO_GIT_PATH_FUNC(merge_head, \"MERGE_HEAD\")\n REPO_GIT_PATH_FUNC(merge_autostash, \"MERGE_AUTOSTASH\")\n+REPO_GIT_PATH_FUNC(auto_merge, \"AUTO_MERGE\")\n REPO_GIT_PATH_FUNC(fetch_head, \"FETCH_HEAD\")\n REPO_GIT_PATH_FUNC(shallow, \"shallow\")\ndiff --git a/path.h b/path.h\nindex e7e77da6aaa5..251c78d98000 100644\n--- a/path.h\n+++ b/path.h\n@@ -176,6 +176,7 @@ struct path_cache {\n \tconst char *merge_mode;\n \tconst char *merge_head;\n \tconst char *merge_autostash;\n+\tconst char *auto_merge;\n \tconst char *fetch_head;\n \tconst char *shallow;\n };\n@@ -191,6 +192,7 @@ const char *git_path_merge_rr(struct repository *r);\n const char *git_path_merge_mode(struct repository *r);\n const char *git_path_merge_head(struct repository *r);\n const char *git_path_merge_autostash(struct repository *r);\n+const char *git_path_auto_merge(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d2332d3e1787..472cdd8c620d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2096,6 +2096,7 @@ static int do_pick_commit(struct repository *r,\n \t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n \t\t\t\tNULL, 0);\n \t\tunlink(git_path_merge_msg(r));\n+\t\tunlink(git_path_auto_merge(r));\n \t\tfprintf(stderr,\n \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n@@ -2451,6 +2452,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \t\tneed_cleanup = 1;\n \t}\n \n+\tunlink(git_path_auto_merge(r));\n+\n \tif (!need_cleanup)\n \t\treturn;\n \n@@ -4111,6 +4114,7 @@ static int pick_commits(struct repository *r,\n \t\t\tunlink(rebase_path_stopped_sha());\n \t\t\tunlink(rebase_path_amend());\n \t\t\tunlink(git_path_merge_head(r));\n+\t\t\tunlink(git_path_auto_merge(r));\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n \t\t\tif (item->command == TODO_BREAK) {\n@@ -4505,6 +4509,7 @@ static int commit_staged_changes(struct repository *r,\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n \tunlink(git_path_merge_head(r));\n+\tunlink(git_path_auto_merge(r));\n \tif (final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n \t\tunlink(rebase_path_squash_msg());\n-- \ngitgitgadget\n\n"},{"id":"418577","messageId":"fcce88c5ac3d317007fd2435f97a9264810e9a22.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 08/10] t: mark several submodule merging tests as fixed under merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:43Z","receivedAt":"2021-03-09T06:25:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-ort handles submodules (and directory/file conflicts in general)\ndifferently than merge-recursive does; it basically puts all the special\nhandling for different filetypes into one place in the codebase instead\nof needing special handling for different filetypes in many different\ncode paths. This one code path in merge-ort could perhaps use some work\nstill (there are still test_expect_failure cases in the testsuite), but\nit passes all the tests that merge-recursive does as well as 12\nadditional ones that merge-recursive fails. Mark those 12 tests as\ntest_expect_success under merge-ort.\n\nSigned-off-by: Elijah Newren \n---\n t/t3512-cherry-pick-submodule.sh | 7 +++++--\n t/t3513-revert-submodule.sh | 5 ++++-\n t/t5572-pull-submodule.sh | 7 +++++--\n t/t6437-submodule-merge.sh | 5 +++--\n t/t6438-submodule-directory-file-conflicts.sh | 7 +++++--\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh\nindex 822f2d4bfbd5..c657840db33b 100755\n--- a/t/t3512-cherry-pick-submodule.sh\n+++ b/t/t3512-cherry-pick-submodule.sh\n@@ -8,8 +8,11 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n+then\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch \"cherry-pick\"\n \n test_expect_success 'unrelated submodule/file conflict is ignored' '\ndiff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh\nindex a759f12cbb1d..74cd96e58223 100755\n--- a/t/t3513-revert-submodule.sh\n+++ b/t/t3513-revert-submodule.sh\n@@ -30,7 +30,10 @@ git_revert () {\n \tgit revert HEAD\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n+then\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+fi\n test_submodule_switch_func \"git_revert\"\n \n test_done\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 29537f4798ef..4f92a116e1f0 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -42,8 +42,11 @@ git_pull_noff () {\n \t$2 git pull --no-ff\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n+then\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch_func \"git_pull_noff\"\n \n test_expect_success 'pull --recurse-submodule setup' '\ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex 0f92bcf326c8..e5e89c2045e7 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-merge.sh\n \n #\n # history\n@@ -328,7 +329,7 @@ test_expect_success 'setup file/submodule conflict' '\n \t)\n '\n \n-test_expect_failure 'file/submodule conflict' '\n+test_expect_merge_algorithm failure success 'file/submodule conflict' '\n \ttest_when_finished \"git -C file-submodule reset --hard\" &&\n \t(\n \t\tcd file-submodule &&\n@@ -437,7 +438,7 @@ test_expect_failure 'directory/submodule conflict; keep submodule clean' '\n \t)\n '\n \n-test_expect_failure !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n+test_expect_merge_algorithm failure success !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n \ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n \ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n \t(\ndiff --git a/t/t6438-submodule-directory-file-conflicts.sh b/t/t6438-submodule-directory-file-conflicts.sh\nindex 04bf4be7d792..8df67a0ef99d 100755\n--- a/t/t6438-submodule-directory-file-conflicts.sh\n+++ b/t/t6438-submodule-directory-file-conflicts.sh\n@@ -12,8 +12,11 @@ test_submodule_switch \"merge --ff\"\n \n test_submodule_switch \"merge --ff-only\"\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+if test \"$GIT_TEST_MERGE_ALGORITHM\" != ort\n+then\n+\tKNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n+\tKNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n+fi\n test_submodule_switch \"merge --no-ff\"\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"418578","messageId":"a9a95bb0391f23b15a6500fbf3282da0668e1d82.1615271086.git.gitgitgadget@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"[PATCH v2 10/10] merge-recursive: add a bunch of FIXME comments documenting known bugs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-09T06:24:45Z","receivedAt":"2021-03-09T06:25:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe plan is to just delete merge-recursive, but not until everyone is\ncomfortable with merge-ort as a replacement. Given that I haven't\nswitched all callers of merge-recursive over yet (e.g. git-am still uses\nmerge-recursive), maybe there's some value documenting known bugs in the\nalgorithm in case we end up keeping it or someone wants to dig it up in\nthe future.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b052974f191c..99a197597db5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1075,6 +1075,11 @@ static int merge_3way(struct merge_options *opt,\n \tread_mmblob(&src1, &a->oid);\n \tread_mmblob(&src2, &b->oid);\n \n+\t/*\n+\t * FIXME: Using a->path for normalization rules in ll_merge could be\n+\t * wrong if we renamed from a->path to b->path. We should use the\n+\t * target path for where the file will be written.\n+\t */\n \tmerge_status = ll_merge(result_buf, a->path, &orig, base,\n \t\t\t\t&src1, name1, &src2, name2,\n \t\t\t\topt->repo->index, &ll_opts);\n@@ -1154,6 +1159,8 @@ static void print_commit(struct commit *commit)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n \tctx.date_mode.type = DATE_NORMAL;\n+\t/* FIXME: Merge this with output_commit_title() */\n+\tassert(!merge_remote_util(commit));\n \tformat_commit_message(commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n@@ -1177,6 +1184,11 @@ static int merge_submodule(struct merge_options *opt,\n \tint search = !opt->priv->call_depth;\n \n \t/* store a in result in case we fail */\n+\t/* FIXME: This is the WRONG resolution for the recursive case when\n+\t * we need to be careful to avoid accidentally matching either side.\n+\t * Should probably use o instead there, much like we do for merging\n+\t * binaries.\n+\t */\n \toidcpy(result, a);\n \n \t/* we can not handle deletion conflicts */\n@@ -1301,6 +1313,13 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \n \tif ((S_IFMT & a->mode) != (S_IFMT & b->mode)) {\n \t\tresult->clean = 0;\n+\t\t/*\n+\t\t * FIXME: This is a bad resolution for recursive case; for\n+\t\t * the recursive case we want something that is unlikely to\n+\t\t * accidentally match either side. Also, while it makes\n+\t\t * sense to prefer regular files over symlinks, it doesn't\n+\t\t * make sense to prefer regular files over submodules.\n+\t\t */\n \t\tif (S_ISREG(a->mode)) {\n \t\t\tresult->blob.mode = a->mode;\n \t\t\toidcpy(&result->blob.oid, &a->oid);\n@@ -1349,6 +1368,7 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\tfree(result_buf.ptr);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n+\t\t\t/* FIXME: bug, what if modes didn't match? */\n \t\t\tresult->clean = (merge_status == 0);\n \t\t} else if (S_ISGITLINK(a->mode)) {\n \t\t\tresult->clean = merge_submodule(opt, &result->blob.oid,\n@@ -2664,6 +2684,14 @@ static int process_renames(struct merge_options *opt,\n \tstruct string_list b_by_dst = STRING_LIST_INIT_NODUP;\n \tconst struct rename *sre;\n \n+\t/*\n+\t * FIXME: As string-list.h notes, it's O(n^2) to build a sorted\n+\t * string_list one-by-one, but O(n log n) to build it unsorted and\n+\t * then sort it. Note that as we build the list, we do not need to\n+\t * check if the existing destination path is already in the list,\n+\t * because the structure of diffcore_rename guarantees we won't\n+\t * have duplicates.\n+\t */\n \tfor (i = 0; i < a_renames->nr; i++) {\n \t\tsre = a_renames->items[i].util;\n \t\tstring_list_insert(&a_by_dst, sre->pair->two->path)->util\n@@ -3602,6 +3630,15 @@ static int merge_recursive_internal(struct merge_options *opt,\n \t\t\treturn err(opt, _(\"merge returned no commit\"));\n \t}\n \n+\t/*\n+\t * FIXME: Since merge_recursive_internal() is only ever called by\n+\t * places that ensure the index is loaded first\n+\t * (e.g. builtin/merge.c, rebase/sequencer, etc.), in the common\n+\t * case where the merge base was unique that means when we get here\n+\t * we immediately discard the index and re-read it, which is a\n+\t * complete waste of time. We should only be discarding and\n+\t * re-reading if we were forced to recurse.\n+\t */\n \tdiscard_index(opt->repo->index);\n \tif (!opt->priv->call_depth)\n \t\trepo_read_index(opt->repo);\n-- \ngitgitgadget\n"},{"id":"418850","messageId":"10b30844-2e4c-157f-b4c6-5e4890e18858@gmail.com","threadId":"55259","inReplyTo":"24454e67b186992b9ef7a8ac6f220cbb4aaafd6b.1615271086.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 02/10] merge-ort: add a special minimal index just for renormalization","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T14:48:23Z","receivedAt":"2021-03-11T14:49:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> renormalize_buffer() requires an index_state, which is something that\n> merge-ort does not operate with. However, all the renormalization code\n> needs is an index with a .gitattributes file...plus a little bit of\n> setup. Create such an index, along with the deallocation and\n> attr_direction handling.\n> \n> A subsequent commit will add a function to finish the initialization\n> of this index.\n\nIs this the best way to solve this problem? Shouldn't we extract the\nlogic that interacts with the .gitattributes file from\nrenormalize_buffer() (say, to renormalize_buffer_attributes()) and\njust pass the .gitattributes file instead of creating a fake index?\n\nIt probably moves more code to do it the way I recommend, but it\nhelps to separate the concerns a bit better. Of course, I haven't\ntried doing it myself, so maybe it's harder than I think.\n \nThanks,\n-Stolee\n"},{"id":"418851","messageId":"a6f65409-29c6-a863-9804-41cba09d5732@gmail.com","threadId":"55259","inReplyTo":"386cb3230b6780817120d83457796ccf6f64c84b.1615271086.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 03/10] merge-ort: have ll_merge() use a special attr_index for renormalization","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T14:52:41Z","receivedAt":"2021-03-11T14:53:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> ll_merge() needs an index when renormalization is requested. Create one\n> specifically for just this purpose with just the one needed entry. This\n> fixes t6418.4 and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.\n\nI have similar concerns here. This strategy of creating an index with\nonly one entry is adding some deep coupling between the ORT code and\nthese normalization methods. It would be a better pattern to extract\nthe logic that normalizes based on some attributes file without caring\nwhere it came from.\n\nThanks,\n-Stolee\n"},{"id":"418852","messageId":"a478e63e-164e-9d38-0d23-5312fedf0953@gmail.com","threadId":"55259","inReplyTo":"d1d8c017b23ff227bbf4b8b500e8a5b1f2a0ebca.1615271086.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 06/10] t6428: new test for SKIP_WORKTREE handling and conflicts","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T14:55:12Z","receivedAt":"2021-03-11T14:56:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> If there is a conflict during a merge for a SKIP_WORKTREE entry, we\n> expect that file to be written to the working copy and have the\n> SKIP_WORKTREE bit cleared in the index. If the user had manually\n> created a file in the working tree despite SKIP_WORKTREE being set, we\n> do not want to clobber their changes to that file, but want to move it\n> out of the way. Add tests that check for these behaviors.\n\nThank you for this test!\n \n-Stolee\n"},{"id":"418855","messageId":"9a69a5fe-8f47-e925-1d20-0e13576fd995@gmail.com","threadId":"55259","inReplyTo":"fcce88c5ac3d317007fd2435f97a9264810e9a22.1615271086.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 08/10] t: mark several submodule merging tests as fixed under merge-ort","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T15:15:51Z","receivedAt":"2021-03-11T15:16:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> merge-ort handles submodules (and directory/file conflicts in general)\n> differently than merge-recursive does; it basically puts all the special\n> handling for different filetypes into one place in the codebase instead\n> of needing special handling for different filetypes in many different\n> code paths. This one code path in merge-ort could perhaps use some work\n> still (there are still test_expect_failure cases in the testsuite), but\n> it passes all the tests that merge-recursive does as well as 12\n> additional ones that merge-recursive fails. Mark those 12 tests as\n> test_expect_success under merge-ort.\n\nNice!\n-Stolee\n\n"},{"id":"418856","messageId":"cbf2716a-afbf-0b36-2f45-4c13ade38768@gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/10] Complete merge-ort implementation...almost","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T15:20:18Z","receivedAt":"2021-03-11T15:21:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> This series (nearly) completes the merge-ort implementation, cleaning up\n> testsuite failures. The exceptions are some t6423 failures being addressed\n> independently[1].\n\nI like that most of the patches here are small and straight-forward, and\nare backed up by tests.\n\nI'm concerned about the coupling of the \"index with one entry\" mechanism.\nI'd like to see that built differently by creating methods that can\noperate on an attributes file directly. If this method extraction is truly\ntoo difficult, then I'm willing to concede this point. It is worth some\neffort to try, though.\n\nThanks,\n-Stolee\n"},{"id":"418862","messageId":"CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com","threadId":"55259","inReplyTo":"cbf2716a-afbf-0b36-2f45-4c13ade38768@gmail.com","subject":"Re: [PATCH v2 00/10] Complete merge-ort implementation...almost","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-11T16:42:12Z","receivedAt":"2021-03-11T16:43:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee wrote:\n>\n> On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:\n> > This series (nearly) completes the merge-ort implementation, cleaning up\n> > testsuite failures. The exceptions are some t6423 failures being addressed\n> > independently[1].\n>\n> I like that most of the patches here are small and straight-forward, and\n> are backed up by tests.\n>\n> I'm concerned about the coupling of the \"index with one entry\" mechanism.\n> I'd like to see that built differently by creating methods that can\n> operate on an attributes file directly. If this method extraction is truly\n> too difficult, then I'm willing to concede this point. It is worth some\n> effort to try, though.\n\nOh, I agree, I hate the \"index with one entry\" mechanism to activate\nthe renormalization code. It feels like a capitulation on merge-ort's\ndesign and is such a hack. I tried to figure out how to do it a\ndifferent way, and failed pretty hard. I was close to the point of\nthrowing my hands in the air and leaving the renormalization tests\nfailing (\"who uses that stuff anyway?\"), but it was the _only_ test\nthat merge-recursive passed that merge-ort didn't, so I kept trying\nanyway. At some point I realized that a hack along these lines might\nwork. Although, to be honest, I still don't understand this code or\nwhy my patches work -- in particular, the get_stream_filter() call has\nme flummoxed. It returns a value, but I throw it away -- yet the code\nfails unless I call it. What is it actually doing? Clearly it has\nsome kind of side-effect somewhere, but reading the code didn't seem\nto help me. I discovered the function and others via various\ndebugging and trial-and-error attempts, and ended up finding out that\njust calling that function is enough. I know it works, but I don't\nknow why. I understand everything else in merge-ort; I hate having a\nmagic corner here. I'm not happy with it.\n\nI'm certain that with enough effort the renormalization stuff could be\nfixed and made callable without an istate. Perhaps there's even a\nsimple way to extract the relevant logic and I'm just being dense, but\nI got very lost in that code and I'm a bit afraid of it now.\n"},{"id":"419600","messageId":"CABPp-BFArfpnirzWZYpX6PnsgLkE+2rcDVjycG6iRWz5Kz0H9g@mail.gmail.com","threadId":"55259","inReplyTo":"pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/10] Complete merge-ort implementation...almost","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-03-17T21:42:01Z","receivedAt":"2021-03-17T21:43:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 8, 2021 at 10:24 PM Elijah Newren via GitGitGadget\n wrote:\n>\n> This series (nearly) completes the merge-ort implementation, cleaning up\n> testsuite failures. The exceptions are some t6423 failures being addressed\n> independently[1].\n\nThis series was subsumed into\nhttps://lore.kernel.org/git/pull.905.v2.git.1616016485.gitgitgadget@gmail.com/.\nSo, ignore this topic and look for the changes over there.\n\n(Reasons for the curious: This series had fixed t6409 and t6418 and\nthe submodule TODO passes, while the other series fixed t6423. The\nother series could not have been included when this was submitted\nbecause it depended on the not-yet-submitted ort-perf-batch-10. Why\nnot just wait? Well Ævar wanted a way to test his changes against\nmerge-ort, this early submission provided that and helped him find a\nbug -- see https://lore.kernel.org/git/20210308150650.18626-1-avarab@gmail.com/.\nNow that we can fix all the tests, I'm withdrawing this one and\nincluding it with the other fixes.)\n"}]}