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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 06/03/26 21:22, Phillip Wood wrote:
> 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.


Right, will drop empties for revert too then.


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