Glen Choo <chooglen@xxxxxxxxxx> writes:
> Note that there are many other callsites that don't check for NULLs from
> parse_tree_indirect(), and some of which are fairly subtle. I wasn't
> confident in changing those, so I stayed on the conservative side and
> only changed the ones that I could get to segfault.
Thanks.
> - tree = parse_tree_indirect(old_branch_info->commit ?
> - &old_branch_info->commit->object.oid :
> - the_hash_algo->empty_tree);
> +
> + old_commit_oid = old_branch_info->commit ?
> + &old_branch_info->commit->object.oid :
> + the_hash_algo->empty_tree;
I guess this is done only so that you can use the object name in the
error message, which is fine.
> + tree = parse_tree_indirect(old_commit_oid);
> + if (!tree)
> + die(_("unable to parse commit %s"),
> + oid_to_hex(old_commit_oid));
"unable to parse commit" is a bit of a white lie. In fact, there is
nothing that makes oid_commit_oid the name of a commit object.
"unable to parse object '%s' as a tree" would be more technically
correct, but one random-looking hexadecimal string is almost
indistinguishable from another, and neither would be a very useful
message from the end user's point of view. I am wondering if we can
use old_branch_info to formulate something easier to understand for
the user. update_refs_for_switch() seems to compute old_desc as a
human readable name by using old_branch_info->name if available
before falling back to old_branch_info->commit object name, which
might be a source of inspiration.
> init_tree_desc(&trees[0], tree->buffer, tree->size);
> parse_tree(new_tree);
> tree = new_tree;
> diff --git a/builtin/clone.c b/builtin/clone.c
> index a572cda503..0aea177660 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -700,6 +700,8 @@
> init_checkout_metadata(&opts.meta, head, &oid, NULL);
>
> tree = parse_tree_indirect(&oid);
> + if (!tree)
> + die(_("unable to parse commit %s"), oid_to_hex(&oid));
If we follow the code path, we can positively tell that oid here has
always came from reading HEAD, so "unable to parse HEAD as a tree"
would be an easier version of the message, I think.
Thanks.
