constraints.txt

@@ -1,2 +1 @@
-# NB: Keep in sync with run_tests.vpython3.
-black<26
+black<24

git_command.py

@@ -238,9 +238,9 @@ def _build_env(
             s = p + " " + s
         env["GIT_CONFIG_PARAMETERS"] = s
     if "GIT_ALLOW_PROTOCOL" not in env:
-        env["GIT_ALLOW_PROTOCOL"] = (
-            "file:git:http:https:ssh:persistent-http:persistent-https:sso:rpc"
-        )
+        env[
+            "GIT_ALLOW_PROTOCOL"
+        ] = "file:git:http:https:ssh:persistent-http:persistent-https:sso:rpc"
     env["GIT_HTTP_USER_AGENT"] = user_agent.git
 
     if objdir:
@@ -350,9 +350,9 @@ class GitCommand:
                         "Project": e.project,
                         "CommandName": command_name,
                         "Message": str(e),
-                        "ReturnCode": (
-                            str(e.git_rc) if e.git_rc is not None else None
-                        ),
+                        "ReturnCode": str(e.git_rc)
+                        if e.git_rc is not None
+                        else None,
                         "IsError": log_as_error,
                     }
                 )

manifest_xml.py

@@ -1014,9 +1014,9 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
     def SetManifestOverride(self, path):
         """Override manifestFile.  The caller must call Unload()"""
-        self._outer_client.manifest.manifestFileOverrides[self.path_prefix] = (
-            path
-        )
+        self._outer_client.manifest.manifestFileOverrides[
+            self.path_prefix
+        ] = path
 
     @property
     def UseLocalManifests(self):
@@ -2112,22 +2112,22 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         # implementation:
         # https://eclipse.googlesource.com/jgit/jgit/+/9110037e3e9461ff4dac22fee84ef3694ed57648/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectChecker.java#884
         BAD_CODEPOINTS = {
-            "\u200c",  # ZERO WIDTH NON-JOINER
-            "\u200d",  # ZERO WIDTH JOINER
-            "\u200e",  # LEFT-TO-RIGHT MARK
-            "\u200f",  # RIGHT-TO-LEFT MARK
-            "\u202a",  # LEFT-TO-RIGHT EMBEDDING
-            "\u202b",  # RIGHT-TO-LEFT EMBEDDING
-            "\u202c",  # POP DIRECTIONAL FORMATTING
-            "\u202d",  # LEFT-TO-RIGHT OVERRIDE
-            "\u202e",  # RIGHT-TO-LEFT OVERRIDE
-            "\u206a",  # INHIBIT SYMMETRIC SWAPPING
-            "\u206b",  # ACTIVATE SYMMETRIC SWAPPING
-            "\u206c",  # INHIBIT ARABIC FORM SHAPING
-            "\u206d",  # ACTIVATE ARABIC FORM SHAPING
-            "\u206e",  # NATIONAL DIGIT SHAPES
-            "\u206f",  # NOMINAL DIGIT SHAPES
-            "\ufeff",  # ZERO WIDTH NO-BREAK SPACE
+            "\u200C",  # ZERO WIDTH NON-JOINER
+            "\u200D",  # ZERO WIDTH JOINER
+            "\u200E",  # LEFT-TO-RIGHT MARK
+            "\u200F",  # RIGHT-TO-LEFT MARK
+            "\u202A",  # LEFT-TO-RIGHT EMBEDDING
+            "\u202B",  # RIGHT-TO-LEFT EMBEDDING
+            "\u202C",  # POP DIRECTIONAL FORMATTING
+            "\u202D",  # LEFT-TO-RIGHT OVERRIDE
+            "\u202E",  # RIGHT-TO-LEFT OVERRIDE
+            "\u206A",  # INHIBIT SYMMETRIC SWAPPING
+            "\u206B",  # ACTIVATE SYMMETRIC SWAPPING
+            "\u206C",  # INHIBIT ARABIC FORM SHAPING
+            "\u206D",  # ACTIVATE ARABIC FORM SHAPING
+            "\u206E",  # NATIONAL DIGIT SHAPES
+            "\u206F",  # NOMINAL DIGIT SHAPES
+            "\uFEFF",  # ZERO WIDTH NO-BREAK SPACE
         }
         if BAD_CODEPOINTS & path_codepoints:
             # This message is more expansive than reality, but should be fine.

pager.py

@@ -40,7 +40,7 @@ def RunPager(globalConfig):
 
 
 def TerminatePager():
-    global pager_process
+    global pager_process, old_stdout, old_stderr
     if pager_process:
         sys.stdout.flush()
         sys.stderr.flush()

platform_utils.py

@@ -156,12 +156,6 @@ def remove(path, missing_ok=False):
                 os.rmdir(longpath)
             else:
                 os.remove(longpath)
-        elif (
-            e.errno == errno.EROFS
-            and missing_ok
-            and not os.path.exists(longpath)
-        ):
-            pass
         elif missing_ok and e.errno == errno.ENOENT:
             pass
         else:

project.py

@@ -2755,14 +2755,6 @@ class Project:
                 # field; it doesn't exist, thus abort the optimization attempt
                 # and do a full sync.
                 break
-            elif depth and is_sha1 and ret == 1:
-                # In sha1 mode, when depth is enabled, syncing the revision
-                # from upstream may not work because some servers only allow
-                # fetching named refs. Fetching a specific sha1 may result
-                # in an error like 'server does not allow request for
-                # unadvertised object'. In this case, attempt a full sync
-                # without depth.
-                break
             elif ret < 0:
                 # Git died with a signal, exit immediately.
                 break
@@ -2883,14 +2875,7 @@ class Project:
 
         # We do not use curl's --retry option since it generally doesn't
         # actually retry anything; code 18 for example, it will not retry on.
-        cmd = [
-            "curl",
-            "--fail",
-            "--output",
-            tmpPath,
-            "--netrc-optional",
-            "--location",
-        ]
+        cmd = ["curl", "--fail", "--output", tmpPath, "--netrc", "--location"]
         if quiet:
             cmd += ["--silent", "--show-error"]
         if os.path.exists(tmpPath):

pyproject.toml

@@ -16,8 +16,3 @@
 line-length = 80
 # NB: Keep in sync with tox.ini.
 target-version = ['py36', 'py37', 'py38', 'py39', 'py310', 'py311'] #, 'py312'
-
-[tool.pytest.ini_options]
-markers = """
-    skip_cq: Skip tests in the CQ. Should be rarely used!
-"""

release/util.py

@@ -16,7 +16,6 @@
 
 import os
 import re
-import shlex
 import subprocess
 import sys
 
@@ -36,7 +35,12 @@ KEYID_ECC = "E1F9040D7A3F6DAFAC897CD3D3B95DA243E48A39"
 
 def cmdstr(cmd):
     """Get a nicely quoted shell command."""
-    return " ".join(shlex.quote(x) for x in cmd)
+    ret = []
+    for arg in cmd:
+        if not re.match(r"^[a-zA-Z0-9/_.=-]+$", arg):
+            arg = f'"{arg}"'
+        ret.append(arg)
+    return " ".join(ret)
 
 
 def run(opts, cmd, check=True, **kwargs):

repo

@@ -27,7 +27,6 @@ import platform
 import shlex
 import subprocess
 import sys
-from typing import NamedTuple
 
 
 # These should never be newer than the main.py version since this needs to be a
@@ -57,14 +56,9 @@ class Trace:
 trace = Trace()
 
 
-def cmdstr(cmd):
-    """Get a nicely quoted shell command."""
-    return " ".join(shlex.quote(x) for x in cmd)
-
-
 def exec_command(cmd):
     """Execute |cmd| or return None on failure."""
-    trace.print(":", cmdstr(cmd))
+    trace.print(":", " ".join(cmd))
     try:
         if platform.system() == "Windows":
             ret = subprocess.call(cmd)
@@ -130,7 +124,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, 54)
+VERSION = (2, 50)
 
 # increment this if the MAINTAINER_KEYS block is modified
 KEYRING_VERSION = (2, 3)
@@ -223,6 +217,7 @@ S_manifests = "manifests"  # special manifest repository
 REPO_MAIN = S_repo + "/main.py"  # main script
 
 
+import collections
 import errno
 import json
 import optparse
@@ -487,6 +482,16 @@ def InitParser(parser):
     return parser
 
 
+# This is a poor replacement for subprocess.run until we require Python 3.6+.
+RunResult = collections.namedtuple(
+    "RunResult", ("returncode", "stdout", "stderr")
+)
+
+
+class RunError(Exception):
+    """Error when running a command failed."""
+
+
 def run_command(cmd, **kwargs):
     """Run |cmd| and return its output."""
     check = kwargs.pop("check", False)
@@ -511,7 +516,7 @@ def run_command(cmd, **kwargs):
     # Run & package the results.
     proc = subprocess.Popen(cmd, **kwargs)
     (stdout, stderr) = proc.communicate(input=cmd_input)
-    dbg = ": " + cmdstr(cmd)
+    dbg = ": " + " ".join(cmd)
     if cmd_input is not None:
         dbg += " 0<|"
     if stdout == subprocess.PIPE:
@@ -521,9 +526,7 @@ def run_command(cmd, **kwargs):
     elif stderr == subprocess.STDOUT:
         dbg += " 2>&1"
     trace.print(dbg)
-    ret = subprocess.CompletedProcess(
-        cmd, proc.returncode, decode(stdout), decode(stderr)
-    )
+    ret = RunResult(proc.returncode, decode(stdout), decode(stderr))
 
     # If things failed, print useful debugging output.
     if check and ret.returncode:
@@ -544,13 +547,13 @@ def run_command(cmd, **kwargs):
 
         _print_output("stdout", ret.stdout)
         _print_output("stderr", ret.stderr)
-        # This will raise subprocess.CalledProcessError for us.
-        ret.check_returncode()
+        raise RunError(ret)
 
     return ret
 
 
 class CloneFailure(Exception):
+
     """Indicate the remote clone of repo itself failed."""
 
 
@@ -669,20 +672,15 @@ def run_git(*args, **kwargs):
             file=sys.stderr,
         )
         sys.exit(1)
-    except subprocess.CalledProcessError:
+    except RunError:
         raise CloneFailure()
 
 
-class GitVersion(NamedTuple):
-    """The git version info broken down into components for easy analysis.
-
-    Similar to Python's sys.version_info.
-    """
-
-    major: int
-    minor: int
-    micro: int
-    full: int
+# The git version info broken down into components for easy analysis.
+# Similar to Python's sys.version_info.
+GitVersion = collections.namedtuple(
+    "GitVersion", ("major", "minor", "micro", "full")
+)
 
 
 def ParseGitVersion(ver_str=None):
@@ -848,11 +846,10 @@ def _GetRepoConfig(name):
         return None
     else:
         print(
-            f"repo: error: git {cmdstr(cmd)} failed:\n{ret.stderr}",
+            f"repo: error: git {' '.join(cmd)} failed:\n{ret.stderr}",
             file=sys.stderr,
         )
-        # This will raise subprocess.CalledProcessError for us.
-        ret.check_returncode()
+        raise RunError()
 
 
 def _InitHttp():

run_tests

@@ -15,57 +15,16 @@
 
 """Wrapper to run linters and pytest with the right settings."""
 
-import functools
 import os
 import subprocess
 import sys
-from typing import List
+
+import pytest
 
 
 ROOT_DIR = os.path.dirname(os.path.realpath(__file__))
 
 
-@functools.lru_cache()
-def is_ci() -> bool:
-    """Whether we're running in our CI system."""
-    return os.getenv("LUCI_CQ") == "yes"
-
-
-def run_pytest(argv: List[str]) -> int:
-    """Returns the exit code from pytest."""
-    if is_ci():
-        argv = ["-m", "not skip_cq"] + argv
-
-    return subprocess.run(
-        [sys.executable, "-m", "pytest"] + argv,
-        check=False,
-        cwd=ROOT_DIR,
-    ).returncode
-
-
-def run_pytest_py38(argv: List[str]) -> int:
-    """Returns the exit code from pytest under Python 3.8."""
-    if is_ci():
-        argv = ["-m", "not skip_cq"] + argv
-
-    try:
-        return subprocess.run(
-            [
-                "vpython3",
-                "-vpython-spec",
-                "run_tests.vpython3.8",
-                "-m",
-                "pytest",
-            ]
-            + argv,
-            check=False,
-            cwd=ROOT_DIR,
-        ).returncode
-    except FileNotFoundError:
-        # Skip if the user doesn't have vpython from depot_tools.
-        return 0
-
-
 def run_black():
     """Returns the exit code from black."""
     # Black by default only matches .py files.  We have to list standalone
@@ -79,40 +38,32 @@ def run_black():
     return subprocess.run(
         [sys.executable, "-m", "black", "--check", ROOT_DIR] + extra_programs,
         check=False,
-        cwd=ROOT_DIR,
     ).returncode
 
 
 def run_flake8():
     """Returns the exit code from flake8."""
     return subprocess.run(
-        [sys.executable, "-m", "flake8", ROOT_DIR],
-        check=False,
-        cwd=ROOT_DIR,
+        [sys.executable, "-m", "flake8", ROOT_DIR], check=False
     ).returncode
 
 
 def run_isort():
     """Returns the exit code from isort."""
     return subprocess.run(
-        [sys.executable, "-m", "isort", "--check", ROOT_DIR],
-        check=False,
-        cwd=ROOT_DIR,
+        [sys.executable, "-m", "isort", "--check", ROOT_DIR], check=False
     ).returncode
 
 
 def main(argv):
     """The main entry."""
     checks = (
-        functools.partial(run_pytest, argv),
-        functools.partial(run_pytest_py38, argv),
+        lambda: pytest.main(argv),
         run_black,
         run_flake8,
         run_isort,
     )
-    # Run all the tests all the time to get full feedback.  Don't exit on the
-    # first error as that makes it more difficult to iterate in the CQ.
-    return 1 if sum(c() for c in checks) else 0
+    return 0 if all(not c() for c in checks) else 1
 
 
 if __name__ == "__main__":

run_tests.vpython3

@@ -54,43 +54,42 @@ wheel: <
   version: "version:21.4.0"
 >
 
-# NB: Keep in sync with constraints.txt.
 wheel: <
   name: "infra/python/wheels/black-py3"
-  version: "version:25.1.0"
+  version: "version:23.1.0"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/mypy-extensions-py3"
   version: "version:0.4.3"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/tomli-py3"
   version: "version:2.0.1"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/platformdirs-py3"
   version: "version:2.5.2"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/pathspec-py3"
   version: "version:0.9.0"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/typing-extensions-py3"
   version: "version:4.3.0"
 >
 
-# Required by black==25.1.0
+# Required by black==23.1.0
 wheel: <
   name: "infra/python/wheels/click-py3"
   version: "version:8.0.3"

run_tests.vpython3.8

@@ -1,67 +0,0 @@
-# This is a vpython "spec" file.
-#
-# Read more about `vpython` and how to modify this file here:
-#   https://chromium.googlesource.com/infra/infra/+/main/doc/users/vpython.md
-# List of available wheels:
-#   https://chromium.googlesource.com/infra/infra/+/main/infra/tools/dockerbuild/wheels.md
-
-python_version: "3.8"
-
-wheel: <
-  name: "infra/python/wheels/pytest-py3"
-  version: "version:8.3.4"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/py-py2_py3"
-  version: "version:1.11.0"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/iniconfig-py3"
-  version: "version:1.1.1"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/packaging-py3"
-  version: "version:23.0"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/pluggy-py3"
-  version: "version:1.5.0"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/toml-py3"
-  version: "version:0.10.1"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/tomli-py3"
-  version: "version:2.1.0"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/pyparsing-py3"
-  version: "version:3.0.7"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/attrs-py2_py3"
-  version: "version:21.4.0"
->
-
-# Required by pytest==8.3.4
-wheel: <
-  name: "infra/python/wheels/exceptiongroup-py3"
-  version: "version:1.1.2"
->

subcmds/diffmanifests.py

@@ -233,9 +233,9 @@ synced and their revisions won't be found.
             )
             self.printRevision = self.out.nofmt_printer("revision", fg="yellow")
         else:
-            self.printProject = self.printAdded = self.printRemoved = (
-                self.printRevision
-            ) = self.printText
+            self.printProject = (
+                self.printAdded
+            ) = self.printRemoved = self.printRevision = self.printText
 
         manifest1 = RepoClient(self.repodir)
         manifest1.Override(args[0], load_local_manifests=False)

subcmds/sync.py

@@ -350,8 +350,6 @@ later is required to fix a server side protocol bug.
     # value later on.
     PARALLEL_JOBS = 0
 
-    _JOBS_WARN_THRESHOLD = 100
-
     def _Options(self, p, show_smart=True):
         p.add_option(
             "--jobs-network",
@@ -1730,24 +1728,6 @@ later is required to fix a server side protocol bug.
         opt.jobs_network = min(opt.jobs_network, jobs_soft_limit)
         opt.jobs_checkout = min(opt.jobs_checkout, jobs_soft_limit)
 
-        # Warn once if effective job counts seem excessively high.
-        # Prioritize --jobs, then --jobs-network, then --jobs-checkout.
-        job_options_to_check = (
-            ("--jobs", opt.jobs),
-            ("--jobs-network", opt.jobs_network),
-            ("--jobs-checkout", opt.jobs_checkout),
-        )
-        for name, value in job_options_to_check:
-            if value > self._JOBS_WARN_THRESHOLD:
-                logger.warning(
-                    "High job count (%d > %d) specified for %s; this may "
-                    "lead to excessive resource usage or diminishing returns.",
-                    value,
-                    self._JOBS_WARN_THRESHOLD,
-                    name,
-                )
-                break
-
     def Execute(self, opt, args):
         errors = []
         try:

tests/test_git_command.py

@@ -21,8 +21,6 @@ import subprocess
 import unittest
 from unittest import mock
 
-import pytest
-
 import git_command
 import wrapper
 
@@ -265,7 +263,6 @@ class UserAgentUnitTest(unittest.TestCase):
         m = re.match(r"^[^ ]+$", os_name)
         self.assertIsNotNone(m)
 
-    @pytest.mark.skip_cq("TODO(b/266734831): Find out why this fails in CQ")
     def test_smoke_repo(self):
         """Make sure repo UA returns something useful."""
         ua = git_command.user_agent.repo
@@ -274,7 +271,6 @@ class UserAgentUnitTest(unittest.TestCase):
         m = re.match(r"^git-repo/[^ ]+ ([^ ]+) git/[^ ]+ Python/[0-9.]+", ua)
         self.assertIsNotNone(m)
 
-    @pytest.mark.skip_cq("TODO(b/266734831): Find out why this fails in CQ")
     def test_smoke_git(self):
         """Make sure git UA returns something useful."""
         ua = git_command.user_agent.git

tests/test_git_superproject.py

@@ -21,7 +21,6 @@ import tempfile
 import unittest
 from unittest import mock
 
-import pytest
 from test_manifest_xml import sort_attributes
 
 import git_superproject
@@ -146,7 +145,6 @@ class SuperprojectTestCase(unittest.TestCase):
         )
         self.assertIsNone(manifest.superproject)
 
-    @pytest.mark.skip_cq("TODO(b/266734831): Find out why this takes 8m+ in CQ")
     def test_superproject_get_superproject_invalid_url(self):
         """Test with an invalid url."""
         manifest = self.getXmlManifest(
@@ -170,7 +168,6 @@ class SuperprojectTestCase(unittest.TestCase):
         self.assertFalse(sync_result.success)
         self.assertTrue(sync_result.fatal)
 
-    @pytest.mark.skip_cq("TODO(b/266734831): Find out why this takes 8m+ in CQ")
     def test_superproject_get_superproject_invalid_branch(self):
         """Test with an invalid branch."""
         manifest = self.getXmlManifest(

tests/test_manifest_xml.py

@@ -51,7 +51,7 @@ INVALID_FS_PATHS = (
     "foo~",
     "blah/foo~",
     # Block Unicode characters that get normalized out by filesystems.
-    "foo\u200cbar",
+    "foo\u200Cbar",
     # Block newlines.
     "f\n/bar",
     "f\r/bar",

tests/test_wrapper.py

@@ -17,7 +17,6 @@
 import io
 import os
 import re
-import subprocess
 import sys
 import tempfile
 import unittest
@@ -126,7 +125,7 @@ class RunCommand(RepoWrapperTestCase):
         self.wrapper.run_command(["true"], check=False)
         self.wrapper.run_command(["true"], check=True)
         self.wrapper.run_command(["false"], check=False)
-        with self.assertRaises(subprocess.CalledProcessError):
+        with self.assertRaises(self.wrapper.RunError):
             self.wrapper.run_command(["false"], check=True)
 
 
@@ -359,8 +358,8 @@ class VerifyRev(RepoWrapperTestCase):
 
     def test_verify_passes(self):
         """Check when we have a valid signed tag."""
-        desc_result = subprocess.CompletedProcess([], 0, "v1.0\n", "")
-        gpg_result = subprocess.CompletedProcess([], 0, "", "")
+        desc_result = self.wrapper.RunResult(0, "v1.0\n", "")
+        gpg_result = self.wrapper.RunResult(0, "", "")
         with mock.patch.object(
             self.wrapper, "run_git", side_effect=(desc_result, gpg_result)
         ):
@@ -371,8 +370,8 @@ class VerifyRev(RepoWrapperTestCase):
 
     def test_unsigned_commit(self):
         """Check we fall back to signed tag when we have an unsigned commit."""
-        desc_result = subprocess.CompletedProcess([], 0, "v1.0-10-g1234\n", "")
-        gpg_result = subprocess.CompletedProcess([], 0, "", "")
+        desc_result = self.wrapper.RunResult(0, "v1.0-10-g1234\n", "")
+        gpg_result = self.wrapper.RunResult(0, "", "")
         with mock.patch.object(
             self.wrapper, "run_git", side_effect=(desc_result, gpg_result)
         ):
@@ -383,7 +382,7 @@ class VerifyRev(RepoWrapperTestCase):
 
     def test_verify_fails(self):
         """Check we fall back to signed tag when we have an unsigned commit."""
-        desc_result = subprocess.CompletedProcess([], 0, "v1.0-10-g1234\n", "")
+        desc_result = self.wrapper.RunResult(0, "v1.0-10-g1234\n", "")
         gpg_result = Exception
         with mock.patch.object(
             self.wrapper, "run_git", side_effect=(desc_result, gpg_result)