merge-tree: fix segmentation fault in read-only repositories

builtin/merge-tree.c +1 -0

index ae5782917b9..25c7142a882 100644
@@ -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)

Quoted 16 lines · Johannes Schindelin via GitGitGadget

builtin/merge-tree.c +1 -0

index ae5782917b9..25c7142a882 100644
@@ -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;
 

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 451

Quoted 3 lines

builtin/merge-tree.c +2 -1

-	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.

t/t4301-merge-tree-write-tree.sh test_expect_success 'can override merge of unrelated histories' ' · line 816

Quoted 12 lines

t/t4301-merge-tree-write-tree.sh +4 -0

index 28ca5c38bb5..e56b1ba6e50 100755
@@ -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" &&

"Johannes Schindelin via GitGitGadget" <[email protected]>

t/t4301-merge-tree-write-tree.sh test_expect_success 'can override merge of unrelated histories' ' · line 819

Quoted 7 lines · writes:

t/t4301-merge-tree-write-tree.sh +7 -0

+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" &&
+	chmod -R a-w read-only &&
+	test_must_fail git -C read-only merge-tree side1 side2
+'

Doesn't this test need a prerequisite to guard against those whose filesystem lacks SANITY?

"Johannes Schindelin via GitGitGadget" <[email protected]>

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 451

Quoted 11 lines · writes:

builtin/merge-tree.c +3 -1

+	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);

As we are not using tree_oid anywhere else, it might be cleaner to switch on result.tree more explicitly, especially given that the return statement also uses result.tree to decide what exit status to use, e.g.

	if (result.tree)
		fputs(oid_to_hex(&result.tree->object.oid), stdout);
	else
		fputs(oid_to_hex(null_oid()), stdout);
	fputc(line_termination, stdout);

but that is merely "might be cleaner".

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 478

Quoted 2 lines

builtin/merge-tree.c +1 -1

-	return !result.clean; /* result.clean < 0 handled above */
+	return !result.tree || !result.clean; /* result.clean < 0 handled above */

The original returned 1 when we failed to cleanly merge and 0 when we cleanly merged. Now, if we failed to come up with a merged tree, result.tree would be NULL and we return 1 from the function, because we failed to cleanly merge.

OK. These negations make my head spin X-<.

From: Johannes Schindelin <[email protected]>

Independent of the question whether we want git merge-tree to report the tree name even when it failed to write the tree objects in a read-only repository, there is no question that we should avoid a segmentation fault.

And when we report an invalid tree name (because the tree could not be written), we need the exit status to be non-zero.

Let's make it so.

Signed-off-by Johannes Schindelin <[email protected]>

---
merge-tree: fix segmentation fault in read-only repositories

Turns out that the segmentation fault reported by Taylor
[https://lore.kernel.org/git/[email protected]/] 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.

Note: I briefly considered using the OID of the_hash_algo->empty_tree instead of null_oid() when no tree object could be constructed. However, I have come to the conclusion that this could potentially cause its own set of problems because it would relate to a valid tree object even if we do not have any valid tree object to play with.

Also note: The question I hinted at in the commit message, namely whether or not to try harder to construct a tree object even if we cannot write it out, maybe merits a longer discussion, one that I think we should have after v2.38.0 is released, so as not to distract from focusing on v2.38.0.

t/t4301-merge-tree-write-tree.sh +9 -0

index 28ca5c38bb5..013b77144bd 100755
@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '
        test_cmp expect actual
 '
 
+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
+       git init --bare read-only &&
+       git push read-only side1 side2 side3 &&
+       test_when_finished "chmod -R u+w read-only" &&
+       chmod -R a-w read-only &&
+       test_must_fail git -C read-only merge-tree side1 side3 &&
+       test_must_fail git -C read-only merge-tree side1 side2
+'
+
 test_done

base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14 --
gitgitgadget

Review static int real_merge(struct merge_tree_options *o, · line 478

Quoted 6 lines

(quoted diff) +1 -1

@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
                                              &result);
        }
        merge_finalize(&opt, &result);
-       return !result.clean; /* result.clean < 0 handled above */
+       return !result.tree || !result.clean; /* result.clean < 0 handled above */

Thinking out loud, should this logic be at the merge-ort.c level,

... However, I do think
merge-ort should be returning a negative value for the "clean" status in such a case.

Review static int real_merge(struct merge_tree_options *o, · line 478

Quoted 7 lines · Johannes Schindelin via GitGitGadget

(quoted diff) +1 -1

@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
                                              &result);
        }
        merge_finalize(&opt, &result);
-       return !result.clean; /* result.clean < 0 handled above */
+       return !result.tree || !result.clean; /* result.clean < 0 handled above */

Thinking out loud, should this logic be at the merge-ort.c level,

Quoted 7 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Independent of the question whether we want git merge-tree to report the tree name even when it failed to write the tree objects in a read-only repository, there is no question that we should avoid a segmentation fault.

Ah, a read-only repo. Looks like we didn't bother to check the return status of write_object_file() in one of the two cases; oops. (And it looks like the other case does not correctly propagate the error upwards far enough...)

Quoted 20 lines

And when we report an invalid tree name (because the tree could not be written), we need the exit status to be non-zero.

Let's make it so.

Signed-off-by Johannes Schindelin <[email protected]>

---
merge-tree: fix segmentation fault in read-only repositories

Turns out that the segmentation fault reported by Taylor
[https://lore.kernel.org/git/[email protected]/] 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.

Note: I briefly considered using the OID of the_hash_algo->empty_tree instead of null_oid() when no tree object could be constructed. However, I have come to the conclusion that this could potentially cause its own set of problems because it would relate to a valid tree object even if we do not have any valid tree object to play with.

Quoted 5 lines

Also note: The question I hinted at in the commit message, namely whether or not to try harder to construct a tree object even if we cannot write it out, maybe merits a longer discussion, one that I think we should have after v2.38.0 is released, so as not to distract from focusing on v2.38.0.

That's fair, but you know I'm going to have a hard time refraining from commenting on it anyway... :-)

[...]

Review static int real_merge(struct merge_tree_options *o, · line 451

Quoted 7 lines

(quoted diff) +2 -1

@@ -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);

Perhaps also print a warning to stderr when result.tree is NULL?

Review static int real_merge(struct merge_tree_options *o, · line 478

Quoted 9 lines

(quoted diff) +1 -1

        if (!result.clean) {
                struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
                const char *last = NULL;

@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
                                              &result);
        }
        merge_finalize(&opt, &result);
-       return !result.clean; /* result.clean < 0 handled above */
+       return !result.tree || !result.clean; /* result.clean < 0 handled above */

Thinking out loud, should this logic be at the merge-ort.c level, perhaps something like this:

(quoted diff) +2 -0

@@ -4940,6 +4941,9 @@ static void
 merge_ort_nonrecursive_internal(struct merge_options *opt,
        result->tree = parse_tree_indirect(&working_tree_oid);
        /* existence of conflicted entries implies unclean */
        result->clean &= strmap_empty(&opt->priv->conflicted);
+       if (!result->tree)
+               /* This shouldn't happen, because if we did fail to

write a tree we should have returned early before getting here. But just in case we have some bugs... */

+               result->clean = -1;
        if (!opt->priv->call_depth) {
                result->priv = opt->priv;
                result->_properly_initialized = RESULT_INITIALIZED;

That might benefit callers other than merge-tree, though maybe it makes it harder to print a helpful error message (unless we're fine with the library always throwing one?)

Quoted 19 lines

t/t4301-merge-tree-write-tree.sh +9 -0

index 28ca5c38bb5..013b77144bd 100755
@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '
        test_cmp expect actual
 '
 
+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
+       git init --bare read-only &&
+       git push read-only side1 side2 side3 &&
+       test_when_finished "chmod -R u+w read-only" &&
+       chmod -R a-w read-only &&
+       test_must_fail git -C read-only merge-tree side1 side3 &&
+       test_must_fail git -C read-only merge-tree side1 side2
+'
+
 test_done

Quoted 3 lines

base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14 --
gitgitgadget

This is a reasonable workaround from the merge-tree side, if we expect merge-ort to not correctly notify us of issues (which may be fair given that a quick glance suggests we have more than one problematic codepath, which I'll discuss more below). However, I do think merge-ort should be returning a negative value for the "clean" status in such a case.

If merge-ort did that, then we wouldn't even get to your new code because merge-tree.c already checks for that:

    merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
    if (result.clean < 0)
        die(_("failure to merge"));

I have a hack above which sets a negative return value, but it's only a workaround since it comes from a late detect-after-the-fact location. Here's another ugly hack, but one which highlights exactly where the real problem occurs:

merge-ort.c +2 -1

index 99dcee2db8..c2144e9220 100644
@@ -3605,7 +3605,8 @@ 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);
+       if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
+               die(_("Unable to add new tree to database"));
        strbuf_release(&buf);
 }
 

die()ing, of course, is not a good choice, we should really return -1 instead. Unfortunately, the callers aren't currently prepared to propagate such a value upwards (as reflected in the return type of the function) so any fix in this area is a little more involved than just modifying these few lines. Also, there is one other call to write_object_file() in merge-ort.c which does check and return a value of -1, though looking around it appears the callers of that other site do not always correctly check and propagate a return value of -1 further upwards as they should, meaning we are losing track of the fact that the merge failed to function.

Given the lack of correct propagation of -1 return values in the other codepath plus the error here, keeping some form of workaround sounds fair (though it may be nice to print an error that something fishy happened).

And then, possibly post-v2.38.0 though we may be able to get it in before, getting correct propagation of a -1 return value from the source of the error would be good. Would you like to look into that, or would you prefer I did?

Hi Elijah,

now that I have studied more of the code, hunted down Coverity reports about merge-ort.c and builtin/merge-tree.c and essentially have grown a lot more confidence about my patch, I'll take the time to respond in a bit more detail.

Quoted 5 lines · Elijah Newren

Quoted 3 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Quoted 9 lines

Review static int real_merge(struct merge_tree_options *o, · line 451

Quoted 7 lines

(quoted diff) +2 -1

@@ -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);

Perhaps also print a warning to stderr when result.tree is NULL?

I ended up not even touching builtin/merge-tree.c but instead ensuring that result.clean is negative if we fail to write an object (which could happen even in read/write repositories, think "disk full").

As a consequence, we do not even reach this printf() anymore, as you pointed out, a negative result.clean is handled much earlier.

It is handled via a die() in real_merge(), and that will need to change, I think, if we want to continue on the batched merges.

Quoted 22 lines

Review static int real_merge(struct merge_tree_options *o, · line 478

Quoted 9 lines

(quoted diff) +1 -1

        if (!result.clean) {
                struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
                const char *last = NULL;

@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
                                              &result);
        }
        merge_finalize(&opt, &result);
-       return !result.clean; /* result.clean < 0 handled above */
+       return !result.tree || !result.clean; /* result.clean < 0 handled above */

Thinking out loud, should this logic be at the merge-ort.c level, perhaps something like this:

(quoted diff) +2 -0

@@ -4940,6 +4941,9 @@ static void
 merge_ort_nonrecursive_internal(struct merge_options *opt,
        result->tree = parse_tree_indirect(&working_tree_oid);
        /* existence of conflicted entries implies unclean */
        result->clean &= strmap_empty(&opt->priv->conflicted);
+       if (!result->tree)
+               /* This shouldn't happen, because if we did fail to

write a tree we should have returned early before getting here. But just in case we have some bugs... */

Quoted 8 lines

+               result->clean = -1;
        if (!opt->priv->call_depth) {
                result->priv = opt->priv;
                result->_properly_initialized = RESULT_INITIALIZED;

That might benefit callers other than merge-tree, though maybe it makes it harder to print a helpful error message (unless we're fine with the library always throwing one?)

The error messages are already thrown about (this is how it looks like with v3 of this patch):

[...]
+ git -C read-only merge-tree side1 side2
error: insufficient permission for adding an object to repository database ./objects
error: error: Unable to add numbers to database
error: insufficient permission for adding an object to repository database ./objects
error: error: Unable to add greeting to database

error: insufficient permission for adding an object to repository database ./objects

fatal: failure to merge

Quoted 4 lines

And then, possibly post-v2.38.0 though we may be able to get it in before, getting correct propagation of a -1 return value from the source of the error would be good. Would you like to look into that, or would you prefer I did?

It turned out not to be too bad.

Essentially, to propagate write_object_file() failures I only had to change a couple of signatures (with the corresponding returns):
write_tree(), write_completed_directory(), and process_entries(). And then, of course, I needed to change merge_ort_nonrecursive_internal() to respect the propagated error by setting result->clean = -1.

Pretty straight-forward, really, much less involved than I had expected.

From: Johannes Schindelin <[email protected]>

If the blob/tree objects cannot be written, we really need the merge operations to fail, and not to continue (and then try to access the tree object which is however still set to NULL).

Let's stop ignoring the return value of write_object_file() and write_tree() and set clean = -1 in the error case.

 merge-ort.c                      | 64 +++++++++++++++++++-------------
 t/t4301-merge-tree-write-tree.sh |  9 +++++
 2 files changed, 48 insertions(+), 25 deletions(-)

merge-ort.c +5 -5

index 99dcee2db8a..f654296220e 100644
@@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)
                                 b->string, strlen(b->string), bmi->result.mode);
 }
 
-static void write_tree(struct object_id *result_oid,
-                      struct string_list *versions,
-                      unsigned int offset,
-                      size_t hash_size)
+static int write_tree(struct object_id *result_oid,
+                     struct string_list *versions,
+                     unsigned int offset,
+                     size_t hash_size)
 {
        size_t maxlen = 0, extra;
        unsigned int nr;
        struct strbuf buf = STRBUF_INIT;
-       int i;
+       int i, ret;
 
        assert(offset <= versions->nr);
        nr = versions->nr - offset;

merge-ort.c +7 -6

        strbuf_release(&buf);
+       return ret;
 }
 
 static void record_entry_for_tree(struct directory_versions *dir_metadata,

@@ -3625,13 +3626,13 @@ static void record_entry_for_tree(struct directory_versions *dir_metadata,
                           basename)->util = &mi->result;
 }
 
-static void write_completed_directory(struct merge_options *opt,
-                                     const char *new_directory_name,
-                                     struct directory_versions *info)
+static int write_completed_directory(struct merge_options *opt,
+                                    const char *new_directory_name,
+                                    struct directory_versions *info)
 {
        const char *prev_dir;
        struct merged_info *dir_info = NULL;
-       unsigned int offset;
+       unsigned int offset, ret = 0;
 
        /*
         * Some explanation of info->versions and info->offsets...

@@ -3717,7 +3718,7 @@ static void write_completed_directory(struct merge_options *opt,
         * strcmp here.)
         */
        if (new_directory_name == info->last_directory)
-               return;
+               return 0;
 
        /*
         * If we are just starting (last_directory is NULL), or last_directory

@@ -3739,7 +3740,7 @@ static void write_completed_directory(struct merge_options *opt,
                 */
                string_list_append(&info->offsets,
                                   info->last_directory)->util = (void*)offset;
-               return;
+               return 0;
        }
 
        /*

merge-ort.c +23 -11

        }
 
        /*

@@ -3798,6 +3800,8 @@ static void write_completed_directory(struct merge_options *opt,
        /* And, of course, we need to update last_directory to match. */
        info->last_directory = new_directory_name;
        info->last_directory_len = strlen(info->last_directory);
+
+       return ret;
 }
 
 /* Per entry merge function */

@@ -4214,8 +4218,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,
        oid_array_clear(&to_fetch);
 }
 
-static void process_entries(struct merge_options *opt,
-                           struct object_id *result_oid)
+static int process_entries(struct merge_options *opt,
+                          struct object_id *result_oid)
 {
        struct hashmap_iter iter;
        struct strmap_entry *e;

@@ -4224,11 +4228,12 @@ 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;
 
        trace2_region_enter("merge", "process_entries setup", opt->repo);
        if (strmap_empty(&opt->priv->paths)) {
                oidcpy(result_oid, opt->repo->hash_algo->empty_tree);
-               return;
+               return 0;
        }
 
        /* Hack to pre-allocate plist to the desired size */

@@ -4270,8 +4275,11 @@ static void process_entries(struct merge_options *opt,
                 */
                struct merged_info *mi = entry->util;
 
-               write_completed_directory(opt, mi->directory_name,
-                                         &dir_metadata);
+               if (write_completed_directory(opt, mi->directory_name,
+                                             &dir_metadata) < 0) {
+                       ret = -1;
+                       goto cleanup;
+               }
                if (mi->clean)
                        record_entry_for_tree(&dir_metadata, path, mi);
                else {

@@ -4291,12 +4299,15 @@ static void process_entries(struct merge_options *opt,
                fflush(stdout);
                BUG("dir_metadata accounting completely off; shouldn't happen");
        }
-       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);
+cleanup:
        string_list_clear(&plist, 0);
        string_list_clear(&dir_metadata.versions, 0);
        string_list_clear(&dir_metadata.offsets, 0);
        trace2_region_leave("merge", "process_entries cleanup", opt->repo);
+
+       return ret;
 }
 
 /*** Function Grouping: functions related to merge_switch_to_result() ***/

@@ -4928,15 +4939,18 @@ redo:
        }
 
        trace2_region_enter("merge", "process_entries", opt->repo);
-       process_entries(opt, &working_tree_oid);
+       if (process_entries(opt, &working_tree_oid) < 0)
+               result->clean = -1;
        trace2_region_leave("merge", "process_entries", opt->repo);
 
        /* Set return values */
        result->path_messages = &opt->priv->conflicts;
 
-       result->tree = parse_tree_indirect(&working_tree_oid);
-       /* existence of conflicted entries implies unclean */
-       result->clean &= strmap_empty(&opt->priv->conflicted);
+       if (result->clean >= 0) {
+               result->tree = parse_tree_indirect(&working_tree_oid);
+               /* existence of conflicted entries implies unclean */
+               result->clean &= strmap_empty(&opt->priv->conflicted);
+       }
        if (!opt->priv->call_depth) {
                result->priv = opt->priv;
                result->_properly_initialized = RESULT_INITIALIZED;

t/t4301-merge-tree-write-tree.sh +9 -0

index 28ca5c38bb5..013b77144bd 100755
@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '
        test_cmp expect actual
 '
 
+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
+       git init --bare read-only &&
+       git push read-only side1 side2 side3 &&
+       test_when_finished "chmod -R u+w read-only" &&
+       chmod -R a-w read-only &&
+       test_must_fail git -C read-only merge-tree side1 side3 &&
+       test_must_fail git -C read-only merge-tree side1 side2
+'
+
 test_done

base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14 --
gitgitgadget

Quoted 27 lines · Johannes Schindelin via GitGitGadget

merge-ort.c +5 -5

index 99dcee2db8a..f654296220e 100644
@@ -3571,15 +3571,15 @@ static int tree_entry_order(const void *a_, const void *b_)
                                 b->string, strlen(b->string), bmi->result.mode);
 }
 
-static void write_tree(struct object_id *result_oid,
-                      struct string_list *versions,
-                      unsigned int offset,
-                      size_t hash_size)
+static int write_tree(struct object_id *result_oid,
+                     struct string_list *versions,
+                     unsigned int offset,
+                     size_t hash_size)
 {
        size_t maxlen = 0, extra;
        unsigned int nr;
        struct strbuf buf = STRBUF_INIT;
-       int i;
+       int i, ret;
 
        assert(offset <= versions->nr);
        nr = versions->nr - offset;

merge-ort.c static void write_tree(struct object_id *result_oid, · line 3608

Quoted 5 lines

merge-ort.c +1 -1

@@ -3605,8 +3605,9 @@ 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);

This sent me into a bit of a hunt to try to figure out what the return value of write_object_file() is. I know it's non-zero on failure, but don't know if it's always positive or always negative or possibly a mix and how that maps to the idea of "clean_merge". I gave up after a few indirections, to be honest.

Anyway, regardless of my inability to find the answer to the above question, would this be a bit easier to read if we initialized ret to 0 and then did something like

if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
ret = -1;

?

Quoted 5 lines

[...]

merge-ort.c +0 -0

@@ -3769,8 +3770,9 @@ static void write_completed_directory(struct merge_options *opt,
                 */
                dir_info->is_null = 0;
                dir_info->result.mode = S_IFDIR;

merge-ort.c static void write_completed_directory(struct merge_options *opt, · line 3775

Quoted 5 lines

merge-ort.c +3 -2

-               write_tree(&dir_info->result.oid, &info->versions, offset,
-                          opt->repo->hash_algo->rawsz);
+               if (write_tree(&dir_info->result.oid, &info->versions, offset,
+                              opt->repo->hash_algo->rawsz) < 0)
+                       ret = -1;

This makes me wonder about write_tree() again. What if its call to write_object_file() returns a value greater than 0? Is that possible?

If it is, are those error cases or not? About half the callers in the code base that check the return value of write_object_file() check for a non-zero value, the other half check for a value less than 0.

And I can't find any documentation. And it seemed like too much time for me to figure out what its range of return values was.

[...]

Nice to see that it didn't take too much work to propagate the -1 value from write_tree() up the hierarchy. Nice work!

I think we've still got some separate problems related to propagating the return value of the other write_object_file() call, from handle_content_merge(). The direct call in that other case is okay, but the higher levels at process_renames() and process_entry() seem to fumble it. Your fix for failed tree writes is still valid without fixing the failed blob writes, and I'm happy to tackle the other half if you'd prefer to hand it off. But, if you'd like to tackle that half too, you might see fewer error messages when it fails to write to the read-only repository, since it'll fail early on the first blob write instead of only failing when it gets around to trying to write a new tree.

From: Johannes Schindelin <[email protected]>

In the previous commit, we fixed a segmentation fault when a tree object could not be written.

However, before the tree object is written, merge-ort wants to write out a blob object (except in cases where the merge results in a blob that already exists in the database). And this can fail, too, but we ignore that write failure so far.

From: Johannes Schindelin <[email protected]>

In the previous commit, we fixed a segmentation fault when a tree object could not be written.

However, before the tree object is written, merge-ort wants to write out a blob object (except in cases where the merge results in a blob that already exists in the database). And this can fail, too, but we ignore that write failure so far.

Since we will always write out a new tree object in addition to the blob (and if the blob did not exist in the database yet, we can be certain that the tree object did not exist yet), the merge will _still_ fail at that point, but it does unnecessary work by continuing after the blob could not be written.

Let's pay close attention and error out early if the blob could not be written. This reduces the error output of t4301.25 ("merge-ort fails gracefully in a read-only repository") from:

error: insufficient permission for adding an object to repository database ./objects
error: error: Unable to add numbers to database
error: insufficient permission for adding an object to repository database ./objects
error: error: Unable to add greeting to database
error: insufficient permission for adding an object to repository database ./objects
fatal: failure to merge

to:

error: insufficient permission for adding an object to repository database ./objects
error: error: Unable to add numbers to database
fatal: failure to merge

Note: This patch adjusts two variable declarations from unsigned to int because their purpose is to hold the return value of handle_content_merge(), which is of type int. The existing users of those variables are only interested whether that variable is zero or non-zero, therefore this type change does not affect the existing code.

Signed-off-by Johannes Schindelin <[email protected]>

---
 merge-ort.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

merge-ort.c +19 -10

index f3bdce1041a..e5f41cce481 100644
@@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,
                                                           pathnames,
                                                           1 + 2 * opt->priv->call_depth,
                                                           &merged);
+                       if (clean_merge < 0)
+                               return -1;
                        if (!clean_merge &&
                            merged.mode == side1->stages[1].mode &&
                            oideq(&merged.oid, &side1->stages[1].oid))

@@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,
                        struct version_info merged;
 
                        struct conflict_info *base, *side1, *side2;
-                       unsigned clean;
+                       int clean;
 
                        pathnames[0] = oldpath;
                        pathnames[other_source_index] = oldpath;

@@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,
                                                     pathnames,
                                                     1 + 2 * opt->priv->call_depth,
                                                     &merged);
+                       if (clean < 0)
+                               return -1;
 
                        memcpy(&newinfo->stages[target_index], &merged,
                               sizeof(merged));

@@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,
 }
 
 /* Per entry merge function */
-static void process_entry(struct merge_options *opt,
-                         const char *path,
-                         struct conflict_info *ci,
-                         struct directory_versions *dir_metadata)
+static int process_entry(struct merge_options *opt,
+                        const char *path,
+                        struct conflict_info *ci,
+                        struct directory_versions *dir_metadata)
 {
        int df_file_index = 0;
 

@@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,
                record_entry_for_tree(dir_metadata, path, &ci->merged);
                if (ci->filemask == 0)
                        /* nothing else to handle */
-                       return;
+                       return 0;
                assert(ci->df_conflict);
        }
 

@@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,
                 */
                if (ci->filemask == 1) {
                        ci->filemask = 0;
-                       return;
+                       return 0;
                }
 
                /*

@@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,
        } else if (ci->filemask >= 6) {
                /* Need a two-way or three-way content merge */
                struct version_info merged_file;
-               unsigned clean_merge;
+               int clean_merge;
                struct version_info *o = &ci->stages[0];
                struct version_info *a = &ci->stages[1];
                struct version_info *b = &ci->stages[2];

@@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,
                                                   ci->pathnames,
                                                   opt->priv->call_depth * 2,
                                                   &merged_file);
+               if (clean_merge < 0)
+                       return -1;
                ci->merged.clean = clean_merge &&
                                   !ci->df_conflict && !ci->path_conflict;
                ci->merged.result.mode = merged_file.mode;

@@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,
 
        /* Record metadata for ci->merged in dir_metadata */
        record_entry_for_tree(dir_metadata, path, &ci->merged);
+       return 0;
 }
 
 static void prefetch_for_content_merges(struct merge_options *opt,

@@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,
                        record_entry_for_tree(&dir_metadata, path, mi);
                else {
                        struct conflict_info *ci = (struct conflict_info *)mi;
-                       process_entry(opt, path, ci, &dir_metadata);
+                       if (process_entry(opt, path, ci, &dir_metadata) < 0) {
+                               ret = -1;
+                               goto cleanup;
+                       };
                }
        }
        trace2_region_leave("merge", "processing", opt->repo);
--

gitgitgadget

Quoted 5 lines

Since we will always write out a new tree object in addition to the blob (and if the blob did not exist in the database yet, we can be certain that the tree object did not exist yet), the merge will _still_ fail at that point, but it does unnecessary work by continuing after the blob could not be written.

I don't think this is quite true. I've had a number of users come to me with "messed up git repositories" where I eventually discover that users just randomly add "sudo" to some of their commands because when things don't work, sometimes "sudo" fixes it. That means they've created stuff in their .git directory which may be owned by root, perhaps even some of the .git/objects/XX directories. However, they may have other .git/objects/XX directories owned by their normal user.

If just some .git/objects/XX directories are owned by root and not others, then it may be when users run git commands as themselves that some things can be written to the object database but not others. In particular, it could be that writing blob objects fail, but writing a tree object which references those blobs succeeds.

Patch looks good to me, though.

Turns out that the segmentation fault reported by Taylor
[https://lore.kernel.org/git/[email protected]/] 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).

This is _not_ just a cosmetic change: Even though one might assume that the operation would have failed anyway at the point when the new tree object is written (and the corresponding tree object _will_ be new if it contains a blob that is new), but that is not so: As pointed out by Elijah Newren, when Git has previously been allowed to add loose objects via sudo calls, it is very possible that the blob object cannot be written (because the corresponding .git/objects/??/ directory may be owned by root) but the tree object can be written (because the corresponding objects directory is owned by the current user). This would result in a corrupt repository because it is missing the blob object, and with this here patch we prevent that.

Note: This patch adjusts two variable declarations from unsigned to int because their purpose is to hold the return value of handle_content_merge(), which is of type int. The existing users of those variables are only interested whether that variable is zero or non-zero, therefore this type change does not affect the existing code.

Reviewed-by Elijah Newren <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---

 merge-ort.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 18 lines · writes:

This is _not_ just a cosmetic change: Even though one might assume that the operation would have failed anyway at the point when the new tree object is written (and the corresponding tree object _will_ be new if it contains a blob that is new), but that is not so: As pointed out by Elijah Newren, when Git has previously been allowed to add loose objects via sudo calls, it is very possible that the blob object cannot be written (because the corresponding .git/objects/??/ directory may be owned by root) but the tree object can be written (because the corresponding objects directory is owned by the current user). This would result in a corrupt repository because it is missing the blob object, and with this here patch we prevent that.

Note: This patch adjusts two variable declarations from unsigned to int because their purpose is to hold the return value of handle_content_merge(), which is of type int. The existing users of those variables are only interested whether that variable is zero or non-zero, therefore this type change does not affect the existing code.

Quoted 3 lines

Reviewed-by Elijah Newren <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---

Thanks. The first paragraph in the quoted part above is new and not exactly "reviewed" by Elijah yet, so let's ask ;-)

To me the description of the issue looks reasonable to me. Any comments, Elijah?

Quoted 2 lines

To me the description of the issue looks reasonable to me. Any comments, Elijah?

Looks good to me!