fix: read reference arguments by exact name in revision APIs

doc/source/changes.rst

@@ -57,7 +57,12 @@ API changes
 * Reference reads require the exact name. A missing branch no longer resolves to
   a similarly named tag, so an unborn branch's initial commit has no parent.
   A tag named ``REBASE_HEAD`` no longer reports a rebase in progress. Revision
-  APIs such as ``Repo.commit(rev)`` retain Git's name guessing.
+  APIs such as ``Repo.commit(rev)`` retain Git's name guessing for strings.
+  A reference object passed as a revision, for example to ``Repo.commit()``,
+  ``Repo.iter_commits()``, ``Repo.merge_base()``, ``IndexFile.from_tree()`` or
+  ``HEAD.reset()``, is read by its exact name, so a tag named like a branch or
+  remote-tracking branch no longer takes its place. A missing reference raises
+  ``ValueError``, as its ``object`` property does.
 * Packed tag names containing non-UTF-8 bytes are represented losslessly as
   surrogate-escaped strings. ``safe_encode()`` recovers their original bytes,
   and Gix reference validation and resolution receive those bytes directly.

git/cmd.py

@@ -1023,6 +1023,21 @@ def _check_operand(value: Any, label: str = "operand") -> str:
             raise UnsafeOptionError(f"Invalid {label}: {value!r}")
         return value
 
+    @staticmethod
+    def _check_revision(value: Any, label: str = "revision") -> str:
+        """Validate a revision, passing a reference object as the object ID it holds.
+
+        A reference converts to its short name, for which Git's revision lookup tries
+        other references, such as a same-named tag, before ``refs/heads/`` and
+        ``refs/remotes/``. Read the exact reference instead, as
+        ``SymbolicReference.object`` does.
+        """
+        from git.refs.symbolic import SymbolicReference
+
+        if isinstance(value, SymbolicReference):
+            value = value.dereference_recursive(value.repo, value.path)
+        return Git._check_operand(value, label)
+
     def _require_version(self) -> None:
         key = None
         if self._version_info_token is not self._refresh_token:

git/diff.py

@@ -261,8 +261,10 @@ def diff(
                 clusterable_short_options="46abceflmnpqrstuvwzBCDMNRW",
             )
 
-        if isinstance(other, str):
-            other = self.repo.rev_parse(Git._check_operand(other, "revision")).hexsha
+        from git.refs.symbolic import SymbolicReference
+
+        if isinstance(other, (str, SymbolicReference)):
+            other = self.repo.rev_parse(Git._check_revision(other)).hexsha
         native = _backend.tree_diff(self.repo, self, other, paths, create_patch, kwargs)
         if native is not NotImplemented:
             return native

git/index/base.py

@@ -280,8 +280,7 @@ def _tree_oid(self, tree: Treeish) -> str:
             tree = tree[0]
         if isinstance(tree, bytes):
             tree = tree.hex() if len(tree) == self.repo._oid_size else tree.decode("ascii")
-        value = str(tree)
-        Git._check_operand(value, "tree revision")
+        value = Git._check_revision(tree, "tree revision")
         return self.repo.git._call_process_safe("rev_parse", "--verify", "--end-of-options", value + "^{tree}")
 
     def _index_path(self) -> PathLike:

git/objects/base.py

@@ -9,6 +9,7 @@
 
 import gitdb.typ as dbtyp
 
+from git.cmd import Git
 from git.compat import force_text
 from git.exc import WorkTreeRepositoryUnsupported
 from git.util import LazyMixin, bin_to_hex, join_path_native, stream_copy
@@ -138,7 +139,7 @@ def new(cls, repo: "Repo", id: Union[str, "Reference"]) -> AnyGitObject:
             This cannot be a ``__new__`` method as it would always call :meth:`__init__`
             with the input id which is not necessarily a binsha.
         """
-        return repo.rev_parse(str(id))
+        return repo.rev_parse(Git._check_revision(id))
 
     @classmethod
     def new_from_sha(cls, repo: "Repo", sha1: bytes) -> AnyGitObject:

git/objects/commit.py

@@ -465,8 +465,8 @@ def iter_items(
             args_list.extend(paths_tup)
         # END if paths
 
-        Git._check_operand(str(rev), "revision")
-        native = _backend.history(repo.git, str(rev), paths, kwargs)
+        rev = Git._check_revision(rev)
+        native = _backend.history(repo.git, rev, paths, kwargs)
         if native is not NotImplemented:
             return (cls(repo, hex_to_bin(oid)) for oid in native)
         proc = repo.git._call_process_safe("rev_list", rev, args_list, as_process=True, **kwargs)

git/refs/head.py

@@ -121,7 +121,7 @@ def reset(
         # END working tree handling
 
         try:
-            oid = self.repo.rev_parse(Git._check_operand(commit, "revision")).hexsha
+            oid = self.repo.rev_parse(Git._check_revision(commit)).hexsha
             self.repo.git._call_process_safe("reset", mode, oid, "--", paths, **kwargs)
         except GitCommandError as e:
             # git nowadays may use 1 as status to indicate there are still unstaged

git/refs/symbolic.py

@@ -575,7 +575,7 @@ def _create(
     ) -> T_References:
         full_path = cls.to_full_path(path)
         cls._get_validated_ref_path(repo, full_path)
-        target = repo.rev_parse(str(reference)) if resolve else reference
+        target = repo.rev_parse(Git._check_revision(reference)) if resolve else reference
         ref = cls(repo, full_path)
         if force:
             ref.set_reference(target, logmsg)

git/refs/tag.py

@@ -170,7 +170,7 @@ def create(
 
         name = Git._check_operand(path, "tag")
         cls._get_validated_ref_path(repo, cls.to_full_path(name))
-        reference = Git._check_operand(reference, "revision")
+        reference = Git._check_revision(reference)
         oid = repo.rev_parse(reference).hexsha
         options = [] if allow_unsafe_options else ["--no-sign"]
         repo.git._call_process_safe("tag", *options, "--", name, oid, **kwargs)

git/repo/base.py

@@ -909,7 +909,7 @@ def commit(self, rev: Union[str, Commit_ish, None] = None) -> Commit:
         """
         if rev is None:
             return self.head.commit
-        return to_commit(self.rev_parse(str(rev)))
+        return to_commit(self.rev_parse(Git._check_revision(rev)))
 
     def iter_trees(self, *args: Any, **kwargs: Any) -> Iterator["Tree"]:
         """:return: Iterator yielding :class:`~git.objects.tree.Tree` objects
@@ -939,7 +939,7 @@ def tree(self, rev: Union[Tree_ish, str, None] = None) -> "Tree":
         """
         if rev is None:
             return self.head.commit.tree
-        obj = self.rev_parse(str(rev))
+        obj = self.rev_parse(Git._check_revision(rev))
         if obj.type == "tree":
             obj.path = getattr(obj, "path", "")
             return obj
@@ -1029,10 +1029,12 @@ def merge_base(self, *rev: TBD, allow_unsafe_options: bool = False, **kwargs: An
                 options=Git._option_candidates(rev, kwargs), unsafe_options=self.unsafe_git_revision_options
             )
 
+        # Resolve reference objects before flattening turns them into short names.
+        revs = [item if isinstance(item, (list, tuple)) else Git._check_revision(item) for item in rev]
         res: List[Commit] = []
         try:
             lines: List[str] = self.git._call_process_safe(
-                "merge_base", "--", *(Git._check_operand(item, "revision") for item in Git._unpack_args(rev)), **kwargs
+                "merge_base", "--", *(Git._check_operand(item, "revision") for item in Git._unpack_args(revs)), **kwargs
             ).splitlines()
         except GitCommandError as err:
             if err.status != 1:
@@ -1065,8 +1067,8 @@ def is_ancestor(self, ancestor_rev: Commit, rev: Commit) -> bool:
                 "merge_base",
                 "--is-ancestor",
                 "--",
-                Git._check_operand(ancestor_rev, "revision"),
-                Git._check_operand(rev, "revision"),
+                Git._check_revision(ancestor_rev),
+                Git._check_revision(rev),
             )
         except GitCommandError as err:
             if err.status == 1:
@@ -1303,7 +1305,7 @@ def blame_incremental(
 
         data: bytes = self.git._call_process_safe(
             "blame",
-            Git._check_operand(rev, "revision") if rev is not None else None,
+            Git._check_revision(rev) if rev is not None else None,
             "--no-textconv" if not allow_unsafe_options else None,
             "--",
             file,
@@ -1424,7 +1426,7 @@ def blame(
             )
         data: bytes = self.git._call_process_safe(
             "blame",
-            Git._check_operand(rev, "revision") if rev is not None else None,
+            Git._check_revision(rev) if rev is not None else None,
             *rev_opts_list,
             "--no-textconv" if not allow_unsafe_options else None,
             "--",
@@ -1925,7 +1927,7 @@ def archive(
         self.git._call_process_safe(
             "archive",
             "--",
-            Git._check_operand(treeish, "revision"),
+            Git._check_revision(treeish),
             *path,
             _allow_network=bool(kwargs.get("remote")),
             _config=()

test/test_refs.py

@@ -4,8 +4,10 @@
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
 import contextlib
+import io
 import os
 import os.path as osp
+import tarfile
 import unittest
 from itertools import chain
 from pathlib import Path
@@ -22,6 +24,8 @@
     GitConfigParser,
     Git,
     Head,
+    IndexFile,
+    Object,
     Reference,
     RefLog,
     Remote,
@@ -222,6 +226,39 @@ def test_initial_commit_does_not_inherit_lookalike_tag_history(object_format, re
         assert repo.git.rev_list("--count", "HEAD") == "1"
 
 
+@pytest.mark.parametrize("object_format", ["sha1", "sha256"])
+@pytest.mark.parametrize("ref_format", ["files", "reftable"])
+def test_reference_arguments_do_not_resolve_same_named_tags(object_format, ref_format):
+    with fixture_repo("exact-refs.sh", object_format, ref_format, "shadowed", write=True) as repo:
+        local = repo.heads.main.commit
+        foreign = repo.tags.main.commit
+        assert repo.tags["origin/main"].commit == foreign != local
+        # Git's name guessing prefers the tags, as for any short name.
+        assert repo.commit("main") == repo.commit("origin/main") == foreign
+        branches = (repo.heads.main, RemoteReference(repo, "refs/remotes/origin/main"))
+        for index, ref in enumerate(branches):
+            assert repo.commit(ref) == Object.new(repo, ref) == local
+            assert repo.tree(ref) == local.tree
+            assert list(repo.iter_commits(ref)) == [local]
+            assert repo.merge_base(ref, local) == [local]
+            assert not repo.is_ancestor(foreign, ref)
+            assert local.diff(ref) == []
+            assert [commit for commit, _ in repo.blame(ref, "file")] == [local]
+            assert [entry.commit for entry in repo.blame_incremental(ref, "file")] == [local]
+            stream = io.BytesIO()
+            repo.archive(stream, ref)
+            with tarfile.open(fileobj=io.BytesIO(stream.getvalue())) as archive:
+                assert archive.getnames() == ["file"]
+            assert IndexFile.from_tree(repo, ref).write_tree() == local.tree
+            assert repo.create_head(f"copy-{index}", ref).commit == local
+            assert repo.create_tag(f"copy-{index}", ref=ref).commit == local
+            repo.head.reset(ref, index=True)
+            repo.index.reset(ref, head=True)
+            repo.index.merge_tree(ref)
+            assert repo.head.commit == local
+            assert repo.index.write_tree() == local.tree
+
+
 @pytest.mark.parametrize("object_format", ["sha1", "sha256"])
 @pytest.mark.parametrize("ref_format", ["files", "reftable"])
 def test_rebase_detection_ignores_tag_named_rebase_head(object_format, ref_format):

tests/fixtures/exact-refs.sh

@@ -15,6 +15,17 @@ git -C repo fetch ../source 'refs/tags/*:refs/tags/*'
 if [ "${3-}" = symbolic ]; then
     git -C repo symbolic-ref refs/tags/REBASE_HEAD refs/tags/refs/heads/unborn
 fi
+if [ "${3-}" = shadowed ]; then
+    # Fetched tags share the short names of a local branch and a remote-tracking branch.
+    git -C repo fetch ../source 'refs/heads/main:refs/tags/main' 'refs/heads/main:refs/tags/origin/main'
+    blob=$(printf 'local\n' | git -C repo hash-object -w --stdin)
+    tree=$(printf '100644 blob %s\tfile\n' "$blob" | git -C repo mktree)
+    commit=$(git -C repo commit-tree -m 'Local history' "$tree")
+    for path in refs/heads/main refs/remotes/origin/main; do
+        git -C repo update-ref "$path" "$commit"
+    done
+    git -C repo symbolic-ref HEAD refs/heads/main
+fi
 if [ "${3-}" = existing ]; then
     commit=$(git -C source rev-parse HEAD)
     for path in refs/heads/unborn refs/stash ORIG_HEAD REBASE_HEAD; do