{"thread":{"id":"60796","subject":"[PATCH] merge-tree: accept 3 trees as arguments","startedAt":"2024-01-26T14:01:38Z","lastAt":"2024-01-31T18:14:42Z","messageCount":12,"participants":["Johannes Schindelin via GitGitGadget","Eric Sunshine","Junio C Hamano","Elijah Newren","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487410","messageId":"pull.1647.git.1706277694231.gitgitgadget@gmail.com","threadId":"60796","inReplyTo":null,"subject":"[PATCH] merge-tree: accept 3 trees as arguments","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-26T14:01:34Z","receivedAt":"2024-01-26T14:01:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen specifying a merge base explicitly, there is actually no good\nreason why the inputs need to be commits: that's only needed if the\nmerge base has to be deduced from the commit graph.\n\nThis commit is best viewed with `--color-moved\n--color-moved-ws=allow-indentation-change`.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-tree: accept 3 trees as arguments\n \n I was asked to implement this at $dayjob and it seems like a feature\n that might be useful to other users, too.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1647%2Fdscho%2Fallow-merge-tree-to-accept-3-trees-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1647/dscho/allow-merge-tree-to-accept-3-trees-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1647\n\n Documentation/git-merge-tree.txt | 5 +++-\n builtin/merge-tree.c | 42 +++++++++++++++++++-------------\n t/t4301-merge-tree-write-tree.sh | 8 ++++++\n 3 files changed, 37 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\nindex b50acace3bc..214e30c70ba 100644\n--- a/Documentation/git-merge-tree.txt\n+++ b/Documentation/git-merge-tree.txt\n@@ -64,10 +64,13 @@ OPTIONS\n \tshare no common history. This flag can be given to override that\n \tcheck and make the merge proceed anyway.\n \n---merge-base=::\n+--merge-base=::\n \tInstead of finding the merge-bases for and ,\n \tspecify a merge-base for the merge, and specifying multiple bases is\n \tcurrently not supported. This option is incompatible with `--stdin`.\n++\n+As the merge-base is provided directly, and do not need\n+o specify commits; it is sufficient if they specify trees.\n \n [[OUTPUT]]\n OUTPUT\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 3bdec53fbe5..cbd8e15af6d 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct merge_options opt;\n \n \tcopy_merge_options(&opt, &o->merge_options);\n-\tparent1 = get_merge_parent(branch1);\n-\tif (!parent1)\n-\t\thelp_unknown_ref(branch1, \"merge-tree\",\n-\t\t\t\t _(\"not something we can merge\"));\n-\n-\tparent2 = get_merge_parent(branch2);\n-\tif (!parent2)\n-\t\thelp_unknown_ref(branch2, \"merge-tree\",\n-\t\t\t\t _(\"not something we can merge\"));\n-\n \topt.show_rename_progress = 0;\n \n \topt.branch1 = branch1;\n \topt.branch2 = branch2;\n \n \tif (merge_base) {\n-\t\tstruct commit *base_commit;\n \t\tstruct tree *base_tree, *parent1_tree, *parent2_tree;\n \n-\t\tbase_commit = lookup_commit_reference_by_name(merge_base);\n-\t\tif (!base_commit)\n-\t\t\tdie(_(\"could not lookup commit '%s'\"), merge_base);\n+\t\t/*\n+\t\t * We actually only need the trees because we already\n+\t\t * have a merge base.\n+\t\t */\n+\t\tstruct object_id base_oid, head_oid, merge_oid;\n+\n+\t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n+\t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n+\t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n+\t\tparent2_tree = parse_tree_indirect(&merge_oid);\n \n \t\topt.ancestor = merge_base;\n-\t\tbase_tree = repo_get_commit_tree(the_repository, base_commit);\n-\t\tparent1_tree = repo_get_commit_tree(the_repository, parent1);\n-\t\tparent2_tree = repo_get_commit_tree(the_repository, parent2);\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\n \t} else {\n+\t\tparent1 = get_merge_parent(branch1);\n+\t\tif (!parent1)\n+\t\t\thelp_unknown_ref(branch1, \"merge-tree\",\n+\t\t\t\t\t _(\"not something we can merge\"));\n+\n+\t\tparent2 = get_merge_parent(branch2);\n+\t\tif (!parent2)\n+\t\t\thelp_unknown_ref(branch2, \"merge-tree\",\n+\t\t\t\t\t _(\"not something we can merge\"));\n+\n \t\t/*\n \t\t * Get the merge bases, in reverse order; see comment above\n \t\t * merge_incore_recursive in merge-ort.h\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 12ac4368736..71f21bb834f 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -945,4 +945,12 @@ test_expect_success 'check the input format when --stdin is passed' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--merge-base with tree OIDs' '\n+\tgit merge-tree --merge-base=side1^ side1 side3 >tree &&\n+\ttree=$(cat tree) &&\n+\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&\n+\ttree2=$(cat tree2) &&\n+\ttest $tree = $tree2\n+'\n+\n test_done\n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\n-- \ngitgitgadget\n"},{"id":"487411","messageId":"CAPig+cQUttmKYnUGbcNFXvvw5rWF4VHuE-GLnEbeOCkCwh056A@mail.gmail.com","threadId":"60796","inReplyTo":"pull.1647.git.1706277694231.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-01-26T14:15:21Z","receivedAt":"2024-01-26T14:15:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 26, 2024 at 9:01 AM Johannes Schindelin via GitGitGadget\n wrote:\n> When specifying a merge base explicitly, there is actually no good\n> reason why the inputs need to be commits: that's only needed if the\n> merge base has to be deduced from the commit graph.\n>\n> This commit is best viewed with `--color-moved\n> --color-moved-ws=allow-indentation-change`.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\n> @@ -64,10 +64,13 @@ OPTIONS\n> +--merge-base=::\n> Instead of finding the merge-bases for and ,\n> specify a merge-base for the merge, and specifying multiple bases is\n> currently not supported. This option is incompatible with `--stdin`.\n> ++\n> +As the merge-base is provided directly, and do not need\n> +o specify commits; it is sufficient if they specify trees.\n\npresumably: s/o/to/\n"},{"id":"487412","messageId":"xmqqplxoozh9.fsf@gitster.g","threadId":"60796","inReplyTo":"pull.1647.git.1706277694231.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-26T16:01:38Z","receivedAt":"2024-01-26T16:01:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When specifying a merge base explicitly, there is actually no good\n> reason why the inputs need to be commits: that's only needed if the\n> merge base has to be deduced from the commit graph.\n\nyes, Yes, YES, YEAHHHHH!\n\n> I was asked to implement this at $dayjob and it seems like a feature\n> that might be useful to other users, too.\n\nYup, I think it is an obvious building block for machinery to\nperform any mergy operation to grow history. Many of the time you\nmay have a commit, but requiring them to be commits when you know\nyou will not do a virtual ancestor synthesis smells fundamentally\nwrong. Thanks for fixing it.\n\n> ---merge-base=::\n> +--merge-base=::\n> \tInstead of finding the merge-bases for and ,\n> \tspecify a merge-base for the merge, and specifying multiple bases is\n> \tcurrently not supported. This option is incompatible with `--stdin`.\n> ++\n> +As the merge-base is provided directly, and do not need\n> +o specify commits; it is sufficient if they specify trees.\n\n\"... do not need to specify commits; trees are enough\"?\n\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index 3bdec53fbe5..cbd8e15af6d 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n> \tstruct merge_options opt;\n> \n> \tcopy_merge_options(&opt, &o->merge_options);\n> -\tparent1 = get_merge_parent(branch1);\n> -\tif (!parent1)\n> -\t\thelp_unknown_ref(branch1, \"merge-tree\",\n> -\t\t\t\t _(\"not something we can merge\"));\n> -\n> -\tparent2 = get_merge_parent(branch2);\n> -\tif (!parent2)\n> -\t\thelp_unknown_ref(branch2, \"merge-tree\",\n> -\t\t\t\t _(\"not something we can merge\"));\n> -\n> \topt.show_rename_progress = 0;\n> \n> \topt.branch1 = branch1;\n> \topt.branch2 = branch2;\n> \n> \tif (merge_base) {\n> -\t\tstruct commit *base_commit;\n> \t\tstruct tree *base_tree, *parent1_tree, *parent2_tree;\n> \n> -\t\tbase_commit = lookup_commit_reference_by_name(merge_base);\n> -\t\tif (!base_commit)\n> -\t\t\tdie(_(\"could not lookup commit '%s'\"), merge_base);\n> +\t\t/*\n> +\t\t * We actually only need the trees because we already\n> +\t\t * have a merge base.\n> +\t\t */\n> +\t\tstruct object_id base_oid, head_oid, merge_oid;\n> +\n> +\t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n> +\t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n> +\t\tbase_tree = parse_tree_indirect(&base_oid);\n> +\t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n> +\t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n> +\t\tparent1_tree = parse_tree_indirect(&head_oid);\n> +\t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n> +\t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n> +\t\tparent2_tree = parse_tree_indirect(&merge_oid);\n> \n> \t\topt.ancestor = merge_base;\n> -\t\tbase_tree = repo_get_commit_tree(the_repository, base_commit);\n> -\t\tparent1_tree = repo_get_commit_tree(the_repository, parent1);\n> -\t\tparent2_tree = repo_get_commit_tree(the_repository, parent2);\n> \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\n> \t} else {\n> +\t\tparent1 = get_merge_parent(branch1);\n> +\t\tif (!parent1)\n> +\t\t\thelp_unknown_ref(branch1, \"merge-tree\",\n> +\t\t\t\t\t _(\"not something we can merge\"));\n> +\n> +\t\tparent2 = get_merge_parent(branch2);\n> +\t\tif (!parent2)\n> +\t\t\thelp_unknown_ref(branch2, \"merge-tree\",\n> +\t\t\t\t\t _(\"not something we can merge\"));\n> +\n> \t\t/*\n> \t\t * Get the merge bases, in reverse order; see comment above\n> \t\t * merge_incore_recursive in merge-ort.h\n\nOK. The changes here look quite straight-forward.\n\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 12ac4368736..71f21bb834f 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -945,4 +945,12 @@ test_expect_success 'check the input format when --stdin is passed' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--merge-base with tree OIDs' '\n> +\tgit merge-tree --merge-base=side1^ side1 side3 >tree &&\n> +\ttree=$(cat tree) &&\n> +\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&\n> +\ttree2=$(cat tree2) &&\n> +\ttest $tree = $tree2\n> +'\n\nYou do not need $tree and $tree2 variables that would make it harder\nto diagnose a failure case when we break merge-tree. You have tree\nand tree2 as files, and I think it is sufficient to do\n\n\tgit merge-tree ... >result-from-commits &&\n\tgit merge-tree ... >result-from-trees &&\n\ttest_cmp result-from-commits result-from-trees\n\n(no, I am not suggesting to rename these tree and tree2 files; I\njust needed them to be descriptive in my illustration to show what\nis going on to myself).\n\nThanks.\n"},{"id":"487479","messageId":"pull.1647.v2.git.1706474063109.gitgitgadget@gmail.com","threadId":"60796","inReplyTo":"pull.1647.git.1706277694231.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-tree: accept 3 trees as arguments","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-28T20:34:22Z","receivedAt":"2024-01-28T20:34:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen specifying a merge base explicitly, there is actually no good\nreason why the inputs need to be commits: that's only needed if the\nmerge base has to be deduced from the commit graph.\n\nThis commit is best viewed with `--color-moved\n--color-moved-ws=allow-indentation-change`.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-tree: accept 3 trees as arguments\n \n I was asked to implement this at $dayjob and it seems like a feature\n that might be useful to other users, too.\n \n Changes since v1:\n \n * Fixed a typo in the manual page, simplified the part after the\n semicolon.\n * Simplified the test case.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1647%2Fdscho%2Fallow-merge-tree-to-accept-3-trees-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1647/dscho/allow-merge-tree-to-accept-3-trees-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1647\n\nRange-diff vs v1:\n\n 1: 29234754c06 ! 1: 233a25e9071 merge-tree: accept 3 trees as arguments\n @@ Documentation/git-merge-tree.txt: OPTIONS\n \tcurrently not supported. This option is incompatible with `--stdin`.\n ++\n +As the merge-base is provided directly, and do not need\n -+o specify commits; it is sufficient if they specify trees.\n ++to specify commits; trees are enough.\n \n [[OUTPUT]]\n OUTPUT\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'check the input format wh\n '\n \n +test_expect_success '--merge-base with tree OIDs' '\n -+\tgit merge-tree --merge-base=side1^ side1 side3 >tree &&\n -+\ttree=$(cat tree) &&\n -+\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&\n -+\ttree2=$(cat tree2) &&\n -+\ttest $tree = $tree2\n ++\tgit merge-tree --merge-base=side1^ side1 side3 >with-commits &&\n ++\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&\n ++\ttest_cmp with-commits with-trees\n +'\n +\n test_done\n\n\n Documentation/git-merge-tree.txt | 5 +++-\n builtin/merge-tree.c | 42 +++++++++++++++++++-------------\n t/t4301-merge-tree-write-tree.sh | 6 +++++\n 3 files changed, 35 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\nindex b50acace3bc..dd388fa21d5 100644\n--- a/Documentation/git-merge-tree.txt\n+++ b/Documentation/git-merge-tree.txt\n@@ -64,10 +64,13 @@ OPTIONS\n \tshare no common history. This flag can be given to override that\n \tcheck and make the merge proceed anyway.\n \n---merge-base=::\n+--merge-base=::\n \tInstead of finding the merge-bases for and ,\n \tspecify a merge-base for the merge, and specifying multiple bases is\n \tcurrently not supported. This option is incompatible with `--stdin`.\n++\n+As the merge-base is provided directly, and do not need\n+to specify commits; trees are enough.\n \n [[OUTPUT]]\n OUTPUT\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 3bdec53fbe5..cbd8e15af6d 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct merge_options opt;\n \n \tcopy_merge_options(&opt, &o->merge_options);\n-\tparent1 = get_merge_parent(branch1);\n-\tif (!parent1)\n-\t\thelp_unknown_ref(branch1, \"merge-tree\",\n-\t\t\t\t _(\"not something we can merge\"));\n-\n-\tparent2 = get_merge_parent(branch2);\n-\tif (!parent2)\n-\t\thelp_unknown_ref(branch2, \"merge-tree\",\n-\t\t\t\t _(\"not something we can merge\"));\n-\n \topt.show_rename_progress = 0;\n \n \topt.branch1 = branch1;\n \topt.branch2 = branch2;\n \n \tif (merge_base) {\n-\t\tstruct commit *base_commit;\n \t\tstruct tree *base_tree, *parent1_tree, *parent2_tree;\n \n-\t\tbase_commit = lookup_commit_reference_by_name(merge_base);\n-\t\tif (!base_commit)\n-\t\t\tdie(_(\"could not lookup commit '%s'\"), merge_base);\n+\t\t/*\n+\t\t * We actually only need the trees because we already\n+\t\t * have a merge base.\n+\t\t */\n+\t\tstruct object_id base_oid, head_oid, merge_oid;\n+\n+\t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n+\t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n+\t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n+\t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n+\t\tparent2_tree = parse_tree_indirect(&merge_oid);\n \n \t\topt.ancestor = merge_base;\n-\t\tbase_tree = repo_get_commit_tree(the_repository, base_commit);\n-\t\tparent1_tree = repo_get_commit_tree(the_repository, parent1);\n-\t\tparent2_tree = repo_get_commit_tree(the_repository, parent2);\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\n \t} else {\n+\t\tparent1 = get_merge_parent(branch1);\n+\t\tif (!parent1)\n+\t\t\thelp_unknown_ref(branch1, \"merge-tree\",\n+\t\t\t\t\t _(\"not something we can merge\"));\n+\n+\t\tparent2 = get_merge_parent(branch2);\n+\t\tif (!parent2)\n+\t\t\thelp_unknown_ref(branch2, \"merge-tree\",\n+\t\t\t\t\t _(\"not something we can merge\"));\n+\n \t\t/*\n \t\t * Get the merge bases, in reverse order; see comment above\n \t\t * merge_incore_recursive in merge-ort.h\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 12ac4368736..c5628c4e613 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -945,4 +945,10 @@ test_expect_success 'check the input format when --stdin is passed' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--merge-base with tree OIDs' '\n+\tgit merge-tree --merge-base=side1^ side1 side3 >with-commits &&\n+\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&\n+\ttest_cmp with-commits with-trees\n+'\n+\n test_done\n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\n-- \ngitgitgadget\n"},{"id":"487531","messageId":"xmqqbk94jahb.fsf@gitster.g","threadId":"60796","inReplyTo":"pull.1647.v2.git.1706474063109.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-tree: accept 3 trees as arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T17:49:36Z","receivedAt":"2024-01-29T17:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When specifying a merge base explicitly, there is actually no good\n> reason why the inputs need to be commits: that's only needed if the\n> merge base has to be deduced from the commit graph.\n>\n> This commit is best viewed with `--color-moved\n> --color-moved-ws=allow-indentation-change`.\n\nThanks, let's merge it down to 'next'.\n"},{"id":"487591","messageId":"CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com","threadId":"60796","inReplyTo":"pull.1647.git.1706277694231.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-30T07:04:53Z","receivedAt":"2024-01-30T07:05:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> When specifying a merge base explicitly, there is actually no good\n> reason why the inputs need to be commits: that's only needed if the\n> merge base has to be deduced from the commit graph.\n>\n> This commit is best viewed with `--color-moved\n> --color-moved-ws=allow-indentation-change`.\n\nMakes sense.\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-tree: accept 3 trees as arguments\n>\n> I was asked to implement this at $dayjob and it seems like a feature\n> that might be useful to other users, too.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1647%2Fdscho%2Fallow-merge-tree-to-accept-3-trees-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1647/dscho/allow-merge-tree-to-accept-3-trees-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1647\n>\n> Documentation/git-merge-tree.txt | 5 +++-\n> builtin/merge-tree.c | 42 +++++++++++++++++++-------------\n> t/t4301-merge-tree-write-tree.sh | 8 ++++++\n> 3 files changed, 37 insertions(+), 18 deletions(-)\n>\n> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\n> index b50acace3bc..214e30c70ba 100644\n> --- a/Documentation/git-merge-tree.txt\n> +++ b/Documentation/git-merge-tree.txt\n> @@ -64,10 +64,13 @@ OPTIONS\n> share no common history. This flag can be given to override that\n> check and make the merge proceed anyway.\n>\n> ---merge-base=::\n> +--merge-base=::\n\nA very minor point, but any chance we can just use ``, like\ngit-restore does? I've never liked the '-ish' that we use as it seems\nto throw users, and I think they understand that they can use a commit\nor a tag where a tree is expected\n\n> Instead of finding the merge-bases for and ,\n> specify a merge-base for the merge, and specifying multiple bases is\n> currently not supported. This option is incompatible with `--stdin`.\n> ++\n> +As the merge-base is provided directly, and do not need\n> +o specify commits; it is sufficient if they specify trees.\n>\n> [[OUTPUT]]\n> OUTPUT\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index 3bdec53fbe5..cbd8e15af6d 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n> struct merge_options opt;\n>\n> copy_merge_options(&opt, &o->merge_options);\n> - parent1 = get_merge_parent(branch1);\n> - if (!parent1)\n> - help_unknown_ref(branch1, \"merge-tree\",\n> - _(\"not something we can merge\"));\n> -\n> - parent2 = get_merge_parent(branch2);\n> - if (!parent2)\n> - help_unknown_ref(branch2, \"merge-tree\",\n> - _(\"not something we can merge\"));\n> -\n> opt.show_rename_progress = 0;\n>\n> opt.branch1 = branch1;\n> opt.branch2 = branch2;\n\nIf branch1 and branch2 refer to trees, then when users hit conflicts\nthey'll see e.g.\n\n<<<<<<< aaaaaaa\n somecode();\n=======\n othercode();\n>>>>>>> bbbbbbb\n\nbut aaaaaaa and bbbbbbb are not commits that they can find. They may\ndiscover how to show the contents of the tree objects (most users I\nrun into seem to be unaware that hashes in git can refer to anything\nother than commits), but they certainly won't get any context from\ngit-log as they might hope.\n\nThe other places in the code that deal directly with merging trees,\ngit-am and git-merge-recursive, both provide specially overridden\nvalues for both branch1 and branch2. (They probably ought to do\nsomething with opt->ancestor as well, but that's been the single ugly\nedge case problem that is solely responsible for merge-recursive not\nhaving been fully replaced by merge-ort yet and I haven't wanted to go\nback and mess with it.)\n\nThat raises the question: if the user passes trees in, should we\nrequire helpful names be provided as additional parameters? Or are\nthe usecases such that we don't expect callers to have any useful\ninformation about where these trees come from and these suboptimal\nconflicts are the best we can do?\n"},{"id":"487603","messageId":"xmqqbk92bv43.fsf@gitster.g","threadId":"60796","inReplyTo":"CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T17:15:40Z","receivedAt":"2024-01-30T17:15:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> ---merge-base=::\n>> +--merge-base=::\n>\n> A very minor point, but any chance we can just use ``, like\n> git-restore does? I've never liked the '-ish' that we use as it seems\n> to throw users, and I think they understand that they can use a commit\n> or a tag where a tree is expected\n\nYou are right that the \"X-ish\" was invented exactly so that folks\n(including the nitpicky types among us) can tell if only \"X\" is\naccepted, or other types of objects that uniquely can be peeled to a\n\"X\" also can be used. Many commands at the Porcelain level may take\na commit or a tag that eventually peel to a tree where at the lowest\nlevel we only need it to be a tree, but it still matters in some\nlower level corners of the system where the plumbing commands that\nonly accept a tree but not a tree-ish exist. We've discussed this\nin the past, e.g.:\n\n https://lore.kernel.org/git/7voc8ihq4a.fsf@alter.siamese.dyndns.org/\n\nIn general, I am OK to update the documentation and usage strings to\ndrop the \"-ish\" suffix when it is clear from the context. But what\ndoes it exactly mean that \"--merge-base=\" is meant to take any\ntree-ish from the context of this documentation we are discussing?\n\n - Is it because merge-tree is a Porcelain command and we would\n adopt \"Porcelains are to peel liberally what they accept, and\n when they are documented to take X they always accept any X-ish\"\n rule?\n\n - Is it because the description that follows \"--merge-base=\"\n header does not mention \"contrary to our usual convention, this\n option only accepts a tree and not a commit or a tag that points\n at a tree\" and we would adopt \"all commands and options that are\n documented to take X take X-ish, unless explicitly documented\n they only take X\"?\n\nAs long as we clearly spell out such a ground rule and make sure\nreaders of the documentation understand it, I will not object to\nsuch a tree-wide clean-up. The current ground rule is \"We write X\nwhen we mean to take only X and we write X-ish otherwise\", if I am\nnot mistaken.\n\n>> opt.branch1 = branch1;\n>> opt.branch2 = branch2;\n>\n> If branch1 and branch2 refer to trees, then when users hit conflicts\n> they'll see e.g.\n>\n> <<<<<<< aaaaaaa\n> somecode();\n> =======\n> othercode();\n>>>>>>>> bbbbbbb\n>\n> but aaaaaaa and bbbbbbb are not commits that they can find.\n\nCorrect. They are what they fed as two trees to be merged, aren't\nthey? They (or the script that drives merge-tree and fed these two\ntrees) should be recognise these two trees, as long as they are told\nthat is what we show, no?\n\n> That raises the question: if the user passes trees in, should we\n> require helpful names be provided as additional parameters?\n\nIf the user wants to give human-readable name to override whatever\nthe code would internally produce, such new options may help them,\nand should probably apply equally to input that happens to be a\ncommit (or a tag that is a tree-ish) as well, I would think.\n\n> Or are\n> the usecases such that we don't expect callers to have any useful\n> information about where these trees come from and these suboptimal\n> conflicts are the best we can do?\n\nI do not necessarily think the output is \"suboptimal\" from the point\nof view of our users, exactly because that is what they fed into the\ncommand themselves.\n"},{"id":"487609","messageId":"557604aa-dea4-995a-4fb2-a71b515a8129@gmx.de","threadId":"60796","inReplyTo":"CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-01-30T20:04:15Z","receivedAt":"2024-01-30T20:04:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Mon, 29 Jan 2024, Elijah Newren wrote:\n\n> On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > Documentation/git-merge-tree.txt | 5 +++-\n> > builtin/merge-tree.c | 42 +++++++++++++++++++-------------\n> > t/t4301-merge-tree-write-tree.sh | 8 ++++++\n> > 3 files changed, 37 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\n> > index b50acace3bc..214e30c70ba 100644\n> > --- a/Documentation/git-merge-tree.txt\n> > +++ b/Documentation/git-merge-tree.txt\n> > @@ -64,10 +64,13 @@ OPTIONS\n> > share no common history. This flag can be given to override that\n> > check and make the merge proceed anyway.\n> >\n> > ---merge-base=::\n> > +--merge-base=::\n>\n> A very minor point, but any chance we can just use ``, like\n> git-restore does? I've never liked the '-ish' that we use as it seems\n> to throw users, and I think they understand that they can use a commit\n> or a tag where a tree is expected\n\nThat's funny: I asked Victoria Dye to look over the patch, and she pointed\nout the exact opposite: I had written `` and she remarked that most\nof Git's manual pages would call this a `` :-)\n\nOn another funny note, I tried to establish the term \"ent\" for this category\nalmost 222 months ago because I also disliked the \"-ish\" convention:\nhttps://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/\n\n> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > index 3bdec53fbe5..cbd8e15af6d 100644\n> > --- a/builtin/merge-tree.c\n> > +++ b/builtin/merge-tree.c\n> > @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n> > struct merge_options opt;\n> >\n> > copy_merge_options(&opt, &o->merge_options);\n> > - parent1 = get_merge_parent(branch1);\n> > - if (!parent1)\n> > - help_unknown_ref(branch1, \"merge-tree\",\n> > - _(\"not something we can merge\"));\n> > -\n> > - parent2 = get_merge_parent(branch2);\n> > - if (!parent2)\n> > - help_unknown_ref(branch2, \"merge-tree\",\n> > - _(\"not something we can merge\"));\n> > -\n> > opt.show_rename_progress = 0;\n> >\n> > opt.branch1 = branch1;\n> > opt.branch2 = branch2;\n>\n> If branch1 and branch2 refer to trees, then when users hit conflicts\n> they'll see e.g.\n>\n> <<<<<<< aaaaaaa\n> somecode();\n> =======\n> othercode();\n> >>>>>>> bbbbbbb\n>\n> but aaaaaaa and bbbbbbb are not commits that they can find.\n\nThat is true. And it is not totally obvious to many users that they could\nthen call `git show aaaaaaa:file` to see the full pre-image on the\nfirst-parent side.\n\nOn the other hand, the label that is shown is precisely what the user\nspecified on the command-line. For example:\n\n\t$ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t\n\nwill result in the following conflict markers:\n\n\t$ git show 021c3ce211:t0091-bugreport.sh\n\t[...]\n\t<<<<<<< v2.43.0~11:t\n\t\tgrep usage output &&\n\t\ttest_path_is_missing git-bugreport-*\n\t'\n\n\ttest_expect_success 'incorrect positional arguments abort with usage and hint' '\n\t\ttest_must_fail git bugreport false 2>output &&\n\t\tgrep usage output &&\n\t\tgrep false output &&\n\t=======\n\t\ttest_grep usage output &&\n\t>>>>>>> v2.43.0~10^2:t\n\t[...]\n\nwhich I personally find very pleasing output.\n\nBesides, the manual page of `git merge-tree` says in no sugar-coated\nwords:\n\n\tDo NOT look through the resulting toplevel tree to try to find which\n\tfiles conflict; [...]\n\n:-)\n\nCiao,\nJohannes\n"},{"id":"487610","messageId":"71c5d2cb-46fb-b6f5-7180-d7b769dcc686@gmx.de","threadId":"60796","inReplyTo":"xmqqplxoozh9.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-01-30T20:05:29Z","receivedAt":"2024-01-30T20:05:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 26 Jan 2024, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When specifying a merge base explicitly, there is actually no good\n> > reason why the inputs need to be commits: that's only needed if the\n> > merge base has to be deduced from the commit graph.\n>\n> yes, Yes, YES, YEAHHHHH!\n\n:-D\n\n> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> > index 12ac4368736..71f21bb834f 100755\n> > --- a/t/t4301-merge-tree-write-tree.sh\n> > +++ b/t/t4301-merge-tree-write-tree.sh\n> > @@ -945,4 +945,12 @@ test_expect_success 'check the input format when --stdin is passed' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success '--merge-base with tree OIDs' '\n> > +\tgit merge-tree --merge-base=side1^ side1 side3 >tree &&\n> > +\ttree=$(cat tree) &&\n> > +\tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&\n> > +\ttree2=$(cat tree2) &&\n> > +\ttest $tree = $tree2\n> > +'\n>\n> You do not need $tree and $tree2 variables that would make it harder\n> to diagnose a failure case when we break merge-tree. You have tree\n> and tree2 as files, and I think it is sufficient to do\n>\n> \tgit merge-tree ... >result-from-commits &&\n> \tgit merge-tree ... >result-from-trees &&\n> \ttest_cmp result-from-commits result-from-trees\n\nThat's valuable feedback, thank you! As you saw, I changed it accordingly\nin v2.\n\nCiao,\nJohannes\n"},{"id":"487646","messageId":"CABPp-BEDLsVFxjr13XJX9eBLeqD+tRoeHxwJyPsc_+AgGY2GTw@mail.gmail.com","threadId":"60796","inReplyTo":"xmqqbk92bv43.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-31T16:34:13Z","receivedAt":"2024-01-31T16:34:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 30, 2024 at 9:15 AM Junio C Hamano wrote:\n>\n[...]\n> >> opt.branch1 = branch1;\n> >> opt.branch2 = branch2;\n> >\n> > If branch1 and branch2 refer to trees, then when users hit conflicts\n> > they'll see e.g.\n> >\n> > <<<<<<< aaaaaaa\n> > somecode();\n> > =======\n> > othercode();\n> >>>>>>>> bbbbbbb\n> >\n> > but aaaaaaa and bbbbbbb are not commits that they can find.\n>\n> Correct. They are what they fed as two trees to be merged, aren't\n> they? They (or the script that drives merge-tree and fed these two\n> trees) should be recognise these two trees, as long as they are told\n> that is what we show, no?\n>\n> > That raises the question: if the user passes trees in, should we\n> > require helpful names be provided as additional parameters?\n>\n> If the user wants to give human-readable name to override whatever\n> the code would internally produce, such new options may help them,\n> and should probably apply equally to input that happens to be a\n> commit (or a tag that is a tree-ish) as well, I would think.\n>\n> > Or are\n> > the usecases such that we don't expect callers to have any useful\n> > information about where these trees come from and these suboptimal\n> > conflicts are the best we can do?\n>\n> I do not necessarily think the output is \"suboptimal\" from the point\n> of view of our users, exactly because that is what they fed into the\n> command themselves.\n\nYeah, I was worried though that the end user wasn't the one running\nthe git-merge-tree command, and was trying to dig more into usage\ncases. Johannes example of using revision:path specification lessens\nmy worry, and you are right to point out that we can add additional\noptions in the future to allow overriding with more human readable\nnames.\n"},{"id":"487647","messageId":"CABPp-BF1p48hZ0Y7tRqKb8R53CjTB1oJGW_iV4dA29ALgbh8KQ@mail.gmail.com","threadId":"60796","inReplyTo":"557604aa-dea4-995a-4fb2-a71b515a8129@gmx.de","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-31T16:40:37Z","receivedAt":"2024-01-31T16:40:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Johannes,\n\nOn Tue, Jan 30, 2024 at 12:04 PM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Mon, 29 Jan 2024, Elijah Newren wrote:\n>\n> > On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > >\n[...]\n> That's funny: I asked Victoria Dye to look over the patch, and she pointed\n> out the exact opposite: I had written `` and she remarked that most\n> of Git's manual pages would call this a `` :-)\n\nA code review isn't complete until you get two contradictory requests, I guess?\n\n> On another funny note, I tried to establish the term \"ent\" for this category\n> almost 222 months ago because I also disliked the \"-ish\" convention:\n> https://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/\n\n:-)\n\n> > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > > index 3bdec53fbe5..cbd8e15af6d 100644\n> > > --- a/builtin/merge-tree.c\n> > > +++ b/builtin/merge-tree.c\n> > > @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,\n> > > struct merge_options opt;\n> > >\n> > > copy_merge_options(&opt, &o->merge_options);\n> > > - parent1 = get_merge_parent(branch1);\n> > > - if (!parent1)\n> > > - help_unknown_ref(branch1, \"merge-tree\",\n> > > - _(\"not something we can merge\"));\n> > > -\n> > > - parent2 = get_merge_parent(branch2);\n> > > - if (!parent2)\n> > > - help_unknown_ref(branch2, \"merge-tree\",\n> > > - _(\"not something we can merge\"));\n> > > -\n> > > opt.show_rename_progress = 0;\n> > >\n> > > opt.branch1 = branch1;\n> > > opt.branch2 = branch2;\n> >\n> > If branch1 and branch2 refer to trees, then when users hit conflicts\n> > they'll see e.g.\n> >\n> > <<<<<<< aaaaaaa\n> > somecode();\n> > =======\n> > othercode();\n> > >>>>>>> bbbbbbb\n> >\n> > but aaaaaaa and bbbbbbb are not commits that they can find.\n>\n> That is true. And it is not totally obvious to many users that they could\n> then call `git show aaaaaaa:file` to see the full pre-image on the\n> first-parent side.\n>\n> On the other hand, the label that is shown is precisely what the user\n> specified on the command-line.\n\nSo this is only for direct use? I was curious if the user was using\nsome other tool of yours, perhaps even some web GUI, and thus that\nsomething else was controlling what was passed to git-merge-tree.\n\n> For example:\n>\n> $ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t\n>\n> will result in the following conflict markers:\n>\n> $ git show 021c3ce211:t0091-bugreport.sh\n> [...]\n> <<<<<<< v2.43.0~11:t\n> grep usage output &&\n> test_path_is_missing git-bugreport-*\n> '\n>\n> test_expect_success 'incorrect positional arguments abort with usage and hint' '\n> test_must_fail git bugreport false 2>output &&\n> grep usage output &&\n> grep false output &&\n> =======\n> test_grep usage output &&\n> >>>>>>> v2.43.0~10^2:t\n> [...]\n>\n> which I personally find very pleasing output.\n\nOh, good point -- if users provide trees in the revision:path format\nthen they still have access to more information about why the change\nwas made via the revision.\n\nOf course, if users are using the tool directly, presumably they have\naccess to more information about where those trees came from anyway\neven without the conflict label.\n\n> Besides, the manual page of `git merge-tree` says in no sugar-coated\n> words:\n>\n> Do NOT look through the resulting toplevel tree to try to find which\n> files conflict; [...]\n>\n> :-)\n\nRight, but this isn't using the tree to find which files conflict;\nthey already are looking at the conflict. They are instead wanting to\nlearn why certain textual changes were made on one side of history to\nbetter inform how to resolve an otherwise less than obvious conflict\nresolution. At least, that's the only thing I've seen those conflict\nlabels be used for.\n"},{"id":"487657","messageId":"xmqqil3974kw.fsf@gitster.g","threadId":"60796","inReplyTo":"CABPp-BEDLsVFxjr13XJX9eBLeqD+tRoeHxwJyPsc_+AgGY2GTw@mail.gmail.com","subject":"Re: [PATCH] merge-tree: accept 3 trees as arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-31T18:14:39Z","receivedAt":"2024-01-31T18:14:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Tue, Jan 30, 2024 at 9:15 AM Junio C Hamano wrote:\n>> ...\n>> > but aaaaaaa and bbbbbbb are not commits that they can find.\n>>\n>> Correct. They are what they fed as two trees to be merged, aren't\n>> they? They (or the script that drives merge-tree and fed these two\n>> trees) should be recognise these two trees, as long as they are told\n\nI notice \"able to\" is missing here...\n\n>> that is what we show, no?\n> ...\n> Yeah, I was worried though that the end user wasn't the one running\n> the git-merge-tree command, and was trying to dig more into usage\n> cases.\n\nSure. That is exactly why I wrote \"They (or the script that\ndrives...\" above.\n"}]}