From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Fri, 02 Jan 2026 11:11:29 GMT
Subject: Re: [PATCH v2 3/5] replay: die descriptively when invalid commit-ish is given
Message-ID: <c488a180-b840-43df-a593-4dac6b7f00d2@app.fastmail.com>
In-Reply-To: <CABPp-BH1b3rHi96qXLQwQRX6g7POmqYLKyAc=_1UsWmfiWsGFg@mail.gmail.com>

On Tue, Dec 30, 2025, at 23:52, Elijah Newren wrote:
>>[snip]
>> @@ -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 */

Okay, I will change to keeping this updated comment at this line but
remove the if-block. And I will remove the moved comment:

    if (!commit->parents) /* FIXME: Should handle replaying down to root commit */
        die(_("replaying down to root commit is not supported yet!"));

Specifically I will remove the if-block on this patch/commit and make
another patch for both renaming the comment and the “replaying down”
die-statement.

>
> 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.

Okay. With options from git-rev-list(1) like `--root` this mode makes sense.

>
>>         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 ?

“replaying down from”? Not “replaying from”?

>
>
>>[snip]