project: don't plan a stateless prune without a worktree

Skip stateless prune planning when a project's worktree directory
does not exist, preventing fetch failures on subsequent syncs.

Also run _LsRemote() bare so it does not depend on worktree
existence.

Bug: 565047698
Change-Id: I385cba4e06813d9113ec7ddc963e8499acffaec5
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/634426
Tested-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Brian Gan <brgan@google.com>

project.py

@@ -886,9 +886,16 @@ class Project:
         ahead_behind: bool = False,
         show_stash: bool = False,
     ) -> Optional[git_status.StatusSnapshot]:
-        """Read one porcelain-v2 snapshot, or select the legacy path."""
+        """Read one porcelain-v2 snapshot, or select the legacy path.
+
+        Returns None on Git older than 2.11, when git status fails, or when
+        the worktree directory is missing. A missing worktree isn't logged:
+        status can't run there, and the legacy path raises its own error.
+        """
         if not git_require((2, 11, 0)):
             return None
+        if not self.worktree or not platform_utils.isdir(self.worktree):
+            return None
         try:
             return git_status.GetStatus(
                 self,
@@ -943,7 +950,26 @@ class Project:
             if has_status_stash:
                 return bool(status.stash_count)
             return self.HasStash()
-        return self.IsDirty(consider_untracked=True) or self.HasStash()
+        # The snapshot already failed; don't run git status again.
+        return self._IsDirtyLegacy(consider_untracked=True) or self.HasStash()
+
+    def GetDirtyAndHead(self) -> Tuple[bool, Optional[str]]:
+        """Return whether the worktree is dirty, and the commit at HEAD.
+
+        Untracked files count as dirty. HEAD is None when it can't be
+        resolved, e.g. on an unborn branch. Both come from one status
+        snapshot when possible.
+        """
+        status = self._GetStatusSnapshot(untracked_files="normal", branch=True)
+        if status is not None:
+            return status.is_dirty(consider_untracked=True), status.branch_oid
+        # The snapshot already failed; don't run git status again.
+        is_dirty = self._IsDirtyLegacy(consider_untracked=True)
+        try:
+            head = self.work_git.rev_parse(HEAD)
+        except GitError:
+            head = None
+        return is_dirty, head
 
     _userident_name = None
     _userident_email = None
@@ -1641,6 +1667,12 @@ class Project:
         if not self.Exists:
             return False
 
+        # Local changes can't be checked without a worktree, and pruning drops
+        # every reflog. Sync_LocalHalf() recreates the worktree, so a later
+        # sync can decide.
+        if not self.worktree or not platform_utils.isdir(self.worktree):
+            return False
+
         if self._CheckForImmutableRevision(use_superproject=use_superproject):
             return False
 
@@ -3888,9 +3920,9 @@ class Project:
                     f"{self.name} cherry-pick {rev} ", project=self.name
                 )
 
-    def _LsRemote(self, refs):
+    def _LsRemote(self, refs: str) -> Optional[str]:
         cmd = ["ls-remote", self.remote.name, refs]
-        p = GitCommand(self, cmd, capture_stdout=True)
+        p = GitCommand(self, cmd, bare=True, capture_stdout=True)
         if p.Wait() == 0:
             return p.stdout
         return None

repo_trace.py

@@ -57,10 +57,10 @@ def SetTraceToStderr():
     _TRACE_TO_STDERR = True
 
 
-def SetTrace():
+def SetTrace(value: bool = True) -> None:
-    """Enables tracing."""
+    """Enables by default, or disables tracing."""
     global _TRACE
-    _TRACE = True
+    _TRACE = value
 
 
 def _SetTraceFile(quiet):

subcmds/sync.py

@@ -1608,26 +1608,12 @@ later is required to fix a server side protocol bug.
         """
         project = cls.get_parallel_context()["projects"][project_index]
 
-        if not project.Exists or not project.worktree:
-            return None
-
         # Only check dirty or locally modified projects. These can't be
         # freshly cloned and will accumulate garbage.
         try:
-            status = project._GetStatusSnapshot(
+            is_dirty, head_rev = project.GetDirtyAndHead()
-                untracked_files="normal", branch=True
-            )
-            if status is not None:
-                is_dirty = status.is_dirty(consider_untracked=True)
-                head_rev = status.branch_oid
-            else:
-                is_dirty = project.IsDirty(consider_untracked=True)
-                head_rev = project.work_git.rev_parse(HEAD)
-
             if head_rev is None:
-                # Porcelain v2 reports an unborn branch as "(initial)".  The
+                # An unborn branch has no HEAD to compare, so skip it.
-                # legacy rev-parse path failed here and skipped the bloat
-                # calculation, so preserve that behavior.
                 return None
 
             manifest_rev = project.GetRevisionId(project.bare_ref.all)
@@ -1671,16 +1657,27 @@ later is required to fix a server side protocol bug.
         run 'git count-objects -v' and warn if the repository is accumulating
         excessive pack files or garbage.
         """
+        # --network-only promises not to touch worktrees, but git status and
+        # update-index --refresh can both rewrite the index.
+        if opt.network_only:
+            return
+
         # We only care about bloated projects if we have a git version that
         # supports --no-auto-gc (2.23.0+) since what we use to disable auto-gc
         # in Project._RemoteFetch.
         if not git_require((2, 23, 0)):
             return
 
+        # Skip projects with no worktree on disk, e.g. ones only ever synced
+        # with --network-only.
         projects = [
             p
             for p in projects
-            if p.clone_depth and not p.stateless_prune_needed
+            if p.clone_depth
+            and not p.stateless_prune_needed
+            and p.worktree
+            and p.Exists
+            and platform_utils.isdir(p.worktree)
         ]
         if not projects:
             return

tests/conftest.py

@@ -26,6 +26,7 @@ import repo_trace
 @pytest.fixture(autouse=True)
 def disable_repo_trace(tmp_path):
     """Set an environment marker to relax certain strict checks for test code."""  # noqa: E501
+    repo_trace.SetTrace(False)
     repo_trace._TRACE_FILE = str(tmp_path / "TRACE_FILE_from_test")
 
 

tests/test_project.py

@@ -713,6 +713,18 @@ class ProjectTests(unittest.TestCase):
             ):
                 self.assertIsNone(proj._GetStatusSnapshot())
 
+    def test_get_status_snapshot_missing_worktree_is_quiet(self) -> None:
+        """A missing worktree skips git status without a warning."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = _create_mock_project(tempdir)
+            proj.worktree = os.path.join(tempdir, "missing")
+            with mock.patch.object(git_status, "GetStatus") as mock_get_status:
+                with mock.patch.object(project, "logger") as mock_logger:
+                    self.assertIsNone(proj._GetStatusSnapshot())
+
+            mock_get_status.assert_not_called()
+            mock_logger.warning.assert_not_called()
+
     def test_dirty_or_stash_uses_status_stash_header(self) -> None:
         """A normal stash is detected without a second Git process."""
         with utils_for_test.TempGitTree() as tempdir:
@@ -756,6 +768,64 @@ class ProjectTests(unittest.TestCase):
             )
             proj.HasStash.assert_called_once_with()
 
+    def test_dirty_and_head_agree_across_status_paths(self) -> None:
+        """Porcelain v2 and legacy plumbing report the same state."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = _create_mock_project(tempdir)
+
+            def check(expected: Tuple[bool, Optional[str]]) -> None:
+                for use_status in (True, False):
+                    with self.subTest(expected=expected, status=use_status):
+                        with mock.patch.object(
+                            project, "git_require", return_value=use_status
+                        ):
+                            self.assertEqual(expected, proj.GetDirtyAndHead())
+
+            check((False, None))
+            Path(tempdir, "untracked").write_text("new")
+            check((True, None))
+
+            Path(tempdir, "tracked").write_text("initial")
+            proj.work_git.add("tracked")
+            proj.work_git.commit("-m", "initial")
+            head = proj.work_git.rev_parse("HEAD")
+            check((True, head))
+            os.remove(os.path.join(tempdir, "untracked"))
+            check((False, head))
+
+    def test_dirty_and_head_fallback_skips_second_snapshot(self) -> None:
+        """A failed snapshot goes straight to the legacy plumbing."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = _create_mock_project(tempdir)
+            proj._GetStatusSnapshot = mock.MagicMock(return_value=None)
+            proj._IsDirtyLegacy = mock.MagicMock(return_value=False)
+            proj.work_git = mock.MagicMock()
+            proj.work_git.rev_parse.return_value = "head"
+
+            self.assertEqual((False, "head"), proj.GetDirtyAndHead())
+
+            proj._GetStatusSnapshot.assert_called_once_with(
+                untracked_files="normal", branch=True
+            )
+            proj._IsDirtyLegacy.assert_called_once_with(consider_untracked=True)
+            proj.work_git.rev_parse.assert_called_once_with("HEAD")
+
+    def test_dirty_or_stash_fallback_skips_second_snapshot(self) -> None:
+        """A failed snapshot goes straight to the legacy dirty check."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = _create_mock_project(tempdir)
+            proj._GetStatusSnapshot = mock.MagicMock(return_value=None)
+            proj._IsDirtyLegacy = mock.MagicMock(return_value=False)
+            proj.HasStash = mock.MagicMock(return_value=False)
+
+            with mock.patch.object(project, "git_require", return_value=True):
+                self.assertFalse(proj._HasDirtyOrStash())
+
+            proj._GetStatusSnapshot.assert_called_once_with(
+                untracked_files="normal", show_stash=True
+            )
+            proj._IsDirtyLegacy.assert_called_once_with(consider_untracked=True)
+
     def test_old_git_dirty_check_uses_legacy_plumbing(self) -> None:
         """Git clients before 2.11 retain the existing dirty-check path."""
         with utils_for_test.TempGitTree() as tempdir:
@@ -2350,6 +2420,33 @@ class StatelessSyncTests(unittest.TestCase):
             self.assertTrue(res.success)
             self.assertFalse(getattr(proj, "stateless_prune_needed", False))
 
+    def test_sync_network_half_stateless_skips_without_worktree(self) -> None:
+        """Test stateless sync doesn't prune a project with no worktree."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = self._get_project(tempdir)
+            proj.worktree = os.path.join(tempdir, "missing")
+            proj._HasDirtyOrStash = mock.MagicMock(return_value=False)
+
+            res = proj.Sync_NetworkHalf()
+
+            self.assertTrue(res.success)
+            self.assertFalse(proj.stateless_prune_needed)
+            proj._LsRemote.assert_not_called()
+            proj._HasDirtyOrStash.assert_not_called()
+
+    def test_ls_remote_runs_without_worktree(self) -> None:
+        """Test ls-remote only needs the gitdir's remote config."""
+        with utils_for_test.TempGitTree() as tempdir:
+            proj = _create_mock_project(tempdir)
+            proj.work_git.commit("--allow-empty", "-m", "initial")
+            proj.work_git.config("remote.origin.url", tempdir)
+            head = proj.work_git.rev_parse("HEAD")
+            proj.worktree = os.path.join(tempdir, "missing")
+            # _create_mock_project() stubs this out.
+            del proj._LsRemote
+
+            self.assertEqual(f"{head}\tHEAD\n", proj._LsRemote("HEAD"))
+
     def test_sync_network_half_stateless_skips_if_local_commits(self):
         """Test stateless sync skips if there are local-only commits."""
         with utils_for_test.TempGitTree() as tempdir:

tests/test_repo_trace.py

@@ -25,6 +25,9 @@ def test_trace_max_size_enforced(monkeypatch: pytest.MonkeyPatch) -> None:
     """Check Trace behavior."""
     content = "git chicken"
 
+    # Enable trace for the test, in case users have it disabled.
+    monkeypatch.setattr(repo_trace, "_TRACE", True)
+
     with repo_trace.Trace(content, first_trace=True):
         pass
     first_trace_size = os.path.getsize(repo_trace._TRACE_FILE)

tests/test_subcmds_sync.py

@@ -30,7 +30,6 @@ import pytest
 import command
 from error import GitError
 from error import RepoExitError
-import git_status
 import manifest_xml
 from project import SyncNetworkHalfResult
 from subcmds import sync
@@ -982,19 +981,20 @@ class CheckForBloatedProjects(unittest.TestCase):
         self.opt = mock.Mock()
         self.opt.quiet = True
         self.opt.jobs = 1
+        self.opt.network_only = False
+        self.tempdirobj = tempfile.TemporaryDirectory(prefix="repo_tests")
+        self.addCleanup(self.tempdirobj.cleanup)
         self.project = mock.MagicMock(clone_depth="1")
         self.project.name = "project"
         self.project.Exists = True
-        self.project.worktree = "worktree"
+        self.project.worktree = self.tempdirobj.name
         self.project.stateless_prune_needed = False
         self.cmd.git_event_log = mock.MagicMock()
         self.cmd._bloated_projects = []
 
-    def test_one_project_reuses_status_head_oid(self) -> None:
+    def test_one_project_uses_dirty_and_head(self) -> None:
-        """The bloat scan gets dirty state and HEAD from one snapshot."""
+        """A project whose HEAD left the manifest revision is measured."""
-        status = git_status.StatusSnapshot()
+        self.project.GetDirtyAndHead.return_value = (False, "local")
-        status.branch_oid = "local"
-        self.project._GetStatusSnapshot.return_value = status
         self.project.GetRevisionId.return_value = "manifest"
         self.project.bare_git.count_objects.return_value = (
             "packs: 0\nsize-pack: 0\nsize-garbage: 0\n"
@@ -1006,15 +1006,28 @@ class CheckForBloatedProjects(unittest.TestCase):
         ):
             self.assertIsNone(self.cmd._CheckOneBloatedProject(0))
 
-        self.project.IsDirty.assert_not_called()
+        self.project.GetDirtyAndHead.assert_called_once_with()
-        self.project.work_git.rev_parse.assert_not_called()
+        self.project.bare_git.count_objects.assert_called_once_with("-v")
+
+    def test_one_dirty_project_is_measured(self) -> None:
+        """A dirty project is measured even if HEAD matches the manifest."""
+        self.project.GetDirtyAndHead.return_value = (True, "local")
+        self.project.GetRevisionId.return_value = "local"
+        self.project.bare_git.count_objects.return_value = (
+            "packs: 0\nsize-pack: 0\nsize-garbage: 0\n"
+        )
+        with mock.patch.object(
+            sync.Sync,
+            "get_parallel_context",
+            return_value={"projects": [self.project]},
+        ):
+            self.assertIsNone(self.cmd._CheckOneBloatedProject(0))
+
         self.project.bare_git.count_objects.assert_called_once_with("-v")
 
     def test_one_unborn_project_skips_bloat_check(self) -> None:
-        """A porcelain initial branch behaves like failed rev-parse HEAD."""
+        """A project without a resolvable HEAD is skipped."""
-        status = git_status.StatusSnapshot()
+        self.project.GetDirtyAndHead.return_value = (True, None)
-        status.index_changes["staged"] = git_status.StatusEntry("staged", "M")
-        self.project._GetStatusSnapshot.return_value = status
 
         with mock.patch.object(
             sync.Sync,
@@ -1041,6 +1054,42 @@ class CheckForBloatedProjects(unittest.TestCase):
         self.cmd._CheckForBloatedProjects([self.project], self.opt)
         self.assertFalse(self.cmd.git_event_log.ErrorEvent.called)
 
+    @mock.patch("subcmds.sync.git_require", return_value=True)
+    @mock.patch("subcmds.sync.Progress")
+    def test_network_only_skips_check(
+        self, mock_progress: mock.Mock, mock_git_require: mock.Mock
+    ) -> None:
+        """--network-only doesn't read any worktree state."""
+        self.opt.network_only = True
+        self.cmd.ExecuteInParallel = mock.Mock()
+
+        self.cmd._CheckForBloatedProjects([self.project], self.opt)
+
+        mock_progress.assert_not_called()
+        self.cmd.ExecuteInParallel.assert_not_called()
+
+    @mock.patch("subcmds.sync.git_require", return_value=True)
+    @mock.patch("subcmds.sync.Progress")
+    def test_projects_without_worktree_excluded(
+        self, mock_progress: mock.Mock, mock_git_require: mock.Mock
+    ) -> None:
+        """Projects without a checked-out worktree are never scanned."""
+        self.cmd.ExecuteInParallel = mock.Mock()
+        missing = os.path.join(self.tempdirobj.name, "missing")
+        for attr, value in (
+            ("worktree", missing),
+            ("worktree", None),
+            ("Exists", False),
+        ):
+            with self.subTest(attr=attr, value=value):
+                mock_progress.reset_mock()
+                self.cmd.ExecuteInParallel.reset_mock()
+                with mock.patch.object(self.project, attr, value):
+                    self.cmd._CheckForBloatedProjects([self.project], self.opt)
+
+                mock_progress.assert_not_called()
+                self.cmd.ExecuteInParallel.assert_not_called()
+
     @mock.patch("subcmds.sync.git_require")
     @mock.patch("subcmds.sync.Progress")
     def test_bloated_project_found(self, mock_progress, mock_git_require):