From: Siddharth Asthana Date: Fri, 06 Mar 2026 16:20:24 GMT Subject: Re: [PATCH v3 2/2] replay: add --revert mode to reverse commit changes Message-ID: <45bf9ad0-418d-44ee-b06c-d2b546f54b7f@gmail.com> In-Reply-To: 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