Repository: CWorthy-ocean/C-Star

Issue:
ValueError: supplied checkout_target does not appear to be a valid reference for this repository
Running the first two cells of the example notebook gives

```python
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 3
      1 #BaseModel object, ROMS:
----> 3 roms_base_model=cstar.ROMSBaseModel(
      4                            source_repo='https://github.com/dafyddstephenson/ucla-roms.git',
      5                            checkout_target='marbl_improvements_20240611')
      7 marbl_base_model=cstar.MARBLBaseModel(
      8                             source_repo='https://github.com/marbl-ecosys/MARBL.git',
      9                             checkout_target='marbl0.45.0')

File [~/Documents/Work/Code/C-Star/cstar_ocean/base_model.py:89](http://localhost:8888/lab/tree/~/Documents/Work/Code/C-Star/cstar_ocean/base_model.py#line=88), in BaseModel.__init__(self, source_repo, checkout_target)
     81 self.source_repo = (
     82     source_repo if source_repo is not None else self.default_source_repo
     83 )
     84 self.checkout_target = (
     85     checkout_target
     86     if checkout_target is not None
     87     else self.default_checkout_target
     88 )
---> 89 self.checkout_hash = _get_hash_from_checkout_target(
     90     self.source_repo, self.checkout_target
     91 )
     92 self.repo_basename = os.path.basename(self.source_repo).replace(".git", "")
     94 self.local_config_status = self.get_local_config_status()

File [~/Documents/Work/Code/C-Star/cstar_ocean/utils.py:119](http://localhost:8888/lab/tree/~/Documents/Work/Code/C-Star/cstar_ocean/utils.py#line=118), in _get_hash_from_checkout_target(repo_url, checkout_target)
    117         return checkout_target
    118     else:
--> 119         raise ValueError(
    120             "supplied checkout_target does not appear "
    121             + "to be a valid reference for this repository"
    122         )
    123 else:
    124     return ls_remote.split()[0]

ValueError: supplied checkout_target does not appear to be a valid reference for this repository
```

Discussion:

Patch:
diff --git a/cstar/base/utils.py b/cstar/base/utils.py
index a9078e1..d87d0e1 100644
--- a/cstar/base/utils.py
+++ b/cstar/base/utils.py
@@ -1,5 +1,6 @@
 import re
 import hashlib
+import warnings
 import subprocess
 from pathlib import Path
 
@@ -109,10 +110,9 @@ def _get_hash_from_checkout_target(repo_url: str, checkout_target: str) -> str:
     """Take a git checkout target (any `arg` accepted by `git checkout arg`) and return
     a commit hash.
 
-    If the target is a 7 or 40 digit hexadecimal string, it is assumed `checkout_target`
-    is already a git hash, so `checkout_target` is returned.
-
-    Otherwise, `git ls-remote` is used to obtain the hash associated with `checkout_target`.
+    This method parses the output of `git ls-remote {repo_url}` to create a dictionary
+    of refs and hashes, returning the hash corresponding to `checkout_target` or
+    raising an error listing available branches and tags if the target is not found.
 
     Parameters:
     -----------
@@ -127,29 +127,71 @@ def _get_hash_from_checkout_target(repo_url: str, checkout_target: str) -> str:
         A git commit hash associated with the checkout target
     """
 
-    # First check if the checkout target is a 7 or 40 digit hexadecimal string
-    is_potential_hash = bool(re.fullmatch(r"^[0-9a-f]{7}$", checkout_target)) or bool(
-        re.fullmatch(r"^[0-9a-f]{40}$", checkout_target)
-    )
-    if is_potential_hash:
-        return checkout_target
-
-    # Then try ls-remote to see if there is a match
-    # (no match if either invalid target or a valid hash):
+    # Get list of targets from git ls-remote
     ls_remote = subprocess.run(
-        "git ls-remote " + repo_url + " " + checkout_target,
+        f"git ls-remote {repo_url}",
         shell=True,
         capture_output=True,
         text=True,
     ).stdout
 
-    if len(ls_remote) == 0:
-        raise ValueError(
-            f"supplied checkout_target ({checkout_target}) does not appear "
-            + f"to be a valid reference for this repository ({repo_url})"
+    # Process the output into a `reference: hash` dictionary
+    ref_dict = {
+        ref: has for has, ref in (line.split() for line in ls_remote.splitlines())
+    }
+
+    # If the checkout target is a valid hash, return it
+    if checkout_target in ref_dict.values():
+        return checkout_target
+
+    # Otherwise, see if it is listed as a branch or tag
+    for ref, has in ref_dict.items():
+        if (
+            ref == f"refs/heads/{checkout_target}"
+            or ref == f"refs/tags/{checkout_target}"
+        ):
+            return has
+
+    # Lastly, if NOTA worked, see if the checkout target is a 7 or 40 digit hexadecimal string
+    is_potential_hash = bool(re.fullmatch(r"^[0-9a-f]{7}$", checkout_target)) or bool(
+        re.fullmatch(r"^[0-9a-f]{40}$", checkout_target)
+    )
+    if is_potential_hash:
+        warnings.warn(
+            f"C-STAR: The checkout target {checkout_target} appears to be a commit hash, "
+            f"but it is not possible to verify that this hash is a valid checkout target of {repo_url}"
         )
-    else:
-        return ls_remote.split()[0]
+
+        return checkout_target
+
+    # If the target is still not found, raise an error listing branches and tags
+    branches = [
+        ref.replace("refs/heads/", "")
+        for ref in ref_dict
+        if ref.startswith("refs/heads/")
+    ]
+    tags = [
+        ref.replace("refs/tags/", "")
+        for ref in ref_dict
+        if ref.startswith("refs/tags/")
+    ]
+
+    error_message = (
+        f"Supplied checkout_target ({checkout_target}) does not appear "
+        f"to be a valid reference for this repository ({repo_url}).\n"
+    )
+    if branches:
+        error_message += (
+            "Available branches:\n"
+            + "\n".join(f" - {branch}" for branch in sorted(branches))
+            + "\n"
+        )
+    if tags:
+        error_message += (
+            "Available tags:\n" + "\n".join(f" - {tag}" for tag in sorted(tags)) + "\n"
+        )
+
+    raise ValueError(error_message.strip())
 
 
 def _replace_text_in_file(file_path: str | Path, old_text: str, new_text: str) -> bool:


Test patch:
diff --git a/cstar/tests/unit_tests/base/test_utils.py b/cstar/tests/unit_tests/base/test_utils.py
index cd5fb06..e9a102b 100644
--- a/cstar/tests/unit_tests/base/test_utils.py
+++ b/cstar/tests/unit_tests/base/test_utils.py
@@ -1,5 +1,6 @@
 import pytest
 import hashlib
+import warnings
 from unittest import mock
 from cstar.base.utils import (
     _get_sha256_hash,
@@ -248,103 +249,148 @@ def test_get_repo_head_hash():
         )
 
 
-def test_get_hash_from_checkout_target_direct_hash():
-    """Test `_get_hash_from_checkout_target` to confirm it returns the input directly
-    when `checkout_target` is already a valid 7-character or 40-character hash.
+class TestGetHashFromCheckoutTarget:
+    """Test class for `_get_hash_from_checkout_target`."""
 
-    Asserts
-    -------
-    - Ensures that `checkout_target` is returned directly if it matches a 7-character or
-      40-character hash pattern.
-    - Verifies `subprocess.run` is not called for direct hash inputs.
-    """
-    repo_url = "https://example.com/repo.git"
-
-    # Test with a 7-character hash
-    checkout_target_7 = "abcdef1"
-    with mock.patch("subprocess.run") as mock_run:
-        result_7 = _get_hash_from_checkout_target(repo_url, checkout_target_7)
-        assert (
-            result_7 == checkout_target_7
-        ), f"Expected '{checkout_target_7}', got '{result_7}'"
-        (
-            mock_run.assert_not_called(),
-            f"subprocess.run was called unexpectedly for 7-character hash '{checkout_target_7}'",
-        )
-
-    # Test with a 40-character hash
-    checkout_target_40 = "abcdef1234567890abcdef1234567890abcdef12"
-    with mock.patch("subprocess.run") as mock_run:
-        result_40 = _get_hash_from_checkout_target(repo_url, checkout_target_40)
-        assert (
-            result_40 == checkout_target_40
-        ), f"Expected '{checkout_target_40}', got '{result_40}'"
-        (
-            mock_run.assert_not_called(),
-            f"subprocess.run was called unexpectedly for 40-character hash '{checkout_target_40}'",
+    def setup_method(self):
+        """Setup method to define common variables and mock data."""
+        self.repo_url = "https://example.com/repo.git"
+
+        # Mock the output of `git ls-remote` with a variety of refs
+        self.ls_remote_output = (
+            "abcdef1234567890abcdef1234567890abcdef12\trefs/heads/main\n"  # Branch
+            "deadbeef1234567890deadbeef1234567890deadbeef\trefs/heads/feature\n"  # Branch
+            "c0ffee1234567890c0ffee1234567890c0ffee1234\trefs/tags/v1.0.0\n"  # Tag
+            "feedface1234567890feedface1234567890feedface\trefs/pull/123/head\n"  # Pull request
+            "1234567890abcdef1234567890abcdef12345678\trefs/heads/develop\n"  # Branch
         )
 
-
-def test_get_hash_from_checkout_target_branch():
-    """Test `_get_hash_from_checkout_target` to confirm it retrieves the correct hash
-    when `checkout_target` is a branch or tag.
-
-    Asserts
-    -------
-    - Ensures the correct hash is returned when `checkout_target` is not a direct hash.
-    - Verifies `subprocess.run` is called with the correct `git ls-remote` command.
-    """
-    repo_url = "https://example.com/repo.git"
-    checkout_target = "main"
-    expected_hash = "abcdef1234567890abcdef1234567890abcdef12"
-
-    # Patch subprocess.run to simulate successful `git ls-remote` command
-    with mock.patch("subprocess.run") as mock_run:
-        mock_run.return_value = mock.Mock(
-            returncode=0, stdout=f"{expected_hash}\trefs/heads/main\n"
+        # Patch subprocess.run to simulate the `git ls-remote` command
+        self.mock_run = mock.patch("subprocess.run").start()
+        self.mock_run.return_value = mock.Mock(
+            returncode=0, stdout=self.ls_remote_output
         )
 
-        # Call the function
-        result = _get_hash_from_checkout_target(repo_url, checkout_target)
-
-        # Assert the correct hash is returned
+    def teardown_method(self):
+        """Teardown method to stop all patches."""
+        mock.patch.stopall()
+
+    @pytest.mark.parametrize(
+        "checkout_target, expected_hash",
+        [
+            pytest.param(target, hash, id=target)
+            for target, hash in [
+                # Branches
+                ("main", "abcdef1234567890abcdef1234567890abcdef12"),
+                ("develop", "1234567890abcdef1234567890abcdef12345678"),
+                # Tags
+                ("v1.0.0", "c0ffee1234567890c0ffee1234567890c0ffee1234"),
+                # Commit hashes
+                (
+                    "1234567890abcdef1234567890abcdef12345678",
+                    "1234567890abcdef1234567890abcdef12345678",
+                ),
+            ]
+        ],
+    )
+    def test_valid_targets(self, checkout_target, expected_hash):
+        """Test `_get_hash_from_checkout_target` with valid checkout targets.
+
+        Parameters
+        ----------
+        checkout_target : str
+            The checkout target to test (branch, tag, pull request, or commit hash).
+        expected_hash : str
+            The expected commit hash for the given checkout target.
+        """
+        # Call the function and assert the result
+        result = _get_hash_from_checkout_target(self.repo_url, checkout_target)
         assert result == expected_hash
 
-        # Check the subprocess call arguments
-        mock_run.assert_called_once_with(
-            f"git ls-remote {repo_url} {checkout_target}",
-            shell=True,
+        # Verify the subprocess call
+        self.mock_run.assert_called_with(
+            f"git ls-remote {self.repo_url}",
             capture_output=True,
+            shell=True,
             text=True,
         )
 
+    def test_invalid_target(self):
+        """Test `_get_hash_from_checkout_target` with an invalid checkout target.
 
-def test_get_hash_from_checkout_target_invalid():
-    """Test `_get_hash_from_checkout_target` when git ls-remote does not return a hash
-    and `checkout_target` is not itself a valid hash.
-
-    Asserts
-    -------
-    - a ValueError is raised
-    - the error message matches an expected error message
-    """
-    repo_url = "https://example.com/repo.git"
-    checkout_target = "pain"
-
-    # Patch subprocess.run to simulate successful `git ls-remote` command
-    with mock.patch("subprocess.run") as mock_run:
-        mock_run.return_value = mock.Mock(returncode=0, stdout="")
+        Asserts
+        -------
+        - A ValueError is raised.
+        - The error message includes a list of available branches and tags.
+        """
+        checkout_target = "invalid-branch"
 
-        # Call the function
+        # Call the function and expect a ValueError
         with pytest.raises(ValueError) as exception_info:
-            _get_hash_from_checkout_target(repo_url, checkout_target)
-
-        expected_message = (
-            "supplied checkout_target (pain) does not appear "
-            + "to be a valid reference for this repository (https://example.com/repo.git)"
-        )
-        # Assert the correct hash is returned
-        assert str(exception_info.value) == expected_message
+            _get_hash_from_checkout_target(self.repo_url, checkout_target)
+
+        # Assert the error message includes the expected content
+        error_message = str(exception_info.value)
+        assert checkout_target in error_message
+        assert self.repo_url in error_message
+        assert "Available branches:" in error_message
+        assert "Available tags:" in error_message
+        assert "main" in error_message
+        assert "feature" in error_message
+        assert "v1.0.0" in error_message
+
+    @pytest.mark.parametrize(
+        "checkout_target, should_warn, should_raise",
+        [
+            # 7-character hex string (valid short hash)
+            ("246c11f", True, False),
+            # 40-character hex string (valid full hash)
+            ("246c11fa537145ba5868f2256dfb4964aeb09a25", True, False),
+            # 8-character hex string (invalid length)
+            ("246c11fa", False, True),
+            # Non-hex string
+            ("not-a-hash", False, True),
+        ],
+    )
+    def test_warning_and_error_for_potential_hash(
+        self, checkout_target, should_warn, should_raise
+    ):
+        """Test `_get_hash_from_checkout_target` to ensure a warning or error is raised
+        appropriately when the checkout target appears to be a commit hash but is not in
+        the dictionary of references returned by git ls-remote.
+
+        Parameters
+        ----------
+        checkout_target : str
+            The checkout target to test.
+        should_warn : bool
+            Whether a warning should be raised for this target.
+        should_raise : bool
+            Whether a ValueError should be raised for this target.
+        """
+        # Use pytest's `warnings.catch_warnings` to capture the warning
+        with warnings.catch_warnings(record=True) as warning_list:
+            if should_raise:
+                # Call the function and expect a ValueError
+                with pytest.raises(ValueError):
+                    _get_hash_from_checkout_target(self.repo_url, checkout_target)
+
+            else:
+                # Call the function and assert the result
+                result = _get_hash_from_checkout_target(self.repo_url, checkout_target)
+                assert result == checkout_target
+
+            # Check if a warning was raised
+            if should_warn:
+                assert len(warning_list) == 1
+                warning = warning_list[0]
+                assert issubclass(warning.category, UserWarning)
+                assert (
+                    f"C-STAR: The checkout target {checkout_target} appears to be a commit hash, "
+                    f"but it is not possible to verify that this hash is a valid checkout target of {self.repo_url}"
+                ) in str(warning.message)
+            else:
+                assert len(warning_list) == 0
 
 
 class TestReplaceTextInFile: