Address review feedback about Gitfile handling

Review feedback identified that chained or self-referential .git pointers recurse, filesystem-encoded metadata can fail text decoding, and a relative GIT_DIR is not retained for later Git commands.

Parse one regular, size-bounded Gitfile exactly once, decode Gitfile and commondir paths with the filesystem codec, and retain the resolved GIT_DIR for subprocesses. This rejects cycles like Git instead of recursing and keeps commands stable after working-directory changes.

Git baseline: 15c6308cf7ad276b306aa5b3ababfbdebfb1a917, setup.c read_gitfile_gently() and get_common_dir_noenv().

Validation: 7 focused tests and 12 subtests; Ruff check and format; mypy; compileall; git diff --check.

Author: codex

git/repo/base.py

@@ -267,7 +267,8 @@ def __init__(
             :class:`Repo`
         """
 
-        epath = path or os.getenv("GIT_DIR")
+        git_dir_env = os.getenv("GIT_DIR")
+        epath = path or git_dir_env
         if not epath:
             epath = os.getcwd()
         epath = os.fspath(epath)
@@ -361,7 +362,7 @@ def __init__(
             self._common_dir = osp.abspath(common_dir_env)
         else:
             try:
-                common_dir = (Path(self.git_dir) / "commondir").read_text().splitlines()[0].strip()
+                common_dir = os.fsdecode((Path(self.git_dir) / "commondir").read_bytes()).rstrip("\r\n")
                 self._common_dir = osp.join(self.git_dir, common_dir)
             except OSError:
                 self._common_dir = ""
@@ -383,6 +384,8 @@ def __init__(
         self.git = self.GitCommandWrapperType(self.working_dir)
         if common_dir_env is not None:
             self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir))
+        elif git_dir_env is not None:
+            self.git.update_environment(GIT_DIR=os.fspath(self.git_dir))
 
         # Special handling, in special times.
         rootpath = osp.join(self.common_dir, "objects")

git/repo/fun.py

@@ -117,44 +117,33 @@ def find_worktree_git_dir(dotgit: PathLike) -> Optional[str]:
         statbuf = os.stat(dotgit)
     except OSError:
         return None
-    if not stat.S_ISREG(statbuf.st_mode):
+    if not stat.S_ISREG(statbuf.st_mode) or statbuf.st_size > (1 << 20):
         return None
 
     try:
-        lines = Path(dotgit).read_text().splitlines()
-        for key, value in [line.strip().split(": ") for line in lines]:
-            if key == "gitdir":
-                return value
-    except ValueError:
-        pass
-    return None
+        content = os.fsdecode(Path(dotgit).read_bytes()).rstrip("\r\n")
+    except OSError:
+        return None
+    return content[8:] if len(content) >= 9 and content.startswith("gitdir: ") else None
 
 
 def find_submodule_git_dir(d: PathLike) -> Optional[PathLike]:
     """Search for a submodule repo."""
     if is_git_dir(d):
         return d
 
-    try:
-        with open(d) as fp:
-            content = fp.read().rstrip()
-    except IOError:
-        # It's probably not a file.
-        pass
-    else:
-        if content.startswith("gitdir: "):
-            path = content[8:]
-
-            if Git.is_cygwin():
-                # Cygwin creates submodules prefixed with `/cygdrive/...`.
-                # Cygwin git understands Cygwin paths much better than Windows ones.
-                # Also the Cygwin tests are assuming Cygwin paths.
-                path = cygpath(path)
-            if not osp.isabs(path):
-                path = osp.normpath(osp.join(osp.dirname(d), path))
-            return find_submodule_git_dir(path)
-    # END handle exception
-    return None
+    path = find_worktree_git_dir(d)
+    if path is None:
+        return None
+
+    if Git.is_cygwin():
+        # Cygwin creates submodules prefixed with `/cygdrive/...`.
+        # Cygwin git understands Cygwin paths much better than Windows ones.
+        # Also the Cygwin tests are assuming Cygwin paths.
+        path = cygpath(path)
+    if not osp.isabs(path):
+        path = osp.normpath(osp.join(osp.dirname(d), path))
+    return path if is_git_dir(path) else None
 
 
 def short_to_long(odb: "GitCmdObjectDB", hexsha: str) -> Optional[bytes]:

test/test_repo.py

@@ -176,11 +176,12 @@ def test_repo_discovery_rejects_invalid_metadata(self):
                 with mock.patch.dict(os.environ, {variable: ""}), self.subTest(metadata=variable):
                     self.assertRaises(InvalidGitRepositoryError, Repo, path)
 
-            (path / ".git").write_text("not a gitfile")
-            with self.subTest(metadata=".git"):
-                self.assertRaises(InvalidGitRepositoryError, Repo, path)
+            for contents in (b"not a gitfile", b"gitdir: \n", b"gitdir: .git\n", b"\xff"):
+                (path / ".git").write_bytes(contents)
+                with self.subTest(metadata=".git", contents=contents):
+                    self.assertRaises(InvalidGitRepositoryError, Repo, path)
 
-    def test_repo_discovery_uses_git_common_dir(self):
+    def test_repo_discovery_uses_storage_environment(self):
         with tempfile.TemporaryDirectory() as tdir:
             git_dir = Path(tdir) / "git"
             common_dir = Path(tdir) / "common"
@@ -201,6 +202,24 @@ def test_repo_discovery_uses_git_common_dir(self):
             assert osp.samefile(repo.git.rev_parse("--absolute-git-dir"), git_dir)
             assert osp.samefile(repo.git.rev_parse("--git-common-dir"), common_dir)
 
+            (git_dir / "commondir").write_text("../common\n")
+            environment = dict(os.environ)
+            environment["GIT_DIR"] = "git"
+            environment.pop("GIT_COMMON_DIR", None)
+            with cwd(tdir), mock.patch.dict(os.environ, environment, clear=True):
+                repo = Repo()
+
+            assert osp.samefile(repo.git.rev_parse("--absolute-git-dir"), git_dir)
+
+            if sys.platform.startswith("linux"):
+                byte_common_dir = Path(tdir) / os.fsdecode(b"common-\xff")
+                byte_common_dir.mkdir()
+                (byte_common_dir / "objects").mkdir()
+                (byte_common_dir / "refs").mkdir()
+                (git_dir / "commondir").write_bytes(b"../common-\xff\n")
+
+                assert osp.samefile(Repo(git_dir).common_dir, byte_common_dir)
+
     @with_rw_repo("0.3.2.1")
     def test_repo_creation_from_different_paths(self, rw_repo):
         r_from_gitdir = Repo(rw_repo.git_dir)