From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 23 Dec 2025 13:41:19 GMT
Subject: Re: [PATCH 1/2] replay: die descriptively when invalid commit-ish
Message-ID: <xmqqpl85pb7k.fsf@gitster.g>
In-Reply-To: <a5822145-5a5c-4a2b-84b9-7c061b3bab97@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

> There are only two callers so I think that is a good idea. If you give 
> an invalid commit name to "--advance" then it dies with
>
>      fatal: argument to --advance must be a reference
>
> so arguably we only need to check the return value when parsing "--onto"

So, in determine_replay_mode(), ...

	if (onto_name) {
		*onto = peel_committish(repo, onto_name);

... here is where we must see *onto is NULL and barf and then ...

		if (rinfo.positive_refexprs <
		    strset_get_size(&rinfo.positive_refs))
			die(_("all positive revisions given must be references"));
	} else if (*advance_name) {
		struct object_id oid;
		char *fullname = NULL;

		*onto = peel_committish(repo, *advance_name);

... for symmetry, we would want to do the same.  

In addition, we probably would want to let the following code that
uses the same *advance_name (and requires that it just not names a
commit-ish object, but is actually a ref, which is a different
requirement that is probably a bit tighter) ...

		if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
			     &oid, &fullname, 0) == 1) {
			free(*advance_name);
			*advance_name = fullname;
		} else {
			die(_("argument to --advance must be a reference"));
		}

... first, and then compute *onto after that by moving code a bit,
perhaps?

Thanks.