{"thread":{"id":"60855","subject":"[PATCH 0/4] merge-tree: handle missing objects correctly","startedAt":"2024-02-06T09:49:45Z","lastAt":"2024-02-23T18:23:25Z","messageCount":48,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Eric Sunshine","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"488057","messageId":"pull.1651.git.1707212981.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":null,"subject":"[PATCH 0/4] merge-tree: handle missing objects correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-06T09:49:37Z","receivedAt":"2024-02-06T09:49:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently looked into issues where git merge-tree calls returned bogus data\n(in one instance returning an empty tree for non-empty merge parents). By\nthe time I had a look at the corresponding repository, the issue was no\nlonger reproducible, but a closer look at the code combined with some manual\nexperimenting turned up the fact that missing tree objects aren't handled as\nerrors by git merge-tree.\n\nWhile at it, I added a commit on top that tries to catch all remaining\nunchecked parse_tree() calls.\n\nThis patch series is based on js/merge-tree-3-trees because I introduced\nthree unchecked parse_tree() calls in that topic branch.\n\nJohannes Schindelin (4):\n merge-tree: fail with a non-zero exit code on missing tree objects\n merge-ort: do check `parse_tree()`'s return value\n t4301: verify that merge-tree fails on missing blob objects\n Always check `parse_tree*()`'s return value\n\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 16 +++++++++++-----\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++\n 13 files changed, 84 insertions(+), 15 deletions(-)\n\n\nbase-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1651\n-- \ngitgitgadget\n"},{"id":"488058","messageId":"a3e8ae8611484f95df15490ea8f1abcec8f4cb36.1707212981.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"[PATCH 1/4] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-06T09:49:38Z","receivedAt":"2024-02-06T09:49:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `git merge-tree` encounters a missing tree object, it should error\nout and not continue quietly as if nothing had happened.\n\nHowever, as of time of writing, `git merge-tree` _does_ continue, and\nthen offers the empty tree as result.\n\nLet's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 7 ++++---\n t/t4301-merge-tree-write-tree.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6491070d965..c37fc035f13 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,\n \tinfo.data = opt;\n \tinfo.show_all_errors = 1;\n \n-\tparse_tree(merge_base);\n-\tparse_tree(side1);\n-\tparse_tree(side2);\n+\tif (parse_tree(merge_base) < 0 ||\n+\t parse_tree(side1) < 0 ||\n+\t parse_tree(side2) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(t + 0, merge_base->buffer, merge_base->size);\n \tinit_tree_desc(t + 1, side1->buffer, side1->size);\n \tinit_tree_desc(t + 2, side2->buffer, side2->size);\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 7d0fa74da74..4ea1b74445d 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -950,5 +950,15 @@ test_expect_success '--merge-base with tree OIDs' '\n \tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&\n \ttest_cmp with-commits with-trees\n '\n+test_expect_success 'error out on missing tree objects' '\n+\tgit init --bare missing-tree.git &&\n+\t(\n+\t\tgit rev-list side3 &&\n+\t\tgit rev-parse side3^:\n+\t) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&\n+\tside3=$(git rev-parse side3) &&\n+\ttest_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&\n+\ttest_must_be_empty actual\n+'\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"488059","messageId":"3e5b787fc03493b1feaada7ef0011ca2d33f8820.1707212981.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"[PATCH 2/4] merge-ort: do check `parse_tree()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-06T09:49:39Z","receivedAt":"2024-02-06T09:49:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe previous commit fixed a bug where a missing tree was reported, but\nnot treated as an error.\n\nThis patch addresses the same issue for the remaining two callers of\n`parse_tree()`.\n\nThis change is not accompanied by a regression test because the code in\nquestion is only reached at the `checkout` stage, i.e. after the merge\nhas happened (and therefore the tree objects could only be missing if\nthe disk had gone bad in that short time window, or something similarly\ntricky to recreate in the test suite).\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c37fc035f13..79d9e18f63d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,\n \tunpack_opts.verbose_update = (opt->verbosity > 2);\n \tunpack_opts.fn = twoway_merge;\n \tunpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */\n-\tparse_tree(prev);\n+\tif (parse_tree(prev) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n-\tparse_tree(next);\n+\tif (parse_tree(next) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[1], next->buffer, next->size);\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n-- \ngitgitgadget\n\n"},{"id":"488061","messageId":"85d3e6728714606c3eac42a561ae89b61457647b.1707212981.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"[PATCH 3/4] t4301: verify that merge-tree fails on missing blob objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-06T09:49:40Z","receivedAt":"2024-02-06T09:49:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a problem where `merge-tree` would not fail on missing\ntree objects. Let's ensure that that problem does not occur with blob\nobjects (and won't, in the future, either).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4301-merge-tree-write-tree.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 4ea1b74445d..86807a57d4d 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'error out on missing blob objects' '\n+\tseq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n+\tseq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n+\tseq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n+\ttree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n+\ttree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n+\ttree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n+\tgit init --bare missing-blob.git &&\n+\ttest_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |\n+\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n+\ttest_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"488060","messageId":"93abd7000b81bbcd7f5422715b4bbb60c69a7cde.1707212981.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"[PATCH 4/4] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-06T09:49:41Z","receivedAt":"2024-02-06T09:49:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOtherwise we may easily run into serious crashes: For example, if we run\n`init_tree_desc()` directly after a failed `parse_tree()`, we are\naccessing uninitialized data or trying to dereference `NULL`.\n\nNote that the `parse_tree()` function already takes care of showing an\nerror message. The `parse_tree_indirectly()` and\n`repo_get_commit_tree()` functions do not, therefore those latter call\nsites need to show a useful error message while the former do not.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 3 +++\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n 12 files changed, 52 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f02434bc155..84108ec3635 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \tinit_checkout_metadata(&opts.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n \t\t\t NULL);\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\treturn 128;\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tif (new_branch_info->commit)\n \t\t\tBUG(\"'switch --orphan' should never accept a commit as starting point\");\n \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n-\t} else\n+\t\tif (!new_tree)\n+\t\t\tBUG(\"unable to read empty tree\");\n+\t} else {\n \t\tnew_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\tnew_branch_info->commit);\n+\t\tif (!new_tree)\n+\t\t\treturn error(_(\"unable to read tree %s\"),\n+\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n+\t}\n \tif (opts->discard_changes) {\n \t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n \t\tif (ret)\n@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\toid_to_hex(old_commit_oid));\n \n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n-\t\tparse_tree(new_tree);\n+\t\tif (parse_tree(new_tree) < 0)\n+\t\t\texit(128);\n \t\ttree = new_tree;\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!new_branch_info->commit) {\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(rev));\n \t} else {\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t\t*source_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t new_branch_info->commit);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n \t}\n }\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af9498..4410b55be98 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)\n \ttree = parse_tree_indirect(&oid);\n \tif (!tree)\n \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 781af2e206c..0723f06de7a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)\n \ttree = parse_tree_indirect(¤t_head->object.oid);\n \tif (!tree)\n \t\tdie(_(\"failed to unpack HEAD tree object\"));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n \t\texit(128); /* We've already reported the error, finish dying */\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 2d4ce5b3886..ba84d00deee 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,\n \t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n \t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (!base_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&base_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n \t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (!parent1_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&head_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n \t\tparent2_tree = parse_tree_indirect(&merge_oid);\n+\t\tif (!parent2_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&merge_oid));\n \n \t\topt.ancestor = merge_base;\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 8196ca9dd85..5923ed36893 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tcache_tree_free(&the_index.cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\n-\t\tparse_tree(tree);\n+\t\tif (parse_tree(tree) < 0)\n+\t\t\treturn 128;\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4b018d20e3b..f030f57f4e9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n+\t\tif (!tree) {\n+\t\t\terror(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\t\tgoto out;\n+\t\t}\n \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n \t}\n \ndiff --git a/cache-tree.c b/cache-tree.c\nindex 641427ed410..c6508b64a5c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed)\n-\t\t\t\tparse_tree(subtree);\n+\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 79d9e18f63d..534ddaf16ba 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \n \tif (result->clean >= 0) {\n \t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\tif (!result->tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&working_tree_oid));\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e3beb0801b1..10d41bfd487 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)\n \n static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n {\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(desc, tree->buffer, tree->size);\n }\n \ndiff --git a/merge.c b/merge.c\nindex b60925459c2..14a7325859d 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,\n \t\treturn -1;\n \t}\n \tfor (i = 0; i < nr_trees; i++) {\n-\t\tparse_tree(trees[i]);\n+\t\tif (parse_tree(trees[i]) < 0) {\n+\t\t\trollback_lock_file(&lock_file);\n+\t\t\treturn -1;\n+\t\t}\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \ndiff --git a/reset.c b/reset.c\nindex 48da0adf851..a93fdbc12e3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \t}\n \n \ttree = parse_tree_indirect(oid);\n+\tif (!tree) {\n+\t\tret = error(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..407473bab28 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,\n \to.show_rename_progress = 1;\n \n \thead_tree = parse_tree_indirect(head);\n+\tif (!head_tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(head));\n \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n \n@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(&oid));\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n-- \ngitgitgadget\n"},{"id":"488092","messageId":"xmqq7cjhguvk.fsf@gitster.g","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] merge-tree: handle missing objects correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T21:12:15Z","receivedAt":"2024-02-06T21:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> I recently looked into issues where git merge-tree calls returned bogus data\n> (in one instance returning an empty tree for non-empty merge parents). By\n> the time I had a look at the corresponding repository, the issue was no\n> longer reproducible, but a closer look at the code combined with some manual\n> experimenting turned up the fact that missing tree objects aren't handled as\n> errors by git merge-tree.\n>\n> While at it, I added a commit on top that tries to catch all remaining\n> unchecked parse_tree() calls.\n>\n> This patch series is based on js/merge-tree-3-trees because I introduced\n> three unchecked parse_tree() calls in that topic branch.\n\nThanks. All the added checks looked reasonable to me.\n\nWill queue.\n\n>\n> Johannes Schindelin (4):\n> merge-tree: fail with a non-zero exit code on missing tree objects\n> merge-ort: do check `parse_tree()`'s return value\n> t4301: verify that merge-tree fails on missing blob objects\n> Always check `parse_tree*()`'s return value\n>\n> builtin/checkout.c | 19 ++++++++++++++++---\n> builtin/clone.c | 3 ++-\n> builtin/commit.c | 3 ++-\n> builtin/merge-tree.c | 6 ++++++\n> builtin/read-tree.c | 3 ++-\n> builtin/reset.c | 4 ++++\n> cache-tree.c | 4 ++--\n> merge-ort.c | 16 +++++++++++-----\n> merge-recursive.c | 3 ++-\n> merge.c | 5 ++++-\n> reset.c | 5 +++++\n> sequencer.c | 4 ++++\n> t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++\n> 13 files changed, 84 insertions(+), 15 deletions(-)\n>\n>\n> base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1651\n"},{"id":"488104","messageId":"xmqqh6ilfdqv.fsf@gitster.g","threadId":"60855","inReplyTo":"93abd7000b81bbcd7f5422715b4bbb60c69a7cde.1707212981.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] Always check `parse_tree*()`'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T22:07:36Z","receivedAt":"2024-02-06T22:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> @@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n> \tinit_checkout_metadata(&opts.meta, info->refname,\n> \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n> \t\t\t NULL);\n> -\tparse_tree(tree);\n> +\tif (parse_tree(tree) < 0)\n> +\t\treturn 128;\n> \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n\nAnother thing that caught my attention during today's integration\ncycle was that this \"parse the tree object in variable tree and then\ncall init_tree_desc on it\" is a recurring pattern. After the dust\nsettles, we might want to have a helper function perhaps like\n\n int init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n {\n\tif (parse_tree(tree))\n\t\treturn -1;\n\tinit_tree_desc(desc, tree->buffer, tree->size);\n }\n\nIt was a bit irritating as another topic in flight changes the\nfunction signature for init_tree_desc(), causing a textual conflict\nthat could be easily avoided.\n\n"},{"id":"488123","messageId":"ZcM0ZhXqCv17c5bN@tanuki","threadId":"60855","inReplyTo":"a3e8ae8611484f95df15490ea8f1abcec8f4cb36.1707212981.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-07T07:42:30Z","receivedAt":"2024-02-07T07:42:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 06, 2024 at 09:49:38AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When `git merge-tree` encounters a missing tree object, it should error\n> out and not continue quietly as if nothing had happened.\n> \n> However, as of time of writing, `git merge-tree` _does_ continue, and\n> then offers the empty tree as result.\n> \n> Let's fix this.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort.c | 7 ++++---\n> t/t4301-merge-tree-write-tree.sh | 10 ++++++++++\n> 2 files changed, 14 insertions(+), 3 deletions(-)\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index 6491070d965..c37fc035f13 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,\n> \tinfo.data = opt;\n> \tinfo.show_all_errors = 1;\n> \n> -\tparse_tree(merge_base);\n> -\tparse_tree(side1);\n> -\tparse_tree(side2);\n> +\tif (parse_tree(merge_base) < 0 ||\n> +\t parse_tree(side1) < 0 ||\n> +\t parse_tree(side2) < 0)\n> +\t\treturn -1;\n\nI was wondering whether we also want to print an error in this case. But\n`parse_tree()` calls `parse_tree_gently(tree, 0)`, where the second\nparameter instructs the function to print an error message when the tree\nis missing.\n\n> \tinit_tree_desc(t + 0, merge_base->buffer, merge_base->size);\n> \tinit_tree_desc(t + 1, side1->buffer, side1->size);\n> \tinit_tree_desc(t + 2, side2->buffer, side2->size);\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 7d0fa74da74..4ea1b74445d 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -950,5 +950,15 @@ test_expect_success '--merge-base with tree OIDs' '\n> \tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&\n> \ttest_cmp with-commits with-trees\n> '\n\nNit: missing newline.\n\n> +test_expect_success 'error out on missing tree objects' '\n> +\tgit init --bare missing-tree.git &&\n> +\t(\n> +\t\tgit rev-list side3 &&\n> +\t\tgit rev-parse side3^:\n> +\t) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&\n> +\tside3=$(git rev-parse side3) &&\n> +\ttest_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&\n> +\ttest_must_be_empty actual\n> +'\n\nCan't we avoid the subshell by using curly braces to pipe the outputs\n\nPatrick\n"},{"id":"488124","messageId":"ZcM0blqMPppLn2oB@tanuki","threadId":"60855","inReplyTo":"93abd7000b81bbcd7f5422715b4bbb60c69a7cde.1707212981.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] Always check `parse_tree*()`'s return value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-07T07:42:38Z","receivedAt":"2024-02-07T07:42:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 06, 2024 at 09:49:41AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n[snip]\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 641427ed410..c6508b64a5c 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n> \t\t\tstruct cache_tree_sub *sub;\n> \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n> \n> -\t\t\tif (!subtree->object.parsed)\n> -\t\t\t\tparse_tree(subtree);\n> +\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n> +\t\t\t\texit(128);\n\nIt's somewhat weird that we have the `subtree->object.parsed` check in\nthe first place, because the first thing that `parse_tree_gently()` does\nis to check that flag and immediately return when the tree is parsed\nalready. It's a preexisting issue though, so this is fine.\n\nPatrick\n"},{"id":"488125","messageId":"ZcM0dvzqJy_PuEEc@tanuki","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] merge-tree: handle missing objects correctly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-07T07:42:46Z","receivedAt":"2024-02-07T07:42:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 06, 2024 at 09:49:37AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> I recently looked into issues where git merge-tree calls returned bogus data\n> (in one instance returning an empty tree for non-empty merge parents). By\n> the time I had a look at the corresponding repository, the issue was no\n> longer reproducible, but a closer look at the code combined with some manual\n> experimenting turned up the fact that missing tree objects aren't handled as\n> errors by git merge-tree.\n> \n> While at it, I added a commit on top that tries to catch all remaining\n> unchecked parse_tree() calls.\n> \n> This patch series is based on js/merge-tree-3-trees because I introduced\n> three unchecked parse_tree() calls in that topic branch.\n\nThese patches all look good to me, thanks! I have two nits for the first\npatch, but I don't really think that those are worth a reroll.\n\nPatrick\n"},{"id":"488149","messageId":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.git.1707212981.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] merge-tree: handle missing objects correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:36Z","receivedAt":"2024-02-07T16:47:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently looked into issues where git merge-tree calls returned bogus data\n(in one instance returning an empty tree for non-empty merge parents). By\nthe time I had a look at the corresponding repository, the issue was no\nlonger reproducible, but a closer look at the code combined with some manual\nexperimenting turned up the fact that missing tree objects aren't handled as\nerrors by git merge-tree.\n\nWhile at it, I added a commit on top that tries to catch all remaining\nunchecked parse_tree() calls.\n\nThis patch series is based on js/merge-tree-3-trees because I introduced\nthree unchecked parse_tree() calls in that topic branch.\n\nChanges since v1:\n\n * Simplified the test case, avoiding a subshell and a pipe in the process.\n * Added a patch to remove a superfluous subtree->object.parsed guard around\n a parse_tree(subtree) call.\n\nJohannes Schindelin (5):\n merge-tree: fail with a non-zero exit code on missing tree objects\n merge-ort: do check `parse_tree()`'s return value\n t4301: verify that merge-tree fails on missing blob objects\n Always check `parse_tree*()`'s return value\n cache-tree: avoid an unnecessary check\n\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 16 +++++++++++-----\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++\n 13 files changed, 84 insertions(+), 15 deletions(-)\n\n\nbase-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1651\n\nRange-diff vs v1:\n\n 1: a3e8ae86114 ! 1: 01dfd66568c merge-tree: fail with a non-zero exit code on missing tree objects\n @@ merge-ort.c: static int collect_merge_info(struct merge_options *opt,\n \n ## t/t4301-merge-tree-write-tree.sh ##\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--merge-base with tree OIDs' '\n - \tgit merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&\n \ttest_cmp with-commits with-trees\n '\n + \n +test_expect_success 'error out on missing tree objects' '\n +\tgit init --bare missing-tree.git &&\n -+\t(\n -+\t\tgit rev-list side3 &&\n -+\t\tgit rev-parse side3^:\n -+\t) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&\n ++\tgit rev-list side3 >list &&\n ++\tgit rev-parse side3^: >list &&\n ++\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing actual &&\n +\ttest_must_be_empty actual\n +'\n - \n ++\n test_done\n 2: 3e5b787fc03 = 2: a1bbb7e06e5 merge-ort: do check `parse_tree()`'s return value\n 3: 85d3e672871 = 3: be1dadf2850 t4301: verify that merge-tree fails on missing blob objects\n 4: 93abd7000b8 = 4: ffd38ad602a Always check `parse_tree*()`'s return value\n -: ----------- > 5: 43c04749513 cache-tree: avoid an unnecessary check\n\n-- \ngitgitgadget\n"},{"id":"488150","messageId":"01dfd66568c1818819e81e001cc189f9066d0cf0.1707324462.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:37Z","receivedAt":"2024-02-07T16:47:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `git merge-tree` encounters a missing tree object, it should error\nout and not continue quietly as if nothing had happened.\n\nHowever, as of time of writing, `git merge-tree` _does_ continue, and\nthen offers the empty tree as result.\n\nLet's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 7 ++++---\n t/t4301-merge-tree-write-tree.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6491070d965..c37fc035f13 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,\n \tinfo.data = opt;\n \tinfo.show_all_errors = 1;\n \n-\tparse_tree(merge_base);\n-\tparse_tree(side1);\n-\tparse_tree(side2);\n+\tif (parse_tree(merge_base) < 0 ||\n+\t parse_tree(side1) < 0 ||\n+\t parse_tree(side2) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(t + 0, merge_base->buffer, merge_base->size);\n \tinit_tree_desc(t + 1, side1->buffer, side1->size);\n \tinit_tree_desc(t + 2, side2->buffer, side2->size);\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 7d0fa74da74..7d588557bdf 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -951,4 +951,14 @@ test_expect_success '--merge-base with tree OIDs' '\n \ttest_cmp with-commits with-trees\n '\n \n+test_expect_success 'error out on missing tree objects' '\n+\tgit init --bare missing-tree.git &&\n+\tgit rev-list side3 >list &&\n+\tgit rev-parse side3^: >list &&\n+\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"488151","messageId":"a1bbb7e06e5c319e14e6a2a3b40680354aca8987.1707324462.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] merge-ort: do check `parse_tree()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:38Z","receivedAt":"2024-02-07T16:47:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe previous commit fixed a bug where a missing tree was reported, but\nnot treated as an error.\n\nThis patch addresses the same issue for the remaining two callers of\n`parse_tree()`.\n\nThis change is not accompanied by a regression test because the code in\nquestion is only reached at the `checkout` stage, i.e. after the merge\nhas happened (and therefore the tree objects could only be missing if\nthe disk had gone bad in that short time window, or something similarly\ntricky to recreate in the test suite).\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c37fc035f13..79d9e18f63d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,\n \tunpack_opts.verbose_update = (opt->verbosity > 2);\n \tunpack_opts.fn = twoway_merge;\n \tunpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */\n-\tparse_tree(prev);\n+\tif (parse_tree(prev) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n-\tparse_tree(next);\n+\tif (parse_tree(next) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[1], next->buffer, next->size);\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n-- \ngitgitgadget\n\n"},{"id":"488152","messageId":"be1dadf28502fe3e9662fa61523e8c57ce3352f1.1707324462.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:39Z","receivedAt":"2024-02-07T16:47:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a problem where `merge-tree` would not fail on missing\ntree objects. Let's ensure that that problem does not occur with blob\nobjects (and won't, in the future, either).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4301-merge-tree-write-tree.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 7d588557bdf..9211cb58aa1 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'error out on missing blob objects' '\n+\tseq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n+\tseq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n+\tseq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n+\ttree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n+\ttree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n+\ttree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n+\tgit init --bare missing-blob.git &&\n+\ttest_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |\n+\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n+\ttest_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"488153","messageId":"ffd38ad602a53dfe72cdbfe0d098ca43e7443895.1707324462.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:40Z","receivedAt":"2024-02-07T16:47:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOtherwise we may easily run into serious crashes: For example, if we run\n`init_tree_desc()` directly after a failed `parse_tree()`, we are\naccessing uninitialized data or trying to dereference `NULL`.\n\nNote that the `parse_tree()` function already takes care of showing an\nerror message. The `parse_tree_indirectly()` and\n`repo_get_commit_tree()` functions do not, therefore those latter call\nsites need to show a useful error message while the former do not.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 3 +++\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n 12 files changed, 52 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f02434bc155..84108ec3635 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \tinit_checkout_metadata(&opts.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n \t\t\t NULL);\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\treturn 128;\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tif (new_branch_info->commit)\n \t\t\tBUG(\"'switch --orphan' should never accept a commit as starting point\");\n \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n-\t} else\n+\t\tif (!new_tree)\n+\t\t\tBUG(\"unable to read empty tree\");\n+\t} else {\n \t\tnew_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\tnew_branch_info->commit);\n+\t\tif (!new_tree)\n+\t\t\treturn error(_(\"unable to read tree %s\"),\n+\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n+\t}\n \tif (opts->discard_changes) {\n \t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n \t\tif (ret)\n@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\toid_to_hex(old_commit_oid));\n \n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n-\t\tparse_tree(new_tree);\n+\t\tif (parse_tree(new_tree) < 0)\n+\t\t\texit(128);\n \t\ttree = new_tree;\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!new_branch_info->commit) {\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(rev));\n \t} else {\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t\t*source_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t new_branch_info->commit);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n \t}\n }\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af9498..4410b55be98 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)\n \ttree = parse_tree_indirect(&oid);\n \tif (!tree)\n \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 781af2e206c..0723f06de7a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)\n \ttree = parse_tree_indirect(¤t_head->object.oid);\n \tif (!tree)\n \t\tdie(_(\"failed to unpack HEAD tree object\"));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n \t\texit(128); /* We've already reported the error, finish dying */\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 2d4ce5b3886..ba84d00deee 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,\n \t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n \t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (!base_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&base_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n \t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (!parent1_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&head_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n \t\tparent2_tree = parse_tree_indirect(&merge_oid);\n+\t\tif (!parent2_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&merge_oid));\n \n \t\topt.ancestor = merge_base;\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 8196ca9dd85..5923ed36893 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tcache_tree_free(&the_index.cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\n-\t\tparse_tree(tree);\n+\t\tif (parse_tree(tree) < 0)\n+\t\t\treturn 128;\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4b018d20e3b..f030f57f4e9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n+\t\tif (!tree) {\n+\t\t\terror(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\t\tgoto out;\n+\t\t}\n \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n \t}\n \ndiff --git a/cache-tree.c b/cache-tree.c\nindex 641427ed410..c6508b64a5c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed)\n-\t\t\t\tparse_tree(subtree);\n+\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 79d9e18f63d..534ddaf16ba 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \n \tif (result->clean >= 0) {\n \t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\tif (!result->tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&working_tree_oid));\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e3beb0801b1..10d41bfd487 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)\n \n static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n {\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(desc, tree->buffer, tree->size);\n }\n \ndiff --git a/merge.c b/merge.c\nindex b60925459c2..14a7325859d 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,\n \t\treturn -1;\n \t}\n \tfor (i = 0; i < nr_trees; i++) {\n-\t\tparse_tree(trees[i]);\n+\t\tif (parse_tree(trees[i]) < 0) {\n+\t\t\trollback_lock_file(&lock_file);\n+\t\t\treturn -1;\n+\t\t}\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \ndiff --git a/reset.c b/reset.c\nindex 48da0adf851..a93fdbc12e3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \t}\n \n \ttree = parse_tree_indirect(oid);\n+\tif (!tree) {\n+\t\tret = error(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..407473bab28 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,\n \to.show_rename_progress = 1;\n \n \thead_tree = parse_tree_indirect(head);\n+\tif (!head_tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(head));\n \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n \n@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(&oid));\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n-- \ngitgitgadget\n\n"},{"id":"488154","messageId":"43c04749513d07733f5fa2c15a694d99d31fe6e3.1707324462.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] cache-tree: avoid an unnecessary check","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:47:41Z","receivedAt":"2024-02-07T16:47:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe first thing the `parse_tree()` function does is to return early if\nthe tree has already been parsed. Therefore we do not need to guard the\n`parse_tree()` call behind a check of that flag.\n\nAs of time of writing, there are no other instances of this in Git's\ncode bases: whenever the `parsed` flag guards a `parse_tree()` call, it\nguards more than just that call.\n\nSuggested-by: Patrick Steinhardt \nSigned-off-by: Johannes Schindelin \n---\n cache-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c6508b64a5c..78d6ba92853 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\tif (parse_tree(subtree) < 0)\n \t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n-- \ngitgitgadget\n"},{"id":"488158","messageId":"xmqqwmrgb49a.fsf@gitster.g","threadId":"60855","inReplyTo":"43c04749513d07733f5fa2c15a694d99d31fe6e3.1707324462.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] cache-tree: avoid an unnecessary check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T16:58:25Z","receivedAt":"2024-02-07T16:58:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The first thing the `parse_tree()` function does is to return early if\n> the tree has already been parsed. Therefore we do not need to guard the\n> `parse_tree()` call behind a check of that flag.\n\nMakes sense, and it doubly makes sense to keep this separate from\nthe change done to the same location in [4/5].\n\nWill queue.\n"},{"id":"488160","messageId":"CAPig+cSs8MFkLasTULh7tybrFm7SwaT+JeR7HnXjh+-agCHYMw@mail.gmail.com","threadId":"60855","inReplyTo":"01dfd66568c1818819e81e001cc189f9066d0cf0.1707324462.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-07T17:02:33Z","receivedAt":"2024-02-07T17:02:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n wrote:\n> When `git merge-tree` encounters a missing tree object, it should error\n> out and not continue quietly as if nothing had happened.\n>\n> However, as of time of writing, `git merge-tree` _does_ continue, and\n> then offers the empty tree as result.\n>\n> Let's fix this.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> @@ -951,4 +951,14 @@ test_expect_success '--merge-base with tree OIDs' '\n> +test_expect_success 'error out on missing tree objects' '\n> + git init --bare missing-tree.git &&\n> + git rev-list side3 >list &&\n> + git rev-parse side3^: >list &&\n\nIsn't the git-rev-parse invocation simply overwriting \"list\" rather\nthan appending to it? Did you mean:\n\n git rev-list side3 >list &&\n git rev-parse side3^: >>list &&\n\nAn alternative would be:\n\n {\n git rev-list side3 &&\n git rev-parse side3^:\n } >list &&\n\n> + git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing + side3=$(git rev-parse side3) &&\n> + test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&\n> + test_must_be_empty actual\n> +'\n"},{"id":"488165","messageId":"CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com","threadId":"60855","inReplyTo":"be1dadf28502fe3e9662fa61523e8c57ce3352f1.1707324462.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-07T17:24:59Z","receivedAt":"2024-02-07T17:25:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n wrote:\n> We just fixed a problem where `merge-tree` would not fail on missing\n> tree objects. Let's ensure that that problem does not occur with blob\n> objects (and won't, in the future, either).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> @@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '\n> +test_expect_success 'error out on missing blob objects' '\n> + seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n> + seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n> + seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n\nIs there significance to the ranges passed to test_seq()? Or, can the\nsame be achieved by using arbitrary content for each blob?\n\n blob1=$(echo \"one\" | git hash-object -w --stdin) &&\n blob2=$(echo \"two\" | git hash-object -w --stdin) &&\n blob3=$(echo \"three\" | git hash-object -w --stdin) &&\n\n> + tree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n> + tree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n> + tree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n\nI found the lack of terminating \"\\n\" in the `printf` confusing,\nespecially since the variable names (seq1, seq2, etc.) and the use of\n`printf` seem to imply, at first glance, that each git-mktree\ninvocation is receiving multiple lines as input which, it turns out,\nis not the case. Adding the missing \"\\n\" would help:\n\n tree1=$(printf \"100644 blob %s\\tsequence\\n\" $seq1 | git mktree) &&\n tree2=$(printf \"100644 blob %s\\tsequence\\n\" $seq2 | git mktree) &&\n tree3=$(printf \"100644 blob %s\\tsequence\\n\" $seq3 | git mktree) &&\n\nInterpolating the $seqN variable directly into the string rather than\nusing %s would make it even clearer that only a single line is being\ngenerated as input to git-mktree:\n\n tree1=$(printf \"100644 blob $seq1\\tsequence\\n\" | git mktree) &&\n tree2=$(printf \"100644 blob $seq2\\tsequence\\n\" | git mktree) &&\n tree3=$(printf \"100644 blob $seq3\\tsequence\\n\" | git mktree) &&\n\nAlternatively `echo` could be used, though it's not necessarily any nicer:\n\n tree1=$(echo \"100644 blob $seq1Qsequence\" | q_to_tab | git mktree) &&\n tree2=$(echo \"100644 blob $seq2Qsequence\" | q_to_tab | git mktree) &&\n tree3=$(echo \"100644 blob $seq3Qsequence\" | q_to_tab | git mktree) &&\n\n> + git init --bare missing-blob.git &&\n> + test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |\n> + git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n> + test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&\n> + test_must_be_empty actual\n> +'\n"},{"id":"488166","messageId":"CAPig+cRK_2BWYkcgaZ1pMJJtztepeEJCwuevJNj6gJwRoJgF5g@mail.gmail.com","threadId":"60855","inReplyTo":"ffd38ad602a53dfe72cdbfe0d098ca43e7443895.1707324462.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] Always check `parse_tree*()`'s return value","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-07T17:26:15Z","receivedAt":"2024-02-07T17:26:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n wrote:\n> Always check `parse_tree*()`'s return value\n\nIf you happen to reroll for some reason, perhaps: s/Always/always/\n\n> Otherwise we may easily run into serious crashes: For example, if we run\n> `init_tree_desc()` directly after a failed `parse_tree()`, we are\n> accessing uninitialized data or trying to dereference `NULL`.\n>\n> Note that the `parse_tree()` function already takes care of showing an\n> error message. The `parse_tree_indirectly()` and\n> `repo_get_commit_tree()` functions do not, therefore those latter call\n> sites need to show a useful error message while the former do not.\n>\n> Signed-off-by: Johannes Schindelin \n"},{"id":"488182","messageId":"xmqqr0ho55ni.fsf@gitster.g","threadId":"60855","inReplyTo":"CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com","subject":"Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T21:24:49Z","receivedAt":"2024-02-07T21:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n>> @@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '\n>> +test_expect_success 'error out on missing blob objects' '\n>> + seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n>> + seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n>> + seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n>\n> Is there significance to the ranges passed to test_seq()? Or, can the\n> same be achieved by using arbitrary content for each blob?\n>\n> blob1=$(echo \"one\" | git hash-object -w --stdin) &&\n> blob2=$(echo \"two\" | git hash-object -w --stdin) &&\n> blob3=$(echo \"three\" | git hash-object -w --stdin) &&\n>\n>> + tree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n>> + tree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n>> + tree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n>\n> I found the lack of terminating \"\\n\" in the `printf` confusing,\n> especially since the variable names (seq1, seq2, etc.) and the use of\n> `printf` seem to imply, at first glance, that each git-mktree\n> invocation is receiving multiple lines as input which, it turns out,\n> is not the case. Adding the missing \"\\n\" would help:\n>\n> tree1=$(printf \"100644 blob %s\\tsequence\\n\" $seq1 | git mktree) &&\n> tree2=$(printf \"100644 blob %s\\tsequence\\n\" $seq2 | git mktree) &&\n> tree3=$(printf \"100644 blob %s\\tsequence\\n\" $seq3 | git mktree) &&\n>\n> Interpolating the $seqN variable directly into the string rather than\n> using %s would make it even clearer that only a single line is being\n> generated as input to git-mktree:\n>\n> tree1=$(printf \"100644 blob $seq1\\tsequence\\n\" | git mktree) &&\n> tree2=$(printf \"100644 blob $seq2\\tsequence\\n\" | git mktree) &&\n> tree3=$(printf \"100644 blob $seq3\\tsequence\\n\" | git mktree) &&\n\nAll good points. Thanks for excellent reviews (not just this one\nbut another one in the series).\n"},{"id":"488200","messageId":"CAPig+cQx1=wM7v0cCuVwNwhd3Zsh0fZgZ1A-vX--uy0dd1+kDg@mail.gmail.com","threadId":"60855","inReplyTo":"CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com","subject":"Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-08T00:52:43Z","receivedAt":"2024-02-08T00:52:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 7, 2024 at 12:24 PM Eric Sunshine wrote:\n> Interpolating the $seqN variable directly into the string rather than\n> using %s would make it even clearer that only a single line is being\n> generated as input to git-mktree:\n>\n> tree1=$(printf \"100644 blob $seq1\\tsequence\\n\" | git mktree) &&\n> tree2=$(printf \"100644 blob $seq2\\tsequence\\n\" | git mktree) &&\n> tree3=$(printf \"100644 blob $seq3\\tsequence\\n\" | git mktree) &&\n>\n> Alternatively `echo` could be used, though it's not necessarily any nicer:\n>\n> tree1=$(echo \"100644 blob $seq1Qsequence\" | q_to_tab | git mktree) &&\n> tree2=$(echo \"100644 blob $seq2Qsequence\" | q_to_tab | git mktree) &&\n> tree3=$(echo \"100644 blob $seq3Qsequence\" | q_to_tab | git mktree) &&\n\nThe `printf` example is probably cleaner, thus preferable. For\ncompleteness, though, I should mention that the `echo` example is, of\ncourse, not quite correct. For the interpolation to work as intended,\nit would need ${...}:\n\n tree1=$(echo \"100644 blob ${seq1}Qsequence\" | q_to_tab | git mktree) &&\n tree2=$(echo \"100644 blob ${seq2}Qsequence\" | q_to_tab | git mktree) &&\n tree3=$(echo \"100644 blob ${seq3}Qsequence\" | q_to_tab | git mktree) &&\n"},{"id":"489148","messageId":"dbde46bc-501e-8433-0b8d-b83d0ca9e759@gmx.de","threadId":"60855","inReplyTo":"CAPig+cRK_2BWYkcgaZ1pMJJtztepeEJCwuevJNj6gJwRoJgF5g@mail.gmail.com","subject":"Re: [PATCH v2 4/5] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-22T14:08:08Z","receivedAt":"2024-02-22T14:08:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 7 Feb 2024, Eric Sunshine wrote:\n\n> On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> > Always check `parse_tree*()`'s return value\n>\n> If you happen to reroll for some reason, perhaps: s/Always/always/\n\nUnless I am missing something we only ask the part of a oneline after an\ninitial \":\" to be downcased.\n\nAt least all the \"Merge branch [...]\" commits are still capitalized and\nnobody complains ;-)\n\nCiao,\nJohannes\n\n>\n> > Otherwise we may easily run into serious crashes: For example, if we run\n> > `init_tree_desc()` directly after a failed `parse_tree()`, we are\n> > accessing uninitialized data or trying to dereference `NULL`.\n> >\n> > Note that the `parse_tree()` function already takes care of showing an\n> > error message. The `parse_tree_indirectly()` and\n> > `repo_get_commit_tree()` functions do not, therefore those latter call\n> > sites need to show a useful error message while the former do not.\n> >\n> > Signed-off-by: Johannes Schindelin \n>\n"},{"id":"489149","messageId":"ea0ad9f6-b3c1-5047-bf1f-c6134bc6218c@gmx.de","threadId":"60855","inReplyTo":"CAPig+cSs8MFkLasTULh7tybrFm7SwaT+JeR7HnXjh+-agCHYMw@mail.gmail.com","subject":"Re: [PATCH v2 1/5] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-22T14:09:49Z","receivedAt":"2024-02-22T14:09:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 7 Feb 2024, Eric Sunshine wrote:\n\n> On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> > When `git merge-tree` encounters a missing tree object, it should error\n> > out and not continue quietly as if nothing had happened.\n> >\n> > However, as of time of writing, `git merge-tree` _does_ continue, and\n> > then offers the empty tree as result.\n> >\n> > Let's fix this.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> > @@ -951,4 +951,14 @@ test_expect_success '--merge-base with tree OIDs' '\n> > +test_expect_success 'error out on missing tree objects' '\n> > + git init --bare missing-tree.git &&\n> > + git rev-list side3 >list &&\n> > + git rev-parse side3^: >list &&\n>\n> Isn't the git-rev-parse invocation simply overwriting \"list\" rather\n> than appending to it? Did you mean:\n>\n> git rev-list side3 >list &&\n> git rev-parse side3^: >>list &&\n\nYes. And the fact that this test case still passed was for the _wrong_\nreason! I adjusted the test case by writing the correct contents to `list`\nand by verifying that the expected error message is shown.\n\n>\n> An alternative would be:\n>\n> {\n> git rev-list side3 &&\n> git rev-parse side3^:\n> } >list &&\n\nI find that uglier and longer, so I'll stay with the first option ;-)\n\nThank you for your review!\nJohannes\n\n>\n> > + git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing > + side3=$(git rev-parse side3) &&\n> > + test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&\n> > + test_must_be_empty actual\n> > +'\n>\n"},{"id":"489150","messageId":"80255493-701b-b4fc-5d0f-54e059d97f71@gmx.de","threadId":"60855","inReplyTo":"CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com","subject":"Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-22T14:12:08Z","receivedAt":"2024-02-22T14:12:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Wed, 7 Feb 2024, Eric Sunshine wrote:\n\n> On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> > We just fixed a problem where `merge-tree` would not fail on missing\n> > tree objects. Let's ensure that that problem does not occur with blob\n> > objects (and won't, in the future, either).\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> > @@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '\n> > +test_expect_success 'error out on missing blob objects' '\n> > + seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n> > + seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n> > + seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n>\n> Is there significance to the ranges passed to test_seq()?\n\nOriginally, there was, since I wanted the merge to succeed without\nconflicts.\n\nHowever, I simplified the test case similar to what you suggested and\nnow specifically verify that the expected error message is shown (as\nopposed to, say, an error message indicating a merge conflict).\n\n> Or, can the same be achieved by using arbitrary content for each blob?\n>\n> blob1=$(echo \"one\" | git hash-object -w --stdin) &&\n> blob2=$(echo \"two\" | git hash-object -w --stdin) &&\n> blob3=$(echo \"three\" | git hash-object -w --stdin) &&\n>\n> > + tree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n> > + tree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n> > + tree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n>\n> I found the lack of terminating \"\\n\" in the `printf` confusing,\n> especially since the variable names (seq1, seq2, etc.) and the use of\n> `printf` seem to imply, at first glance, that each git-mktree\n> invocation is receiving multiple lines as input which, it turns out,\n> is not the case. Adding the missing \"\\n\" would help:\n>\n> tree1=$(printf \"100644 blob %s\\tsequence\\n\" $seq1 | git mktree) &&\n> tree2=$(printf \"100644 blob %s\\tsequence\\n\" $seq2 | git mktree) &&\n> tree3=$(printf \"100644 blob %s\\tsequence\\n\" $seq3 | git mktree) &&\n\nI added the `\\n` and now avoid the `%s`.\n\nThank you for your review!\nJohannes\n\n>\n> Interpolating the $seqN variable directly into the string rather than\n> using %s would make it even clearer that only a single line is being\n> generated as input to git-mktree:\n>\n> tree1=$(printf \"100644 blob $seq1\\tsequence\\n\" | git mktree) &&\n> tree2=$(printf \"100644 blob $seq2\\tsequence\\n\" | git mktree) &&\n> tree3=$(printf \"100644 blob $seq3\\tsequence\\n\" | git mktree) &&\n>\n> Alternatively `echo` could be used, though it's not necessarily any nicer:\n>\n> tree1=$(echo \"100644 blob $seq1Qsequence\" | q_to_tab | git mktree) &&\n> tree2=$(echo \"100644 blob $seq2Qsequence\" | q_to_tab | git mktree) &&\n> tree3=$(echo \"100644 blob $seq3Qsequence\" | q_to_tab | git mktree) &&\n>\n> > + git init --bare missing-blob.git &&\n> > + test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |\n> > + git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n> > + test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&\n> > + test_must_be_empty actual\n> > +'\n>\n"},{"id":"489151","messageId":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v2.git.1707324461.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] merge-tree: handle missing objects correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:40Z","receivedAt":"2024-02-22T14:36:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently looked into issues where git merge-tree calls returned bogus data\n(in one instance returning an empty tree for non-empty merge parents). By\nthe time I had a look at the corresponding repository, the issue was no\nlonger reproducible, but a closer look at the code combined with some manual\nexperimenting turned up the fact that missing tree objects aren't handled as\nerrors by git merge-tree.\n\nWhile at it, I added a commit on top that tries to catch all remaining\nunchecked parse_tree() calls.\n\nThis patch series is based on 5f43cf5b2e4 (merge-tree: accept 3 trees as\narguments, 2024-01-28) (the original tip commit of js/merge-tree-3-trees)\nbecause I introduced three unchecked parse_tree() calls in that topic.\n\nChanges since v2:\n\n * Fixed the new \"missing tree object\" test case in t4301 that succeeded for\n the wrong reason.\n * Adjusted the new \"missing blob object\" test case to avoid succeeding for\n the wrong reason.\n * Simplified the \"missing blob object\" test case.\n\nChanges since v1:\n\n * Simplified the test case, avoiding a subshell and a pipe in the process.\n * Added a patch to remove a superfluous subtree->object.parsed guard around\n a parse_tree(subtree) call.\n\nJohannes Schindelin (5):\n merge-tree: fail with a non-zero exit code on missing tree objects\n merge-ort: do check `parse_tree()`'s return value\n t4301: verify that merge-tree fails on missing blob objects\n Always check `parse_tree*()`'s return value\n cache-tree: avoid an unnecessary check\n\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 16 +++++++++++-----\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++++++++++\n 13 files changed, 87 insertions(+), 15 deletions(-)\n\n\nbase-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1651\n\nRange-diff vs v2:\n\n 1: 01dfd66568c ! 1: 11b9cd8c5da merge-tree: fail with a non-zero exit code on missing tree objects\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--merge-base with tree OI\n +test_expect_success 'error out on missing tree objects' '\n +\tgit init --bare missing-tree.git &&\n +\tgit rev-list side3 >list &&\n -+\tgit rev-parse side3^: >list &&\n ++\tgit rev-parse side3^: >>list &&\n +\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing actual &&\n ++\ttest_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&\n ++\ttest_grep \"Could not read $(git rev-parse $side3:)\" err &&\n +\ttest_must_be_empty actual\n +'\n +\n 2: a1bbb7e06e5 = 2: f01f4eb011b merge-ort: do check `parse_tree()`'s return value\n 3: be1dadf2850 ! 3: e82fdf7fbcb t4301: verify that merge-tree fails on missing blob objects\n @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'error out on missing tree\n '\n \n +test_expect_success 'error out on missing blob objects' '\n -+\tseq1=$(test_seq 1 10 | git hash-object -w --stdin) &&\n -+\tseq2=$(test_seq 1 11 | git hash-object -w --stdin) &&\n -+\tseq3=$(test_seq 0 10 | git hash-object -w --stdin) &&\n -+\ttree1=$(printf \"100644 blob %s\\tsequence\" $seq1 | git mktree) &&\n -+\ttree2=$(printf \"100644 blob %s\\tsequence\" $seq2 | git mktree) &&\n -+\ttree3=$(printf \"100644 blob %s\\tsequence\" $seq3 | git mktree) &&\n ++\techo 1 | git hash-object -w --stdin >blob1 &&\n ++\techo 2 | git hash-object -w --stdin >blob2 &&\n ++\techo 3 | git hash-object -w --stdin >blob3 &&\n ++\tprintf \"100644 blob $(cat blob1)\\tblob\\n\" | git mktree >tree1 &&\n ++\tprintf \"100644 blob $(cat blob2)\\tblob\\n\" | git mktree >tree2 &&\n ++\tprintf \"100644 blob $(cat blob3)\\tblob\\n\" | git mktree >tree3 &&\n +\tgit init --bare missing-blob.git &&\n -+\ttest_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |\n ++\tcat blob1 blob3 tree1 tree2 tree3 |\n +\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n -+\ttest_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&\n ++\ttest_must_fail git --git-dir=missing-blob.git >actual 2>err \\\n ++\t\tmerge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&\n ++\ttest_grep \"unable to read blob object $(cat blob2)\" err &&\n +\ttest_must_be_empty actual\n +'\n +\n 4: ffd38ad602a = 4: 9e4dc94ef03 Always check `parse_tree*()`'s return value\n 5: 43c04749513 = 5: 91dc4ccd04e cache-tree: avoid an unnecessary check\n\n-- \ngitgitgadget\n"},{"id":"489152","messageId":"11b9cd8c5da5e6792ce940ea29d2e93e57731555.1708612605.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:41Z","receivedAt":"2024-02-22T14:36:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `git merge-tree` encounters a missing tree object, it should error\nout and not continue quietly as if nothing had happened.\n\nHowever, as of time of writing, `git merge-tree` _does_ continue, and\nthen offers the empty tree as result.\n\nLet's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 7 ++++---\n t/t4301-merge-tree-write-tree.sh | 11 +++++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6491070d965..c37fc035f13 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,\n \tinfo.data = opt;\n \tinfo.show_all_errors = 1;\n \n-\tparse_tree(merge_base);\n-\tparse_tree(side1);\n-\tparse_tree(side2);\n+\tif (parse_tree(merge_base) < 0 ||\n+\t parse_tree(side1) < 0 ||\n+\t parse_tree(side2) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(t + 0, merge_base->buffer, merge_base->size);\n \tinit_tree_desc(t + 1, side1->buffer, side1->size);\n \tinit_tree_desc(t + 2, side2->buffer, side2->size);\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 7d0fa74da74..908c9b540c8 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '\n \ttest_cmp with-commits with-trees\n '\n \n+test_expect_success 'error out on missing tree objects' '\n+\tgit init --bare missing-tree.git &&\n+\tgit rev-list side3 >list &&\n+\tgit rev-parse side3^: >>list &&\n+\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing actual 2>err &&\n+\ttest_grep \"Could not read $(git rev-parse $side3:)\" err &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"489153","messageId":"f01f4eb011b400faeff1c33934775a521dec7a3d.1708612605.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] merge-ort: do check `parse_tree()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:42Z","receivedAt":"2024-02-22T14:36:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe previous commit fixed a bug where a missing tree was reported, but\nnot treated as an error.\n\nThis patch addresses the same issue for the remaining two callers of\n`parse_tree()`.\n\nThis change is not accompanied by a regression test because the code in\nquestion is only reached at the `checkout` stage, i.e. after the merge\nhas happened (and therefore the tree objects could only be missing if\nthe disk had gone bad in that short time window, or something similarly\ntricky to recreate in the test suite).\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c37fc035f13..79d9e18f63d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,\n \tunpack_opts.verbose_update = (opt->verbosity > 2);\n \tunpack_opts.fn = twoway_merge;\n \tunpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */\n-\tparse_tree(prev);\n+\tif (parse_tree(prev) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n-\tparse_tree(next);\n+\tif (parse_tree(next) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[1], next->buffer, next->size);\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n-- \ngitgitgadget\n\n"},{"id":"489154","messageId":"e82fdf7fbcbf12fffdf4a720927c2f4f006068f8.1708612605.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:43Z","receivedAt":"2024-02-22T14:36:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a problem where `merge-tree` would not fail on missing\ntree objects. Let's ensure that that problem does not occur with blob\nobjects (and won't, in the future, either).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 908c9b540c8..d4463a45706 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'error out on missing blob objects' '\n+\techo 1 | git hash-object -w --stdin >blob1 &&\n+\techo 2 | git hash-object -w --stdin >blob2 &&\n+\techo 3 | git hash-object -w --stdin >blob3 &&\n+\tprintf \"100644 blob $(cat blob1)\\tblob\\n\" | git mktree >tree1 &&\n+\tprintf \"100644 blob $(cat blob2)\\tblob\\n\" | git mktree >tree2 &&\n+\tprintf \"100644 blob $(cat blob3)\\tblob\\n\" | git mktree >tree3 &&\n+\tgit init --bare missing-blob.git &&\n+\tcat blob1 blob3 tree1 tree2 tree3 |\n+\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n+\ttest_must_fail git --git-dir=missing-blob.git >actual 2>err \\\n+\t\tmerge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&\n+\ttest_grep \"unable to read blob object $(cat blob2)\" err &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"489155","messageId":"9e4dc94ef036882c3ce27208ca9fa545d018f199.1708612605.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:44Z","receivedAt":"2024-02-22T14:36:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOtherwise we may easily run into serious crashes: For example, if we run\n`init_tree_desc()` directly after a failed `parse_tree()`, we are\naccessing uninitialized data or trying to dereference `NULL`.\n\nNote that the `parse_tree()` function already takes care of showing an\nerror message. The `parse_tree_indirectly()` and\n`repo_get_commit_tree()` functions do not, therefore those latter call\nsites need to show a useful error message while the former do not.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 3 +++\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n 12 files changed, 52 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f02434bc155..84108ec3635 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \tinit_checkout_metadata(&opts.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n \t\t\t NULL);\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\treturn 128;\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tif (new_branch_info->commit)\n \t\t\tBUG(\"'switch --orphan' should never accept a commit as starting point\");\n \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n-\t} else\n+\t\tif (!new_tree)\n+\t\t\tBUG(\"unable to read empty tree\");\n+\t} else {\n \t\tnew_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\tnew_branch_info->commit);\n+\t\tif (!new_tree)\n+\t\t\treturn error(_(\"unable to read tree %s\"),\n+\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n+\t}\n \tif (opts->discard_changes) {\n \t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n \t\tif (ret)\n@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\toid_to_hex(old_commit_oid));\n \n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n-\t\tparse_tree(new_tree);\n+\t\tif (parse_tree(new_tree) < 0)\n+\t\t\texit(128);\n \t\ttree = new_tree;\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!new_branch_info->commit) {\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(rev));\n \t} else {\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t\t*source_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t new_branch_info->commit);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n \t}\n }\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af9498..4410b55be98 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)\n \ttree = parse_tree_indirect(&oid);\n \tif (!tree)\n \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 781af2e206c..0723f06de7a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)\n \ttree = parse_tree_indirect(¤t_head->object.oid);\n \tif (!tree)\n \t\tdie(_(\"failed to unpack HEAD tree object\"));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n \t\texit(128); /* We've already reported the error, finish dying */\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 2d4ce5b3886..ba84d00deee 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,\n \t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n \t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (!base_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&base_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n \t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (!parent1_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&head_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n \t\tparent2_tree = parse_tree_indirect(&merge_oid);\n+\t\tif (!parent2_tree)\n+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&merge_oid));\n \n \t\topt.ancestor = merge_base;\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 8196ca9dd85..5923ed36893 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tcache_tree_free(&the_index.cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\n-\t\tparse_tree(tree);\n+\t\tif (parse_tree(tree) < 0)\n+\t\t\treturn 128;\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4b018d20e3b..f030f57f4e9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n+\t\tif (!tree) {\n+\t\t\terror(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\t\tgoto out;\n+\t\t}\n \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n \t}\n \ndiff --git a/cache-tree.c b/cache-tree.c\nindex 641427ed410..c6508b64a5c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed)\n-\t\t\t\tparse_tree(subtree);\n+\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 79d9e18f63d..534ddaf16ba 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \n \tif (result->clean >= 0) {\n \t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\tif (!result->tree)\n+\t\t\tdie(_(\"unable to read tree %s\"),\n+\t\t\t oid_to_hex(&working_tree_oid));\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e3beb0801b1..10d41bfd487 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)\n \n static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n {\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(desc, tree->buffer, tree->size);\n }\n \ndiff --git a/merge.c b/merge.c\nindex b60925459c2..14a7325859d 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,\n \t\treturn -1;\n \t}\n \tfor (i = 0; i < nr_trees; i++) {\n-\t\tparse_tree(trees[i]);\n+\t\tif (parse_tree(trees[i]) < 0) {\n+\t\t\trollback_lock_file(&lock_file);\n+\t\t\treturn -1;\n+\t\t}\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \ndiff --git a/reset.c b/reset.c\nindex 48da0adf851..a93fdbc12e3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \t}\n \n \ttree = parse_tree_indirect(oid);\n+\tif (!tree) {\n+\t\tret = error(_(\"unable to read tree %s\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..407473bab28 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,\n \to.show_rename_progress = 1;\n \n \thead_tree = parse_tree_indirect(head);\n+\tif (!head_tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(head));\n \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n \n@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(&oid));\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n-- \ngitgitgadget\n\n"},{"id":"489156","messageId":"91dc4ccd04e3a6cc50ed389edb6814e1e7a0c4dc.1708612605.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] cache-tree: avoid an unnecessary check","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-22T14:36:45Z","receivedAt":"2024-02-22T14:36:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe first thing the `parse_tree()` function does is to return early if\nthe tree has already been parsed. Therefore we do not need to guard the\n`parse_tree()` call behind a check of that flag.\n\nAs of time of writing, there are no other instances of this in Git's\ncode bases: whenever the `parsed` flag guards a `parse_tree()` call, it\nguards more than just that call.\n\nSuggested-by: Patrick Steinhardt \nSigned-off-by: Johannes Schindelin \n---\n cache-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c6508b64a5c..78d6ba92853 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\tif (parse_tree(subtree) < 0)\n \t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n-- \ngitgitgadget\n"},{"id":"489170","messageId":"xmqq8r3ch1jt.fsf@gitster.g","threadId":"60855","inReplyTo":"dbde46bc-501e-8433-0b8d-b83d0ca9e759@gmx.de","subject":"Re: [PATCH v2 4/5] Always check `parse_tree*()`'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T17:05:42Z","receivedAt":"2024-02-22T17:05:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> If you happen to reroll for some reason, perhaps: s/Always/always/\n>\n> Unless I am missing something we only ask the part of a oneline after an\n> initial \":\" to be downcased.\n>\n> At least all the \"Merge branch [...]\" commits are still capitalized and\n> nobody complains ;-)\n\nOf course. Merge commits and single-parent commits are very\ndifferent.\n\nI suspect that the lack of \":\" was what triggered the comment,\nbut this being more about the callers all over the place in the\ncode, it may be hard to say what \"area\" this belongs to. If I were\npressed to choose:\n\n Subject: parse_tree*(): make callers always check return value\n\nwould probably be what I would choose, but I usually opt for going\nthe lazy way of not labelling the area at all ;-).\n\n"},{"id":"489171","messageId":"xmqq4je0h171.fsf@gitster.g","threadId":"60855","inReplyTo":"11b9cd8c5da5e6792ce940ea29d2e93e57731555.1708612605.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/5] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T17:13:22Z","receivedAt":"2024-02-22T17:13:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> -\tparse_tree(merge_base);\n> -\tparse_tree(side1);\n> -\tparse_tree(side2);\n> +\tif (parse_tree(merge_base) < 0 ||\n> +\t parse_tree(side1) < 0 ||\n> +\t parse_tree(side2) < 0)\n> +\t\treturn -1;\n\nObviously good.\n\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 7d0fa74da74..908c9b540c8 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '\n> \ttest_cmp with-commits with-trees\n> '\n> \n> +test_expect_success 'error out on missing tree objects' '\n> +\tgit init --bare missing-tree.git &&\n> +\tgit rev-list side3 >list &&\n> +\tgit rev-parse side3^: >>list &&\n> +\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing +\tside3=$(git rev-parse side3) &&\n> +\ttest_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&\n> +\ttest_grep \"Could not read $(git rev-parse $side3:)\" err &&\n> +\ttest_must_be_empty actual\n> +'\n\nI very much like the way this test emulates an operation in a\nrepository that lack certaion objects so cleanly, and wish we\nhad used this pattern instead of poking at loose object files\nin hundreds of existing tests (#leftoverbits obviously).\n\nIt also justifies why a silent \"return -1\" in the patch is\nsufficient ;-)\n\nThanks.\n"},{"id":"489172","messageId":"xmqqy1bcfmev.fsf@gitster.g","threadId":"60855","inReplyTo":"f01f4eb011b400faeff1c33934775a521dec7a3d.1708612605.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/5] merge-ort: do check `parse_tree()`'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T17:18:00Z","receivedAt":"2024-02-22T17:18:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This change is not accompanied by a regression test because the code in\n> question is only reached at the `checkout` stage, i.e. after the merge\n> has happened (and therefore the tree objects could only be missing if\n> the disk had gone bad in that short time window, or something similarly\n> tricky to recreate in the test suite).\n\nMakes sense.\n\nA complete tangent I wonder is if unit-test-minded folks have clever\nideas to allow better test coverage, perhaps injecting a failure on\ndemand to any codepath (in this case, the codepath to write the\nresulting tree) to simulate a situation where we fail to parse the\ntree.\n\nIn any case, the patch looks good, of course, and I see no need for\nfurther comments.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index c37fc035f13..79d9e18f63d 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,\n> \tunpack_opts.verbose_update = (opt->verbosity > 2);\n> \tunpack_opts.fn = twoway_merge;\n> \tunpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */\n> -\tparse_tree(prev);\n> +\tif (parse_tree(prev) < 0)\n> +\t\treturn -1;\n> \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n> -\tparse_tree(next);\n> +\tif (parse_tree(next) < 0)\n> +\t\treturn -1;\n> \tinit_tree_desc(&trees[1], next->buffer, next->size);\n> \n> \tret = unpack_trees(2, trees, &unpack_opts);\n"},{"id":"489174","messageId":"xmqqttm0fly9.fsf@gitster.g","threadId":"60855","inReplyTo":"e82fdf7fbcbf12fffdf4a720927c2f4f006068f8.1708612605.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T17:27:58Z","receivedAt":"2024-02-22T17:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> We just fixed a problem where `merge-tree` would not fail on missing\n> tree objects. Let's ensure that that problem does not occur with blob\n> objects (and won't, in the future, either).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++\n> 1 file changed, 16 insertions(+)\n>\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index 908c9b540c8..d4463a45706 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'error out on missing blob objects' '\n> +\techo 1 | git hash-object -w --stdin >blob1 &&\n> +\techo 2 | git hash-object -w --stdin >blob2 &&\n> +\techo 3 | git hash-object -w --stdin >blob3 &&\n> +\tprintf \"100644 blob $(cat blob1)\\tblob\\n\" | git mktree >tree1 &&\n> +\tprintf \"100644 blob $(cat blob2)\\tblob\\n\" | git mktree >tree2 &&\n> +\tprintf \"100644 blob $(cat blob3)\\tblob\\n\" | git mktree >tree3 &&\n> +\tgit init --bare missing-blob.git &&\n> +\tcat blob1 blob3 tree1 tree2 tree3 |\n> +\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n> +\ttest_must_fail git --git-dir=missing-blob.git >actual 2>err \\\n> +\t\tmerge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&\n> +\ttest_grep \"unable to read blob object $(cat blob2)\" err &&\n> +\ttest_must_be_empty actual\n> +'\n\nIt would have been even easier to see that blob2 is what we expect\nto be complained about, if you listed all objects and filtered blob2\nout, but the number of objects involved here is so small, a \"cat\" of\nall objects we want to keep is OK here.\n\nAgain, I very much love the way the test repository/object store\nthat lack certain objects are constructed without making our hands\ndirty.\n\nI see no need for further comments. Looking very good.\n"},{"id":"489179","messageId":"xmqqplwoe5yv.fsf@gitster.g","threadId":"60855","inReplyTo":"9e4dc94ef036882c3ce27208ca9fa545d018f199.1708612605.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/5] Always check `parse_tree*()`'s return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T17:58:32Z","receivedAt":"2024-02-22T17:58:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> @@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n> \tinit_checkout_metadata(&opts.meta, info->refname,\n> \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n> \t\t\t NULL);\n> -\tparse_tree(tree);\n> +\tif (parse_tree(tree) < 0)\n> +\t\treturn 128;\n\nThe other error returned from this function is when unpack_trees()\nfails well before the writeout phase and the value we return is also\n128, so the caller is prepared to act on it. OK.\n\n> @@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\tif (new_branch_info->commit)\n> \t\t\tBUG(\"'switch --orphan' should never accept a commit as starting point\");\n> \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n> -\t} else\n> +\t\tif (!new_tree)\n> +\t\t\tBUG(\"unable to read empty tree\");\n\nparse_tree() of the_hash_algo->empty_tree should result in a tree\nobject without having to even consult the object store, so BUG(),\nnot die(), is very much appropriate here. OK.\n\n> +\t} else {\n> \t\tnew_tree = repo_get_commit_tree(the_repository,\n> \t\t\t\t\t\tnew_branch_info->commit);\n> +\t\tif (!new_tree)\n> +\t\t\treturn error(_(\"unable to read tree %s\"),\n> +\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n\nWe can help translators by enclosing %s inside a pair of parentheses.\n\n $ git grep -h 'msgid .*unable to read tree' po | sort | uniq -c\n 18 msgid \"unable to read tree (%s)\"\n\nFYI, the message with \"(%s)\" is shared by four places; there is one\ninstance of the message without parentheses added very recently that\nforced .po files to have both entries. We probably should unify them\nto use the one with more existing users.\n\n> @@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\t\t\toid_to_hex(old_commit_oid));\n> \n> \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n> -\t\tparse_tree(new_tree);\n> +\t\tif (parse_tree(new_tree) < 0)\n> +\t\t\texit(128);\n\nThere is another exit() in the same else clause this code is in, and\nupon failing to unpack_trees(), that call exits with 128. This\nparse_tree() is about preparing the input for that call, so it makes\nsense to exit with the same code. Excellent.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index c6357af9498..4410b55be98 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)\n> \ttree = parse_tree_indirect(&oid);\n> \tif (!tree)\n> \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n> -\tparse_tree(tree);\n> +\tif (parse_tree(tree) < 0)\n> +\t\texit(128);\n> \tinit_tree_desc(&t, tree->buffer, tree->size);\n> \tif (unpack_trees(1, &t, &opts) < 0)\n> \t\tdie(_(\"unable to checkout working tree\"));\n\nExactly the same reasoning applies, as die() exits with 128.\n\nWe may want to \"#define EXIT_DIE 128\" and use it in appropriate\nplaces to make such a reasoning/review easier (possibly an entry for\n#leftoverbits)?\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 781af2e206c..0723f06de7a 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)\n> \ttree = parse_tree_indirect(¤t_head->object.oid);\n> \tif (!tree)\n> \t\tdie(_(\"failed to unpack HEAD tree object\"));\n> -\tparse_tree(tree);\n> +\tif (parse_tree(tree) < 0)\n> +\t\texit(128);\n> \tinit_tree_desc(&t, tree->buffer, tree->size);\n> \tif (unpack_trees(1, &t, &opts))\n> \t\texit(128); /* We've already reported the error, finish dying */\n\nDitto.\n\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 8196ca9dd85..5923ed36893 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n> \tcache_tree_free(&the_index.cache_tree);\n> \tfor (i = 0; i < nr_trees; i++) {\n> \t\tstruct tree *tree = trees[i];\n> -\t\tparse_tree(tree);\n> +\t\tif (parse_tree(tree) < 0)\n> +\t\t\treturn 128;\n> \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n> \t}\n> \tif (unpack_trees(nr_trees, t, &opts))\n\nDitto. After the post-context we also return 128.\n\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 4b018d20e3b..f030f57f4e9 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n> \n> \tif (reset_type == MIXED || reset_type == HARD) {\n> \t\ttree = parse_tree_indirect(oid);\n> +\t\tif (!tree) {\n> +\t\t\terror(_(\"unable to read tree %s\"), oid_to_hex(oid));\n> +\t\t\tgoto out;\n> +\t\t}\n> \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n> \t}\n\nGood.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 641427ed410..c6508b64a5c 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n> \t\t\tstruct cache_tree_sub *sub;\n> \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n> \n> -\t\t\tif (!subtree->object.parsed)\n> -\t\t\t\tparse_tree(subtree);\n> +\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n> +\t\t\t\texit(128);\n> \t\t\tsub = cache_tree_sub(it, entry.path);\n> \t\t\tsub->cache_tree = cache_tree();\n\nThe cache_tree() used to be just an optimization mechanism, but\nthere is no other way than fully populating it to write a tree\nobject out of the index, so dying here is the only sensible thing to\ndo upon unparseable subtree. Otherwise we would end up silently\nwriting a bogus result. Good.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index e3beb0801b1..10d41bfd487 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)\n> \n> static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n> {\n> -\tparse_tree(tree);\n> +\tif (parse_tree(tree) < 0)\n> +\t\texit(128);\n> \tinit_tree_desc(desc, tree->buffer, tree->size);\n> }\n\nOK.\n\n> diff --git a/merge.c b/merge.c\n> index b60925459c2..14a7325859d 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,\n> \t\treturn -1;\n> \t}\n> \tfor (i = 0; i < nr_trees; i++) {\n> -\t\tparse_tree(trees[i]);\n> +\t\tif (parse_tree(trees[i]) < 0) {\n> +\t\t\trollback_lock_file(&lock_file);\n> +\t\t\treturn -1;\n> +\t\t}\n> \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n> \t}\n\nThis handles the error in the same way as the other case earlier\nwhere any of the tree-ish failed to load. OK.\n\n> diff --git a/reset.c b/reset.c\n> index 48da0adf851..a93fdbc12e3 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n> \t}\n> \n> \ttree = parse_tree_indirect(oid);\n> +\tif (!tree) {\n> +\t\tret = error(_(\"unable to read tree %s\"), oid_to_hex(oid));\n> +\t\tgoto leave_reset_head;\n> +\t}\n\nOK, but the _(\"unable to read tree (%s)\") comment applies here, too.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index d584cac8ed9..407473bab28 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,\n> \to.show_rename_progress = 1;\n> \n> \thead_tree = parse_tree_indirect(head);\n> +\tif (!head_tree)\n> +\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(head));\n> \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n> \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n\nDitto.\n\n> @@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,\n> \t}\n> \n> \ttree = parse_tree_indirect(&oid);\n> +\tif (!tree)\n> +\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(&oid));\n\nDitto.\n\n> \tprime_cache_tree(r, r->index, tree);\n> \n> \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n\nLooking good, modulo the additional translator burden.\n\nThanks.\n"},{"id":"489180","messageId":"xmqqle7ce5vy.fsf@gitster.g","threadId":"60855","inReplyTo":"91dc4ccd04e3a6cc50ed389edb6814e1e7a0c4dc.1708612605.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/5] cache-tree: avoid an unnecessary check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T18:00:17Z","receivedAt":"2024-02-22T18:00:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The first thing the `parse_tree()` function does is to return early if\n> the tree has already been parsed. Therefore we do not need to guard the\n> `parse_tree()` call behind a check of that flag.\n>\n> As of time of writing, there are no other instances of this in Git's\n> code bases: whenever the `parsed` flag guards a `parse_tree()` call, it\n> guards more than just that call.\n>\n> Suggested-by: Patrick Steinhardt \n> Signed-off-by: Johannes Schindelin \n> ---\n> cache-tree.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index c6508b64a5c..78d6ba92853 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,\n> \t\t\tstruct cache_tree_sub *sub;\n> \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n> \n> -\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n> +\t\t\tif (parse_tree(subtree) < 0)\n> \t\t\t\texit(128);\n> \t\t\tsub = cache_tree_sub(it, entry.path);\n> \t\t\tsub->cache_tree = cache_tree();\n\nObviously makes sense.\nI see no need for further comments. Will queue.\n\nThanks.\n\n"},{"id":"489181","messageId":"xmqqcysoe4ty.fsf@gitster.g","threadId":"60855","inReplyTo":"xmqqttm0fly9.fsf@gitster.g","subject":"Re: [PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T18:23:05Z","receivedAt":"2024-02-22T18:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success 'error out on missing blob objects' '\n>> +\techo 1 | git hash-object -w --stdin >blob1 &&\n>> +\techo 2 | git hash-object -w --stdin >blob2 &&\n>> +\techo 3 | git hash-object -w --stdin >blob3 &&\n>> +\tprintf \"100644 blob $(cat blob1)\\tblob\\n\" | git mktree >tree1 &&\n>> +\tprintf \"100644 blob $(cat blob2)\\tblob\\n\" | git mktree >tree2 &&\n>> +\tprintf \"100644 blob $(cat blob3)\\tblob\\n\" | git mktree >tree3 &&\n>> +\tgit init --bare missing-blob.git &&\n>> +\tcat blob1 blob3 tree1 tree2 tree3 |\n>> +\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n>> +\ttest_must_fail git --git-dir=missing-blob.git >actual 2>err \\\n>> +\t\tmerge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&\n>> +\ttest_grep \"unable to read blob object $(cat blob2)\" err &&\n>> +\ttest_must_be_empty actual\n>> +'\n>\n> It would have been even easier to see that blob2 is what we expect\n> to be complained about, if you listed all objects and filtered blob2\n> out, but the number of objects involved here is so small, a \"cat\" of\n> all objects we want to keep is OK here.\n\nJust FYI for anybody reading from the sideline.\n\n\tgit cat-file --unordered \\\n\t\t--batch-check=\"%(objectname)\" --batch-all-objects |\n\tgrep -v $(cat blob2) |\n\tgit pack-objects ...\n\nwould be a compact way to say \"Give me a pack that lets me easily\nsimulate what happens in a repository identical to the current one,\nif it lacked object \"blob2\", without having to enumerate what object\nwe want to include in the test repository.\n\nAgain, I think in this particular case, it is easy enough to see in\nthe \"blob1 blob3 tree1 tree2 tree3\" enumeration what is missing, so\nthe way the patch is written is fine.\n\nThanks.\n"},{"id":"489224","messageId":"883087b8-b013-7b30-5485-719a1c310608@gmx.de","threadId":"60855","inReplyTo":"xmqqplwoe5yv.fsf@gitster.g","subject":"Re: [PATCH v3 4/5] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-23T08:33:38Z","receivedAt":"2024-02-23T08:33:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nthank you so much for reviewing!\n\nOn Thu, 22 Feb 2024, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +\t} else {\n> > \t\tnew_tree = repo_get_commit_tree(the_repository,\n> > \t\t\t\t\t\tnew_branch_info->commit);\n> > +\t\tif (!new_tree)\n> > +\t\t\treturn error(_(\"unable to read tree %s\"),\n> > +\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n>\n> We can help translators by enclosing %s inside a pair of parentheses.\n>\n> $ git grep -h 'msgid .*unable to read tree' po | sort | uniq -c\n> 18 msgid \"unable to read tree (%s)\"\n\nRight. I had used\n\n $ git grep 'unable to read tree .*%s' | sed -n 's/.*_(\"\\([^\"]*\\).*/\\1/p' | sort | uniq -c\n 11 unable to read tree %s\n 3 unable to read tree (%s)\n\nonly to realize that the 11 were the ones I added.🤦 Re-running the same\ncommand on v2.43.0 reports only the 3 parenthesized ones.\n\nI've fixed those error messages in v4, and also added a patch to adjust\nthe one error message that I imitated (and to mark it for translation).\nThis patch might be slightly controversial because, in what is probably\nonly a hypothetical scenario, users might have scripted around `git\nbisect` to parse error messages _and_ special-cased the \"unable to read\ntree\" message to be able to deal with the missing tree in a programmatical\nmanner. I might be misjudging the likelihood for something like that, but\nI vividly remember the, ahem, \"exciting times\" I had after 7560f547e61\n(treewide: correct several \"up-to-date\" to \"up to date\", 2017-08-23). If\nthat patch is too concerning, I am open to dropping it.\n\nCiao,\nJohannes\n"},{"id":"489225","messageId":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v3.git.1708612605.gitgitgadget@gmail.com","subject":"[PATCH v4 0/6] merge-tree: handle missing objects correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:19Z","receivedAt":"2024-02-23T08:34:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently looked into issues where git merge-tree calls returned bogus data\n(in one instance returning an empty tree for non-empty merge parents). By\nthe time I had a look at the corresponding repository, the issue was no\nlonger reproducible, but a closer look at the code combined with some manual\nexperimenting turned up the fact that missing tree objects aren't handled as\nerrors by git merge-tree.\n\nWhile at it, I added a commit on top that tries to catch all remaining\nunchecked parse_tree() calls.\n\nThis patch series is based on 5f43cf5b2e4 (merge-tree: accept 3 trees as\narguments, 2024-01-28) (the original tip commit of js/merge-tree-3-trees)\nbecause I introduced three unchecked parse_tree() calls in that topic.\n\nChanges since v3:\n\n * Aligned the translated error messages with pre-existing ones (sorry, I\n forgot to make that change in v2!).\n * Added a new commit at the end to mark the one error message for\n translation which I had imitated, after making it consistent with the\n remaining \"unable to read tree\" error messages.\n\nChanges since v2:\n\n * Fixed the new \"missing tree object\" test case in t4301 that succeeded for\n the wrong reason.\n * Adjusted the new \"missing blob object\" test case to avoid succeeding for\n the wrong reason.\n * Simplified the \"missing blob object\" test case.\n\nChanges since v1:\n\n * Simplified the test case, avoiding a subshell and a pipe in the process.\n * Added a patch to remove a superfluous subtree->object.parsed guard around\n a parse_tree(subtree) call.\n\nJohannes Schindelin (6):\n merge-tree: fail with a non-zero exit code on missing tree objects\n merge-ort: do check `parse_tree()`'s return value\n t4301: verify that merge-tree fails on missing blob objects\n Always check `parse_tree*()`'s return value\n cache-tree: avoid an unnecessary check\n fill_tree_descriptor(): mark error message for translation\n\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 16 +++++++++++-----\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++++++++++\n t/t6030-bisect-porcelain.sh | 2 +-\n tree-walk.c | 2 +-\n 15 files changed, 89 insertions(+), 17 deletions(-)\n\n\nbase-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1651\n\nRange-diff vs v3:\n\n 1: 11b9cd8c5da = 1: 11b9cd8c5da merge-tree: fail with a non-zero exit code on missing tree objects\n 2: f01f4eb011b = 2: f01f4eb011b merge-ort: do check `parse_tree()`'s return value\n 3: e82fdf7fbcb = 3: e82fdf7fbcb t4301: verify that merge-tree fails on missing blob objects\n 4: 9e4dc94ef03 ! 4: 5942c27f439 Always check `parse_tree*()`'s return value\n @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op\n \t\tnew_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\tnew_branch_info->commit);\n +\t\tif (!new_tree)\n -+\t\t\treturn error(_(\"unable to read tree %s\"),\n ++\t\t\treturn error(_(\"unable to read tree (%s)\"),\n +\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n +\t}\n \tif (opts->discard_changes) {\n @@ builtin/checkout.c: static void setup_new_branch_info_and_source_tree(\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n +\t\tif (!*source_tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(rev));\n ++\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(rev));\n \t} else {\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t\t*source_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t new_branch_info->commit);\n +\t\tif (!*source_tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"),\n ++\t\t\tdie(_(\"unable to read tree (%s)\"),\n +\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n \t}\n }\n @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n \t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n \t\tbase_tree = parse_tree_indirect(&base_oid);\n +\t\tif (!base_tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&base_oid));\n ++\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&base_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n \t\tparent1_tree = parse_tree_indirect(&head_oid);\n +\t\tif (!parent1_tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&head_oid));\n ++\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&head_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n \t\tparent2_tree = parse_tree_indirect(&merge_oid);\n +\t\tif (!parent2_tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&merge_oid));\n ++\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&merge_oid));\n \n \t\topt.ancestor = merge_base;\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\n @@ builtin/reset.c: static int reset_index(const char *ref, const struct object_id\n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n +\t\tif (!tree) {\n -+\t\t\terror(_(\"unable to read tree %s\"), oid_to_hex(oid));\n ++\t\t\terror(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n +\t\t\tgoto out;\n +\t\t}\n \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o\n \tif (result->clean >= 0) {\n \t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n +\t\tif (!result->tree)\n -+\t\t\tdie(_(\"unable to read tree %s\"),\n ++\t\t\tdie(_(\"unable to read tree (%s)\"),\n +\t\t\t oid_to_hex(&working_tree_oid));\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n @@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts\n \n \ttree = parse_tree_indirect(oid);\n +\tif (!tree) {\n -+\t\tret = error(_(\"unable to read tree %s\"), oid_to_hex(oid));\n ++\t\tret = error(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n +\t\tgoto leave_reset_head;\n +\t}\n +\n @@ sequencer.c: static int do_recursive_merge(struct repository *r,\n \n \thead_tree = parse_tree_indirect(head);\n +\tif (!head_tree)\n -+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(head));\n ++\t\treturn error(_(\"unable to read tree (%s)\"), oid_to_hex(head));\n \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n \n @@ sequencer.c: static int do_reset(struct repository *r,\n \n \ttree = parse_tree_indirect(&oid);\n +\tif (!tree)\n -+\t\treturn error(_(\"unable to read tree %s\"), oid_to_hex(&oid));\n ++\t\treturn error(_(\"unable to read tree (%s)\"), oid_to_hex(&oid));\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n 5: 91dc4ccd04e = 5: 7e5e84a4e7c cache-tree: avoid an unnecessary check\n -: ----------- > 6: ee2fcee5a10 fill_tree_descriptor(): mark error message for translation\n\n-- \ngitgitgadget\n"},{"id":"489226","messageId":"11b9cd8c5da5e6792ce940ea29d2e93e57731555.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 1/6] merge-tree: fail with a non-zero exit code on missing tree objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:20Z","receivedAt":"2024-02-23T08:34:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `git merge-tree` encounters a missing tree object, it should error\nout and not continue quietly as if nothing had happened.\n\nHowever, as of time of writing, `git merge-tree` _does_ continue, and\nthen offers the empty tree as result.\n\nLet's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 7 ++++---\n t/t4301-merge-tree-write-tree.sh | 11 +++++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6491070d965..c37fc035f13 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,\n \tinfo.data = opt;\n \tinfo.show_all_errors = 1;\n \n-\tparse_tree(merge_base);\n-\tparse_tree(side1);\n-\tparse_tree(side2);\n+\tif (parse_tree(merge_base) < 0 ||\n+\t parse_tree(side1) < 0 ||\n+\t parse_tree(side2) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(t + 0, merge_base->buffer, merge_base->size);\n \tinit_tree_desc(t + 1, side1->buffer, side1->size);\n \tinit_tree_desc(t + 2, side2->buffer, side2->size);\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 7d0fa74da74..908c9b540c8 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '\n \ttest_cmp with-commits with-trees\n '\n \n+test_expect_success 'error out on missing tree objects' '\n+\tgit init --bare missing-tree.git &&\n+\tgit rev-list side3 >list &&\n+\tgit rev-parse side3^: >>list &&\n+\tgit pack-objects missing-tree.git/objects/pack/side3-tree-is-missing actual 2>err &&\n+\ttest_grep \"Could not read $(git rev-parse $side3:)\" err &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"489227","messageId":"f01f4eb011b400faeff1c33934775a521dec7a3d.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 2/6] merge-ort: do check `parse_tree()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:21Z","receivedAt":"2024-02-23T08:34:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe previous commit fixed a bug where a missing tree was reported, but\nnot treated as an error.\n\nThis patch addresses the same issue for the remaining two callers of\n`parse_tree()`.\n\nThis change is not accompanied by a regression test because the code in\nquestion is only reached at the `checkout` stage, i.e. after the merge\nhas happened (and therefore the tree objects could only be missing if\nthe disk had gone bad in that short time window, or something similarly\ntricky to recreate in the test suite).\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c37fc035f13..79d9e18f63d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,\n \tunpack_opts.verbose_update = (opt->verbosity > 2);\n \tunpack_opts.fn = twoway_merge;\n \tunpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */\n-\tparse_tree(prev);\n+\tif (parse_tree(prev) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n-\tparse_tree(next);\n+\tif (parse_tree(next) < 0)\n+\t\treturn -1;\n \tinit_tree_desc(&trees[1], next->buffer, next->size);\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n-- \ngitgitgadget\n\n"},{"id":"489228","messageId":"e82fdf7fbcbf12fffdf4a720927c2f4f006068f8.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 3/6] t4301: verify that merge-tree fails on missing blob objects","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:22Z","receivedAt":"2024-02-23T08:34:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a problem where `merge-tree` would not fail on missing\ntree objects. Let's ensure that that problem does not occur with blob\nobjects (and won't, in the future, either).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 908c9b540c8..d4463a45706 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'error out on missing blob objects' '\n+\techo 1 | git hash-object -w --stdin >blob1 &&\n+\techo 2 | git hash-object -w --stdin >blob2 &&\n+\techo 3 | git hash-object -w --stdin >blob3 &&\n+\tprintf \"100644 blob $(cat blob1)\\tblob\\n\" | git mktree >tree1 &&\n+\tprintf \"100644 blob $(cat blob2)\\tblob\\n\" | git mktree >tree2 &&\n+\tprintf \"100644 blob $(cat blob3)\\tblob\\n\" | git mktree >tree3 &&\n+\tgit init --bare missing-blob.git &&\n+\tcat blob1 blob3 tree1 tree2 tree3 |\n+\tgit pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&\n+\ttest_must_fail git --git-dir=missing-blob.git >actual 2>err \\\n+\t\tmerge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&\n+\ttest_grep \"unable to read blob object $(cat blob2)\" err &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"489230","messageId":"5942c27f4390d5bafea0a63f71b1193a57e8e770.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 4/6] Always check `parse_tree*()`'s return value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:23Z","receivedAt":"2024-02-23T08:34:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOtherwise we may easily run into serious crashes: For example, if we run\n`init_tree_desc()` directly after a failed `parse_tree()`, we are\naccessing uninitialized data or trying to dereference `NULL`.\n\nNote that the `parse_tree()` function already takes care of showing an\nerror message. The `parse_tree_indirectly()` and\n`repo_get_commit_tree()` functions do not, therefore those latter call\nsites need to show a useful error message while the former do not.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 19 ++++++++++++++++---\n builtin/clone.c | 3 ++-\n builtin/commit.c | 3 ++-\n builtin/merge-tree.c | 6 ++++++\n builtin/read-tree.c | 3 ++-\n builtin/reset.c | 4 ++++\n cache-tree.c | 4 ++--\n merge-ort.c | 3 +++\n merge-recursive.c | 3 ++-\n merge.c | 5 ++++-\n reset.c | 5 +++++\n sequencer.c | 4 ++++\n 12 files changed, 52 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f02434bc155..9ab0901d629 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \tinit_checkout_metadata(&opts.meta, info->refname,\n \t\t\t info->commit ? &info->commit->object.oid : null_oid(),\n \t\t\t NULL);\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\treturn 128;\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tif (new_branch_info->commit)\n \t\t\tBUG(\"'switch --orphan' should never accept a commit as starting point\");\n \t\tnew_tree = parse_tree_indirect(the_hash_algo->empty_tree);\n-\t} else\n+\t\tif (!new_tree)\n+\t\t\tBUG(\"unable to read empty tree\");\n+\t} else {\n \t\tnew_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\tnew_branch_info->commit);\n+\t\tif (!new_tree)\n+\t\t\treturn error(_(\"unable to read tree (%s)\"),\n+\t\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n+\t}\n \tif (opts->discard_changes) {\n \t\tret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);\n \t\tif (ret)\n@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\toid_to_hex(old_commit_oid));\n \n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n-\t\tparse_tree(new_tree);\n+\t\tif (parse_tree(new_tree) < 0)\n+\t\t\texit(128);\n \t\ttree = new_tree;\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(\n \tif (!new_branch_info->commit) {\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(rev));\n \t} else {\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t\t*source_tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t new_branch_info->commit);\n+\t\tif (!*source_tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"),\n+\t\t\t oid_to_hex(&new_branch_info->commit->object.oid));\n \t}\n }\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af9498..4410b55be98 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)\n \ttree = parse_tree_indirect(&oid);\n \tif (!tree)\n \t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 781af2e206c..0723f06de7a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)\n \ttree = parse_tree_indirect(¤t_head->object.oid);\n \tif (!tree)\n \t\tdie(_(\"failed to unpack HEAD tree object\"));\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n \t\texit(128); /* We've already reported the error, finish dying */\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 2d4ce5b3886..3492a575a6c 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,\n \t\tif (repo_get_oid_treeish(the_repository, merge_base, &base_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), merge_base);\n \t\tbase_tree = parse_tree_indirect(&base_oid);\n+\t\tif (!base_tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&base_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch1, &head_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch1);\n \t\tparent1_tree = parse_tree_indirect(&head_oid);\n+\t\tif (!parent1_tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&head_oid));\n \t\tif (repo_get_oid_treeish(the_repository, branch2, &merge_oid))\n \t\t\tdie(_(\"could not parse as tree '%s'\"), branch2);\n \t\tparent2_tree = parse_tree_indirect(&merge_oid);\n+\t\tif (!parent2_tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&merge_oid));\n \n \t\topt.ancestor = merge_base;\n \t\tmerge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 8196ca9dd85..5923ed36893 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tcache_tree_free(&the_index.cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\n-\t\tparse_tree(tree);\n+\t\tif (parse_tree(tree) < 0)\n+\t\t\treturn 128;\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4b018d20e3b..fd36fc5bd95 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n+\t\tif (!tree) {\n+\t\t\terror(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n+\t\t\tgoto out;\n+\t\t}\n \t\tprime_cache_tree(the_repository, the_repository->index, tree);\n \t}\n \ndiff --git a/cache-tree.c b/cache-tree.c\nindex 641427ed410..c6508b64a5c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed)\n-\t\t\t\tparse_tree(subtree);\n+\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 79d9e18f63d..910ba38ff05 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \n \tif (result->clean >= 0) {\n \t\tresult->tree = parse_tree_indirect(&working_tree_oid);\n+\t\tif (!result->tree)\n+\t\t\tdie(_(\"unable to read tree (%s)\"),\n+\t\t\t oid_to_hex(&working_tree_oid));\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e3beb0801b1..10d41bfd487 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)\n \n static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)\n {\n-\tparse_tree(tree);\n+\tif (parse_tree(tree) < 0)\n+\t\texit(128);\n \tinit_tree_desc(desc, tree->buffer, tree->size);\n }\n \ndiff --git a/merge.c b/merge.c\nindex b60925459c2..14a7325859d 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,\n \t\treturn -1;\n \t}\n \tfor (i = 0; i < nr_trees; i++) {\n-\t\tparse_tree(trees[i]);\n+\t\tif (parse_tree(trees[i]) < 0) {\n+\t\t\trollback_lock_file(&lock_file);\n+\t\t\treturn -1;\n+\t\t}\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \ndiff --git a/reset.c b/reset.c\nindex 48da0adf851..080bcb6d656 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \t}\n \n \ttree = parse_tree_indirect(oid);\n+\tif (!tree) {\n+\t\tret = error(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n+\t\tgoto leave_reset_head;\n+\t}\n+\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..33d12b2ffd1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,\n \to.show_rename_progress = 1;\n \n \thead_tree = parse_tree_indirect(head);\n+\tif (!head_tree)\n+\t\treturn error(_(\"unable to read tree (%s)\"), oid_to_hex(head));\n \tnext_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);\n \tbase_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);\n \n@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,\n \t}\n \n \ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\treturn error(_(\"unable to read tree (%s)\"), oid_to_hex(&oid));\n \tprime_cache_tree(r, r->index, tree);\n \n \tif (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)\n-- \ngitgitgadget\n\n"},{"id":"489229","messageId":"7e5e84a4e7c5733c7f229a7713713d44406ad573.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 5/6] cache-tree: avoid an unnecessary check","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:24Z","receivedAt":"2024-02-23T08:34:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe first thing the `parse_tree()` function does is to return early if\nthe tree has already been parsed. Therefore we do not need to guard the\n`parse_tree()` call behind a check of that flag.\n\nAs of time of writing, there are no other instances of this in Git's\ncode bases: whenever the `parsed` flag guards a `parse_tree()` call, it\nguards more than just that call.\n\nSuggested-by: Patrick Steinhardt \nSigned-off-by: Johannes Schindelin \n---\n cache-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c6508b64a5c..78d6ba92853 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(r, &entry.oid);\n \n-\t\t\tif (!subtree->object.parsed && parse_tree(subtree) < 0)\n+\t\t\tif (parse_tree(subtree) < 0)\n \t\t\t\texit(128);\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n-- \ngitgitgadget\n\n"},{"id":"489231","messageId":"ee2fcee5a10f5977df95656188ae2cc8052e818b.1708677266.git.gitgitgadget@gmail.com","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"[PATCH v4 6/6] fill_tree_descriptor(): mark error message for translation","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-23T08:34:25Z","receivedAt":"2024-02-23T08:34:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere is an error message in that function to report a missing tree; In\ncontrast to three other, similar error messages, it is not marked for\ntranslation yet.\n\nMark it for translation, and while at it, make the error message\nconsistent with the others by enclosing the SHA in parentheses.\n\nThis requires a change to t6030 which expects the previous format of the\ncommit message. Theoretically, this could present problems with existing\nscripts that use `git bisect` and parse its output (because Git does not\nprovide other means for callers to discern between error conditions).\nHowever, this is unlikely to matter in practice because the most common\ncourse of action to deal with fatal corruptions is to report the error\nmessage to the user and exit, rather than trying to do something with\nthe reported SHA of the missing tree.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6030-bisect-porcelain.sh | 2 +-\n tree-walk.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 2a5b7d8379c..58f3d9c675e 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -872,7 +872,7 @@ test_expect_success 'broken branch creation' '\n \n echo \"\" > expected.ok\n cat > expected.missing-tree.default < writes:\n\n> Right. I had used\n>\n> $ git grep 'unable to read tree .*%s' | sed -n 's/.*_(\"\\([^\"]*\\).*/\\1/p' | sort | uniq -c\n> 11 unable to read tree %s\n> 3 unable to read tree (%s)\n>\n> only to realize that the 11 were the ones I added.🤦 Re-running the same\n> command on v2.43.0 reports only the 3 parenthesized ones.\n\nI think we probably should discuss what format is the easiest to\nunderstand and most logical to readers, and unify to such a format,\nnot necessarily the current majority, in the longer term. But for\nnow, let's pick one that costs less to unify to.\n\nWe may also want to cast a wider net to make things consistent. For\nexample, we learn that the parenthesized one is not necessarily more\nprevalent in a larger picture.\n\nLet me annotate the output from this command:\n\n $ git grep -h -E \\\n -e '(unable to|not) (read|find|acccess) (blob|tree|commit|tag) .*%s' master po/ |\n sort -u\n\n1. msgid \"cannot find commit %s (%s)\"\n\n The first one is a textual refname, the next one is oid-to-hex.\n\n2. msgid \"cannot read blob %s for path %s\"\n\n The first one is oid-to-hex, the other is a pathname.\n\n3. msgid \"could not find commit %s\"\n\n oid-to-hex (in commit-graph.c)\n\n4. msgid \"could not read commit message of %s\"\n\n oid-to-hex (in sequencer.c)\n\n5. msgid \"could not read commit message: %s\"\n\n This is irrelevant to the topic, as %s is for strerror().\n\n6. msgid \"unable to access commit %s\"\n\n oid-to-hex (in builtin/pull.c)\n\n7. msgid \"unable to read commit message from '%s'\"\n\n This message is in po/ but it seems that it no longer is used\n anywhere in the source.\n\n8. msgid \"unable to read tree %s\"\n9. msgid \"unable to read tree (%s)\"\n\n We know about these two already.\n\nWe seem to just use unadorned %s for many messages when talking\nabout commit objects, some are inside a pair of 'single quotes',\nWhen we are giving a long hexadecimal string, especially without\ndoing any abbreviation, I personally think it is a waste to enclose\nit in any punctuation pair, so if I were to vote today, I would\nprobably support standardizing on \"tree %s\", \"blob %s\", etc., but I\nthink that is just a personal preference (not even a \"taste\" thing).\n\nIn any case, all of the above is clearly outside the scope of this\nseries.\n"},{"id":"489251","messageId":"xmqq4jdzt4yw.fsf@gitster.g","threadId":"60855","inReplyTo":"pull.1651.v4.git.1708677266.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/6] merge-tree: handle missing objects correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-23T18:23:19Z","receivedAt":"2024-02-23T18:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> I recently looked into issues where git merge-tree calls returned bogus data\n> (in one instance returning an empty tree for non-empty merge parents). By\n> the time I had a look at the corresponding repository, the issue was no\n> longer reproducible, but a closer look at the code combined with some manual\n> experimenting turned up the fact that missing tree objects aren't handled as\n> errors by git merge-tree.\n\nLooking good. Will replace. Thanks.\n\n"}]}