Show 68 quoted lines
> 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.
Siddharth Asthana <siddharthasthana31@gmail.com>