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>
diff --git a/git_command.py b/git_command.py index ebba926..7add447 100644 --- a/git_command.py +++ b/git_command.py
@@ -70,6 +70,19 @@ 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)
diff --git a/manifest_xml.py b/manifest_xml.py index dd00868..379ce09 100644 --- a/manifest_xml.py +++ b/manifest_xml.py
@@ -692,9 +692,9 @@ 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:
diff --git a/project.py b/project.py index f4b4d5d..036e9fe 100644 --- a/project.py +++ b/project.py
@@ -1767,7 +1767,7 @@ 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 @@ 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 @@ ) 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:
diff --git a/subcmds/abandon.py b/subcmds/abandon.py index f1688e7..b266900 100644 --- a/subcmds/abandon.py +++ b/subcmds/abandon.py
@@ -20,7 +20,7 @@ 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 @@ 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(
diff --git a/subcmds/start.py b/subcmds/start.py index 73dddf3..5817bf6 100644 --- a/subcmds/start.py +++ b/subcmds/start.py
@@ -18,7 +18,7 @@ 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 @@ 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
diff --git a/tests/test_git_command.py b/tests/test_git_command.py index 5bdaca5..2d8b0af 100644 --- a/tests/test_git_command.py +++ b/tests/test_git_command.py
@@ -231,6 +231,24 @@ 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()
diff --git a/tests/test_project.py b/tests/test_project.py index 08d2806..06d7c76 100644 --- a/tests/test_project.py +++ b/tests/test_project.py
@@ -386,6 +386,49 @@ 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: