From: Junio C Hamano Date: Tue, 23 Dec 2025 03:12:28 GMT Subject: Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish Message-ID: In-Reply-To: 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 to a commit").