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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2022, 07:26 UTC
Message-ID: <xmqq5yowolvs.fsf@gitster.g>
In-Reply-To: <20220302003613.15567-1-chooglen@google.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.

Show 7 quoted lines

> -		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.

Show 13 quoted lines

>  		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.