{"thread":{"id":"58326","subject":"[PATCH 0/3] Show original filenames in merge tree","startedAt":"2022-08-19T06:57:42Z","lastAt":"2022-08-30T02:13:55Z","messageCount":12,"participants":["Johannes Schindelin via GitGitGadget","Johannes Schindelin","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"461507","messageId":"pull.1327.git.1660892256.gitgitgadget@gmail.com","threadId":"58326","inReplyTo":null,"subject":"[PATCH 0/3] Show original filenames in merge tree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T06:57:33Z","receivedAt":"2022-08-19T06:57:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The git merge-tree command has not seen much of real-world usage (because it\nonly recently learned to do something actually useful, and those changes\nhave not been part of any official Git version yet), which means that we are\nprone to find corner-cases where we want to improve it in ways that we had\nnot originally foreseen.\n\nI had the chance to give it some real-world exposure and noticed the\nfollowing concern: So far, git merge-tree shows munged file names in its\noutput that would make sense if the corresponding files were written to disk\n(you cannot write a file and a directory of the same name, after all). But\nfor an application or a web service relying on git merge-tree to operate in\nworktree-less mode, it makes much more sense to show the original file names\nbecause that's what the user needs to see.\n\nSo let's do that.\n\nNote: The written-out tree object still uses the munged names, as do the\nconflict messages. But the machine-parseable part now consistently uses the\noriginal file names.\n\nAlso note: This patch series addresses a bug where the ls-files -u-formatted\noutput would not reveal the correct file names in a rename/rename conflict.\n\nThis patch series is based on en/merge-tree.\n\nJohannes Schindelin (3):\n merge-tree -z: always show the original file name first\n merge-tree: show the original file names in the conflict output\n t4301: add a test case involving a rename, type change & modification\n\n builtin/merge-tree.c | 2 +-\n merge-ort.c | 16 +++++-----\n merge-ort.h | 1 +\n t/t4069-remerge-diff.sh | 8 ++---\n t/t4301-merge-tree-write-tree.sh | 54 +++++++++++++++++++++++++-------\n 5 files changed, 57 insertions(+), 24 deletions(-)\n\n\nbase-commit: 7260e87248e743b197d34c1caf3949ae4fa3bc12\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1327%2Fdscho%2Fshow-original-filenames-in-merge-tree-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1327/dscho/show-original-filenames-in-merge-tree-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1327\n-- \ngitgitgadget\n"},{"id":"461508","messageId":"c6eb286b60808bc9e69ce9b09fe4389db15db492.1660892256.git.gitgitgadget@gmail.com","threadId":"58326","inReplyTo":"pull.1327.git.1660892256.gitgitgadget@gmail.com","subject":"[PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T06:57:34Z","receivedAt":"2022-08-19T06:57:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen printing the messages in a machine-readable format (i.e. in `-z`\nmode), the intention is to support third-party software that wants to\npresent the conflicts in a \"pretty\" way to human readers.\n\nTo that end, `git merge-tree -z` prefixes the conflict messages by a\nvariable-size list of paths and a conflict identifier. This list and\nthis identifier are intended to be machine-parseable, the conflict\nmessage is sort of free-form and not intended to be parsed.\n\nKeeping in mind that the intended use case is to have third-party\nsoftware use `git merge-tree` to perform worktree-less merges and then\npresent the conflicts (if any) to human readers, it makes much more\nsense to show the original file names of the involved files instead of\nthe ones we internally munged to allow for creating a tree object that\ncontains entries for both sides of the conflict (which requires them to\nhave different names).\n\nSo let's mention the original file names prominently, as first item in\nthat variable-size list of paths.\n\nNote: For the modify/delete conflict type, we used to mention _only_ the\nmunged name in that path list. To allow for tools to read the tree\nobject produced by `git merge-tree -z` into a Git index in order to\nresolve the conflicts, it is necessary to list not only the original\nname but _also_ the munged name so that the item with the munged file\nname can be removed from that Git index. Therefore, this patch teaches\n`git merge-tree` to show both the original _and_ the munged name in that\nlist.\n\nAlso note: This patch changes the output of the remerge diff slightly:\nwhereas before, we printed the notice about a file/directory conflict\nunder the diff header mentioning the munged file name, we now print it\nunder a separate diff header that mentions the original file name.\nThat is the explanation why t4301 is touched by this patch.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 14 +++++++-------\n t/t4069-remerge-diff.sh | 8 ++++----\n t/t4301-merge-tree-write-tree.sh | 4 ++--\n 3 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 01f150ef3b5..211f6823e1d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3741,6 +3741,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t struct conflict_info *ci,\n \t\t\t struct directory_versions *dir_metadata)\n {\n+\tconst char *orig_path = path;\n \tint df_file_index = 0;\n \n \tVERIFY_CI(ci);\n@@ -3787,7 +3788,6 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tstruct conflict_info *new_ci;\n \t\tconst char *branch;\n-\t\tconst char *old_path = path;\n \t\tint i;\n \n \t\tassert(ci->merged.result.mode == S_IFDIR);\n@@ -3838,10 +3838,10 @@ static void process_entry(struct merge_options *opt,\n \t\tstrmap_put(&opt->priv->paths, path, new_ci);\n \n \t\tpath_msg(opt, CONFLICT_FILE_DIRECTORY, 0,\n-\t\t\t path, old_path, NULL, NULL,\n+\t\t\t orig_path, path, NULL, NULL,\n \t\t\t _(\"CONFLICT (file/directory): directory in the way \"\n \t\t\t \"of %s from %s; moving it to %s instead.\"),\n-\t\t\t old_path, branch, path);\n+\t\t\t orig_path, branch, path);\n \n \t\t/*\n \t\t * Zero out the filemask for the old ci. At this point, ci\n@@ -3921,7 +3921,7 @@ static void process_entry(struct merge_options *opt,\n \n \t\t\tif (rename_a && rename_b) {\n \t\t\t\tpath_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n-\t\t\t\t\t path, a_path, b_path, NULL,\n+\t\t\t\t\t orig_path, a_path, b_path, NULL,\n \t\t\t\t\t _(\"CONFLICT (distinct types): %s had \"\n \t\t\t\t\t \"different types on each side; \"\n \t\t\t\t\t \"renamed both of them so each can \"\n@@ -3929,7 +3929,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\t\t path);\n \t\t\t} else {\n \t\t\t\tpath_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n-\t\t\t\t\t path, rename_a ? a_path : b_path,\n+\t\t\t\t\t orig_path, rename_a ? a_path : b_path,\n \t\t\t\t\t NULL, NULL,\n \t\t\t\t\t _(\"CONFLICT (distinct types): %s had \"\n \t\t\t\t\t \"different types on each side; \"\n@@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\tif (S_ISGITLINK(merged_file.mode))\n \t\t\t\treason = _(\"submodule\");\n \t\t\tpath_msg(opt, CONFLICT_CONTENTS, 0,\n-\t\t\t\t path, NULL, NULL, NULL,\n+\t\t\t\t orig_path, NULL, NULL, NULL,\n \t\t\t\t _(\"CONFLICT (%s): Merge conflict in %s\"),\n \t\t\t\t reason, path);\n \t\t}\n@@ -4067,7 +4067,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t */\n \t\t} else {\n \t\t\tpath_msg(opt, CONFLICT_MODIFY_DELETE, 0,\n-\t\t\t\t path, NULL, NULL, NULL,\n+\t\t\t\t orig_path, path, NULL, NULL,\n \t\t\t\t _(\"CONFLICT (modify/delete): %s deleted in %s \"\n \t\t\t\t \"and modified in %s. Version %s of %s left \"\n \t\t\t\t \"in tree.\"),\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 35f94957fce..bc580a242ac 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n test_expect_success 'remerge-diff with non-content conflicts' '\n \tgit log -1 --oneline resolution >tmp &&\n \tcat <<-EOF >>tmp &&\n+\tdiff --git a/file_or_directory b/file_or_directory\n+\tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n \tdiff --git a/file_or_directory~HASH (side1) b/wanted_content\n \tsimilarity index 100%\n \trename from file_or_directory~HASH (side1)\n \trename to wanted_content\n-\tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n \tdiff --git a/letters b/letters\n \tremerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n \tdiff --git a/letters_side2 b/letters_side2\n@@ -168,7 +169,7 @@ test_expect_success 'remerge-diff with non-content conflicts' '\n test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no diff content' '\n \tgit log -1 --oneline resolution >tmp &&\n \tcat <<-EOF >>tmp &&\n-\tdiff --git a/file_or_directory~HASH (side1) b/file_or_directory~HASH (side1)\n+\tdiff --git a/file_or_directory b/file_or_directory\n \tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n \tdiff --git a/letters b/letters\n \tremerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n@@ -184,14 +185,13 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' '\n+test_expect_success 'remerge-diff w/ diff-filter=R: relevant file' '\n \tgit log -1 --oneline resolution >tmp &&\n \tcat <<-EOF >>tmp &&\n \tdiff --git a/file_or_directory~HASH (side1) b/wanted_content\n \tsimilarity index 100%\n \trename from file_or_directory~HASH (side1)\n \trename to wanted_content\n-\tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n \tEOF\n \t# We still have some sha1 hashes above; rip them out so test works\n \t# with sha256\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex f091259a55e..e0ef9724b2b 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -211,8 +211,8 @@ test_expect_success 'NUL terminated conflicted file \"lines\"' '\n \tq_to_nul <<-EOF >>expect &&\n \t1QgreetingQAuto-mergingQAuto-merging greeting\n \tQ1QgreetingQCONFLICT (contents)QCONFLICT (content): Merge conflict in greeting\n-\tQ2Qwhatever~tweak1QwhateverQCONFLICT (file/directory)QCONFLICT (file/directory): directory in the way of whatever from tweak1; moving it to whatever~tweak1 instead.\n-\tQ1Qwhatever~tweak1QCONFLICT (modify/delete)QCONFLICT (modify/delete): whatever~tweak1 deleted in side2 and modified in tweak1. Version tweak1 of whatever~tweak1 left in tree.\n+\tQ2QwhateverQwhatever~tweak1QCONFLICT (file/directory)QCONFLICT (file/directory): directory in the way of whatever from tweak1; moving it to whatever~tweak1 instead.\n+\tQ2QwhateverQwhatever~tweak1QCONFLICT (modify/delete)QCONFLICT (modify/delete): whatever~tweak1 deleted in side2 and modified in tweak1. Version tweak1 of whatever~tweak1 left in tree.\n \tQ1QΑυτά μου φαίνονται κινέζικαQAuto-mergingQAuto-merging Αυτά μου φαίνονται κινέζικα\n \tQ1QΑυτά μου φαίνονται κινέζικαQCONFLICT (contents)QCONFLICT (content): Merge conflict in Αυτά μου φαίνονται κινέζικα\n \tQ\n-- \ngitgitgadget\n\n"},{"id":"461509","messageId":"3aa390f3007f7f15e780bc9b36159846104be32e.1660892256.git.gitgitgadget@gmail.com","threadId":"58326","inReplyTo":"pull.1327.git.1660892256.gitgitgadget@gmail.com","subject":"[PATCH 2/3] merge-tree: show the original file names in the conflict output","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T06:57:35Z","receivedAt":"2022-08-19T06:57:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen showing the conflict output in an `ls-files -u`-style form, it\nmakes sense to show conflicted files' names in the form that they were\nwritten on disk (for example, in case of a file/directory conflict, we\nwould have written the file under a different name by appending a\n`~` suffix).\n\nHowever, when the intention is to perform a worktree-less merge, this\nmakes less sense, and causes more confusion than is necessary. Let's\nshow the original file names instead.\n\nThis patch also fixes a bug where the output of conflicts involving a\nrename did not always show the correct file name for the respective\nstages.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/merge-tree.c | 2 +-\n merge-ort.c | 2 +-\n merge-ort.h | 1 +\n t/t4301-merge-tree-write-tree.sh | 18 +++++++++---------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex ae5782917b9..8cd9ce399aa 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -454,8 +454,8 @@ static int real_merge(struct merge_tree_options *o,\n \n \t\tmerge_get_conflicted_files(&result, &conflicted_files);\n \t\tfor (i = 0; i < conflicted_files.nr; i++) {\n-\t\t\tconst char *name = conflicted_files.items[i].string;\n \t\t\tstruct stage_info *c = conflicted_files.items[i].util;\n+\t\t\tconst char *name = c->path;\n \t\t\tif (!o->name_only)\n \t\t\t\tprintf(\"%06o %s %d\\t\",\n \t\t\t\t c->mode, oid_to_hex(&c->oid), c->stage);\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 211f6823e1d..ed72d2e338e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4488,7 +4488,7 @@ void merge_get_conflicted_files(struct merge_result *result,\n \t\t\tif (!(ci->filemask & (1ul << i)))\n \t\t\t\tcontinue;\n \n-\t\t\tsi = xmalloc(sizeof(*si));\n+\t\t\tFLEX_ALLOC_STR(si, path, ci->pathnames[i]);\n \t\t\tsi->stage = i+1;\n \t\t\tsi->mode = ci->stages[i].mode;\n \t\t\toidcpy(&si->oid, &ci->stages[i].oid);\ndiff --git a/merge-ort.h b/merge-ort.h\nindex a994c9a5fcd..595031fbab9 100644\n--- a/merge-ort.h\n+++ b/merge-ort.h\n@@ -94,6 +94,7 @@ struct stage_info {\n \tstruct object_id oid;\n \tint mode;\n \tint stage;\n+\tconst char path[FLEX_ARRAY];\n };\n \n /*\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex e0ef9724b2b..e08452a8531 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -125,7 +125,7 @@ test_expect_success 'test conflict notices and such' '\n \tcat <<-EOF >expect &&\n \tHASH\n \tgreeting\n-\twhatever~side1\n+\twhatever\n \n \tAuto-merging greeting\n \tCONFLICT (content): Merge conflict in greeting\n@@ -153,7 +153,7 @@ test_expect_success 'Just the conflicted files without the messages' '\n \ttest_expect_code 1 git merge-tree --write-tree --no-messages --name-only side1 side2 >out &&\n \tanonymize_hash out >actual &&\n \n-\ttest_write_lines HASH greeting whatever~side1 >expect &&\n+\ttest_write_lines HASH greeting whatever >expect &&\n \n \ttest_cmp expect actual\n '\n@@ -168,14 +168,14 @@ test_expect_success 'Check conflicted oids and modes without messages' '\n \t100644 HASH 1Qgreeting\n \t100644 HASH 2Qgreeting\n \t100644 HASH 3Qgreeting\n-\t100644 HASH 1Qwhatever~side1\n-\t100644 HASH 2Qwhatever~side1\n+\t100644 HASH 1Qwhatever\n+\t100644 HASH 2Qwhatever\n \tEOF\n \n \ttest_cmp expect actual &&\n \n \t# Check the actual hashes against the `ls-files -u` output too\n-\ttail -n +2 out | sed -e s/side1/HEAD/ >actual &&\n+\ttail -n +2 out | sed -e s/whatever/whatever~HEAD/ >actual &&\n \ttest_cmp conflicted-file-info actual\n '\n \n@@ -200,11 +200,11 @@ test_expect_success 'NUL terminated conflicted file \"lines\"' '\n \t100644 HASH 1Qgreeting\n \t100644 HASH 2Qgreeting\n \t100644 HASH 3Qgreeting\n-\t100644 HASH 1Qwhatever~tweak1\n-\t100644 HASH 2Qwhatever~tweak1\n-\t100644 HASH 1QΑυτά μου φαίνονται κινέζικα\n+\t100644 HASH 1Qwhatever\n+\t100644 HASH 2Qwhatever\n+\t100644 HASH 1Qnumbers\n \t100644 HASH 2QΑυτά μου φαίνονται κινέζικα\n-\t100644 HASH 3QΑυτά μου φαίνονται κινέζικα\n+\t100644 HASH 3Qnumbers\n \n \tEOF\n \n-- \ngitgitgadget\n\n"},{"id":"461510","messageId":"3b4ed8bb1bb615277ee51a7b2af5fc53bae0a6e4.1660892256.git.gitgitgadget@gmail.com","threadId":"58326","inReplyTo":"pull.1327.git.1660892256.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t4301: add a test case involving a rename, type change & modification","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T06:57:36Z","receivedAt":"2022-08-19T06:57:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis is a reduced version of a real-world scenario that triggered the\nwork on the original file names.\n\nLet's make sure that Git does not regress in that scenario.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4301-merge-tree-write-tree.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex e08452a8531..3c41ade0a47 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -137,6 +137,38 @@ test_expect_success 'test conflict notices and such' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'original file names and type changes' '\n+\tgit switch --detach side1^ &&\n+\ttest_commit modified whatever &&\n+\n+\tgit switch --detach HEAD^ &&\n+\tgit mv whatever wherever &&\n+\tln -s wherever whatever &&\n+\tgit add whatever &&\n+\ttest_tick &&\n+\tgit commit -m symlink &&\n+\tgit tag symlink &&\n+\n+\ttest_expect_code 1 \\\n+\tgit merge-tree -z modified symlink >out &&\n+\tprintf \"\\\\n\" >>out &&\n+\tanonymize_hash out >actual &&\n+\n+\tq_to_tab <<-\\EOF | lf_to_nul >expect &&\n+\tHASH\n+\t120000 HASH 3Qwhatever\n+\t100644 HASH 1Qwhatever\n+\t100644 HASH 2Qwhatever\n+\n+\tEOF\n+\n+\tq_to_nul <<-EOF >>expect &&\n+\t2QwhateverQwhatever~modifiedQCONFLICT (distinct modes)QCONFLICT (distinct types): whatever had different types on each side; renamed one of them so each can be recorded somewhere.\n+\tQ\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n for opt in $(git merge-tree --git-completion-helper-all)\n do\n \tif test $opt = \"--trivial-merge\" || test $opt = \"--write-tree\"\n-- \ngitgitgadget\n"},{"id":"461519","messageId":"44pr310n-rpr1-0660-0961-386rsq9q11o1@tzk.qr","threadId":"58326","inReplyTo":"c6eb286b60808bc9e69ce9b09fe4389db15db492.1660892256.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-19T09:17:17Z","receivedAt":"2022-08-19T09:17:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi all (and I guess in particular Elijah),\n\nOn Fri, 19 Aug 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 35f94957fce..bc580a242ac 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> test_expect_success 'remerge-diff with non-content conflicts' '\n> \tgit log -1 --oneline resolution >tmp &&\n> \tcat <<-EOF >>tmp &&\n> +\tdiff --git a/file_or_directory b/file_or_directory\n> +\tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> \tdiff --git a/file_or_directory~HASH (side1) b/wanted_content\n> \tsimilarity index 100%\n> \trename from file_or_directory~HASH (side1)\n> \trename to wanted_content\n> -\tremerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> \tdiff --git a/letters b/letters\n> \tremerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> \tdiff --git a/letters_side2 b/letters_side2\n\nI would have liked to avoid touching this file altogether, of course, but\nwhen I investigated, I came to the conclusion that while this patch still\ndoes not produce the best possible outcome for remerge diffs, it does\nimprove upon the current situation.\n\nThe thing is, when mentioning that we had to rename `file_or_directory` to\n`file_or_directory~HASH (side1)` to be able to write the file because a\ndirectory of the same name already was in the way, I would actually have\nexpected this to come under the diff header\n\n\tdiff --git a/file_or_directory b/file_or_directory~HASH (side1)\n\nPreviously, it was under the header\n\n\tdiff --git a/file_or_directory~HASH (side1) b/wanted_content\n\nand with this patch it is under\n\n\tdiff --git a/file_or_directory b/file_or_directory\n\nwhich is still not ideal: It mentions only the original file name, not the\nmunged one.\n\nWhen I looked into the implementation details I figured out that the\ninformation is not quite at our fingertips. In\n[`create_filepairs_for_header_only_notifications()`][create_filepairs], we\nhave access to the primary filename (in the above example,\n`file_or_directory`) as the key of the entry in the strmap\n`o->additional_path_headers`, which is [populated from\n`path_messages`][additional-headers], which in turn [points to the\n`conflicts` strmap][path_messages]. Note: this code has changed between\nv2.37.2 and the current main branch, in v2.37.2 the information is still\ncopied from `conflicts` to `path_messages`.\n\nSo basically, when we generate that diff header, we know the primary path\n(great!) and we have access to a strmap that points to a string list of\nconflict messages. So from where do we get that munged path name? The\nstring list of conflict messages [has `util` pointers to `struct\nlogical_conflict_info` data][conflicts-strmap], which does contain the\nlist of involved paths. This should be enough, right? But:\n\n- `struct logical_conflict_info` is declared locally in `merge-ort.c`, we\n do not have access to that information in `diff.c` (nor would we really\n _want_ to let that `merge-ort` implementation detail spill over into\n `diff.c`), and even if we _could_ declare it in `merge-ort.h`, there is\n still this problem:\n\n- The path list in `struct logical_conflict_info` is arbitrary-sized, i.e.\n we could even have _three_ paths in there, and that does not fit into a\n standard diff header.\n\n- Additionally, we generate _one_ diff header for _all_ of the conflict\n messages listed for a particular primary path in\n `additional_path_headers`. But each of these conflict messages relates\n to its very own list of non-primary paths. For example, one conflict\n message could talk about a file/directory conflict, another one could\n talk about conflicting renames to two different filenames (in a\n recursive merge, both is possible in the same merge). It is simply\n impossible to combine all of those conflict messages under a single\n diff header that matches all of the involved paths.\n\n- Even aside from that, the remerge diff code seems to have a usability\n wart (or even bug) where restricting the diff via a pathspec that\n matches a secondary path but not the primary path of a conflict (think:\n renames) would _not_ show the conflict message to the user, it would be\n shown only if the pathspec also includes the primary path.\n\nIn general, I found it hard to think of a _really_ ideal design where to\npresent that remerge conflict information. After all, in a remerge diff,\nwe do not even write out any files, therefore that conflict message\nsuggesting that we renamed the file to a munged name is somewhat\nmisleading. Sure, we should mention the conflict between the file and the\ndirectory, but since nothing was written to disk, there is not actually\nany _need_ to mention a munged filename at all. And that goes even for the\npart of the remerge diff shown in the patch quoted above, where it talks\nabout `file_or_directory~HASH (side1)` being renamed... That file never\nhad that filename...\n\nSo I am not quite sure where we want the remerge design to go from here.\n\nIn any case, this remerge aspect is safely outside the scope of this here\npatch series, which means that this here patch series should not be\nconcerned with it. In this email, I just wanted to mention _why_ I did not\ninclude any patch to address the remerge issues any further.\n\nCiao,\nDscho\n\n[create_filepairs]: https://github.com/git/git/blob/v2.37.2/diff.c#L6337-L6383\n[additional-headers]: https://github.com/git/git/blob/v2.37.2/log-tree.c#L992\n[path_messages]: https://github.com/git/git/blob/795ea8776be/merge-ort.c#L4846\n[conflicts-strmap]: https://github.com/git/git/blob/795ea8776be/merge-ort.c#L351-L360\n"},{"id":"461672","messageId":"CABPp-BGPioxH2-2Pde8vAqFOOoW7WvCSf_fLzRWZB3uy=9Xc=g@mail.gmail.com","threadId":"58326","inReplyTo":"c6eb286b60808bc9e69ce9b09fe4389db15db492.1660892256.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-20T23:17:04Z","receivedAt":"2022-08-20T23:17:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Thu, Aug 18, 2022 at 11:57 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> When printing the messages in a machine-readable format (i.e. in `-z`\n> mode), the intention is to support third-party software that wants to\n> present the conflicts in a \"pretty\" way to human readers.\n>\n> To that end, `git merge-tree -z` prefixes the conflict messages by a\n> variable-size list of paths and a conflict identifier. This list and\n> this identifier are intended to be machine-parseable, the conflict\n> message is sort of free-form and not intended to be parsed.\n>\n> Keeping in mind that the intended use case is to have third-party\n> software use `git merge-tree` to perform worktree-less merges and then\n> present the conflicts (if any) to human readers, it makes much more\n> sense to show the original file names of the involved files instead of\n> the ones we internally munged to allow for creating a tree object that\n> contains entries for both sides of the conflict (which requires them to\n> have different names).\n\nI can see how this would be useful to you if it could be\nachieved...well, assuming you replaced \"instead of\" with \"in addition\nto\". It'd be a huge undertaking to achieve, though; far beyond what\nthis series attempts.\n\nI think your attempt here may be based on the presumption that any\nhash in a higher stage in the index maps directly to the hash of some\n: pair from one of the commits being merged. That\nassumption holds in simple cases, but does not hold generally.\nmerge-ort has a very strong \"(at most) 3 stages\" assumption for any\nconflict, driven by the need to ultimately represent conflicts in the\nindex, which is pervasive in the code. Trying to create such a direct\nmapping between higher stage conflicts and : pairs\nfrom the commits being merged would require breaking that assumption.\nThat would mean getting rid of many of the hardcoded array sizes of 3,\nas well as modifying who knows how many portions of logic built around\nthis assumption.\n\nYour attempt also seems to be based on the presumption that the\noriginal value of \"path\" as passed to process_entry(), represents a\npath from one of the two sides of history. In simple cases, it does.\nBut the assumption does not generally hold.\n\nFurther...\n\n> So let's mention the original file names prominently, as first item in\n> that variable-size list of paths.\n\nThis is in conflict with the needs of other uses of merge-ort:\n * When the user is running an active merge with conflicts written to\nthe index and working tree, the filenames where the conflicts are\nrecorded may not match the original filenames, and the filenames where\nthings are recorded is more useful and thus primary.\n * For remerge-diff, the filename where the conflict would be placed\nis important because that's where stuff ends up. Any changes the user\nmade for the final merge is relative to that munged path, so that\nmunged path is the important bit, not the original path.\n\nAdding additional paths to path_msg() to cover other paths relevant to\nthe conflict make sense, but the primary path really needs to be left\nas-is for these other cases.\n\n> Note: For the modify/delete conflict type, we used to mention _only_ the\n> munged name in that path list. To allow for tools to read the tree\n> object produced by `git merge-tree -z` into a Git index in order to\n> resolve the conflicts, it is necessary to list not only the original\n> name but _also_ the munged name so that the item with the munged file\n> name can be removed from that Git index. Therefore, this patch teaches\n> `git merge-tree` to show both the original _and_ the munged name in that\n> list.\n\nThis makes sense. Passing additional valid and relevant information\nto callers of merge-tree seems beneficial.\n\n> Also note: This patch changes the output of the remerge diff slightly:\n> whereas before, we printed the notice about a file/directory conflict\n> under the diff header mentioning the munged file name, we now print it\n> under a separate diff header that mentions the original file name.\n> That is the explanation why t4301 is touched by this patch.\n\nSounds problematic.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort.c | 14 +++++++-------\n> t/t4069-remerge-diff.sh | 8 ++++----\n> t/t4301-merge-tree-write-tree.sh | 4 ++--\n> 3 files changed, 13 insertions(+), 13 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 01f150ef3b5..211f6823e1d 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3741,6 +3741,7 @@ static void process_entry(struct merge_options *opt,\n> struct conflict_info *ci,\n> struct directory_versions *dir_metadata)\n> {\n> + const char *orig_path = path;\n> int df_file_index = 0;\n>\n> VERIFY_CI(ci);\n> @@ -3787,7 +3788,6 @@ static void process_entry(struct merge_options *opt,\n> */\n> struct conflict_info *new_ci;\n> const char *branch;\n> - const char *old_path = path;\n> int i;\n>\n> assert(ci->merged.result.mode == S_IFDIR);\n\nThis looks like you are trying to make sure you always have the\n\"original\" path available, meaning a path corresponding to a file from\none of the commits being merged. This doesn't quite accomplish that,\nthough. Some questions:\n\n * What about cases where \"path\" is a location that exists only in\nthe merge base? (rename/rename(1to2) is an example where this will\ncome up)\n * What about cases where \"path\" is a location that does not exist\nanywhere in history? (directory renames yield such a result. e.g.\nside A renames olddir/ -> newdir/, and side B adds olddir/foo, then\n\"path\" will be newdir/foo)\n\n> @@ -3838,10 +3838,10 @@ static void process_entry(struct merge_options *opt,\n> strmap_put(&opt->priv->paths, path, new_ci);\n>\n> path_msg(opt, CONFLICT_FILE_DIRECTORY, 0,\n> - path, old_path, NULL, NULL,\n> + orig_path, path, NULL, NULL,\n> _(\"CONFLICT (file/directory): directory in the way \"\n> \"of %s from %s; moving it to %s instead.\"),\n> - old_path, branch, path);\n> + orig_path, branch, path);\n> /*\n> * Zero out the filemask for the old ci. At this point, ci\n> @@ -3921,7 +3921,7 @@ static void process_entry(struct merge_options *opt,\n>\n> if (rename_a && rename_b) {\n> path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> - path, a_path, b_path, NULL,\n> + orig_path, a_path, b_path, NULL,\n> _(\"CONFLICT (distinct types): %s had \"\n> \"different types on each side; \"\n> \"renamed both of them so each can \"\n> @@ -3929,7 +3929,7 @@ static void process_entry(struct merge_options *opt,\n> path);\n> } else {\n> path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> - path, rename_a ? a_path : b_path,\n> + orig_path, rename_a ? a_path : b_path,\n> NULL, NULL,\n> _(\"CONFLICT (distinct types): %s had \"\n> \"different types on each side; \"\n\nI think for both of these last two cases, path == orig_path, so you\nhaven't actually made an effective change here.\n\n(And, as above, I want path to always be the primary_path passed to\npath_msg() as other code was written with that assumption in mind.)\n\n> @@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n> if (S_ISGITLINK(merged_file.mode))\n> reason = _(\"submodule\");\n> path_msg(opt, CONFLICT_CONTENTS, 0,\n> - path, NULL, NULL, NULL,\n> + orig_path, NULL, NULL, NULL,\n> _(\"CONFLICT (%s): Merge conflict in %s\"),\n> reason, path);\n> }\n\nHere's another case where path == orig_path, so you haven't made an\neffective change. But this one highlights something interesting...\n\nIn addition to the fact that path/orig_path may be a location that\ndidn't exist on either side of history, there might actually be two\nrelevant paths from the two different sides of history which are\ninvolved in the content merge, with neither of them being path or\norig_path. Let me break it down, starting with a simpler two path\ncase:\n\nIf we have a standard rename, e.g. foo -> bar, and both sides modified\nthe file but did so in a conflicting manner, then we will end up in\nthis chunk of code. The conflict info emitted by merge-tree -z which\nis always of the form\n NULNULNULNULNULNUL\nwill in this particular case be\n 1barAuto-mergingAuto-merging bar\n 1barCONFLICT (contents)CONFLICT (content): Merge\nconflict in bar\nNeither of these messages has any mention of \"foo\", despite the fact\nthat \"foo\" was the name of the file in both the merge base and one\nside, and \"bar\" was only the name of the file on one side.\n\nNow, let's make it more interesting. side A modifies foo, and renames\nolddir/->newdir/. side B modifies foo in a conflicting manner, and\nrenames foo->olddir/bar. Our `merge-tree -z` conflict information\nemitted will be of the form\n 1newdir/barAuto-mergingAuto-merging newdir/bar\n 1newdir/barCONFLICT (contents)CONFLICT (content):\nMerge conflict in newdir/bar\nFor this more interesting case, the only files that existed were \"foo\"\nand \"olddir/bar\", neither of which are mentioned in the conflict info.\nThe conflict info only reports on \"newdir/bar\".\n\nAnd for both of these examples, your patch doesn't change the existing\nbehavior at all since path == orig_path for this hunk of the patch.\n\n> @@ -4067,7 +4067,7 @@ static void process_entry(struct merge_options *opt,\n> */\n> } else {\n> path_msg(opt, CONFLICT_MODIFY_DELETE, 0,\n> - path, NULL, NULL, NULL,\n> + orig_path, path, NULL, NULL,\n> _(\"CONFLICT (modify/delete): %s deleted in %s \"\n> \"and modified in %s. Version %s of %s left \"\n> \"in tree.\"),\n\nAdding orig_path here after path would make sense, though it's still\nincomplete. For example if a path was modified and renamed on one\nside, and deleted on the other, then we'd get to this hunk, and the\nmodify/delete would omit information about the other path. Further,\ndirectory renames could be at play, such that neither orig_path nor\npath would correspond to a filename in either of the commits being\nmerged.\n\nThere are also other path_msg() calls that should probably be informed\nof additional filenames due to simple renames: CONFLICT_CONTENTS (as\nnoted above), CONFLICT_BINARY, and INFO_AUTO_MERGING.\n\nGoing beyond simple renames, if you want \"fully original\" filenames,\nthen we'd probably have to audit nearly all the path_msg() calls for\nwhen directory renames are possible, and modify the directory renaming\ncode to plumb the original name through to this layer.\n\nAnd even if we do all that, then we still aren't matching what I think\nyou are expecting where every conflicted higher order entry we report\nmaps to a single : pairing. We may have a higher\nstage conflict entry that represents merged contents of three\ndifferent filecontents from three different filenames. For example,\nif someone has the following setup:\n\n Commit O: one file named \"original\"\n Commit A: modify original + rename to primordial\n Commit B: add file named primordial, modify original differently +\nrename it to primary\n\nwhere A & B both have O as a parent or ancestor, and O is the unique\nmerge base of A & B. Now if we try to merge A & B, then the index\nwill have four higher order entries (using \n , as we'd see from either `ls-files -u` style\noutput or from the first part of merge-tree -z output):\n\n 100644 1 original\n 100644 3 primary\n 100644 2 primordial\n 100644 3 primordial\n\nNote that the two middle entries, despite having one name attached to\nthem, actually come from a merge of three different files. Further,\nthat hash for those two entries will not have yet appeared as the\ncontents for any blob in any revision of history, and will in fact\nrepresent a file with conflict markers (even if none of the original\nfiles -- O:original, A:primodial, B:primary -- had any conflicts).\nAnd, the version of \"primodial\" in the merge-result-tree would likely\nhave nested conflict markers, despite O being a unique merge base\n(meaning we get nested conflict markers without merging merge bases).\n\nTrying to have all the conflict output from merge-tree map to some\noriginal file the user had from one of their sides of history is thus\na goal that merge-ort is nowhere close to satisfying. I'm not even\nsure it's tractable.\n\nHowever, there is a much smaller scoped goal that we could easily\nachieve: providing additional filename information in the path_msg()\ncalls, much like you start to do with the modify/delete case.\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 35f94957fce..bc580a242ac 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> test_expect_success 'remerge-diff with non-content conflicts' '\n> git log -1 --oneline resolution >tmp &&\n> cat <<-EOF >>tmp &&\n> + diff --git a/file_or_directory b/file_or_directory\n> + remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> similarity index 100%\n> rename from file_or_directory~HASH (side1)\n> rename to wanted_content\n> - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> diff --git a/letters b/letters\n> remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> diff --git a/letters_side2 b/letters_side2\n> @@ -168,7 +169,7 @@ test_expect_success 'remerge-diff with non-content conflicts' '\n> test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no diff content' '\n> git log -1 --oneline resolution >tmp &&\n> cat <<-EOF >>tmp &&\n> - diff --git a/file_or_directory~HASH (side1) b/file_or_directory~HASH (side1)\n> + diff --git a/file_or_directory b/file_or_directory\n> remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> diff --git a/letters b/letters\n> remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> @@ -184,14 +185,13 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' '\n> +test_expect_success 'remerge-diff w/ diff-filter=R: relevant file' '\n> git log -1 --oneline resolution >tmp &&\n> cat <<-EOF >>tmp &&\n> diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> similarity index 100%\n> rename from file_or_directory~HASH (side1)\n> rename to wanted_content\n> - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> EOF\n> # We still have some sha1 hashes above; rip them out so test works\n> # with sha256\n\nAll of these look like regressions to me; you discussed these in a\nseparate email so I'll go into detail in response to that one.\n"},{"id":"461673","messageId":"CABPp-BH98_N4WrYUWTx8oOvfOWhEL+9sGnR8U2tRZjYUfzv1_Q@mail.gmail.com","threadId":"58326","inReplyTo":"44pr310n-rpr1-0660-0961-386rsq9q11o1@tzk.qr","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-20T23:22:30Z","receivedAt":"2022-08-20T23:22:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 2:17 AM Johannes Schindelin\n wrote:\n>\n> Hi all (and I guess in particular Elijah),\n>\n> On Fri, 19 Aug 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> > index 35f94957fce..bc580a242ac 100755\n> > --- a/t/t4069-remerge-diff.sh\n> > +++ b/t/t4069-remerge-diff.sh\n> > @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> > test_expect_success 'remerge-diff with non-content conflicts' '\n> > git log -1 --oneline resolution >tmp &&\n> > cat <<-EOF >>tmp &&\n> > + diff --git a/file_or_directory b/file_or_directory\n> > + remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > similarity index 100%\n> > rename from file_or_directory~HASH (side1)\n> > rename to wanted_content\n> > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > diff --git a/letters b/letters\n> > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > diff --git a/letters_side2 b/letters_side2\n>\n> I would have liked to avoid touching this file altogether, of course, but\n> when I investigated, I came to the conclusion that while this patch still\n> does not produce the best possible outcome for remerge diffs, it does\n> improve upon the current situation.\n\nNo, this is a strict regression. Those file headers corresponding to\nthe file/directory conflict (similarity index, rename from/to, and\nCONFLICT) are all supposed to be grouped together in the same diff\nheader. As to the names...\n\n> The thing is, when mentioning that we had to rename `file_or_directory` to\n> `file_or_directory~HASH (side1)` to be able to write the file because a\n> directory of the same name already was in the way, I would actually have\n> expected this to come under the diff header\n>\n> diff --git a/file_or_directory b/file_or_directory~HASH (side1)\n>\n> Previously, it was under the header\n>\n> diff --git a/file_or_directory~HASH (side1) b/wanted_content\n>\n> and with this patch it is under\n>\n> diff --git a/file_or_directory b/file_or_directory\n>\n> which is still not ideal: It mentions only the original file name, not the\n> munged one.\n\nThe point of remerge-diff is to show how a user modifies from the\nauto-merged state to the final merge commit. As such, the filenames\nfrom before the merge starts are irrelevant; the only two relevant\nthings are the filename at the end of the automatic merge, and the\nfilename recorded in the commit.\n\nIn particular, for the case here, upon \"git merge $BRANCH\" the merge\nwould have stopped with a file named \"file_or_directory~HASH\\ (side1)\"\npresent in the working copy[1]. The user resolved it via renaming\nthat file to \"wanted_content\". As such, the first and third options\nyou list are incorrect, and the second one -- from the behavior\nprevious to your patch -- is correct.\n\n\n[1] Technically, \"git merge $BRANCH\" would have used \"HEAD\" rather\nthan \"HASH (side1)\", but that's only because at the time of invocation\nof git merge, HEAD would have been a very specific commit. The same\nisn't true of HEAD when the user runs `git log --remerge-diff` at some\nvery different time, so we opt to replace \"HEAD\" with something more\nmeaningful for readers of the diff when running --remerge-diff.\n"},{"id":"461678","messageId":"CABPp-BG0NGEmGCcF36HhGCNNGL6csJXqAa5s6cXvXWFJfDor7w@mail.gmail.com","threadId":"58326","inReplyTo":"CABPp-BGPioxH2-2Pde8vAqFOOoW7WvCSf_fLzRWZB3uy=9Xc=g@mail.gmail.com","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-21T02:00:22Z","receivedAt":"2022-08-21T02:01:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Aug 20, 2022 at 4:17 PM Elijah Newren wrote:\n> On Thu, Aug 18, 2022 at 11:57 PM Johannes Schindelin via GitGitGadget\n> wrote:\n[...]\n> > @@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n> > if (S_ISGITLINK(merged_file.mode))\n> > reason = _(\"submodule\");\n> > path_msg(opt, CONFLICT_CONTENTS, 0,\n> > - path, NULL, NULL, NULL,\n> > + orig_path, NULL, NULL, NULL,\n> > _(\"CONFLICT (%s): Merge conflict in %s\"),\n> > reason, path);\n> > }\n>\n> Here's another case where path == orig_path, so you haven't made an\n> effective change. But this one highlights something interesting...\n>\n> In addition to the fact that path/orig_path may be a location that\n> didn't exist on either side of history, there might actually be two\n> relevant paths from the two different sides of history which are\n> involved in the content merge, with neither of them being path or\n> orig_path. Let me break it down, starting with a simpler two path\n> case:\n>\n> If we have a standard rename, e.g. foo -> bar, and both sides modified\n> the file but did so in a conflicting manner, then we will end up in\n> this chunk of code. The conflict info emitted by merge-tree -z which\n> is always of the form\n> NULNULNULNULNULNUL\n> will in this particular case be\n> 1barAuto-mergingAuto-merging bar\n> 1barCONFLICT (contents)CONFLICT (content): Merge\n> conflict in bar\n> Neither of these messages has any mention of \"foo\", despite the fact\n> that \"foo\" was the name of the file in both the merge base and one\n> side, and \"bar\" was only the name of the file on one side.\n>\n> Now, let's make it more interesting. side A modifies foo, and renames\n> olddir/->newdir/. side B modifies foo in a conflicting manner, and\n> renames foo->olddir/bar. Our `merge-tree -z` conflict information\n> emitted will be of the form\n> 1newdir/barAuto-mergingAuto-merging newdir/bar\n> 1newdir/barCONFLICT (contents)CONFLICT (content):\n> Merge conflict in newdir/bar\n> For this more interesting case, the only files that existed were \"foo\"\n> and \"olddir/bar\", neither of which are mentioned in the conflict info.\n> The conflict info only reports on \"newdir/bar\".\n>\n> And for both of these examples, your patch doesn't change the existing\n> behavior at all since path == orig_path for this hunk of the patch.\n\nBut, actually, trying to create some more testcases for the testsuite,\nmaybe this isn't so bad. For example, with this interesting testcase:\n\n Commit O: foo, olddir/{a,b,c}\n Commit A: delete foo, rename olddir/ -> newdir/, add newdir/bar/file\n Commit B: modify foo & rename foo -> olddir/bar\n\nWhich involves four different types of conflicts:\n\n * a directory rename (which further modifies foo->olddir/bar to end\nup at newdir/bar)\n * a rename/delete (delete foo vs. rename to olddir/bar)\n * a modify/delete (since foo was modified as well on one side)\n * a directory/file conflict (newdir/bar vs newdir/bar/file, forcing\nnewdir/bar to be further renamed to newdir/bar~B^0)\n\nThe section will look like\n 100644 1 newdir/bar~B^0\n 100644 3 newdir/bar~B^0\n\nWhile that only includes the name \"newdir/bar~B^0\" and not\n\"newdir/bar\", or \"olddir/bar\", or \"foo\", the \nhas all necessary information to tie it together. Replacing null\ncharacters with newlines, the section is:\n\n 2\n newdir/bar\n olddir/bar\n CONFLICT (directory rename suggested)\n CONFLICT (file location): foo renamed to olddir/bar in B^0, inside\na directory that was renamed in A^0, suggesting it should perhaps be\nmoved to newdir/bar.\n\n 2\n newdir/bar\n foo\n CONFLICT (rename/delete)\n CONFLICT (rename/delete): foo renamed to newdir/bar in B^0, but\ndeleted in A^0.\n\n 2\n newdir/bar~B^0\n newdir/bar\n CONFLICT (file/directory)\n CONFLICT (file/directory): directory in the way of newdir/bar from\nB^0; moving it to newdir/bar~B^0 instead.\n\n 1\n newdir/bar~B^0\n CONFLICT (modify/delete)\n CONFLICT (modify/delete): newdir/bar~B^0 deleted in A^0 and\nmodified in B^0. Version B^0 of newdir/bar~B^0 left in tree.\n\nThis provides all the mappings to tie together foo, olddir/bar,\nnewdir/bar, and newdir/bar~B^0, and shows the four conflict types\npresent. So, all the information you need is present, it just can't\nbe parsed out of a single line like you want. (But adding the\n\"newdir/bar\" name to the modify/delete conflict at least does seem\nlike it'd be a little nicer.)\n\nAnd trying to parse it out of a single line is probably tantamount to\ntrying to enforce a 1-to-1 mapping between paths and conflicts, which\nwill paint us into a corner, as I've pointed out before[1,2]\n\n[1] https://lore.kernel.org/git/CABPp-BGCL0onSmpgKuO1k2spYCkx=v27ed9TSSxFib=OdDcLbw@mail.gmail.com/\n[2] https://lore.kernel.org/git/CABPp-BGnqXdFBNAyKRXgvCHv+aUZTMg-CgcQf95dKAR-e1zSjQ@mail.gmail.com/\n"},{"id":"461808","messageId":"9p08998o-n88r-2nno-8703-s99o5qpn9o35@tzk.qr","threadId":"58326","inReplyTo":"CABPp-BGPioxH2-2Pde8vAqFOOoW7WvCSf_fLzRWZB3uy=9Xc=g@mail.gmail.com","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-22T20:12:42Z","receivedAt":"2022-08-22T20:12:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Sat, 20 Aug 2022, Elijah Newren wrote:\n\n> On Thu, Aug 18, 2022 at 11:57 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > From: Johannes Schindelin \n> >\n> > When printing the messages in a machine-readable format (i.e. in `-z`\n> > mode), the intention is to support third-party software that wants to\n> > present the conflicts in a \"pretty\" way to human readers.\n> >\n> > To that end, `git merge-tree -z` prefixes the conflict messages by a\n> > variable-size list of paths and a conflict identifier. This list and\n> > this identifier are intended to be machine-parseable, the conflict\n> > message is sort of free-form and not intended to be parsed.\n> >\n> > Keeping in mind that the intended use case is to have third-party\n> > software use `git merge-tree` to perform worktree-less merges and then\n> > present the conflicts (if any) to human readers, it makes much more\n> > sense to show the original file names of the involved files instead of\n> > the ones we internally munged to allow for creating a tree object that\n> > contains entries for both sides of the conflict (which requires them to\n> > have different names).\n>\n> I can see how this would be useful to you if it could be\n> achieved...well, assuming you replaced \"instead of\" with \"in addition\n> to\". It'd be a huge undertaking to achieve, though; far beyond what\n> this series attempts.\n\nI feared you would say that :-)\n\nAnd I realize that I should have lead with the context that is relevant to\nunderstand what I am driving at.\n\nFor now, all I am trying to do is to verify that we can achieve identical\nresults between a new code path that uses `git merge-tree` (with options)\nand an existing code path that uses libgit2 to perform a 3-way merge.\n\nTo that end, I have added some patches to optionally disable rename\ndetection, to allow specifying a merge base instead of performing a full\nrecursive merge, to optionally detect whether the `MERGE_HEAD` is already\nreachable from `HEAD` and in that case exit with a specific exit status,\namong other things.\n\nAnd yes, I realize that I \"only\" benefit from the worktree-less nature of\n`merge-ort` so far, and not from any other aspects `merge-ort` offers.\n\nOne thing I need from the result is a list of conflicts in the same format\nas we would have received if writing out an index with conflicts and then\nparsing just the entries with non-zero stages.\n\nSo for now, I do not care _so_ much about rename conflicts, or conflicts\nresulting from merging merge base candidates. I don't want to slam the\ndoor shut, of course, but due to a teeny tiny bit of time pressure, I want\nto focus very, very much on the libgit2-like operation for now :-)\n\nWhat I do need, however, is the unmunged filenames as well as the\ncorresponding file modes and the object hashes.\n\n> I think your attempt here may be based on the presumption that any\n> hash in a higher stage in the index maps directly to the hash of some\n> : pair from one of the commits being merged. That\n> assumption holds in simple cases, but does not hold generally.\n> merge-ort has a very strong \"(at most) 3 stages\" assumption for any\n> conflict, driven by the need to ultimately represent conflicts in the\n> index, which is pervasive in the code.\n\nThis is actually very helpful information, this had not sunk in yet.\n\nSo when encountering a \"stage 3 before stages 1 & 2\" situation arises,\nlike in the test case I added, you're saying that I could simply reorder\nthose and be done and not have to expect that the `stage 1 & 2` part is\nfollowed by _another_ stage 3 part for the same name?\n\nExcept, of course, that I need the original file names, and the invariant\nof at most 3 stages holds true only when taking munged file names into\naccount, right?\n\n> Trying to create such a direct mapping between higher stage conflicts\n> and : pairs from the commits being merged would\n> require breaking that assumption. That would mean getting rid of many of\n> the hardcoded array sizes of 3, as well as modifying who knows how many\n> portions of logic built around this assumption.\n>\n> Your attempt also seems to be based on the presumption that the\n> original value of \"path\" as passed to process_entry(), represents a\n> path from one of the two sides of history. In simple cases, it does.\n> But the assumption does not generally hold.\n\nHeh, in my case it always holds because I disable rename detection. The\nonly time when multiple file names are involved is in case of\nfile/directory conflicts and the like. In which case I still need the\noriginal filename.\n\nBut I would contend that we need to know the original file name(s) even in\nthe case of renames, so that we can present the users with all the\ninformation pertinent to the conflict, to allow them to resolve it (so\nthat they have the full context).\n\n> Further...\n>\n> > So let's mention the original file names prominently, as first item in\n> > that variable-size list of paths.\n>\n> This is in conflict with the needs of other uses of merge-ort:\n> * When the user is running an active merge with conflicts written to\n> the index and working tree, the filenames where the conflicts are\n> recorded may not match the original filenames, and the filenames where\n> things are recorded is more useful and thus primary.\n> * For remerge-diff, the filename where the conflict would be placed\n> is important because that's where stuff ends up. Any changes the user\n> made for the final merge is relative to that munged path, so that\n> munged path is the important bit, not the original path.\n\nOkay, I understand now.\n\nSo basically, if I need different/additional information, I will _have_ to\nintroduce a new flag to trigger code paths that produce that.\n\nAnd my changes to the remerge test cases merely demonstrate precisely that\nneed.\n\n> Adding additional paths to path_msg() to cover other paths relevant to\n> the conflict make sense, but the primary path really needs to be left\n> as-is for these other cases.\n\nI fear that I will need much, much more than just the additional paths ;-)\nSo far, I am parsing the `ls-files -u`-like part so that I get mode and\nhash information, but eventually I want to parse that out of the messages\npart and that only produces lists of paths so far...\n\nAnd a simple, single list of paths is not enough to convey the information\nthat I need. Concretely, I need:\n\n- the original file name(s) involved in the conflict,\n- the modes and object hashes corresponding to the different stages\n- maybe some provenance information in the future, in case of complex\n rename/merge recursions\n\nIn particular when presenting a file/directory conflict, it will be\nimportant to know what stage refers to the file and what stage to the\ndirectory (so as to know whether the respective object hash refers to a\nfile or even symlink and needs to be read as a blob).\n\n> > Note: For the modify/delete conflict type, we used to mention _only_ the\n> > munged name in that path list. To allow for tools to read the tree\n> > object produced by `git merge-tree -z` into a Git index in order to\n> > resolve the conflicts, it is necessary to list not only the original\n> > name but _also_ the munged name so that the item with the munged file\n> > name can be removed from that Git index. Therefore, this patch teaches\n> > `git merge-tree` to show both the original _and_ the munged name in that\n> > list.\n>\n> This makes sense. Passing additional valid and relevant information\n> to callers of merge-tree seems beneficial.\n\nAnd now that you mention this, I realize that this should be pulled out\ninto its own patch.\n\n> > Also note: This patch changes the output of the remerge diff slightly:\n> > whereas before, we printed the notice about a file/directory conflict\n> > under the diff header mentioning the munged file name, we now print it\n> > under a separate diff header that mentions the original file name.\n> > That is the explanation why t4301 is touched by this patch.\n>\n> Sounds problematic.\n\nFair enough.\n\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > merge-ort.c | 14 +++++++-------\n> > t/t4069-remerge-diff.sh | 8 ++++----\n> > t/t4301-merge-tree-write-tree.sh | 4 ++--\n> > 3 files changed, 13 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 01f150ef3b5..211f6823e1d 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3741,6 +3741,7 @@ static void process_entry(struct merge_options *opt,\n> > struct conflict_info *ci,\n> > struct directory_versions *dir_metadata)\n> > {\n> > + const char *orig_path = path;\n> > int df_file_index = 0;\n> >\n> > VERIFY_CI(ci);\n> > @@ -3787,7 +3788,6 @@ static void process_entry(struct merge_options *opt,\n> > */\n> > struct conflict_info *new_ci;\n> > const char *branch;\n> > - const char *old_path = path;\n> > int i;\n> >\n> > assert(ci->merged.result.mode == S_IFDIR);\n>\n> This looks like you are trying to make sure you always have the\n> \"original\" path available, meaning a path corresponding to a file from\n> one of the commits being merged. This doesn't quite accomplish that,\n> though. Some questions:\n>\n> * What about cases where \"path\" is a location that exists only in\n> the merge base? (rename/rename(1to2) is an example where this will\n> come up)\n\nThat's a good point. I will need to invent some way to convey all three\noriginal file names in that case, not just one.\n\n> * What about cases where \"path\" is a location that does not exist\n> anywhere in history? (directory renames yield such a result. e.g.\n> side A renames olddir/ -> newdir/, and side B adds olddir/foo, then\n> \"path\" will be newdir/foo)\n\nHmm. As mentioned, I want to focus on the scenario with disabled renames,\nbut not shut the door on rename scenarios like this one.\n\nSo maybe I need to collect the original names in this instance, too, with\nsome (machine-parseable) indicator talking about the nature of that\norigin.\n\n> > @@ -3838,10 +3838,10 @@ static void process_entry(struct merge_options *opt,\n> > strmap_put(&opt->priv->paths, path, new_ci);\n> >\n> > path_msg(opt, CONFLICT_FILE_DIRECTORY, 0,\n> > - path, old_path, NULL, NULL,\n> > + orig_path, path, NULL, NULL,\n> > _(\"CONFLICT (file/directory): directory in the way \"\n> > \"of %s from %s; moving it to %s instead.\"),\n> > - old_path, branch, path);\n> > + orig_path, branch, path);\n> > /*\n> > * Zero out the filemask for the old ci. At this point, ci\n> > @@ -3921,7 +3921,7 @@ static void process_entry(struct merge_options *opt,\n> >\n> > if (rename_a && rename_b) {\n> > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > - path, a_path, b_path, NULL,\n> > + orig_path, a_path, b_path, NULL,\n> > _(\"CONFLICT (distinct types): %s had \"\n> > \"different types on each side; \"\n> > \"renamed both of them so each can \"\n> > @@ -3929,7 +3929,7 @@ static void process_entry(struct merge_options *opt,\n> > path);\n> > } else {\n> > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > - path, rename_a ? a_path : b_path,\n> > + orig_path, rename_a ? a_path : b_path,\n> > NULL, NULL,\n> > _(\"CONFLICT (distinct types): %s had \"\n> > \"different types on each side; \"\n>\n> I think for both of these last two cases, path == orig_path, so you\n> haven't actually made an effective change here.\n\nNot quite. If `ci->df_conflict && ci->merged.result.mode != 0`, then we\nassign `path = unique_path(opt, path, branch);` and `path` no longer\nrefers to the same string as `orig_path`.\n\nIt is possible that there is no actual code path that hits that clause and\nthen also hits the `CONFLICT_DISTINC_MODES` calls, but I think there is:\nif a file has changed to a symlink in one branch _and_ was replaced by a\ndirectory in the other branch.\n\n> (And, as above, I want path to always be the primary_path passed to\n> path_msg() as other code was written with that assumption in mind.)\n>\n> > @@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n> > if (S_ISGITLINK(merged_file.mode))\n> > reason = _(\"submodule\");\n> > path_msg(opt, CONFLICT_CONTENTS, 0,\n> > - path, NULL, NULL, NULL,\n> > + orig_path, NULL, NULL, NULL,\n> > _(\"CONFLICT (%s): Merge conflict in %s\"),\n> > reason, path);\n> > }\n>\n> Here's another case where path == orig_path, so you haven't made an\n> effective change.\n\nI cannot think of a case where a file/directory conflict can also result\nin a content conflict, so at least in the regular 3-way merge, I think\nyou're right and this changed line does the same as before in all cases.\n\n> But this one highlights something interesting...\n>\n> In addition to the fact that path/orig_path may be a location that\n> didn't exist on either side of history, there might actually be two\n> relevant paths from the two different sides of history which are\n> involved in the content merge, with neither of them being path or\n> orig_path. Let me break it down, starting with a simpler two path\n> case:\n>\n> If we have a standard rename, e.g. foo -> bar, and both sides modified\n> the file but did so in a conflicting manner, then we will end up in\n> this chunk of code. The conflict info emitted by merge-tree -z which\n> is always of the form\n> NULNULNULNULNULNUL\n> will in this particular case be\n> 1barAuto-mergingAuto-merging bar\n> 1barCONFLICT (contents)CONFLICT (content): Merge\n> conflict in bar\n> Neither of these messages has any mention of \"foo\", despite the fact\n> that \"foo\" was the name of the file in both the merge base and one\n> side, and \"bar\" was only the name of the file on one side.\n\nI guess so, as no conflict message will mention the rename: it does not\nconflict.\n\n> Now, let's make it more interesting. side A modifies foo, and renames\n> olddir/->newdir/. side B modifies foo in a conflicting manner, and\n> renames foo->olddir/bar. Our `merge-tree -z` conflict information\n> emitted will be of the form\n> 1newdir/barAuto-mergingAuto-merging newdir/bar\n> 1newdir/barCONFLICT (contents)CONFLICT (content):\n> Merge conflict in newdir/bar\n> For this more interesting case, the only files that existed were \"foo\"\n> and \"olddir/bar\", neither of which are mentioned in the conflict info.\n> The conflict info only reports on \"newdir/bar\".\n>\n> And for both of these examples, your patch doesn't change the existing\n> behavior at all since path == orig_path for this hunk of the patch.\n\nThat is true, too.\n\nWhich makes me think that the consuming code I have to work with will need\nsome elaborate changes when it wants to start supporting renames.\n\nBut that's not the stage where I'm at right now.\n\n> > @@ -4067,7 +4067,7 @@ static void process_entry(struct merge_options *opt,\n> > */\n> > } else {\n> > path_msg(opt, CONFLICT_MODIFY_DELETE, 0,\n> > - path, NULL, NULL, NULL,\n> > + orig_path, path, NULL, NULL,\n> > _(\"CONFLICT (modify/delete): %s deleted in %s \"\n> > \"and modified in %s. Version %s of %s left \"\n> > \"in tree.\"),\n>\n> Adding orig_path here after path would make sense, though it's still\n> incomplete. For example if a path was modified and renamed on one\n> side, and deleted on the other, then we'd get to this hunk, and the\n> modify/delete would omit information about the other path. Further,\n> directory renames could be at play, such that neither orig_path nor\n> path would correspond to a filename in either of the commits being\n> merged.\n>\n> There are also other path_msg() calls that should probably be informed\n> of additional filenames due to simple renames: CONFLICT_CONTENTS (as\n> noted above), CONFLICT_BINARY, and INFO_AUTO_MERGING.\n>\n> Going beyond simple renames, if you want \"fully original\" filenames,\n> then we'd probably have to audit nearly all the path_msg() calls for\n> when directory renames are possible, and modify the directory renaming\n> code to plumb the original name through to this layer.\n>\n> And even if we do all that, then we still aren't matching what I think\n> you are expecting where every conflicted higher order entry we report\n> maps to a single : pairing. We may have a higher\n> stage conflict entry that represents merged contents of three\n> different filecontents from three different filenames. For example,\n> if someone has the following setup:\n>\n> Commit O: one file named \"original\"\n> Commit A: modify original + rename to primordial\n> Commit B: add file named primordial, modify original differently +\n> rename it to primary\n>\n> where A & B both have O as a parent or ancestor, and O is the unique\n> merge base of A & B. Now if we try to merge A & B, then the index\n> will have four higher order entries (using \n> , as we'd see from either `ls-files -u` style\n> output or from the first part of merge-tree -z output):\n>\n> 100644 1 original\n> 100644 B:primary> 3 primary\n> 100644 B:primary> 2 primordial\n> 100644 3 primordial\n>\n> Note that the two middle entries, despite having one name attached to\n> them, actually come from a merge of three different files. Further,\n> that hash for those two entries will not have yet appeared as the\n> contents for any blob in any revision of history, and will in fact\n> represent a file with conflict markers (even if none of the original\n> files -- O:original, A:primodial, B:primary -- had any conflicts).\n> And, the version of \"primodial\" in the merge-result-tree would likely\n> have nested conflict markers, despite O being a unique merge base\n> (meaning we get nested conflict markers without merging merge bases).\n>\n> Trying to have all the conflict output from merge-tree map to some\n> original file the user had from one of their sides of history is thus\n> a goal that merge-ort is nowhere close to satisfying. I'm not even\n> sure it's tractable.\n\nSure, but I am getting more and more convinced that we will need to\nprovide \"sort of\" the original name. Somehow.\n\nThat is, if there were successful (directory) renames happening before the\n3-way merge that resulted in a conflict, then I do not really care if the\n\"original original\" filename isn't shown. But I do care that the filename\nbefore munging is shown (and yes, we should probably also show the munged\nfilename).\n\nAnd since recursive merges can result in multiple levels of munging, it\ngets harder to explain what I want, but I think you'll get the idea that\nthe \"pre-munged\" filename of one conflict could be the \"post-munged\"\nfilename of a conflict that happened at a lower level of that recursion.\n\n> However, there is a much smaller scoped goal that we could easily\n> achieve: providing additional filename information in the path_msg()\n> calls, much like you start to do with the modify/delete case.\n\nSure, but we also need an indicator somehow what purpose each of those\nadditional filenames serve. I guess we could make it implicit, just like\nwe make it implicit that the first path in a file/directory conflict\nrefers to the file's name and the second to the directory's. But it makes\nme a bit uneasy and I wish we had more like a structure instead of just a\nstring that we could print out. That would make it possible to convey\nfrom where (as in \"which branch?\") the file or directory came.\n\n> > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> > index 35f94957fce..bc580a242ac 100755\n> > --- a/t/t4069-remerge-diff.sh\n> > +++ b/t/t4069-remerge-diff.sh\n> > @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> > test_expect_success 'remerge-diff with non-content conflicts' '\n> > git log -1 --oneline resolution >tmp &&\n> > cat <<-EOF >>tmp &&\n> > + diff --git a/file_or_directory b/file_or_directory\n> > + remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > similarity index 100%\n> > rename from file_or_directory~HASH (side1)\n> > rename to wanted_content\n> > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > diff --git a/letters b/letters\n> > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > diff --git a/letters_side2 b/letters_side2\n> > @@ -168,7 +169,7 @@ test_expect_success 'remerge-diff with non-content conflicts' '\n> > test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no diff content' '\n> > git log -1 --oneline resolution >tmp &&\n> > cat <<-EOF >>tmp &&\n> > - diff --git a/file_or_directory~HASH (side1) b/file_or_directory~HASH (side1)\n> > + diff --git a/file_or_directory b/file_or_directory\n> > remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > diff --git a/letters b/letters\n> > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > @@ -184,14 +185,13 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif\n> > test_cmp expect actual\n> > '\n> >\n> > -test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' '\n> > +test_expect_success 'remerge-diff w/ diff-filter=R: relevant file' '\n> > git log -1 --oneline resolution >tmp &&\n> > cat <<-EOF >>tmp &&\n> > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > similarity index 100%\n> > rename from file_or_directory~HASH (side1)\n> > rename to wanted_content\n> > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > EOF\n> > # We still have some sha1 hashes above; rip them out so test works\n> > # with sha256\n>\n> All of these look like regressions to me; you discussed these in a\n> separate email so I'll go into detail in response to that one.\n\nThank you for being so diligent. I always appreciate it when I see a\nwell-written, obviously well-edited mail. I find it very considerate.\n\nYou did a good job at convincing me that my approach is sub-optimal, and\nthat we need at least one new flag to get the information that I need to\nproduce in my use case because the caller requires it to present a nice\nconflict resolution UI to the users.\n\nBack to the drawing table for me, I guess.\n\nThank you!\nDscho\n"},{"id":"461832","messageId":"CABPp-BF24DO7vfYf856gXcfP7pT4moaiz-BUazCwYngmVCP5Pg@mail.gmail.com","threadId":"58326","inReplyTo":"9p08998o-n88r-2nno-8703-s99o5qpn9o35@tzk.qr","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-23T06:24:00Z","receivedAt":"2022-08-23T06:25:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 22, 2022 at 1:12 PM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Sat, 20 Aug 2022, Elijah Newren wrote:\n>\n> > On Thu, Aug 18, 2022 at 11:57 PM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > >\n> > > From: Johannes Schindelin \n> > >\n> > > When printing the messages in a machine-readable format (i.e. in `-z`\n> > > mode), the intention is to support third-party software that wants to\n> > > present the conflicts in a \"pretty\" way to human readers.\n> > >\n> > > To that end, `git merge-tree -z` prefixes the conflict messages by a\n> > > variable-size list of paths and a conflict identifier. This list and\n> > > this identifier are intended to be machine-parseable, the conflict\n> > > message is sort of free-form and not intended to be parsed.\n> > >\n> > > Keeping in mind that the intended use case is to have third-party\n> > > software use `git merge-tree` to perform worktree-less merges and then\n> > > present the conflicts (if any) to human readers, it makes much more\n> > > sense to show the original file names of the involved files instead of\n> > > the ones we internally munged to allow for creating a tree object that\n> > > contains entries for both sides of the conflict (which requires them to\n> > > have different names).\n> >\n> > I can see how this would be useful to you if it could be\n> > achieved...well, assuming you replaced \"instead of\" with \"in addition\n> > to\". It'd be a huge undertaking to achieve, though; far beyond what\n> > this series attempts.\n>\n> I feared you would say that :-)\n>\n> And I realize that I should have lead with the context that is relevant to\n> understand what I am driving at.\n>\n> For now, all I am trying to do is to verify that we can achieve identical\n> results between a new code path that uses `git merge-tree` (with options)\n> and an existing code path that uses libgit2 to perform a 3-way merge.\n>\n> To that end, I have added some patches to optionally disable rename\n> detection, to allow specifying a merge base instead of performing a full\n> recursive merge, to optionally detect whether the `MERGE_HEAD` is already\n> reachable from `HEAD` and in that case exit with a specific exit status,\n> among other things.\n>\n> And yes, I realize that I \"only\" benefit from the worktree-less nature of\n> `merge-ort` so far, and not from any other aspects `merge-ort` offers.\n>\n> One thing I need from the result is a list of conflicts in the same format\n> as we would have received if writing out an index with conflicts and then\n> parsing just the entries with non-zero stages.\n\nThanks, this is good context. Especially this last paragraph, where\nyou say you want the non-zero stage entries from what would be written\nto the index. I'll return to this many times.\n\n> So for now, I do not care _so_ much about rename conflicts, or conflicts\n> resulting from merging merge base candidates. I don't want to slam the\n> door shut, of course, but due to a teeny tiny bit of time pressure, I want\n> to focus very, very much on the libgit2-like operation for now :-)\n>\n> What I do need, however, is the unmunged filenames as well as the\n> corresponding file modes and the object hashes.\n\nIf you're under time pressure...don't you already have all the\ninformation you need? Why can't you just parse the and sections to get it? I'll come back\nto this again with the testcase you highlighted in your series, at the\nvery end of the email.\n\n> > I think your attempt here may be based on the presumption that any\n> > hash in a higher stage in the index maps directly to the hash of some\n> > : pair from one of the commits being merged. That\n> > assumption holds in simple cases, but does not hold generally.\n> > merge-ort has a very strong \"(at most) 3 stages\" assumption for any\n> > conflict, driven by the need to ultimately represent conflicts in the\n> > index, which is pervasive in the code.\n>\n> This is actually very helpful information, this had not sunk in yet.\n>\n> So when encountering a \"stage 3 before stages 1 & 2\" situation arises,\n> like in the test case I added, you're saying that I could simply reorder\n> those and be done and not have to expect that the `stage 1 & 2` part is\n> followed by _another_ stage 3 part for the same name?\n>\n> Except, of course, that I need the original file names, and the invariant\n> of at most 3 stages holds true only when taking munged file names into\n> account, right?\n\nYou are correct that ort is never going to present two \"stage 3\"\nentries for any path it reports.\n\nstages are very much an index-related concept, and always associated\nwith potentially munged filenames in ort. This importantly means that\nthere is no such thing as \"the unmunged filename\" for some index\nentries.\n\nIn order to achieve the \"at most 3 stages\" assumption plastered all\nover ort, it has to compress information from more than 3\n: pairs down to at most 3. So some of those\nnon-zero stage index entries do *not* correspond to a file from\nhistory. Any request to get \"the original filename\" would somehow\nhave to respond with up to three filenames instead. (And likewise,\nany request to get \"the original file contents from one of the sides\nof history\" would have to respond with the content of up to three\nfiles instead.)\n\n\nI know, I know, you don't care because you're not dealing with renames\nright now. But if you're modifying the code, it has to be in a way\nthat doesn't paint us into a corner that doesn't work for renames.\n\n> > Trying to create such a direct mapping between higher stage conflicts\n> > and : pairs from the commits being merged would\n> > require breaking that assumption. That would mean getting rid of many of\n> > the hardcoded array sizes of 3, as well as modifying who knows how many\n> > portions of logic built around this assumption.\n> >\n> > Your attempt also seems to be based on the presumption that the\n> > original value of \"path\" as passed to process_entry(), represents a\n> > path from one of the two sides of history. In simple cases, it does.\n> > But the assumption does not generally hold.\n>\n> Heh, in my case it always holds because I disable rename detection. The\n> only time when multiple file names are involved is in case of\n> file/directory conflicts and the like. In which case I still need the\n> original filename.\n>\n> But I would contend that we need to know the original file name(s) even in\n> the case of renames, so that we can present the users with all the\n> information pertinent to the conflict, to allow them to resolve it (so\n> that they have the full context).\n\nTo reiterate, \"the original file name(s)\" is an ill-posed problem. It\nonly exists for some entries. (Same for \"the original file contents\")\n ...well, unless you rewire ort completely and toss out the \"at most 3\nstages\" assumption.\n\n> > Further...\n> >\n> > > So let's mention the original file names prominently, as first item in\n> > > that variable-size list of paths.\n> >\n> > This is in conflict with the needs of other uses of merge-ort:\n> > * When the user is running an active merge with conflicts written to\n> > the index and working tree, the filenames where the conflicts are\n> > recorded may not match the original filenames, and the filenames where\n> > things are recorded is more useful and thus primary.\n> > * For remerge-diff, the filename where the conflict would be placed\n> > is important because that's where stuff ends up. Any changes the user\n> > made for the final merge is relative to that munged path, so that\n> > munged path is the important bit, not the original path.\n>\n> Okay, I understand now.\n>\n> So basically, if I need different/additional information, I will _have_ to\n> introduce a new flag to trigger code paths that produce that.\n>\n> And my changes to the remerge test cases merely demonstrate precisely that\n> need.\n>\n> > Adding additional paths to path_msg() to cover other paths relevant to\n> > the conflict make sense, but the primary path really needs to be left\n> > as-is for these other cases.\n>\n> I fear that I will need much, much more than just the additional paths ;-)\n> So far, I am parsing the `ls-files -u`-like part so that I get mode and\n> hash information, but eventually I want to parse that out of the messages\n> part and that only produces lists of paths so far...\n\nList of paths + conflict type + detailed conflict message, right?\n\n> And a simple, single list of paths is not enough to convey the information\n> that I need. Concretely, I need:\n>\n> - the original file name(s) involved in the conflict,\n\nTrying to get the original file name(s) may well cause you to need to\ninclude additional conflicts, since they were the cause of the name\nmunging. So, are you displaying multiple conflicts at once, or just\ngathering the extra names and printing them without the extra context?\n If you do display multiple conflicts at once, are you focusing on one\nconflict and printing the necessary extra conflicts and paths to\nunderstand that one, and then potentially again listing some of those\npaths and conflicts again separately?\n\nAnd, to make it worse, what about cases where there is not a clear\nline between where one conflict starts and another ends? Do you start\njust agglomerating arbitrarily many conflicts together? And get the\noriginal names for all of them or some subset? And then do you print\nall the \"original file name(s)\" once for the whole set, or once per\nconflict within it?\n\nI'll list some examples of what I mean below with some different\npossibilities...\n\n> - the modes and object hashes corresponding to the different stages\n\nWhich you can get by parsing the section, and\ncomparing to the names from your list of paths, right?\n\n> - maybe some provenance information in the future, in case of complex\n> rename/merge recursions\n>\n> In particular when presenting a file/directory conflict, it will be\n> important to know what stage refers to the file and what stage to the\n> directory (so as to know whether the respective object hash refers to a\n> file or even symlink and needs to be read as a blob).\n\nIs this follow-on paragraph meant to be an example of provenance\ninformation? Or a separate thought? I'm struggling a bit to follow.\n\nThe stage and mode and hash information all come from the section (i.e. the section with `ls-files -u`-like output). You\nalready have that available. And the mode gives you the\nfile/directory/symlink/submodule type.\n\nNo conflict entries for a \"tree\" are presented (because it'd be kind\nof confusing to have a conflict entry for a tree as well as for\nseveral files under the tree, and allow the user to resolve them\nindependently). So, when there's a file/directory conflict in\nparticular, there will not be a stage provided to represent the\ndirectory.\n\nBut I'm not sure how relevant that all is; it feels like I'm missing\nwhat you mean here.\n\n> > > Note: For the modify/delete conflict type, we used to mention _only_ the\n> > > munged name in that path list. To allow for tools to read the tree\n> > > object produced by `git merge-tree -z` into a Git index in order to\n> > > resolve the conflicts, it is necessary to list not only the original\n> > > name but _also_ the munged name so that the item with the munged file\n> > > name can be removed from that Git index. Therefore, this patch teaches\n> > > `git merge-tree` to show both the original _and_ the munged name in that\n> > > list.\n> >\n> > This makes sense. Passing additional valid and relevant information\n> > to callers of merge-tree seems beneficial.\n>\n> And now that you mention this, I realize that this should be pulled out\n> into its own patch.\n>\n> > > Also note: This patch changes the output of the remerge diff slightly:\n> > > whereas before, we printed the notice about a file/directory conflict\n> > > under the diff header mentioning the munged file name, we now print it\n> > > under a separate diff header that mentions the original file name.\n> > > That is the explanation why t4301 is touched by this patch.\n> >\n> > Sounds problematic.\n>\n> Fair enough.\n>\n> > > Signed-off-by: Johannes Schindelin \n> > > ---\n> > > merge-ort.c | 14 +++++++-------\n> > > t/t4069-remerge-diff.sh | 8 ++++----\n> > > t/t4301-merge-tree-write-tree.sh | 4 ++--\n> > > 3 files changed, 13 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/merge-ort.c b/merge-ort.c\n> > > index 01f150ef3b5..211f6823e1d 100644\n> > > --- a/merge-ort.c\n> > > +++ b/merge-ort.c\n> > > @@ -3741,6 +3741,7 @@ static void process_entry(struct merge_options *opt,\n> > > struct conflict_info *ci,\n> > > struct directory_versions *dir_metadata)\n> > > {\n> > > + const char *orig_path = path;\n> > > int df_file_index = 0;\n> > >\n> > > VERIFY_CI(ci);\n> > > @@ -3787,7 +3788,6 @@ static void process_entry(struct merge_options *opt,\n> > > */\n> > > struct conflict_info *new_ci;\n> > > const char *branch;\n> > > - const char *old_path = path;\n> > > int i;\n> > >\n> > > assert(ci->merged.result.mode == S_IFDIR);\n> >\n> > This looks like you are trying to make sure you always have the\n> > \"original\" path available, meaning a path corresponding to a file from\n> > one of the commits being merged. This doesn't quite accomplish that,\n> > though. Some questions:\n> >\n> > * What about cases where \"path\" is a location that exists only in\n> > the merge base? (rename/rename(1to2) is an example where this will\n> > come up)\n>\n> That's a good point. I will need to invent some way to convey all three\n> original file names in that case, not just one.\n\nDo you? The CONFLICT_RENAME_RENAME output in the section will have\n 3\n path-from-merge-base\n path-from-side1\n path-from-side2\n CONFLICT (rename/rename)\n \nSo the user of merge-tree -z already has the information to tie these\nthree filenames together. Why does process_entry() also need to have\nthis information and provide it to subsequent calls?\n\n(Note that for a rename/rename(1to2) without other conflicts involved,\nprocess_entry() will be called for each path involved, and you'll get\na entry for each. One of those is from the merge\nbase. So you'll have 3 entries, as well as the above\n to be able to tie them together.)\n\nHowever, my comments in the last 2 paragraphs gets us further away\nfrom my original question from my last email.\n\nIn my previous email, I was trying to ask whether you were\nconcentrating on being able to say which \"side\" or \"branch\" a file was\nfrom. Index entries at stage 1 don't come from either side. Is that\nproblematic to you?\n\n> > * What about cases where \"path\" is a location that does not exist\n> > anywhere in history? (directory renames yield such a result. e.g.\n> > side A renames olddir/ -> newdir/, and side B adds olddir/foo, then\n> > \"path\" will be newdir/foo)\n>\n> Hmm. As mentioned, I want to focus on the scenario with disabled renames,\n> but not shut the door on rename scenarios like this one.\n>\n> So maybe I need to collect the original names in this instance, too, with\n> some (machine-parseable) indicator talking about the nature of that\n> origin.\n\nWhich I think you can get from the that\nalready exists... (again, example at the end)\n\n> > > @@ -3838,10 +3838,10 @@ static void process_entry(struct merge_options *opt,\n> > > strmap_put(&opt->priv->paths, path, new_ci);\n> > >\n> > > path_msg(opt, CONFLICT_FILE_DIRECTORY, 0,\n> > > - path, old_path, NULL, NULL,\n> > > + orig_path, path, NULL, NULL,\n> > > _(\"CONFLICT (file/directory): directory in the way \"\n> > > \"of %s from %s; moving it to %s instead.\"),\n> > > - old_path, branch, path);\n> > > + orig_path, branch, path);\n> > > /*\n> > > * Zero out the filemask for the old ci. At this point, ci\n> > > @@ -3921,7 +3921,7 @@ static void process_entry(struct merge_options *opt,\n> > >\n> > > if (rename_a && rename_b) {\n> > > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > > - path, a_path, b_path, NULL,\n> > > + orig_path, a_path, b_path, NULL,\n> > > _(\"CONFLICT (distinct types): %s had \"\n> > > \"different types on each side; \"\n> > > \"renamed both of them so each can \"\n> > > @@ -3929,7 +3929,7 @@ static void process_entry(struct merge_options *opt,\n> > > path);\n> > > } else {\n> > > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > > - path, rename_a ? a_path : b_path,\n> > > + orig_path, rename_a ? a_path : b_path,\n> > > NULL, NULL,\n> > > _(\"CONFLICT (distinct types): %s had \"\n> > > \"different types on each side; \"\n> >\n> > I think for both of these last two cases, path == orig_path, so you\n> > haven't actually made an effective change here.\n>\n> Not quite. If `ci->df_conflict && ci->merged.result.mode != 0`, then we\n> assign `path = unique_path(opt, path, branch);` and `path` no longer\n> refers to the same string as `orig_path`.\n>\n> It is possible that there is no actual code path that hits that clause and\n> then also hits the `CONFLICT_DISTINC_MODES` calls, but I think there is:\n> if a file has changed to a symlink in one branch _and_ was replaced by a\n> directory in the other branch.\n\nThe \"ci->filemask >= 6\" check that guards this code prevents any such\npossibility. In particular, from your example, if it was changed to a\nsymlink in one branch and replaced by a directory in the other branch,\nthen ci->filemask is either 3 or 5, which would prevent the code from\ngetting here.\n\n> > (And, as above, I want path to always be the primary_path passed to\n> > path_msg() as other code was written with that assumption in mind.)\n> >\n> > > @@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n> > > if (S_ISGITLINK(merged_file.mode))\n> > > reason = _(\"submodule\");\n> > > path_msg(opt, CONFLICT_CONTENTS, 0,\n> > > - path, NULL, NULL, NULL,\n> > > + orig_path, NULL, NULL, NULL,\n> > > _(\"CONFLICT (%s): Merge conflict in %s\"),\n> > > reason, path);\n> > > }\n> >\n> > Here's another case where path == orig_path, so you haven't made an\n> > effective change.\n>\n> I cannot think of a case where a file/directory conflict can also result\n> in a content conflict, so at least in the regular 3-way merge, I think\n> you're right and this changed line does the same as before in all cases.\n>\n> > But this one highlights something interesting...\n> >\n> > In addition to the fact that path/orig_path may be a location that\n> > didn't exist on either side of history, there might actually be two\n> > relevant paths from the two different sides of history which are\n> > involved in the content merge, with neither of them being path or\n> > orig_path. Let me break it down, starting with a simpler two path\n> > case:\n> >\n> > If we have a standard rename, e.g. foo -> bar, and both sides modified\n> > the file but did so in a conflicting manner, then we will end up in\n> > this chunk of code. The conflict info emitted by merge-tree -z which\n> > is always of the form\n> > NULNULNULNULNULNUL\n> > will in this particular case be\n> > 1barAuto-mergingAuto-merging bar\n> > 1barCONFLICT (contents)CONFLICT (content): Merge\n> > conflict in bar\n> > Neither of these messages has any mention of \"foo\", despite the fact\n> > that \"foo\" was the name of the file in both the merge base and one\n> > side, and \"bar\" was only the name of the file on one side.\n>\n> I guess so, as no conflict message will mention the rename: it does not\n> conflict.\n\nRight, so we should probably also produce informational messages for\nnormal renames; adding a path_msg() call to the \"/* normal rename */\"\ncode path should do it. Though, I think we might only want such\nmessages when merge-tree -z is being used. (Or perhaps if some kind\nof higher verbosity is requested?)\n\n> > Now, let's make it more interesting. side A modifies foo, and renames\n> > olddir/->newdir/. side B modifies foo in a conflicting manner, and\n> > renames foo->olddir/bar. Our `merge-tree -z` conflict information\n> > emitted will be of the form\n> > 1newdir/barAuto-mergingAuto-merging newdir/bar\n> > 1newdir/barCONFLICT (contents)CONFLICT (content):\n> > Merge conflict in newdir/bar\n> > For this more interesting case, the only files that existed were \"foo\"\n> > and \"olddir/bar\", neither of which are mentioned in the conflict info.\n> > The conflict info only reports on \"newdir/bar\".\n> >\n> > And for both of these examples, your patch doesn't change the existing\n> > behavior at all since path == orig_path for this hunk of the patch.\n>\n> That is true, too.\n>\n> Which makes me think that the consuming code I have to work with will need\n> some elaborate changes when it wants to start supporting renames.\n>\n> But that's not the stage where I'm at right now.\n>\n> > > @@ -4067,7 +4067,7 @@ static void process_entry(struct merge_options *opt,\n> > > */\n> > > } else {\n> > > path_msg(opt, CONFLICT_MODIFY_DELETE, 0,\n> > > - path, NULL, NULL, NULL,\n> > > + orig_path, path, NULL, NULL,\n> > > _(\"CONFLICT (modify/delete): %s deleted in %s \"\n> > > \"and modified in %s. Version %s of %s left \"\n> > > \"in tree.\"),\n> >\n> > Adding orig_path here after path would make sense, though it's still\n> > incomplete. For example if a path was modified and renamed on one\n> > side, and deleted on the other, then we'd get to this hunk, and the\n> > modify/delete would omit information about the other path. Further,\n> > directory renames could be at play, such that neither orig_path nor\n> > path would correspond to a filename in either of the commits being\n> > merged.\n> >\n> > There are also other path_msg() calls that should probably be informed\n> > of additional filenames due to simple renames: CONFLICT_CONTENTS (as\n> > noted above), CONFLICT_BINARY, and INFO_AUTO_MERGING.\n> >\n> > Going beyond simple renames, if you want \"fully original\" filenames,\n> > then we'd probably have to audit nearly all the path_msg() calls for\n> > when directory renames are possible, and modify the directory renaming\n> > code to plumb the original name through to this layer.\n> >\n> > And even if we do all that, then we still aren't matching what I think\n> > you are expecting where every conflicted higher order entry we report\n> > maps to a single : pairing. We may have a higher\n> > stage conflict entry that represents merged contents of three\n> > different filecontents from three different filenames. For example,\n> > if someone has the following setup:\n> >\n> > Commit O: one file named \"original\"\n> > Commit A: modify original + rename to primordial\n> > Commit B: add file named primordial, modify original differently +\n> > rename it to primary\n> >\n> > where A & B both have O as a parent or ancestor, and O is the unique\n> > merge base of A & B. Now if we try to merge A & B, then the index\n> > will have four higher order entries (using \n> > , as we'd see from either `ls-files -u` style\n> > output or from the first part of merge-tree -z output):\n> >\n> > 100644 1 original\n> > 100644 > B:primary> 3 primary\n> > 100644 > B:primary> 2 primordial\n> > 100644 3 primordial\n> >\n> > Note that the two middle entries, despite having one name attached to\n> > them, actually come from a merge of three different files. Further,\n> > that hash for those two entries will not have yet appeared as the\n> > contents for any blob in any revision of history, and will in fact\n> > represent a file with conflict markers (even if none of the original\n> > files -- O:original, A:primodial, B:primary -- had any conflicts).\n> > And, the version of \"primodial\" in the merge-result-tree would likely\n> > have nested conflict markers, despite O being a unique merge base\n> > (meaning we get nested conflict markers without merging merge bases).\n> >\n> > Trying to have all the conflict output from merge-tree map to some\n> > original file the user had from one of their sides of history is thus\n> > a goal that merge-ort is nowhere close to satisfying. I'm not even\n> > sure it's tractable.\n>\n> Sure, but I am getting more and more convinced that we will need to\n> provide \"sort of\" the original name. Somehow.\n\nCan I ask in more detail what you want to present to a user? Maybe\nit'd help frame the conversation more.\n\nLet me provide two examples, each with lots of alternatives, to see if\nany are the kind of output you want to be able to present to a user\n(and assume in each case it's not just a block message but you have\nsome kind of programmatic knowledge of the components referenced in\nthe message):\n\nExample 1.\n\nAre you thinking of something like \"The following files were involved\nin a modify/delete conflict: foo, olddir/bar, newdir/bar,\nnewdir/bar~B\", followed by three other conflict messages that also involve\nthose same files?\n\nOr, more like: \"The following files were involved in a directory\nrename, and in a rename/delete conflict, and in a file/directory\nconflict, and in a modify/delete conflict: foo, olddir/bar,\nnewdir/bar, newdir/bar~B\"\n\nOr, more like: \"The following files were involved in a directory\nrename, and in a rename/delete conflict, and in a file/directory\nconflict, and in a modify/delete conflict: foo (from side A),\nolddir/bar (from side B), newdir/bar (munged name), newdir/bar~B\n(munged name)\"\n\nOr, more like: \"The file newdir/bar~B has a modify/delete conflict.\nIt was renamed to newdir/bar~B from newdir/bar because of a\nfile/directory conflict. It was renamed to newdir/bar from olddir/bar\nbecause of a directory rename. It was renamed from foo to olddir/bar\non side B. foo was deleted on side A.\"\n\nOr, more like: \"foo was deleted on side A, and renamed to olddir/bar\non side B. olddir/bar was renamed to newdir/bar because of a\ndirectory rename. newdir/bar was renamed to newdir/bar~B because of a\nfile/directory conflict. newdir/bar~B has a modify/delete conflict.\"\n\nOr something completely different?\n\n\nExample 2.\n\nAre you thinking of something like: \"The following files were involved\nin an add/add conflict: file-one, file-two, file-three, file-four, and\nfile-five\" ?\n\nOr, is it more like: \"The following files were involved in an add/add\nconflict: file-one (from branch A), file-two (from merge base),\nfile-three (from branch A), file-three (from branch B), file-four\n(from merge base), and file-five (from branch B)\" ?\n\nOr, is it more like: \"The following files were involved in an add/add\nconflict: file-three (from [see details]) and file-three (from [see\ndetails]). Note that file-three was part of a rename/rename(1to2)\nconflict, which involved file-one (from branch A), file-two (from\nmerge base), and file-three (from branch B). Note that file-three was\npart of a rename/rename(1to2) conflict, which involved file-three\n(from branch A), file-four (from merge base), and file-five (from\nbranch B)\"\n\nOr, is it more like: \"file-three was part of a rename/rename(1to2)\nconflict, which involved file-one (from branch A), file-two (from\nmerge base), and file-three (from branch B). file-three was part of a\nrename/rename(1to2) conflict, which involved file-three (from branch\nA), file-four (from merge base), and file-five (from branch B).\nfile-three and file-three are involved in an add/add conflict\"\n\n(Of course, all of this example is assuming we're just focusing on the\nultimate file-three vs file-three add/add conflict, not the additional\nconflicts that file-one and file-five may be involved in.)\n\nOr are you wanting to display something completely different?\n\n\n\nI think we already have enough info to do most of the above.\n\nThe main shortcoming is just that path_msg() calls for various\nrename-oriented conflicts might also need to be augmented with\nbranches (though perhaps that's natural to do if we consider the list\npassed to it to be a list of names rather than just a list of\npathnames; then we can just add the branch(s) to the list)\n\n> That is, if there were successful (directory) renames happening before the\n> 3-way merge that resulted in a conflict, then I do not really care if the\n> \"original original\" filename isn't shown. But I do care that the filename\n> before munging is shown (and yes, we should probably also show the munged\n> filename).\n\nNo, directory renames are part of the filename munging done *by* the\nthree-way merge (not before it). That munging, though, applies before\nprocess_entry() is called. Here's an example (from Example 1 above):\n\n Commit O: foo, olddir/{a,b,c}\n Commit A: delete foo, rename olddir/ -> newdir/, add newdir/bar/file\n Commit B: modify foo & rename foo -> olddir/bar\n\nHere, A had a file named \"foo\". B had a file named \"olddir/bar\"\n(renamed from foo). The merge notices the olddir/ -> newdir/\ndirectory rename in A and munges the filename further to \"newdir/bar\".\nThe merge also notices that there is a directory in the way of\n\"newdir/bar\", and thus munges the filename even further to\n\"newdir/bar~B\". This would result in two higher order stages in the\n`ls-files -u`-style output, both for newdir/bar~B.\n\nThe \"original filename\" for one of those is \"foo\" and for the other is\n\"olddir/bar\". However, the value for \"path\" passed to the lone\n\"process_entry()\" call will be \"newdir/bar\". The value of \"path\"\nafter the call to unique_path() will be \"newdir/bar~B\". Neither\n\"newdir/bar\" or \"newdir/bar~B\" is an original filename.\n\n> And since recursive merges can result in multiple levels of munging, it\n> gets harder to explain what I want, but I think you'll get the idea that\n> the \"pre-munged\" filename of one conflict could be the \"post-munged\"\n> filename of a conflict that happened at a lower level of that recursion.\n\nYes, recursing makes things more interesting. But even if you assume\na unique merge-base, you still cannot assume that there is any such\nthing as \"the original filename\" for an entry in the `ls-files -u` output.\n\n> > However, there is a much smaller scoped goal that we could easily\n> > achieve: providing additional filename information in the path_msg()\n> > calls, much like you start to do with the modify/delete case.\n>\n> Sure, but we also need an indicator somehow what purpose each of those\n> additional filenames serve. I guess we could make it implicit, just like\n> we make it implicit that the first path in a file/directory conflict\n> refers to the file's name and the second to the directory's.\n\nYes, I mean the point of the was to provide\nparsable information and this was what I had in mind when I created\nit.\n\nVery minor nitpick, though: the first path refers to the *munged*\nfile's name, and the second path refers to the directory's name. (If\nthe file and directory had different names before our additional\nmunging, we wouldn't have needed that additional munging).\n\n> But it makes\n> me a bit uneasy and I wish we had more like a structure instead of just a\n> string that we could print out. That would make it possible to convey\n> from where (as in \"which branch?\") the file or directory came.\n\nAnd what about other cases where there is no \"the branch\" that a file\ncame from? For example, doing rename + add/add:\n\n Commit O: foo\n Commit A: modify foo + rename foo->bar\n Commit B: modify foo, add unrelated bar\n\nIn this case, we'll have a \"bar\" at stage 3 representing B:bar. We'll\nalso get a three-way merge of O:foo, A:bar, and B:foo, stuck in a\nstage 2 entry under the filename bar. The stage 2 entry for bar did\nnot come from one branch, it represents merged contents that came from\nall three commits.\n\n> > > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> > > index 35f94957fce..bc580a242ac 100755\n> > > --- a/t/t4069-remerge-diff.sh\n> > > +++ b/t/t4069-remerge-diff.sh\n> > > @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> > > test_expect_success 'remerge-diff with non-content conflicts' '\n> > > git log -1 --oneline resolution >tmp &&\n> > > cat <<-EOF >>tmp &&\n> > > + diff --git a/file_or_directory b/file_or_directory\n> > > + remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > > similarity index 100%\n> > > rename from file_or_directory~HASH (side1)\n> > > rename to wanted_content\n> > > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > diff --git a/letters b/letters\n> > > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > > diff --git a/letters_side2 b/letters_side2\n> > > @@ -168,7 +169,7 @@ test_expect_success 'remerge-diff with non-content conflicts' '\n> > > test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no diff content' '\n> > > git log -1 --oneline resolution >tmp &&\n> > > cat <<-EOF >>tmp &&\n> > > - diff --git a/file_or_directory~HASH (side1) b/file_or_directory~HASH (side1)\n> > > + diff --git a/file_or_directory b/file_or_directory\n> > > remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > diff --git a/letters b/letters\n> > > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > > @@ -184,14 +185,13 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif\n> > > test_cmp expect actual\n> > > '\n> > >\n> > > -test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' '\n> > > +test_expect_success 'remerge-diff w/ diff-filter=R: relevant file' '\n> > > git log -1 --oneline resolution >tmp &&\n> > > cat <<-EOF >>tmp &&\n> > > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > > similarity index 100%\n> > > rename from file_or_directory~HASH (side1)\n> > > rename to wanted_content\n> > > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > EOF\n> > > # We still have some sha1 hashes above; rip them out so test works\n> > > # with sha256\n> >\n> > All of these look like regressions to me; you discussed these in a\n> > separate email so I'll go into detail in response to that one.\n>\n> Thank you for being so diligent. I always appreciate it when I see a\n> well-written, obviously well-edited mail. I find it very considerate.\n>\n> You did a good job at convincing me that my approach is sub-optimal, and\n> that we need at least one new flag to get the information that I need to\n> produce in my use case because the caller requires it to present a nice\n> conflict resolution UI to the users.\n\nI'm not so sure you need additional information. Don't the\n and provide enough\ninfo already, for the usecases you are considering? In particular,\nyou were trying to add a testcase in your series for a simple\ndirectory/file conflict, but for such a case you can already determine\noriginal filenames. The and sections of the `merge-tree -z` output for these, after\nreplacing NUL characters with newlines, would be:\n\n 120000 3 origfile\n 100644 1 origfile~branch\n 100644 2 origfile~branch\n\n 2\n origfile\n origfile~branch\n CONFLICT (distinct modes)\n CONFLICT (distinct types): whatever had different types on each\nside; renamed one of them so each can be recorded somewhere.\n\nNow, the \"distinct modes\" conflict always results in filename munging,\nwith both names listed. So, from this output, you know the original\nname is \"origfile\" whenever you see \"origfile~branch\". So now you can\npresent all the `ls-files -u` output from the first section with\nremapping munged filenames back to original filenames.\n\nDoesn't that solve your immediate usecase already, without even\nneeding any merge-ort changes?\n"},{"id":"461996","messageId":"6p614ron-qrsq-p802-1892-o2r4o8n72p44@tzk.qr","threadId":"58326","inReplyTo":"CABPp-BF24DO7vfYf856gXcfP7pT4moaiz-BUazCwYngmVCP5Pg@mail.gmail.com","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-26T15:35:14Z","receivedAt":"2022-08-26T15:35:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nI'll top-post because I will still need much more time to wrap my head\naround everything you said in your very thoughtful reply.\n\nBy the way, have I mentioned before how much I appreciate well-edited,\nwell-crafted mails? It must have taken quite a bit of care and editing to\nwrite this, and I just want to thank you for that.\n\nAs to the problem we are discussing: I am fully on board with you on the\nnecessity to keep the door open for rename detection and recursive merges\nand the zoo of conflict types that comes with it.\n\nAfter thinking about this for a couple of nights, I think I want to tackle\nthe design from the \"other\" side, namely from the UI.\n\nAt this stage, I _think_ it would do well for me to give a more high-level\ncontext as to my motivating factors.\n\nThe thing is, these worktree-less merges do not live in a vacuum. They are\nbacking the Pull Request UI. There exists a relatively simple UI to\nresolve simple merge conflicts, which factors into what I want from `git\nmerge-tree`.\n\nYou've pointed out in the past, very rightfully, that the simple way to\n\"pre-resolve\" conflicts that I have implemented over here is just not good\nenough except for a very, very limited manner of driving `git merge-tree`.\nThese \"pre-resolved conflicts\" are currently essentially a map of \n-> pairs.\n\nNow, to design a much better way to present conflicts as well as allowing\nusers to resolve some of them (the simpler ones, like content conflicts)\nin a web UI, we need `git merge-tree` to give us not an `ls-files\n-u`-style output, that only works reliably for very simple cases.\n\nIn general, I think the design would work best if we parsed the `messages`\npart exclusively on the server side (ignoring the `ls-files`-like output,\nprobably even suppressing it) and if we received enough information to\nrecreate the conflicts from there. Which means that we need the involved\npaths (whether munged or not, I am less and less concerned) together with\nthe modes and OIDs. It would probably be helpful even to provide a tree\nOID for the directory part of any file/directory conflict, where the web\nUI could then present the contents in a tree view to the user.\n\nThis way, we could present multiple conflicts, even multiple conflicts for\nthe very same file, and allow resolving them individually.\n\nAnd we could associate resolutions with the exact list of paths, modes and\nOIDs involved, then teach `merge-tree` to optionally take such resolutions\ninto account, to produce merges with resolved conflicts.\n\nDoing it this way will require substantial changes on the server side, in\nparticular in the UI, which currently does not have any support for\nhandling conflicts involving renames. It will be a lot of work, and I am\ncertain that I will have to pivot many times regarding the design of `git\nmerge-tree` features supporting that UI. I'm excited to start that\njourney.\n\nWhat do you think?\n\nCiao,\nDscho\n\nOn Mon, 22 Aug 2022, Elijah Newren wrote:\n\n> On Mon, Aug 22, 2022 at 1:12 PM Johannes Schindelin\n> wrote:\n> >\n> > Hi Elijah,\n> >\n> > On Sat, 20 Aug 2022, Elijah Newren wrote:\n> >\n> > > On Thu, Aug 18, 2022 at 11:57 PM Johannes Schindelin via GitGitGadget\n> > > wrote:\n> > > >\n> > > > From: Johannes Schindelin \n> > > >\n> > > > When printing the messages in a machine-readable format (i.e. in `-z`\n> > > > mode), the intention is to support third-party software that wants to\n> > > > present the conflicts in a \"pretty\" way to human readers.\n> > > >\n> > > > To that end, `git merge-tree -z` prefixes the conflict messages by a\n> > > > variable-size list of paths and a conflict identifier. This list and\n> > > > this identifier are intended to be machine-parseable, the conflict\n> > > > message is sort of free-form and not intended to be parsed.\n> > > >\n> > > > Keeping in mind that the intended use case is to have third-party\n> > > > software use `git merge-tree` to perform worktree-less merges and then\n> > > > present the conflicts (if any) to human readers, it makes much more\n> > > > sense to show the original file names of the involved files instead of\n> > > > the ones we internally munged to allow for creating a tree object that\n> > > > contains entries for both sides of the conflict (which requires them to\n> > > > have different names).\n> > >\n> > > I can see how this would be useful to you if it could be\n> > > achieved...well, assuming you replaced \"instead of\" with \"in addition\n> > > to\". It'd be a huge undertaking to achieve, though; far beyond what\n> > > this series attempts.\n> >\n> > I feared you would say that :-)\n> >\n> > And I realize that I should have lead with the context that is relevant to\n> > understand what I am driving at.\n> >\n> > For now, all I am trying to do is to verify that we can achieve identical\n> > results between a new code path that uses `git merge-tree` (with options)\n> > and an existing code path that uses libgit2 to perform a 3-way merge.\n> >\n> > To that end, I have added some patches to optionally disable rename\n> > detection, to allow specifying a merge base instead of performing a full\n> > recursive merge, to optionally detect whether the `MERGE_HEAD` is already\n> > reachable from `HEAD` and in that case exit with a specific exit status,\n> > among other things.\n> >\n> > And yes, I realize that I \"only\" benefit from the worktree-less nature of\n> > `merge-ort` so far, and not from any other aspects `merge-ort` offers.\n> >\n> > One thing I need from the result is a list of conflicts in the same format\n> > as we would have received if writing out an index with conflicts and then\n> > parsing just the entries with non-zero stages.\n>\n> Thanks, this is good context. Especially this last paragraph, where\n> you say you want the non-zero stage entries from what would be written\n> to the index. I'll return to this many times.\n>\n> > So for now, I do not care _so_ much about rename conflicts, or conflicts\n> > resulting from merging merge base candidates. I don't want to slam the\n> > door shut, of course, but due to a teeny tiny bit of time pressure, I want\n> > to focus very, very much on the libgit2-like operation for now :-)\n> >\n> > What I do need, however, is the unmunged filenames as well as the\n> > corresponding file modes and the object hashes.\n>\n> If you're under time pressure...don't you already have all the\n> information you need? Why can't you just parse the info> and sections to get it? I'll come back\n> to this again with the testcase you highlighted in your series, at the\n> very end of the email.\n>\n> > > I think your attempt here may be based on the presumption that any\n> > > hash in a higher stage in the index maps directly to the hash of some\n> > > : pair from one of the commits being merged. That\n> > > assumption holds in simple cases, but does not hold generally.\n> > > merge-ort has a very strong \"(at most) 3 stages\" assumption for any\n> > > conflict, driven by the need to ultimately represent conflicts in the\n> > > index, which is pervasive in the code.\n> >\n> > This is actually very helpful information, this had not sunk in yet.\n> >\n> > So when encountering a \"stage 3 before stages 1 & 2\" situation arises,\n> > like in the test case I added, you're saying that I could simply reorder\n> > those and be done and not have to expect that the `stage 1 & 2` part is\n> > followed by _another_ stage 3 part for the same name?\n> >\n> > Except, of course, that I need the original file names, and the invariant\n> > of at most 3 stages holds true only when taking munged file names into\n> > account, right?\n>\n> You are correct that ort is never going to present two \"stage 3\"\n> entries for any path it reports.\n>\n> stages are very much an index-related concept, and always associated\n> with potentially munged filenames in ort. This importantly means that\n> there is no such thing as \"the unmunged filename\" for some index\n> entries.\n>\n> In order to achieve the \"at most 3 stages\" assumption plastered all\n> over ort, it has to compress information from more than 3\n> : pairs down to at most 3. So some of those\n> non-zero stage index entries do *not* correspond to a file from\n> history. Any request to get \"the original filename\" would somehow\n> have to respond with up to three filenames instead. (And likewise,\n> any request to get \"the original file contents from one of the sides\n> of history\" would have to respond with the content of up to three\n> files instead.)\n>\n>\n> I know, I know, you don't care because you're not dealing with renames\n> right now. But if you're modifying the code, it has to be in a way\n> that doesn't paint us into a corner that doesn't work for renames.\n>\n> > > Trying to create such a direct mapping between higher stage conflicts\n> > > and : pairs from the commits being merged would\n> > > require breaking that assumption. That would mean getting rid of many of\n> > > the hardcoded array sizes of 3, as well as modifying who knows how many\n> > > portions of logic built around this assumption.\n> > >\n> > > Your attempt also seems to be based on the presumption that the\n> > > original value of \"path\" as passed to process_entry(), represents a\n> > > path from one of the two sides of history. In simple cases, it does.\n> > > But the assumption does not generally hold.\n> >\n> > Heh, in my case it always holds because I disable rename detection. The\n> > only time when multiple file names are involved is in case of\n> > file/directory conflicts and the like. In which case I still need the\n> > original filename.\n> >\n> > But I would contend that we need to know the original file name(s) even in\n> > the case of renames, so that we can present the users with all the\n> > information pertinent to the conflict, to allow them to resolve it (so\n> > that they have the full context).\n>\n> To reiterate, \"the original file name(s)\" is an ill-posed problem. It\n> only exists for some entries. (Same for \"the original file contents\")\n> ...well, unless you rewire ort completely and toss out the \"at most 3\n> stages\" assumption.\n>\n> > > Further...\n> > >\n> > > > So let's mention the original file names prominently, as first item in\n> > > > that variable-size list of paths.\n> > >\n> > > This is in conflict with the needs of other uses of merge-ort:\n> > > * When the user is running an active merge with conflicts written to\n> > > the index and working tree, the filenames where the conflicts are\n> > > recorded may not match the original filenames, and the filenames where\n> > > things are recorded is more useful and thus primary.\n> > > * For remerge-diff, the filename where the conflict would be placed\n> > > is important because that's where stuff ends up. Any changes the user\n> > > made for the final merge is relative to that munged path, so that\n> > > munged path is the important bit, not the original path.\n> >\n> > Okay, I understand now.\n> >\n> > So basically, if I need different/additional information, I will _have_ to\n> > introduce a new flag to trigger code paths that produce that.\n> >\n> > And my changes to the remerge test cases merely demonstrate precisely that\n> > need.\n> >\n> > > Adding additional paths to path_msg() to cover other paths relevant to\n> > > the conflict make sense, but the primary path really needs to be left\n> > > as-is for these other cases.\n> >\n> > I fear that I will need much, much more than just the additional paths ;-)\n> > So far, I am parsing the `ls-files -u`-like part so that I get mode and\n> > hash information, but eventually I want to parse that out of the messages\n> > part and that only produces lists of paths so far...\n>\n> List of paths + conflict type + detailed conflict message, right?\n>\n> > And a simple, single list of paths is not enough to convey the information\n> > that I need. Concretely, I need:\n> >\n> > - the original file name(s) involved in the conflict,\n>\n> Trying to get the original file name(s) may well cause you to need to\n> include additional conflicts, since they were the cause of the name\n> munging. So, are you displaying multiple conflicts at once, or just\n> gathering the extra names and printing them without the extra context?\n> If you do display multiple conflicts at once, are you focusing on one\n> conflict and printing the necessary extra conflicts and paths to\n> understand that one, and then potentially again listing some of those\n> paths and conflicts again separately?\n>\n> And, to make it worse, what about cases where there is not a clear\n> line between where one conflict starts and another ends? Do you start\n> just agglomerating arbitrarily many conflicts together? And get the\n> original names for all of them or some subset? And then do you print\n> all the \"original file name(s)\" once for the whole set, or once per\n> conflict within it?\n>\n> I'll list some examples of what I mean below with some different\n> possibilities...\n>\n> > - the modes and object hashes corresponding to the different stages\n>\n> Which you can get by parsing the section, and\n> comparing to the names from your list of paths, right?\n>\n> > - maybe some provenance information in the future, in case of complex\n> > rename/merge recursions\n> >\n> > In particular when presenting a file/directory conflict, it will be\n> > important to know what stage refers to the file and what stage to the\n> > directory (so as to know whether the respective object hash refers to a\n> > file or even symlink and needs to be read as a blob).\n>\n> Is this follow-on paragraph meant to be an example of provenance\n> information? Or a separate thought? I'm struggling a bit to follow.\n>\n> The stage and mode and hash information all come from the info> section (i.e. the section with `ls-files -u`-like output). You\n> already have that available. And the mode gives you the\n> file/directory/symlink/submodule type.\n>\n> No conflict entries for a \"tree\" are presented (because it'd be kind\n> of confusing to have a conflict entry for a tree as well as for\n> several files under the tree, and allow the user to resolve them\n> independently). So, when there's a file/directory conflict in\n> particular, there will not be a stage provided to represent the\n> directory.\n>\n> But I'm not sure how relevant that all is; it feels like I'm missing\n> what you mean here.\n>\n> > > > Note: For the modify/delete conflict type, we used to mention _only_ the\n> > > > munged name in that path list. To allow for tools to read the tree\n> > > > object produced by `git merge-tree -z` into a Git index in order to\n> > > > resolve the conflicts, it is necessary to list not only the original\n> > > > name but _also_ the munged name so that the item with the munged file\n> > > > name can be removed from that Git index. Therefore, this patch teaches\n> > > > `git merge-tree` to show both the original _and_ the munged name in that\n> > > > list.\n> > >\n> > > This makes sense. Passing additional valid and relevant information\n> > > to callers of merge-tree seems beneficial.\n> >\n> > And now that you mention this, I realize that this should be pulled out\n> > into its own patch.\n> >\n> > > > Also note: This patch changes the output of the remerge diff slightly:\n> > > > whereas before, we printed the notice about a file/directory conflict\n> > > > under the diff header mentioning the munged file name, we now print it\n> > > > under a separate diff header that mentions the original file name.\n> > > > That is the explanation why t4301 is touched by this patch.\n> > >\n> > > Sounds problematic.\n> >\n> > Fair enough.\n> >\n> > > > Signed-off-by: Johannes Schindelin \n> > > > ---\n> > > > merge-ort.c | 14 +++++++-------\n> > > > t/t4069-remerge-diff.sh | 8 ++++----\n> > > > t/t4301-merge-tree-write-tree.sh | 4 ++--\n> > > > 3 files changed, 13 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/merge-ort.c b/merge-ort.c\n> > > > index 01f150ef3b5..211f6823e1d 100644\n> > > > --- a/merge-ort.c\n> > > > +++ b/merge-ort.c\n> > > > @@ -3741,6 +3741,7 @@ static void process_entry(struct merge_options *opt,\n> > > > struct conflict_info *ci,\n> > > > struct directory_versions *dir_metadata)\n> > > > {\n> > > > + const char *orig_path = path;\n> > > > int df_file_index = 0;\n> > > >\n> > > > VERIFY_CI(ci);\n> > > > @@ -3787,7 +3788,6 @@ static void process_entry(struct merge_options *opt,\n> > > > */\n> > > > struct conflict_info *new_ci;\n> > > > const char *branch;\n> > > > - const char *old_path = path;\n> > > > int i;\n> > > >\n> > > > assert(ci->merged.result.mode == S_IFDIR);\n> > >\n> > > This looks like you are trying to make sure you always have the\n> > > \"original\" path available, meaning a path corresponding to a file from\n> > > one of the commits being merged. This doesn't quite accomplish that,\n> > > though. Some questions:\n> > >\n> > > * What about cases where \"path\" is a location that exists only in\n> > > the merge base? (rename/rename(1to2) is an example where this will\n> > > come up)\n> >\n> > That's a good point. I will need to invent some way to convey all three\n> > original file names in that case, not just one.\n>\n> Do you? The CONFLICT_RENAME_RENAME output in the Messages> section will have\n> 3\n> path-from-merge-base\n> path-from-side1\n> path-from-side2\n> CONFLICT (rename/rename)\n> \n> So the user of merge-tree -z already has the information to tie these\n> three filenames together. Why does process_entry() also need to have\n> this information and provide it to subsequent calls?\n>\n> (Note that for a rename/rename(1to2) without other conflicts involved,\n> process_entry() will be called for each path involved, and you'll get\n> a entry for each. One of those is from the merge\n> base. So you'll have 3 entries, as well as the above\n> to be able to tie them together.)\n>\n> However, my comments in the last 2 paragraphs gets us further away\n> from my original question from my last email.\n>\n> In my previous email, I was trying to ask whether you were\n> concentrating on being able to say which \"side\" or \"branch\" a file was\n> from. Index entries at stage 1 don't come from either side. Is that\n> problematic to you?\n>\n> > > * What about cases where \"path\" is a location that does not exist\n> > > anywhere in history? (directory renames yield such a result. e.g.\n> > > side A renames olddir/ -> newdir/, and side B adds olddir/foo, then\n> > > \"path\" will be newdir/foo)\n> >\n> > Hmm. As mentioned, I want to focus on the scenario with disabled renames,\n> > but not shut the door on rename scenarios like this one.\n> >\n> > So maybe I need to collect the original names in this instance, too, with\n> > some (machine-parseable) indicator talking about the nature of that\n> > origin.\n>\n> Which I think you can get from the that\n> already exists... (again, example at the end)\n>\n> > > > @@ -3838,10 +3838,10 @@ static void process_entry(struct merge_options *opt,\n> > > > strmap_put(&opt->priv->paths, path, new_ci);\n> > > >\n> > > > path_msg(opt, CONFLICT_FILE_DIRECTORY, 0,\n> > > > - path, old_path, NULL, NULL,\n> > > > + orig_path, path, NULL, NULL,\n> > > > _(\"CONFLICT (file/directory): directory in the way \"\n> > > > \"of %s from %s; moving it to %s instead.\"),\n> > > > - old_path, branch, path);\n> > > > + orig_path, branch, path);\n> > > > /*\n> > > > * Zero out the filemask for the old ci. At this point, ci\n> > > > @@ -3921,7 +3921,7 @@ static void process_entry(struct merge_options *opt,\n> > > >\n> > > > if (rename_a && rename_b) {\n> > > > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > > > - path, a_path, b_path, NULL,\n> > > > + orig_path, a_path, b_path, NULL,\n> > > > _(\"CONFLICT (distinct types): %s had \"\n> > > > \"different types on each side; \"\n> > > > \"renamed both of them so each can \"\n> > > > @@ -3929,7 +3929,7 @@ static void process_entry(struct merge_options *opt,\n> > > > path);\n> > > > } else {\n> > > > path_msg(opt, CONFLICT_DISTINCT_MODES, 0,\n> > > > - path, rename_a ? a_path : b_path,\n> > > > + orig_path, rename_a ? a_path : b_path,\n> > > > NULL, NULL,\n> > > > _(\"CONFLICT (distinct types): %s had \"\n> > > > \"different types on each side; \"\n> > >\n> > > I think for both of these last two cases, path == orig_path, so you\n> > > haven't actually made an effective change here.\n> >\n> > Not quite. If `ci->df_conflict && ci->merged.result.mode != 0`, then we\n> > assign `path = unique_path(opt, path, branch);` and `path` no longer\n> > refers to the same string as `orig_path`.\n> >\n> > It is possible that there is no actual code path that hits that clause and\n> > then also hits the `CONFLICT_DISTINC_MODES` calls, but I think there is:\n> > if a file has changed to a symlink in one branch _and_ was replaced by a\n> > directory in the other branch.\n>\n> The \"ci->filemask >= 6\" check that guards this code prevents any such\n> possibility. In particular, from your example, if it was changed to a\n> symlink in one branch and replaced by a directory in the other branch,\n> then ci->filemask is either 3 or 5, which would prevent the code from\n> getting here.\n>\n> > > (And, as above, I want path to always be the primary_path passed to\n> > > path_msg() as other code was written with that assumption in mind.)\n> > >\n> > > > @@ -4022,7 +4022,7 @@ static void process_entry(struct merge_options *opt,\n> > > > if (S_ISGITLINK(merged_file.mode))\n> > > > reason = _(\"submodule\");\n> > > > path_msg(opt, CONFLICT_CONTENTS, 0,\n> > > > - path, NULL, NULL, NULL,\n> > > > + orig_path, NULL, NULL, NULL,\n> > > > _(\"CONFLICT (%s): Merge conflict in %s\"),\n> > > > reason, path);\n> > > > }\n> > >\n> > > Here's another case where path == orig_path, so you haven't made an\n> > > effective change.\n> >\n> > I cannot think of a case where a file/directory conflict can also result\n> > in a content conflict, so at least in the regular 3-way merge, I think\n> > you're right and this changed line does the same as before in all cases.\n> >\n> > > But this one highlights something interesting...\n> > >\n> > > In addition to the fact that path/orig_path may be a location that\n> > > didn't exist on either side of history, there might actually be two\n> > > relevant paths from the two different sides of history which are\n> > > involved in the content merge, with neither of them being path or\n> > > orig_path. Let me break it down, starting with a simpler two path\n> > > case:\n> > >\n> > > If we have a standard rename, e.g. foo -> bar, and both sides modified\n> > > the file but did so in a conflicting manner, then we will end up in\n> > > this chunk of code. The conflict info emitted by merge-tree -z which\n> > > is always of the form\n> > > NULNULNULNULNULNUL\n> > > will in this particular case be\n> > > 1barAuto-mergingAuto-merging bar\n> > > 1barCONFLICT (contents)CONFLICT (content): Merge\n> > > conflict in bar\n> > > Neither of these messages has any mention of \"foo\", despite the fact\n> > > that \"foo\" was the name of the file in both the merge base and one\n> > > side, and \"bar\" was only the name of the file on one side.\n> >\n> > I guess so, as no conflict message will mention the rename: it does not\n> > conflict.\n>\n> Right, so we should probably also produce informational messages for\n> normal renames; adding a path_msg() call to the \"/* normal rename */\"\n> code path should do it. Though, I think we might only want such\n> messages when merge-tree -z is being used. (Or perhaps if some kind\n> of higher verbosity is requested?)\n>\n> > > Now, let's make it more interesting. side A modifies foo, and renames\n> > > olddir/->newdir/. side B modifies foo in a conflicting manner, and\n> > > renames foo->olddir/bar. Our `merge-tree -z` conflict information\n> > > emitted will be of the form\n> > > 1newdir/barAuto-mergingAuto-merging newdir/bar\n> > > 1newdir/barCONFLICT (contents)CONFLICT (content):\n> > > Merge conflict in newdir/bar\n> > > For this more interesting case, the only files that existed were \"foo\"\n> > > and \"olddir/bar\", neither of which are mentioned in the conflict info.\n> > > The conflict info only reports on \"newdir/bar\".\n> > >\n> > > And for both of these examples, your patch doesn't change the existing\n> > > behavior at all since path == orig_path for this hunk of the patch.\n> >\n> > That is true, too.\n> >\n> > Which makes me think that the consuming code I have to work with will need\n> > some elaborate changes when it wants to start supporting renames.\n> >\n> > But that's not the stage where I'm at right now.\n> >\n> > > > @@ -4067,7 +4067,7 @@ static void process_entry(struct merge_options *opt,\n> > > > */\n> > > > } else {\n> > > > path_msg(opt, CONFLICT_MODIFY_DELETE, 0,\n> > > > - path, NULL, NULL, NULL,\n> > > > + orig_path, path, NULL, NULL,\n> > > > _(\"CONFLICT (modify/delete): %s deleted in %s \"\n> > > > \"and modified in %s. Version %s of %s left \"\n> > > > \"in tree.\"),\n> > >\n> > > Adding orig_path here after path would make sense, though it's still\n> > > incomplete. For example if a path was modified and renamed on one\n> > > side, and deleted on the other, then we'd get to this hunk, and the\n> > > modify/delete would omit information about the other path. Further,\n> > > directory renames could be at play, such that neither orig_path nor\n> > > path would correspond to a filename in either of the commits being\n> > > merged.\n> > >\n> > > There are also other path_msg() calls that should probably be informed\n> > > of additional filenames due to simple renames: CONFLICT_CONTENTS (as\n> > > noted above), CONFLICT_BINARY, and INFO_AUTO_MERGING.\n> > >\n> > > Going beyond simple renames, if you want \"fully original\" filenames,\n> > > then we'd probably have to audit nearly all the path_msg() calls for\n> > > when directory renames are possible, and modify the directory renaming\n> > > code to plumb the original name through to this layer.\n> > >\n> > > And even if we do all that, then we still aren't matching what I think\n> > > you are expecting where every conflicted higher order entry we report\n> > > maps to a single : pairing. We may have a higher\n> > > stage conflict entry that represents merged contents of three\n> > > different filecontents from three different filenames. For example,\n> > > if someone has the following setup:\n> > >\n> > > Commit O: one file named \"original\"\n> > > Commit A: modify original + rename to primordial\n> > > Commit B: add file named primordial, modify original differently +\n> > > rename it to primary\n> > >\n> > > where A & B both have O as a parent or ancestor, and O is the unique\n> > > merge base of A & B. Now if we try to merge A & B, then the index\n> > > will have four higher order entries (using \n> > > , as we'd see from either `ls-files -u` style\n> > > output or from the first part of merge-tree -z output):\n> > >\n> > > 100644 1 original\n> > > 100644 > > B:primary> 3 primary\n> > > 100644 > > B:primary> 2 primordial\n> > > 100644 3 primordial\n> > >\n> > > Note that the two middle entries, despite having one name attached to\n> > > them, actually come from a merge of three different files. Further,\n> > > that hash for those two entries will not have yet appeared as the\n> > > contents for any blob in any revision of history, and will in fact\n> > > represent a file with conflict markers (even if none of the original\n> > > files -- O:original, A:primodial, B:primary -- had any conflicts).\n> > > And, the version of \"primodial\" in the merge-result-tree would likely\n> > > have nested conflict markers, despite O being a unique merge base\n> > > (meaning we get nested conflict markers without merging merge bases).\n> > >\n> > > Trying to have all the conflict output from merge-tree map to some\n> > > original file the user had from one of their sides of history is thus\n> > > a goal that merge-ort is nowhere close to satisfying. I'm not even\n> > > sure it's tractable.\n> >\n> > Sure, but I am getting more and more convinced that we will need to\n> > provide \"sort of\" the original name. Somehow.\n>\n> Can I ask in more detail what you want to present to a user? Maybe\n> it'd help frame the conversation more.\n>\n> Let me provide two examples, each with lots of alternatives, to see if\n> any are the kind of output you want to be able to present to a user\n> (and assume in each case it's not just a block message but you have\n> some kind of programmatic knowledge of the components referenced in\n> the message):\n>\n> Example 1.\n>\n> Are you thinking of something like \"The following files were involved\n> in a modify/delete conflict: foo, olddir/bar, newdir/bar,\n> newdir/bar~B\", followed by three other conflict messages that also involve\n> those same files?\n>\n> Or, more like: \"The following files were involved in a directory\n> rename, and in a rename/delete conflict, and in a file/directory\n> conflict, and in a modify/delete conflict: foo, olddir/bar,\n> newdir/bar, newdir/bar~B\"\n>\n> Or, more like: \"The following files were involved in a directory\n> rename, and in a rename/delete conflict, and in a file/directory\n> conflict, and in a modify/delete conflict: foo (from side A),\n> olddir/bar (from side B), newdir/bar (munged name), newdir/bar~B\n> (munged name)\"\n>\n> Or, more like: \"The file newdir/bar~B has a modify/delete conflict.\n> It was renamed to newdir/bar~B from newdir/bar because of a\n> file/directory conflict. It was renamed to newdir/bar from olddir/bar\n> because of a directory rename. It was renamed from foo to olddir/bar\n> on side B. foo was deleted on side A.\"\n>\n> Or, more like: \"foo was deleted on side A, and renamed to olddir/bar\n> on side B. olddir/bar was renamed to newdir/bar because of a\n> directory rename. newdir/bar was renamed to newdir/bar~B because of a\n> file/directory conflict. newdir/bar~B has a modify/delete conflict.\"\n>\n> Or something completely different?\n>\n>\n> Example 2.\n>\n> Are you thinking of something like: \"The following files were involved\n> in an add/add conflict: file-one, file-two, file-three, file-four, and\n> file-five\" ?\n>\n> Or, is it more like: \"The following files were involved in an add/add\n> conflict: file-one (from branch A), file-two (from merge base),\n> file-three (from branch A), file-three (from branch B), file-four\n> (from merge base), and file-five (from branch B)\" ?\n>\n> Or, is it more like: \"The following files were involved in an add/add\n> conflict: file-three (from [see details]) and file-three (from [see\n> details]). Note that file-three was part of a rename/rename(1to2)\n> conflict, which involved file-one (from branch A), file-two (from\n> merge base), and file-three (from branch B). Note that file-three was\n> part of a rename/rename(1to2) conflict, which involved file-three\n> (from branch A), file-four (from merge base), and file-five (from\n> branch B)\"\n>\n> Or, is it more like: \"file-three was part of a rename/rename(1to2)\n> conflict, which involved file-one (from branch A), file-two (from\n> merge base), and file-three (from branch B). file-three was part of a\n> rename/rename(1to2) conflict, which involved file-three (from branch\n> A), file-four (from merge base), and file-five (from branch B).\n> file-three and file-three are involved in an add/add conflict\"\n>\n> (Of course, all of this example is assuming we're just focusing on the\n> ultimate file-three vs file-three add/add conflict, not the additional\n> conflicts that file-one and file-five may be involved in.)\n>\n> Or are you wanting to display something completely different?\n>\n>\n>\n> I think we already have enough info to do most of the above.\n>\n> The main shortcoming is just that path_msg() calls for various\n> rename-oriented conflicts might also need to be augmented with\n> branches (though perhaps that's natural to do if we consider the list\n> passed to it to be a list of names rather than just a list of\n> pathnames; then we can just add the branch(s) to the list)\n>\n> > That is, if there were successful (directory) renames happening before the\n> > 3-way merge that resulted in a conflict, then I do not really care if the\n> > \"original original\" filename isn't shown. But I do care that the filename\n> > before munging is shown (and yes, we should probably also show the munged\n> > filename).\n>\n> No, directory renames are part of the filename munging done *by* the\n> three-way merge (not before it). That munging, though, applies before\n> process_entry() is called. Here's an example (from Example 1 above):\n>\n> Commit O: foo, olddir/{a,b,c}\n> Commit A: delete foo, rename olddir/ -> newdir/, add newdir/bar/file\n> Commit B: modify foo & rename foo -> olddir/bar\n>\n> Here, A had a file named \"foo\". B had a file named \"olddir/bar\"\n> (renamed from foo). The merge notices the olddir/ -> newdir/\n> directory rename in A and munges the filename further to \"newdir/bar\".\n> The merge also notices that there is a directory in the way of\n> \"newdir/bar\", and thus munges the filename even further to\n> \"newdir/bar~B\". This would result in two higher order stages in the\n> `ls-files -u`-style output, both for newdir/bar~B.\n>\n> The \"original filename\" for one of those is \"foo\" and for the other is\n> \"olddir/bar\". However, the value for \"path\" passed to the lone\n> \"process_entry()\" call will be \"newdir/bar\". The value of \"path\"\n> after the call to unique_path() will be \"newdir/bar~B\". Neither\n> \"newdir/bar\" or \"newdir/bar~B\" is an original filename.\n>\n> > And since recursive merges can result in multiple levels of munging, it\n> > gets harder to explain what I want, but I think you'll get the idea that\n> > the \"pre-munged\" filename of one conflict could be the \"post-munged\"\n> > filename of a conflict that happened at a lower level of that recursion.\n>\n> Yes, recursing makes things more interesting. But even if you assume\n> a unique merge-base, you still cannot assume that there is any such\n> thing as \"the original filename\" for an entry in the `ls-files -u` output.\n>\n> > > However, there is a much smaller scoped goal that we could easily\n> > > achieve: providing additional filename information in the path_msg()\n> > > calls, much like you start to do with the modify/delete case.\n> >\n> > Sure, but we also need an indicator somehow what purpose each of those\n> > additional filenames serve. I guess we could make it implicit, just like\n> > we make it implicit that the first path in a file/directory conflict\n> > refers to the file's name and the second to the directory's.\n>\n> Yes, I mean the point of the was to provide\n> parsable information and this was what I had in mind when I created\n> it.\n>\n> Very minor nitpick, though: the first path refers to the *munged*\n> file's name, and the second path refers to the directory's name. (If\n> the file and directory had different names before our additional\n> munging, we wouldn't have needed that additional munging).\n>\n> > But it makes\n> > me a bit uneasy and I wish we had more like a structure instead of just a\n> > string that we could print out. That would make it possible to convey\n> > from where (as in \"which branch?\") the file or directory came.\n>\n> And what about other cases where there is no \"the branch\" that a file\n> came from? For example, doing rename + add/add:\n>\n> Commit O: foo\n> Commit A: modify foo + rename foo->bar\n> Commit B: modify foo, add unrelated bar\n>\n> In this case, we'll have a \"bar\" at stage 3 representing B:bar. We'll\n> also get a three-way merge of O:foo, A:bar, and B:foo, stuck in a\n> stage 2 entry under the filename bar. The stage 2 entry for bar did\n> not come from one branch, it represents merged contents that came from\n> all three commits.\n>\n> > > > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> > > > index 35f94957fce..bc580a242ac 100755\n> > > > --- a/t/t4069-remerge-diff.sh\n> > > > +++ b/t/t4069-remerge-diff.sh\n> > > > @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '\n> > > > test_expect_success 'remerge-diff with non-content conflicts' '\n> > > > git log -1 --oneline resolution >tmp &&\n> > > > cat <<-EOF >>tmp &&\n> > > > + diff --git a/file_or_directory b/file_or_directory\n> > > > + remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > > > similarity index 100%\n> > > > rename from file_or_directory~HASH (side1)\n> > > > rename to wanted_content\n> > > > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > > diff --git a/letters b/letters\n> > > > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > > > diff --git a/letters_side2 b/letters_side2\n> > > > @@ -168,7 +169,7 @@ test_expect_success 'remerge-diff with non-content conflicts' '\n> > > > test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no diff content' '\n> > > > git log -1 --oneline resolution >tmp &&\n> > > > cat <<-EOF >>tmp &&\n> > > > - diff --git a/file_or_directory~HASH (side1) b/file_or_directory~HASH (side1)\n> > > > + diff --git a/file_or_directory b/file_or_directory\n> > > > remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > > diff --git a/letters b/letters\n> > > > remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).\n> > > > @@ -184,14 +185,13 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif\n> > > > test_cmp expect actual\n> > > > '\n> > > >\n> > > > -test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' '\n> > > > +test_expect_success 'remerge-diff w/ diff-filter=R: relevant file' '\n> > > > git log -1 --oneline resolution >tmp &&\n> > > > cat <<-EOF >>tmp &&\n> > > > diff --git a/file_or_directory~HASH (side1) b/wanted_content\n> > > > similarity index 100%\n> > > > rename from file_or_directory~HASH (side1)\n> > > > rename to wanted_content\n> > > > - remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.\n> > > > EOF\n> > > > # We still have some sha1 hashes above; rip them out so test works\n> > > > # with sha256\n> > >\n> > > All of these look like regressions to me; you discussed these in a\n> > > separate email so I'll go into detail in response to that one.\n> >\n> > Thank you for being so diligent. I always appreciate it when I see a\n> > well-written, obviously well-edited mail. I find it very considerate.\n> >\n> > You did a good job at convincing me that my approach is sub-optimal, and\n> > that we need at least one new flag to get the information that I need to\n> > produce in my use case because the caller requires it to present a nice\n> > conflict resolution UI to the users.\n>\n> I'm not so sure you need additional information. Don't the\n> and provide enough\n> info already, for the usecases you are considering? In particular,\n> you were trying to add a testcase in your series for a simple\n> directory/file conflict, but for such a case you can already determine\n> original filenames. The and messages> sections of the `merge-tree -z` output for these, after\n> replacing NUL characters with newlines, would be:\n>\n> 120000 3 origfile\n> 100644 1 origfile~branch\n> 100644 2 origfile~branch\n>\n> 2\n> origfile\n> origfile~branch\n> CONFLICT (distinct modes)\n> CONFLICT (distinct types): whatever had different types on each\n> side; renamed one of them so each can be recorded somewhere.\n>\n> Now, the \"distinct modes\" conflict always results in filename munging,\n> with both names listed. So, from this output, you know the original\n> name is \"origfile\" whenever you see \"origfile~branch\". So now you can\n> present all the `ls-files -u` output from the first section with\n> remapping munged filenames back to original filenames.\n>\n> Doesn't that solve your immediate usecase already, without even\n> needing any merge-ort changes?\n>\n"},{"id":"462121","messageId":"CABPp-BE0DA6jN7i8zHj+zy_SXo0DV_JyiRgDR1OwBYRTv6M6Vg@mail.gmail.com","threadId":"58326","inReplyTo":"6p614ron-qrsq-p802-1892-o2r4o8n72p44@tzk.qr","subject":"Re: [PATCH 1/3] merge-tree -z: always show the original file name first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-30T02:13:38Z","receivedAt":"2022-08-30T02:13:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 26, 2022 at 8:35 AM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> I'll top-post because I will still need much more time to wrap my head\n> around everything you said in your very thoughtful reply.\n>\n> By the way, have I mentioned before how much I appreciate well-edited,\n> well-crafted mails? It must have taken quite a bit of care and editing to\n> write this, and I just want to thank you for that.\n>\n> As to the problem we are discussing: I am fully on board with you on the\n> necessity to keep the door open for rename detection and recursive merges\n> and the zoo of conflict types that comes with it.\n>\n> After thinking about this for a couple of nights, I think I want to tackle\n> the design from the \"other\" side, namely from the UI.\n>\n> At this stage, I _think_ it would do well for me to give a more high-level\n> context as to my motivating factors.\n\nThis is helpful, but I would really like a few concrete examples too.\nI tried to provide some in my last email with alternative guesses at\nwhat you might want. It'd be great to have an answer on those. I'll\nhighlight them again at the end.\n\n> The thing is, these worktree-less merges do not live in a vacuum. They are\n> backing the Pull Request UI. There exists a relatively simple UI to\n> resolve simple merge conflicts, which factors into what I want from `git\n> merge-tree`.\n>\n> You've pointed out in the past, very rightfully, that the simple way to\n> \"pre-resolve\" conflicts that I have implemented over here is just not good\n> enough except for a very, very limited manner of driving `git merge-tree`.\n> These \"pre-resolved conflicts\" are currently essentially a map of \n> -> pairs.\n>\n> Now, to design a much better way to present conflicts as well as allowing\n> users to resolve some of them (the simpler ones, like content conflicts)\n> in a web UI, we need `git merge-tree` to give us not an `ls-files\n> -u`-style output, that only works reliably for very simple cases.\n\nI agree we should not _only_ pay attention to `ls-files -u` info;\nthat's too simplistic. And, in fact, that's the reason for having\nmerge-tree provide another section of information. If we used both\nsets of information, we can handle more complex cases...\n\n> In general, I think the design would work best if we parsed the `messages`\n> part exclusively on the server side (ignoring the `ls-files`-like output,\n> probably even suppressing it) and if we received enough information to\n> recreate the conflicts from there. Which means that we need the involved\n> paths (whether munged or not, I am less and less concerned) together with\n> the modes and OIDs. It would probably be helpful even to provide a tree\n> OID for the directory part of any file/directory conflict, where the web\n> UI could then present the contents in a tree view to the user.\n\nBy the `messages` part, do you mean the existing section, which consists of multiple entries of the form\n\n NULNULNULNULNULNUL\n\n? Or do you mean the part of each entry from that section,\nor do you perhaps mean something else? I'll assume for now you mean\nthe full section rather than some\nsub-component or something else, but let me know if I'm wrong.\n\n\nAlso, why ignore or suppress the `ls-files -u`-like output? Doesn't\nit provide the modes and OIDs that you want?\n\n> This way, we could present multiple conflicts, even multiple conflicts for\n> the very same file, and allow resolving them individually.\n\nYou seem to still be assuming separable conflicts? I tried to ask\nabout two concrete examples in the last email with lots of different\npossibilities for what you might want to show the user (search for\n\"Example 1\" and \"Example 2\"; I'll also include them at the end of this\nemail and snip out the other parts of the old email). I'm not sure\neither example is one where it makes sense for a user to attempt to\nresolve conflicts individually.\n\nOr is this more of a limited UI thing, where you want to search\nthrough the conflicts and affected paths, and if the conflicts aren't\nseparable then you want to tell users to resolve the conflicts some\nother way, but proceed and allow users to use your UI if the conflicts\nare simple enough that they are separable?\n\n> And we could associate resolutions with the exact list of paths, modes and\n> OIDs involved, then teach `merge-tree` to optionally take such resolutions\n> into account, to produce merges with resolved conflicts.\n\nThis seems like future work we could address another time...but\nperhaps I could follow the tangent for just a second. Why would it\nmake sense to add conflict-resolution information to `merge-tree`?\nWhen you ran `merge-tree` the first time, it already provided you with\na tree, and a list of conflicted files. If you have updates for those\nconflicted files, you can update the tree yourself without needing to\nredo any merges...right? Why would extending merge-tree and calling\nit again and having it repeat a merge even help?\n\n> Doing it this way will require substantial changes on the server side, in\n> particular in the UI, which currently does not have any support for\n> handling conflicts involving renames. It will be a lot of work, and I am\n> certain that I will have to pivot many times regarding the design of `git\n> merge-tree` features supporting that UI. I'm excited to start that\n> journey.\n>\n> What do you think?\n\nI'm open to going down new paths and even doing a lot of refactoring\nto get what we need, I'd just like to understand why they are needed\nfirst. In particular, I'm confused why you feel you should only ever\nuse part of the \"git merge-tree -z\" output. Previously it seemed you\nwere trying to use only `ls-files -u`-style output, now you're trying\nto use only the section, and I'm trying to\nadvocate for using both. I think you can already handle many of the\ncases you are concerned about if you do so, without any further\nmerge-ort.c changes, and that the remaining cases should be\naddressable with just a few small tweaks to some rename-related\npath_msg() calls.\n\nBut maybe I'm still not understanding something about the usecase, or\nthere's something I'm missing about why the information provided\nreally isn't sufficient to solve the cases you're currently facing.\nCould you look over the concrete examples I provided and give some\nfeedback on what you want to do with them? That might point out what\nI'm missing. Let me quote the two relevant sections of my last email\nwith these examples, in reverse order:\n\n> > I'm not so sure you need additional information. Don't the\n> > and provide enough\n> > info already, for the usecases you are considering? In particular,\n> > you were trying to add a testcase in your series for a simple\n> > directory/file conflict, but for such a case you can already determine\n> > original filenames. The and > messages> sections of the `merge-tree -z` output for these, after\n> > replacing NUL characters with newlines, would be:\n> >\n> > 120000 3 origfile\n> > 100644 1 origfile~branch\n> > 100644 2 origfile~branch\n> >\n> > 2\n> > origfile\n> > origfile~branch\n> > CONFLICT (distinct modes)\n> > CONFLICT (distinct types): whatever had different types on each\n> > side; renamed one of them so each can be recorded somewhere.\n> >\n> > Now, the \"distinct modes\" conflict always results in filename munging,\n> > with both names listed. So, from this output, you know the original\n> > name is \"origfile\" whenever you see \"origfile~branch\". So now you can\n> > present all the `ls-files -u` output from the first section with\n> > remapping munged filenames back to original filenames.\n> >\n> > Doesn't that solve your immediate usecase already, without even\n> > needing any merge-ort changes?\n\nAnd, also the section of my previous email with the two more\ninteresting/complicated examples:\n\n> > Can I ask in more detail what you want to present to a user? Maybe\n> > it'd help frame the conversation more.\n> >\n> > Let me provide two examples, each with lots of alternatives, to see if\n> > any are the kind of output you want to be able to present to a user\n> > (and assume in each case it's not just a block message but you have\n> > some kind of programmatic knowledge of the components referenced in\n> > the message):\n> >\n> > Example 1.\n\n(Updated note: if you want to see Example 1 created in test code, go\nto the en/t4301-more-merge-tree-tests topic, look at t4301, and find\nthe \"directory rename + rename/delete + modify/delete + directory/file\nconflict\" testcase.)\n\n> > Are you thinking of something like \"The following files were involved\n> > in a modify/delete conflict: foo, olddir/bar, newdir/bar,\n> > newdir/bar~B\", followed by three other conflict messages that also involve\n> > those same files?\n> >\n> > Or, more like: \"The following files were involved in a directory\n> > rename, and in a rename/delete conflict, and in a file/directory\n> > conflict, and in a modify/delete conflict: foo, olddir/bar,\n> > newdir/bar, newdir/bar~B\"\n> >\n> > Or, more like: \"The following files were involved in a directory\n> > rename, and in a rename/delete conflict, and in a file/directory\n> > conflict, and in a modify/delete conflict: foo (from side A),\n> > olddir/bar (from side B), newdir/bar (munged name), newdir/bar~B\n> > (munged name)\"\n> >\n> > Or, more like: \"The file newdir/bar~B has a modify/delete conflict.\n> > It was renamed to newdir/bar~B from newdir/bar because of a\n> > file/directory conflict. It was renamed to newdir/bar from olddir/bar\n> > because of a directory rename. It was renamed from foo to olddir/bar\n> > on side B. foo was deleted on side A.\"\n> >\n> > Or, more like: \"foo was deleted on side A, and renamed to olddir/bar\n> > on side B. olddir/bar was renamed to newdir/bar because of a\n> > directory rename. newdir/bar was renamed to newdir/bar~B because of a\n> > file/directory conflict. newdir/bar~B has a modify/delete conflict.\"\n> >\n> > Or something completely different?\n> >\n> >\n> > Example 2.\n\n(Updated note: if you want to see a close neighbor to Example 2 in\ntest code, go to the en/t4301-more-merge-tree-tests topic, look at\nt4301, and find the \"mod6: chains of rename/rename(1to2) and add/add\nvia colliding renames\" testcase. That testcase is actually a bit more\ncomplex, having three intertwined sets of each of these three already\ntightly bound conflicts, reusing them in a way to end up with 6 total\nintertwined conflicts. But if you want to pick just one of the three\nintertwined sets, instead of \"file-one\" through \"file-five\", look at\nthe files \"two\" through \"six\" from that testcase.)\n\n\n> > Are you thinking of something like: \"The following files were involved\n> > in an add/add conflict: file-one, file-two, file-three, file-four, and\n> > file-five\" ?\n> >\n> > Or, is it more like: \"The following files were involved in an add/add\n> > conflict: file-one (from branch A), file-two (from merge base),\n> > file-three (from branch A), file-three (from branch B), file-four\n> > (from merge base), and file-five (from branch B)\" ?\n> >\n> > Or, is it more like: \"The following files were involved in an add/add\n> > conflict: file-three (from [see details]) and file-three (from [see\n> > details]). Note that file-three was part of a rename/rename(1to2)\n> > conflict, which involved file-one (from branch A), file-two (from\n> > merge base), and file-three (from branch B). Note that file-three was\n> > part of a rename/rename(1to2) conflict, which involved file-three\n> > (from branch A), file-four (from merge base), and file-five (from\n> > branch B)\"\n> >\n> > Or, is it more like: \"file-three was part of a rename/rename(1to2)\n> > conflict, which involved file-one (from branch A), file-two (from\n> > merge base), and file-three (from branch B). file-three was part of a\n> > rename/rename(1to2) conflict, which involved file-three (from branch\n> > A), file-four (from merge base), and file-five (from branch B).\n> > file-three and file-three are involved in an add/add conflict\"\n> >\n> > (Of course, all of this example is assuming we're just focusing on the\n> > ultimate file-three vs file-three add/add conflict, not the additional\n> > conflicts that file-one and file-five may be involved in.)\n> >\n> > Or are you wanting to display something completely different?\n"}]}