Page MenuHomePhabricator

Update Mercurial's cascading of commit sets to rebase non-landed commits
Changes PlannedPublic

Authored by cspeckmim on Sep 23 2021, 3:09 AM.
Referenced Files
Unknown Object (File)
Sat, Apr 20, 5:48 PM
Unknown Object (File)
Wed, Apr 17, 8:35 PM
Unknown Object (File)
Thu, Apr 11, 8:54 AM
Unknown Object (File)
Sun, Apr 7, 11:40 AM
Unknown Object (File)
Wed, Apr 3, 11:30 AM
Unknown Object (File)
Mon, Apr 1, 1:57 AM
Unknown Object (File)
Sat, Mar 30, 10:14 AM
Unknown Object (File)
Thu, Mar 28, 5:08 PM



Fixes T13668

There are two issues which prevent cascading/rebasing of non-landed commits during a land,

  1. ArcanistLandEngine repeatedly called cascadeState() passing in the last commit set and not any intermediary sets, resulting in failing to cascade branches made off of intermediate sets/revisions.
  2. ArcanistMercurialLandEngine::cascadeState() unconditionally tried to rebase each child branch from a given set onto the last published commit.

To fix these,

  1. Updated ArcanistLandEngine to pass in each set that needs cascaded.
  2. For Mercurial repositories, whenever a merge is executed, keep a map of the old commit to the destination/squashed commit. During cascading, reference this map for two purposes:
    1. If a child branch from a commit set has a corresponding new commit from the squash, ignore/avoid cascading since it was part of the land process. Attempting to rebase this would result in rebase conflicts.
    2. For a child branch that does need cascaded, determine the appropriate destination commit so it rebases cleanly.
Test Plan


  1. I created 3 revisions, each with 2 commits and dependent on the previous revision.
  2. From the middle revision I created a new branch of changes bookmarked fest and created a new revision for it.
  3. I ran arc land test3 to land the newest/tip revision.
  4. I verified that the original 3 revisions were landed properly as well as their original state being cleaned up.
  5. I verified that the fest branch had been rebased onto the new commit for the middle revision.

TODO: same test with git

Diff Detail

rARC Arcanist
Lint Passed
Advicesrc/land/engine/ArcanistLandEngine.php:1269XHP16TODO Comment
Tests Passed
Build Status
Buildable 25571
Build 35370: Run Core Tests
Build 35369: arc lint + arc unit

Event Timeline

cspeckmim added inline comments.

The number of scope/blocks was tripping me up a little so I inverted this condition to break early allowing the rest of the code in this loop to be indented one level less. I don't feel too strong about this so if preferred I can revert the change.


Coincidentally someone ran into this issue yesterday evening completely unrelated to discussion in D21680#inline-60963. Fortunately we were able to resurrect their missing commits from .hg/strip-backup without too much issue.


I saw this in ArcanistGitLandEngine and felt a little jealous

Screen Shot 2021-09-22 at 11.20.24 PM.png (1×2 px, 224 KB)


Now that the entire commit translation is being tracked I think I can remove $obsolete_map here along with $this->rebasedActiveCommit. I'll look into this real quick.

Remove the $obsolete_map and $rebasedActiveCommit and just use $rebasedCommitMap

Marking Plan Changes until I test this out in Git

Just making a note that I did test git a while back and saw behavior that I wasn't expecting; I haven't had a chance to dig further into this. I'm not super familiar with how to visualize the commit graph in git to confirm this but I believe what happened is a graph that looked like this

A  B
| /
D  (master)

Running arc land A resulted in A and C being landed but instead of B being rebased onto the landed C I think it was rebased onto the landed A. I haven't checked the results without these changes to confirm yet but I do suspect the change to cascade multiple things caused this