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.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 10:24 PM
Unknown Object (File)
Wed, Dec 18, 2:54 PM
Unknown Object (File)
Sun, Dec 15, 9:11 PM
Unknown Object (File)
Tue, Dec 10, 1:06 PM
Unknown Object (File)
Mon, Dec 9, 8:27 PM
Unknown Object (File)
Sun, Dec 8, 7:39 PM
Unknown Object (File)
Fri, Dec 6, 2:23 PM
Unknown Object (File)
Sun, Dec 1, 10:58 PM
Subscribers

Details

Summary

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

Mercurial:

  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

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

Event Timeline

cspeckmim added inline comments.
src/land/engine/ArcanistLandEngine.php
1260–1286

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.

1301

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.

src/land/engine/ArcanistMercurialLandEngine.php
848–854

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)

887–888

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
| /
C
|
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