Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish

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

kristofferhaugsbakk@fastmail.com writes:

> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6172c8aacc9..175b64c5335 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -33,7 +33,7 @@ static struct commit *peel_committish(struct repository *repo, const char *name)
>  	struct object_id oid;
>  
>  	if (repo_get_oid(repo, name, &oid))
> -		return NULL;
> +		die(_("'%s' is not a valid commit-ish"), name);

This is after repo_get_oid() fails to turn the "name" into an oid.
The only thing we know about "name" is that it does not name an
object, but we want to get a commit-ish and the new message sounds
like a reasonable way to tell both of these two facts.

>  	obj = parse_object(repo, &oid);
>  	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
>  						  OBJ_COMMIT);

The previous parse_object() can return NULL, in which case
repo_peel_to_type() would also silently return NULL.

If obj is not NULL, repo_peel_to_type() would die with a descriptive
message when the thing does not peel to an object of the expected
type.

So the caller of this function still needs to be prepared for
receiving a NULL from here.

How many callers use this function?  I am wondering if it is better
to give a better message at the caller(s), rather than here, where
we lack context to tell something like "You gave string 'ource' as
the argument to the '--onto' option, but 'ource' does not name any
commit" (in other words, "for what our caller is trying to peel
<name> to a commit").