merge-tree: handle missing objects correctly

I recently looked into issues where git merge-tree calls returned bogus data (in one instance returning an empty tree for non-empty merge parents). By the time I had a look at the corresponding repository, the issue was no longer reproducible, but a closer look at the code combined with some manual experimenting turned up the fact that missing tree objects aren't handled as errors by git merge-tree.

While at it, I added a commit on top that tries to catch all remaining unchecked parse_tree() calls.

This patch series is based on js/merge-tree-3-trees because I introduced three unchecked parse_tree() calls in that topic branch.

Johannes Schindelin (4):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check parse_tree()'s return value
t4301: verify that merge-tree fails on missing blob objects Always check parse_tree*()'s return value

 builtin/checkout.c               | 19 ++++++++++++++++---
 builtin/clone.c                  |  3 ++-
 builtin/commit.c                 |  3 ++-
 builtin/merge-tree.c             |  6 ++++++
 builtin/read-tree.c              |  3 ++-
 builtin/reset.c                  |  4 ++++
 cache-tree.c                     |  4 ++--
 merge-ort.c                      | 16 +++++++++++-----
 merge-recursive.c                |  3 ++-
 merge.c                          |  5 ++++-
 reset.c                          |  5 +++++
 sequencer.c                      |  4 ++++
 t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++
 13 files changed, 84 insertions(+), 15 deletions(-)

base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1651

gitgitgadget

From: Johannes Schindelin <[email protected]>

When git merge-tree encounters a missing tree object, it should error out and not continue quietly as if nothing had happened.

However, as of time of writing, git merge-tree _does_ continue, and then offers the empty tree as result.

Let's fix this.

---
 merge-ort.c                      |  7 ++++---
 t/t4301-merge-tree-write-tree.sh | 10 ++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

index 6491070d965..c37fc035f13 100644
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	parse_tree(merge_base);
-	parse_tree(side1);
-	parse_tree(side2);
+	if (parse_tree(merge_base) < 0 ||
+	    parse_tree(side1) < 0 ||
+	    parse_tree(side2) < 0)
+		return -1;
 	init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
 	init_tree_desc(t + 1, side1->buffer, side1->size);
 	init_tree_desc(t + 2, side2->buffer, side2->size);

index 7d0fa74da74..4ea1b74445d 100755
@@ -950,5 +950,15 @@ test_expect_success '--merge-base with tree OIDs' '
 	git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&
 	test_cmp with-commits with-trees
 '
+test_expect_success 'error out on missing tree objects' '
+	git init --bare missing-tree.git &&
+	(
+		git rev-list side3 &&
+		git rev-parse side3^:
+	) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&
+	side3=$(git rev-parse side3) &&
+	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&
+	test_must_be_empty actual
+'
 
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

The previous commit fixed a bug where a missing tree was reported, but not treated as an error.

This patch addresses the same issue for the remaining two callers of parse_tree().

This change is not accompanied by a regression test because the code in question is only reached at the checkout stage, i.e. after the merge has happened (and therefore the tree objects could only be missing if the disk had gone bad in that short time window, or something similarly tricky to recreate in the test suite).

---
 merge-ort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index c37fc035f13..79d9e18f63d 100644
@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	parse_tree(prev);
+	if (parse_tree(prev) < 0)
+		return -1;
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
-	parse_tree(next);
+	if (parse_tree(next) < 0)
+		return -1;
 	init_tree_desc(&trees[1], next->buffer, next->size);
 
 	ret = unpack_trees(2, trees, &unpack_opts);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We just fixed a problem where merge-tree would not fail on missing tree objects. Let's ensure that that problem does not occur with blob objects (and won't, in the future, either).

---
 t/t4301-merge-tree-write-tree.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 4ea1b74445d..86807a57d4d 100755
@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'error out on missing blob objects' '
+	seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
+	seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
+	seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
+	tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
+	tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
+	tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
+	git init --bare missing-blob.git &&
+	test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
+	git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+	test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

Otherwise we may easily run into serious crashes: For example, if we run init_tree_desc() directly after a failed parse_tree(), we are accessing uninitialized data or trying to dereference NULL.

Note that the parse_tree() function already takes care of showing an error message. The parse_tree_indirectly() and repo_get_commit_tree() functions do not, therefore those latter call sites need to show a useful error message while the former do not.

---
 builtin/checkout.c   | 19 ++++++++++++++++---
 builtin/clone.c      |  3 ++-
 builtin/commit.c     |  3 ++-
 builtin/merge-tree.c |  6 ++++++
 builtin/read-tree.c  |  3 ++-
 builtin/reset.c      |  4 ++++
 cache-tree.c         |  4 ++--
 merge-ort.c          |  3 +++
 merge-recursive.c    |  3 ++-
 merge.c              |  5 ++++-
 reset.c              |  5 +++++
 sequencer.c          |  4 ++++
 12 files changed, 52 insertions(+), 10 deletions(-)

index f02434bc155..84108ec3635 100644
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		return 128;
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
 	case -2:

@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		if (new_branch_info->commit)
 			BUG("'switch --orphan' should never accept a commit as starting point");
 		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
-	} else
+		if (!new_tree)
+			BUG("unable to read empty tree");
+	} else {
 		new_tree = repo_get_commit_tree(the_repository,
 						new_branch_info->commit);
+		if (!new_tree)
+			return error(_("unable to read tree %s"),
+				     oid_to_hex(&new_branch_info->commit->object.oid));
+	}
 	if (opts->discard_changes) {
 		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
 		if (ret)

@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				oid_to_hex(old_commit_oid));
 
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
-		parse_tree(new_tree);
+		if (parse_tree(new_tree) < 0)
+			exit(128);
 		tree = new_tree;
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 

@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
 	if (!new_branch_info->commit) {
 		/* not a commit */
 		*source_tree = parse_tree_indirect(rev);
+		if (!*source_tree)
+			die(_("unable to read tree %s"), oid_to_hex(rev));
 	} else {
 		parse_commit_or_die(new_branch_info->commit);
 		*source_tree = repo_get_commit_tree(the_repository,
 						    new_branch_info->commit);
+		if (!*source_tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&new_branch_info->commit->object.oid));
 	}
 }
 

index c6357af9498..4410b55be98 100644
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die(_("unable to parse commit %s"), oid_to_hex(&oid));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));

index 781af2e206c..0723f06de7a 100644
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
 	tree = parse_tree_indirect(&current_head->object.oid);
 	if (!tree)
 		die(_("failed to unpack HEAD tree object"));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
 		exit(128); /* We've already reported the error, finish dying */

index 2d4ce5b3886..ba84d00deee 100644
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
 		if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
 			die(_("could not parse as tree '%s'"), merge_base);
 		base_tree = parse_tree_indirect(&base_oid);
+		if (!base_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&base_oid));
 		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
 			die(_("could not parse as tree '%s'"), branch1);
 		parent1_tree = parse_tree_indirect(&head_oid);
+		if (!parent1_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&head_oid));
 		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
 			die(_("could not parse as tree '%s'"), branch2);
 		parent2_tree = parse_tree_indirect(&merge_oid);
+		if (!parent2_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
 
 		opt.ancestor = merge_base;
 		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);

index 8196ca9dd85..5923ed36893 100644
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	cache_tree_free(&the_index.cache_tree);
 	for (i = 0; i < nr_trees; i++) {
 		struct tree *tree = trees[i];
-		parse_tree(tree);
+		if (parse_tree(tree) < 0)
+			return 128;
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))

index 4b018d20e3b..f030f57f4e9 100644
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == MIXED || reset_type == HARD) {
 		tree = parse_tree_indirect(oid);
+		if (!tree) {
+			error(_("unable to read tree %s"), oid_to_hex(oid));
+			goto out;
+		}
 		prime_cache_tree(the_repository, the_repository->index, tree);
 	}
 

index 641427ed410..c6508b64a5c 100644
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed)
-				parse_tree(subtree);
+			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();
 

index 79d9e18f63d..534ddaf16ba 100644
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 
 	if (result->clean >= 0) {
 		result->tree = parse_tree_indirect(&working_tree_oid);
+		if (!result->tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&working_tree_oid));
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}

index e3beb0801b1..10d41bfd487 100644
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
 
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(desc, tree->buffer, tree->size);
 }
 

index b60925459c2..14a7325859d 100644
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
 		return -1;
 	}
 	for (i = 0; i < nr_trees; i++) {
-		parse_tree(trees[i]);
+		if (parse_tree(trees[i]) < 0) {
+			rollback_lock_file(&lock_file);
+			return -1;
+		}
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 

index 48da0adf851..a93fdbc12e3 100644
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
 	}
 
 	tree = parse_tree_indirect(oid);
+	if (!tree) {
+		ret = error(_("unable to read tree %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {

index d584cac8ed9..407473bab28 100644
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
 	o.show_rename_progress = 1;
 
 	head_tree = parse_tree_indirect(head);
+	if (!head_tree)
+		return error(_("unable to read tree %s"), oid_to_hex(head));
 	next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
 	base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
 

@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
 	}
 
 	tree = parse_tree_indirect(&oid);
+	if (!tree)
+		return error(_("unable to read tree %s"), oid_to_hex(&oid));
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)

gitgitgadget

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

Thanks. All the added checks looked reasonable to me.

Will queue.

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

    int init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
    {
	if (parse_tree(tree))
		return -1;
	init_tree_desc(desc, tree->buffer, tree->size);
    }

[snip]

These patches all look good to me, thanks! I have two nits for the first patch, but I don't really think that those are worth a reroll.

Patrick

I recently looked into issues where git merge-tree calls returned bogus data (in one instance returning an empty tree for non-empty merge parents). By the time I had a look at the corresponding repository, the issue was no longer reproducible, but a closer look at the code combined with some manual experimenting turned up the fact that missing tree objects aren't handled as errors by git merge-tree.

While at it, I added a commit on top that tries to catch all remaining unchecked parse_tree() calls.

This patch series is based on js/merge-tree-3-trees because I introduced three unchecked parse_tree() calls in that topic branch.

Changes since v1:

* Simplified the test case, avoiding a subshell and a pipe in the process.
* Added a patch to remove a superfluous subtree->object.parsed guard around a parse_tree(subtree) call.

Johannes Schindelin (5):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check parse_tree()'s return value
t4301: verify that merge-tree fails on missing blob objects Always check parse_tree*()'s return value
cache-tree: avoid an unnecessary check

 builtin/checkout.c               | 19 ++++++++++++++++---
 builtin/clone.c                  |  3 ++-
 builtin/commit.c                 |  3 ++-
 builtin/merge-tree.c             |  6 ++++++
 builtin/read-tree.c              |  3 ++-
 builtin/reset.c                  |  4 ++++
 cache-tree.c                     |  4 ++--
 merge-ort.c                      | 16 +++++++++++-----
 merge-recursive.c                |  3 ++-
 merge.c                          |  5 ++++-
 reset.c                          |  5 +++++
 sequencer.c                      |  4 ++++
 t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++
 13 files changed, 84 insertions(+), 15 deletions(-)

base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1651

Range-diff vs v1:

1: a3e8ae86114 ! 1: 01dfd66568c merge-tree: fail with a non-zero exit code on missing tree objects

     @@ merge-ort.c: static int collect_merge_info(struct merge_options *opt,
      
       ## t/t4301-merge-tree-write-tree.sh ##
      @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--merge-base with tree OIDs' '
     - 	git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&
       	test_cmp with-commits with-trees
       '
     + 
      +test_expect_success 'error out on missing tree objects' '
      +	git init --bare missing-tree.git &&
     -+	(
     -+		git rev-list side3 &&
     -+		git rev-parse side3^:
     -+	) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&
     ++	git rev-list side3 >list &&
     ++	git rev-parse side3^: >list &&
     ++	git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
      +	side3=$(git rev-parse side3) &&
      +	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&
      +	test_must_be_empty actual
      +'
     - 
     ++
       test_done
 2:  3e5b787fc03 = 2:  a1bbb7e06e5 merge-ort: do check `parse_tree()`'s return value
 3:  85d3e672871 = 3:  be1dadf2850 t4301: verify that merge-tree fails on missing blob objects
 4:  93abd7000b8 = 4:  ffd38ad602a Always check `parse_tree*()`'s return value
 -:  ----------- > 5:  43c04749513 cache-tree: avoid an unnecessary check

gitgitgadget

From: Johannes Schindelin <[email protected]>

When git merge-tree encounters a missing tree object, it should error out and not continue quietly as if nothing had happened.

However, as of time of writing, git merge-tree _does_ continue, and then offers the empty tree as result.

Let's fix this.

---
 merge-ort.c                      |  7 ++++---
 t/t4301-merge-tree-write-tree.sh | 10 ++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

index 6491070d965..c37fc035f13 100644
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	parse_tree(merge_base);
-	parse_tree(side1);
-	parse_tree(side2);
+	if (parse_tree(merge_base) < 0 ||
+	    parse_tree(side1) < 0 ||
+	    parse_tree(side2) < 0)
+		return -1;
 	init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
 	init_tree_desc(t + 1, side1->buffer, side1->size);
 	init_tree_desc(t + 2, side2->buffer, side2->size);

index 7d0fa74da74..7d588557bdf 100755
@@ -951,4 +951,14 @@ test_expect_success '--merge-base with tree OIDs' '
 	test_cmp with-commits with-trees
 '
 
+test_expect_success 'error out on missing tree objects' '
+	git init --bare missing-tree.git &&
+	git rev-list side3 >list &&
+	git rev-parse side3^: >list &&
+	git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
+	side3=$(git rev-parse side3) &&
+	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

The previous commit fixed a bug where a missing tree was reported, but not treated as an error.

This patch addresses the same issue for the remaining two callers of parse_tree().

This change is not accompanied by a regression test because the code in question is only reached at the checkout stage, i.e. after the merge has happened (and therefore the tree objects could only be missing if the disk had gone bad in that short time window, or something similarly tricky to recreate in the test suite).

---
 merge-ort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index c37fc035f13..79d9e18f63d 100644
@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	parse_tree(prev);
+	if (parse_tree(prev) < 0)
+		return -1;
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
-	parse_tree(next);
+	if (parse_tree(next) < 0)
+		return -1;
 	init_tree_desc(&trees[1], next->buffer, next->size);
 
 	ret = unpack_trees(2, trees, &unpack_opts);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We just fixed a problem where merge-tree would not fail on missing tree objects. Let's ensure that that problem does not occur with blob objects (and won't, in the future, either).

---
 t/t4301-merge-tree-write-tree.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 7d588557bdf..9211cb58aa1 100755
@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'error out on missing blob objects' '
+	seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
+	seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
+	seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
+	tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
+	tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
+	tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
+	git init --bare missing-blob.git &&
+	test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
+	git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+	test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

Otherwise we may easily run into serious crashes: For example, if we run init_tree_desc() directly after a failed parse_tree(), we are accessing uninitialized data or trying to dereference NULL.

Note that the parse_tree() function already takes care of showing an error message. The parse_tree_indirectly() and repo_get_commit_tree() functions do not, therefore those latter call sites need to show a useful error message while the former do not.

---
 builtin/checkout.c   | 19 ++++++++++++++++---
 builtin/clone.c      |  3 ++-
 builtin/commit.c     |  3 ++-
 builtin/merge-tree.c |  6 ++++++
 builtin/read-tree.c  |  3 ++-
 builtin/reset.c      |  4 ++++
 cache-tree.c         |  4 ++--
 merge-ort.c          |  3 +++
 merge-recursive.c    |  3 ++-
 merge.c              |  5 ++++-
 reset.c              |  5 +++++
 sequencer.c          |  4 ++++
 12 files changed, 52 insertions(+), 10 deletions(-)

index f02434bc155..84108ec3635 100644
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		return 128;
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
 	case -2:

@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		if (new_branch_info->commit)
 			BUG("'switch --orphan' should never accept a commit as starting point");
 		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
-	} else
+		if (!new_tree)
+			BUG("unable to read empty tree");
+	} else {
 		new_tree = repo_get_commit_tree(the_repository,
 						new_branch_info->commit);
+		if (!new_tree)
+			return error(_("unable to read tree %s"),
+				     oid_to_hex(&new_branch_info->commit->object.oid));
+	}
 	if (opts->discard_changes) {
 		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
 		if (ret)

@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				oid_to_hex(old_commit_oid));
 
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
-		parse_tree(new_tree);
+		if (parse_tree(new_tree) < 0)
+			exit(128);
 		tree = new_tree;
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 

@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
 	if (!new_branch_info->commit) {
 		/* not a commit */
 		*source_tree = parse_tree_indirect(rev);
+		if (!*source_tree)
+			die(_("unable to read tree %s"), oid_to_hex(rev));
 	} else {
 		parse_commit_or_die(new_branch_info->commit);
 		*source_tree = repo_get_commit_tree(the_repository,
 						    new_branch_info->commit);
+		if (!*source_tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&new_branch_info->commit->object.oid));
 	}
 }
 

index c6357af9498..4410b55be98 100644
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die(_("unable to parse commit %s"), oid_to_hex(&oid));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));

index 781af2e206c..0723f06de7a 100644
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
 	tree = parse_tree_indirect(&current_head->object.oid);
 	if (!tree)
 		die(_("failed to unpack HEAD tree object"));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
 		exit(128); /* We've already reported the error, finish dying */

index 2d4ce5b3886..ba84d00deee 100644
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
 		if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
 			die(_("could not parse as tree '%s'"), merge_base);
 		base_tree = parse_tree_indirect(&base_oid);
+		if (!base_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&base_oid));
 		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
 			die(_("could not parse as tree '%s'"), branch1);
 		parent1_tree = parse_tree_indirect(&head_oid);
+		if (!parent1_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&head_oid));
 		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
 			die(_("could not parse as tree '%s'"), branch2);
 		parent2_tree = parse_tree_indirect(&merge_oid);
+		if (!parent2_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
 
 		opt.ancestor = merge_base;
 		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);

index 8196ca9dd85..5923ed36893 100644
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	cache_tree_free(&the_index.cache_tree);
 	for (i = 0; i < nr_trees; i++) {
 		struct tree *tree = trees[i];
-		parse_tree(tree);
+		if (parse_tree(tree) < 0)
+			return 128;
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))

index 4b018d20e3b..f030f57f4e9 100644
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == MIXED || reset_type == HARD) {
 		tree = parse_tree_indirect(oid);
+		if (!tree) {
+			error(_("unable to read tree %s"), oid_to_hex(oid));
+			goto out;
+		}
 		prime_cache_tree(the_repository, the_repository->index, tree);
 	}
 

index 641427ed410..c6508b64a5c 100644
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed)
-				parse_tree(subtree);
+			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();
 

index 79d9e18f63d..534ddaf16ba 100644
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 
 	if (result->clean >= 0) {
 		result->tree = parse_tree_indirect(&working_tree_oid);
+		if (!result->tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&working_tree_oid));
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}

index e3beb0801b1..10d41bfd487 100644
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
 
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(desc, tree->buffer, tree->size);
 }
 

index b60925459c2..14a7325859d 100644
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
 		return -1;
 	}
 	for (i = 0; i < nr_trees; i++) {
-		parse_tree(trees[i]);
+		if (parse_tree(trees[i]) < 0) {
+			rollback_lock_file(&lock_file);
+			return -1;
+		}
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 

index 48da0adf851..a93fdbc12e3 100644
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
 	}
 
 	tree = parse_tree_indirect(oid);
+	if (!tree) {
+		ret = error(_("unable to read tree %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {

index d584cac8ed9..407473bab28 100644
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
 	o.show_rename_progress = 1;
 
 	head_tree = parse_tree_indirect(head);
+	if (!head_tree)
+		return error(_("unable to read tree %s"), oid_to_hex(head));
 	next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
 	base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
 

@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
 	}
 
 	tree = parse_tree_indirect(&oid);
+	if (!tree)
+		return error(_("unable to read tree %s"), oid_to_hex(&oid));
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)

gitgitgadget

From: Johannes Schindelin <[email protected]>

The first thing the parse_tree() function does is to return early if the tree has already been parsed. Therefore we do not need to guard the parse_tree() call behind a check of that flag.

As of time of writing, there are no other instances of this in Git's code bases: whenever the parsed flag guards a parse_tree() call, it guards more than just that call.

---
 cache-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c6508b64a5c..78d6ba92853 100644
@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+			if (parse_tree(subtree) < 0)
 				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();

gitgitgadget

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

Makes sense, and it doubly makes sense to keep this separate from the change done to the same location in [4/5].

Will queue.

If you happen to reroll for some reason, perhaps: s/Always/always/

All good points. Thanks for excellent reviews (not just this one but another one in the series).

The printf example is probably cleaner, thus preferable. For completeness, though, I should mention that the echo example is, of course, not quite correct. For the interpolation to work as intended, it would need ${...}:

    tree1=$(echo "100644 blob ${seq1}Qsequence" | q_to_tab | git mktree) &&
    tree2=$(echo "100644 blob ${seq2}Qsequence" | q_to_tab | git mktree) &&
    tree3=$(echo "100644 blob ${seq3}Qsequence" | q_to_tab | git mktree) &&

Hi Eric,

Unless I am missing something we only ask the part of a oneline after an initial "<something>:" to be downcased.

At least all the "Merge branch [...]" commits are still capitalized and nobody complains ;-)

Ciao,
Johannes

Hi Eric,

Yes. And the fact that this test case still passed was for the _wrong_ reason! I adjusted the test case by writing the correct contents to list and by verifying that the expected error message is shown.

I find that uglier and longer, so I'll stay with the first option ;-)

Thank you for your review!
Johannes

Hi Eric,

Originally, there was, since I wanted the merge to succeed without conflicts.

However, I simplified the test case similar to what you suggested and now specifically verify that the expected error message is shown (as opposed to, say, an error message indicating a merge conflict).

I added the \n and now avoid the %s.

Thank you for your review!
Johannes

I recently looked into issues where git merge-tree calls returned bogus data (in one instance returning an empty tree for non-empty merge parents). By the time I had a look at the corresponding repository, the issue was no longer reproducible, but a closer look at the code combined with some manual experimenting turned up the fact that missing tree objects aren't handled as errors by git merge-tree.

While at it, I added a commit on top that tries to catch all remaining unchecked parse_tree() calls.

This patch series is based on 5f43cf5b2e4 (merge-tree: accept 3 trees as arguments, 2024-01-28) (the original tip commit of js/merge-tree-3-trees) because I introduced three unchecked parse_tree() calls in that topic.

Changes since v2:

* Fixed the new "missing tree object" test case in t4301 that succeeded for the wrong reason.
* Adjusted the new "missing blob object" test case to avoid succeeding for the wrong reason.
* Simplified the "missing blob object" test case.

Changes since v1:

* Simplified the test case, avoiding a subshell and a pipe in the process.
* Added a patch to remove a superfluous subtree->object.parsed guard around a parse_tree(subtree) call.

Johannes Schindelin (5):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check parse_tree()'s return value
t4301: verify that merge-tree fails on missing blob objects Always check parse_tree*()'s return value
cache-tree: avoid an unnecessary check

 builtin/checkout.c               | 19 ++++++++++++++++---
 builtin/clone.c                  |  3 ++-
 builtin/commit.c                 |  3 ++-
 builtin/merge-tree.c             |  6 ++++++
 builtin/read-tree.c              |  3 ++-
 builtin/reset.c                  |  4 ++++
 cache-tree.c                     |  4 ++--
 merge-ort.c                      | 16 +++++++++++-----
 merge-recursive.c                |  3 ++-
 merge.c                          |  5 ++++-
 reset.c                          |  5 +++++
 sequencer.c                      |  4 ++++
 t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++++++++++
 13 files changed, 87 insertions(+), 15 deletions(-)

base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1651

Range-diff vs v2:

1: 01dfd66568c ! 1: 11b9cd8c5da merge-tree: fail with a non-zero exit code on missing tree objects

     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--merge-base with tree OI
      +test_expect_success 'error out on missing tree objects' '
      +	git init --bare missing-tree.git &&
      +	git rev-list side3 >list &&
     -+	git rev-parse side3^: >list &&
     ++	git rev-parse side3^: >>list &&
      +	git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
      +	side3=$(git rev-parse side3) &&
     -+	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&
     ++	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&
     ++	test_grep "Could not read $(git rev-parse $side3:)" err &&
      +	test_must_be_empty actual
      +'
      +
 2:  a1bbb7e06e5 = 2:  f01f4eb011b merge-ort: do check `parse_tree()`'s return value
 3:  be1dadf2850 ! 3:  e82fdf7fbcb t4301: verify that merge-tree fails on missing blob objects
     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'error out on missing tree
       '
       
      +test_expect_success 'error out on missing blob objects' '
     -+	seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
     -+	seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
     -+	seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
     -+	tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
     -+	tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
     -+	tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
     ++	echo 1 | git hash-object -w --stdin >blob1 &&
     ++	echo 2 | git hash-object -w --stdin >blob2 &&
     ++	echo 3 | git hash-object -w --stdin >blob3 &&
     ++	printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
     ++	printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
     ++	printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
      +	git init --bare missing-blob.git &&
     -+	test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
     ++	cat blob1 blob3 tree1 tree2 tree3 |
      +	git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
     -+	test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
     ++	test_must_fail git --git-dir=missing-blob.git >actual 2>err \
     ++		merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
     ++	test_grep "unable to read blob object $(cat blob2)" err &&
      +	test_must_be_empty actual
      +'
      +
 4:  ffd38ad602a = 4:  9e4dc94ef03 Always check `parse_tree*()`'s return value
 5:  43c04749513 = 5:  91dc4ccd04e cache-tree: avoid an unnecessary check

gitgitgadget

From: Johannes Schindelin <[email protected]>

When git merge-tree encounters a missing tree object, it should error out and not continue quietly as if nothing had happened.

However, as of time of writing, git merge-tree _does_ continue, and then offers the empty tree as result.

Let's fix this.

---
 merge-ort.c                      |  7 ++++---
 t/t4301-merge-tree-write-tree.sh | 11 +++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

index 6491070d965..c37fc035f13 100644
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	parse_tree(merge_base);
-	parse_tree(side1);
-	parse_tree(side2);
+	if (parse_tree(merge_base) < 0 ||
+	    parse_tree(side1) < 0 ||
+	    parse_tree(side2) < 0)
+		return -1;
 	init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
 	init_tree_desc(t + 1, side1->buffer, side1->size);
 	init_tree_desc(t + 2, side2->buffer, side2->size);

index 7d0fa74da74..908c9b540c8 100755
@@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '
 	test_cmp with-commits with-trees
 '
 
+test_expect_success 'error out on missing tree objects' '
+	git init --bare missing-tree.git &&
+	git rev-list side3 >list &&
+	git rev-parse side3^: >>list &&
+	git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
+	side3=$(git rev-parse side3) &&
+	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&
+	test_grep "Could not read $(git rev-parse $side3:)" err &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

The previous commit fixed a bug where a missing tree was reported, but not treated as an error.

This patch addresses the same issue for the remaining two callers of parse_tree().

This change is not accompanied by a regression test because the code in question is only reached at the checkout stage, i.e. after the merge has happened (and therefore the tree objects could only be missing if the disk had gone bad in that short time window, or something similarly tricky to recreate in the test suite).

---
 merge-ort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index c37fc035f13..79d9e18f63d 100644
@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	parse_tree(prev);
+	if (parse_tree(prev) < 0)
+		return -1;
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
-	parse_tree(next);
+	if (parse_tree(next) < 0)
+		return -1;
 	init_tree_desc(&trees[1], next->buffer, next->size);
 
 	ret = unpack_trees(2, trees, &unpack_opts);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We just fixed a problem where merge-tree would not fail on missing tree objects. Let's ensure that that problem does not occur with blob objects (and won't, in the future, either).

---
 t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index 908c9b540c8..d4463a45706 100755
@@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'error out on missing blob objects' '
+	echo 1 | git hash-object -w --stdin >blob1 &&
+	echo 2 | git hash-object -w --stdin >blob2 &&
+	echo 3 | git hash-object -w --stdin >blob3 &&
+	printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
+	printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
+	printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
+	git init --bare missing-blob.git &&
+	cat blob1 blob3 tree1 tree2 tree3 |
+	git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+	test_must_fail git --git-dir=missing-blob.git >actual 2>err \
+		merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
+	test_grep "unable to read blob object $(cat blob2)" err &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

Otherwise we may easily run into serious crashes: For example, if we run init_tree_desc() directly after a failed parse_tree(), we are accessing uninitialized data or trying to dereference NULL.

Note that the parse_tree() function already takes care of showing an error message. The parse_tree_indirectly() and repo_get_commit_tree() functions do not, therefore those latter call sites need to show a useful error message while the former do not.

---
 builtin/checkout.c   | 19 ++++++++++++++++---
 builtin/clone.c      |  3 ++-
 builtin/commit.c     |  3 ++-
 builtin/merge-tree.c |  6 ++++++
 builtin/read-tree.c  |  3 ++-
 builtin/reset.c      |  4 ++++
 cache-tree.c         |  4 ++--
 merge-ort.c          |  3 +++
 merge-recursive.c    |  3 ++-
 merge.c              |  5 ++++-
 reset.c              |  5 +++++
 sequencer.c          |  4 ++++
 12 files changed, 52 insertions(+), 10 deletions(-)

index f02434bc155..84108ec3635 100644
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		return 128;
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
 	case -2:

@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		if (new_branch_info->commit)
 			BUG("'switch --orphan' should never accept a commit as starting point");
 		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
-	} else
+		if (!new_tree)
+			BUG("unable to read empty tree");
+	} else {
 		new_tree = repo_get_commit_tree(the_repository,
 						new_branch_info->commit);
+		if (!new_tree)
+			return error(_("unable to read tree %s"),
+				     oid_to_hex(&new_branch_info->commit->object.oid));
+	}
 	if (opts->discard_changes) {
 		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
 		if (ret)

@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				oid_to_hex(old_commit_oid));
 
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
-		parse_tree(new_tree);
+		if (parse_tree(new_tree) < 0)
+			exit(128);
 		tree = new_tree;
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 

@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
 	if (!new_branch_info->commit) {
 		/* not a commit */
 		*source_tree = parse_tree_indirect(rev);
+		if (!*source_tree)
+			die(_("unable to read tree %s"), oid_to_hex(rev));
 	} else {
 		parse_commit_or_die(new_branch_info->commit);
 		*source_tree = repo_get_commit_tree(the_repository,
 						    new_branch_info->commit);
+		if (!*source_tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&new_branch_info->commit->object.oid));
 	}
 }
 

index c6357af9498..4410b55be98 100644
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die(_("unable to parse commit %s"), oid_to_hex(&oid));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));

index 781af2e206c..0723f06de7a 100644
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
 	tree = parse_tree_indirect(&current_head->object.oid);
 	if (!tree)
 		die(_("failed to unpack HEAD tree object"));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
 		exit(128); /* We've already reported the error, finish dying */

index 2d4ce5b3886..ba84d00deee 100644
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
 		if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
 			die(_("could not parse as tree '%s'"), merge_base);
 		base_tree = parse_tree_indirect(&base_oid);
+		if (!base_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&base_oid));
 		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
 			die(_("could not parse as tree '%s'"), branch1);
 		parent1_tree = parse_tree_indirect(&head_oid);
+		if (!parent1_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&head_oid));
 		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
 			die(_("could not parse as tree '%s'"), branch2);
 		parent2_tree = parse_tree_indirect(&merge_oid);
+		if (!parent2_tree)
+			die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
 
 		opt.ancestor = merge_base;
 		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);

index 8196ca9dd85..5923ed36893 100644
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	cache_tree_free(&the_index.cache_tree);
 	for (i = 0; i < nr_trees; i++) {
 		struct tree *tree = trees[i];
-		parse_tree(tree);
+		if (parse_tree(tree) < 0)
+			return 128;
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))

index 4b018d20e3b..f030f57f4e9 100644
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == MIXED || reset_type == HARD) {
 		tree = parse_tree_indirect(oid);
+		if (!tree) {
+			error(_("unable to read tree %s"), oid_to_hex(oid));
+			goto out;
+		}
 		prime_cache_tree(the_repository, the_repository->index, tree);
 	}
 

index 641427ed410..c6508b64a5c 100644
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed)
-				parse_tree(subtree);
+			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();
 

index 79d9e18f63d..534ddaf16ba 100644
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 
 	if (result->clean >= 0) {
 		result->tree = parse_tree_indirect(&working_tree_oid);
+		if (!result->tree)
+			die(_("unable to read tree %s"),
+			    oid_to_hex(&working_tree_oid));
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}

index e3beb0801b1..10d41bfd487 100644
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
 
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(desc, tree->buffer, tree->size);
 }
 

index b60925459c2..14a7325859d 100644
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
 		return -1;
 	}
 	for (i = 0; i < nr_trees; i++) {
-		parse_tree(trees[i]);
+		if (parse_tree(trees[i]) < 0) {
+			rollback_lock_file(&lock_file);
+			return -1;
+		}
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 

index 48da0adf851..a93fdbc12e3 100644
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
 	}
 
 	tree = parse_tree_indirect(oid);
+	if (!tree) {
+		ret = error(_("unable to read tree %s"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {

index d584cac8ed9..407473bab28 100644
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
 	o.show_rename_progress = 1;
 
 	head_tree = parse_tree_indirect(head);
+	if (!head_tree)
+		return error(_("unable to read tree %s"), oid_to_hex(head));
 	next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
 	base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
 

@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
 	}
 
 	tree = parse_tree_indirect(&oid);
+	if (!tree)
+		return error(_("unable to read tree %s"), oid_to_hex(&oid));
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)

gitgitgadget

From: Johannes Schindelin <[email protected]>

The first thing the parse_tree() function does is to return early if the tree has already been parsed. Therefore we do not need to guard the parse_tree() call behind a check of that flag.

As of time of writing, there are no other instances of this in Git's code bases: whenever the parsed flag guards a parse_tree() call, it guards more than just that call.

---
 cache-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c6508b64a5c..78d6ba92853 100644
@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+			if (parse_tree(subtree) < 0)
 				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();

gitgitgadget

Of course. Merge commits and single-parent commits are very different.

I suspect that the lack of "<area>:" was what triggered the comment, but this being more about the callers all over the place in the code, it may be hard to say what "area" this belongs to. If I were pressed to choose:

Subject: parse_tree*(): make callers always check return value

would probably be what I would choose, but I usually opt for going the lazy way of not labelling the area at all ;-).

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

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

Makes sense.

A complete tangent I wonder is if unit-test-minded folks have clever ideas to allow better test coverage, perhaps injecting a failure on demand to any codepath (in this case, the codepath to write the resulting tree) to simulate a situation where we fail to parse the tree.

In any case, the patch looks good, of course, and I see no need for further comments.

Thanks.

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

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

    $ git grep -h 'msgid .*unable to read tree' po | sort | uniq -c
     18 msgid "unable to read tree (%s)"

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

Just FYI for anybody reading from the sideline.

	git cat-file --unordered \
		--batch-check="%(objectname)" --batch-all-objects |
	grep -v $(cat blob2) |
	git pack-objects ...

would be a compact way to say "Give me a pack that lets me easily simulate what happens in a repository identical to the current one, if it lacked object "blob2", without having to enumerate what object we want to include in the test repository.

Again, I think in this particular case, it is easy enough to see in the "blob1 blob3 tree1 tree2 tree3" enumeration what is missing, so the way the patch is written is fine.

Thanks.

Hi Junio,

thank you so much for reviewing!

Right. I had used

  $ git grep 'unable to read tree .*%s' | sed -n 's/.*_("\([^"]*\).*/\1/p' | sort | uniq -c
       11 unable to read tree %s
        3 unable to read tree (%s)

only to realize that the 11 were the ones I added.🤦 Re-running the same command on v2.43.0 reports only the 3 parenthesized ones.

I've fixed those error messages in v4, and also added a patch to adjust the one error message that I imitated (and to mark it for translation). This patch might be slightly controversial because, in what is probably only a hypothetical scenario, users might have scripted around git bisect to parse error messages _and_ special-cased the "unable to read tree" message to be able to deal with the missing tree in a programmatical manner. I might be misjudging the likelihood for something like that, but I vividly remember the, ahem, "exciting times" I had after 7560f547e61 (treewide: correct several "up-to-date" to "up to date", 2017-08-23). If that patch is too concerning, I am open to dropping it.

Ciao,
Johannes

I recently looked into issues where git merge-tree calls returned bogus data (in one instance returning an empty tree for non-empty merge parents). By the time I had a look at the corresponding repository, the issue was no longer reproducible, but a closer look at the code combined with some manual experimenting turned up the fact that missing tree objects aren't handled as errors by git merge-tree.

While at it, I added a commit on top that tries to catch all remaining unchecked parse_tree() calls.

This patch series is based on 5f43cf5b2e4 (merge-tree: accept 3 trees as arguments, 2024-01-28) (the original tip commit of js/merge-tree-3-trees) because I introduced three unchecked parse_tree() calls in that topic.

Changes since v3:

* Aligned the translated error messages with pre-existing ones (sorry, I forgot to make that change in v2!).
* Added a new commit at the end to mark the one error message for translation which I had imitated, after making it consistent with the remaining "unable to read tree" error messages.

Changes since v2:

* Fixed the new "missing tree object" test case in t4301 that succeeded for the wrong reason.
* Adjusted the new "missing blob object" test case to avoid succeeding for the wrong reason.
* Simplified the "missing blob object" test case.

Changes since v1:

* Simplified the test case, avoiding a subshell and a pipe in the process.
* Added a patch to remove a superfluous subtree->object.parsed guard around a parse_tree(subtree) call.

Johannes Schindelin (6):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check parse_tree()'s return value
t4301: verify that merge-tree fails on missing blob objects Always check parse_tree*()'s return value
cache-tree: avoid an unnecessary check fill_tree_descriptor(): mark error message for translation

 builtin/checkout.c               | 19 ++++++++++++++++---
 builtin/clone.c                  |  3 ++-
 builtin/commit.c                 |  3 ++-
 builtin/merge-tree.c             |  6 ++++++
 builtin/read-tree.c              |  3 ++-
 builtin/reset.c                  |  4 ++++
 cache-tree.c                     |  4 ++--
 merge-ort.c                      | 16 +++++++++++-----
 merge-recursive.c                |  3 ++-
 merge.c                          |  5 ++++-
 reset.c                          |  5 +++++
 sequencer.c                      |  4 ++++
 t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++++++++++
 t/t6030-bisect-porcelain.sh      |  2 +-
 tree-walk.c                      |  2 +-
 15 files changed, 89 insertions(+), 17 deletions(-)

base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1651

Range-diff vs v3:

 1:  11b9cd8c5da = 1:  11b9cd8c5da merge-tree: fail with a non-zero exit code on missing tree objects
 2:  f01f4eb011b = 2:  f01f4eb011b merge-ort: do check `parse_tree()`'s return value
 3:  e82fdf7fbcb = 3:  e82fdf7fbcb t4301: verify that merge-tree fails on missing blob objects
 4:  9e4dc94ef03 ! 4:  5942c27f439 Always check `parse_tree*()`'s return value
     @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op
       		new_tree = repo_get_commit_tree(the_repository,
       						new_branch_info->commit);
      +		if (!new_tree)
     -+			return error(_("unable to read tree %s"),
     ++			return error(_("unable to read tree (%s)"),
      +				     oid_to_hex(&new_branch_info->commit->object.oid));
      +	}
       	if (opts->discard_changes) {
     @@ builtin/checkout.c: static void setup_new_branch_info_and_source_tree(
       		/* not a commit */
       		*source_tree = parse_tree_indirect(rev);
      +		if (!*source_tree)
     -+			die(_("unable to read tree %s"), oid_to_hex(rev));
     ++			die(_("unable to read tree (%s)"), oid_to_hex(rev));
       	} else {
       		parse_commit_or_die(new_branch_info->commit);
       		*source_tree = repo_get_commit_tree(the_repository,
       						    new_branch_info->commit);
      +		if (!*source_tree)
     -+			die(_("unable to read tree %s"),
     ++			die(_("unable to read tree (%s)"),
      +			    oid_to_hex(&new_branch_info->commit->object.oid));
       	}
       }
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
       			die(_("could not parse as tree '%s'"), merge_base);
       		base_tree = parse_tree_indirect(&base_oid);
      +		if (!base_tree)
     -+			die(_("unable to read tree %s"), oid_to_hex(&base_oid));
     ++			die(_("unable to read tree (%s)"), oid_to_hex(&base_oid));
       		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
       			die(_("could not parse as tree '%s'"), branch1);
       		parent1_tree = parse_tree_indirect(&head_oid);
      +		if (!parent1_tree)
     -+			die(_("unable to read tree %s"), oid_to_hex(&head_oid));
     ++			die(_("unable to read tree (%s)"), oid_to_hex(&head_oid));
       		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
       			die(_("could not parse as tree '%s'"), branch2);
       		parent2_tree = parse_tree_indirect(&merge_oid);
      +		if (!parent2_tree)
     -+			die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
     ++			die(_("unable to read tree (%s)"), oid_to_hex(&merge_oid));
       
       		opt.ancestor = merge_base;
       		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
     @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id
       	if (reset_type == MIXED || reset_type == HARD) {
       		tree = parse_tree_indirect(oid);
      +		if (!tree) {
     -+			error(_("unable to read tree %s"), oid_to_hex(oid));
     ++			error(_("unable to read tree (%s)"), oid_to_hex(oid));
      +			goto out;
      +		}
       		prime_cache_tree(the_repository, the_repository->index, tree);
     @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o
       	if (result->clean >= 0) {
       		result->tree = parse_tree_indirect(&working_tree_oid);
      +		if (!result->tree)
     -+			die(_("unable to read tree %s"),
     ++			die(_("unable to read tree (%s)"),
      +			    oid_to_hex(&working_tree_oid));
       		/* existence of conflicted entries implies unclean */
       		result->clean &= strmap_empty(&opt->priv->conflicted);
     @@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts
       
       	tree = parse_tree_indirect(oid);
      +	if (!tree) {
     -+		ret = error(_("unable to read tree %s"), oid_to_hex(oid));
     ++		ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
      +		goto leave_reset_head;
      +	}
      +
     @@ sequencer.c: static int do_recursive_merge(struct repository *r,
       
       	head_tree = parse_tree_indirect(head);
      +	if (!head_tree)
     -+		return error(_("unable to read tree %s"), oid_to_hex(head));
     ++		return error(_("unable to read tree (%s)"), oid_to_hex(head));
       	next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
       	base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
       
     @@ sequencer.c: static int do_reset(struct repository *r,
       
       	tree = parse_tree_indirect(&oid);
      +	if (!tree)
     -+		return error(_("unable to read tree %s"), oid_to_hex(&oid));
     ++		return error(_("unable to read tree (%s)"), oid_to_hex(&oid));
       	prime_cache_tree(r, r->index, tree);
       
       	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
 5:  91dc4ccd04e = 5:  7e5e84a4e7c cache-tree: avoid an unnecessary check
 -:  ----------- > 6:  ee2fcee5a10 fill_tree_descriptor(): mark error message for translation

gitgitgadget

From: Johannes Schindelin <[email protected]>

When git merge-tree encounters a missing tree object, it should error out and not continue quietly as if nothing had happened.

However, as of time of writing, git merge-tree _does_ continue, and then offers the empty tree as result.

Let's fix this.

---
 merge-ort.c                      |  7 ++++---
 t/t4301-merge-tree-write-tree.sh | 11 +++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

index 6491070d965..c37fc035f13 100644
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	parse_tree(merge_base);
-	parse_tree(side1);
-	parse_tree(side2);
+	if (parse_tree(merge_base) < 0 ||
+	    parse_tree(side1) < 0 ||
+	    parse_tree(side2) < 0)
+		return -1;
 	init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
 	init_tree_desc(t + 1, side1->buffer, side1->size);
 	init_tree_desc(t + 2, side2->buffer, side2->size);

index 7d0fa74da74..908c9b540c8 100755
@@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '
 	test_cmp with-commits with-trees
 '
 
+test_expect_success 'error out on missing tree objects' '
+	git init --bare missing-tree.git &&
+	git rev-list side3 >list &&
+	git rev-parse side3^: >>list &&
+	git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
+	side3=$(git rev-parse side3) &&
+	test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&
+	test_grep "Could not read $(git rev-parse $side3:)" err &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

The previous commit fixed a bug where a missing tree was reported, but not treated as an error.

This patch addresses the same issue for the remaining two callers of parse_tree().

This change is not accompanied by a regression test because the code in question is only reached at the checkout stage, i.e. after the merge has happened (and therefore the tree objects could only be missing if the disk had gone bad in that short time window, or something similarly tricky to recreate in the test suite).

---
 merge-ort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index c37fc035f13..79d9e18f63d 100644
@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	parse_tree(prev);
+	if (parse_tree(prev) < 0)
+		return -1;
 	init_tree_desc(&trees[0], prev->buffer, prev->size);
-	parse_tree(next);
+	if (parse_tree(next) < 0)
+		return -1;
 	init_tree_desc(&trees[1], next->buffer, next->size);
 
 	ret = unpack_trees(2, trees, &unpack_opts);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We just fixed a problem where merge-tree would not fail on missing tree objects. Let's ensure that that problem does not occur with blob objects (and won't, in the future, either).

---
 t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index 908c9b540c8..d4463a45706 100755
@@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'error out on missing blob objects' '
+	echo 1 | git hash-object -w --stdin >blob1 &&
+	echo 2 | git hash-object -w --stdin >blob2 &&
+	echo 3 | git hash-object -w --stdin >blob3 &&
+	printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
+	printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
+	printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
+	git init --bare missing-blob.git &&
+	cat blob1 blob3 tree1 tree2 tree3 |
+	git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+	test_must_fail git --git-dir=missing-blob.git >actual 2>err \
+		merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
+	test_grep "unable to read blob object $(cat blob2)" err &&
+	test_must_be_empty actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

Otherwise we may easily run into serious crashes: For example, if we run init_tree_desc() directly after a failed parse_tree(), we are accessing uninitialized data or trying to dereference NULL.

Note that the parse_tree() function already takes care of showing an error message. The parse_tree_indirectly() and repo_get_commit_tree() functions do not, therefore those latter call sites need to show a useful error message while the former do not.

---
 builtin/checkout.c   | 19 ++++++++++++++++---
 builtin/clone.c      |  3 ++-
 builtin/commit.c     |  3 ++-
 builtin/merge-tree.c |  6 ++++++
 builtin/read-tree.c  |  3 ++-
 builtin/reset.c      |  4 ++++
 cache-tree.c         |  4 ++--
 merge-ort.c          |  3 +++
 merge-recursive.c    |  3 ++-
 merge.c              |  5 ++++-
 reset.c              |  5 +++++
 sequencer.c          |  4 ++++
 12 files changed, 52 insertions(+), 10 deletions(-)

index f02434bc155..9ab0901d629 100644
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	init_checkout_metadata(&opts.meta, info->refname,
 			       info->commit ? &info->commit->object.oid : null_oid(),
 			       NULL);
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		return 128;
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
 	case -2:

@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		if (new_branch_info->commit)
 			BUG("'switch --orphan' should never accept a commit as starting point");
 		new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
-	} else
+		if (!new_tree)
+			BUG("unable to read empty tree");
+	} else {
 		new_tree = repo_get_commit_tree(the_repository,
 						new_branch_info->commit);
+		if (!new_tree)
+			return error(_("unable to read tree (%s)"),
+				     oid_to_hex(&new_branch_info->commit->object.oid));
+	}
 	if (opts->discard_changes) {
 		ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
 		if (ret)

@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				oid_to_hex(old_commit_oid));
 
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
-		parse_tree(new_tree);
+		if (parse_tree(new_tree) < 0)
+			exit(128);
 		tree = new_tree;
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 

@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
 	if (!new_branch_info->commit) {
 		/* not a commit */
 		*source_tree = parse_tree_indirect(rev);
+		if (!*source_tree)
+			die(_("unable to read tree (%s)"), oid_to_hex(rev));
 	} else {
 		parse_commit_or_die(new_branch_info->commit);
 		*source_tree = repo_get_commit_tree(the_repository,
 						    new_branch_info->commit);
+		if (!*source_tree)
+			die(_("unable to read tree (%s)"),
+			    oid_to_hex(&new_branch_info->commit->object.oid));
 	}
 }
 

index c6357af9498..4410b55be98 100644
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die(_("unable to parse commit %s"), oid_to_hex(&oid));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));

index 781af2e206c..0723f06de7a 100644
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
 	tree = parse_tree_indirect(&current_head->object.oid);
 	if (!tree)
 		die(_("failed to unpack HEAD tree object"));
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
 		exit(128); /* We've already reported the error, finish dying */

index 2d4ce5b3886..3492a575a6c 100644
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
 		if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
 			die(_("could not parse as tree '%s'"), merge_base);
 		base_tree = parse_tree_indirect(&base_oid);
+		if (!base_tree)
+			die(_("unable to read tree (%s)"), oid_to_hex(&base_oid));
 		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
 			die(_("could not parse as tree '%s'"), branch1);
 		parent1_tree = parse_tree_indirect(&head_oid);
+		if (!parent1_tree)
+			die(_("unable to read tree (%s)"), oid_to_hex(&head_oid));
 		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
 			die(_("could not parse as tree '%s'"), branch2);
 		parent2_tree = parse_tree_indirect(&merge_oid);
+		if (!parent2_tree)
+			die(_("unable to read tree (%s)"), oid_to_hex(&merge_oid));
 
 		opt.ancestor = merge_base;
 		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);

index 8196ca9dd85..5923ed36893 100644
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	cache_tree_free(&the_index.cache_tree);
 	for (i = 0; i < nr_trees; i++) {
 		struct tree *tree = trees[i];
-		parse_tree(tree);
+		if (parse_tree(tree) < 0)
+			return 128;
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))

index 4b018d20e3b..fd36fc5bd95 100644
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == MIXED || reset_type == HARD) {
 		tree = parse_tree_indirect(oid);
+		if (!tree) {
+			error(_("unable to read tree (%s)"), oid_to_hex(oid));
+			goto out;
+		}
 		prime_cache_tree(the_repository, the_repository->index, tree);
 	}
 

index 641427ed410..c6508b64a5c 100644
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed)
-				parse_tree(subtree);
+			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();
 

index 79d9e18f63d..910ba38ff05 100644
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 
 	if (result->clean >= 0) {
 		result->tree = parse_tree_indirect(&working_tree_oid);
+		if (!result->tree)
+			die(_("unable to read tree (%s)"),
+			    oid_to_hex(&working_tree_oid));
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}

index e3beb0801b1..10d41bfd487 100644
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
 
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
-	parse_tree(tree);
+	if (parse_tree(tree) < 0)
+		exit(128);
 	init_tree_desc(desc, tree->buffer, tree->size);
 }
 

index b60925459c2..14a7325859d 100644
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
 		return -1;
 	}
 	for (i = 0; i < nr_trees; i++) {
-		parse_tree(trees[i]);
+		if (parse_tree(trees[i]) < 0) {
+			rollback_lock_file(&lock_file);
+			return -1;
+		}
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 

index 48da0adf851..080bcb6d656 100644
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
 	}
 
 	tree = parse_tree_indirect(oid);
+	if (!tree) {
+		ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
+		goto leave_reset_head;
+	}
+
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {

index d584cac8ed9..33d12b2ffd1 100644
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
 	o.show_rename_progress = 1;
 
 	head_tree = parse_tree_indirect(head);
+	if (!head_tree)
+		return error(_("unable to read tree (%s)"), oid_to_hex(head));
 	next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
 	base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
 

@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
 	}
 
 	tree = parse_tree_indirect(&oid);
+	if (!tree)
+		return error(_("unable to read tree (%s)"), oid_to_hex(&oid));
 	prime_cache_tree(r, r->index, tree);
 
 	if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)

gitgitgadget

From: Johannes Schindelin <[email protected]>

The first thing the parse_tree() function does is to return early if the tree has already been parsed. Therefore we do not need to guard the parse_tree() call behind a check of that flag.

As of time of writing, there are no other instances of this in Git's code bases: whenever the parsed flag guards a parse_tree() call, it guards more than just that call.

---
 cache-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c6508b64a5c..78d6ba92853 100644
@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(r, &entry.oid);
 
-			if (!subtree->object.parsed && parse_tree(subtree) < 0)
+			if (parse_tree(subtree) < 0)
 				exit(128);
 			sub = cache_tree_sub(it, entry.path);
 			sub->cache_tree = cache_tree();

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is an error message in that function to report a missing tree; In contrast to three other, similar error messages, it is not marked for translation yet.

Mark it for translation, and while at it, make the error message consistent with the others by enclosing the SHA in parentheses.

This requires a change to t6030 which expects the previous format of the commit message. Theoretically, this could present problems with existing scripts that use git bisect and parse its output (because Git does not provide other means for callers to discern between error conditions). However, this is unlikely to matter in practice because the most common course of action to deal with fatal corruptions is to report the error message to the user and exit, rather than trying to do something with the reported SHA of the missing tree.

---
 t/t6030-bisect-porcelain.sh | 2 +-
 tree-walk.c                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 2a5b7d8379c..58f3d9c675e 100755
@@ -872,7 +872,7 @@ test_expect_success 'broken branch creation' '
 
 echo "" > expected.ok
 cat > expected.missing-tree.default <<EOF
-fatal: unable to read tree $deleted
+fatal: unable to read tree ($deleted)
 EOF
 
 test_expect_success 'bisect fails if tree is broken on start commit' '

index b517792ba23..690fa6569bd 100644
@@ -100,7 +100,7 @@ void *fill_tree_descriptor(struct repository *r,
 	if (oid) {
 		buf = read_object_with_reference(r, oid, OBJ_TREE, &size, NULL);
 		if (!buf)
-			die("unable to read tree %s", oid_to_hex(oid));
+			die(_("unable to read tree (%s)"), oid_to_hex(oid));
 	}
 	init_tree_desc(desc, buf, size);
 	return buf;

gitgitgadget

I think we probably should discuss what format is the easiest to understand and most logical to readers, and unify to such a format, not necessarily the current majority, in the longer term. But for now, let's pick one that costs less to unify to.

We may also want to cast a wider net to make things consistent. For example, we learn that the parenthesized one is not necessarily more prevalent in a larger picture.

Let me annotate the output from this command:

    $ git grep -h -E \
      -e '(unable to|not) (read|find|acccess) (blob|tree|commit|tag) .*%s' master po/ |
      sort -u

1. msgid "cannot find commit %s (%s)"

The first one is a textual refname, the next one is oid-to-hex.

2. msgid "cannot read blob %s for path %s"

The first one is oid-to-hex, the other is a pathname.

3. msgid "could not find commit %s"

oid-to-hex (in commit-graph.c)

4. msgid "could not read commit message of %s"

oid-to-hex (in sequencer.c)

5. msgid "could not read commit message: %s"

This is irrelevant to the topic, as %s is for strerror().

6. msgid "unable to access commit %s"

oid-to-hex (in builtin/pull.c)

7. msgid "unable to read commit message from '%s'"

This message is in po/ but it seems that it no longer is used anywhere in the source.

8. msgid "unable to read tree %s"
9. msgid "unable to read tree (%s)"

We know about these two already.

We seem to just use unadorned %s for many messages when talking about commit objects, some are inside a pair of 'single quotes', When we are giving a long hexadecimal string, especially without doing any abbreviation, I personally think it is a waste to enclose it in any punctuation pair, so if I were to vote today, I would probably support standardizing on "tree %s", "blob %s", etc., but I think that is just a personal preference (not even a "taste" thing).

In any case, all of the above is clearly outside the scope of this series.

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

Looking good. Will replace. Thanks.