From: Siddharth Asthana Date: Fri, 06 Mar 2026 05:28:29 GMT Subject: Re: [PATCH v3 2/2] replay: add --revert mode to reverse commit changes Message-ID: <77fa95d9-3ea3-4a32-b8fa-22c05c048160@gmail.com> In-Reply-To: <405b0d34-c2ad-498d-93a1-2e7925ae11f1@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 and >> 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 :: >> +    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 >> +. The is then updated to point at the new commits. >> This is >> +the same as running `git revert ` but does not update >> the >> +working tree. >> ++ >> +The commit messages follow `git revert` conventions: they are >> prefixed with >> +"Revert" and include "This reverts commit ." 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