Use gitpython branch handling during clone

Author: tpoliaw

helm/blueapi/config_schema.json

@@ -431,7 +431,7 @@
                     "type": "string"
                 },
                 "branch": {
-                    "description": "Branch of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists",
+                    "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.",
                     "title": "Branch",
                     "type": "string"
                 }

helm/blueapi/values.schema.json

@@ -854,7 +854,7 @@
             "properties": {
                 "branch": {
                     "title": "Branch",
-                    "description": "Branch of repo to check out - defaults to remote's default when cloning and the existing branch when the repo already exists",
+                    "description": "Branch (or tag) to check out when cloning - defaults to remote's HEAD. If a tag is used, the repo will be left in a 'detached head' state.",
                     "type": "string"
                 },
                 "name": {

src/blueapi/cli/scratch.py

@@ -48,15 +48,12 @@ def setup_scratch(
             )
     for repo in config.repositories:
         local_directory = config.root / repo.name
-        ensure_repo(repo.remote_url, local_directory, repo.branch, repo.tag)
+        ensure_repo(repo.remote_url, local_directory, repo.branch)
         scratch_install(local_directory, timeout=install_timeout)
 
 
 def ensure_repo(
-    remote_url: str,
-    local_directory: Path,
-    branch: str | None = None,
-    tag: str | None = None,
+    remote_url: str, local_directory: Path, branch: str | None = None
 ) -> None:
     """
     Ensure that a repository is checked out for use in the scratch area.
@@ -67,41 +64,20 @@ def ensure_repo(
         local_directory: Output path for cloning
     """
 
-    if tag and branch:
-        raise ValueError(f"Can't use both branch ({branch!r}) and tag ({tag!r})")
-
     # Set umask to DLS standard
     os.umask(stat.S_IWOTH)
 
     if not local_directory.exists():
         LOGGER.info(f"Cloning {remote_url}")
-        repo = Repo.clone_from(remote_url, local_directory)
+        Repo.clone_from(remote_url, local_directory, branch=branch)
         LOGGER.info(f"Cloned {remote_url} -> {local_directory}")
-        if branch:
-            if not (local := getattr(repo.heads, branch, None)):
-                origin = repo.remotes[0]
-                origin.fetch()
-                LOGGER.info(
-                    "Creating branch '%s' to track remote '%s'",
-                    branch,
-                    origin.refs[branch],
-                )
-                local = repo.create_head(branch, origin.refs[branch])
-                local.set_tracking_branch(origin.refs[branch])
-
-            LOGGER.info("Checking out branch %r", branch)
-            local.checkout()
-        elif tag:
-            if tag_obj := getattr(repo.tags, tag, None):
-                LOGGER.info("Checking out tag %r", tag)
-                tag_obj.checkout()
-            else:
-                raise ValueError("Could not find tag: " + repr(tag))
     elif local_directory.is_dir():
+        Repo(local_directory)
         LOGGER.info(f"Found {local_directory}")
     else:
         raise KeyError(
-            f"Unable to open {local_directory} as a git repository because it is a file"
+            f"Unable to open {local_directory} as a git repository because it is not a "
+            "directory"
         )
 
 

src/blueapi/config.py

@@ -6,7 +6,7 @@
 from functools import cached_property
 from pathlib import Path
 from string import Template
-from typing import Annotated, Any, ClassVar, Generic, Literal, Self, TypeVar, cast
+from typing import Annotated, Any, ClassVar, Generic, Literal, TypeVar, cast
 
 import requests
 import yaml
@@ -21,7 +21,6 @@
     UrlConstraints,
     ValidationError,
     field_validator,
-    model_validator,
 )
 from pydantic.json_schema import SkipJsonSchema
 
@@ -193,14 +192,10 @@ class ScratchRepository(BlueapiBaseModel):
         default="https://github.com/example/example.git",
     )
     branch: str | SkipJsonSchema[None] = Field(
-        description="Branch of repo to check out - defaults to remote's default",
-        exclude_if=lambda f: f is None,
-        # using default_factory instead of default means the schema doesn't
-        # include an invalid value
-        default_factory=lambda: None,
-    )
-    tag: str | SkipJsonSchema[None] = Field(
-        description="Tag of repo to check out.",
+        description=(
+            "Branch (or tag) to check out when cloning - defaults to remote's HEAD. "
+            "If a tag is used, the repo will be left in a 'detached head' state."
+        ),
         exclude_if=lambda f: f is None,
         # using default_factory instead of default means the schema doesn't
         # include an invalid value
@@ -214,14 +209,6 @@ def check_remote_url(cls, value: str) -> str:
             raise ValueError(f"remote_url '{value}' is not allowed.")
         return value
 
-    @model_validator(mode="after")
-    def check_clone_options(self) -> Self:
-        if self.branch is not None and self.tag is not None:
-            raise ValueError(
-                f"Cannot set both branch ({self.branch}) and tag ({self.tag})"
-            )
-        return self
-
 
 class ScratchConfig(BlueapiBaseModel):
     root: Path = Field(

tests/unit_tests/cli/test_scratch.py

@@ -123,7 +123,7 @@ def test_repo_cloned_if_not_found_locally(
     ensure_repo("http://example.com/foo.git", nonexistant_path)
     mock_repo.assert_not_called()
     mock_repo.clone_from.assert_called_once_with(
-        "http://example.com/foo.git", nonexistant_path
+        "http://example.com/foo.git", nonexistant_path, branch=None
     )
 
 
@@ -140,7 +140,7 @@ def write_repo_files():
         with file_path.open("w") as stream:
             stream.write("foo")
 
-    mock_repo.clone_from.side_effect = lambda url, path: write_repo_files()
+    mock_repo.clone_from.side_effect = lambda url, path, branch=None: write_repo_files()
 
     ensure_repo("http://example.com/foo.git", repo_root)
     assert file_path.exists()
@@ -162,26 +162,23 @@ def test_cloned_repo_changes_to_new_branch(mock_repo, directory_path: Path):
 
     ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo")
 
-    mock_repo.clone_from.assert_called_once_with("http://example.com/foo.git", ANY)
-    repo.create_head.assert_called_once_with("demo", ANY)
-    repo.create_head().checkout.assert_called_once()
+    mock_repo.clone_from.assert_called_once_with(
+        "http://example.com/foo.git", ANY, branch="demo"
+    )
 
 
 @patch("blueapi.cli.scratch.Repo")
-def test_existing_repo_changes_to_existing_branch(mock_repo, directory_path: Path):
+def test_existing_repo_not_changed_to_existing_branch(mock_repo, directory_path: Path):
     (directory_path / "demo_branch").mkdir()
-    repo = Mock(spec=Repo)
-    mock_repo.return_value = repo
 
     ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo")
 
+    mock_repo.assert_called_once_with(directory_path / "demo_branch")
     mock_repo.clone_from.assert_not_called()
-    repo.create_head.assert_not_called()
-    repo.heads.demo.checkout.assert_called_once()
 
 
 @patch("blueapi.cli.scratch.Repo")
-def test_existing_repo_changes_to_new_branch(mock_repo, directory_path: Path):
+def test_existing_repo_not_changed_to_new_branch(mock_repo, directory_path: Path):
     (directory_path / "demo_branch").mkdir()
     repo = MagicMock(name="ExistingRepo", spec=Repo)
     repo.heads.demo = None
@@ -190,8 +187,7 @@ def test_existing_repo_changes_to_new_branch(mock_repo, directory_path: Path):
     ensure_repo("http://example.com/foo.git", directory_path / "demo_branch", "demo")
 
     mock_repo.clone_from.assert_not_called()
-    repo.create_head.assert_called_once_with("demo", repo.remotes[0].refs["demo"])
-    repo.create_head().checkout.assert_called_once()
+    repo.create_head.assert_not_called()
 
 
 def test_setup_scratch_fails_on_nonexistant_root(
@@ -292,10 +288,7 @@ def test_setup_scratch_iterates_repos(
     config = ScratchConfig(
         root=directory_path_with_sgid,
         repositories=[
-            ScratchRepository(
-                name="foo",
-                remote_url="http://example.com/foo.git",
-            ),
+            ScratchRepository(name="foo", remote_url="http://example.com/foo.git"),
             ScratchRepository(
                 name="bar",
                 remote_url="http://example.com/bar.git",