Re: [PATCH] merge-tree: fix segmentation fault in read-only repositories
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 21, 2022, 15:42 UTC
- Message-ID
- <Yysw/YIxx/JRF4Ph@nand.local>
- In-Reply-To
- <pull.1362.git.1663774248660.gitgitgadget@gmail.com>
On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 19 quoted lines
> 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.
Show 12 quoted lines
> 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