Re: [PATCH] checkout, clone: die if tree cannot be parsed

[Junio C Hamano <gitster@pobox.com>]

Glen Choo <chooglen@google.com> 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.