diff --git a/project.py b/project.py index 6815e4f29..d4c2307cd 100644 --- a/project.py +++ b/project.py @@ -559,6 +559,9 @@ class RemoteSpec: class Project: + # How long to wait before the first retry of a failed fetch. + RETRY_SLEEP_INITIAL_SEC = 4.0 + # These objects can be shared between several working trees. @property def shareable_dirs(self): @@ -3353,7 +3356,6 @@ class Project: force_sync=False, clone_filter=None, retry_fetches=2, - retry_sleep_initial_sec=4.0, retry_exp_factor=2.0, ) -> bool: tag_name = None @@ -3546,7 +3548,7 @@ class Project: # At least one retry minimum due to git remote prune. retry_fetches = max(retry_fetches, 2) - retry_cur_sleep = retry_sleep_initial_sec + retry_cur_sleep = self.RETRY_SLEEP_INITIAL_SEC ok = prune_tried = False try: for try_n in range(retry_fetches): @@ -5488,6 +5490,10 @@ class MetaProject(Project): class RepoProject(MetaProject): """The MetaProject for repo itself.""" + # The automated release process deletes the stable branch and then + # recreates it, so wait long enough for it to come back before retrying. + RETRY_SLEEP_INITIAL_SEC = 10.0 + @property def LastFetch(self): try: diff --git a/repo b/repo index dd9934ff9..47c30cec5 100755 --- a/repo +++ b/repo @@ -26,7 +26,7 @@ import platform import shlex import subprocess import sys -from typing import NamedTuple +from typing import NamedTuple, Tuple # These should never be newer than the main.py version since this needs to be a @@ -129,7 +129,7 @@ if not REPO_REV: BUG_URL = "https://issues.gerritcodereview.com/issues/new?component=1370071" # increment this whenever we make important changes to this script -VERSION = (2, 65) +VERSION = (2, 69) # increment this if the MAINTAINER_KEYS block is modified KEYRING_VERSION = (2, 3) @@ -220,6 +220,10 @@ repodir = ".repo" # name of repo's private directory S_repo = "repo" # special repo repository S_manifests = "manifests" # special manifest repository REPO_MAIN = S_repo + "/main.py" # main script +# The automated release process deletes the stable branch and then recreates it +# at the new release, so a fetch can briefly miss it. Wait this long and +# refetch once before giving up on --repo-rev. +REV_REFETCH_DELAY_SEC = 10 import errno @@ -228,6 +232,7 @@ import optparse import re import shutil import stat +import time import urllib.error import urllib.request @@ -631,6 +636,7 @@ def _Init(args): dst = dst_final + ".tmp" shutil.rmtree(dst, ignore_errors=True) _Clone(url, dst, opt.clone_bundle, opt.quiet, opt.verbose) + _RefetchIfRevMissing(url, dst, rev, opt.quiet, opt.verbose) remote_ref, rev = check_repo_rev( dst, rev, opt.repo_verify, quiet=opt.quiet @@ -1005,7 +1011,43 @@ def _Clone(url, cwd, clone_bundle, quiet, verbose): _Fetch(url, cwd, "origin", quiet, verbose) -def resolve_repo_rev(cwd, committish): +def _RefetchIfRevMissing( + url: str, cwd: str, rev: str, quiet: bool, verbose: bool +) -> None: + """Refetch once if |rev| doesn't resolve. + + This doesn't fail if |rev| still doesn't resolve; check_repo_rev reports + that. + + Any unresolvable |rev| is retried, not just branches. Telling a branch + apart from a tag or commit isn't possible while it's missing, and the only + cost for a bad --repo-rev is a short delay before the same error. + + Args: + url: The URL of the repo source. + cwd: The git checkout to fetch into. + rev: The --repo-rev to wait for. + quiet: Whether to suppress fetch output. + verbose: Whether to show verbose fetch output. + """ + try: + resolve_repo_rev(cwd, rev, report_errors=False) + return + except CloneFailure: + pass + # Print even with --quiet since this explains the delay. + print( + f'repo: warning: unable to resolve "{rev}"; ' + f"refetching in {REV_REFETCH_DELAY_SEC} seconds", + file=sys.stderr, + ) + time.sleep(REV_REFETCH_DELAY_SEC) + _Fetch(url, cwd, "origin", quiet, verbose) + + +def resolve_repo_rev( + cwd: str, committish: str, report_errors: bool = True +) -> Tuple[str, str]: """Figure out what REPO_REV represents. We support: @@ -1016,6 +1058,7 @@ def resolve_repo_rev(cwd, committish): Args: cwd: The git checkout to run in. committish: The REPO_REV argument to resolve. + report_errors: Whether to print an error if it can't be resolved. Returns: A tuple of (remote ref, commit) as makes sense for the committish. @@ -1034,17 +1077,18 @@ def resolve_repo_rev(cwd, committish): ) return None if ret.returncode else ret.stdout.strip() + def error(msg: str) -> CloneFailure: + if report_errors: + print(f"repo: error: {msg}", file=sys.stderr) + return CloneFailure() + # An explicit branch. if committish.startswith("refs/heads/"): remote_ref = committish committish = committish[len("refs/heads/") :] rev = resolve("refs/remotes/origin/%s" % committish) if rev is None: - print( - f'repo: error: unknown branch "{committish}"', - file=sys.stderr, - ) - raise CloneFailure() + raise error(f'unknown branch "{committish}"') return (remote_ref, rev) # An explicit tag. @@ -1053,11 +1097,7 @@ def resolve_repo_rev(cwd, committish): committish = committish[len("refs/tags/") :] rev = resolve(remote_ref) if rev is None: - print( - f'repo: error: unknown tag "{committish}"', - file=sys.stderr, - ) - raise CloneFailure() + raise error(f'unknown tag "{committish}"') return (remote_ref, rev) # See if it's a short branch name. @@ -1076,11 +1116,7 @@ def resolve_repo_rev(cwd, committish): return (rev, rev) # Give up! - print( - f'repo: error: unable to resolve "{committish}"', - file=sys.stderr, - ) - raise CloneFailure() + raise error(f'unable to resolve "{committish}"') def verify_rev(cwd, remote_ref, rev, quiet): diff --git a/tests/test_project.py b/tests/test_project.py index 8908c79e3..21f30ba99 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -15,6 +15,7 @@ """Unittests for the project.py module.""" import contextlib +import io import os from pathlib import Path import shutil @@ -2113,6 +2114,35 @@ def test_metaproject_has_changes( assert proj.HasChanges is expected +@pytest.mark.parametrize( + "project_cls,expected_sleep", + ( + (project.ManifestProject, 4.0), + # Repo waits longer for a briefly deleted stable branch to come back. + (project.RepoProject, 10.0), + ), + ids=lambda v: v.__name__ if isinstance(v, type) else None, +) +def test_remote_fetch_retry_sleep( + project_cls: Type[project.MetaProject], expected_sleep: float +) -> None: + """A failed fetch waits RETRY_SLEEP_INITIAL_SEC before retrying.""" + with utils_for_test.TempGitTree() as tempdir: + proj = _create_meta_project(tempdir, project_cls) + proj.config.GetRemote("origin").ResetFetch() + proj.revisionExpr = "refs/heads/stable" + with mock.patch.object(project, "GitCommand") as mock_git: + # What git does while stable is deleted. + mock_git.return_value.Wait.return_value = 128 + mock_git.return_value.stdout = ( + "fatal: couldn't find remote ref refs/heads/stable\n" + ) + with mock.patch.object(project.time, "sleep") as mock_sleep: + assert not proj._RemoteFetch(output_redir=io.StringIO()) + + assert mock_sleep.call_args_list == [mock.call(expected_sleep)] + + @pytest.mark.parametrize( "state,expect_failure", [ diff --git a/tests/test_wrapper.py b/tests/test_wrapper.py index 12c30c786..d27b1705c 100644 --- a/tests/test_wrapper.py +++ b/tests/test_wrapper.py @@ -515,6 +515,62 @@ class TestResolveRepoRev: with pytest.raises(repo_wrapper.CloneFailure): repo_wrapper.resolve_repo_rev(git_checkout.git_dir, "boooooooya") + @pytest.mark.parametrize( + "committish", + ["refs/heads/unknown", "refs/tags/unknown", "boooooooya"], + ) + @pytest.mark.parametrize("report_errors", [True, False]) + def test_report_errors( + self, + repo_wrapper: wrapper.Wrapper, + git_checkout: GitCheckout, + capsys: pytest.CaptureFixture, + committish: str, + report_errors: bool, + ) -> None: + """Check errors are only printed when requested.""" + with pytest.raises(repo_wrapper.CloneFailure): + repo_wrapper.resolve_repo_rev( + git_checkout.git_dir, committish, report_errors=report_errors + ) + assert ("repo: error:" in capsys.readouterr().err) == report_errors + + +class TestRefetchIfRevMissing: + """Check _RefetchIfRevMissing behavior.""" + + @pytest.mark.parametrize("missing", [False, True]) + def test_refetch( + self, + repo_wrapper: wrapper.Wrapper, + capsys: pytest.CaptureFixture, + missing: bool, + ) -> None: + """Check it refetches once only if the rev doesn't resolve.""" + mock_resolve = mock.Mock( + side_effect=repo_wrapper.CloneFailure() if missing else None + ) + with mock.patch.object(repo_wrapper, "_Fetch") as mock_fetch: + with mock.patch.object(repo_wrapper.time, "sleep") as mock_sleep: + with mock.patch.object( + repo_wrapper, "resolve_repo_rev", mock_resolve + ): + repo_wrapper._RefetchIfRevMissing( + "url", "/dst", "stable", False, False + ) + + # It resolves quietly; check_repo_rev reports the error. + mock_resolve.assert_called_once_with( + "/dst", "stable", report_errors=False + ) + fetches = int(missing) + fetch = mock.call("url", "/dst", "origin", False, False) + assert mock_fetch.call_args_list == [fetch] * fetches + delay = repo_wrapper.REV_REFETCH_DELAY_SEC + assert mock_sleep.call_args_list == [mock.call(delay)] * fetches + warnings = capsys.readouterr().err.count(f"refetching in {delay} ") + assert warnings == fetches + class TestCheckRepoVerify: """Check check_repo_verify behavior."""