project: report why refreshing the index failed

"repo status" aborts the whole tree when one project fails, without
saying why. The three callers that refresh the index stat cache pass -q,
which suppresses git's diagnosis when it cannot create the index lock;
git then exits 128 with both streams empty. A stale lock, a permissions
problem on the gitdir and a full disk all reach that same silent exit,
yet each needs a different remedy.

Route the three identical call sites through one helper that keeps -q
for the refresh itself. When that fails, it repeats the command without
-q to obtain git's diagnosis. The command still fails, which is correct,
but the user is now told why.

Keeping -q on the common path matters: without it git exits 1 whenever
paths hold uncommitted changes, which would report a failure to
telemetry for every modified project, even if we treat it as benign in
the code. The retry accepts that 1, since --unmerged makes git skip
conflicted paths rather than count them, so nothing else yields it.

The similar call in subcmds/sync.py is unchanged: it passes different
flags, without --unmerged, so exit 1 cannot be assumed benign there.

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

2 files changed, 95 insertions, 9 deletions

diff --git a/project.py b/project.py
index 2f72bb02d..a084d8db8 100644
--- a/project.py
+++ b/project.py
@@ -45,6 +45,7 @@ from error import UploadError
 import fetch
 from git_command import git_require
 from git_command import GitCommand
+from git_command import GitCommandError
 from git_config import GetSchemeFromUrl
 from git_config import GetUrlCookieFile
 from git_config import GitConfig
@@ -825,11 +826,32 @@ class Project:
         _git("rebase", "--abort")
         _git("am", "--abort")
 
+    def _RefreshIndexStatCache(self) -> None:
+        """Refresh the index's cached stat information."""
+        args = ["--unmerged", "--ignore-missing", "--refresh"]
+
+        # Run twice because -q is needed and unhelpful in equal measure. It
+        # keeps git quiet about uncommitted changes, which would otherwise
+        # exit 1 and report a failure to telemetry for every modified project,
+        # but it also suppresses the reason a refresh failed, leaving a bare
+        # exit 128. So refresh with it, and repeat without it only to get
+        # the reason.
+        try:
+            self.work_git.update_index("-q", *args, log_as_error=False)
+            return
+        except GitError:
+            pass
+
+        # Exit code 1 means there are modified files, which is okay.
+        try:
+            self.work_git.update_index(*args)
+        except GitCommandError as e:
+            if e.git_rc != 1:
+                raise
+
     def IsDirty(self, consider_untracked=True):
         """Is the working directory modified in some way?"""
-        self.work_git.update_index(
-            "-q", "--unmerged", "--ignore-missing", "--refresh"
-        )
+        self._RefreshIndexStatCache()
         if self.work_git.DiffZ("diff-index", "-M", "--cached", HEAD):
             return True
         if self.work_git.DiffZ("diff-files"):
@@ -954,9 +976,7 @@ class Project:
                 uncommitted files is detected.
         """
         details = []
-        self.work_git.update_index(
-            "-q", "--unmerged", "--ignore-missing", "--refresh"
-        )
+        self._RefreshIndexStatCache()
         if self.IsRebaseInProgress():
             details.append("rebase in progress")
             if not get_all:
@@ -1007,9 +1027,7 @@ class Project:
             print('  missing (run "repo sync")', file=output_redir)
             return
 
-        self.work_git.update_index(
-            "-q", "--unmerged", "--ignore-missing", "--refresh"
-        )
+        self._RefreshIndexStatCache()
         rb = self.IsRebaseInProgress()
         di = self.work_git.DiffZ("diff-index", "-M", "--cached", HEAD)
         df = self.work_git.DiffZ("diff-files")
diff --git a/tests/test_project.py b/tests/test_project.py
index af296c2fb..911bf7285 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -28,6 +28,7 @@ import pytest
 import utils_for_test
 
 import error
+import git_command
 import git_config
 import git_trace2_event_log
 import manifest_xml
@@ -1794,6 +1795,73 @@ def test_metaproject_has_changes_bounds_revision_walk() -> None:
     meta._revlist.assert_called_once_with("-1", "^HEAD", "remote")
 
 
+@pytest.mark.parametrize(
+    "state,expect_failure",
+    [
+        ("clean", False),
+        # -q makes git exit 0 here rather than 1, so the repeat is never
+        # reached. That is the point of it: an ordinary modified tree must
+        # not be reported as a failure.
+        ("modified", False),
+        ("locked", True),
+    ],
+)
+def test_refresh_index_stat_cache(state: str, expect_failure: bool) -> None:
+    """Refreshing tolerates local changes but reports a real failure."""
+    with utils_for_test.TempGitTree() as tempdir:
+        proj = _create_mock_project(tempdir)
+        readme = Path(tempdir) / "readme"
+        readme.write_text("hello")
+        proj.work_git.add("readme")
+        proj.work_git.commit("-m", "initial commit")
+
+        if state == "modified":
+            readme.write_text("different contents")
+        elif state == "locked":
+            # git only needs the lock when the index has to be rewritten, so
+            # age the cached stat data while leaving the contents alone.
+            os.utime(readme, (1, 1))
+            (Path(proj.gitdir) / "index.lock").write_text("")
+
+        if not expect_failure:
+            proj._RefreshIndexStatCache()
+            return
+
+        with pytest.raises(error.GitError) as excinfo:
+            proj._RefreshIndexStatCache()
+        # Omitting -q is what lets git name the file it could not create.
+        assert "index.lock" in str(excinfo.value)
+
+
+@pytest.mark.parametrize(
+    "repeat_rc,expect_raise",
+    [
+        # Whatever blocked the quiet attempt cleared in between, leaving the
+        # repeat to report nothing worse than uncommitted changes.
+        (1, False),
+        (128, True),
+    ],
+)
+def test_refresh_index_stat_cache_repeat(
+    repeat_rc: int, expect_raise: bool
+) -> None:
+    """The repeat re-raises unless it failed with an ordinary exit 1."""
+    proj = mock.MagicMock()
+    proj.work_git.update_index.side_effect = [
+        git_command.GitCommandError("quiet attempt failed", git_rc=128),
+        git_command.GitCommandError("repeat failed", git_rc=repeat_rc),
+    ]
+
+    if expect_raise:
+        with pytest.raises(git_command.GitCommandError):
+            project.Project._RefreshIndexStatCache(proj)
+    else:
+        project.Project._RefreshIndexStatCache(proj)
+
+    # The quiet attempt ran, then the repeat that explains it.
+    assert proj.work_git.update_index.call_count == 2
+
+
 def _create_mock_project(
     tempdir,
     use_local_gitdirs=False,