Bug 1500188 - Change the API to create a try commit. r=releng-reviewers,sheehan,ahal

Currently, the caller potentially prepares files, add them to the VCS, and then calls the function to create the commit, does whatever it wants to do with it, then undoes it. With the ultimate goal of not doing local changes to the work tree to act on that try commit (for a push), this change in API makes it so that everything except what do do with the commit is handled by one function, that now takes paths and file contents for the files that were previously created by the caller. Differential Revision: https://phabricator.services.mozilla.com/D214214
2024-06-24 23:07:29 +00:00 · 2024-06-24 23:07:29 +00:00 · 1644cb07e3
--- a/python/mozversioncontrol/mozversioncontrol/init.py
+++ b/python/mozversioncontrol/mozversioncontrol/init.py
@ -8,9 +8,11 @@ import os
 import re
 import shutil
 import subprocess
+from contextlib import contextmanager
 from datetime import datetime
 from pathlib import Path
 from typing import (
+    Dict,
    Iterator,
    List,
    Optional,
@ -260,7 +262,12 @@ class Repository(object):
        """

    @abc.abstractmethod
-    def push_to_try(self, message, allow_log_capture=False):
+    def push_to_try(
+        self,
+        message: str,
+        changed_files: Dict[str, str] = {},
+        allow_log_capture: bool = False,
+    ):
        """Create a temporary commit, push it to try and clean it up
        afterwards.

@ -268,6 +275,9 @@ class Repository(object):
        extension is not installed. On git, MissingVCSExtension will be raised
        if git cinnabar is not present.

+        `changed_files` is a dict of file paths and their contents, see
+        `stage_changes`.
+
        If `allow_log_capture` is set to `True`, then the push-to-try will be run using
        Popen instead of check_call so that the logs can be captured elsewhere.
        """
@ -327,24 +337,43 @@ class Repository(object):
            )

    @abc.abstractmethod
-    def get_branch_nodes(self) -> List[str]:
+    def get_branch_nodes(self, head: Optional[str] = None) -> List[str]:
        """Return a list of commit SHAs for nodes on the current branch."""

    @abc.abstractmethod
    def get_commit_patches(self, nodes: str) -> List[bytes]:
        """Return the contents of the patch `node` in the VCS's standard format."""

+    @contextmanager
    @abc.abstractmethod
-    def create_try_commit(self, commit_message: str):
-        """Create a temporary try commit.
+    def try_commit(
+        self, commit_message: str, changed_files: Optional[Dict[str, str]] = None
+    ):
+        """Create a temporary try commit as a context manager.

        Create a new commit using `commit_message` as the commit message. The commit
        may be empty, for example when only including try syntax.
+
+        `changed_files` may contain a dict of file paths and their contents,
+        see `stage_changes`.
        """

-    @abc.abstractmethod
-    def remove_current_commit(self):
-        """Remove the currently checked out commit from VCS history."""
+    def stage_changes(self, changed_files: Dict[str, str]):
+        """Stage a set of file changes
+
+        `changed_files` is a dict that contains the paths of files to change or
+        create as keys and their respective contents as values.
+        """
+        paths = []
+        for path, content in changed_files.items():
+            full_path = Path(self.path) / path
+            full_path.parent.mkdir(parents=True, exist_ok=True)
+            with full_path.open("w") as fh:
+                fh.write(content)
+            paths.append(full_path)
+
+        if paths:
+            self.add_remove_files(*paths)

    @abc.abstractmethod
    def get_last_modified_time_for_file(self, path: Path):
@ -589,7 +618,15 @@ class HgRepository(Repository):
        except subprocess.CalledProcessError:
            raise MissingVCSExtension(extension)

-    def push_to_try(self, message, allow_log_capture=False):
+    def push_to_try(
+        self,
+        message: str,
+        changed_files: Dict[str, str] = {},
+        allow_log_capture: bool = False,
+    ):
+        if changed_files:
+            self.stage_changes(changed_files)
+
        try:
            cmd = (str(self._tool), "push-to-try", "-m", message)
            if allow_log_capture:
@ -616,12 +653,14 @@ class HgRepository(Repository):
        finally:
            self._run("revert", "-a")

-    def get_branch_nodes(self, base_ref: Optional[str] = None) -> List[str]:
+    def get_branch_nodes(
+        self, head: Optional[str] = None, base_ref: Optional[str] = None
+    ) -> List[str]:
        """Return a list of commit SHAs for nodes on the current branch."""
        if not base_ref:
            base_ref = self.base_ref

-        head_ref = self.head_ref
+        head_ref = head or self.head_ref

        return self._run(
            "log",
@ -661,17 +700,26 @@ class HgRepository(Repository):

        return patches

-    def create_try_commit(self, commit_message: str):
-        """Create a temporary try commit.
+    @contextmanager
+    def try_commit(
+        self, commit_message: str, changed_files: Optional[Dict[str, str]] = None
+    ):
+        """Create a temporary try commit as a context manager.

        Create a new commit using `commit_message` as the commit message. The commit
        may be empty, for example when only including try syntax.
+
+        `changed_files` may contain a dict of file paths and their contents,
+        see `stage_changes`.
        """
+        if changed_files:
+            self.stage_changes(changed_files)
+
        # Allow empty commit messages in case we only use try-syntax.
        self._run("--config", "ui.allowemptycommit=1", "commit", "-m", commit_message)

-    def remove_current_commit(self):
-        """Remove the currently checked out commit from VCS history."""
+        yield self.head_ref
+
        try:
            self._run("prune", ".")
        except subprocess.CalledProcessError:
@ -869,12 +917,16 @@ class GitRepository(Repository):
    def update(self, ref):
        self._run("checkout", ref)

-    def push_to_try(self, message, allow_log_capture=False):
+    def push_to_try(
+        self,
+        message: str,
+        changed_files: Dict[str, str] = {},
+        allow_log_capture: bool = False,
+    ):
        if not self.has_git_cinnabar:
            raise MissingVCSExtension("cinnabar")

-        self.create_try_commit(message)
-        try:
+        with self.try_commit(message, changed_files) as head:
            cmd = (
                str(self._tool),
                "-c",
@ -884,7 +936,7 @@ class GitRepository(Repository):
                "cinnabar.data=never",
                "push",
                "hg::ssh://hg.mozilla.org/try",
-                "+HEAD:refs/heads/branches/default/tip",
+                f"+{head}:refs/heads/branches/default/tip",
            )
            if allow_log_capture:
                self._push_to_try_with_log_capture(
@ -899,19 +951,17 @@ class GitRepository(Repository):
                )
            else:
                subprocess.check_call(cmd, cwd=self.path)
-        finally:
-            self.remove_current_commit()

    def set_config(self, name, value):
        self._run("config", name, value)

-    def get_branch_nodes(self) -> List[str]:
+    def get_branch_nodes(self, head: Optional[str] = None) -> List[str]:
        """Return a list of commit SHAs for nodes on the current branch."""
        remote_args = self.get_mozilla_remote_args()

        return self._run(
            "log",
-            "HEAD",
+            head or "HEAD",
            "--reverse",
            "--not",
            *remote_args,
@ -927,18 +977,32 @@ class GitRepository(Repository):
            for node in nodes
        ]

-    def create_try_commit(self, message: str):
-        """Create a temporary try commit.
+    @contextmanager
+    def try_commit(
+        self, commit_message: str, changed_files: Optional[Dict[str, str]] = None
+    ):
+        """Create a temporary try commit as a context manager.

        Create a new commit using `commit_message` as the commit message. The commit
        may be empty, for example when only including try syntax.
+
+        `changed_files` may contain a dict of file paths and their contents,
+        see `stage_changes`.
        """
+        if changed_files:
+            self.stage_changes(changed_files)
+
        self._run(
-            "-c", "commit.gpgSign=false", "commit", "--allow-empty", "-m", message
+            "-c",
+            "commit.gpgSign=false",
+            "commit",
+            "--allow-empty",
+            "-m",
+            commit_message,
        )

-    def remove_current_commit(self):
-        """Remove the currently checked out commit from VCS history."""
+        yield "HEAD"
+
        self._run("reset", "HEAD~")

    def get_last_modified_time_for_file(self, path: Path):
@ -1062,7 +1126,12 @@ class SrcRepository(Repository):
    def update(self, ref):
        pass

-    def push_to_try(self, message, allow_log_capture=False):
+    def push_to_try(
+        self,
+        message: str,
+        changed_files: Dict[str, str] = {},
+        allow_log_capture: bool = False,
+    ):
        pass

    def set_config(self, name, value):
--- a/python/mozversioncontrol/test/python.toml
+++ b/python/mozversioncontrol/test/python.toml
@ -7,8 +7,6 @@ subsuite = "mozversioncontrol"

 ["test_context_manager.py"]

-["test_create_try_commit.py"]
-
 ["test_get_branch_nodes.py"]

 ["test_get_commit_patches.py"]
@ -19,7 +17,7 @@ subsuite = "mozversioncontrol"

 ["test_push_to_try.py"]

-["test_remove_current_commit.py"]
+["test_try_commit.py"]

 ["test_update.py"]

--- a/python/mozversioncontrol/test/test_create_try_commit.py
+++ b/python/mozversioncontrol/test/test_create_try_commit.py
@ -1,45 +0,0 @@
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this,
-# file, You can obtain one at http://mozilla.org/MPL/2.0/.
-
-import mozunit
-
-from mozversioncontrol import get_repository_object
-
-STEPS = {
-    "hg": [
-        """
-        echo "{}" > try_task_config.json
-        hg add try_task_config.json
-        """,
-    ],
-    "git": [
-        """
-        echo "{}" > try_task_config.json
-        git add try_task_config.json
-        """,
-    ],
-}
-
-
-def test_create_try_commit(repo):
-    commit_message = "try commit message"
-    vcs = get_repository_object(repo.dir)
-
-    # Create a non-empty commit.
-    repo.execute_next_step()
-    vcs.create_try_commit(commit_message)
-    non_empty_commit_sha = vcs.head_ref
-
-    assert vcs.get_changed_files(rev=non_empty_commit_sha) == ["try_task_config.json"]
-
-    # Create an empty commit.
-    vcs.create_try_commit(commit_message)
-    empty_commit_sha = vcs.head_ref
-
-    # Commit should be created with no changed files.
-    assert vcs.get_changed_files(rev=empty_commit_sha) == []
-
-
-if __name__ == "__main__":
-    mozunit.main()
--- a/python/mozversioncontrol/test/test_push_to_try.py
+++ b/python/mozversioncontrol/test/test_push_to_try.py
@ -2,6 +2,7 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.

+import os
 import subprocess

 import mozunit
@ -17,22 +18,46 @@ def test_push_to_try(repo, monkeypatch):
    captured_commands = []

    def fake_run(*args, **kwargs):
-        captured_commands.append(args[0])
+        cmd = args[0]
+        captured_commands.append(cmd)
+        if os.path.basename(cmd[0]).startswith("hg") and cmd[1] == "--version":
+            return "version 6.7"

    monkeypatch.setattr(subprocess, "check_output", fake_run)
    monkeypatch.setattr(subprocess, "check_call", fake_run)

-    vcs.push_to_try(commit_message)
+    vcs.push_to_try(
+        commit_message,
+        {
+            "extra-file": "content",
+            "other/extra-file": "content2",
+        },
+    )
    tool = vcs._tool

    if repo.vcs == "hg":
        expected = [
+            (str(tool), "--version"),
+            (
+                str(tool),
+                "--config",
+                "extensions.automv=",
+                "addremove",
+                os.path.join(vcs.path, "extra-file"),
+                os.path.join(vcs.path, "other", "extra-file"),
+            ),
            (str(tool), "push-to-try", "-m", commit_message),
            (str(tool), "revert", "-a"),
        ]
    else:
        expected = [
            (str(tool), "cinnabar", "--version"),
+            (
+                str(tool),
+                "add",
+                os.path.join(vcs.path, "extra-file"),
+                os.path.join(vcs.path, "other", "extra-file"),
+            ),
            (
                str(tool),
                "-c",
--- a/python/mozversioncontrol/test/test_remove_current_commit.py
+++ b/python/mozversioncontrol/test/test_remove_current_commit.py
@ -1,44 +0,0 @@
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this
-# file, You can obtain one at http://mozilla.org/MPL/2.0/.
-
-import mozunit
-import pytest
-
-from mozversioncontrol import get_repository_object
-
-STEPS = {
-    "hg": [
-        """
-        echo "{}" > try_task_config.json
-        hg add try_task_config.json
-        hg commit -m "Try config commit"
-        """
-    ],
-    "git": [
-        """
-        echo "{}" > try_task_config.json
-        git add try_task_config.json
-        git commit -m "Try config commit"
-        """
-    ],
-}
-
-
-@pytest.mark.xfail(reason="Requires the Mercurial evolve extension.", strict=False)
-def test_remove_current_commit(repo):
-    vcs = get_repository_object(repo.dir)
-    initial_head_ref = vcs.head_ref
-
-    # Create a new commit.
-    repo.execute_next_step()
-
-    vcs.remove_current_commit()
-
-    assert (
-        vcs.head_ref == initial_head_ref
-    ), "Removing current commit should revert to previous head."
-
-
-if __name__ == "__main__":
-    mozunit.main()
--- a/python/mozversioncontrol/test/test_try_commit.py
+++ b/python/mozversioncontrol/test/test_try_commit.py
@ -0,0 +1,35 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this,
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+import mozunit
+import pytest
+
+from mozversioncontrol import get_repository_object
+
+
+@pytest.mark.xfail(reason="Requires the Mercurial evolve extension.", strict=False)
+def test_try_commit(repo):
+    commit_message = "try commit message"
+    vcs = get_repository_object(repo.dir)
+    initial_head_ref = vcs.head_ref
+
+    # Create a non-empty commit.
+    with vcs.try_commit(commit_message, {"try_task_config.json": "{}"}) as head:
+        assert vcs.get_changed_files(rev=head) == ["try_task_config.json"]
+
+    assert (
+        vcs.head_ref == initial_head_ref
+    ), "We should have reverted to previous head after try_commit"
+
+    # Create an empty commit.
+    with vcs.try_commit(commit_message) as head:
+        assert vcs.get_changed_files(rev=head) == []
+
+    assert (
+        vcs.head_ref == initial_head_ref
+    ), "We should have reverted to previous head after try_commit"
+
+
+if __name__ == "__main__":
+    mozunit.main()
--- a/tools/tryselect/lando.py
+++ b/tools/tryselect/lando.py
@ -35,10 +35,6 @@ from mozversioncontrol import (
    HgRepository,
 )

-from .task_config import (
-    try_config_commit,
-)
-
 TOKEN_FILE = (
    Path(get_state_dir(specific_to_topsrcdir=False)) / "lando_auth0_user_token.json"
 )
@ -74,7 +70,9 @@ def load_token_from_disk() -> Optional[dict]:
    return user_token


-def get_stack_info(vcs: SupportedVcsRepository) -> Tuple[str, List[str]]:
+def get_stack_info(
+    vcs: SupportedVcsRepository, head: Optional[str]
+) -> Tuple[str, List[str]]:
    """Retrieve information about the current stack for submission via Lando.

    Returns a tuple of the current public base commit as a Mercurial SHA,
@ -92,7 +90,7 @@ def get_stack_info(vcs: SupportedVcsRepository) -> Tuple[str, List[str]]:
    if isinstance(vcs, HgRepository):
        branch_nodes_kwargs["base_ref"] = base_commit

-    nodes = vcs.get_branch_nodes(**branch_nodes_kwargs)
+    nodes = vcs.get_branch_nodes(head, **branch_nodes_kwargs)
    if not nodes:
        raise ValueError("Could not find any commit hashes for submission.")
    elif len(nodes) == 1:
@ -395,7 +393,9 @@ class LandoAPI:
        return response_json


-def push_to_lando_try(vcs: SupportedVcsRepository, commit_message: str):
+def push_to_lando_try(
+    vcs: SupportedVcsRepository, commit_message: str, changed_files: dict
+):
    """Push a set of patches to Lando's try endpoint."""
    # Map `Repository` subclasses to the `patch_format` value Lando expects.
    PATCH_FORMAT_STRING_MAPPING = {
@ -419,9 +419,9 @@ def push_to_lando_try(vcs: SupportedVcsRepository, commit_message: str):
    # Get the time when the push was initiated, not including Auth0 login time.
    push_start_time = time.perf_counter()

-    with try_config_commit(vcs, commit_message):
+    with vcs.try_commit(commit_message) as head:
        try:
-            base_commit, patches = get_stack_info(vcs)
+            base_commit, patches = get_stack_info(vcs, head)
        except ValueError as exc:
            error_msg = "abort: error gathering patches for submission."
            print(error_msg)
--- a/tools/tryselect/push.py
+++ b/tools/tryselect/push.py
@ -8,7 +8,6 @@ import os
 import sys
 import traceback

-import six
 from mach.util import get_state_dir
 from mozbuild.base import MozbuildObject
 from mozversioncontrol import MissingVCSExtension, get_repository_object
@ -57,14 +56,6 @@ history_path = os.path.join(
 )


-def write_task_config(try_task_config):
-    config_path = os.path.join(vcs.path, "try_task_config.json")
-    with open(config_path, "w") as fh:
-        json.dump(try_task_config, fh, indent=4, separators=(",", ": "), sort_keys=True)
-        fh.write("\n")
-    return config_path
-
-
 def write_task_config_history(msg, try_task_config):
    if not os.path.isfile(history_path):
        if not os.path.isdir(os.path.dirname(history_path)):
@ -230,46 +221,47 @@ def push_to_try(
        method,
    )

-    config_path = None
-    changed_files = []
+    changed_files = {}
+
    if try_task_config:
+        changed_files["try_task_config.json"] = (
+            json.dumps(
+                try_task_config, indent=4, separators=(",", ": "), sort_keys=True
+            )
+            + "\n"
+        )
        if push and method not in ("again", "auto", "empty"):
            write_task_config_history(msg, try_task_config)
-        config_path = write_task_config(try_task_config)
-        changed_files.append(config_path)

    if (push or stage_changes) and files_to_change:
-        for path, content in files_to_change.items():
-            path = os.path.join(vcs.path, path)
-            with open(path, "wb") as fh:
-                fh.write(six.ensure_binary(content))
-            changed_files.append(path)
+        changed_files.update(files_to_change.items())
+
+    if not push:
+        print("Commit message:")
+        print(commit_message)
+        config = changed_files.pop("try_task_config.json", None)
+        if config:
+            print("Calculated try_task_config.json:")
+            print(config)
+        if stage_changes:
+            vcs.stage_changes(changed_files)
+
+        return

    try:
-        if not push:
-            print("Commit message:")
-            print(commit_message)
-            if config_path:
-                print("Calculated try_task_config.json:")
-                with open(config_path) as fh:
-                    print(fh.read())
-            return
-
-        vcs.add_remove_files(*changed_files)
-
-        try:
-            if push_to_lando:
-                push_to_lando_try(vcs, commit_message)
-            else:
-                vcs.push_to_try(commit_message, allow_log_capture=allow_log_capture)
-        except MissingVCSExtension as e:
-            if e.ext == "push-to-try":
-                print(HG_PUSH_TO_TRY_NOT_FOUND)
-            elif e.ext == "cinnabar":
-                print(GIT_CINNABAR_NOT_FOUND)
-            else:
-                raise
-            sys.exit(1)
-    finally:
-        if config_path and os.path.isfile(config_path):
-            os.remove(config_path)
+        if push_to_lando:
+            push_to_lando_try(vcs, commit_message, changed_files)
+        else:
+            vcs.push_to_try(
+                commit_message,
+                changed_files=changed_files,
+                allow_log_capture=allow_log_capture,
+            )
+    except MissingVCSExtension as e:
+        if e.ext == "push-to-try":
+            print(HG_PUSH_TO_TRY_NOT_FOUND)
+        elif e.ext == "cinnabar":
+            print(GIT_CINNABAR_NOT_FOUND)
+        else:
+            raise
+        sys.exit(1)
--- a/tools/tryselect/task_config.py
+++ b/tools/tryselect/task_config.py
@ -15,14 +15,12 @@ import subprocess
 import sys
 from abc import ABCMeta, abstractmethod, abstractproperty
 from argparse import SUPPRESS, Action
-from contextlib import contextmanager
 from textwrap import dedent

 import mozpack.path as mozpath
 import requests
 import six
 from mozbuild.base import BuildEnvironmentNotFoundException, MozbuildObject
-from mozversioncontrol import Repository
 from taskgraph.util import taskcluster

 from .tasks import resolve_tests_by_suite
@ -32,24 +30,6 @@ here = pathlib.Path(__file__).parent
 build = MozbuildObject.from_environment(cwd=str(here))


-@contextmanager
-def try_config_commit(vcs: Repository, commit_message: str):
-    """Context manager that creates and removes a try config commit."""
-    # Add the `try_task_config.json` file if it exists.
-    try_task_config_path = pathlib.Path(build.topsrcdir) / "try_task_config.json"
-    if try_task_config_path.exists():
-        vcs.add_remove_files("try_task_config.json")
-
-    try:
-        # Create a try config commit.
-        vcs.create_try_commit(commit_message)
-
-        yield
-    finally:
-        # Revert the try config commit.
-        vcs.remove_current_commit()
-
-
 class ParameterConfig:
    __metaclass__ = ABCMeta