[v4,0/6] merge-tree: handle missing objects correctly

Message

Philippe Blain via GitGitGadget Feb. 23, 2024, 8:34 a.m. UTC

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

Comments

Junio C Hamano Feb. 23, 2024, 6:23 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

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

Looking good.  Will replace.  Thanks.