From: kristofferhaugsbakk@fastmail.com Date: Mon, 05 Jan 2026 19:53:16 GMT Subject: [PATCH v3 0/6] replay: die descriptively when invalid commit-ish Message-ID: In-Reply-To: From: Kristoffer Haugsbakk 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 v1) Somewhat unrelated to this change ... § Changes in v3 Apply review feedback from Elijah. See patches for details. • Patch 1: More terse function name • Patch 2: Improve commit message • Patch 3: Improve commit message: fix outdated function name mention • Patch 4: [new] Apply code comment/error message tweaks § Link to v2 https://lore.kernel.org/git/V2_CV_replay_die_descr.17b@msgid.xyz/ Kristoffer Haugsbakk (6): 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: improve code comment and die message 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, 79 insertions(+), 62 deletions(-) Interdiff against v2: diff --git a/builtin/replay.c b/builtin/replay.c index ca5a14de4c7..dc46c921667 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -156,16 +156,16 @@ static void get_ref_information(struct repository *repo, free(fullname); } } -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) +static void set_up_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) { struct ref_info rinfo; get_ref_information(repo, cmd_info, &rinfo); if (!rinfo.positive_refexprs) @@ -347,13 +347,15 @@ int cmd_replay(int argc, "'%s' bit in 'struct rev_info' will be forced"), "simplify_history"); revs.simplify_history = 0; } - populate_for_onto_or_advance_mode(repo, &revs.cmdline, - onto_name, &advance_name, - &onto, &update_refs); + set_up_replay_mode(repo, &revs.cmdline, + onto_name, &advance_name, + &onto, &update_refs); + + /* FIXME: Should allow replaying commits with the first as a root commit */ if (prepare_revision_walk(&revs) < 0) { ret = error(_("error preparing revisions")); goto cleanup; } @@ -366,12 +368,12 @@ int cmd_replay(int argc, while ((commit = get_revision(&revs))) { const struct name_decoration *decoration; khint_t pos; int hr; - if (!commit->parents) /* FIXME: Should handle replaying down to root commit */ - die(_("replaying down to root commit is not supported yet!")); + if (!commit->parents) + die(_("replaying down from 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, onto, &merge_opt, &result); diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index c0c59ae6938..d10c01506f1 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -78,11 +78,11 @@ test_expect_success 'option --onto or --advance is mandatory' ' 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' ' - echo "fatal: replaying down to root commit is not supported yet!" >expect && + echo "fatal: replaying down from 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' ' Range-diff against v2: 1: 314ba49dd2f ! 1: 6d785c6b45f replay: remove dead code and rearrange @@ Commit message *advance_name = xstrdup_or_null(last_key); - Let’s also rename the function since we do not determine - the replay mode here. We simply populate data structures. + Let’s also rename the function since we do not determine the + replay mode here. We just set up `*onto` and refs to update. Note that there might be more dead code caused by this *guess mode*. We only concern ourselves with this function for now. @@ Commit message ## Notes (series) ## + v3: + + Use more terse function rename.[1] Also tweak commit message based on + this change. + + 🔗 1: https://lore.kernel.org/git/CABPp-BEJV1XG62_hn_OiZ9q9S3jsyTP0VdOEzS4pME2rrkKFrg@mail.gmail.com/ + v2: [new] See the link in the commit message. @@ builtin/replay.c: static void get_ref_information(struct repository *repo, - 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) ++static void set_up_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) { struct ref_info rinfo; @@ builtin/replay.c: int cmd_replay(int argc, - determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name, - &onto, &update_refs); -+ populate_for_onto_or_advance_mode(repo, &revs.cmdline, -+ onto_name, &advance_name, -+ &onto, &update_refs); ++ set_up_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!"); 2: 976d336adef ! 2: 7f9aac28792 replay: find *onto only after testing for ref name @@ Commit message Let’s try to find the ref and only after that try to peel to as a commit-ish. - Also add a regression test to protect this error-order from future + Also add a regression test to protect this error order from future modifications. Suggested-by: Junio C Hamano @@ Commit message ## Notes (series) ## + v3: + + Don’t use a hyphen in “error-order” since that can be confusing.[1] + + 🔗 1: https://lore.kernel.org/git/CABPp-BE13K1QB42YLv3mLzB9+jUgkMtHNmbs_XWoTsbv2zSYog@mail.gmail.com/ + v2: [new] Fallout of v1. Needs to be moved so that the new error message does not @@ Notes (series) See: https://lore.kernel.org/git/xmqqpl85pb7k.fsf@gitster.g/ ## builtin/replay.c ## -@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo, +@@ builtin/replay.c: static void set_up_replay_mode(struct repository *repo, if (!*advance_name) BUG("expected either onto_name or *advance_name in this function"); @@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repositor if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), &oid, &fullname, 0) == 1) { free(*advance_name); -@@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repository *repo, +@@ builtin/replay.c: static void set_up_replay_mode(struct repository *repo, } else { die(_("argument to --advance must be a reference")); } 3: a7a7e1e720e ! 3: 88544dcad3e replay: die descriptively when invalid commit-ish is given @@ Commit message Going backwards from this point: - 1. `onto` is `NULL` from `determine_replay_mode`; + 1. `onto` is `NULL` from `set_up_replay_mode`; 2. that function in turn calls `peel_committish`; and 3. here we return `NULL` if `repo_get_oid` fails. @@ Commit message ## Notes (series) ## + v3: + + Update commit message since it uses the outdated function name.[1] + + 🔗 1: https://lore.kernel.org/git/CABPp-BH1b3rHi96qXLQwQRX6g7POmqYLKyAc=_1UsWmfiWsGFg@mail.gmail.com/#t + v2: Let’s use a slightly longer subject line in the commit message so that it @@ builtin/replay.c: static const char *short_commit_name(struct repository *repo, 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, +@@ builtin/replay.c: static void set_up_replay_mode(struct repository *repo, die_for_incompatible_opt2(!!onto_name, "--onto", !!*advance_name, "--advance"); if (onto_name) { @@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repositor 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, +@@ builtin/replay.c: static void set_up_replay_mode(struct repository *repo, } else { die(_("argument to --advance must be a reference")); } @@ builtin/replay.c: static void populate_for_onto_or_advance_mode(struct repositor 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); + 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!"); -- ++ /* FIXME: Should handle replaying down to root commit */ + 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 'argument to --advance must be a reference' ' -: ----------- > 4: 2e149c41634 replay: improve code comment and die message 4: 9700630d95e = 5: 35cce78b469 replay: die if we cannot parse object 5: 5e1b9205df5 ! 6: cd87a11f96b t3650: add more regression tests for failure conditions @@ t/t3650-replay-basics.sh: test_expect_success '--onto with invalid commit-ish' ' +' + +test_expect_success 'no base or negative ref gives no-replaying down to root error' ' -+ echo "fatal: replaying down to root commit is not supported yet!" >expect && ++ echo "fatal: replaying down from root commit is not supported yet!" >expect && + test_must_fail git replay --onto=topic1 topic2 2>actual && + test_cmp expect actual +' base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed -- 2.52.0.383.gb1c58d6b301