Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories
From: Taylor Blau <hidden>
Date: 2022-09-21 15:43:12
On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index ae5782917b9..25c7142a882 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c@@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o, struct commit_list *merge_bases = NULL; struct merge_options opt; struct merge_result result = { 0 }; + const struct object_id *tree_oid; parent1 = get_merge_parent(branch1); if (!parent1)@@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o, if (o->show_messages == -1) o->show_messages = !result.clean; - printf("%s%c", oid_to_hex(&result.tree->object.oid), line_termination); + tree_oid = result.tree ? &result.tree->object.oid : null_oid(); + printf("%s%c", oid_to_hex(tree_oid), line_termination);
My understanding is that we can get a clean result from merge_incore_recursive(), but still have failed to write the physical tree object out? In other words, the two sides *could* have been merged, but the actual result of doing that merge couldn't be written to disk (e.g., because the repository is read-only or some such)? If so, then this approach makes sense, and I agree with your idea to use the all-zeros OID instead of the empty tree one. It would be nice(r?) if we could just abort this command earlier, since `merge-tree` promises that we'll write the result out as an object. So I don't think a non-zero exit before we have to print the resulting tree object would be unexpected. But I don't have a strong feeling about it either way. So, if you want to proceed here and just emit the all-zeros OID, I think that is a fine approach.
quoted hunk ↗ jump to hunk
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index 28ca5c38bb5..e56b1ba6e50 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh@@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' ' test_cmp expect actual ' +test_expect_success 'merge-ort fails gracefully in a read-only repository' ' + git init --bare read-only && + git push read-only side1 side2 && + test_when_finished "chmod -R u+w read-only" &&
Do we care about keeping this read-only repository around after the test is done? It seems odd to have a directory called "read-only" be user-writable. I'd probably just as soon replace this with: test_when_finished "rm -fr read-only" && Otherwise, this patch looks good. Thanks for working on it! Thanks, Taylor