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
