On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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.
> 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
