Improve filter specification validation and error messages

dulwich/partial_clone.py

@@ -262,36 +262,87 @@ def parse_filter_spec(spec: str | bytes) -> FilterSpec:
 
     Raises:
         ValueError: If spec is not a valid filter specification
+
+    Examples:
+        >>> parse_filter_spec("blob:none")
+        BlobNoneFilter()
+        >>> parse_filter_spec("blob:limit=1m")
+        BlobLimitFilter(limit=1048576)
+        >>> parse_filter_spec("tree:0")
+        TreeDepthFilter(max_depth=0)
     """
     if isinstance(spec, bytes):
-        spec = spec.decode("utf-8")
+        try:
+            spec = spec.decode("utf-8")
+        except UnicodeDecodeError as e:
+            raise ValueError(f"Filter specification must be valid UTF-8: {e}")
 
     spec = spec.strip()
 
+    if not spec:
+        raise ValueError("Filter specification cannot be empty")
+
     if spec == "blob:none":
         return BlobNoneFilter()
     elif spec.startswith("blob:limit="):
         limit_str = spec[11:]  # len('blob:limit=') == 11
-        limit = _parse_size(limit_str)
-        return BlobLimitFilter(limit)
+        if not limit_str:
+            raise ValueError("blob:limit requires a size value (e.g., blob:limit=1m)")
+        try:
+            limit = _parse_size(limit_str)
+            if limit < 0:
+                raise ValueError(f"blob:limit size must be non-negative, got {limit_str}")
+            return BlobLimitFilter(limit)
+        except ValueError as e:
+            raise ValueError(f"Invalid blob:limit specification: {e}")
     elif spec.startswith("tree:"):
         depth_str = spec[5:]  # len('tree:') == 5
+        if not depth_str:
+            raise ValueError("tree filter requires a depth value (e.g., tree:0)")
         try:
             depth = int(depth_str)
+            if depth < 0:
+                raise ValueError(f"tree depth must be non-negative, got {depth}")
             return TreeDepthFilter(depth)
-        except ValueError:
-            raise ValueError(f"Invalid tree depth: {depth_str}")
+        except ValueError as e:
+            raise ValueError(f"Invalid tree filter: {e}")
     elif spec.startswith("sparse:oid="):
         oid_str = spec[11:]  # len('sparse:oid=') == 11
-        # Convert to bytes for OID
-        oid = oid_str.encode("ascii")
+        if not oid_str:
+            raise ValueError("sparse:oid requires an object ID (e.g., sparse:oid=abc123...)")
+        # Validate OID format (should be 40 hex chars for SHA-1 or 64 for SHA-256)
+        if len(oid_str) not in (40, 64):
+            raise ValueError(
+                f"sparse:oid requires a valid object ID (40 or 64 hex chars), got {len(oid_str)} chars"
+            )
+        try:
+            # Convert to bytes and validate hex
+            oid = oid_str.encode("ascii")
+            int(oid_str, 16)  # Validate it's valid hex
+        except (ValueError, UnicodeEncodeError):
+            raise ValueError(f"sparse:oid must be a hexadecimal object ID, got: {oid_str}")
         return SparseOidFilter(oid)
     elif spec.startswith("combine:"):
-        filter_specs = spec[8:].split("+")  # len('combine:') == 8
-        filters = [parse_filter_spec(f) for f in filter_specs]
+        filter_str = spec[8:]  # len('combine:') == 8
+        if not filter_str:
+            raise ValueError("combine filter requires at least one filter (e.g., combine:blob:none+tree:0)")
+        filter_specs = filter_str.split("+")
+        if len(filter_specs) < 2:
+            raise ValueError(
+                "combine filter requires at least two filters separated by '+'"
+            )
+        try:
+            filters = [parse_filter_spec(f) for f in filter_specs]
+        except ValueError as e:
+            raise ValueError(f"Invalid filter in combine specification: {e}")
         return CombineFilter(filters)
     else:
-        raise ValueError(f"Unknown filter specification: {spec}")
+        # Provide helpful error message with supported formats
+        raise ValueError(
+            f"Unknown filter specification: '{spec}'. "
+            f"Supported formats: blob:none, blob:limit=<n>[kmg], tree:<depth>, "
+            f"sparse:oid=<oid>, combine:<filter>+<filter>+..."
+        )
 
 
 def filter_pack_objects(

tests/test_partial_clone.py

@@ -122,13 +122,64 @@ class ParseFilterSpecTests(TestCase):
         """Test that invalid tree depth raises ValueError."""
         with self.assertRaises(ValueError) as cm:
             parse_filter_spec("tree:invalid")
-        self.assertIn("Invalid tree depth", str(cm.exception))
+        self.assertIn("Invalid tree filter", str(cm.exception))
 
     def test_parse_invalid_blob_limit(self):
         """Test that invalid blob limit raises ValueError."""
         with self.assertRaises(ValueError) as cm:
             parse_filter_spec("blob:limit=invalid")
-        self.assertIn("Invalid size specification", str(cm.exception))
+        self.assertIn("Invalid", str(cm.exception))
+
+    def test_parse_empty_spec(self):
+        """Test that empty filter spec raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("")
+        self.assertIn("cannot be empty", str(cm.exception))
+
+    def test_parse_blob_limit_no_value(self):
+        """Test that blob:limit without value raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("blob:limit=")
+        self.assertIn("requires a size value", str(cm.exception))
+
+    def test_parse_tree_no_value(self):
+        """Test that tree: without depth raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("tree:")
+        self.assertIn("requires a depth value", str(cm.exception))
+
+    def test_parse_tree_negative_depth(self):
+        """Test that negative tree depth raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("tree:-1")
+        self.assertIn("non-negative", str(cm.exception))
+
+    def test_parse_sparse_oid_invalid_length(self):
+        """Test that invalid OID length raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("sparse:oid=abc123")
+        self.assertIn("40 or 64 hex chars", str(cm.exception))
+
+    def test_parse_sparse_oid_invalid_hex(self):
+        """Test that non-hex OID raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("sparse:oid=" + "x" * 40)
+        self.assertIn("hexadecimal", str(cm.exception))
+
+    def test_parse_combine_single_filter(self):
+        """Test that combine with single filter raises ValueError."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("combine:blob:none")
+        self.assertIn("at least two filters", str(cm.exception))
+
+    def test_parse_unknown_with_helpful_message(self):
+        """Test that unknown spec gives helpful error message."""
+        with self.assertRaises(ValueError) as cm:
+            parse_filter_spec("unknown:spec")
+        error_msg = str(cm.exception)
+        self.assertIn("Unknown filter specification", error_msg)
+        self.assertIn("Supported formats", error_msg)
+        self.assertIn("blob:none", error_msg)
 
 
 class BlobNoneFilterTests(TestCase):