Bug 1504967 - HG: Rebase after all amends r=glob

Summary:
Instead of rebasing after each commit is ammended we leave it to the
last moment. In the `finalize` method we iterate over all commits and
rebase them on top of the parent.

Reviewers: glob

Reviewed By: glob

Bug #: 1504967

Differential Revision: https://phabricator.services.mozilla.com/D12437

moz-phab

@@ -813,24 +813,30 @@ class Mercurial(Repository):
         commit["node"] = node
         commit["name"] = "%s:%s" % (rev, node[:12])
 
+    def _get_successor(self, node):
+        """Get the successor of the commit represented by its node.
+
+        Returns: a tuple containing rev and node"""
+        hg_log = self.hg_out(
+            ["log"]
+            + ["-T", "{rev} {node}\n"]
+            + ["--hidden"]
+            + ["-r", "successors(%s) and not obsolete()" % node]
+        )
+        if not hg_log:
+            return None, None
+
+        # Not sure the best way to handle multiple successors, so just bail out.
+        if len(hg_log) > 1:
+            raise Error("Multiple successors found for %s, unable to continue" % node)
+
+        return hg_log[0].split(" ", 1)
+
     def refresh_commit_stack(self, commits):
         """Update all commits to point to their superseded commit."""
         for commit in commits:
-            hg_log = self.hg_out(
-                ["log"]
-                + ["-T", "{rev} {node}\n"]
-                + ["--hidden"]
-                + ["-r", "successors(%s) and not obsolete()" % commit["node"]]
-            )
-            if hg_log:
-                # Not sure the best way to handle multiple successors, so just bail out.
-                if len(hg_log) > 1:
-                    raise Error(
-                        "Multiple successors found for %s, unable to continue"
-                        % commit["node"]
-                    )
-
-                (rev, node) = hg_log[0].split(" ", 1)
+            (rev, node) = self._get_successor(commit["node"])
+            if rev and node:
                 self._refresh_commit(commit, node, rev)
 
         self.revset = "%s::%s" % (commits[0]["node"], commits[-1]["node"])
@@ -901,6 +907,22 @@ class Mercurial(Repository):
             self.checkout(node)
             self.hg(["commit", "--amend", "--logfile", temp_f.name])
 
+    def finalize(self, commits):
+        """Rebase stack children commits if needed."""
+        # Currently we do all rebases in `amend_commit` if the evolve extension
+        # is not installed.
+        if not self.use_evolve:
+            return
+
+        parent = None
+        for commit in commits:
+            if parent:
+                self.rebase_commit(commit, parent)
+                (rev, node) = self._get_successor(commit["node"])
+                if rev and node:
+                    self._refresh_commit(commit, node, rev)
+            parent = commit
+
     def amend_commit(self, commit, commits):
         updated_body = "%s\n%s" % (commit["title"], commit["body"])
         current_body = self.hg_out(
@@ -934,10 +956,7 @@ class Mercurial(Repository):
             # Amend and refresh the stack to get the new commit node.
             self._amend_commit_body(commit["node"], updated_body)
             self.refresh_commit_stack(commits)
-
-            # Rebase children atop new commit node.
-            if first_child:
-                self.rebase_commit(first_child, commit)
+            # Rebase will happen in `finalize`
 
         elif not first_child and not post_stack_children:
             # Without evolve things are much more exciting.
@@ -2073,9 +2092,9 @@ def submit(repo, args):
 
     # Cleanup (eg. strip nodes) and refresh to ensure the stack is right for the
     # final showing.
+    repo.finalize(commits)
     repo.cleanup()
     repo.refresh_commit_stack(commits)
-    repo.finalize(commits)
 
     logger.warning("\nCompleted")
     show_commit_stack(repo, commits, show_rev_urls=True)

tests/conftest.py

@@ -8,7 +8,7 @@
 )
 
 
-@pytest.fixture()
+@pytest.fixture
 @mock.patch("mozphab.Git.git_out")
 @mock.patch("mozphab.Git._get_current_head")
 @mock.patch("mozphab.Config")
@@ -30,3 +30,29 @@ def git(
     m_git_get_current_head.return_value = "branch"
     m_git_git_out.return_value = ["user.email=email"]
     return mozphab.Git("x")
+
+
+@pytest.fixture
+@mock.patch("mozphab.Mercurial.hg_out")
+@mock.patch("mozphab.Config")
+@mock.patch("mozphab.os.path")
+@mock.patch("mozphab.which")
+@mock.patch("mozphab.Repository._phab_url")
+def hg(m_repository_phab_url, m_which, m_os_path, m_config, m_hg_hg_out, safe_environ):
+    m_os_path.join = os.path.join
+    m_os_path.exists.return_value = True
+    m_which.return_value = True
+    m_os_path.isfile.return_value = False
+    m_hg_hg_out.side_effect = [
+        "Mercurial Distributed SCM (version 4.7.1)",
+        ["ui.username=username", "extensions.evolve="],
+    ]
+    return mozphab.Mercurial("x")
+
+
+@pytest.fixture
+def safe_environ(monkeypatch):
+    # Make sure we preserve the system defaults.
+    monkeypatch.setattr(os, "environ", os.environ.copy())
+    # Disable logging to keep the testrunner output clean
+    monkeypatch.setattr(mozphab, "init_logging", mock.MagicMock())

tests/test_hg.py

@@ -0,0 +1,55 @@
+import imp
+import mock
+import os
+import pytest
+
+mozphab = imp.load_source(
+    "mozphab", os.path.join(os.path.dirname(__file__), os.path.pardir, "moz-phab")
+)
+
+
+@mock.patch("mozphab.Mercurial.hg_out")
+def test_get_successor(m_hg_hg_out, hg):
+    m_hg_hg_out.return_value = []
+    assert (None, None) == hg._get_successor("x")
+
+    m_hg_hg_out.return_value = ["1 abcde"]
+    assert ["1", "abcde"] == hg._get_successor("x")
+
+    m_hg_hg_out.return_value = ["a", "b"]
+    with pytest.raises(mozphab.Error):
+        hg._get_successor("x")
+
+
+@mock.patch("mozphab.Mercurial._get_successor")
+@mock.patch("mozphab.Mercurial.rebase_commit")
+def test_finalize(m_hg_rebase, m_hg_get_successor, hg):
+    commits = [
+        dict(rev="1", node="aaa"),
+        dict(rev="2", node="bbb"),
+        dict(rev="3", node="ccc"),
+    ]
+
+    m_hg_get_successor.return_value = (None, None)
+    hg.finalize(commits[:])
+    assert m_hg_rebase.call_count == 2
+    assert m_hg_rebase.call_args_list == [
+        mock.call(dict(rev="2", node="bbb"), dict(rev="1", node="aaa")),
+        mock.call(dict(rev="3", node="ccc"), dict(rev="2", node="bbb"))
+    ]
+
+    m_hg_get_successor.side_effect = [(None, None), ("4", "ddd")]
+    hg.finalize(commits)
+    assert m_hg_get_successor.called_once_with(dict(rev="3", node="ccc"), "ddd", "4")
+    assert commits == [
+        dict(rev="1", node="aaa"),
+        dict(rev="2", node="bbb"),
+        dict(rev="3", node="ddd", name="4:ddd"),
+    ]
+
+
+@mock.patch("mozphab.Mercurial.rebase_commit")
+def test_finalize_no_evolve(m_hg_rebase, hg):
+    hg.use_evolve = False
+    hg.finalize([dict(rev="1", node="aaa"), dict(rev="2", node="bbb")])
+    assert m_hg_rebase.not_called()

tests/test_integration.py

@@ -21,18 +21,14 @@ def hg(*args):
 
 
 @pytest.fixture
-def in_process(monkeypatch):
+def in_process(monkeypatch, safe_environ):
     """Set up an environment to run moz-phab within the current process."""
     # Make sure other tests didn't leak and mess up the module-level
     # global variables :/
     monkeypatch.setattr(mozphab, "IS_WINDOWS", False)
     monkeypatch.setattr(mozphab, "DEBUG", False)
     monkeypatch.setattr(mozphab, "HAS_ANSI", False)
     # Constructing the Mercurial() object modifies os.environ for all tests.
-    # Make sure we preserve the system defaults.
-    monkeypatch.setattr(os, "environ", os.environ.copy())
-    # Disable logging to keep the testrunner output clean
-    monkeypatch.setattr(mozphab, "init_logging", mock.MagicMock())
     # Disable update checking.  It modifies the program on disk which we do /not/ want
     # to do during a test run.
     monkeypatch.setattr(mozphab, "check_for_updates", mock.MagicMock())