Address review feedback about unsafe option abbreviations

codex · Byron · commit 25c684bf6500 · 2026-07-12T13:53:54.000+02:00
The prefix check could reject otherwise allowed one-letter short options when an unrelated blocked long option began with the same character. Keep exact matches for every spelling, but restrict prefix matching to explicit long options and multi-character kwargs.

This addresses the review comment: single-letter short options and kwargs must not be treated as long-option abbreviations. It also adds coverage for abbreviated unsafe kwargs so both clone validation paths remain protected.
diff --git a/git/cmd.py b/git/cmd.py
@@ -970,12 +970,20 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) ->
         # Git accepts any unambiguous prefix of a long option, so an abbreviated
         # spelling such as `--upl` for `--upload-pack` must be rejected too. An
         # option is unsafe if its canonical name is a prefix of any blocked
-        # option's canonical name.
+        # option's canonical name. Only long options and multi-character kwargs
+        # can be abbreviations; single-character short options remain exact-match
+        # only.
         canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options}
+        options_are_kwargs = all(not option.startswith("-") for option in options)
         for option in options:
             candidate = cls._canonicalize_option_name(option)
             if not candidate:
                 continue
+            unsafe_option = canonical_unsafe_options.get(candidate)
+            if unsafe_option is not None:
+                raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
+            if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)):
+                continue
             for canonical, unsafe_option in canonical_unsafe_options.items():
                 if canonical.startswith(candidate):
                     raise UnsafeOptionError(
diff --git a/test/test_clone.py b/test/test_clone.py
@@ -153,6 +153,16 @@ def test_clone_unsafe_options_abbreviated(self, rw_repo):
                     rw_repo.clone(tmp_dir, multi_options=[unsafe_option])
                 assert not tmp_file.exists()
 
+            unsafe_kwargs = [
+                {"upl": f"touch {tmp_file}"},
+                {"upload_pac": f"touch {tmp_file}"},
+                {"conf": "protocol.ext.allow=always"},
+            ]
+            for unsafe_option in unsafe_kwargs:
+                with self.assertRaises(UnsafeOptionError):
+                    rw_repo.clone(tmp_dir, **unsafe_option)
+                assert not tmp_file.exists()
+
     @with_rw_repo("HEAD")
     def test_clone_unsafe_options_are_checked_after_splitting_multi_options(self, rw_repo):
         with tempfile.TemporaryDirectory() as tdir:
diff --git a/test/test_git.py b/test/test_git.py
@@ -170,6 +170,16 @@ def test_check_unsafe_options_normalizes_kwargs(self):
             with self.assertRaises(UnsafeOptionError):
                 Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
 
+    def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self):
+        unsafe_options = ["--upload-pack"]
+
+        Git.check_unsafe_options(options=["-u", "u", "-upl"], unsafe_options=unsafe_options)
+        with self.assertRaises(UnsafeOptionError):
+            Git.check_unsafe_options(options=["--u"], unsafe_options=unsafe_options)
+
+    def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self):
+        Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"])
+
     _shell_cases = (
         # value_in_call, value_from_class, expected_popen_arg
         (None, False, False),
