Address rev-parse review feedback

codex · Byron · commit 6cf7ac33d449 · 2026-04-29T07:31:08.000+08:00
diff --git a/git/repo/fun.py b/git/repo/fun.py
@@ -35,7 +35,7 @@
 
 # Typing ----------------------------------------------------------------------
 
-from typing import Optional, TYPE_CHECKING, Tuple, Union, cast, overload
+from typing import Iterator, Optional, TYPE_CHECKING, Tuple, Union, cast, overload
 
 from git.types import AnyGitObject, Literal, PathLike
 
@@ -190,10 +190,6 @@ def name_to_object(repo: "Repo", name: str, return_ref: bool = False) -> Union[A
         # END handle short shas
     # END find sha if it matches
 
-    if hexsha is None:
-        hexsha = _describe_to_long(repo, name)
-    # END handle describe output
-
     # If we couldn't find an object for what seemed to be a short hexsha, try to find it
     # as reference anyway, it could be named 'aaa' for instance.
     if hexsha is None:
@@ -216,6 +212,10 @@ def name_to_object(repo: "Repo", name: str, return_ref: bool = False) -> Union[A
         # END for each base
     # END handle hexsha
 
+    if hexsha is None:
+        hexsha = _describe_to_long(repo, name)
+    # END handle describe output
+
     # Didn't find any ref, this is an error.
     if return_ref:
         raise BadObject("Couldn't find reference named %r" % name)
@@ -363,6 +363,8 @@ def _tracking_branch_object(repo: "Repo", ref: Optional[SymbolicReference]) -> A
             raise BadName("@{upstream}") from e
     elif isinstance(ref, Head):
         head = ref
+    elif os.fspath(ref.path).startswith("refs/heads/"):
+        head = Head(repo, ref.path)
     else:
         raise BadName("%s@{upstream}" % ref.name)
     # END handle head
@@ -479,11 +481,15 @@ def _find_commit_by_message(
     repo: "Repo", rev: Optional[AnyGitObject], pattern: str, braced: bool = False
 ) -> AnyGitObject:
     pattern, negated = _parse_search(_unescape_braced_regex(pattern) if braced else pattern)
-    regex = re.compile(pattern)
+    try:
+        regex = re.compile(pattern)
+    except re.error as e:
+        raise ValueError("Invalid commit message regex %r" % pattern) from e
+    # END handle invalid regex
     if rev is None:
-        commits = repo.iter_commits("--all")
+        commits = _all_ref_commits(repo)
     else:
-        commits = repo.iter_commits(to_commit(cast(Object, rev)).hexsha)
+        commits = _reachable_commits([to_commit(cast(Object, rev))])
     # END handle starting point
 
     for commit in commits:
@@ -499,6 +505,38 @@ def _find_commit_by_message(
     raise BadName("No commit found matching message pattern %r" % pattern)
 
 
+def _all_ref_commits(repo: "Repo") -> Iterator["Commit"]:
+    starts = []
+    for ref in repo.references:
+        try:
+            starts.append(to_commit(cast(Object, ref.object)))
+        except (BadName, ValueError):
+            pass
+        # END skip refs that do not point to commits
+    # END for each ref
+    try:
+        starts.append(repo.head.commit)
+    except ValueError:
+        pass
+    # END handle unborn head
+    return _reachable_commits(starts)
+
+
+def _reachable_commits(starts: list["Commit"]) -> Iterator["Commit"]:
+    seen = set()
+    pending = starts[:]
+    while pending:
+        pending.sort(key=lambda commit: commit.committed_date, reverse=True)
+        commit = pending.pop(0)
+        if commit.binsha in seen:
+            continue
+        # END skip seen commit
+        seen.add(commit.binsha)
+        yield commit
+        pending.extend(commit.parents)
+    # END while commits remain
+
+
 def _index_lookup(repo: "Repo", spec: str) -> AnyGitObject:
     if not spec:
         raise ValueError("':' must be followed by a path")
@@ -527,8 +565,6 @@ def _tree_lookup(obj: AnyGitObject, path: str) -> AnyGitObject:
 
 
 def _peel(obj: AnyGitObject, output_type: str, repo: "Repo", rev: str) -> AnyGitObject:
-    if output_type == "/":
-        return obj
     if output_type.startswith("/"):
         return _find_commit_by_message(repo, obj, output_type[1:], braced=True)
     if output_type == "":
diff --git a/test/test_rev_parse.py b/test/test_rev_parse.py
@@ -1,8 +1,15 @@
+# Copyright (C) 2026 Michael Trier (mtrier@gmail.com) and contributors
+#
+# This module is part of GitPython and is released under the
+# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
+
 from pathlib import Path
 
 import pytest
 
 from git import Repo
+from git.refs import RemoteReference
+from git.refs import SymbolicReference
 from gitdb.exc import BadName
 
 
@@ -31,14 +38,12 @@ def rev_parse_repo(tmp_path):
     repo.create_tag("v1.0", ref=release)
     main = repo.active_branch
 
-    side = repo.create_head("side", root)
-    side.checkout()
     _write(repo, "side.txt", "side\n")
-    side_commit = repo.index.commit("side branch")
+    side_commit = repo.index.commit("side branch", parent_commits=[root], head=False, skip_hooks=True)
+    repo.create_head("side", side_commit)
 
-    main.checkout()
-    repo.git.merge("--no-ff", "side", "-m", "merge side")
-    merge = repo.head.commit
+    merge = repo.index.commit("merge side", parent_commits=[release, side_commit], skip_hooks=True)
+    repo.head.log_append(side_commit.binsha, "checkout: moving from side to main", merge.binsha)
 
     repo.create_head("aaaaaaaa", merge)
     repo.create_tag("@foo", ref=merge)
@@ -55,16 +60,21 @@ def rev_parse_repo(tmp_path):
 
 def test_rev_parse_names_hex_and_describe_forms(rev_parse_repo):
     repo = rev_parse_repo["repo"]
+    release = rev_parse_repo["release"]
     merge = rev_parse_repo["merge"]
 
     assert repo.rev_parse("@") == merge
     assert repo.rev_parse("@foo") == merge
     assert repo.rev_parse("aaaaaaaa") == merge
     assert repo.rev_parse(merge.hexsha[:7]) == merge
+    describe_name = "anything-9-g%s" % merge.hexsha[:7]
     assert repo.rev_parse("v1.0-1-g%s" % merge.hexsha[:7]) == merge
-    assert repo.rev_parse("anything-9-g%s" % merge.hexsha[:7]) == merge
+    assert repo.rev_parse(describe_name) == merge
     assert repo.rev_parse("%s-dirty" % merge.hexsha[:7]) == merge
 
+    repo.create_tag(describe_name, ref=release)
+    assert repo.rev_parse(describe_name) == release
+
 
 def test_rev_parse_navigation_and_peeling(rev_parse_repo):
     repo = rev_parse_repo["repo"]
@@ -87,7 +97,8 @@ def test_rev_parse_navigation_and_peeling(rev_parse_repo):
     assert repo.rev_parse("ann^{}") == root
     assert repo.rev_parse("ann^{commit}") == root
     assert repo.rev_parse("HEAD^{tree}") == merge.tree
-    assert repo.rev_parse("HEAD^{/}") == merge
+    with pytest.raises(ValueError):
+        repo.rev_parse("HEAD^{/}")
 
 
 def test_rev_parse_tree_and_index_paths(rev_parse_repo):
@@ -114,6 +125,10 @@ def test_rev_parse_reflog_selectors(rev_parse_repo):
     assert repo.rev_parse("%s@{0}" % main.name) == merge
     assert repo.rev_parse("@{-1}") == side
 
+    SymbolicReference.create(repo, "refs/remotes/origin/%s" % main.name, merge)
+    main.set_tracking_branch(RemoteReference(repo, "refs/remotes/origin/%s" % main.name))
+    assert repo.rev_parse("%s@{upstream}" % main.name) == merge
+
 
 def test_rev_parse_commit_message_search(rev_parse_repo):
     repo = rev_parse_repo["repo"]
@@ -132,6 +147,10 @@ def test_rev_parse_rejects_invalid_object_specs(rev_parse_repo):
         repo.rev_parse(":")
     with pytest.raises(ValueError):
         repo.rev_parse(":/")
+    with pytest.raises(ValueError):
+        repo.rev_parse(":/[")
+    with pytest.raises(ValueError):
+        repo.rev_parse("HEAD^{/[}")
     with pytest.raises(ValueError):
         repo.rev_parse("@{-0}")
     with pytest.raises(ValueError):
