[PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given

From: kristofferhaugsbakk@fastmail.com <kristofferhaugsbakk@fastmail.com>
Date: Dec 30, 2025, 15:01 UTC
Message-ID: <V2_replay_die_descr.17e@msgid.xyz>
In-Reply-To: <V2_CV_replay_die_descr.17b@msgid.xyz>

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with:

    fatal: Replaying down to root commit is not supported yet!

Going backwards from this point:

1. `onto` is `NULL` from `determine_replay_mode`;
2. that function in turn calls `peel_committish`; and
3. here we return `NULL` if `repo_get_oid` fails.

Let’s die immediately with a descriptive error message instead.

Doing this also provides us with a descriptive error if we “forget” to provide an argument to `--onto` (but we really do unintentionally):[1]

    $ git replay --onto ^main topic1
    fatal: '^main' is not a valid commit-ish

Note that the `--advance` case won’t be triggered in practice because of the “argument to --advance must be a reference” check (see the previous test, and commit).

† 1: The argument to `--onto` is mandatory and the option parser accepts
     both `--onto=<name>` (stuck form) and `--onto name`. The latter
     form makes it easy to unintentionally pass something to the option
     when you really meant to pass a positional argument.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    
    Let’s use a slightly longer subject line in the commit message so that it
    looks more like a full sentence (no dropped/implied words).[1]
    
    Also remove the test for `--advance` which is now wrong because of the
    previous commit/patch. And reword the commit message now that only `--onto`
    is relevant in practice.
    
    There was also feedback about *where* to give this error:[2]
    
    > 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").
    
    But I opted to keep the check here by using the new `mode` parameter to
    provide the context; it is either `--onto` or `--advance`.
    
    Also remove the “not supported yet” now that `*onto` cannot be `NULL` at
    this point. I wasn’t confident enough to pull the trigger on that in the
    first round. But after Elijah’s comment[3] I feel like I understand the code
    well enough.
    
    Also change the test to use printf since it’s only one line. That will be
    in line with the later commits/patches here.
    
    🔗 1: https://lore.kernel.org/git/xmqqecolrip7.fsf@gitster.g/
    🔗 2: https://lore.kernel.org/git/xmqqikdxriw3.fsf@gitster.g/
    🔗 3: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/

 builtin/replay.c         | 15 +++++++--------
 t/t3650-replay-basics.sh |  7 +++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/replay.c b/builtin/replay.c
index 35813140e99..07a6767ade1 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -25,17 +25,19 @@ static const char *short_commit_name(struct repository *repo,
 {
 	return repo_find_unique_abbrev(repo, &commit->object.oid,
 				       DEFAULT_ABBREV);
 }
 
-static struct commit *peel_committish(struct repository *repo, const char *name)
+static struct commit *peel_committish(struct repository *repo,
+				      const char *name,
+				      const char *mode)
 {
 	struct object *obj;
 	struct object_id oid;
 
 	if (repo_get_oid(repo, name, &oid))
-		return NULL;
+		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
 	obj = parse_object(repo, &oid);
 	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
 						  OBJ_COMMIT);
 }
 
@@ -170,11 +172,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
 		die(_("need some commits to replay"));
 
 	die_for_incompatible_opt2(!!onto_name, "--onto",
 				  !!*advance_name, "--advance");
 	if (onto_name) {
-		*onto = peel_committish(repo, onto_name);
+		*onto = peel_committish(repo, onto_name, "--onto");
 		if (rinfo.positive_refexprs <
 		    strset_get_size(&rinfo.positive_refs))
 			die(_("all positive revisions given must be references"));
 		*update_refs = xcalloc(1, sizeof(**update_refs));
 		**update_refs = rinfo.positive_refs;
@@ -191,11 +193,11 @@ static void populate_for_onto_or_advance_mode(struct repository *repo,
 			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
 		}
-		*onto = peel_committish(repo, *advance_name);
+		*onto = peel_committish(repo, *advance_name, "--advance");
 		if (rinfo.positive_refexprs > 1)
 			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
 	}
 	strset_clear(&rinfo.negative_refs);
 	strset_clear(&rinfo.positive_refs);
@@ -349,13 +351,10 @@ int cmd_replay(int argc,
 
 	populate_for_onto_or_advance_mode(repo, &revs.cmdline,
 					  onto_name, &advance_name,
 					  &onto, &update_refs);
 
-	if (!onto) /* FIXME: Should handle replaying down to root commit */
-		die("Replaying down to root commit is not supported yet!");
-
 	if (prepare_revision_walk(&revs) < 0) {
 		ret = error(_("error preparing revisions"));
 		goto cleanup;
 	}
 
@@ -367,11 +366,11 @@ int cmd_replay(int argc,
 	while ((commit = get_revision(&revs))) {
 		const struct name_decoration *decoration;
 		khint_t pos;
 		int hr;
 
-		if (!commit->parents)
+		if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
 			die(_("replaying down to root commit is not supported yet!"));
 		if (commit->parents->next)
 			die(_("replaying merge commits is not supported yet!"));
 
 		last_commit = pick_regular_commit(repo, commit, replayed_commits,
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 7dea62f064f..d4399aa1662 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -56,10 +56,17 @@ test_expect_success 'argument to --advance must be a reference' '
 	oid=$(git rev-parse main) &&
 	test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
 	test_cmp expect actual
 '
 
+test_expect_success '--onto with invalid commit-ish' '
+	printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
+	printf "a valid commit-ish for --onto\n" >>expect &&
+	test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'using replay to rebase two branches, one on top of other' '
 	git replay --onto main topic1..topic2 >result &&
 
 	test_line_count = 1 result &&

-- 
2.52.0.10.g08704017180