From: kristofferhaugsbakk@fastmail•com
To: git@vger•kernel.org
Cc: Kristoffer Haugsbakk <code@khaugsbakk•name>,
christian.couder@gmail•com, newren@gmail•com,
Siddharth Asthana <siddharthasthana31@gmail•com>,
Phillip Wood <phillip.wood@dunelm•org.uk>
Subject: [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given
Date: Tue, 30 Dec 2025 16:01:49 +0100 [thread overview]
Message-ID: <V2_replay_die_descr.17e@msgid.xyz> (raw)
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
next prev parent reply other threads:[~2025-12-30 15:03 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-22 22:04 [PATCH 0/2] replay: die descriptively when invalid commit-ish kristofferhaugsbakk
2025-12-22 22:04 ` [PATCH 1/2] " kristofferhaugsbakk
2025-12-23 3:12 ` Junio C Hamano
2025-12-23 10:52 ` Phillip Wood
2025-12-23 13:41 ` Junio C Hamano
2025-12-30 14:30 ` Kristoffer Haugsbakk
2025-12-22 22:04 ` [PATCH 2/2] t3650: add more regression tests for failure conditions kristofferhaugsbakk
2025-12-23 10:58 ` Phillip Wood
2025-12-30 14:33 ` Kristoffer Haugsbakk
2025-12-23 3:16 ` [PATCH 0/2] replay: die descriptively when invalid commit-ish Junio C Hamano
2025-12-30 14:33 ` Kristoffer Haugsbakk
2025-12-24 3:03 ` Elijah Newren
2025-12-30 14:31 ` Kristoffer Haugsbakk
2025-12-30 15:01 ` [PATCH v2 0/5] " kristofferhaugsbakk
2025-12-30 15:01 ` [PATCH v2 1/5] replay: remove dead code and rearrange kristofferhaugsbakk
2025-12-30 22:50 ` Elijah Newren
2025-12-30 23:37 ` Junio C Hamano
2026-01-02 9:51 ` Kristoffer Haugsbakk
2025-12-30 15:01 ` [PATCH v2 2/5] replay: find *onto only after testing for ref name kristofferhaugsbakk
2025-12-30 22:51 ` Elijah Newren
2025-12-30 15:01 ` kristofferhaugsbakk [this message]
2025-12-30 22:52 ` [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given Elijah Newren
2026-01-02 11:11 ` Kristoffer Haugsbakk
2025-12-30 15:01 ` [PATCH v2 4/5] replay: die if we cannot parse object kristofferhaugsbakk
2025-12-30 15:01 ` [PATCH v2 5/5] t3650: add more regression tests for failure conditions kristofferhaugsbakk
2025-12-30 22:53 ` [PATCH v2 0/5] replay: die descriptively when invalid commit-ish Elijah Newren
2026-01-05 19:53 ` [PATCH v3 0/6] " kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 1/6] replay: remove dead code and rearrange kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 2/6] replay: find *onto only after testing for ref name kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 3/6] replay: die descriptively when invalid commit-ish is given kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 4/6] replay: improve code comment and die message kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 5/6] replay: die if we cannot parse object kristofferhaugsbakk
2026-01-05 19:53 ` [PATCH v3 6/6] t3650: add more regression tests for failure conditions kristofferhaugsbakk
2026-01-06 23:12 ` [PATCH v3 0/6] replay: die descriptively when invalid commit-ish Elijah Newren
2026-01-07 3:56 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=V2_replay_die_descr.17e@msgid.xyz \
--to=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=siddharthasthana31@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox