Move peeled tags protocol functions to dulwich.protocol

Remove InfoRefsContainer class, inlining its functionality into
SwiftInfoRefsContainer in dulwich.contrib.swift.

This properly isolates the ^{} protocol syntax to protocol-level code,
which is a bit of an architectural leak.

Fixes #2009

NEWS

@@ -1,5 +1,12 @@
 0.25.0	UNRELEASED
 
+ * Move protocol-level peeled tags functions (``serialize_refs()``,
+   ``write_info_refs()``, ``split_peeled_refs()``, ``strip_peeled_refs()``)
+   from ``dulwich.refs`` to ``dulwich.protocol``. The ``^{}`` peeled tags syntax
+   is now properly isolated to protocol-level code. Remove ``InfoRefsContainer``
+   class (functionality inlined into ``SwiftInfoRefsContainer``).
+   (Jelmer Vernooij, #2009)
+
  * Fix ``get_unstaged_changes()`` to correctly pass Blob objects to filter
    callbacks instead of raw bytes. This fixes crashes when using ``.gitattributes``
    files with filter callbacks like ``checkin_normalize``.

dulwich/client.py

@@ -164,6 +164,7 @@ from .protocol import (
     parse_capability,
     pkt_line,
     pkt_seq,
+    split_peeled_refs,
 )
 from .refs import (
     HEADREF,
@@ -175,7 +176,6 @@ from .refs import (
     _set_origin_head,
     filter_ref_prefix,
     read_info_refs,
-    split_peeled_refs,
 )
 from .repo import BaseRepo, Repo
 

dulwich/contrib/swift.py

@@ -65,15 +65,8 @@ from ..pack import (
     write_pack_index_v2,
     write_pack_object,
 )
-from ..protocol import TCP_GIT_PORT
-from ..refs import (
-    HEADREF,
-    InfoRefsContainer,
-    Ref,
-    read_info_refs,
-    split_peeled_refs,
-    write_info_refs,
-)
+from ..protocol import TCP_GIT_PORT, split_peeled_refs, write_info_refs
+from ..refs import HEADREF, Ref, RefsContainer, read_info_refs
 from ..repo import OBJECTDIR, BaseRepo
 from ..server import Backend, BackendRepo, TCPGitServer
 
@@ -969,7 +962,7 @@ class SwiftObjectStore(PackBasedObjectStore):
         return final_pack
 
 
-class SwiftInfoRefsContainer(InfoRefsContainer):
+class SwiftInfoRefsContainer(RefsContainer):
     """Manage references in info/refs object."""
 
     def __init__(self, scon: SwiftConnector, store: object) -> None:
@@ -987,7 +980,12 @@ class SwiftInfoRefsContainer(InfoRefsContainer):
             f = BytesIO(b"")
         elif isinstance(f, bytes):
             f = BytesIO(f)
-        super().__init__(f)
+
+        # Initialize refs from info/refs file
+        self._refs: dict[Ref, ObjectID] = {}
+        self._peeled: dict[Ref, ObjectID] = {}
+        refs = read_info_refs(f)
+        (self._refs, self._peeled) = split_peeled_refs(refs)
 
     def _load_check_ref(
         self, name: Ref, old_ref: ObjectID | None
@@ -1053,6 +1051,25 @@ class SwiftInfoRefsContainer(InfoRefsContainer):
         del self._refs[name]
         return True
 
+    def read_loose_ref(self, name: Ref) -> bytes | None:
+        """Read a loose reference."""
+        return self._refs.get(name, None)
+
+    def get_packed_refs(self) -> dict[Ref, ObjectID]:
+        """Get packed references."""
+        return {}
+
+    def get_peeled(self, name: Ref) -> ObjectID | None:
+        """Get peeled version of a reference."""
+        try:
+            return self._peeled[name]
+        except KeyError:
+            ref_value = self._refs.get(name)
+            # Only return if it's an ObjectID (not a symref)
+            if isinstance(ref_value, bytes) and len(ref_value) == 40:
+                return ObjectID(ref_value)
+            return None
+
     def allkeys(self) -> set[Ref]:
         """Get all reference names.
 

dulwich/dumb.py

@@ -44,7 +44,8 @@ from .objects import (
     sha_to_hex,
 )
 from .pack import Pack, PackData, PackIndex, UnpackedObject, load_pack_index_file
-from .refs import Ref, read_info_refs, split_peeled_refs
+from .protocol import split_peeled_refs
+from .refs import Ref, read_info_refs
 
 
 class DumbHTTPObjectStore(BaseObjectStore):

dulwich/object_store.py

@@ -2564,22 +2564,30 @@ def _collect_filetree_revs(
 
 
 def _split_commits_and_tags(
-    obj_store: ObjectContainer, lst: Iterable[ObjectID], *, ignore_unknown: bool = False
+    obj_store: ObjectContainer,
+    lst: Iterable[ObjectID],
+    *,
+    unknown: str = "error",
 ) -> tuple[set[ObjectID], set[ObjectID], set[ObjectID]]:
     """Split object id list into three lists with commit, tag, and other SHAs.
 
     Commits referenced by tags are included into commits
     list as well. Only SHA1s known in this repository will get
-    through, and unless ignore_unknown argument is True, KeyError
-    is thrown for SHA1 missing in the repository
+    through, controlled by the unknown parameter.
 
     Args:
       obj_store: Object store to get objects by SHA1 from
       lst: Collection of commit and tag SHAs
-      ignore_unknown: True to skip SHA1 missing in the repository
-        silently.
+      unknown: How to handle unknown objects: "error", "warn", or "ignore"
     Returns: A tuple of (commits, tags, others) SHA1s
     """
+    import logging
+
+    if unknown not in ("error", "warn", "ignore"):
+        raise ValueError(
+            f"unknown must be 'error', 'warn', or 'ignore', got {unknown!r}"
+        )
+
     commits: set[ObjectID] = set()
     tags: set[ObjectID] = set()
     others: set[ObjectID] = set()
@@ -2587,17 +2595,20 @@ def _split_commits_and_tags(
         try:
             o = obj_store[e]
         except KeyError:
-            if not ignore_unknown:
+            if unknown == "error":
                 raise
+            elif unknown == "warn":
+                logging.warning(
+                    "Object %s not found in object store", e.decode("ascii")
+                )
+            # else: ignore
         else:
             if isinstance(o, Commit):
                 commits.add(e)
             elif isinstance(o, Tag):
                 tags.add(e)
                 tagged = o.object[1]
-                c, t, os = _split_commits_and_tags(
-                    obj_store, [tagged], ignore_unknown=ignore_unknown
-                )
+                c, t, os = _split_commits_and_tags(obj_store, [tagged], unknown=unknown)
                 commits |= c
                 tags |= t
                 others |= os
@@ -2648,15 +2659,13 @@ class MissingObjectFinder:
         self._get_parents = get_parents
         reachability = object_store.get_reachability_provider()
         # process Commits and Tags differently
-        # Note, while haves may list commits/tags not available locally,
-        # and such SHAs would get filtered out by _split_commits_and_tags,
-        # wants shall list only known SHAs, and otherwise
-        # _split_commits_and_tags fails with KeyError
+        # haves may list commits/tags not available locally (silently ignore them).
+        # wants should only contain valid SHAs (fail fast if not).
         have_commits, have_tags, have_others = _split_commits_and_tags(
-            object_store, haves, ignore_unknown=True
+            object_store, haves, unknown="ignore"
         )
         want_commits, want_tags, want_others = _split_commits_and_tags(
-            object_store, wants, ignore_unknown=False
+            object_store, wants, unknown="error"
         )
         # all_ancestors is a set of commits that shall not be sent
         # (complete repository up to 'haves')

dulwich/protocol.py

@@ -23,15 +23,20 @@
 """Generic functions for talking the git smart server protocol."""
 
 import types
-from collections.abc import Callable, Iterable, Sequence
+from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence
 from io import BytesIO
 from os import SEEK_END
+from typing import TYPE_CHECKING
 
 import dulwich
 
 from .errors import GitProtocolError, HangupException
 from .objects import ObjectID
 
+if TYPE_CHECKING:
+    from .pack import ObjectContainer
+    from .refs import Ref
+
 TCP_GIT_PORT = 9418
 
 # Git protocol version 0 is the original Git protocol, which lacked a
@@ -774,3 +779,113 @@ def format_ack_line(sha: bytes, ack_type: bytes = b"") -> bytes:
     if ack_type:
         ack_type = b" " + ack_type
     return b"ACK " + sha + ack_type + b"\n"
+
+
+def strip_peeled_refs(
+    refs: "Mapping[Ref, ObjectID | None]",
+) -> "dict[Ref, ObjectID | None]":
+    """Remove all peeled refs from a refs dictionary.
+
+    Args:
+      refs: Dictionary of refs (may include peeled refs with ^{} suffix)
+
+    Returns:
+      Dictionary with peeled refs removed
+    """
+    return {
+        ref: sha for (ref, sha) in refs.items() if not ref.endswith(PEELED_TAG_SUFFIX)
+    }
+
+
+def split_peeled_refs(
+    refs: "Mapping[Ref, ObjectID]",
+) -> "tuple[dict[Ref, ObjectID], dict[Ref, ObjectID]]":
+    """Split peeled refs from regular refs.
+
+    Args:
+      refs: Dictionary of refs (may include peeled refs with ^{} suffix)
+
+    Returns:
+      Tuple of (regular_refs, peeled_refs) where peeled_refs keys have
+      the ^{} suffix removed
+    """
+    from .refs import Ref
+
+    peeled: dict[Ref, ObjectID] = {}
+    regular = {k: v for k, v in refs.items() if not k.endswith(PEELED_TAG_SUFFIX)}
+
+    for ref, sha in refs.items():
+        if ref.endswith(PEELED_TAG_SUFFIX):
+            # Peeled refs are always ObjectID values
+            peeled[Ref(ref[: -len(PEELED_TAG_SUFFIX)])] = sha
+
+    return regular, peeled
+
+
+def write_info_refs(
+    refs: "Mapping[Ref, ObjectID]", store: "ObjectContainer"
+) -> "Iterator[bytes]":
+    """Generate info refs in the format used by the dumb HTTP protocol.
+
+    Args:
+      refs: Dictionary of refs
+      store: Object store to peel tags from
+
+    Yields:
+      Lines in info/refs format (sha + tab + refname)
+    """
+    from .object_store import peel_sha
+    from .refs import HEADREF
+
+    for name, sha in sorted(refs.items()):
+        # get_refs() includes HEAD as a special case, but we don't want to
+        # advertise it
+        if name == HEADREF:
+            continue
+        try:
+            o = store[sha]
+        except KeyError:
+            continue
+        _unpeeled, peeled = peel_sha(store, sha)
+        yield o.id + b"\t" + name + b"\n"
+        if o.id != peeled.id:
+            yield peeled.id + b"\t" + name + PEELED_TAG_SUFFIX + b"\n"
+
+
+def serialize_refs(
+    store: "ObjectContainer", refs: "Mapping[Ref, ObjectID]"
+) -> "dict[bytes, ObjectID]":
+    """Serialize refs with peeled refs for Git protocol v0/v1.
+
+    This function is used to prepare refs for transmission over the Git protocol.
+    For tags, it includes both the tag object and the dereferenced object.
+
+    Args:
+      store: Object store to peel refs from
+      refs: Dictionary of ref names to SHAs
+
+    Returns:
+      Dictionary with refs and peeled refs (marked with ^{})
+    """
+    import warnings
+
+    from .object_store import peel_sha
+    from .objects import Tag
+
+    ret: dict[bytes, ObjectID] = {}
+    for ref, sha in refs.items():
+        try:
+            unpeeled, peeled = peel_sha(store, ObjectID(sha))
+        except KeyError:
+            warnings.warn(
+                "ref {} points at non-present sha {}".format(
+                    ref.decode("utf-8", "replace"), sha.decode("ascii")
+                ),
+                UserWarning,
+            )
+            continue
+        else:
+            if isinstance(unpeeled, Tag):
+                ret[ref + PEELED_TAG_SUFFIX] = peeled.id
+            ret[ref] = unpeeled.id
+    return ret

dulwich/refs.py

@@ -24,7 +24,6 @@
 
 import os
 import types
-import warnings
 from collections.abc import Callable, Iterable, Iterator, Mapping
 from contextlib import suppress
 from typing import (
@@ -42,7 +41,6 @@ if TYPE_CHECKING:
 from .errors import PackedRefsException, RefFormatError
 from .file import GitFile, ensure_dir_exists
 from .objects import ZERO_SHA, ObjectID, Tag, git_line, valid_hexsha
-from .pack import ObjectContainer
 
 Ref = NewType("Ref", bytes)
 
@@ -774,40 +772,6 @@ class DictRefsContainer(RefsContainer):
         self._peeled.update(peeled)
 
 
-class InfoRefsContainer(RefsContainer):
-    """Refs container that reads refs from a info/refs file."""
-
-    def __init__(self, f: BinaryIO) -> None:
-        """Initialize InfoRefsContainer from info/refs file."""
-        self._refs: dict[Ref, ObjectID] = {}
-        self._peeled: dict[Ref, ObjectID] = {}
-        refs = read_info_refs(f)
-        (self._refs, self._peeled) = split_peeled_refs(refs)
-
-    def allkeys(self) -> set[Ref]:
-        """Return all reference keys."""
-        return set(self._refs.keys())
-
-    def read_loose_ref(self, name: Ref) -> bytes | None:
-        """Read a loose reference."""
-        return self._refs.get(name, None)
-
-    def get_packed_refs(self) -> dict[Ref, ObjectID]:
-        """Get packed references."""
-        return {}
-
-    def get_peeled(self, name: Ref) -> ObjectID | None:
-        """Get peeled version of a reference."""
-        try:
-            return self._peeled[name]
-        except KeyError:
-            ref_value = self._refs.get(name)
-            # Only return if it's an ObjectID (not a symref)
-            if isinstance(ref_value, bytes) and len(ref_value) == 40:
-                return ObjectID(ref_value)
-            return None
-
-
 class DiskRefsContainer(RefsContainer):
     """Refs container that reads refs from disk."""
 
@@ -1444,31 +1408,6 @@ def read_info_refs(f: BinaryIO) -> dict[Ref, ObjectID]:
     return ret
 
 
-def write_info_refs(
-    refs: Mapping[Ref, ObjectID], store: ObjectContainer
-) -> Iterator[bytes]:
-    """Generate info refs."""
-    # TODO: Avoid recursive import :(
-    from .object_store import peel_sha
-
-    # TODO: Move this function to dulwich.protocol
-    from .protocol import PEELED_TAG_SUFFIX
-
-    for name, sha in sorted(refs.items()):
-        # get_refs() includes HEAD as a special case, but we don't want to
-        # advertise it
-        if name == HEADREF:
-            continue
-        try:
-            o = store[sha]
-        except KeyError:
-            continue
-        _unpeeled, peeled = peel_sha(store, sha)
-        yield o.id + b"\t" + name + b"\n"
-        if o.id != peeled.id:
-            yield peeled.id + b"\t" + name + PEELED_TAG_SUFFIX + b"\n"
-
-
 def is_local_branch(x: bytes) -> bool:
     """Check if a ref name is a local branch."""
     return x.startswith(LOCAL_BRANCH_PREFIX)
@@ -1606,36 +1545,6 @@ def shorten_ref_name(ref: bytes) -> bytes:
     return ref
 
 
-def strip_peeled_refs(
-    refs: Mapping[Ref, ObjectID | None],
-) -> dict[Ref, ObjectID | None]:
-    """Remove all peeled refs."""
-    # TODO: Move this function to dulwich.protocol
-    from .protocol import PEELED_TAG_SUFFIX
-
-    return {
-        ref: sha for (ref, sha) in refs.items() if not ref.endswith(PEELED_TAG_SUFFIX)
-    }
-
-
-def split_peeled_refs(
-    refs: Mapping[Ref, ObjectID],
-) -> tuple[dict[Ref, ObjectID], dict[Ref, ObjectID]]:
-    """Split peeled refs from regular refs."""
-    # TODO: Move this function to dulwich.protocol
-    from .protocol import PEELED_TAG_SUFFIX
-
-    peeled: dict[Ref, ObjectID] = {}
-    regular = {k: v for k, v in refs.items() if not k.endswith(PEELED_TAG_SUFFIX)}
-
-    for ref, sha in refs.items():
-        if ref.endswith(PEELED_TAG_SUFFIX):
-            # Peeled refs are always ObjectID values
-            peeled[Ref(ref[: -len(PEELED_TAG_SUFFIX)])] = sha
-
-    return regular, peeled
-
-
 def _set_origin_head(
     refs: RefsContainer, origin: bytes, origin_head: bytes | None
 ) -> None:
@@ -1712,8 +1621,7 @@ def _import_remote_refs(
     prune: bool = False,
     prune_tags: bool = False,
 ) -> None:
-    # TODO: Move this function to dulwich.protocol
-    from .protocol import PEELED_TAG_SUFFIX
+    from .protocol import PEELED_TAG_SUFFIX, strip_peeled_refs
 
     stripped_refs = strip_peeled_refs(refs)
     branches: dict[Ref, ObjectID | None] = {
@@ -1737,43 +1645,6 @@ def _import_remote_refs(
     )
 
 
-def serialize_refs(
-    store: ObjectContainer, refs: Mapping[Ref, ObjectID]
-) -> dict[bytes, ObjectID]:
-    """Serialize refs with peeled refs.
-
-    Args:
-      store: Object store to peel refs from
-      refs: Dictionary of ref names to SHAs
-
-    Returns:
-      Dictionary with refs and peeled refs (marked with ^{})
-    """
-    # TODO: Avoid recursive import :(
-    from .object_store import peel_sha
-
-    # TODO: Move this function to dulwich.protocol
-    from .protocol import PEELED_TAG_SUFFIX
-
-    ret: dict[bytes, ObjectID] = {}
-    for ref, sha in refs.items():
-        try:
-            unpeeled, peeled = peel_sha(store, ObjectID(sha))
-        except KeyError:
-            warnings.warn(
-                "ref {} points at non-present sha {}".format(
-                    ref.decode("utf-8", "replace"), sha.decode("ascii")
-                ),
-                UserWarning,
-            )
-            continue
-        else:
-            if isinstance(unpeeled, Tag):
-                ret[ref + PEELED_TAG_SUFFIX] = peeled.id
-            ret[ref] = unpeeled.id
-    return ret
-
-
 class locked_ref:
     """Lock a ref while making modifications.
 

dulwich/repo.py

@@ -106,7 +106,6 @@ from .refs import (
     SYMREF,  # noqa: F401
     DictRefsContainer,
     DiskRefsContainer,
-    InfoRefsContainer,  # noqa: F401
     Ref,
     RefsContainer,
     _set_default_branch,
@@ -118,7 +117,6 @@ from .refs import (
     local_branch_name,
     read_packed_refs,  # noqa: F401
     read_packed_refs_with_peeled,  # noqa: F401
-    serialize_refs,
     write_packed_refs,  # noqa: F401
 )
 
@@ -670,11 +668,24 @@ class BaseRepo:
           depth: Shallow fetch depth
         Returns: iterator over objects, with __len__ implemented
         """
-        # TODO: serialize_refs returns dict[bytes, ObjectID] with peeled refs (^{}),
-        # but determine_wants expects Mapping[Ref, ObjectID]. Need to reconcile this.
-        refs = serialize_refs(self.object_store, self.get_refs())
+        import logging
 
-        wants = determine_wants(refs, depth)  # type: ignore[arg-type]
+        # Filter out refs pointing to missing objects to avoid errors downstream.
+        # This makes Dulwich more robust when dealing with broken refs on disk.
+        # Previously serialize_refs() did this filtering as a side-effect.
+        all_refs = self.get_refs()
+        refs: dict[Ref, ObjectID] = {}
+        for ref, sha in all_refs.items():
+            if sha in self.object_store:
+                refs[ref] = sha
+            else:
+                logging.warning(
+                    "ref %s points at non-present sha %s",
+                    ref.decode("utf-8", "replace"),
+                    sha.decode("ascii"),
+                )
+
+        wants = determine_wants(refs, depth)
         if not isinstance(wants, list):
             raise TypeError("determine_wants() did not return a list")
 

dulwich/server.py

@@ -118,8 +118,9 @@ from .protocol import (
     format_shallow_line,
     format_unshallow_line,
     symref_capabilities,
+    write_info_refs,
 )
-from .refs import Ref, RefsContainer, write_info_refs
+from .refs import Ref, RefsContainer
 from .repo import Repo
 
 logger = log_utils.getLogger(__name__)

tests/test_refs.py

@@ -30,9 +30,9 @@ from typing import ClassVar
 from dulwich import errors
 from dulwich.file import GitFile
 from dulwich.objects import ZERO_SHA
+from dulwich.protocol import split_peeled_refs, strip_peeled_refs
 from dulwich.refs import (
     DictRefsContainer,
-    InfoRefsContainer,
     NamespacedRefsContainer,
     SymrefLoop,
     _split_ref_line,
@@ -43,8 +43,6 @@ from dulwich.refs import (
     read_packed_refs,
     read_packed_refs_with_peeled,
     shorten_ref_name,
-    split_peeled_refs,
-    strip_peeled_refs,
     write_packed_refs,
 )
 from dulwich.repo import Repo
@@ -1081,55 +1079,6 @@ class DiskRefsContainerPathlibTests(TestCase):
         self.assertEqual(ref_path, os.path.join(worktree_dir.encode(), b"HEAD"))
 
 
-class InfoRefsContainerTests(TestCase):
-    def test_invalid_refname(self) -> None:
-        text = _TEST_REFS_SERIALIZED + b"00" * 20 + b"\trefs/stash\n"
-        refs = InfoRefsContainer(BytesIO(text))
-        expected_refs = dict(_TEST_REFS)
-        del expected_refs[b"HEAD"]
-        expected_refs[b"refs/stash"] = b"00" * 20
-        del expected_refs[b"refs/heads/loop"]
-        self.assertEqual(expected_refs, refs.as_dict())
-
-    def test_keys(self) -> None:
-        refs = InfoRefsContainer(BytesIO(_TEST_REFS_SERIALIZED))
-        actual_keys = set(refs.keys())
-        self.assertEqual(set(refs.allkeys()), actual_keys)
-        expected_refs = dict(_TEST_REFS)
-        del expected_refs[b"HEAD"]
-        del expected_refs[b"refs/heads/loop"]
-        self.assertEqual(set(expected_refs.keys()), actual_keys)
-
-        actual_keys = refs.keys(b"refs/heads")
-        actual_keys.discard(b"loop")
-        self.assertEqual(
-            [b"40-char-ref-aaaaaaaaaaaaaaaaaa", b"master", b"packed"],
-            sorted(actual_keys),
-        )
-        self.assertEqual([b"refs-0.1", b"refs-0.2"], sorted(refs.keys(b"refs/tags")))
-
-    def test_as_dict(self) -> None:
-        refs = InfoRefsContainer(BytesIO(_TEST_REFS_SERIALIZED))
-        # refs/heads/loop does not show up even if it exists
-        expected_refs = dict(_TEST_REFS)
-        del expected_refs[b"HEAD"]
-        del expected_refs[b"refs/heads/loop"]
-        self.assertEqual(expected_refs, refs.as_dict())
-
-    def test_contains(self) -> None:
-        refs = InfoRefsContainer(BytesIO(_TEST_REFS_SERIALIZED))
-        self.assertIn(b"refs/heads/master", refs)
-        self.assertNotIn(b"refs/heads/bar", refs)
-
-    def test_get_peeled(self) -> None:
-        refs = InfoRefsContainer(BytesIO(_TEST_REFS_SERIALIZED))
-        # refs/heads/loop does not show up even if it exists
-        self.assertEqual(
-            _TEST_REFS[b"refs/heads/master"],
-            refs.get_peeled(b"refs/heads/master"),
-        )
-
-
 class ParseSymrefValueTests(TestCase):
     def test_valid(self) -> None:
         self.assertEqual(b"refs/heads/foo", parse_symref_value(b"ref: refs/heads/foo"))