Turns out that the segmentation fault reported by Taylor [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened while testing merge-ort in a read-only repository, and that the upstream version of git merge-tree is as affected as GitHub's internal version. Changes since v3: * I now consistently use the pattern int ret = 0; ... if (...) ret = -1. * I added a commit to properly propagate write failures through the handle_content_merge() call path, even if it is not really critical (it just fails sooner, but the merge would have failed just the same without this patch). Changes since v2: * Completely changed the approach by no longer touching builtin/merge-tree.c but instead fixing the underlying merge-ort layer: we no longer ignore the return value of write_tree() and write_object_file(). Changes since v1: * Using the SANITY prereq now * If the merge was aborted due to a write error, exit with a non-zero code even if the (potentially partial) merge is clean * The test case now also verifies that the git merge-tree command fails in a read-only repository even if the merge would have succeeded Johannes Schindelin (2): merge-ort: fix segmentation fault in read-only repositories merge-ort: return early when failing to write a blob merge-ort.c | 94 ++++++++++++++++++++------------ t/t4301-merge-tree-write-tree.sh | 9 +++ 2 files changed, 69 insertions(+), 34 deletions(-) base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1362 Range-diff vs v3: 1: 198ff455f90 ! 1: ab6df092eba merge-ort: fix segmentation fault in read-only repositories @@ merge-ort.c: static int tree_entry_order(const void *a_, const void *b_) unsigned int nr; struct strbuf buf = STRBUF_INIT; - int i; -+ int i, ret; ++ int i, ret = 0; assert(offset <= versions->nr); nr = versions->nr - offset; @@ merge-ort.c: static void write_tree(struct object_id *result_oid, /* Write this object file out, and record in result_oid */ - write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid); -+ ret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid); ++ if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid)) ++ ret = -1; strbuf_release(&buf); + return ret; } @@ merge-ort.c: static void process_entries(struct merge_options *opt, struct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP, NULL, 0 }; -+ int ret; ++ int ret = 0; trace2_region_enter("merge", "process_entries setup", opt->repo); if (strmap_empty(&opt->priv->paths)) { @@ merge-ort.c: static void process_entries(struct merge_options *opt, } - write_tree(result_oid, &dir_metadata.versions, 0, - opt->repo->hash_algo->rawsz); -+ ret = write_tree(result_oid, &dir_metadata.versions, 0, -+ opt->repo->hash_algo->rawsz); ++ if (write_tree(result_oid, &dir_metadata.versions, 0, ++ opt->repo->hash_algo->rawsz) < 0) ++ ret = -1; +cleanup: string_list_clear(&plist, 0); string_list_clear(&dir_metadata.versions, 0); -: ----------- > 2: 087207ae0b0 merge-ort: return early when failing to write a blob -- gitgitgadget
