[PATCH v2 0/5] replay: die descriptively when invalid commit-ish
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
You get this error when you for example mistype the argument to `--onto`:
fatal: Replaying down to root commit is not supported yet!
Consider that you might not know yourself that you have mistyped something; then this looks even more puzzling. You might have given a range like `main..topic` but the command says that it would need to replay down to the root commit.
The only thing that’s happened though is that `NULL` has been interpreted in the wrong way.
Let’s instead die immediately when the real error happens, in other words when we can’t find the commit for the given commit-ish.
Also:
• Add more regression tests • Remove dead code • Slightly more robust object parsing (parse_object_or_die)
§ Dropped section (see previous)
Somewhat unrelated to this change ...
§ Changes in v2
In this version we remove some dead code (see previous section), improve tests and add a couple more, and improve the main “die descriptively” part, with two additional patches/commits to that end:
• replay: find *onto only after testing for ref name • replay: die if we cannot parse object
See the notes on each patch/commit for change details.
§ Link to v1
https://lore.kernel.org/git/CV_replay_die_descr.13f@msgid.xyz/
❦ ❦ ❦ ❦ ❦
(this means you can stop reading here)
§ How I Present Patch Series
Version: 2
The cover letter:
• Problem statement and solution
• (optional) Summary of what each commit/patch does
• Changes compared to the previous version
• Note that all versions are not included
• (optional) “CC”/“Cc” which explains all or parts of the CC list
• But some things are not noteworthy, like the standard practice of
including previous-round respondents
• Link to the previous version
• This is new. I thought it was redundant but some people (like me!)
use webapp email clients which are not that great for navigating
among tree-like email threads. So I was inspired by this practice
which many others already use.Then each commit/patch might have Git notes with a `series` namespace. These contain:
• Comments/questions to reviewers about my approach or statements made
in the commit message.
• May quote the commit message or emails (with links)
• Lines starting with `v<version>:` which introduce a changelog for that
version. Might have `v<num>: [new]` if this version is 2+ and the
patch/commit is new. Now, lately (Sep 2025) these have been written in
the “imperative mood”, like what is done for a commit message in this
(Git) project. A bit strange, given that most others seem to use the
more immediately natural past-tense. But on the other hand: how many
new contributors to this project use the wrong tense/mood in their
commit messages? The Git project rule is not “natural”. But I think
it’s better nonetheless and worth the effort.
• ... But this kind of changelog might also be conducive to a bullet
list of changes. So I might either skip the previous point, do only a
bullet point, or do both: a presentation and then the bullet points
summarizing the presentation.
• Note that all `v<version>:` are kept between versions, which is not
consistent with how I only have the “Changes” part for the previous
version in the cover letter.
• `v<version>:` are ordered newest-to-oldest.
Kristoffer Haugsbakk (5):
replay: remove dead code and rearrange
replay: find *onto only after testing for ref name
replay: die descriptively when invalid commit-ish is given
replay: die if we cannot parse object
t3650: add more regression tests for failure conditions
builtin/replay.c | 87 +++++++++++-----------------------------
t/t3650-replay-basics.sh | 54 +++++++++++++++++++++++++
2 files changed, 78 insertions(+), 63 deletions(-)
Interdiff against v1:
Show changes to 2 files +57 −88
builtin/replay.c, t/t3650-replay-basics.sh
diff --git a/builtin/replay.c b/builtin/replay.c
index 175b64c5335..ca5a14de4c7 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -25,18 +25,20 @@ 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))
- die(_("'%s' is not a valid commit-ish"), name);
- obj = parse_object(repo, &oid);
+ die(_("'%s' is not a valid commit-ish for %s"), name, mode);
+ obj = parse_object_or_die(repo, &oid, name);
return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
OBJ_COMMIT);
}
static char *get_author(const char *message)
@@ -154,89 +156,50 @@ static void get_ref_information(struct repository *repo,
free(fullname);
}
}
-static void determine_replay_mode(struct repository *repo,
- struct rev_cmdline_info *cmd_info,
- const char *onto_name,
- char **advance_name,
- struct commit **onto,
- struct strset **update_refs)
+static void populate_for_onto_or_advance_mode(struct repository *repo,
+ struct rev_cmdline_info *cmd_info,
+ const char *onto_name,
+ char **advance_name,
+ struct commit **onto,
+ struct strset **update_refs)
{
struct ref_info rinfo;
get_ref_information(repo, cmd_info, &rinfo);
if (!rinfo.positive_refexprs)
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"));
- } else if (*advance_name) {
+ *update_refs = xcalloc(1, sizeof(**update_refs));
+ **update_refs = rinfo.positive_refs;
+ memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
+ } else {
struct object_id oid;
char *fullname = NULL;
- *onto = peel_committish(repo, *advance_name);
+ if (!*advance_name)
+ BUG("expected either onto_name or *advance_name in this function");
+
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"));
}
+ *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"));
- } else {
- int positive_refs_complete = (
- rinfo.positive_refexprs ==
- strset_get_size(&rinfo.positive_refs));
- int negative_refs_complete = (
- rinfo.negative_refexprs ==
- strset_get_size(&rinfo.negative_refs));
- /*
- * We need either positive_refs_complete or
- * negative_refs_complete, but not both.
- */
- if (rinfo.negative_refexprs > 0 &&
- positive_refs_complete == negative_refs_complete)
- die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
- if (negative_refs_complete) {
- struct hashmap_iter iter;
- struct strmap_entry *entry;
- const char *last_key = NULL;
-
- if (rinfo.negative_refexprs == 0)
- die(_("all positive revisions given must be references"));
- else if (rinfo.negative_refexprs > 1)
- die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
- else if (rinfo.positive_refexprs > 1)
- die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
-
- /* Only one entry, but we have to loop to get it */
- strset_for_each_entry(&rinfo.negative_refs,
- &iter, entry) {
- last_key = entry->key;
- }
-
- free(*advance_name);
- *advance_name = xstrdup_or_null(last_key);
- } else { /* positive_refs_complete */
- if (rinfo.negative_refexprs > 1)
- die(_("cannot implicitly determine correct base for --onto"));
- if (rinfo.negative_refexprs == 1)
- *onto = rinfo.onto;
- }
- }
- if (!*advance_name) {
- *update_refs = xcalloc(1, sizeof(**update_refs));
- **update_refs = rinfo.positive_refs;
- memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
}
strset_clear(&rinfo.negative_refs);
strset_clear(&rinfo.positive_refs);
}
@@ -384,15 +347,13 @@ int cmd_replay(int argc,
"'%s' bit in 'struct rev_info' will be forced"),
"simplify_history");
revs.simplify_history = 0;
}
- determine_replay_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!");
+ populate_for_onto_or_advance_mode(repo, &revs.cmdline,
+ onto_name, &advance_name,
+ &onto, &update_refs);
if (prepare_revision_walk(&revs) < 0) {
ret = error(_("error preparing revisions"));
goto cleanup;
}
@@ -405,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 c543d55857b..c0c59ae6938 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -41,61 +41,72 @@ test_expect_success 'setup' '
git switch main &&
test_commit L &&
test_commit M &&
+ git switch --detach topic4 &&
+ test_commit N &&
+ test_commit O &&
+ git switch -c topic-with-merge topic4 &&
+ test_merge P O --no-ff &&
+ git switch main &&
+
git switch -c conflict B &&
test_commit C.conflict C.t conflict
'
test_expect_success 'setup bare' '
git clone --bare . bare
'
-test_expect_success '--onto with invalid commit-ish' '
- cat >expect <<-EOF &&
- fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
- EOF
- test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
+test_expect_success 'argument to --advance must be a reference' '
+ echo "fatal: argument to --advance must be a reference" >expect &&
+ oid=$(git rev-parse main) &&
+ test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
test_cmp expect actual
'
-test_expect_success '--advance with invalid commit-ish' '
- cat >expect <<-EOF &&
- fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
- EOF
- test_must_fail git replay --advance=refs/not-valid topic1..topic2 2>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 'option --onto or --advance is mandatory' '
- cat >expect <<-\EOF &&
- error: option --onto or --advance is mandatory
- EOF
- # First line is the error; rest is Usage
- test_must_fail git replay topic1..topic2 >&1 2>&1 |
- head -1 >actual &&
+ echo "error: option --onto or --advance is mandatory" >expect &&
+ test_might_fail git replay -h >>expect &&
+ test_must_fail git replay topic1..topic2 2>actual &&
test_cmp expect actual
'
test_expect_success 'no base or negative ref gives no-replaying down to root error' '
- cat >expect <<-\EOF &&
- fatal: replaying down to root commit is not supported yet!
- EOF
+ echo "fatal: replaying down to root commit is not supported yet!" >expect &&
test_must_fail git replay --onto=topic1 topic2 2>actual &&
test_cmp expect actual
'
test_expect_success 'options --advance and --contained cannot be used together' '
- cat >expect <<-EOF &&
- fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
- EOF
+ printf "fatal: options ${SQ}--advance${SQ} " >expect &&
+ printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
test_must_fail git replay --advance=main --contained \
topic1..topic2 2>actual &&
test_cmp expect actual
'
+test_expect_success 'cannot advance target ... ordering would be ill-defined' '
+ echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
+ test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'replaying merge commits is not supported yet' '
+ echo "fatal: replaying merge commits is not supported yet!" >expect &&
+ test_must_fail git replay --advance=main main..topic-with-merge 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 &&
Range-diff against v1:
-: ----------- > 1: 314ba49dd2f replay: remove dead code and rearrange
-: ----------- > 2: 976d336adef replay: find *onto only after testing for ref name
1: d49923de7c5 ! 3: a7a7e1e720e replay: die descriptively when invalid commit-ish
@@ Metadata
Author: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Commit message ##
- replay: die descriptively when invalid commit-ish
+ replay: die descriptively when invalid commit-ish is given
- Giving an invalid commit-ish to `--onto` or `--advance` makes
- git-replay(1) fail with:
+ Giving an invalid commit-ish to `--onto` makes git-replay(1) fail with:
fatal: Replaying down to root commit is not supported yet!
@@ Commit message
$ 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
@@ Commit message
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 ##
-@@ builtin/replay.c: static struct commit *peel_committish(struct repository *repo, const char *name)
+@@ builtin/replay.c: static const char *short_commit_name(struct repository *repo,
+ 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"), name);
++ 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);
+@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo,
+ 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"));
+@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo,
+ } 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"));
+ }
+@@ builtin/replay.c: int cmd_replay(int argc,
+ 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;
+@@ builtin/replay.c: int cmd_replay(int argc,
+ 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!"));
## t/t3650-replay-basics.sh ##
-@@ t/t3650-replay-basics.sh: test_expect_success 'setup bare' '
- git clone --bare . bare
+@@ t/t3650-replay-basics.sh: test_expect_success 'argument to --advance must be a reference' '
+ test_cmp expect actual
'
+test_expect_success '--onto with invalid commit-ish' '
-+ cat >expect <<-EOF &&
-+ fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
-+ EOF
++ 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 '--advance with invalid commit-ish' '
-+ cat >expect <<-EOF &&
-+ fatal: ${SQ}refs/not-valid${SQ} is not a valid commit-ish
-+ EOF
-+ test_must_fail git replay --advance=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 &&
-: ----------- > 4: 9700630d95e replay: die if we cannot parse object
2: 5aa5b96950e ! 5: 5e1b9205df5 t3650: add more regression tests for failure conditions
@@ Commit message
a few more since these are simple to write and remove if they become
obsolete.
+ Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ v2:
+
+ Improve test `option --onto or --advance is mandatory`. Phillip pointed out
+ that using a pipe loses the return value. Instead let’s test the whole
+ output by just appending `git replay -h` to `expect`.
+
+ Also “normalize” to just using echo/printf for the `expect` since these are
+ just oneliner errors.
+
+ Also add two more tests (at the end).
+
## t/t3650-replay-basics.sh ##
-@@ t/t3650-replay-basics.sh: test_expect_success '--advance with invalid commit-ish' '
+@@ t/t3650-replay-basics.sh: test_expect_success 'setup' '
+ test_commit L &&
+ test_commit M &&
+
++ git switch --detach topic4 &&
++ test_commit N &&
++ test_commit O &&
++ git switch -c topic-with-merge topic4 &&
++ test_merge P O --no-ff &&
++ git switch main &&
++
+ git switch -c conflict B &&
+ test_commit C.conflict C.t conflict
+ '
+@@ t/t3650-replay-basics.sh: test_expect_success '--onto with invalid commit-ish' '
test_cmp expect actual
'
+test_expect_success 'option --onto or --advance is mandatory' '
-+ cat >expect <<-\EOF &&
-+ error: option --onto or --advance is mandatory
-+ EOF
-+ # First line is the error; rest is Usage
-+ test_must_fail git replay topic1..topic2 >&1 2>&1 |
-+ head -1 >actual &&
++ echo "error: option --onto or --advance is mandatory" >expect &&
++ test_might_fail git replay -h >>expect &&
++ test_must_fail git replay topic1..topic2 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'no base or negative ref gives no-replaying down to root error' '
-+ cat >expect <<-\EOF &&
-+ fatal: replaying down to root commit is not supported yet!
-+ EOF
++ echo "fatal: replaying down to root commit is not supported yet!" >expect &&
+ test_must_fail git replay --onto=topic1 topic2 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'options --advance and --contained cannot be used together' '
-+ cat >expect <<-EOF &&
-+ fatal: options ${SQ}--advance${SQ} and ${SQ}--contained${SQ} cannot be used together
-+ EOF
++ printf "fatal: options ${SQ}--advance${SQ} " >expect &&
++ printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
+ test_must_fail git replay --advance=main --contained \
+ topic1..topic2 2>actual &&
+ test_cmp expect actual
+'
++
++test_expect_success 'cannot advance target ... ordering would be ill-defined' '
++ echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
++ test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
++ test_cmp expect actual
++'
++
++test_expect_success 'replaying merge commits is not supported yet' '
++ echo "fatal: replaying merge commits is not supported yet!" >expect &&
++ test_must_fail git replay --advance=main main..topic-with-merge 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 &&
base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
--
2.52.0.10.g08704017180
