project: centralize safe branch and commit resolution

Resolve commit-ish values through one typed helper using rev-parse
--verify --quiet and, on Git 2.30+, --end-of-options. Reject option-like
revisions on older clients, reuse the helper for project and manifest
resolution, and validate user branch names with check-ref-format
--branch.

Bug: 553599402
Change-Id: I2dda49ff3e781cec14d3b84263a8dace06b4073f
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/623182
Tested-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Brian Gan <brgan@google.com>

git_command.py

@@ -70,6 +70,19 @@ class _GitCall:
 git = _GitCall()
 
 
+def IsValidBranchName(name: str) -> bool:
+    """Return whether |name| is valid where Git expects a branch name."""
+    p = GitCommand(
+        None,
+        ["check-ref-format", "--branch", name],
+        capture_stdout=True,
+        capture_stderr=True,
+        add_event_log=False,
+        log_as_error=False,
+    )
+    return p.Wait() == 0
+
+
 def RepoSourceVersion():
     """Return the version of the repo.git tree."""
     ver = getattr(RepoSourceVersion, "version", None)

manifest_xml.py

@@ -692,9 +692,9 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 e.setAttribute("remote", remoteName)
             if peg_rev:
                 if self.IsMirror:
-                    value = p.bare_git.rev_parse(p.revisionExpr + "^0")
+                    value = p.bare_git.ResolveCommit(p.revisionExpr)
                 else:
-                    value = p.work_git.rev_parse(HEAD + "^0")
+                    value = p.work_git.ResolveCommit(HEAD)
                 e.setAttribute("revision", value)
                 if peg_rev_upstream:
                     if p.upstream:

project.py

@@ -1767,7 +1767,7 @@ class Project:
             return self.GetRevisionId(self._allrefs)
 
         try:
-            return self.bare_git.rev_list(self.revisionExpr, "-1")[0]
+            return self.bare_git.ResolveCommit(self.revisionExpr)
         except GitError:
             raise ManifestInvalidRevisionError(
                 f"revision {self.revisionExpr} in {self.name} not found"
@@ -1800,7 +1800,7 @@ class Project:
             return all_refs[rev]
 
         try:
-            return self.bare_git.rev_parse("--verify", "%s^0" % rev)
+            return self.bare_git.ResolveCommit(rev)
         except GitError:
             raise ManifestInvalidRevisionError(
                 f"revision {self.revisionExpr} in {self.name} not found"
@@ -4581,6 +4581,19 @@ class Project:
                     )
                 return ref
 
+        def ResolveCommit(self, revision: str) -> str:
+            """Resolve |revision| to a commit without option ambiguity."""
+            cmdv = ["--verify", "--quiet"]
+            if git_require((2, 30, 0)):
+                cmdv.append("--end-of-options")
+            elif revision.startswith("-"):
+                raise GitError(
+                    f"invalid revision: {revision}",
+                    project=self._project.name,
+                )
+            cmdv.append(f"{revision}^{{commit}}")
+            return self.rev_parse(*cmdv, log_as_error=False)
+
         def SetHead(self, ref, message=None):
             cmdv = []
             if message is not None:

subcmds/abandon.py

@@ -20,7 +20,7 @@ from command import Command
 from command import DEFAULT_LOCAL_JOBS
 from error import RepoError
 from error import RepoExitError
-from git_command import git
+from git_command import IsValidBranchName
 from progress import Progress
 from repo_logging import RepoLogger
 
@@ -58,9 +58,7 @@ It is equivalent to "git branch -D <branchname>".
 
         if not opt.all:
             branches = args[0].split()
-            invalid_branches = [
-                x for x in branches if not git.check_ref_format(f"heads/{x}")
-            ]
+            invalid_branches = [x for x in branches if not IsValidBranchName(x)]
 
             if invalid_branches:
                 self.OptionParser.error(

subcmds/start.py

@@ -18,7 +18,7 @@ from typing import NamedTuple
 from command import Command
 from command import DEFAULT_LOCAL_JOBS
 from error import RepoExitError
-from git_command import git
+from git_command import IsValidBranchName
 from git_config import IsImmutable
 from progress import Progress
 from repo_logging import RepoLogger
@@ -75,7 +75,7 @@ revision specified in the manifest.
             self.Usage()
 
         nb = args[0]
-        if not git.check_ref_format("heads/%s" % nb):
+        if not IsValidBranchName(nb):
             self.OptionParser.error("'%s' is not a valid name" % nb)
 
     @classmethod

tests/test_git_command.py

@@ -231,6 +231,24 @@ class GitCommandStreamLogsTest(unittest.TestCase):
 class GitCallUnitTest(unittest.TestCase):
     """Tests the _GitCall class (via git_command.git)."""
 
+    def test_valid_branch_name_uses_branch_mode(self) -> None:
+        """Branch validation applies Git's branch-specific restrictions."""
+        command = mock.MagicMock()
+        command.Wait.return_value = 1
+        with mock.patch.object(
+            git_command, "GitCommand", return_value=command
+        ) as check:
+            self.assertFalse(git_command.IsValidBranchName("-topic"))
+
+        check.assert_called_once_with(
+            None,
+            ["check-ref-format", "--branch", "-topic"],
+            capture_stdout=True,
+            capture_stderr=True,
+            add_event_log=False,
+            log_as_error=False,
+        )
+
     def test_version_tuple(self):
         """Check git.version_tuple() handling."""
         ver = git_command.git.version_tuple()

tests/test_project.py

@@ -386,6 +386,49 @@ class ProjectTests(unittest.TestCase):
             proj.work_git.checkout("HEAD~0")
             self.assertEqual(commit_sha, proj.GetHeadRevisionId())
 
+    def test_resolve_commit_uses_end_of_options_when_supported(self) -> None:
+        """Commit resolution separates user revisions from options."""
+        proj = mock.MagicMock(name="project")
+        proj.name = "project"
+        git = project.Project._GitGetByExec(proj, bare=True, gitdir="gitdir")
+        command = mock.MagicMock()
+        command.stdout = "1" * 40 + "\n"
+        command.Wait.return_value = 0
+
+        with mock.patch.object(
+            project, "git_require", return_value=True
+        ), mock.patch.object(
+            project, "GitCommand", return_value=command
+        ) as cmd:
+            self.assertEqual("1" * 40, git.ResolveCommit("topic"))
+
+        cmd.assert_called_once_with(
+            proj,
+            [
+                "rev-parse",
+                "--verify",
+                "--quiet",
+                "--end-of-options",
+                "topic^{commit}",
+            ],
+            bare=True,
+            gitdir="gitdir",
+            capture_stdout=True,
+            capture_stderr=True,
+            verify_command=True,
+            log_as_error=False,
+        )
+
+    def test_resolve_commit_rejects_option_on_old_git(self) -> None:
+        """Old Git never receives a revision that looks like an option."""
+        proj = mock.MagicMock(name="project")
+        proj.name = "project"
+        git = project.Project._GitGetByExec(proj, bare=True, gitdir="gitdir")
+
+        with mock.patch.object(project, "git_require", return_value=False):
+            with self.assertRaises(error.GitError):
+                git.ResolveCommit("--not-a-revision")
+
     def test_get_branches_reuses_ref_snapshot_for_current_branch(self) -> None:
         """GetBranches derives the current branch from the loaded refs."""
         with utils_for_test.TempGitTree() as tempdir: