From 5f43cf5b2e4b68386d3774bce880b0f74d801635 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 28 Jan 2024 20:34:22 +0000 Subject: [PATCH 1/7] merge-tree: accept 3 trees as arguments When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph. This commit is best viewed with `--color-moved --color-moved-ws=allow-indentation-change`. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/git-merge-tree.txt | 5 +++- builtin/merge-tree.c | 42 +++++++++++++++++++------------- t/t4301-merge-tree-write-tree.sh | 6 +++++ 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt index b50acace3b..dd388fa21d 100644 --- a/Documentation/git-merge-tree.txt +++ b/Documentation/git-merge-tree.txt @@ -64,10 +64,13 @@ OPTIONS share no common history. This flag can be given to override that check and make the merge proceed anyway. ---merge-base=:: +--merge-base=:: Instead of finding the merge-bases for and , specify a merge-base for the merge, and specifying multiple bases is currently not supported. This option is incompatible with `--stdin`. ++ +As the merge-base is provided directly, and do not need +to specify commits; trees are enough. [[OUTPUT]] OUTPUT diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index a35e0452d6..2d4ce5b388 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -430,35 +430,43 @@ static int real_merge(struct merge_tree_options *o, struct merge_options opt; copy_merge_options(&opt, &o->merge_options); - parent1 = get_merge_parent(branch1); - if (!parent1) - help_unknown_ref(branch1, "merge-tree", - _("not something we can merge")); - - parent2 = get_merge_parent(branch2); - if (!parent2) - help_unknown_ref(branch2, "merge-tree", - _("not something we can merge")); - opt.show_rename_progress = 0; opt.branch1 = branch1; opt.branch2 = branch2; if (merge_base) { - struct commit *base_commit; struct tree *base_tree, *parent1_tree, *parent2_tree; - base_commit = lookup_commit_reference_by_name(merge_base); - if (!base_commit) - die(_("could not lookup commit '%s'"), merge_base); + /* + * We actually only need the trees because we already + * have a merge base. + */ + struct object_id base_oid, head_oid, merge_oid; + + 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 (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 (repo_get_oid_treeish(the_repository, branch2, &merge_oid)) + die(_("could not parse as tree '%s'"), branch2); + parent2_tree = parse_tree_indirect(&merge_oid); opt.ancestor = merge_base; - base_tree = repo_get_commit_tree(the_repository, base_commit); - parent1_tree = repo_get_commit_tree(the_repository, parent1); - parent2_tree = repo_get_commit_tree(the_repository, parent2); merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result); } else { + parent1 = get_merge_parent(branch1); + if (!parent1) + help_unknown_ref(branch1, "merge-tree", + _("not something we can merge")); + + parent2 = get_merge_parent(branch2); + if (!parent2) + help_unknown_ref(branch2, "merge-tree", + _("not something we can merge")); + /* * Get the merge bases, in reverse order; see comment above * merge_incore_recursive in merge-ort.h diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index b2c8a43fce..7d0fa74da7 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh @@ -945,4 +945,10 @@ test_expect_success 'check the input format when --stdin is passed' ' test_cmp expect actual ' +test_expect_success '--merge-base with tree OIDs' ' + git merge-tree --merge-base=side1^ side1 side3 >with-commits && + git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees && + test_cmp with-commits with-trees +' + test_done From d4bf19308b81653cfb0d7b54489010dc1e0113b0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:20 +0000 Subject: [PATCH 2/7] merge-tree: fail with a non-zero exit code on missing tree objects 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. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- merge-ort.c | 7 ++++--- t/t4301-merge-tree-write-tree.sh | 11 +++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 6491070d96..c37fc035f1 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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); diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index 7d0fa74da7..908c9b540c 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh @@ -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 actual 2>err && + test_grep "Could not read $(git rev-parse $side3:)" err && + test_must_be_empty actual +' + test_done From f30e6c32d8a27760915922b1ddf76f95f11539bb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:21 +0000 Subject: [PATCH 3/7] merge-ort: do check `parse_tree()`'s return value 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). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- merge-ort.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index c37fc035f1..79d9e18f63 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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); From 98c6d16d6746059dc1e1183f8f8366eef2a41eff Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:22 +0000 Subject: [PATCH 4/7] t4301: verify that merge-tree fails on missing blob objects 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). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index 908c9b540c..d4463a4570 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh @@ -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 From aa9f618909d30e3f0c7181243e89a81220507e6e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:23 +0000 Subject: [PATCH 5/7] Always check `parse_tree*()`'s return value 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. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- 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(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index f02434bc15..9ab0901d62 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -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)); } } diff --git a/builtin/clone.c b/builtin/clone.c index c6357af949..4410b55be9 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -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")); diff --git a/builtin/commit.c b/builtin/commit.c index 781af2e206..0723f06de7 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head) tree = parse_tree_indirect(¤t_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 */ diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 2d4ce5b388..3492a575a6 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -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); diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 8196ca9dd8..5923ed3689 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -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)) diff --git a/builtin/reset.c b/builtin/reset.c index 4b018d20e3..fd36fc5bd9 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -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); } diff --git a/cache-tree.c b/cache-tree.c index 641427ed41..c6508b64a5 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -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(); diff --git a/merge-ort.c b/merge-ort.c index 79d9e18f63..910ba38ff0 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4983,6 +4983,9 @@ redo: 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); } diff --git a/merge-recursive.c b/merge-recursive.c index e3beb0801b..10d41bfd48 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -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); } diff --git a/merge.c b/merge.c index b60925459c..14a7325859 100644 --- a/merge.c +++ b/merge.c @@ -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); } diff --git a/reset.c b/reset.c index 48da0adf85..080bcb6d65 100644 --- a/reset.c +++ b/reset.c @@ -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) { diff --git a/sequencer.c b/sequencer.c index d584cac8ed..33d12b2ffd 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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) From 5aca024a74e900bd9bc2c14a8e99494063ea4cc5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:24 +0000 Subject: [PATCH 6/7] cache-tree: avoid an unnecessary check 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. Suggested-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- cache-tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache-tree.c b/cache-tree.c index c6508b64a5..78d6ba9285 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -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(); From 342990c7aaef5ac645e89101cb84569caf64baf4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 23 Feb 2024 08:34:25 +0000 Subject: [PATCH 7/7] fill_tree_descriptor(): mark error message for translation 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. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- t/t6030-bisect-porcelain.sh | 2 +- tree-walk.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 2a5b7d8379..58f3d9c675 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -872,7 +872,7 @@ test_expect_success 'broken branch creation' ' echo "" > expected.ok cat > expected.missing-tree.default <