Re: [PATCH v3 2/2] replay: add --revert mode to reverse commit changes

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 26/02/26 20:15, Phillip Wood wrote:
> Hi Siddharth
> 
> On 18/02/2026 23:42, Siddharth Asthana wrote:
>> @@ -42,6 +42,25 @@ The history is replayed on top of the <branch> and 
>> <branch> is updated to
>>   point at the tip of the resulting history. This is different from 
>> `--onto`,
>>   which uses the target only as a starting point without updating it.
>> +--revert <branch>::
>> +    Starting point at which to create the reverted commits; must be a
>> +    branch name.
>> ++
>> +When `--revert` is specified, the commits in the revision range are 
>> reverted
>> +(their changes are undone) and the reverted commits are created on 
>> top of
>> +<branch>. The <branch> is then updated to point at the new commits. 
>> This is
>> +the same as running `git revert <revision-range>` but does not update 
>> the
>> +working tree.
>> ++
>> +The commit messages follow `git revert` conventions: they are 
>> prefixed with
>> +"Revert" and include "This reverts commit <hash>." When reverting a 
>> commit
>> +whose message starts with "Revert", the new message uses "Reapply" 
>> instead.
>> +Unlike cherry-pick which preserves the original author, revert 
>> commits use
>> +the current user as the author, matching the behavior of `git revert`.
>> ++
>> +This option is mutually exclusive with `--onto` and `--advance`. It 
>> is also
>> +incompatible with `--contained` (which is a modifier for `--onto` only).
> 
> We seem to have lost
> 
>      NOTE: For reverting an entire merge request as a single commit
>      (rather than commit-by-commit), consider using `git merge-tree
>      --merge-base $TIP HEAD $BASE` which can avoid unnecessary merge
>      conflicts.
> 
> from V2 which is a shame.


Yeah, I dropped it during the v3 cleanup when I was trimming the example 
text. will  add it back.

> 
> I do think we should seriously consider reverting commits in the reverse 
> order that they were created (i.e. do not set '--reverse' when setting 
> up the rev-list options) to reduce the likely-hood of conflicts when 
> reverting a sequence of commits.


Good catch. sequencer.c does exactly this in prepare_revs() -- it only 
sets reverse for REPLAY_PICK, not REPLAY_REVERT, so git revert processes 
newest-first.

The complication in replay is that pick_regular_commit() chains commits 
through mapped_commit(base, onto). With oldest-first, the parent is 
always already in replayed_commits so the chain works. With 
newest-first, the parent hasn't been processed yet and mapped_commit() 
falls back to onto -- so each revert be independently based on the 
original branch tip instead of chaining.

The fix is straightforward: for revert mode, pass last_commit instead of 
onto as the fallback in the main loop:

	pick_regular_commit(repo, commit, replayed_commits,
                         mode == REPLAY_MODE_REVERT ? last_commit : onto,
                         &merge_opt, &result, mode);

That way each revert builds on the previous one regardless of walk 
order. I will do this in v4 together with skipping the reverse=1 
override for revert mode.

> 
>> @@ -152,6 +172,15 @@ all commits they have since `base`, playing them 
>> on top of
>>   `origin/main`. These three branches may have commits on top of `base`
>>   that they have in common, but that does not need to be the case.
>> +To revert commits on a branch:
>> +
>> +------------
>> +$ git replay --revert main main~2..main
> 
> It might be more realistic to revert some commits from a different 
> branch, for example
> 
>    git replay --revert main topic~2..topic


Makes sense. v2 had `git replay --revert main feature~2..feature` for 
this reason but I simplified it in v3. I will go back to something like 
your example:

	git replay --revert main topic2..topic


> 
>> +static void set_up_branch_mode(struct repository *repo,
>> +                   char **branch_name,
>> +                   const char *option_name,
>> +                   struct ref_info *rinfo,
>> +                   struct commit **onto)
>> [...]
>> +    if (rinfo->positive_refexprs > 1)
>> +        die(_("cannot %s target with multiple sources because 
>> ordering would be ill-defined"),
>> +            option_name + 2); /* skip "--" prefix */
> 
> This is a bit of a nasty hack as it stuffs an English word into the 
> middle of a translated sentence. Using the option name as below might be 
> nicer


Agreed, will use your suggested form:

	die(_("'%s' cannot be used with multiple revision ranges "
           "because the ordering would be ill-defined"), option_name);


> 
>      die(_("'%s' cannot be used with multiple revision ranges because 
> the ordering would be ill defined", option_name);
>> @@ -226,25 +269,46 @@ static struct commit *pick_regular_commit(struct 
>> repository *repo,
>>  [...]
>> -    /* Drop commits that become empty */
>> -    if (oideq(&replayed_base_tree->object.oid, &result->tree- 
>> >object.oid) &&
>> +    /* Drop commits that become empty (only for picks) */
> 
> Why? What's the advantage in creating empty revert commits?


Consistency with git revert, which doesn't silently drop empty reverts 
either -- it stops and asks the user to deal with it. Since replay is 
non-interactive and can't prompt, I kept them rather than silently 
dropping, to avoid hiding that something unexpected happened.

That being said, I don't feel strong about it. If you think dropping is 
the better default for replay, I am  happy to change it. Or we could 
error out (exit code 1) like we do for conflicts?


Thanks,
Siddharth


> 
>> +    if (mode == REPLAY_MODE_PICK &&
>> +        oideq(&replayed_base_tree->object.oid, &result->tree- 
>> >object.oid) &&
> 
> Thanks
> 
> Phillip