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

[Phillip Wood <phillip.wood123@gmail.com>]

On 06/03/2026 05:28, Siddharth Asthana wrote:
> On 26/02/26 20:15, Phillip Wood wrote:
>> On 18/02/2026 23:42, Siddharth Asthana wrote:
> 
>> 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);

As we only allow a single range of commits with --revert that should work.

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

Great

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

So does "git cherry-pick" unless you pass --empty=drop or --empty=keep 
(I was surprised that "git revert" does not support --empty, that seems 
to be an oversight)

> Since replay is 
> non-interactive and can't prompt, I kept them rather than silently 
> dropping, to avoid hiding that something unexpected happened.

I don't think creating empty commits for revert is very helpful, when 
cherry-picking one could argue that the user may want to preserve the 
commit message (though I think that's unlikely in practice which is why 
we drop commits that become empty) but that does not apply to revert.

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

We don't error out when cherry-picking and so we shouldn't do that when 
reverting.

Thanks

Phillip

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