{"thread":{"id":"58480","subject":"[PATCH] merge-tree: fix segmentation fault in read-only repositories","startedAt":"2022-09-21T15:35:31Z","lastAt":"2022-09-29T01:50:39Z","messageCount":29,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Junio C Hamano","Johannes Schindelin","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463370","messageId":"pull.1362.git.1663774248660.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":null,"subject":"[PATCH] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-21T15:30:48Z","receivedAt":"2022-09-21T15:35:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIndependent of the question whether we want `git merge-tree` to report a\ntree name even when it failed to write the tree objects in a read-only\nrepository, there is no question that we should avoid a segmentation\nfault.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-tree: fix segmentation fault in read-only repositories\n \n Turns out that the segmentation fault reported by Taylor\n [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened\n while testing merge-ort in a read-only repository, and that the upstream\n version of git merge-tree is as affected as GitHub's internal version.\n \n Note: I briefly considered using the OID of the_hash_algo->empty_tree\n instead of null_oid() when no tree object could be constructed. However,\n I have come to the conclusion that this could potentially cause its own\n set of problems because it would relate to a valid tree object even if\n we do not have any valid tree object to play with.\n \n Also note: The question I hinted at in the commit message, namely\n whether or not to try harder to construct a tree object even if we\n cannot write it out, maybe merits a longer discussion, one that I think\n we should have after v2.38.0 is released, so as not to distract from\n focusing on v2.38.0.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1362\n\n builtin/merge-tree.c | 4 +++-\n t/t4301-merge-tree-write-tree.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex ae5782917b9..25c7142a882 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct commit_list *merge_bases = NULL;\n \tstruct merge_options opt;\n \tstruct merge_result result = { 0 };\n+\tconst struct object_id *tree_oid;\n \n \tparent1 = get_merge_parent(branch1);\n \tif (!parent1)\n@@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n \tif (o->show_messages == -1)\n \t\to->show_messages = !result.clean;\n \n-\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n+\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n+\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n \tif (!result.clean) {\n \t\tstruct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n \t\tconst char *last = NULL;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 28ca5c38bb5..e56b1ba6e50 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n+\tgit init --bare read-only &&\n+\tgit push read-only side1 side2 &&\n+\ttest_when_finished \"chmod -R u+w read-only\" &&\n+\tchmod -R a-w read-only &&\n+\ttest_must_fail git -C read-only merge-tree side1 side2\n+'\n+\n test_done\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \ngitgitgadget\n"},{"id":"463372","messageId":"Yysw/YIxx/JRF4Ph@nand.local","threadId":"58480","inReplyTo":"pull.1362.git.1663774248660.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-21T15:42:53Z","receivedAt":"2022-09-21T15:43:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index ae5782917b9..25c7142a882 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,\n> \tstruct commit_list *merge_bases = NULL;\n> \tstruct merge_options opt;\n> \tstruct merge_result result = { 0 };\n> +\tconst struct object_id *tree_oid;\n>\n> \tparent1 = get_merge_parent(branch1);\n> \tif (!parent1)\n> @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> \tif (o->show_messages == -1)\n> \t\to->show_messages = !result.clean;\n>\n> -\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> +\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> +\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n\nMy understanding is that we can get a clean result from\nmerge_incore_recursive(), but still have failed to write the physical\ntree object out?\n\nIn other words, the two sides *could* have been merged, but the actual\nresult of doing that merge couldn't be written to disk (e.g., because\nthe repository is read-only or some such)?\n\nIf so, then this approach makes sense, and I agree with your idea to\nuse the all-zeros OID instead of the empty tree one. It would be\nnice(r?) if we could just abort this command earlier, since `merge-tree`\npromises that we'll write the result out as an object. So I don't think\na non-zero exit before we have to print the resulting tree object would\nbe unexpected.\n\nBut I don't have a strong feeling about it either way. So, if you want\nto proceed here and just emit the all-zeros OID, I think that is a fine\napproach.\n\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 28ca5c38bb5..e56b1ba6e50 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' '\n> \ttest_cmp expect actual\n> '\n>\n> +test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n> +\tgit init --bare read-only &&\n> +\tgit push read-only side1 side2 &&\n> +\ttest_when_finished \"chmod -R u+w read-only\" &&\n\nDo we care about keeping this read-only repository around after the\ntest is done? It seems odd to have a directory called \"read-only\" be\nuser-writable. I'd probably just as soon replace this with:\n\n test_when_finished \"rm -fr read-only\" &&\n\nOtherwise, this patch looks good. Thanks for working on it!\n\nThanks,\nTaylor\n"},{"id":"463382","messageId":"xmqqedw438bq.fsf@gitster.g","threadId":"58480","inReplyTo":"pull.1362.git.1663774248660.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T18:12:41Z","receivedAt":"2022-09-21T18:12:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n> +\tgit init --bare read-only &&\n> +\tgit push read-only side1 side2 &&\n> +\ttest_when_finished \"chmod -R u+w read-only\" &&\n> +\tchmod -R a-w read-only &&\n> +\ttest_must_fail git -C read-only merge-tree side1 side2\n> +'\n\nDoesn't this test need a prerequisite to guard against those whose\nfilesystem lacks SANITY?\n\n\n"},{"id":"463388","messageId":"26922prr-pn98-40q9-q537-8sp986r07136@tzk.qr","threadId":"58480","inReplyTo":"Yysw/YIxx/JRF4Ph@nand.local","subject":"Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-21T20:12:20Z","receivedAt":"2022-09-21T20:12:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Wed, 21 Sep 2022, Taylor Blau wrote:\n\n> On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > index ae5782917b9..25c7142a882 100644\n> > --- a/builtin/merge-tree.c\n> > +++ b/builtin/merge-tree.c\n> > @@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,\n> > \tstruct commit_list *merge_bases = NULL;\n> > \tstruct merge_options opt;\n> > \tstruct merge_result result = { 0 };\n> > +\tconst struct object_id *tree_oid;\n> >\n> > \tparent1 = get_merge_parent(branch1);\n> > \tif (!parent1)\n> > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> > \tif (o->show_messages == -1)\n> > \t\to->show_messages = !result.clean;\n> >\n> > -\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> > +\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> > +\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n>\n> My understanding is that we can get a clean result from\n> merge_incore_recursive(), but still have failed to write the physical\n> tree object out?\n\nNo, we do not get a clean result _because_ we have failed to write the\ntree object.\n\n> In other words, the two sides *could* have been merged, but the actual\n> result of doing that merge couldn't be written to disk (e.g., because\n> the repository is read-only or some such)?\n\nSince we could not write the tree object, we cannot say whether the\noverall merge could have been merged or not.\n\nAll we can say is whether the merge has been clean _so far_ or not. Which\nis not very helpful.\n\nNow, taking that thought a bit further, I did get suspicious that my patch\nstill contained a bug, and lo and behold, this bug really was there: when\nthe merge _has_ been clean so far, we report success (via exit code 0),\neven if the tree could not be written, and therefore the `git merge-tree`\ncommand should have failed!\n\nI fixed the bug in the upcoming v2 and extended the test case to verify\nthat this bug no longer exists.\n\n> If so, then this approach makes sense, and I agree with your idea to\n> use the all-zeros OID instead of the empty tree one. It would be\n> nice(r?) if we could just abort this command earlier, since `merge-tree`\n> promises that we'll write the result out as an object. So I don't think\n> a non-zero exit before we have to print the resulting tree object would\n> be unexpected.\n\nThe idea of aborting the command earlier might make sense in isolation,\nand when you're familiar with Git's built-ins taking the easy `die()` way\nout upon failure without having to bother to release resources.\n\nHowever, when you keep in mind that the `merge-tree` command is far from\nits full potential and that really interesting ideas are still to be\nimplemented, some even well on their way like batched merges (see\nhttps://github.com/gitgitgadget/git/pull/1361), it starts to become a much\nbetter idea to keep the code as libified as possible, without any aborts.\nSuch an abort in the middle of the code path would interfere horribly with\nthe idea of batched merges.\n\n> But I don't have a strong feeling about it either way. So, if you want\n> to proceed here and just emit the all-zeros OID, I think that is a fine\n> approach.\n>\n> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> > index 28ca5c38bb5..e56b1ba6e50 100755\n> > --- a/t/t4301-merge-tree-write-tree.sh\n> > +++ b/t/t4301-merge-tree-write-tree.sh\n> > @@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n> > +\tgit init --bare read-only &&\n> > +\tgit push read-only side1 side2 &&\n> > +\ttest_when_finished \"chmod -R u+w read-only\" &&\n>\n> Do we care about keeping this read-only repository around after the\n> test is done? It seems odd to have a directory called \"read-only\" be\n> user-writable. I'd probably just as soon replace this with:\n>\n> test_when_finished \"rm -fr read-only\" &&\n\nSpeaking from way more experience debugging CI failures than I care to\nhave, it makes a lot of sense to keep this directory until the complete\ntest script finishes with utter success. Because if the test case fails,\nand the CI tars up the \"trash\" directory, and the crucial files to\ndiagnose what might have gone wrong are missing from that tar file, my own\nfuture life will be a lot harder than I like.\n\nCiao,\nDscho\n"},{"id":"463389","messageId":"p25141n2-p277-21o4-r8s3-4396491s41n8@tzk.qr","threadId":"58480","inReplyTo":"xmqqedw438bq.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-21T20:13:03Z","receivedAt":"2022-09-21T20:13:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 21 Sep 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n> > +\tgit init --bare read-only &&\n> > +\tgit push read-only side1 side2 &&\n> > +\ttest_when_finished \"chmod -R u+w read-only\" &&\n> > +\tchmod -R a-w read-only &&\n> > +\ttest_must_fail git -C read-only merge-tree side1 side2\n> > +'\n>\n> Doesn't this test need a prerequisite to guard against those whose\n> filesystem lacks SANITY?\n\nYes, of course!\n\nThank you for the sanity check (pun intended!),\nDscho\n"},{"id":"463393","messageId":"pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.git.1663774248660.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-21T22:08:03Z","receivedAt":"2022-09-21T22:08:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIndependent of the question whether we want `git merge-tree` to report\nthe tree name even when it failed to write the tree objects in a\nread-only repository, there is no question that we should avoid a\nsegmentation fault.\n\nAnd when we report an invalid tree name (because the tree could not be\nwritten), we need the exit status to be non-zero.\n\nLet's make it so.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-tree: fix segmentation fault in read-only repositories\n \n Turns out that the segmentation fault reported by Taylor\n [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened\n while testing merge-ort in a read-only repository, and that the upstream\n version of git merge-tree is as affected as GitHub's internal version.\n \n Note: I briefly considered using the OID of the_hash_algo->empty_tree\n instead of null_oid() when no tree object could be constructed. However,\n I have come to the conclusion that this could potentially cause its own\n set of problems because it would relate to a valid tree object even if\n we do not have any valid tree object to play with.\n \n Also note: The question I hinted at in the commit message, namely\n whether or not to try harder to construct a tree object even if we\n cannot write it out, maybe merits a longer discussion, one that I think\n we should have after v2.38.0 is released, so as not to distract from\n focusing on v2.38.0.\n \n Changes since v1:\n \n * Using the SANITY prereq now\n * If the merge was aborted due to a write error, exit with a non-zero\n code even if the (potentially partial) merge is clean\n * The test case now also verifies that the git merge-tree command fails\n in a read-only repository even if the merge would have succeeded\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1362\n\nRange-diff vs v1:\n\n 1: beba3fd9a95 ! 1: 1de4df3f471 merge-tree: fix segmentation fault in read-only repositories\n @@ Metadata\n ## Commit message ##\n merge-tree: fix segmentation fault in read-only repositories\n \n - Independent of the question whether we want `git merge-tree` to report a\n - tree name even when it failed to write the tree objects in a read-only\n - repository, there is no question that we should avoid a segmentation\n - fault.\n + Independent of the question whether we want `git merge-tree` to report\n + the tree name even when it failed to write the tree objects in a\n + read-only repository, there is no question that we should avoid a\n + segmentation fault.\n +\n + And when we report an invalid tree name (because the tree could not be\n + written), we need the exit status to be non-zero.\n +\n + Let's make it so.\n \n Signed-off-by: Johannes Schindelin \n \n @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n \tif (!result.clean) {\n \t\tstruct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n \t\tconst char *last = NULL;\n +@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n + \t\t\t\t\t &result);\n + \t}\n + \tmerge_finalize(&opt, &result);\n +-\treturn !result.clean; /* result.clean < 0 handled above */\n ++\treturn !result.tree || !result.clean; /* result.clean < 0 handled above */\n + }\n + \n + int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \n ## t/t4301-merge-tree-write-tree.sh ##\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n -+test_expect_success 'merge-ort fails gracefully in a read-only repository' '\n ++test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n +\tgit init --bare read-only &&\n -+\tgit push read-only side1 side2 &&\n ++\tgit push read-only side1 side2 side3 &&\n +\ttest_when_finished \"chmod -R u+w read-only\" &&\n +\tchmod -R a-w read-only &&\n ++\ttest_must_fail git -C read-only merge-tree side1 side3 &&\n +\ttest_must_fail git -C read-only merge-tree side1 side2\n +'\n +\n\n\n builtin/merge-tree.c | 6 ++++--\n t/t4301-merge-tree-write-tree.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex ae5782917b9..0df24eb82d4 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct commit_list *merge_bases = NULL;\n \tstruct merge_options opt;\n \tstruct merge_result result = { 0 };\n+\tconst struct object_id *tree_oid;\n \n \tparent1 = get_merge_parent(branch1);\n \tif (!parent1)\n@@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n \tif (o->show_messages == -1)\n \t\to->show_messages = !result.clean;\n \n-\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n+\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n+\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n \tif (!result.clean) {\n \t\tstruct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n \t\tconst char *last = NULL;\n@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n \t\t\t\t\t &result);\n \t}\n \tmerge_finalize(&opt, &result);\n-\treturn !result.clean; /* result.clean < 0 handled above */\n+\treturn !result.tree || !result.clean; /* result.clean < 0 handled above */\n }\n \n int cmd_merge_tree(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 28ca5c38bb5..013b77144bd 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n+\tgit init --bare read-only &&\n+\tgit push read-only side1 side2 side3 &&\n+\ttest_when_finished \"chmod -R u+w read-only\" &&\n+\tchmod -R a-w read-only &&\n+\ttest_must_fail git -C read-only merge-tree side1 side3 &&\n+\ttest_must_fail git -C read-only merge-tree side1 side2\n+'\n+\n test_done\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \ngitgitgadget\n"},{"id":"463404","messageId":"xmqqfsgk1i2t.fsf@gitster.g","threadId":"58480","inReplyTo":"pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T22:24:58Z","receivedAt":"2022-09-21T22:25:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +\tconst struct object_id *tree_oid;\n> \n> \tparent1 = get_merge_parent(branch1);\n> \tif (!parent1)\n> @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> \tif (o->show_messages == -1)\n> \t\to->show_messages = !result.clean;\n> \n> -\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> +\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> +\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n\nAs we are not using tree_oid anywhere else, it might be cleaner to\nswitch on result.tree more explicitly, especially given that the\nreturn statement also uses result.tree to decide what exit status to\nuse, e.g.\n\n\tif (result.tree)\n\t\tfputs(oid_to_hex(&result.tree->object.oid), stdout);\n\telse\n\t\tfputs(oid_to_hex(null_oid()), stdout);\n\tfputc(line_termination, stdout);\n\t\nbut that is merely \"might be cleaner\".\n\n> -\treturn !result.clean; /* result.clean < 0 handled above */\n> +\treturn !result.tree || !result.clean; /* result.clean < 0 handled above */\n\nThe original returned 1 when we failed to cleanly merge and 0 when\nwe cleanly merged. Now, if we failed to come up with a merged tree,\nresult.tree would be NULL and we return 1 from the function, because\nwe failed to cleanly merge.\n\nOK. These negations make my head spin X-<.\n\n"},{"id":"463406","messageId":"CABPp-BGUiDXxv8eQhKQXHcem3ke9e=Q99a_FDExZ5XZYUgir6A@mail.gmail.com","threadId":"58480","inReplyTo":"pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-21T22:56:37Z","receivedAt":"2022-09-21T22:56:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> Independent of the question whether we want `git merge-tree` to report\n> the tree name even when it failed to write the tree objects in a\n> read-only repository, there is no question that we should avoid a\n> segmentation fault.\n\nAh, a read-only repo. Looks like we didn't bother to check the return\nstatus of write_object_file() in one of the two cases; oops. (And it\nlooks like the other case does not correctly propagate the error\nupwards far enough...)\n\n> And when we report an invalid tree name (because the tree could not be\n> written), we need the exit status to be non-zero.\n>\n> Let's make it so.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-tree: fix segmentation fault in read-only repositories\n>\n> Turns out that the segmentation fault reported by Taylor\n> [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened\n> while testing merge-ort in a read-only repository, and that the upstream\n> version of git merge-tree is as affected as GitHub's internal version.\n>\n> Note: I briefly considered using the OID of the_hash_algo->empty_tree\n> instead of null_oid() when no tree object could be constructed. However,\n> I have come to the conclusion that this could potentially cause its own\n> set of problems because it would relate to a valid tree object even if\n> we do not have any valid tree object to play with.\n>\n> Also note: The question I hinted at in the commit message, namely\n> whether or not to try harder to construct a tree object even if we\n> cannot write it out, maybe merits a longer discussion, one that I think\n> we should have after v2.38.0 is released, so as not to distract from\n> focusing on v2.38.0.\n\nThat's fair, but you know I'm going to have a hard time refraining\nfrom commenting on it anyway... :-)\n\n[...]\n> @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> if (o->show_messages == -1)\n> o->show_messages = !result.clean;\n>\n> - printf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> + tree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> + printf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n\nPerhaps also print a warning to stderr when result.tree is NULL?\n\n> if (!result.clean) {\n> struct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n> const char *last = NULL;\n> @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n> &result);\n> }\n> merge_finalize(&opt, &result);\n> - return !result.clean; /* result.clean < 0 handled above */\n> + return !result.tree || !result.clean; /* result.clean < 0 handled above */\n\nThinking out loud, should this logic be at the merge-ort.c level,\nperhaps something like this:\n\n@@ -4940,6 +4941,9 @@ static void\nmerge_ort_nonrecursive_internal(struct merge_options *opt,\n result->tree = parse_tree_indirect(&working_tree_oid);\n /* existence of conflicted entries implies unclean */\n result->clean &= strmap_empty(&opt->priv->conflicted);\n+ if (!result->tree)\n+ /* This shouldn't happen, because if we did fail to\nwrite a tree we should have returned early before getting here. But\njust in case we have some bugs... */\n+ result->clean = -1;\n if (!opt->priv->call_depth) {\n result->priv = opt->priv;\n result->_properly_initialized = RESULT_INITIALIZED;\n\nThat might benefit callers other than merge-tree, though maybe it\nmakes it harder to print a helpful error message (unless we're fine\nwith the library always throwing one?)\n\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 28ca5c38bb5..013b77144bd 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n> + git init --bare read-only &&\n> + git push read-only side1 side2 side3 &&\n> + test_when_finished \"chmod -R u+w read-only\" &&\n> + chmod -R a-w read-only &&\n> + test_must_fail git -C read-only merge-tree side1 side3 &&\n> + test_must_fail git -C read-only merge-tree side1 side2\n> +'\n> +\n> test_done\n>\n> base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n> --\n> gitgitgadget\n\nThis is a reasonable workaround from the merge-tree side, if we expect\nmerge-ort to not correctly notify us of issues (which may be fair\ngiven that a quick glance suggests we have more than one problematic\ncodepath, which I'll discuss more below). However, I do think\nmerge-ort should be returning a negative value for the \"clean\" status\nin such a case. If merge-ort did that, then we wouldn't even get to\nyour new code because merge-tree.c already checks for that:\n\n merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);\n if (result.clean < 0)\n die(_(\"failure to merge\"));\n\nI have a hack above which sets a negative return value, but it's only\na workaround since it comes from a late detect-after-the-fact\nlocation. Here's another ugly hack, but one which highlights exactly\nwhere the real problem occurs:\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 99dcee2db8..c2144e9220 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3605,7 +3605,8 @@ static void write_tree(struct object_id *result_oid,\n }\n\n /* Write this object file out, and record in result_oid */\n- write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+ if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n+ die(_(\"Unable to add new tree to database\"));\n strbuf_release(&buf);\n }\n\ndie()ing, of course, is not a good choice, we should really return -1\ninstead. Unfortunately, the callers aren't currently prepared to\npropagate such a value upwards (as reflected in the return type of the\nfunction) so any fix in this area is a little more involved than just\nmodifying these few lines. Also, there is one other call to\nwrite_object_file() in merge-ort.c which does check and return a value\nof -1, though looking around it appears the callers of that other site\ndo not always correctly check and propagate a return value of -1\nfurther upwards as they should, meaning we are losing track of the\nfact that the merge failed to function.\n\nGiven the lack of correct propagation of -1 return values in the other\ncodepath plus the error here, keeping some form of workaround sounds\nfair (though it may be nice to print an error that something fishy\nhappened).\n\nAnd then, possibly post-v2.38.0 though we may be able to get it in\nbefore, getting correct propagation of a -1 return value from the\nsource of the error would be good. Would you like to look into that,\nor would you prefer I did?\n"},{"id":"463407","messageId":"xmqqpmfoz5kj.fsf@gitster.g","threadId":"58480","inReplyTo":"CABPp-BGUiDXxv8eQhKQXHcem3ke9e=Q99a_FDExZ5XZYUgir6A@mail.gmail.com","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T23:11:08Z","receivedAt":"2022-09-21T23:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n>> &result);\n>> }\n>> merge_finalize(&opt, &result);\n>> - return !result.clean; /* result.clean < 0 handled above */\n>> + return !result.tree || !result.clean; /* result.clean < 0 handled above */\n>\n> Thinking out loud, should this logic be at the merge-ort.c level,\n\nTrue.\n\n> ... However, I do think\n> merge-ort should be returning a negative value for the \"clean\" status\n> in such a case.\n\nTrue, too.\n"},{"id":"463456","messageId":"322o3q9q-4559-63n5-9o56-q16463o3q7p6@tzk.qr","threadId":"58480","inReplyTo":"CABPp-BGUiDXxv8eQhKQXHcem3ke9e=Q99a_FDExZ5XZYUgir6A@mail.gmail.com","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-22T17:24:14Z","receivedAt":"2022-09-22T17:24:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\njust quickly...\n\nOn Wed, 21 Sep 2022, Elijah Newren wrote:\n\n> On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n> > &result);\n> > }\n> > merge_finalize(&opt, &result);\n> > - return !result.clean; /* result.clean < 0 handled above */\n> > + return !result.tree || !result.clean; /* result.clean < 0 handled above */\n>\n> Thinking out loud, should this logic be at the merge-ort.c level,\n\nYou're right, of course. I have pushed up a tentative v3 that does it as\nyou proposed, and it looks not half bad. I'll look a bit more deeply,\nstill, but chances are that I'll submit this later today as v3 as-is.\n\nAnd then I will reply a bit more verbosely, either ;-)\n\nCiao,\nDscho\n"},{"id":"463470","messageId":"pull.1362.v3.git.1663875999939.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com","subject":"[PATCH v3] merge-ort: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T19:46:39Z","receivedAt":"2022-09-22T19:46:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIf the blob/tree objects cannot be written, we really need the merge\noperations to fail, and not to continue (and then try to access the tree\nobject which is however still set to `NULL`).\n\nLet's stop ignoring the return value of `write_object_file()` and\n`write_tree()` and set `clean = -1` in the error case.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-tree: fix segmentation fault in read-only repositories\n \n Turns out that the segmentation fault reported by Taylor\n [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened\n while testing merge-ort in a read-only repository, and that the upstream\n version of git merge-tree is as affected as GitHub's internal version.\n \n Changes since v2:\n \n * Completely changed the approach by no longer touching\n builtin/merge-tree.c but instead fixing the underlying merge-ort\n layer: we no longer ignore the return value of write_tree() and\n write_object_file().\n \n Changes since v1:\n \n * Using the SANITY prereq now\n * If the merge was aborted due to a write error, exit with a non-zero\n code even if the (potentially partial) merge is clean\n * The test case now also verifies that the git merge-tree command fails\n in a read-only repository even if the merge would have succeeded\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1362\n\nRange-diff vs v2:\n\n 1: 1de4df3f471 ! 1: 198ff455f90 merge-tree: fix segmentation fault in read-only repositories\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - merge-tree: fix segmentation fault in read-only repositories\n + merge-ort: fix segmentation fault in read-only repositories\n \n - Independent of the question whether we want `git merge-tree` to report\n - the tree name even when it failed to write the tree objects in a\n - read-only repository, there is no question that we should avoid a\n - segmentation fault.\n + If the blob/tree objects cannot be written, we really need the merge\n + operations to fail, and not to continue (and then try to access the tree\n + object which is however still set to `NULL`).\n \n - And when we report an invalid tree name (because the tree could not be\n - written), we need the exit status to be non-zero.\n -\n - Let's make it so.\n + Let's stop ignoring the return value of `write_object_file()` and\n + `write_tree()` and set `clean = -1` in the error case.\n \n Signed-off-by: Johannes Schindelin \n \n - ## builtin/merge-tree.c ##\n -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n - \tstruct commit_list *merge_bases = NULL;\n - \tstruct merge_options opt;\n - \tstruct merge_result result = { 0 };\n -+\tconst struct object_id *tree_oid;\n - \n - \tparent1 = get_merge_parent(branch1);\n - \tif (!parent1)\n -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n - \tif (o->show_messages == -1)\n - \t\to->show_messages = !result.clean;\n - \n --\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n -+\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n -+\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n - \tif (!result.clean) {\n - \t\tstruct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n - \t\tconst char *last = NULL;\n -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n - \t\t\t\t\t &result);\n + ## merge-ort.c ##\n +@@ merge-ort.c: static int tree_entry_order(const void *a_, const void *b_)\n + \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n + }\n + \n +-static void write_tree(struct object_id *result_oid,\n +-\t\t struct string_list *versions,\n +-\t\t unsigned int offset,\n +-\t\t size_t hash_size)\n ++static int write_tree(struct object_id *result_oid,\n ++\t\t struct string_list *versions,\n ++\t\t unsigned int offset,\n ++\t\t size_t hash_size)\n + {\n + \tsize_t maxlen = 0, extra;\n + \tunsigned int nr;\n + \tstruct strbuf buf = STRBUF_INIT;\n +-\tint i;\n ++\tint i, ret;\n + \n + \tassert(offset <= versions->nr);\n + \tnr = versions->nr - offset;\n +@@ merge-ort.c: static void write_tree(struct object_id *result_oid,\n + \t}\n + \n + \t/* Write this object file out, and record in result_oid */\n +-\twrite_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n ++\tret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n + \tstrbuf_release(&buf);\n ++\treturn ret;\n + }\n + \n + static void record_entry_for_tree(struct directory_versions *dir_metadata,\n +@@ merge-ort.c: static void record_entry_for_tree(struct directory_versions *dir_metadata,\n + \t\t\t basename)->util = &mi->result;\n + }\n + \n +-static void write_completed_directory(struct merge_options *opt,\n +-\t\t\t\t const char *new_directory_name,\n +-\t\t\t\t struct directory_versions *info)\n ++static int write_completed_directory(struct merge_options *opt,\n ++\t\t\t\t const char *new_directory_name,\n ++\t\t\t\t struct directory_versions *info)\n + {\n + \tconst char *prev_dir;\n + \tstruct merged_info *dir_info = NULL;\n +-\tunsigned int offset;\n ++\tunsigned int offset, ret = 0;\n + \n + \t/*\n + \t * Some explanation of info->versions and info->offsets...\n +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,\n + \t * strcmp here.)\n + \t */\n + \tif (new_directory_name == info->last_directory)\n +-\t\treturn;\n ++\t\treturn 0;\n + \n + \t/*\n + \t * If we are just starting (last_directory is NULL), or last_directory\n +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,\n + \t\t */\n + \t\tstring_list_append(&info->offsets,\n + \t\t\t\t info->last_directory)->util = (void*)offset;\n +-\t\treturn;\n ++\t\treturn 0;\n + \t}\n + \n + \t/*\n +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,\n + \t\t */\n + \t\tdir_info->is_null = 0;\n + \t\tdir_info->result.mode = S_IFDIR;\n +-\t\twrite_tree(&dir_info->result.oid, &info->versions, offset,\n +-\t\t\t opt->repo->hash_algo->rawsz);\n ++\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n ++\t\t\t opt->repo->hash_algo->rawsz) < 0)\n ++\t\t\tret = -1;\n + \t}\n + \n + \t/*\n +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,\n + \t/* And, of course, we need to update last_directory to match. */\n + \tinfo->last_directory = new_directory_name;\n + \tinfo->last_directory_len = strlen(info->last_directory);\n ++\n ++\treturn ret;\n + }\n + \n + /* Per entry merge function */\n +@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,\n + \toid_array_clear(&to_fetch);\n + }\n + \n +-static void process_entries(struct merge_options *opt,\n +-\t\t\t struct object_id *result_oid)\n ++static int process_entries(struct merge_options *opt,\n ++\t\t\t struct object_id *result_oid)\n + {\n + \tstruct hashmap_iter iter;\n + \tstruct strmap_entry *e;\n +@@ merge-ort.c: static void process_entries(struct merge_options *opt,\n + \tstruct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n + \t\t\t\t\t\t STRING_LIST_INIT_NODUP,\n + \t\t\t\t\t\t NULL, 0 };\n ++\tint ret;\n + \n + \ttrace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n + \tif (strmap_empty(&opt->priv->paths)) {\n + \t\toidcpy(result_oid, opt->repo->hash_algo->empty_tree);\n +-\t\treturn;\n ++\t\treturn 0;\n \t}\n - \tmerge_finalize(&opt, &result);\n --\treturn !result.clean; /* result.clean < 0 handled above */\n -+\treturn !result.tree || !result.clean; /* result.clean < 0 handled above */\n + \n + \t/* Hack to pre-allocate plist to the desired size */\n +@@ merge-ort.c: static void process_entries(struct merge_options *opt,\n + \t\t */\n + \t\tstruct merged_info *mi = entry->util;\n + \n +-\t\twrite_completed_directory(opt, mi->directory_name,\n +-\t\t\t\t\t &dir_metadata);\n ++\t\tif (write_completed_directory(opt, mi->directory_name,\n ++\t\t\t\t\t &dir_metadata) < 0) {\n ++\t\t\tret = -1;\n ++\t\t\tgoto cleanup;\n ++\t\t}\n + \t\tif (mi->clean)\n + \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n + \t\telse {\n +@@ merge-ort.c: static void process_entries(struct merge_options *opt,\n + \t\tfflush(stdout);\n + \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n + \t}\n +-\twrite_tree(result_oid, &dir_metadata.versions, 0,\n +-\t\t opt->repo->hash_algo->rawsz);\n ++\tret = write_tree(result_oid, &dir_metadata.versions, 0,\n ++\t\t\t opt->repo->hash_algo->rawsz);\n ++cleanup:\n + \tstring_list_clear(&plist, 0);\n + \tstring_list_clear(&dir_metadata.versions, 0);\n + \tstring_list_clear(&dir_metadata.offsets, 0);\n + \ttrace2_region_leave(\"merge\", \"process_entries cleanup\", opt->repo);\n ++\n ++\treturn ret;\n }\n \n - int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n + /*** Function Grouping: functions related to merge_switch_to_result() ***/\n +@@ merge-ort.c: redo:\n + \t}\n + \n + \ttrace2_region_enter(\"merge\", \"process_entries\", opt->repo);\n +-\tprocess_entries(opt, &working_tree_oid);\n ++\tif (process_entries(opt, &working_tree_oid) < 0)\n ++\t\tresult->clean = -1;\n + \ttrace2_region_leave(\"merge\", \"process_entries\", opt->repo);\n + \n + \t/* Set return values */\n + \tresult->path_messages = &opt->priv->conflicts;\n + \n +-\tresult->tree = parse_tree_indirect(&working_tree_oid);\n +-\t/* existence of conflicted entries implies unclean */\n +-\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n ++\tif (result->clean >= 0) {\n ++\t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n ++\t\t/* existence of conflicted entries implies unclean */\n ++\t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n ++\t}\n + \tif (!opt->priv->call_depth) {\n + \t\tresult->priv = opt->priv;\n + \t\tresult->_properly_initialized = RESULT_INITIALIZED;\n \n ## t/t4301-merge-tree-write-tree.sh ##\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'can override merge of unrelated histories' '\n\n\n merge-ort.c | 64 +++++++++++++++++++-------------\n t/t4301-merge-tree-write-tree.sh | 9 +++++\n 2 files changed, 48 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 99dcee2db8a..f654296220e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)\n \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n }\n \n-static void write_tree(struct object_id *result_oid,\n-\t\t struct string_list *versions,\n-\t\t unsigned int offset,\n-\t\t size_t hash_size)\n+static int write_tree(struct object_id *result_oid,\n+\t\t struct string_list *versions,\n+\t\t unsigned int offset,\n+\t\t size_t hash_size)\n {\n \tsize_t maxlen = 0, extra;\n \tunsigned int nr;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint i;\n+\tint i, ret;\n \n \tassert(offset <= versions->nr);\n \tnr = versions->nr - offset;\n@@ -3605,8 +3605,9 @@ static void write_tree(struct object_id *result_oid,\n \t}\n \n \t/* Write this object file out, and record in result_oid */\n-\twrite_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+\tret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n \tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static void record_entry_for_tree(struct directory_versions *dir_metadata,\n@@ -3625,13 +3626,13 @@ static void record_entry_for_tree(struct directory_versions *dir_metadata,\n \t\t\t basename)->util = &mi->result;\n }\n \n-static void write_completed_directory(struct merge_options *opt,\n-\t\t\t\t const char *new_directory_name,\n-\t\t\t\t struct directory_versions *info)\n+static int write_completed_directory(struct merge_options *opt,\n+\t\t\t\t const char *new_directory_name,\n+\t\t\t\t struct directory_versions *info)\n {\n \tconst char *prev_dir;\n \tstruct merged_info *dir_info = NULL;\n-\tunsigned int offset;\n+\tunsigned int offset, ret = 0;\n \n \t/*\n \t * Some explanation of info->versions and info->offsets...\n@@ -3717,7 +3718,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t * strcmp here.)\n \t */\n \tif (new_directory_name == info->last_directory)\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If we are just starting (last_directory is NULL), or last_directory\n@@ -3739,7 +3740,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tstring_list_append(&info->offsets,\n \t\t\t\t info->last_directory)->util = (void*)offset;\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/*\n@@ -3769,8 +3770,9 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tdir_info->is_null = 0;\n \t\tdir_info->result.mode = S_IFDIR;\n-\t\twrite_tree(&dir_info->result.oid, &info->versions, offset,\n-\t\t\t opt->repo->hash_algo->rawsz);\n+\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n+\t\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\t\tret = -1;\n \t}\n \n \t/*\n@@ -3798,6 +3800,8 @@ static void write_completed_directory(struct merge_options *opt,\n \t/* And, of course, we need to update last_directory to match. */\n \tinfo->last_directory = new_directory_name;\n \tinfo->last_directory_len = strlen(info->last_directory);\n+\n+\treturn ret;\n }\n \n /* Per entry merge function */\n@@ -4214,8 +4218,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,\n \toid_array_clear(&to_fetch);\n }\n \n-static void process_entries(struct merge_options *opt,\n-\t\t\t struct object_id *result_oid)\n+static int process_entries(struct merge_options *opt,\n+\t\t\t struct object_id *result_oid)\n {\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n@@ -4224,11 +4228,12 @@ static void process_entries(struct merge_options *opt,\n \tstruct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t NULL, 0 };\n+\tint ret;\n \n \ttrace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n \tif (strmap_empty(&opt->priv->paths)) {\n \t\toidcpy(result_oid, opt->repo->hash_algo->empty_tree);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/* Hack to pre-allocate plist to the desired size */\n@@ -4270,8 +4275,11 @@ static void process_entries(struct merge_options *opt,\n \t\t */\n \t\tstruct merged_info *mi = entry->util;\n \n-\t\twrite_completed_directory(opt, mi->directory_name,\n-\t\t\t\t\t &dir_metadata);\n+\t\tif (write_completed_directory(opt, mi->directory_name,\n+\t\t\t\t\t &dir_metadata) < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tif (mi->clean)\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n \t\telse {\n@@ -4291,12 +4299,15 @@ static void process_entries(struct merge_options *opt,\n \t\tfflush(stdout);\n \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n \t}\n-\twrite_tree(result_oid, &dir_metadata.versions, 0,\n-\t\t opt->repo->hash_algo->rawsz);\n+\tret = write_tree(result_oid, &dir_metadata.versions, 0,\n+\t\t\t opt->repo->hash_algo->rawsz);\n+cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n \tstring_list_clear(&dir_metadata.offsets, 0);\n \ttrace2_region_leave(\"merge\", \"process_entries cleanup\", opt->repo);\n+\n+\treturn ret;\n }\n \n /*** Function Grouping: functions related to merge_switch_to_result() ***/\n@@ -4928,15 +4939,18 @@ redo:\n \t}\n \n \ttrace2_region_enter(\"merge\", \"process_entries\", opt->repo);\n-\tprocess_entries(opt, &working_tree_oid);\n+\tif (process_entries(opt, &working_tree_oid) < 0)\n+\t\tresult->clean = -1;\n \ttrace2_region_leave(\"merge\", \"process_entries\", opt->repo);\n \n \t/* Set return values */\n \tresult->path_messages = &opt->priv->conflicts;\n \n-\tresult->tree = parse_tree_indirect(&working_tree_oid);\n-\t/* existence of conflicted entries implies unclean */\n-\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\tif (result->clean >= 0) {\n+\t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\t/* existence of conflicted entries implies unclean */\n+\t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\t}\n \tif (!opt->priv->call_depth) {\n \t\tresult->priv = opt->priv;\n \t\tresult->_properly_initialized = RESULT_INITIALIZED;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 28ca5c38bb5..013b77144bd 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n+\tgit init --bare read-only &&\n+\tgit push read-only side1 side2 side3 &&\n+\ttest_when_finished \"chmod -R u+w read-only\" &&\n+\tchmod -R a-w read-only &&\n+\ttest_must_fail git -C read-only merge-tree side1 side3 &&\n+\ttest_must_fail git -C read-only merge-tree side1 side2\n+'\n+\n test_done\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \ngitgitgadget\n"},{"id":"463471","messageId":"n920r07n-8443-2r5o-09pr-q20r99q6o12o@tzk.qr","threadId":"58480","inReplyTo":"CABPp-BGUiDXxv8eQhKQXHcem3ke9e=Q99a_FDExZ5XZYUgir6A@mail.gmail.com","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-22T19:50:34Z","receivedAt":"2022-09-22T19:50:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nnow that I have studied more of the code, hunted down Coverity reports\nabout `merge-ort.c` and `builtin/merge-tree.c` and essentially have grown\na lot more confidence about my patch, I'll take the time to respond in a\nbit more detail.\n\nOn Wed, 21 Sep 2022, Elijah Newren wrote:\n\n> On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > From: Johannes Schindelin \n> >\n> > Independent of the question whether we want `git merge-tree` to report\n> > the tree name even when it failed to write the tree objects in a\n> > read-only repository, there is no question that we should avoid a\n> > segmentation fault.\n>\n> Ah, a read-only repo. Looks like we didn't bother to check the return\n> status of write_object_file() in one of the two cases; oops. (And it\n> looks like the other case does not correctly propagate the error\n> upwards far enough...)\n\nIndeed, that was the actual root cause, but I had failed to even look\nwhether the return values of `write_tree()` and `write_object_file()` were\nrespected. Narrator's voice: they weren't.\n\n> > And when we report an invalid tree name (because the tree could not be\n> > written), we need the exit status to be non-zero.\n> >\n> > Let's make it so.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > merge-tree: fix segmentation fault in read-only repositories\n> >\n> > Turns out that the segmentation fault reported by Taylor\n> > [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened\n> > while testing merge-ort in a read-only repository, and that the upstream\n> > version of git merge-tree is as affected as GitHub's internal version.\n> >\n> > Note: I briefly considered using the OID of the_hash_algo->empty_tree\n> > instead of null_oid() when no tree object could be constructed. However,\n> > I have come to the conclusion that this could potentially cause its own\n> > set of problems because it would relate to a valid tree object even if\n> > we do not have any valid tree object to play with.\n> >\n> > Also note: The question I hinted at in the commit message, namely\n> > whether or not to try harder to construct a tree object even if we\n> > cannot write it out, maybe merits a longer discussion, one that I think\n> > we should have after v2.38.0 is released, so as not to distract from\n> > focusing on v2.38.0.\n>\n> That's fair, but you know I'm going to have a hard time refraining\n> from commenting on it anyway... :-)\n\nHahaha, of course you would ;-)\n\n> [...]\n> > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> > if (o->show_messages == -1)\n> > o->show_messages = !result.clean;\n> >\n> > - printf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> > + tree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> > + printf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n>\n> Perhaps also print a warning to stderr when result.tree is NULL?\n\nI ended up not even touching `builtin/merge-tree.c` but instead ensuring\nthat `result.clean` is negative if we fail to write an object (which could\nhappen even in read/write repositories, think \"disk full\").\n\nAs a consequence, we do not even reach this `printf()` anymore, as you\npointed out, a negative `result.clean` is handled much earlier.\n\nIt is handled via a `die()` in `real_merge()`, and that will need to\nchange, I think, if we want to continue on the batched merges.\n\n> > if (!result.clean) {\n> > struct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n> > const char *last = NULL;\n> > @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n> > &result);\n> > }\n> > merge_finalize(&opt, &result);\n> > - return !result.clean; /* result.clean < 0 handled above */\n> > + return !result.tree || !result.clean; /* result.clean < 0 handled above */\n>\n> Thinking out loud, should this logic be at the merge-ort.c level,\n> perhaps something like this:\n>\n> @@ -4940,6 +4941,9 @@ static void\n> merge_ort_nonrecursive_internal(struct merge_options *opt,\n> result->tree = parse_tree_indirect(&working_tree_oid);\n> /* existence of conflicted entries implies unclean */\n> result->clean &= strmap_empty(&opt->priv->conflicted);\n> + if (!result->tree)\n> + /* This shouldn't happen, because if we did fail to\n> write a tree we should have returned early before getting here. But\n> just in case we have some bugs... */\n> + result->clean = -1;\n> if (!opt->priv->call_depth) {\n> result->priv = opt->priv;\n> result->_properly_initialized = RESULT_INITIALIZED;\n>\n> That might benefit callers other than merge-tree, though maybe it\n> makes it harder to print a helpful error message (unless we're fine\n> with the library always throwing one?)\n\nThe error messages are already thrown about (this is how it looks like\nwith v3 of this patch):\n\n\t[...]\n\t+ git -C read-only merge-tree side1 side2\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add numbers to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add greeting to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\tfatal: failure to merge\n\t+ exit_code=128\n\t[...]\n\n> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> > index 28ca5c38bb5..013b77144bd 100755\n> > --- a/t/t4301-merge-tree-write-tree.sh\n> > +++ b/t/t4301-merge-tree-write-tree.sh\n> > @@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n> > + git init --bare read-only &&\n> > + git push read-only side1 side2 side3 &&\n> > + test_when_finished \"chmod -R u+w read-only\" &&\n> > + chmod -R a-w read-only &&\n> > + test_must_fail git -C read-only merge-tree side1 side3 &&\n> > + test_must_fail git -C read-only merge-tree side1 side2\n> > +'\n> > +\n> > test_done\n> >\n> > base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n> > --\n> > gitgitgadget\n>\n> This is a reasonable workaround from the merge-tree side, if we expect\n> merge-ort to not correctly notify us of issues (which may be fair\n> given that a quick glance suggests we have more than one problematic\n> codepath, which I'll discuss more below). However, I do think\n> merge-ort should be returning a negative value for the \"clean\" status\n> in such a case.\n\nYou're absolutely right. That's what v3 now does.\n\n> If merge-ort did that, then we wouldn't even get to your new code\n> because merge-tree.c already checks for that:\n>\n> merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);\n> if (result.clean < 0)\n> die(_(\"failure to merge\"));\n>\n> I have a hack above which sets a negative return value, but it's only\n> a workaround since it comes from a late detect-after-the-fact\n> location. Here's another ugly hack, but one which highlights exactly\n> where the real problem occurs:\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 99dcee2db8..c2144e9220 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3605,7 +3605,8 @@ static void write_tree(struct object_id *result_oid,\n> }\n>\n> /* Write this object file out, and record in result_oid */\n> - write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n> + if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n> + die(_(\"Unable to add new tree to database\"));\n> strbuf_release(&buf);\n> }\n>\n> die()ing, of course, is not a good choice, we should really return -1\n> instead. Unfortunately, the callers aren't currently prepared to\n> propagate such a value upwards (as reflected in the return type of the\n> function) so any fix in this area is a little more involved than just\n> modifying these few lines. Also, there is one other call to\n> write_object_file() in merge-ort.c which does check and return a value\n> of -1, though looking around it appears the callers of that other site\n> do not always correctly check and propagate a return value of -1\n> further upwards as they should, meaning we are losing track of the\n> fact that the merge failed to function.\n>\n> Given the lack of correct propagation of -1 return values in the other\n> codepath plus the error here, keeping some form of workaround sounds\n> fair (though it may be nice to print an error that something fishy\n> happened).\n>\n> And then, possibly post-v2.38.0 though we may be able to get it in\n> before, getting correct propagation of a -1 return value from the\n> source of the error would be good. Would you like to look into that,\n> or would you prefer I did?\n\nIt turned out not to be too bad.\n\nEssentially, to propagate `write_object_file()` failures I only had to\nchange a couple of signatures (with the corresponding `return`s):\n`write_tree()`, `write_completed_directory()`, and `process_entries()`.\nAnd then, of course, I needed to change\n`merge_ort_nonrecursive_internal()` to respect the propagated error by\nsetting `result->clean = -1`.\n\nPretty straight-forward, really, much less involved than I had expected.\n\nThank you!\nDscho\n"},{"id":"463472","messageId":"p7896r78-o578-80nr-s3n5-sr01o9q8prs9@tzk.qr","threadId":"58480","inReplyTo":"xmqqfsgk1i2t.fsf@gitster.g","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-22T19:52:23Z","receivedAt":"2022-09-22T19:53:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 21 Sep 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +\tconst struct object_id *tree_oid;\n> >\n> > \tparent1 = get_merge_parent(branch1);\n> > \tif (!parent1)\n> > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> > \tif (o->show_messages == -1)\n> > \t\to->show_messages = !result.clean;\n> >\n> > -\tprintf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> > +\ttree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> > +\tprintf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n>\n> As we are not using tree_oid anywhere else, it might be cleaner to\n> switch on result.tree more explicitly, especially given that the\n> return statement also uses result.tree to decide what exit status to\n> use, e.g.\n>\n> \tif (result.tree)\n> \t\tfputs(oid_to_hex(&result.tree->object.oid), stdout);\n> \telse\n> \t\tfputs(oid_to_hex(null_oid()), stdout);\n> \tfputc(line_termination, stdout);\n>\n> but that is merely \"might be cleaner\".\n\nI would call it \"more readable\", even, not just \"cleaner\" :-)\n\n> > -\treturn !result.clean; /* result.clean < 0 handled above */\n> > +\treturn !result.tree || !result.clean; /* result.clean < 0 handled above */\n>\n> The original returned 1 when we failed to cleanly merge and 0 when\n> we cleanly merged. Now, if we failed to come up with a merged tree,\n> result.tree would be NULL and we return 1 from the function, because\n> we failed to cleanly merge.\n>\n> OK. These negations make my head spin X-<.\n\nAnd here I thought that you couldn't possibly not like if I didn't refrain\nfrom not avoiding double negations...\n\nCiao,\nDscho\n"},{"id":"463491","messageId":"CABPp-BGizu+dq+8Hbqo5EjKFAWU_Ni9RwQ6p+4oUMLp0oamu7w@mail.gmail.com","threadId":"58480","inReplyTo":"pull.1362.v3.git.1663875999939.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] merge-ort: fix segmentation fault in read-only repositories","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-23T01:38:00Z","receivedAt":"2022-09-23T01:38:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 22, 2022 at 12:46 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> If the blob/tree objects cannot be written, we really need the merge\n> operations to fail, and not to continue (and then try to access the tree\n> object which is however still set to `NULL`).\n>\n> Let's stop ignoring the return value of `write_object_file()` and\n> `write_tree()` and set `clean = -1` in the error case.\n\n:-)\n\n> merge-ort.c | 64 +++++++++++++++++++-------------\n> t/t4301-merge-tree-write-tree.sh | 9 +++++\n> 2 files changed, 48 insertions(+), 25 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 99dcee2db8a..f654296220e 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)\n> b->string, strlen(b->string), bmi->result.mode);\n> }\n>\n> -static void write_tree(struct object_id *result_oid,\n> - struct string_list *versions,\n> - unsigned int offset,\n> - size_t hash_size)\n> +static int write_tree(struct object_id *result_oid,\n> + struct string_list *versions,\n> + unsigned int offset,\n> + size_t hash_size)\n> {\n> size_t maxlen = 0, extra;\n> unsigned int nr;\n> struct strbuf buf = STRBUF_INIT;\n> - int i;\n> + int i, ret;\n>\n> assert(offset <= versions->nr);\n> nr = versions->nr - offset;\n> @@ -3605,8 +3605,9 @@ static void write_tree(struct object_id *result_oid,\n> }\n>\n> /* Write this object file out, and record in result_oid */\n> - write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n> + ret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n\nThis sent me into a bit of a hunt to try to figure out what the return\nvalue of write_object_file() is. I know it's non-zero on failure, but\ndon't know if it's always positive or always negative or possibly a\nmix and how that maps to the idea of \"clean_merge\". I gave up after a\nfew indirections, to be honest.\n\nAnyway, regardless of my inability to find the answer to the above\nquestion, would this be a bit easier to read if we initialized ret to\n0 and then did something like\n\n if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n ret = -1;\n\n?\n\n> strbuf_release(&buf);\n> + return ret;\n> }\n>\n> static void record_entry_for_tree(struct directory_versions *dir_metadata,\n> @@ -3625,13 +3626,13 @@ static void record_entry_for_tree(struct directory_versions *dir_metadata,\n> basename)->util = &mi->result;\n> }\n>\n> -static void write_completed_directory(struct merge_options *opt,\n> - const char *new_directory_name,\n> - struct directory_versions *info)\n> +static int write_completed_directory(struct merge_options *opt,\n> + const char *new_directory_name,\n> + struct directory_versions *info)\n> {\n> const char *prev_dir;\n> struct merged_info *dir_info = NULL;\n> - unsigned int offset;\n> + unsigned int offset, ret = 0;\n>\n> /*\n> * Some explanation of info->versions and info->offsets...\n> @@ -3717,7 +3718,7 @@ static void write_completed_directory(struct merge_options *opt,\n> * strcmp here.)\n> */\n> if (new_directory_name == info->last_directory)\n> - return;\n> + return 0;\n>\n> /*\n> * If we are just starting (last_directory is NULL), or last_directory\n> @@ -3739,7 +3740,7 @@ static void write_completed_directory(struct merge_options *opt,\n> */\n> string_list_append(&info->offsets,\n> info->last_directory)->util = (void*)offset;\n> - return;\n> + return 0;\n> }\n>\n> /*\n> @@ -3769,8 +3770,9 @@ static void write_completed_directory(struct merge_options *opt,\n> */\n> dir_info->is_null = 0;\n> dir_info->result.mode = S_IFDIR;\n> - write_tree(&dir_info->result.oid, &info->versions, offset,\n> - opt->repo->hash_algo->rawsz);\n> + if (write_tree(&dir_info->result.oid, &info->versions, offset,\n> + opt->repo->hash_algo->rawsz) < 0)\n> + ret = -1;\n\nThis makes me wonder about write_tree() again. What if its call to\nwrite_object_file() returns a value greater than 0? Is that possible?\n If it is, are those error cases or not? About half the callers in\nthe code base that check the return value of write_object_file() check\nfor a non-zero value, the other half check for a value less than 0.\nAnd I can't find any documentation. And it seemed like too much time\nfor me to figure out what its range of return values was.\n\n> }\n>\n> /*\n> @@ -3798,6 +3800,8 @@ static void write_completed_directory(struct merge_options *opt,\n> /* And, of course, we need to update last_directory to match. */\n> info->last_directory = new_directory_name;\n> info->last_directory_len = strlen(info->last_directory);\n> +\n> + return ret;\n> }\n>\n> /* Per entry merge function */\n> @@ -4214,8 +4218,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,\n> oid_array_clear(&to_fetch);\n> }\n>\n> -static void process_entries(struct merge_options *opt,\n> - struct object_id *result_oid)\n> +static int process_entries(struct merge_options *opt,\n> + struct object_id *result_oid)\n> {\n> struct hashmap_iter iter;\n> struct strmap_entry *e;\n> @@ -4224,11 +4228,12 @@ static void process_entries(struct merge_options *opt,\n> struct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n> STRING_LIST_INIT_NODUP,\n> NULL, 0 };\n> + int ret;\n>\n> trace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n> if (strmap_empty(&opt->priv->paths)) {\n> oidcpy(result_oid, opt->repo->hash_algo->empty_tree);\n> - return;\n> + return 0;\n> }\n>\n> /* Hack to pre-allocate plist to the desired size */\n> @@ -4270,8 +4275,11 @@ static void process_entries(struct merge_options *opt,\n> */\n> struct merged_info *mi = entry->util;\n>\n> - write_completed_directory(opt, mi->directory_name,\n> - &dir_metadata);\n> + if (write_completed_directory(opt, mi->directory_name,\n> + &dir_metadata) < 0) {\n> + ret = -1;\n> + goto cleanup;\n> + }\n> if (mi->clean)\n> record_entry_for_tree(&dir_metadata, path, mi);\n> else {\n> @@ -4291,12 +4299,15 @@ static void process_entries(struct merge_options *opt,\n> fflush(stdout);\n> BUG(\"dir_metadata accounting completely off; shouldn't happen\");\n> }\n> - write_tree(result_oid, &dir_metadata.versions, 0,\n> - opt->repo->hash_algo->rawsz);\n> + ret = write_tree(result_oid, &dir_metadata.versions, 0,\n> + opt->repo->hash_algo->rawsz);\n> +cleanup:\n> string_list_clear(&plist, 0);\n> string_list_clear(&dir_metadata.versions, 0);\n> string_list_clear(&dir_metadata.offsets, 0);\n> trace2_region_leave(\"merge\", \"process_entries cleanup\", opt->repo);\n> +\n> + return ret;\n> }\n>\n> /*** Function Grouping: functions related to merge_switch_to_result() ***/\n> @@ -4928,15 +4939,18 @@ redo:\n> }\n>\n> trace2_region_enter(\"merge\", \"process_entries\", opt->repo);\n> - process_entries(opt, &working_tree_oid);\n> + if (process_entries(opt, &working_tree_oid) < 0)\n> + result->clean = -1;\n> trace2_region_leave(\"merge\", \"process_entries\", opt->repo);\n>\n> /* Set return values */\n> result->path_messages = &opt->priv->conflicts;\n>\n> - result->tree = parse_tree_indirect(&working_tree_oid);\n> - /* existence of conflicted entries implies unclean */\n> - result->clean &= strmap_empty(&opt->priv->conflicted);\n> + if (result->clean >= 0) {\n> + result->tree = parse_tree_indirect(&working_tree_oid);\n> + /* existence of conflicted entries implies unclean */\n> + result->clean &= strmap_empty(&opt->priv->conflicted);\n> + }\n> if (!opt->priv->call_depth) {\n> result->priv = opt->priv;\n> result->_properly_initialized = RESULT_INITIALIZED;\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 28ca5c38bb5..013b77144bd 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n> + git init --bare read-only &&\n> + git push read-only side1 side2 side3 &&\n> + test_when_finished \"chmod -R u+w read-only\" &&\n> + chmod -R a-w read-only &&\n> + test_must_fail git -C read-only merge-tree side1 side3 &&\n> + test_must_fail git -C read-only merge-tree side1 side2\n> +'\n> +\n> test_done\n>\n> base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n> --\n> gitgitgadget\n\nNice to see that it didn't take too much work to propagate the -1\nvalue from write_tree() up the hierarchy. Nice work!\n\nI think we've still got some separate problems related to propagating\nthe return value of the other write_object_file() call, from\nhandle_content_merge(). The direct call in that other case is okay,\nbut the higher levels at process_renames() and process_entry() seem to\nfumble it. Your fix for failed tree writes is still valid without\nfixing the failed blob writes, and I'm happy to tackle the other half\nif you'd prefer to hand it off. But, if you'd like to tackle that\nhalf too, you might see fewer error messages when it fails to write to\nthe read-only repository, since it'll fail early on the first blob\nwrite instead of only failing when it gets around to trying to write a\nnew tree.\n"},{"id":"463492","messageId":"CABPp-BEW9RdH8RSFk2BtPwYSyP0mKTz-7oCQQP7jsR6ChmF3aQ@mail.gmail.com","threadId":"58480","inReplyTo":"n920r07n-8443-2r5o-09pr-q20r99q6o12o@tzk.qr","subject":"Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-23T01:47:10Z","receivedAt":"2022-09-23T01:47:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 22, 2022 at 12:50 PM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> now that I have studied more of the code, hunted down Coverity reports\n> about `merge-ort.c` and `builtin/merge-tree.c` and essentially have grown\n> a lot more confidence about my patch, I'll take the time to respond in a\n> bit more detail.\n>\n> On Wed, 21 Sep 2022, Elijah Newren wrote:\n>\n> > On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > >\n> > > From: Johannes Schindelin \n> > >\n[...]\n> > > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,\n> > > if (o->show_messages == -1)\n> > > o->show_messages = !result.clean;\n> > >\n> > > - printf(\"%s%c\", oid_to_hex(&result.tree->object.oid), line_termination);\n> > > + tree_oid = result.tree ? &result.tree->object.oid : null_oid();\n> > > + printf(\"%s%c\", oid_to_hex(tree_oid), line_termination);\n> >\n> > Perhaps also print a warning to stderr when result.tree is NULL?\n>\n> I ended up not even touching `builtin/merge-tree.c` but instead ensuring\n> that `result.clean` is negative if we fail to write an object (which could\n> happen even in read/write repositories, think \"disk full\").\n>\n> As a consequence, we do not even reach this `printf()` anymore, as you\n> pointed out, a negative `result.clean` is handled much earlier.\n\nWorks for me. :-)\n\n> It is handled via a `die()` in `real_merge()`, and that will need to\n> change, I think, if we want to continue on the batched merges.\n\nIndeed, good point.\n\n> > > if (!result.clean) {\n> > > struct string_list conflicted_files = STRING_LIST_INIT_NODUP;\n> > > const char *last = NULL;\n> > > @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,\n> > > &result);\n> > > }\n> > > merge_finalize(&opt, &result);\n> > > - return !result.clean; /* result.clean < 0 handled above */\n> > > + return !result.tree || !result.clean; /* result.clean < 0 handled above */\n> >\n> > Thinking out loud, should this logic be at the merge-ort.c level,\n> > perhaps something like this:\n> >\n> > @@ -4940,6 +4941,9 @@ static void\n> > merge_ort_nonrecursive_internal(struct merge_options *opt,\n> > result->tree = parse_tree_indirect(&working_tree_oid);\n> > /* existence of conflicted entries implies unclean */\n> > result->clean &= strmap_empty(&opt->priv->conflicted);\n> > + if (!result->tree)\n> > + /* This shouldn't happen, because if we did fail to\n> > write a tree we should have returned early before getting here. But\n> > just in case we have some bugs... */\n> > + result->clean = -1;\n> > if (!opt->priv->call_depth) {\n> > result->priv = opt->priv;\n> > result->_properly_initialized = RESULT_INITIALIZED;\n> >\n> > That might benefit callers other than merge-tree, though maybe it\n> > makes it harder to print a helpful error message (unless we're fine\n> > with the library always throwing one?)\n>\n> The error messages are already thrown about (this is how it looks like\n> with v3 of this patch):\n>\n> [...]\n> + git -C read-only merge-tree side1 side2\n> error: insufficient permission for adding an object to repository database ./objects\n> error: error: Unable to add numbers to database\n> error: insufficient permission for adding an object to repository database ./objects\n> error: error: Unable to add greeting to database\n\nAh, this confirms my suspicions. These lines look like two failures,\nboth for adding blob objects. This shows that the return value from\nthe write_object_file() call within handle_content_merge() is not\nbeing correctly propagated upwards, and the merge is (erroneously)\ncontinuing on despite that.\n\n> error: insufficient permission for adding an object to repository database ./objects\n\nAnd I'm guessing the lack of the \"Unable to add %s to database\"\nfollowing message means this was the attempt to write a new tree.\n\n> fatal: failure to merge\n\nAnd this shows that with your patch, you are propagating and catching\nthe failure to write the tree object and aborting, so we at least\ncatch failures to write trees now.\n\n[...]\n> > And then, possibly post-v2.38.0 though we may be able to get it in\n> > before, getting correct propagation of a -1 return value from the\n> > source of the error would be good. Would you like to look into that,\n> > or would you prefer I did?\n>\n> It turned out not to be too bad.\n>\n> Essentially, to propagate `write_object_file()` failures I only had to\n> change a couple of signatures (with the corresponding `return`s):\n> `write_tree()`, `write_completed_directory()`, and `process_entries()`.\n> And then, of course, I needed to change\n> `merge_ort_nonrecursive_internal()` to respect the propagated error by\n> setting `result->clean = -1`.\n>\n> Pretty straight-forward, really, much less involved than I had expected.\n\nNice!\n"},{"id":"463507","messageId":"916o55op-qpqo-5o41-931s-8q54p7301sr2@tzk.qr","threadId":"58480","inReplyTo":"CABPp-BGizu+dq+8Hbqo5EjKFAWU_Ni9RwQ6p+4oUMLp0oamu7w@mail.gmail.com","subject":"Re: [PATCH v3] merge-ort: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-23T09:55:27Z","receivedAt":"2022-09-23T09:55:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Thu, 22 Sep 2022, Elijah Newren wrote:\n\n> On Thu, Sep 22, 2022 at 12:46 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 99dcee2db8a..f654296220e 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)\n> > b->string, strlen(b->string), bmi->result.mode);\n> > }\n> >\n> > -static void write_tree(struct object_id *result_oid,\n> > - struct string_list *versions,\n> > - unsigned int offset,\n> > - size_t hash_size)\n> > +static int write_tree(struct object_id *result_oid,\n> > + struct string_list *versions,\n> > + unsigned int offset,\n> > + size_t hash_size)\n> > {\n> > size_t maxlen = 0, extra;\n> > unsigned int nr;\n> > struct strbuf buf = STRBUF_INIT;\n> > - int i;\n> > + int i, ret;\n> >\n> > assert(offset <= versions->nr);\n> > nr = versions->nr - offset;\n> > @@ -3605,8 +3605,9 @@ static void write_tree(struct object_id *result_oid,\n> > }\n> >\n> > /* Write this object file out, and record in result_oid */\n> > - write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n> > + ret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n>\n> This sent me into a bit of a hunt to try to figure out what the return\n> value of write_object_file() is. I know it's non-zero on failure, but\n> don't know if it's always positive or always negative or possibly a\n> mix and how that maps to the idea of \"clean_merge\". I gave up after a\n> few indirections, to be honest.\n>\n> Anyway, regardless of my inability to find the answer to the above\n> question, would this be a bit easier to read if we initialized ret to\n> 0 and then did something like\n>\n> if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n> ret = -1;\n>\n> ?\n\nYou're right, this is a better pattern to follow because it makes it\neasier to wrap your head around.\n\n> > [...]\n> > @@ -3769,8 +3770,9 @@ static void write_completed_directory(struct merge_options *opt,\n> > */\n> > dir_info->is_null = 0;\n> > dir_info->result.mode = S_IFDIR;\n> > - write_tree(&dir_info->result.oid, &info->versions, offset,\n> > - opt->repo->hash_algo->rawsz);\n> > + if (write_tree(&dir_info->result.oid, &info->versions, offset,\n> > + opt->repo->hash_algo->rawsz) < 0)\n> > + ret = -1;\n>\n> This makes me wonder about write_tree() again. What if its call to\n> write_object_file() returns a value greater than 0? Is that possible?\n\nNo, `write_object_file()` returns either 0 or -1, always. I followed all\nthe code paths.\n\nHere is a tangent (feel free to skip over this lengthy analysis):\n\nThese code paths include a _very, very_ misleading one. In\n`write_loose_object()` (which is called via the call chain\n`write_object_file()` -> `write_object_file_flags()` ->\n`write_object_file_flags()`), we call `finalize_object_file()`, see\nhttps://github.com/git/git/blob/v2.37.3/object-file.c#L2030). And in that\n`finalize_object_file()` function, we set `ret = errno;` in two places\n(https://github.com/git/git/blob/v2.37.3/object-file.c#L1833 and\nhttps://github.com/git/git/blob/v2.37.3/object-file.c#L1850).\n\nWhen I read that, I chased down what the conventions are for `errno`\nvalues. Now, while\nhttps://pubs.opengroup.org/onlinepubs/007908799/xsh/errno.h.html does not\nsay anything about `errno` values being non-negative,\nhttps://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html\nvery much says:\n\n\tValues for errno are now required to be distinct positive values\n\trather than non-zero values. This change is for alignment with the\n\tISO/IEC 9899:1999 standard.\n\nTerrible, right? If we set `ret = errno`, that means that we return\nsomething positive!\n\nI then even dug up the origins of the `ret = errno` statements. Turns out\nthat yours truly introduced the first of these `ret = errno` statements in\n9e48b389990 (Work around missing hard links on FAT formatted media,\n2005-10-26). :facepalm:\n\nSo why did I do such a thing? Well, back then, the `ret` value _already_\ncontained an `errno`, thanks to calling `link_temp_to_file()`, which _did_\nreturn an `errno`. In fact, there was no code path in\n`move_temp_to_file()` (as the function was called back then) that would\nreturn -1, it always returned either 0 or an `errno`.\n\nAnd after coming so far, after one hour of analyzing, I finally, finally\nrealized that there is no `return ret;` in that function. That function\nhas a `ret` variable that is never returned _from_ that function, but it\nused to be returned _to_ that function, by the `link_temp_to_file()`\nfunction that does not even exist any longer.\n\nIn my defense, this terribly misleading code was not even introduced by\nmyself. It was introduced in 230f13225df (Create object subdirectories on\ndemand, 2005-10-08).\n\ntl;dr all is good, `finalize_object_file()` returns 0 upon success, -1\nupon failure.\n\n> If it is, are those error cases or not? About half the callers in the\n> code base that check the return value of write_object_file() check for a\n> non-zero value, the other half check for a value less than 0.\n\nAnd then there is `apply.c` where the return value isn't even checked at\nall most of the time.\n\n> And I can't find any documentation. And it seemed like too much time\n> for me to figure out what its range of return values was.\n>\n> [...]\n>\n> Nice to see that it didn't take too much work to propagate the -1 value\n>from write_tree() up the hierarchy. Nice work!\n\nThank you!\n\n> I think we've still got some separate problems related to propagating\n> the return value of the other write_object_file() call, from\n> handle_content_merge(). The direct call in that other case is okay, but\n> the higher levels at process_renames() and process_entry() seem to\n> fumble it. Your fix for failed tree writes is still valid without\n> fixing the failed blob writes, and I'm happy to tackle the other half if\n> you'd prefer to hand it off. But, if you'd like to tackle that half\n> too, you might see fewer error messages when it fails to write to the\n> read-only repository, since it'll fail early on the first blob write\n> instead of only failing when it gets around to trying to write a new\n> tree.\n\nIt'll be part of v4 ;-)\n\nCiao,\nDscho\n"},{"id":"463677","messageId":"pull.1362.v4.git.1664229348.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v3.git.1663875999939.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T21:55:46Z","receivedAt":"2022-09-26T21:55:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Turns out that the segmentation fault reported by Taylor\n[https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened while\ntesting merge-ort in a read-only repository, and that the upstream version\nof git merge-tree is as affected as GitHub's internal version.\n\nChanges since v3:\n\n * I now consistently use the pattern int ret = 0; ... if (...) ret = -1.\n * I added a commit to properly propagate write failures through the\n handle_content_merge() call path, even if it is not really critical (it\n just fails sooner, but the merge would have failed just the same without\n this patch).\n\nChanges since v2:\n\n * Completely changed the approach by no longer touching\n builtin/merge-tree.c but instead fixing the underlying merge-ort layer:\n we no longer ignore the return value of write_tree() and\n write_object_file().\n\nChanges since v1:\n\n * Using the SANITY prereq now\n * If the merge was aborted due to a write error, exit with a non-zero code\n even if the (potentially partial) merge is clean\n * The test case now also verifies that the git merge-tree command fails in\n a read-only repository even if the merge would have succeeded\n\nJohannes Schindelin (2):\n merge-ort: fix segmentation fault in read-only repositories\n merge-ort: return early when failing to write a blob\n\n merge-ort.c | 94 ++++++++++++++++++++------------\n t/t4301-merge-tree-write-tree.sh | 9 +++\n 2 files changed, 69 insertions(+), 34 deletions(-)\n\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1362\n\nRange-diff vs v3:\n\n 1: 198ff455f90 ! 1: ab6df092eba merge-ort: fix segmentation fault in read-only repositories\n @@ merge-ort.c: static int tree_entry_order(const void *a_, const void *b_)\n \tunsigned int nr;\n \tstruct strbuf buf = STRBUF_INIT;\n -\tint i;\n -+\tint i, ret;\n ++\tint i, ret = 0;\n \n \tassert(offset <= versions->nr);\n \tnr = versions->nr - offset;\n @@ merge-ort.c: static void write_tree(struct object_id *result_oid,\n \n \t/* Write this object file out, and record in result_oid */\n -\twrite_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n -+\tret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n ++\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n ++\t\tret = -1;\n \tstrbuf_release(&buf);\n +\treturn ret;\n }\n @@ merge-ort.c: static void process_entries(struct merge_options *opt,\n \tstruct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t NULL, 0 };\n -+\tint ret;\n ++\tint ret = 0;\n \n \ttrace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n \tif (strmap_empty(&opt->priv->paths)) {\n @@ merge-ort.c: static void process_entries(struct merge_options *opt,\n \t}\n -\twrite_tree(result_oid, &dir_metadata.versions, 0,\n -\t\t opt->repo->hash_algo->rawsz);\n -+\tret = write_tree(result_oid, &dir_metadata.versions, 0,\n -+\t\t\t opt->repo->hash_algo->rawsz);\n ++\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n ++\t\t opt->repo->hash_algo->rawsz) < 0)\n ++\t\tret = -1;\n +cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n -: ----------- > 2: 087207ae0b0 merge-ort: return early when failing to write a blob\n\n-- \ngitgitgadget\n"},{"id":"463678","messageId":"087207ae0b0932fcec9aa25e97bbe9227eff81cb.1664229348.git.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v4.git.1664229348.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] merge-ort: return early when failing to write a blob","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T21:55:48Z","receivedAt":"2022-09-26T21:55:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the previous commit, we fixed a segmentation fault when a tree object\ncould not be written.\n\nHowever, before the tree object is written, `merge-ort` wants to write\nout a blob object (except in cases where the merge results in a blob\nthat already exists in the database). And this can fail, too, but we\nignore that write failure so far.\n\nSince we will always write out a new tree object in addition to the blob\n(and if the blob did not exist in the database yet, we can be certain\nthat the tree object did not exist yet), the merge will _still_ fail at\nthat point, but it does unnecessary work by continuing after the blob\ncould not be written.\n\nLet's pay close attention and error out early if the blob could not be\nwritten. This reduces the error output of t4301.25 (\"merge-ort fails\ngracefully in a read-only repository\") from:\n\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add numbers to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add greeting to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\tfatal: failure to merge\n\nto:\n\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add numbers to database\n\tfatal: failure to merge\n\nNote: This patch adjusts two variable declarations from `unsigned` to\n`int` because their purpose is to hold the return value of\n`handle_content_merge()`, which is of type `int`. The existing users of\nthose variables are only interested whether that variable is zero or\nnon-zero, therefore this type change does not affect the existing code.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 28 +++++++++++++++++++---------\n 1 file changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex f3bdce1041a..e5f41cce481 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\t\t\t\t pathnames,\n \t\t\t\t\t\t\t 1 + 2 * opt->priv->call_depth,\n \t\t\t\t\t\t\t &merged);\n+\t\t\tif (clean_merge < 0)\n+\t\t\t\treturn -1;\n \t\t\tif (!clean_merge &&\n \t\t\t merged.mode == side1->stages[1].mode &&\n \t\t\t oideq(&merged.oid, &side1->stages[1].oid))\n@@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\tstruct version_info merged;\n \n \t\t\tstruct conflict_info *base, *side1, *side2;\n-\t\t\tunsigned clean;\n+\t\t\tint clean;\n \n \t\t\tpathnames[0] = oldpath;\n \t\t\tpathnames[other_source_index] = oldpath;\n@@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\t\t\t pathnames,\n \t\t\t\t\t\t 1 + 2 * opt->priv->call_depth,\n \t\t\t\t\t\t &merged);\n+\t\t\tif (clean < 0)\n+\t\t\t\treturn -1;\n \n \t\t\tmemcpy(&newinfo->stages[target_index], &merged,\n \t\t\t sizeof(merged));\n@@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,\n }\n \n /* Per entry merge function */\n-static void process_entry(struct merge_options *opt,\n-\t\t\t const char *path,\n-\t\t\t struct conflict_info *ci,\n-\t\t\t struct directory_versions *dir_metadata)\n+static int process_entry(struct merge_options *opt,\n+\t\t\t const char *path,\n+\t\t\t struct conflict_info *ci,\n+\t\t\t struct directory_versions *dir_metadata)\n {\n \tint df_file_index = 0;\n \n@@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,\n \t\trecord_entry_for_tree(dir_metadata, path, &ci->merged);\n \t\tif (ci->filemask == 0)\n \t\t\t/* nothing else to handle */\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\tassert(ci->df_conflict);\n \t}\n \n@@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tif (ci->filemask == 1) {\n \t\t\tci->filemask = 0;\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\t}\n \n \t\t/*\n@@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,\n \t} else if (ci->filemask >= 6) {\n \t\t/* Need a two-way or three-way content merge */\n \t\tstruct version_info merged_file;\n-\t\tunsigned clean_merge;\n+\t\tint clean_merge;\n \t\tstruct version_info *o = &ci->stages[0];\n \t\tstruct version_info *a = &ci->stages[1];\n \t\tstruct version_info *b = &ci->stages[2];\n@@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\t\t\t ci->pathnames,\n \t\t\t\t\t\t opt->priv->call_depth * 2,\n \t\t\t\t\t\t &merged_file);\n+\t\tif (clean_merge < 0)\n+\t\t\treturn -1;\n \t\tci->merged.clean = clean_merge &&\n \t\t\t\t !ci->df_conflict && !ci->path_conflict;\n \t\tci->merged.result.mode = merged_file.mode;\n@@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,\n \n \t/* Record metadata for ci->merged in dir_metadata */\n \trecord_entry_for_tree(dir_metadata, path, &ci->merged);\n+\treturn 0;\n }\n \n static void prefetch_for_content_merges(struct merge_options *opt,\n@@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n \t\telse {\n \t\t\tstruct conflict_info *ci = (struct conflict_info *)mi;\n-\t\t\tprocess_entry(opt, path, ci, &dir_metadata);\n+\t\t\tif (process_entry(opt, path, ci, &dir_metadata) < 0) {\n+\t\t\t\tret = -1;\n+\t\t\t\tgoto cleanup;\n+\t\t\t};\n \t\t}\n \t}\n \ttrace2_region_leave(\"merge\", \"processing\", opt->repo);\n-- \ngitgitgadget\n"},{"id":"463679","messageId":"ab6df092eba04854b2fe333d6344fd008c4e9021.1664229348.git.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v4.git.1664229348.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] merge-ort: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T21:55:47Z","receivedAt":"2022-09-26T21:56:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIf the blob/tree objects cannot be written, we really need the merge\noperations to fail, and not to continue (and then try to access the tree\nobject which is however still set to `NULL`).\n\nLet's stop ignoring the return value of `write_object_file()` and\n`write_tree()` and set `clean = -1` in the error case.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 66 ++++++++++++++++++++------------\n t/t4301-merge-tree-write-tree.sh | 9 +++++\n 2 files changed, 50 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 99dcee2db8a..f3bdce1041a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)\n \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n }\n \n-static void write_tree(struct object_id *result_oid,\n-\t\t struct string_list *versions,\n-\t\t unsigned int offset,\n-\t\t size_t hash_size)\n+static int write_tree(struct object_id *result_oid,\n+\t\t struct string_list *versions,\n+\t\t unsigned int offset,\n+\t\t size_t hash_size)\n {\n \tsize_t maxlen = 0, extra;\n \tunsigned int nr;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint i;\n+\tint i, ret = 0;\n \n \tassert(offset <= versions->nr);\n \tnr = versions->nr - offset;\n@@ -3605,8 +3605,10 @@ static void write_tree(struct object_id *result_oid,\n \t}\n \n \t/* Write this object file out, and record in result_oid */\n-\twrite_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n+\t\tret = -1;\n \tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static void record_entry_for_tree(struct directory_versions *dir_metadata,\n@@ -3625,13 +3627,13 @@ static void record_entry_for_tree(struct directory_versions *dir_metadata,\n \t\t\t basename)->util = &mi->result;\n }\n \n-static void write_completed_directory(struct merge_options *opt,\n-\t\t\t\t const char *new_directory_name,\n-\t\t\t\t struct directory_versions *info)\n+static int write_completed_directory(struct merge_options *opt,\n+\t\t\t\t const char *new_directory_name,\n+\t\t\t\t struct directory_versions *info)\n {\n \tconst char *prev_dir;\n \tstruct merged_info *dir_info = NULL;\n-\tunsigned int offset;\n+\tunsigned int offset, ret = 0;\n \n \t/*\n \t * Some explanation of info->versions and info->offsets...\n@@ -3717,7 +3719,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t * strcmp here.)\n \t */\n \tif (new_directory_name == info->last_directory)\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If we are just starting (last_directory is NULL), or last_directory\n@@ -3739,7 +3741,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tstring_list_append(&info->offsets,\n \t\t\t\t info->last_directory)->util = (void*)offset;\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/*\n@@ -3769,8 +3771,9 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tdir_info->is_null = 0;\n \t\tdir_info->result.mode = S_IFDIR;\n-\t\twrite_tree(&dir_info->result.oid, &info->versions, offset,\n-\t\t\t opt->repo->hash_algo->rawsz);\n+\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n+\t\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\t\tret = -1;\n \t}\n \n \t/*\n@@ -3798,6 +3801,8 @@ static void write_completed_directory(struct merge_options *opt,\n \t/* And, of course, we need to update last_directory to match. */\n \tinfo->last_directory = new_directory_name;\n \tinfo->last_directory_len = strlen(info->last_directory);\n+\n+\treturn ret;\n }\n \n /* Per entry merge function */\n@@ -4214,8 +4219,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,\n \toid_array_clear(&to_fetch);\n }\n \n-static void process_entries(struct merge_options *opt,\n-\t\t\t struct object_id *result_oid)\n+static int process_entries(struct merge_options *opt,\n+\t\t\t struct object_id *result_oid)\n {\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n@@ -4224,11 +4229,12 @@ static void process_entries(struct merge_options *opt,\n \tstruct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t NULL, 0 };\n+\tint ret = 0;\n \n \ttrace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n \tif (strmap_empty(&opt->priv->paths)) {\n \t\toidcpy(result_oid, opt->repo->hash_algo->empty_tree);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/* Hack to pre-allocate plist to the desired size */\n@@ -4270,8 +4276,11 @@ static void process_entries(struct merge_options *opt,\n \t\t */\n \t\tstruct merged_info *mi = entry->util;\n \n-\t\twrite_completed_directory(opt, mi->directory_name,\n-\t\t\t\t\t &dir_metadata);\n+\t\tif (write_completed_directory(opt, mi->directory_name,\n+\t\t\t\t\t &dir_metadata) < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tif (mi->clean)\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n \t\telse {\n@@ -4291,12 +4300,16 @@ static void process_entries(struct merge_options *opt,\n \t\tfflush(stdout);\n \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n \t}\n-\twrite_tree(result_oid, &dir_metadata.versions, 0,\n-\t\t opt->repo->hash_algo->rawsz);\n+\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n+\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\tret = -1;\n+cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n \tstring_list_clear(&dir_metadata.offsets, 0);\n \ttrace2_region_leave(\"merge\", \"process_entries cleanup\", opt->repo);\n+\n+\treturn ret;\n }\n \n /*** Function Grouping: functions related to merge_switch_to_result() ***/\n@@ -4928,15 +4941,18 @@ redo:\n \t}\n \n \ttrace2_region_enter(\"merge\", \"process_entries\", opt->repo);\n-\tprocess_entries(opt, &working_tree_oid);\n+\tif (process_entries(opt, &working_tree_oid) < 0)\n+\t\tresult->clean = -1;\n \ttrace2_region_leave(\"merge\", \"process_entries\", opt->repo);\n \n \t/* Set return values */\n \tresult->path_messages = &opt->priv->conflicts;\n \n-\tresult->tree = parse_tree_indirect(&working_tree_oid);\n-\t/* existence of conflicted entries implies unclean */\n-\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\tif (result->clean >= 0) {\n+\t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\t/* existence of conflicted entries implies unclean */\n+\t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\t}\n \tif (!opt->priv->call_depth) {\n \t\tresult->priv = opt->priv;\n \t\tresult->_properly_initialized = RESULT_INITIALIZED;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 28ca5c38bb5..013b77144bd 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n+\tgit init --bare read-only &&\n+\tgit push read-only side1 side2 side3 &&\n+\ttest_when_finished \"chmod -R u+w read-only\" &&\n+\tchmod -R a-w read-only &&\n+\ttest_must_fail git -C read-only merge-tree side1 side3 &&\n+\ttest_must_fail git -C read-only merge-tree side1 side2\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"463680","messageId":"xmqq5yh9bxhd.fsf@gitster.g","threadId":"58480","inReplyTo":"087207ae0b0932fcec9aa25e97bbe9227eff81cb.1664229348.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] merge-ort: return early when failing to write a blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T22:07:58Z","receivedAt":"2022-09-26T22:08:03Z","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> In the previous commit, we fixed a segmentation fault when a tree object\n> could not be written.\n>\n> However, before the tree object is written, `merge-ort` wants to write\n> out a blob object (except in cases where the merge results in a blob\n> that already exists in the database). And this can fail, too, but we\n> ignore that write failure so far.\n\nNice find.\n\nAs long as we create at least one \"new\" blob the repository has not\nseen, we have a chance to exit before we attempt to write a tree\nobject (and notice a failure). Checking the return value of\nprocess_entry() for failures is a good thing to do.\n\nWill replace. Thanks.\n"},{"id":"463701","messageId":"CABPp-BHau2qwoxj121H-cpjTPPCpfxjMpmhya0mV79qyvkpQ+g@mail.gmail.com","threadId":"58480","inReplyTo":"087207ae0b0932fcec9aa25e97bbe9227eff81cb.1664229348.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] merge-ort: return early when failing to write a blob","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-27T08:05:32Z","receivedAt":"2022-09-27T08:12:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 26, 2022 at 2:55 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> In the previous commit, we fixed a segmentation fault when a tree object\n> could not be written.\n>\n> However, before the tree object is written, `merge-ort` wants to write\n> out a blob object (except in cases where the merge results in a blob\n> that already exists in the database). And this can fail, too, but we\n> ignore that write failure so far.\n>\n> Since we will always write out a new tree object in addition to the blob\n> (and if the blob did not exist in the database yet, we can be certain\n> that the tree object did not exist yet), the merge will _still_ fail at\n> that point, but it does unnecessary work by continuing after the blob\n> could not be written.\n\nI don't think this is quite true. I've had a number of users come to\nme with \"messed up git repositories\" where I eventually discover that\nusers just randomly add \"sudo\" to some of their commands because when\nthings don't work, sometimes \"sudo\" fixes it. That means they've\ncreated stuff in their .git directory which may be owned by root,\nperhaps even some of the .git/objects/XX directories. However, they\nmay have other .git/objects/XX directories owned by their normal user.\n\nIf just some .git/objects/XX directories are owned by root and not\nothers, then it may be when users run git commands as themselves that\nsome things can be written to the object database but not others. In\nparticular, it could be that writing blob objects fail, but writing a\ntree object which references those blobs succeeds.\n\n> Let's pay close attention and error out early if the blob could not be\n> written. This reduces the error output of t4301.25 (\"merge-ort fails\n> gracefully in a read-only repository\") from:\n>\n> error: insufficient permission for adding an object to repository database ./objects\n> error: error: Unable to add numbers to database\n> error: insufficient permission for adding an object to repository database ./objects\n> error: error: Unable to add greeting to database\n> error: insufficient permission for adding an object to repository database ./objects\n> fatal: failure to merge\n>\n> to:\n>\n> error: insufficient permission for adding an object to repository database ./objects\n> error: error: Unable to add numbers to database\n> fatal: failure to merge\n>\n> Note: This patch adjusts two variable declarations from `unsigned` to\n> `int` because their purpose is to hold the return value of\n> `handle_content_merge()`, which is of type `int`. The existing users of\n> those variables are only interested whether that variable is zero or\n> non-zero, therefore this type change does not affect the existing code.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort.c | 28 +++++++++++++++++++---------\n> 1 file changed, 19 insertions(+), 9 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index f3bdce1041a..e5f41cce481 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,\n> pathnames,\n> 1 + 2 * opt->priv->call_depth,\n> &merged);\n> + if (clean_merge < 0)\n> + return -1;\n> if (!clean_merge &&\n> merged.mode == side1->stages[1].mode &&\n> oideq(&merged.oid, &side1->stages[1].oid))\n> @@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,\n> struct version_info merged;\n>\n> struct conflict_info *base, *side1, *side2;\n> - unsigned clean;\n> + int clean;\n>\n> pathnames[0] = oldpath;\n> pathnames[other_source_index] = oldpath;\n> @@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,\n> pathnames,\n> 1 + 2 * opt->priv->call_depth,\n> &merged);\n> + if (clean < 0)\n> + return -1;\n>\n> memcpy(&newinfo->stages[target_index], &merged,\n> sizeof(merged));\n> @@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,\n> }\n>\n> /* Per entry merge function */\n> -static void process_entry(struct merge_options *opt,\n> - const char *path,\n> - struct conflict_info *ci,\n> - struct directory_versions *dir_metadata)\n> +static int process_entry(struct merge_options *opt,\n> + const char *path,\n> + struct conflict_info *ci,\n> + struct directory_versions *dir_metadata)\n> {\n> int df_file_index = 0;\n>\n> @@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,\n> record_entry_for_tree(dir_metadata, path, &ci->merged);\n> if (ci->filemask == 0)\n> /* nothing else to handle */\n> - return;\n> + return 0;\n> assert(ci->df_conflict);\n> }\n>\n> @@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,\n> */\n> if (ci->filemask == 1) {\n> ci->filemask = 0;\n> - return;\n> + return 0;\n> }\n>\n> /*\n> @@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,\n> } else if (ci->filemask >= 6) {\n> /* Need a two-way or three-way content merge */\n> struct version_info merged_file;\n> - unsigned clean_merge;\n> + int clean_merge;\n> struct version_info *o = &ci->stages[0];\n> struct version_info *a = &ci->stages[1];\n> struct version_info *b = &ci->stages[2];\n> @@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,\n> ci->pathnames,\n> opt->priv->call_depth * 2,\n> &merged_file);\n> + if (clean_merge < 0)\n> + return -1;\n> ci->merged.clean = clean_merge &&\n> !ci->df_conflict && !ci->path_conflict;\n> ci->merged.result.mode = merged_file.mode;\n> @@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,\n>\n> /* Record metadata for ci->merged in dir_metadata */\n> record_entry_for_tree(dir_metadata, path, &ci->merged);\n> + return 0;\n> }\n>\n> static void prefetch_for_content_merges(struct merge_options *opt,\n> @@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,\n> record_entry_for_tree(&dir_metadata, path, mi);\n> else {\n> struct conflict_info *ci = (struct conflict_info *)mi;\n> - process_entry(opt, path, ci, &dir_metadata);\n> + if (process_entry(opt, path, ci, &dir_metadata) < 0) {\n> + ret = -1;\n> + goto cleanup;\n> + };\n> }\n> }\n> trace2_region_leave(\"merge\", \"processing\", opt->repo);\n> --\n> gitgitgadget\n\nPatch looks good to me, though.\n"},{"id":"463702","messageId":"CABPp-BGJueKtcdvzGWH_ZK6yeA6r3457ue5Uub9_gdU5M-WmvQ@mail.gmail.com","threadId":"58480","inReplyTo":"pull.1362.v4.git.1664229348.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-27T08:11:22Z","receivedAt":"2022-09-27T08:15:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 26, 2022 at 2:55 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> Turns out that the segmentation fault reported by Taylor\n> [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened while\n> testing merge-ort in a read-only repository, and that the upstream version\n> of git merge-tree is as affected as GitHub's internal version.\n>\n> Changes since v3:\n>\n> * I now consistently use the pattern int ret = 0; ... if (...) ret = -1.\n> * I added a commit to properly propagate write failures through the\n> handle_content_merge() call path, even if it is not really critical (it\n> just fails sooner, but the merge would have failed just the same without\n> this patch).\n\nI'm a little unsure about the commit message of patch 2, as I\ncommented on, but the code all looks good to me. I don't think the\ncomment in the commit message matters too much; whether your commit\nmessage addresses the weird case I mentioned, I feel the code handles\nit just fine. So I'll leave it up to you to decide whether to tweak\nthe commit message or not. Anyway, after you decide on that, feel\nfree to apply my\n\nReviewed-by: Elijah Newren \n\nThanks for investigating, for tracking down all the return codes and\nhow to propagate them, and getting it all fixed up!\n"},{"id":"463740","messageId":"xmqqill87om9.fsf@gitster.g","threadId":"58480","inReplyTo":"CABPp-BHau2qwoxj121H-cpjTPPCpfxjMpmhya0mV79qyvkpQ+g@mail.gmail.com","subject":"Re: [PATCH v4 2/2] merge-ort: return early when failing to write a blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T16:45:18Z","receivedAt":"2022-09-27T16:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Since we will always write out a new tree object in addition to the blob\n>> (and if the blob did not exist in the database yet, we can be certain\n>> that the tree object did not exist yet), the merge will _still_ fail at\n>> that point, but it does unnecessary work by continuing after the blob\n>> could not be written.\n>\n> I don't think this is quite true. I've had a number of users come to\n> me with \"messed up git repositories\" where I eventually discover that\n> users just randomly add \"sudo\" to some of their commands because when\n> things don't work, sometimes \"sudo\" fixes it. That means they've\n> created stuff in their .git directory which may be owned by root,\n> perhaps even some of the .git/objects/XX directories. However, they\n> may have other .git/objects/XX directories owned by their normal user.\n>\n> If just some .git/objects/XX directories are owned by root and not\n> others, then it may be when users run git commands as themselves that\n> some things can be written to the object database but not others. In\n> particular, it could be that writing blob objects fail, but writing a\n> tree object which references those blobs succeeds.\n\nIt is a good example scenario that argues even more strongly for\nthis change. We may know the correct object contents and name for\nthe top-level tree and even manage to write it out, after computing\nall the blobs and the trees contained within, some of which we may\nknow the object name but have failed to write out. Letting such a\nbreakage unnoticed would be, eh, bad.\n\n> Patch looks good to me, though.\n\nYup, thanks for a review.\n"},{"id":"463796","messageId":"63be9f9f7174fb71b3b84c293e299cfaa6a94310.1664350162.git.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v5.git.1664350162.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] merge-ort: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-28T07:29:21Z","receivedAt":"2022-09-28T07:32:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIf the blob/tree objects cannot be written, we really need the merge\noperations to fail, and not to continue (and then try to access the tree\nobject which is however still set to `NULL`).\n\nLet's stop ignoring the return value of `write_object_file()` and\n`write_tree()` and set `clean = -1` in the error case.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 66 ++++++++++++++++++++------------\n t/t4301-merge-tree-write-tree.sh | 9 +++++\n 2 files changed, 50 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 99dcee2db8a..f3bdce1041a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)\n \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n }\n \n-static void write_tree(struct object_id *result_oid,\n-\t\t struct string_list *versions,\n-\t\t unsigned int offset,\n-\t\t size_t hash_size)\n+static int write_tree(struct object_id *result_oid,\n+\t\t struct string_list *versions,\n+\t\t unsigned int offset,\n+\t\t size_t hash_size)\n {\n \tsize_t maxlen = 0, extra;\n \tunsigned int nr;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint i;\n+\tint i, ret = 0;\n \n \tassert(offset <= versions->nr);\n \tnr = versions->nr - offset;\n@@ -3605,8 +3605,10 @@ static void write_tree(struct object_id *result_oid,\n \t}\n \n \t/* Write this object file out, and record in result_oid */\n-\twrite_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n+\t\tret = -1;\n \tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static void record_entry_for_tree(struct directory_versions *dir_metadata,\n@@ -3625,13 +3627,13 @@ static void record_entry_for_tree(struct directory_versions *dir_metadata,\n \t\t\t basename)->util = &mi->result;\n }\n \n-static void write_completed_directory(struct merge_options *opt,\n-\t\t\t\t const char *new_directory_name,\n-\t\t\t\t struct directory_versions *info)\n+static int write_completed_directory(struct merge_options *opt,\n+\t\t\t\t const char *new_directory_name,\n+\t\t\t\t struct directory_versions *info)\n {\n \tconst char *prev_dir;\n \tstruct merged_info *dir_info = NULL;\n-\tunsigned int offset;\n+\tunsigned int offset, ret = 0;\n \n \t/*\n \t * Some explanation of info->versions and info->offsets...\n@@ -3717,7 +3719,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t * strcmp here.)\n \t */\n \tif (new_directory_name == info->last_directory)\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If we are just starting (last_directory is NULL), or last_directory\n@@ -3739,7 +3741,7 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tstring_list_append(&info->offsets,\n \t\t\t\t info->last_directory)->util = (void*)offset;\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/*\n@@ -3769,8 +3771,9 @@ static void write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tdir_info->is_null = 0;\n \t\tdir_info->result.mode = S_IFDIR;\n-\t\twrite_tree(&dir_info->result.oid, &info->versions, offset,\n-\t\t\t opt->repo->hash_algo->rawsz);\n+\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n+\t\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\t\tret = -1;\n \t}\n \n \t/*\n@@ -3798,6 +3801,8 @@ static void write_completed_directory(struct merge_options *opt,\n \t/* And, of course, we need to update last_directory to match. */\n \tinfo->last_directory = new_directory_name;\n \tinfo->last_directory_len = strlen(info->last_directory);\n+\n+\treturn ret;\n }\n \n /* Per entry merge function */\n@@ -4214,8 +4219,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,\n \toid_array_clear(&to_fetch);\n }\n \n-static void process_entries(struct merge_options *opt,\n-\t\t\t struct object_id *result_oid)\n+static int process_entries(struct merge_options *opt,\n+\t\t\t struct object_id *result_oid)\n {\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n@@ -4224,11 +4229,12 @@ static void process_entries(struct merge_options *opt,\n \tstruct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t STRING_LIST_INIT_NODUP,\n \t\t\t\t\t\t NULL, 0 };\n+\tint ret = 0;\n \n \ttrace2_region_enter(\"merge\", \"process_entries setup\", opt->repo);\n \tif (strmap_empty(&opt->priv->paths)) {\n \t\toidcpy(result_oid, opt->repo->hash_algo->empty_tree);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \t/* Hack to pre-allocate plist to the desired size */\n@@ -4270,8 +4276,11 @@ static void process_entries(struct merge_options *opt,\n \t\t */\n \t\tstruct merged_info *mi = entry->util;\n \n-\t\twrite_completed_directory(opt, mi->directory_name,\n-\t\t\t\t\t &dir_metadata);\n+\t\tif (write_completed_directory(opt, mi->directory_name,\n+\t\t\t\t\t &dir_metadata) < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tif (mi->clean)\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n \t\telse {\n@@ -4291,12 +4300,16 @@ static void process_entries(struct merge_options *opt,\n \t\tfflush(stdout);\n \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n \t}\n-\twrite_tree(result_oid, &dir_metadata.versions, 0,\n-\t\t opt->repo->hash_algo->rawsz);\n+\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n+\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\tret = -1;\n+cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n \tstring_list_clear(&dir_metadata.offsets, 0);\n \ttrace2_region_leave(\"merge\", \"process_entries cleanup\", opt->repo);\n+\n+\treturn ret;\n }\n \n /*** Function Grouping: functions related to merge_switch_to_result() ***/\n@@ -4928,15 +4941,18 @@ redo:\n \t}\n \n \ttrace2_region_enter(\"merge\", \"process_entries\", opt->repo);\n-\tprocess_entries(opt, &working_tree_oid);\n+\tif (process_entries(opt, &working_tree_oid) < 0)\n+\t\tresult->clean = -1;\n \ttrace2_region_leave(\"merge\", \"process_entries\", opt->repo);\n \n \t/* Set return values */\n \tresult->path_messages = &opt->priv->conflicts;\n \n-\tresult->tree = parse_tree_indirect(&working_tree_oid);\n-\t/* existence of conflicted entries implies unclean */\n-\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\tif (result->clean >= 0) {\n+\t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\t/* existence of conflicted entries implies unclean */\n+\t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n+\t}\n \tif (!opt->priv->call_depth) {\n \t\tresult->priv = opt->priv;\n \t\tresult->_properly_initialized = RESULT_INITIALIZED;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 28ca5c38bb5..013b77144bd 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '\n+\tgit init --bare read-only &&\n+\tgit push read-only side1 side2 side3 &&\n+\ttest_when_finished \"chmod -R u+w read-only\" &&\n+\tchmod -R a-w read-only &&\n+\ttest_must_fail git -C read-only merge-tree side1 side3 &&\n+\ttest_must_fail git -C read-only merge-tree side1 side2\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"463798","messageId":"pull.1362.v5.git.1664350162.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v4.git.1664229348.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] merge-tree: fix segmentation fault in read-only repositories","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-28T07:29:20Z","receivedAt":"2022-09-28T07:40:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Turns out that the segmentation fault reported by Taylor\n[https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened while\ntesting merge-ort in a read-only repository, and that the upstream version\nof git merge-tree is as affected as GitHub's internal version.\n\nChanges since v4:\n\n * Corrected the commit message that claimed that the second patch was not\n fixing a real bug but just avoided unnecessary work. As pointed out by\n Elijah, there are scenarios where this second patch fixes a very real\n bug.\n * Added Elijah's \"Reviewed-by\" footer.\n\nChanges since v3:\n\n * I now consistently use the pattern int ret = 0; ... if (...) ret = -1.\n * I added a commit to properly propagate write failures through the\n handle_content_merge() call path, even if it is not really critical (it\n just fails sooner, but the merge would have failed just the same without\n this patch).\n\nChanges since v2:\n\n * Completely changed the approach by no longer touching\n builtin/merge-tree.c but instead fixing the underlying merge-ort layer:\n we no longer ignore the return value of write_tree() and\n write_object_file().\n\nChanges since v1:\n\n * Using the SANITY prereq now\n * If the merge was aborted due to a write error, exit with a non-zero code\n even if the (potentially partial) merge is clean\n * The test case now also verifies that the git merge-tree command fails in\n a read-only repository even if the merge would have succeeded\n\nJohannes Schindelin (2):\n merge-ort: fix segmentation fault in read-only repositories\n merge-ort: return early when failing to write a blob\n\n merge-ort.c | 94 ++++++++++++++++++++------------\n t/t4301-merge-tree-write-tree.sh | 9 +++\n 2 files changed, 69 insertions(+), 34 deletions(-)\n\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1362\n\nRange-diff vs v4:\n\n 1: ab6df092eba ! 1: 63be9f9f717 merge-ort: fix segmentation fault in read-only repositories\n @@ Commit message\n Let's stop ignoring the return value of `write_object_file()` and\n `write_tree()` and set `clean = -1` in the error case.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Johannes Schindelin \n \n ## merge-ort.c ##\n 2: 087207ae0b0 ! 2: bfc71a2d8ad merge-ort: return early when failing to write a blob\n @@ Commit message\n that already exists in the database). And this can fail, too, but we\n ignore that write failure so far.\n \n - Since we will always write out a new tree object in addition to the blob\n - (and if the blob did not exist in the database yet, we can be certain\n - that the tree object did not exist yet), the merge will _still_ fail at\n - that point, but it does unnecessary work by continuing after the blob\n - could not be written.\n -\n Let's pay close attention and error out early if the blob could not be\n written. This reduces the error output of t4301.25 (\"merge-ort fails\n gracefully in a read-only repository\") from:\n @@ Commit message\n error: error: Unable to add numbers to database\n fatal: failure to merge\n \n + This is _not_ just a cosmetic change: Even though one might assume that\n + the operation would have failed anyway at the point when the new tree\n + object is written (and the corresponding tree object _will_ be new if it\n + contains a blob that is new), but that is not so: As pointed out by\n + Elijah Newren, when Git has previously been allowed to add loose objects\n + via `sudo` calls, it is very possible that the blob object cannot be\n + written (because the corresponding `.git/objects/??/` directory may be\n + owned by `root`) but the tree object can be written (because the\n + corresponding objects directory is owned by the current user). This\n + would result in a corrupt repository because it is missing the blob\n + object, and with this here patch we prevent that.\n +\n Note: This patch adjusts two variable declarations from `unsigned` to\n `int` because their purpose is to hold the return value of\n `handle_content_merge()`, which is of type `int`. The existing users of\n those variables are only interested whether that variable is zero or\n non-zero, therefore this type change does not affect the existing code.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Johannes Schindelin \n \n ## merge-ort.c ##\n\n-- \ngitgitgadget\n"},{"id":"463799","messageId":"bfc71a2d8adfbf9b899a47d469fe2343e4703ff7.1664350162.git.gitgitgadget@gmail.com","threadId":"58480","inReplyTo":"pull.1362.v5.git.1664350162.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] merge-ort: return early when failing to write a blob","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-28T07:29:22Z","receivedAt":"2022-09-28T07:47:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the previous commit, we fixed a segmentation fault when a tree object\ncould not be written.\n\nHowever, before the tree object is written, `merge-ort` wants to write\nout a blob object (except in cases where the merge results in a blob\nthat already exists in the database). And this can fail, too, but we\nignore that write failure so far.\n\nLet's pay close attention and error out early if the blob could not be\nwritten. This reduces the error output of t4301.25 (\"merge-ort fails\ngracefully in a read-only repository\") from:\n\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add numbers to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add greeting to database\n\terror: insufficient permission for adding an object to repository database ./objects\n\tfatal: failure to merge\n\nto:\n\n\terror: insufficient permission for adding an object to repository database ./objects\n\terror: error: Unable to add numbers to database\n\tfatal: failure to merge\n\nThis is _not_ just a cosmetic change: Even though one might assume that\nthe operation would have failed anyway at the point when the new tree\nobject is written (and the corresponding tree object _will_ be new if it\ncontains a blob that is new), but that is not so: As pointed out by\nElijah Newren, when Git has previously been allowed to add loose objects\nvia `sudo` calls, it is very possible that the blob object cannot be\nwritten (because the corresponding `.git/objects/??/` directory may be\nowned by `root`) but the tree object can be written (because the\ncorresponding objects directory is owned by the current user). This\nwould result in a corrupt repository because it is missing the blob\nobject, and with this here patch we prevent that.\n\nNote: This patch adjusts two variable declarations from `unsigned` to\n`int` because their purpose is to hold the return value of\n`handle_content_merge()`, which is of type `int`. The existing users of\nthose variables are only interested whether that variable is zero or\nnon-zero, therefore this type change does not affect the existing code.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 28 +++++++++++++++++++---------\n 1 file changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex f3bdce1041a..e5f41cce481 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\t\t\t\t pathnames,\n \t\t\t\t\t\t\t 1 + 2 * opt->priv->call_depth,\n \t\t\t\t\t\t\t &merged);\n+\t\t\tif (clean_merge < 0)\n+\t\t\t\treturn -1;\n \t\t\tif (!clean_merge &&\n \t\t\t merged.mode == side1->stages[1].mode &&\n \t\t\t oideq(&merged.oid, &side1->stages[1].oid))\n@@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\tstruct version_info merged;\n \n \t\t\tstruct conflict_info *base, *side1, *side2;\n-\t\t\tunsigned clean;\n+\t\t\tint clean;\n \n \t\t\tpathnames[0] = oldpath;\n \t\t\tpathnames[other_source_index] = oldpath;\n@@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\t\t\t pathnames,\n \t\t\t\t\t\t 1 + 2 * opt->priv->call_depth,\n \t\t\t\t\t\t &merged);\n+\t\t\tif (clean < 0)\n+\t\t\t\treturn -1;\n \n \t\t\tmemcpy(&newinfo->stages[target_index], &merged,\n \t\t\t sizeof(merged));\n@@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,\n }\n \n /* Per entry merge function */\n-static void process_entry(struct merge_options *opt,\n-\t\t\t const char *path,\n-\t\t\t struct conflict_info *ci,\n-\t\t\t struct directory_versions *dir_metadata)\n+static int process_entry(struct merge_options *opt,\n+\t\t\t const char *path,\n+\t\t\t struct conflict_info *ci,\n+\t\t\t struct directory_versions *dir_metadata)\n {\n \tint df_file_index = 0;\n \n@@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,\n \t\trecord_entry_for_tree(dir_metadata, path, &ci->merged);\n \t\tif (ci->filemask == 0)\n \t\t\t/* nothing else to handle */\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\tassert(ci->df_conflict);\n \t}\n \n@@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tif (ci->filemask == 1) {\n \t\t\tci->filemask = 0;\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\t}\n \n \t\t/*\n@@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,\n \t} else if (ci->filemask >= 6) {\n \t\t/* Need a two-way or three-way content merge */\n \t\tstruct version_info merged_file;\n-\t\tunsigned clean_merge;\n+\t\tint clean_merge;\n \t\tstruct version_info *o = &ci->stages[0];\n \t\tstruct version_info *a = &ci->stages[1];\n \t\tstruct version_info *b = &ci->stages[2];\n@@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\t\t\t ci->pathnames,\n \t\t\t\t\t\t opt->priv->call_depth * 2,\n \t\t\t\t\t\t &merged_file);\n+\t\tif (clean_merge < 0)\n+\t\t\treturn -1;\n \t\tci->merged.clean = clean_merge &&\n \t\t\t\t !ci->df_conflict && !ci->path_conflict;\n \t\tci->merged.result.mode = merged_file.mode;\n@@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,\n \n \t/* Record metadata for ci->merged in dir_metadata */\n \trecord_entry_for_tree(dir_metadata, path, &ci->merged);\n+\treturn 0;\n }\n \n static void prefetch_for_content_merges(struct merge_options *opt,\n@@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,\n \t\t\trecord_entry_for_tree(&dir_metadata, path, mi);\n \t\telse {\n \t\t\tstruct conflict_info *ci = (struct conflict_info *)mi;\n-\t\t\tprocess_entry(opt, path, ci, &dir_metadata);\n+\t\t\tif (process_entry(opt, path, ci, &dir_metadata) < 0) {\n+\t\t\t\tret = -1;\n+\t\t\t\tgoto cleanup;\n+\t\t\t};\n \t\t}\n \t}\n \ttrace2_region_leave(\"merge\", \"processing\", opt->repo);\n-- \ngitgitgadget\n"},{"id":"463851","messageId":"xmqqleq3zea3.fsf@gitster.g","threadId":"58480","inReplyTo":"bfc71a2d8adfbf9b899a47d469fe2343e4703ff7.1664350162.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] merge-ort: return early when failing to write a blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-28T15:53:24Z","receivedAt":"2022-09-28T15:53:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This is _not_ just a cosmetic change: Even though one might assume that\n> the operation would have failed anyway at the point when the new tree\n> object is written (and the corresponding tree object _will_ be new if it\n> contains a blob that is new), but that is not so: As pointed out by\n> Elijah Newren, when Git has previously been allowed to add loose objects\n> via `sudo` calls, it is very possible that the blob object cannot be\n> written (because the corresponding `.git/objects/??/` directory may be\n> owned by `root`) but the tree object can be written (because the\n> corresponding objects directory is owned by the current user). This\n> would result in a corrupt repository because it is missing the blob\n> object, and with this here patch we prevent that.\n>\n> Note: This patch adjusts two variable declarations from `unsigned` to\n> `int` because their purpose is to hold the return value of\n> `handle_content_merge()`, which is of type `int`. The existing users of\n> those variables are only interested whether that variable is zero or\n> non-zero, therefore this type change does not affect the existing code.\n>\n> Reviewed-by: Elijah Newren \n> Signed-off-by: Johannes Schindelin \n> ---\n\nThanks. The first paragraph in the quoted part above is new and not\nexactly \"reviewed\" by Elijah yet, so let's ask ;-) \n\nTo me the description of the issue looks reasonable to me. Any\ncomments, Elijah?\n\nThe code is the same as the one in the previous round and Elijah\ngave his Reviewed-by, and does look good to me, too.\n\n> merge-ort.c | 28 +++++++++++++++++++---------\n> 1 file changed, 19 insertions(+), 9 deletions(-)\n\nThanks, all. Queued.\n\n"},{"id":"463892","messageId":"CABPp-BFML-7osm-UUpXHf9exzUk0iktDWwm4tKu7ggv68SgJ_w@mail.gmail.com","threadId":"58480","inReplyTo":"xmqqleq3zea3.fsf@gitster.g","subject":"Re: [PATCH v5 2/2] merge-ort: return early when failing to write a blob","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-29T01:36:33Z","receivedAt":"2022-09-29T01:36:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 28, 2022 at 8:53 AM Junio C Hamano wrote:\n>\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > This is _not_ just a cosmetic change: Even though one might assume that\n> > the operation would have failed anyway at the point when the new tree\n> > object is written (and the corresponding tree object _will_ be new if it\n> > contains a blob that is new), but that is not so: As pointed out by\n> > Elijah Newren, when Git has previously been allowed to add loose objects\n> > via `sudo` calls, it is very possible that the blob object cannot be\n> > written (because the corresponding `.git/objects/??/` directory may be\n> > owned by `root`) but the tree object can be written (because the\n> > corresponding objects directory is owned by the current user). This\n> > would result in a corrupt repository because it is missing the blob\n> > object, and with this here patch we prevent that.\n> >\n> > Note: This patch adjusts two variable declarations from `unsigned` to\n> > `int` because their purpose is to hold the return value of\n> > `handle_content_merge()`, which is of type `int`. The existing users of\n> > those variables are only interested whether that variable is zero or\n> > non-zero, therefore this type change does not affect the existing code.\n> >\n> > Reviewed-by: Elijah Newren \n> > Signed-off-by: Johannes Schindelin \n> > ---\n>\n> Thanks. The first paragraph in the quoted part above is new and not\n> exactly \"reviewed\" by Elijah yet, so let's ask ;-)\n>\n> To me the description of the issue looks reasonable to me. Any\n> comments, Elijah?\n\nLooks good to me!\n"},{"id":"463893","messageId":"xmqqwn9nospg.fsf@gitster.g","threadId":"58480","inReplyTo":"CABPp-BFML-7osm-UUpXHf9exzUk0iktDWwm4tKu7ggv68SgJ_w@mail.gmail.com","subject":"Re: [PATCH v5 2/2] merge-ort: return early when failing to write a blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T01:49:31Z","receivedAt":"2022-09-29T01:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> To me the description of the issue looks reasonable to me. Any\n>> comments, Elijah?\n>\n> Looks good to me!\n\nExcellent. Thanks, both.\n"}]}