Merge pull request #2948 from ytausch/continue-git-backend-part-3

(Git-3) Use GitCli to Commit Migrations

.github/workflows/tests-reusable.yml

@@ -72,6 +72,12 @@ jobs:
         run: |
           pip install --no-deps --no-build-isolation -e .
 
+      - name: Set up git
+        run: |
+          git config --global user.name regro-cf-autotick-bot
+          git config --global user.email [email protected]
+          git config --global pull.rebase false
+
       - name: test versions
         run: |
           cd ..

conda_forge_tick/auto_tick.py

@@ -7,7 +7,6 @@
 import time
 import traceback
 import typing
-from subprocess import CalledProcessError
 from textwrap import dedent
 from typing import Literal, MutableMapping, cast
 from urllib.error import URLError
@@ -31,6 +30,8 @@
 from conda_forge_tick.feedstock_parser import BOOTSTRAP_MAPPINGS
 from conda_forge_tick.git_utils import (
     DryRunBackend,
+    GitCli,
+    GitCliError,
     GitPlatformBackend,
     RepositoryNotFoundError,
     comment_on_pr,
@@ -189,6 +190,39 @@ def _prepare_feedstock_repository(
     return True
 
 
+def _commit_migration(
+    cli: GitCli,
+    context: ClonedFeedstockContext,
+    commit_message: str,
+    allow_empty_commits: bool = False,
+    raise_commit_errors: bool = True,
+) -> None:
+    """
+    Commit a migration that has been run in the local clone of a feedstock repository.
+    If an error occurs during the commit, it is logged.
+    :param cli: The GitCli instance to use.
+    :param context: The FeedstockContext instance.
+    :param commit_message: The commit message to use.
+    :param allow_empty_commits: Whether the migrator allows empty commits.
+    :param raise_commit_errors: Whether to raise an exception if an error occurs during the commit.
+    :raises GitCliError: If an error occurs during the commit and raise_commit_errors is True.
+    """
+    cli.add(
+        context.local_clone_dir,
+        all_=True,
+    )
+
+    try:
+        cli.commit(
+            context.local_clone_dir, commit_message, allow_empty=allow_empty_commits
+        )
+    except GitCliError as e:
+        logger.info("could not commit to feedstock - likely no changes", exc_info=e)
+
+        if raise_commit_errors:
+            raise
+
+
 def run_with_tmpdir(
     context: FeedstockContext,
     migrator: Migrator,
@@ -274,19 +308,10 @@ def run(
         # something went wrong during forking or cloning
         return False, False
 
-    # need to use an absolute path here
-    feedstock_dir = str(context.local_clone_dir.resolve())
-
-    # This is needed because we want to migrate to the new backend step-by-step
-    repo: github3.repos.Repository | None = github3_client().repository(
-        context.git_repo_owner, context.git_repo_name
-    )
-
-    assert repo is not None
-
+    # feedstock_dir must be an absolute path
     migration_run_data = run_migration(
         migrator=migrator,
-        feedstock_dir=feedstock_dir,
+        feedstock_dir=str(context.local_clone_dir.resolve()),
         feedstock_name=context.feedstock_name,
         node_attrs=context.attrs,
         default_branch=context.default_branch,
@@ -301,27 +326,28 @@ def run(
         )
         return False, False
 
+    # We raise an exception if we don't plan to rerender and wanted an empty commit.
+    # This prevents PRs that don't actually get made from getting marked as done.
+    _commit_migration(
+        cli=git_backend.cli,
+        context=context,
+        commit_message=migration_run_data["commit_message"],
+        allow_empty_commits=migrator.allow_empty_commits,
+        raise_commit_errors=migrator.allow_empty_commits and not rerender,
+    )
+
+    # This is needed because we want to migrate to the new backend step-by-step
+    repo: github3.repos.Repository | None = github3_client().repository(
+        context.git_repo_owner, context.git_repo_name
+    )
+
+    assert repo is not None
+
+    feedstock_dir = str(context.local_clone_dir.resolve())
+
     # rerender, maybe
     diffed_files: typing.List[str] = []
     with pushd(feedstock_dir):
-        msg = migration_run_data["commit_message"]
-        try:
-            eval_cmd(["git", "add", "--all", "."])
-            if migrator.allow_empty_commits:
-                eval_cmd(["git", "commit", "--allow-empty", "-am", msg])
-            else:
-                eval_cmd(["git", "commit", "-am", msg])
-        except CalledProcessError as e:
-            logger.info(
-                "could not commit to feedstock - "
-                "likely no changes - error is '%s'" % (repr(e)),
-            )
-            # we bail here if we do not plan to rerender and we wanted an empty
-            # commit
-            # this prevents PRs that don't actually get made from getting marked as done
-            if migrator.allow_empty_commits and not rerender:
-                raise e
-
         if rerender:
             head_ref = eval_cmd(["git", "rev-parse", "HEAD"]).strip()
             logger.info("Rerendering the feedstock")

conda_forge_tick/git_utils.py

@@ -166,6 +166,43 @@ def _run_git_command(
         except subprocess.CalledProcessError as e:
             raise GitCliError("Error running git command.") from e
 
+    @lock_git_operation()
+    def add(self, git_dir: Path, *pathspec: Path, all_: bool = False):
+        """
+        Add files to the git index with `git add`.
+        :param git_dir: The directory of the git repository.
+        :param pathspec: The files to add.
+        :param all_: If True, not only add the files in pathspec, but also where the index already has an entry.
+        If all_ is set with empty pathspec, all files in the entire working tree are updated.
+        :raises ValueError: If pathspec is empty and all_ is False.
+        :raises GitCliError: If the git command fails.
+        """
+        if not pathspec and not all_:
+            raise ValueError("Either pathspec or all_ must be set.")
+
+        all_arg = ["--all"] if all_ else []
+
+        self._run_git_command(["add", *all_arg, *pathspec], git_dir)
+
+    @lock_git_operation()
+    def commit(
+        self, git_dir: Path, message: str, all_: bool = False, allow_empty: bool = False
+    ):
+        """
+        Commit changes to the git repository with `git commit`.
+        :param git_dir: The directory of the git repository.
+        :param message: The commit message.
+        :param allow_empty: If True, allow an empty commit.
+        :param all_: Automatically stage files that have been modified and deleted, but new files are not affected.
+        :raises GitCliError: If the git command fails.
+        """
+        all_arg = ["-a"] if all_ else []
+        allow_empty_arg = ["--allow-empty"] if allow_empty else []
+
+        self._run_git_command(
+            ["commit", *all_arg, *allow_empty_arg, "-m", message], git_dir
+        )
+
     @lock_git_operation()
     def reset_hard(self, git_dir: Path, to_treeish: str = "HEAD"):
         """

tests/test_auto_tick.py

@@ -8,10 +8,16 @@
 import pytest
 from conftest import FakeLazyJson
 
-from conda_forge_tick.auto_tick import _prepare_feedstock_repository, run_with_tmpdir
+from conda_forge_tick.auto_tick import (
+    _commit_migration,
+    _prepare_feedstock_repository,
+    run_with_tmpdir,
+)
 from conda_forge_tick.contexts import ClonedFeedstockContext, FeedstockContext
 from conda_forge_tick.git_utils import (
     DryRunBackend,
+    GitCli,
+    GitCliError,
     GitPlatformBackend,
     RepositoryNotFoundError,
 )
@@ -143,6 +149,157 @@ def test_prepare_feedstock_repository_complete_fail():
         )
 
 
+@pytest.mark.parametrize("raise_commit_errors", [True, False])
+@pytest.mark.parametrize("allow_empty_commits", [True, False])
+def test_commit_migration_nonempty(
+    raise_commit_errors: bool, allow_empty_commits: bool
+):
+    backend = DryRunBackend()
+
+    context = FeedstockContext(
+        feedstock_name="pytest",
+        attrs=demo_attrs,
+    )
+
+    with context.reserve_clone_directory() as cloned_context:
+        assert _prepare_feedstock_repository(
+            backend, cloned_context, "new_branch", "main"
+        )
+
+        # now we do our "migration"
+        cloned_context.local_clone_dir.joinpath("conda-forge.yml").unlink()
+        with cloned_context.local_clone_dir.joinpath("new_file.txt").open("w") as f:
+            f.write("Hello World!")
+
+        _commit_migration(
+            backend.cli,
+            cloned_context,
+            "COMMIT_MESSAGE_1337",
+            allow_empty_commits,
+            raise_commit_errors,
+        )
+
+        # commit should be there
+        assert (
+            "COMMIT_MESSAGE_1337"
+            in backend.cli._run_git_command(
+                ["log", "-1", "--pretty=%B"],
+                cloned_context.local_clone_dir,
+                capture_text=True,
+            ).stdout
+        )
+
+
+@pytest.mark.parametrize("raise_commit_errors", [True, False])
+@pytest.mark.parametrize("allow_empty_commits", [True, False])
+def test_commit_migration_empty(raise_commit_errors: bool, allow_empty_commits: bool):
+    backend = DryRunBackend()
+
+    context = FeedstockContext(
+        feedstock_name="pytest",
+        attrs=demo_attrs,
+    )
+
+    with context.reserve_clone_directory() as cloned_context:
+        assert _prepare_feedstock_repository(
+            backend, cloned_context, "new_branch", "main"
+        )
+
+        if raise_commit_errors and not allow_empty_commits:
+            with pytest.raises(GitCliError):
+                _commit_migration(
+                    backend.cli,
+                    cloned_context,
+                    "COMMIT_MESSAGE",
+                    allow_empty_commits,
+                    raise_commit_errors,
+                )
+            return
+        else:
+            # everything should work normally
+            _commit_migration(
+                backend.cli,
+                cloned_context,
+                "COMMIT_MESSAGE_1337",
+                allow_empty_commits,
+                raise_commit_errors,
+            )
+
+        if not allow_empty_commits:
+            return
+
+        # commit should be there
+        assert (
+            "COMMIT_MESSAGE_1337"
+            in backend.cli._run_git_command(
+                ["log", "-1", "--pretty=%B"],
+                cloned_context.local_clone_dir,
+                capture_text=True,
+            ).stdout
+        )
+
+
+@pytest.mark.parametrize("raise_commit_errors", [True, False])
+@pytest.mark.parametrize("allow_empty_commits", [True, False])
+def test_commit_migration_mock_no_error(
+    allow_empty_commits: bool, raise_commit_errors: bool
+):
+    cli = create_autospec(GitCli)
+
+    clone_dir = Path("LOCAL_CLONE_DIR")
+
+    context = ClonedFeedstockContext(
+        feedstock_name="pytest", attrs=demo_attrs, local_clone_dir=clone_dir
+    )
+
+    _commit_migration(
+        cli, context, "COMMIT MESSAGE 1337", allow_empty_commits, raise_commit_errors
+    )
+
+    cli.commit.assert_called_once_with(
+        clone_dir, "COMMIT MESSAGE 1337", allow_empty=allow_empty_commits
+    )
+
+
+@pytest.mark.parametrize("raise_commit_errors", [True, False])
+@pytest.mark.parametrize("allow_empty_commits", [True, False])
+def test_commit_migration_mock_error(
+    allow_empty_commits: bool, raise_commit_errors: bool
+):
+    cli = create_autospec(GitCli)
+
+    clone_dir = Path("LOCAL_CLONE_DIR")
+
+    cli.commit.side_effect = GitCliError("Error")
+
+    context = ClonedFeedstockContext(
+        feedstock_name="pytest", attrs=demo_attrs, local_clone_dir=clone_dir
+    )
+
+    if raise_commit_errors:
+        with pytest.raises(GitCliError):
+            _commit_migration(
+                cli,
+                context,
+                "COMMIT MESSAGE 1337",
+                allow_empty_commits,
+                raise_commit_errors,
+            )
+    else:
+        _commit_migration(
+            cli,
+            context,
+            "COMMIT MESSAGE 1337",
+            allow_empty_commits,
+            raise_commit_errors,
+        )
+
+    cli.add.assert_called_once_with(clone_dir, all_=True)
+    cli.commit.assert_called_once_with(
+        clone_dir, "COMMIT MESSAGE 1337", allow_empty=allow_empty_commits
+    )
+
+
 @pytest.mark.parametrize("dry_run", [True, False])
 @pytest.mark.parametrize("base_branch", ["main", "master"])
 @pytest.mark.parametrize("rerender", [True, False])