From: Elijah Newren Date: Tue, 30 Dec 2025 22:52:07 GMT Subject: Re: [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given Message-ID: In-Reply-To: On Tue, Dec 30, 2025 at 7:03 AM wrote: > > From: Kristoffer Haugsbakk > > 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`; `determine_replay_mode` no longer exists due to your new patch 1. > 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=` (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 > --- > > 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 > > 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!"); > - Removing the `if` makes sense given the current code, but I wonder if we should keep a corrected FIXME here: /* FIXME: Should allow replaying commits with the first as a root commit */ This is out-of-scope for this series, but behind that FIXME... I'm guessing the user would specify to cherry-pick onto NULL via something like git replay --root A..B which would translate into making `onto` be NULL, and mean that the first commit after A would be a root commit. Similarly the user could be allowed to do something like git replay --advance new-empty-branch A..B where new-empty-branch doesn't yet point to a commit, this would also result in `onto` being NULL, and start new-empty-branch by cherry-picking some commits into it. > 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!")); I wonder if I should have written s/to/from/ here ? > 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