Run more submodule tests on Cygwin (fix flaky xfails)

.github/workflows/alpine-test.yml

@@ -61,6 +61,29 @@ jobs:
         . .venv/bin/activate
         pip install '.[test]'
 
+    - name: Show POSIX file ownership
+      run: |
+        for p in \
+          "$(pwd)" \
+          "$(pwd)/.git" \
+          "$(pwd)/git/ext/gitdb" \
+          "$(pwd)/git/ext/gitdb/.git" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
+          "${HOME:?HOME is not set}/.gitconfig"
+        do
+          ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
+        done
+
+    - name: Show safe.directory entries
+      # `actions/checkout`'s safe.directory add is only durable for the
+      # checkout itself (it writes under a throwaway HOME override and
+      # then discards it), so by the time this step runs the runner
+      # user's `~/.gitconfig` has no entries -- and the Alpine container
+      # chowns the workspace to runner:docker to match the test user, so
+      # git accepts the ownership without one. Expected: `(none)`.
+      run: git config --global --get-all safe.directory || echo "(none)"
+
     - name: Show version and platform information
       run: |
         . .venv/bin/activate

.github/workflows/cygwin-test.yml

@@ -53,6 +53,8 @@ jobs:
       run: |
         git config --global --add safe.directory "$(pwd)"
         git config --global --add safe.directory "$(pwd)/.git"
+        git config --global --add safe.directory "$(pwd)/git/ext/gitdb"
+        git config --global --add safe.directory "$(pwd)/git/ext/gitdb/gitdb/ext/smmap"
         git config --global core.autocrlf false
 
     - name: Prepare this repo for tests
@@ -80,6 +82,60 @@ jobs:
       run: |
         pip install '.[test]'
 
+    - name: Show POSIX file ownership
+      # Cygwin's `ls -ld` reports the NTFS Owner SID via Cygwin's SID-to-uid
+      # mapping (well-known SIDs by their RID, machine-local accounts by
+      # 0x30000+RID). That mapping is what Cygwin git's
+      # `is_path_owned_by_current_user` reduces to, so this is the view that
+      # determines whether `safe.directory` is consulted.
+      run: |
+        for p in \
+          "$(pwd)" \
+          "$(pwd)/.git" \
+          "$(pwd)/git/ext/gitdb" \
+          "$(pwd)/git/ext/gitdb/.git" \
+          "$(pwd)/.git/modules/gitdb" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
+          "$(pwd)/.git/modules/gitdb/modules/smmap" \
+          "${HOME:?HOME is not set}/.gitconfig"
+        do
+          ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
+        done
+
+    - name: Show NTFS file ownership
+      # Authoritative NTFS Owner via Get-Acl, with no Cygwin SID-to-uid layer
+      # in between -- useful for confirming what the Cygwin view reports as
+      # "Administrators" is the BUILTIN\Administrators SID (S-1-5-32-544).
+      shell: pwsh
+      run: |
+        $paths = @(
+          "$pwd",
+          "$pwd\.git",
+          "$pwd\git\ext\gitdb",
+          "$pwd\git\ext\gitdb\.git",
+          "$pwd\.git\modules\gitdb",
+          "$pwd\git\ext\gitdb\gitdb\ext\smmap",
+          "$pwd\git\ext\gitdb\gitdb\ext\smmap\.git",
+          "$pwd\.git\modules\gitdb\modules\smmap",
+          "$env:USERPROFILE\.gitconfig"
+        )
+        foreach ($p in $paths) {
+          if (Test-Path -LiteralPath $p) {
+            try {
+              $owner = (Get-Acl -LiteralPath $p).Owner
+            } catch {
+              $owner = "ERROR: $($_.Exception.Message)"
+            }
+            "{0,-44}  {1}" -f $owner, $p
+          } else {
+            "(missing: $p)"
+          }
+        }
+
+    - name: Show safe.directory entries
+      run: git config --global --get-all safe.directory
+
     - name: Show version and platform information
       run: |
         uname -a

.github/workflows/pythonpackage.yml

@@ -87,6 +87,64 @@ jobs:
       run: |
         pip install '.[test]'
 
+    - name: Show POSIX file ownership
+      # Linux and macOS only. On Windows, Git Bash's `ls -ld` reports a
+      # uniform uid+gid for every path regardless of NTFS Owner (MSYS2's
+      # SID-to-uid mapping doesn't have Cygwin's fidelity), so it would
+      # not be informative here. The NTFS Owner check below covers Windows.
+      if: matrix.os-type != 'windows'
+      run: |
+        for p in \
+          "$(pwd)" \
+          "$(pwd)/.git" \
+          "$(pwd)/git/ext/gitdb" \
+          "$(pwd)/git/ext/gitdb/.git" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
+          "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
+          "${HOME:?HOME is not set}/.gitconfig"
+        do
+          ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
+        done
+
+    - name: Show NTFS file ownership
+      # Windows only. Reads NTFS Owner directly via Get-Acl, which is the
+      # authoritative view for Windows-side ownership questions; the POSIX
+      # view via Git Bash's MSYS2 layer is not a reliable proxy here.
+      if: matrix.os-type == 'windows'
+      shell: pwsh
+      run: |
+        $paths = @(
+          "$pwd",
+          "$pwd\.git",
+          "$pwd\git\ext\gitdb",
+          "$pwd\git\ext\gitdb\.git",
+          "$pwd\git\ext\gitdb\gitdb\ext\smmap",
+          "$pwd\git\ext\gitdb\gitdb\ext\smmap\.git",
+          "$env:USERPROFILE\.gitconfig"
+        )
+        foreach ($p in $paths) {
+          if (Test-Path -LiteralPath $p) {
+            try {
+              $owner = (Get-Acl -LiteralPath $p).Owner
+            } catch {
+              $owner = "ERROR: $($_.Exception.Message)"
+            }
+            "{0,-44}  {1}" -f $owner, $p
+          } else {
+            "(missing: $p)"
+          }
+        }
+
+    - name: Show safe.directory entries
+      # `actions/checkout`'s safe.directory add is only durable for the
+      # checkout itself (it writes under a throwaway HOME override and
+      # then discards it), so by the time this step runs the runner
+      # user's `~/.gitconfig` has no entries -- and git accepts the
+      # workspace's ownership anyway: Git for Windows via its
+      # Admins-group exemption on the windows matrix; on Linux/macOS
+      # the workspace is owned by the test user. Expected: `(none)`.
+      run: git config --global --get-all safe.directory || echo "(none)"
+
     - name: Show version and platform information
       run: |
         uname -a

test/test_docs.py

@@ -6,9 +6,6 @@
 import gc
 import os
 import os.path
-import sys
-
-import pytest
 
 from test.lib import TestBase
 from test.lib.helper import with_rw_directory
@@ -478,11 +475,6 @@ def test_references_and_objects(self, rw_dir):
 
         repo.git.clear_cache()
 
-    @pytest.mark.xfail(
-        sys.platform == "cygwin",
-        reason="Cygwin GitPython can't find SHA for submodule",
-        raises=ValueError,
-    )
     def test_submodules(self):
         # [1-test_submodules]
         repo = self.rorepo

test/test_fixture_health.py

@@ -0,0 +1,131 @@
+# This module is part of GitPython and is released under the
+# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
+
+"""Verify that fixture directories are usable by git.
+
+If a fixture directory is missing, isn't an initialized git repository,
+or is rejected by git for "dubious ownership", dependent tests
+elsewhere in the suite fail in opaque ways. The checks here name the
+preconditions directly so a misconfigured environment is recognizable
+from the test output rather than from a cascade of unrelated-seeming
+failures.
+
+These tests do not exercise GitPython's production code. They verify
+the conditions under which production code is exercised are valid.
+"""
+
+import subprocess
+from pathlib import Path
+
+import pytest
+
+REPO_ROOT = Path(__file__).resolve().parent.parent
+
+# Directories git must trust for the test suite to operate normally. The
+# current set is the GitPython working tree plus the working trees of its
+# gitdb submodule and the smmap submodule nested inside gitdb. New entries
+# should be added here whenever the test suite gains a dependency on git
+# accepting another directory.
+FIXTURE_DIRS = [
+    pytest.param(REPO_ROOT, id="repo_root"),
+    pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"),
+    pytest.param(
+        REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap",
+        id="smmap",
+    ),
+]
+
+# Submodule working trees that must be present and initialized for the
+# test suite to operate normally: gitdb at `git/ext/gitdb`, and smmap
+# nested inside gitdb at `git/ext/gitdb/gitdb/ext/smmap`. The paths
+# below are anchored at REPO_ROOT (the GitPython source tree), not at
+# any rorepo redirection target.
+SUBMODULE_DIRS = [
+    pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"),
+    pytest.param(
+        REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap",
+        id="smmap",
+    ),
+]
+
+
+@pytest.mark.parametrize("fixture_dir", FIXTURE_DIRS)
+def test_fixture_dir_is_trusted_by_git(fixture_dir: Path) -> None:
+    """git accepts ``fixture_dir`` as its own repository owned by a trusted user.
+
+    Run ``git -C <fixture_dir> rev-parse --show-toplevel`` and assert it
+    succeeds and reports ``fixture_dir`` itself as the toplevel. Failure
+    typically means the directory's on-disk ownership doesn't match the
+    running user and the CI workflow's ``safe.directory`` list is missing
+    an entry that would override the check.
+    """
+    if not fixture_dir.exists():
+        pytest.skip(f"{fixture_dir} not present (run `git submodule update --init --recursive` from the repo root)")
+    if not (fixture_dir / ".git").exists():
+        pytest.skip(
+            f"{fixture_dir} has no .git marker "
+            "(submodule not initialized; run "
+            "`git submodule update --init --recursive` from the repo root)"
+        )
+    try:
+        result = subprocess.run(
+            ["git", "-C", str(fixture_dir), "rev-parse", "--show-toplevel"],
+            capture_output=True,
+            text=True,
+            check=False,
+        )
+    except FileNotFoundError:
+        pytest.skip("git is not installed or not on PATH")
+    assert result.returncode == 0, (
+        f"git refuses to operate in {fixture_dir}.\n"
+        f"stderr: {result.stderr.strip()}\n"
+        "The directory's owner doesn't match the running user and no "
+        "`safe.directory` entry overrides the check. On CI, the "
+        "workflow's `safe.directory` list typically needs an entry for "
+        "this path. Locally, this is unexpected and usually indicates "
+        "an ownership problem worth investigating."
+    )
+    reported = Path(result.stdout.strip())
+    assert reported.samefile(fixture_dir), (
+        f"git reports the toplevel as {reported}, "
+        f"not as {fixture_dir} itself. "
+        "This usually means the directory is not an initialized git "
+        "repository (its `.git` marker may be stale or pointing elsewhere)."
+    )
+
+
+@pytest.mark.parametrize("submodule_dir", SUBMODULE_DIRS)
+def test_required_submodule_is_initialized(submodule_dir: Path) -> None:
+    """The submodule's working tree is present and initialized.
+
+    Failure means the source tree is a git clone but the submodule's
+    working tree hasn't been populated. Skipped when the source tree
+    itself isn't a git clone (e.g. an extracted release tarball), since
+    ``git submodule update`` cannot operate there; setups that handle
+    submodules in a separately-prepared tree (via
+    ``GIT_PYTHON_TEST_GIT_REPO_BASE``) are exempted from this check.
+    """
+    if not (REPO_ROOT / ".git").exists():
+        pytest.skip(
+            "Source tree is not a git clone (no .git in REPO_ROOT); submodules "
+            "cannot be initialized via `git submodule update` here. Setups "
+            "that prepare submodules in a separately-pointed tree (via "
+            "GIT_PYTHON_TEST_GIT_REPO_BASE) are exempted from this check."
+        )
+    # The assertion messages below recommend `git submodule update --init
+    # --recursive` rather than `init-tests-after-clone.sh`, even though the
+    # latter is the documented entry point for first-time test setup. Two
+    # reasons: the script performs `git reset --hard` operations that can
+    # destroy local work, and #1713 showed the script itself can carry
+    # submodule-init regressions, in which case recommending it would lead
+    # developers in a circle. The direct git command is a safe minimal fix
+    # for this test's specific failure mode and bypasses any such regression.
+    assert submodule_dir.is_dir(), (
+        f"Submodule working tree missing: {submodule_dir}.\n"
+        "Run `git submodule update --init --recursive` from the repo root."
+    )
+    assert (submodule_dir / ".git").exists(), (
+        f"Submodule directory exists but has no .git marker: {submodule_dir}.\n"
+        "The submodule hasn't been initialized. "
+        "Run `git submodule update --init --recursive` from the repo root."
+    )

test/test_repo.py

@@ -877,11 +877,6 @@ def test_repo_odbtype(self):
         target_type = GitCmdObjectDB
         self.assertIsInstance(self.rorepo.odb, target_type)
 
-    @pytest.mark.xfail(
-        sys.platform == "cygwin",
-        reason="Cygwin GitPython can't find submodule SHA",
-        raises=ValueError,
-    )
     def test_submodules(self):
         self.assertEqual(len(self.rorepo.submodules), 1)  # non-recursive
         self.assertGreaterEqual(len(list(self.rorepo.iter_submodules())), 2)

test/test_submodule.py

@@ -480,11 +480,6 @@ def test_base_rw(self, rwrepo):
     def test_base_bare(self, rwrepo):
         self._do_base_tests(rwrepo)
 
-    @pytest.mark.xfail(
-        sys.platform == "cygwin",
-        reason="Cygwin GitPython can't find submodule SHA",
-        raises=ValueError,
-    )
     @pytest.mark.xfail(
         HIDE_WINDOWS_KNOWN_ERRORS,
         reason=(
@@ -513,9 +508,9 @@ def test_root_module(self, rwrepo):
         with rm.config_writer():
             pass
 
-        # Deep traversal gitdb / async.
+        # Deep traversal yields gitdb and its nested smmap.
         rsmsp = [sm.path for sm in rm.traverse()]
-        assert len(rsmsp) >= 2  # gitdb and async [and smmap], async being a child of gitdb.
+        assert rsmsp == ["git/ext/gitdb", "gitdb/ext/smmap"]
 
         # Cannot set the parent commit as root module's path didn't exist.
         self.assertRaises(ValueError, rm.set_parent_commit, "HEAD")