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>
diff --git a/project.py b/project.py index 2f72bb0..a084d8d 100644 --- a/project.py +++ b/project.py
@@ -45,6 +45,7 @@ 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 @@ _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 @@ 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 @@ 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 af296c2..911bf72 100644 --- a/tests/test_project.py +++ b/tests/test_project.py
@@ -28,6 +28,7 @@ import utils_for_test import error +import git_command import git_config import git_trace2_event_log import manifest_xml @@ -1794,6 +1795,73 @@ 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,