From db9fbbce86003a9028b21fb324d6ab15f064e5ee Mon Sep 17 00:00:00 2001 From: Julien Cristau Date: Thu, 15 Oct 2026 08:43:47 +0200 Subject: [PATCH] repo_synchronizer: don't ignore fetch errors (bug 2082293) fetch_all_from_remote's check for an empty remote was inverted: it raised only when the remote was empty, and silently ignored every other fetch failure. Ignore only the "couldn't find remote ref HEAD" error that git (and cinnabar) report for an empty repository, and let other errors propagate. The check was added in 6a80f4e ("fix: handle empty repo"), and inverted in 491720b (#26, "Automatically retry actions that touch the network"), presumably by mistake: retry() only retries on exceptions, so swallowing the errors also defeated the retries. Do this check inside the retried callable, so an empty remote isn't retried, and drop the now-redundant retry around fetch_all_from_remote in _ensure_cinnabar_metadata, which would otherwise nest with the inner one. In verbose mode, stderr is streamed to the log, so it wasn't included in the GitCommandError and the empty-remote check couldn't work: collect it and pass it to proc.wait(). --- git_hg_sync/repo_synchronizer.py | 38 ++++++++++++++++++-------------- tests/test_repo_synchronizer.py | 20 +++++++++++++++++ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/git_hg_sync/repo_synchronizer.py b/git_hg_sync/repo_synchronizer.py index d9bd087..7ea0470 100644 --- a/git_hg_sync/repo_synchronizer.py +++ b/git_hg_sync/repo_synchronizer.py @@ -62,20 +62,20 @@ def _commit_has_mercurial_metadata(self, repo: Repo, git_commit: str) -> bool: def fetch_all_from_remote( self, repo: Repo, remote: str, verbose: bool = False ) -> None: - try: - retry( - f"fetching changes and tags from {remote}", - lambda: self._log_git_execute( + def fetch() -> None: + try: + self._log_git_execute( repo, ["git", "-c", "cinnabar.graft=true", "fetch", "--tags", remote], verbose, - ), - ) + ) + except GitCommandError as exc: + # can't fetch if repo is empty + if "fatal: couldn't find remote ref HEAD" not in exc.stderr: + raise + logger.info(f"Remote {remote} is empty, nothing to fetch") - except GitCommandError as exc: - # can't fetch if repo is empty - if "fatal: couldn't find remote ref HEAD" in exc.stderr: - raise exc + retry(f"fetching changes and tags from {remote}", fetch) if remote.startswith("hg::"): retry( @@ -95,25 +95,32 @@ def _log_git_execute(repo: Repo, command: list[str], verbose: bool = False) -> N if verbose: logger.info(f"Running `{' '.join(command)}` as PID {proc.pid}") + stderr_lines: list[bytes] = [] stdout_thread = threading.Thread( target=RepoSynchronizer._stream_output, args=(proc.stdout, f"{proc.pid}/STDOUT"), ) stderr_thread = threading.Thread( target=RepoSynchronizer._stream_output, - args=(proc.stderr, f"{proc.pid}/STDERR"), + args=(proc.stderr, f"{proc.pid}/STDERR", stderr_lines), ) stdout_thread.start() stderr_thread.start() stdout_thread.join() stderr_thread.join() - proc.wait() + # The stream has been consumed, so pass it on to be included in any + # GitCommandError. + proc.wait(stderr=b"".join(stderr_lines)) @staticmethod - def _stream_output(stream: io.BufferedReader, label: str) -> None: + def _stream_output( + stream: io.BufferedReader, label: str, lines: list[bytes] | None = None + ) -> None: for line in iter(stream.readline, b""): logger.info(f"{label}: {line.decode().strip()}") + if lines is not None: + lines.append(line) def sync( self, destination_url: str, operations: list[SyncOperation], request_user: str @@ -315,10 +322,7 @@ def _ensure_cinnabar_metadata(self, repo: Repo, destination_remote: str) -> None logger.debug("Cinnabar metadata already present, not updating") return - retry( - f"fetching commits from {destination_remote}", - lambda: self.fetch_all_from_remote(repo, destination_remote), - ) + self.fetch_all_from_remote(repo, destination_remote) def _git2hg(self, repo: Repo, git_commit: str) -> str: return repo.git.cinnabar(["git2hg", git_commit]).strip() diff --git a/tests/test_repo_synchronizer.py b/tests/test_repo_synchronizer.py index 930d11d..89026e0 100644 --- a/tests/test_repo_synchronizer.py +++ b/tests/test_repo_synchronizer.py @@ -323,3 +323,23 @@ def test_get_connection_and_queue(pulse_config: PulseConfig) -> None: assert connection.userid == pulse_config.userid assert connection.host == f"{pulse_config.host}:{pulse_config.port}" assert queue.name == pulse_config.queue + + +@pytest.mark.parametrize("verbose", [False, True]) +def test_fetch_all_from_remote_empty(tmp_path: Path, verbose: bool) -> None: + empty_remote = Repo.init(tmp_path / "empty", bare=True) + clone = Repo.init(tmp_path / "clone", bare=True) + syncrepos = RepoSynchronizer(tmp_path / "clone", str(empty_remote.git_dir)) + + # Fetching from an empty repository is not an error. + syncrepos.fetch_all_from_remote(clone, str(empty_remote.git_dir), verbose) + + +@pytest.mark.parametrize("verbose", [False, True]) +def test_fetch_all_from_remote_error(tmp_path: Path, verbose: bool) -> None: + clone = Repo.init(tmp_path / "clone", bare=True) + missing_remote = str(tmp_path / "missing") + syncrepos = RepoSynchronizer(tmp_path / "clone", missing_remote) + + with pytest.raises(GitCommandError, match="does not appear to be a git repository"): + syncrepos.fetch_all_from_remote(clone, missing_remote, verbose)