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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 23, 2025, 03:12 UTC
Message-ID: <xmqqikdxriw3.fsf@gitster.g>
In-Reply-To: <replay_die_descr.140@msgid.xyz>

kristofferhaugsbakk@fastmail.com writes:

Show 10 quoted lines

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