Re: [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given
On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail.com> wrote:
Show 10 quoted lines
>
> 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`;
`determine_replay_mode` no longer exists due to your new patch 1.
Show 122 quoted lines
> 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!");
> -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.
Show 14 quoted lines
> 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 ?
Show 28 quoted lines
> 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.g08704017180Elijah Newren <newren@gmail.com>