From a14b019d3c34864ff3e54b6da4e5405de053f49d Mon Sep 17 00:00:00 2001 From: Piotr Zalewa Date: Tue, 20 Nov 2018 16:37:15 +0100 Subject: [PATCH] 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 | 59 ++++++++++++++++++++++++++------------- tests/conftest.py | 28 ++++++++++++++++++- tests/test_hg.py | 55 ++++++++++++++++++++++++++++++++++++ tests/test_integration.py | 6 +--- 4 files changed, 122 insertions(+), 26 deletions(-) create mode 100644 tests/test_hg.py diff --git a/moz-phab b/moz-phab index 1e9953a..0563acc 100755 --- a/moz-phab +++ b/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) diff --git a/tests/conftest.py b/tests/conftest.py index 011b306..cd06f99 100644 --- a/tests/conftest.py +++ b/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()) diff --git a/tests/test_hg.py b/tests/test_hg.py new file mode 100644 index 0000000..ab73708 --- /dev/null +++ b/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() diff --git a/tests/test_integration.py b/tests/test_integration.py index 5873dc0..118ecdc 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -21,7 +21,7 @@ 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 :/ @@ -29,10 +29,6 @@ def in_process(monkeypatch): 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())