[PATCH v2 0/5] replay: die descriptively when invalid commit-ish
- From
- kristofferhaugsbakk@fastmail.com <kristofferhaugsbakk@fastmail.com>
- Date
- Dec 30, 2025, 15:01 UTC
- Message-ID
- <V2_CV_replay_die_descr.17b@msgid.xyz>
- In-Reply-To
- <CV_replay_die_descr.13f@msgid.xyz>
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:
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