Don't log spam repo sync by default

Most times a repo sync after some time (week+) results in a bunch of
messages, which are not very useful for average user:
- discarding 1 commits
- Deleting obsolete checkout.

Bug: N/A
Test: repo sync
Change-Id: I881eab61f9f261e98f3656c09e73ddd159ce288c
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/397038
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Tested-by: Tomasz Wasilczyk <twasilczyk@google.com>

.flake8

@@ -11,3 +11,6 @@ extend-ignore =
     E402,
     # E731: do not assign a lambda expression, use a def
     E731,
+exclude =
+    venv,
+    .tox,

.github/workflows/close-pull-request.yml

@@ -0,0 +1,22 @@
+# GitHub actions workflow.
+# https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions
+
+# https://github.com/superbrothers/close-pull-request
+name: Close Pull Request
+
+on:
+  pull_request_target:
+    types: [opened]
+
+jobs:
+  run:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: superbrothers/close-pull-request@v3
+      with:
+        comment: >
+          Thanks for your contribution!
+          Unfortunately, we don't use GitHub pull requests to manage code
+          contributions to this repository.
+          Instead, please see [README.md](../blob/HEAD/SUBMITTING_PATCHES.md)
+          which provides full instructions on how to get involved.

.github/workflows/test-ci.yml

@@ -13,8 +13,9 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        os: [ubuntu-latest, macos-latest, windows-latest]
-        python-version: ['3.6', '3.7', '3.8', '3.9', '3.10']
+        # ubuntu-20.04 is the last version that supports python 3.6
+        os: [ubuntu-20.04, macos-latest, windows-latest]
+        python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11', '3.12']
     runs-on: ${{ matrix.os }}
 
     steps:

.isort.cfg

@@ -0,0 +1,41 @@
+# Copyright 2023 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Config file for the isort python module.
+# This is used to enforce import sorting standards.
+#
+# https://pycqa.github.io/isort/docs/configuration/options.html
+
+[settings]
+# Be compatible with `black` since it also matches what we want.
+profile = black
+
+line_length = 80
+length_sort = false
+force_single_line = true
+lines_after_imports = 2
+from_first = false
+case_sensitive = false
+force_sort_within_sections = true
+order_by_type = false
+
+# Ignore generated files.
+extend_skip_glob = *_pb2.py
+
+# Allow importing multiple classes on a single line from these modules.
+# https://google.github.io/styleguide/pyguide#s2.2-imports
+single_line_exclusions =
+    abc,
+    collections.abc,
+    typing,

color.py

@@ -17,6 +17,7 @@ import sys
 
 import pager
 
+
 COLORS = {
     None: -1,
     "normal": -1,
@@ -102,7 +103,7 @@ def SetDefaultColoring(state):
         DEFAULT = "never"
 
 
-class Coloring(object):
+class Coloring:
     def __init__(self, config, section_type):
         self._section = "color.%s" % section_type
         self._config = config
@@ -193,7 +194,7 @@ class Coloring(object):
         if not opt:
             return _Color(fg, bg, attr)
 
-        v = self._config.GetString("%s.%s" % (self._section, opt))
+        v = self._config.GetString(f"{self._section}.{opt}")
         if v is None:
             return _Color(fg, bg, attr)
 

command.py

@@ -13,14 +13,14 @@
 # limitations under the License.
 
 import multiprocessing
-import os
 import optparse
+import os
 import re
-import sys
 
-from event_log import EventLog
-from error import NoSuchProjectError
 from error import InvalidProjectGroupsError
+from error import NoSuchProjectError
+from error import RepoExitError
+from event_log import EventLog
 import progress
 
 
@@ -42,7 +42,11 @@ WORKER_BATCH_SIZE = 32
 DEFAULT_LOCAL_JOBS = min(os.cpu_count(), 8)
 
 
-class Command(object):
+class UsageError(RepoExitError):
+    """Exception thrown with invalid command usage."""
+
+
+class Command:
     """Base class for any command line action in repo."""
 
     # Singleton for all commands to track overall repo command execution and
@@ -71,7 +75,6 @@ class Command(object):
         repodir=None,
         client=None,
         manifest=None,
-        gitc_manifest=None,
         git_event_log=None,
         outer_client=None,
         outer_manifest=None,
@@ -80,7 +83,6 @@ class Command(object):
         self.client = client
         self.outer_client = outer_client or client
         self.manifest = manifest
-        self.gitc_manifest = gitc_manifest
         self.git_event_log = git_event_log
         self.outer_manifest = outer_manifest
 
@@ -215,7 +217,7 @@ class Command(object):
     def Usage(self):
         """Display usage and terminate."""
         self.OptionParser.print_usage()
-        sys.exit(1)
+        raise UsageError()
 
     def CommonValidateOptions(self, opt, args):
         """Validate common options."""
@@ -288,7 +290,7 @@ class Command(object):
                 output.end()
 
     def _ResetPathToProjectMap(self, projects):
-        self._by_path = dict((p.worktree, p) for p in projects)
+        self._by_path = {p.worktree: p for p in projects}
 
     def _UpdatePathToProjectMap(self, project):
         self._by_path[project.worktree] = project
@@ -474,8 +476,7 @@ class Command(object):
             top = self.manifest
         yield top
         if not opt.this_manifest_only:
-            for child in top.all_children:
-                yield child
+            yield from top.all_children
 
 
 class InteractiveCommand(Command):
@@ -496,17 +497,11 @@ class PagedCommand(Command):
         return True
 
 
-class MirrorSafeCommand(object):
+class MirrorSafeCommand:
     """Command permits itself to run within a mirror, and does not require a
     working directory.
     """
 
 
-class GitcAvailableCommand(object):
-    """Command that requires GITC to be available, but does not require the
-    local client to be a GITC client.
-    """
-
-
-class GitcClientCommand(object):
+class GitcClientCommand:
     """Command that requires the local client to be a GITC client."""

docs/internal-fs-layout.md

@@ -42,8 +42,12 @@ For example, if you want to change the manifest branch, you can simply run
     change the git URL/branch that this tracks, re-run `repo init` with the new
     settings.
 
-*   `.repo_fetchtimes.json`: Used by `repo sync` to record stats when syncing
-    the various projects.
+*   `.repo_fetchtimes.json`: Used by `repo sync` to record fetch times when
+    syncing the various projects.
+
+*   `.repo_localsyncstate.json`: Used by `repo sync` to detect and warn on
+    on partial tree syncs.  Partial syncs are allowed by `repo` itself, but are
+    unsupported by many projects where `repo` is used.
 
 ### Manifests
 

docs/python-support.md

@@ -1,47 +1,93 @@
 # Supported Python Versions
 
-With Python 2.7 officially going EOL on [01 Jan 2020](https://pythonclock.org/),
-we need a support plan for the repo project itself.
-Inevitably, there will be a long tail of users who still want to use Python 2 on
-their old LTS/corp systems and have little power to change the system.
+This documents the current supported Python versions, and tries to provide
+guidance for when we decide to drop support for older versions.
 
 ## Summary
 
-* Python 3.6 (released Dec 2016) is required by default starting with repo-2.x.
-* Older versions of Python (e.g. v2.7) may use the legacy feature-frozen branch
-  based on repo-1.x.
+*   Python 3.6 (released Dec 2016) is required starting with repo-2.0.
+*   Older versions of Python (e.g. v2.7) may use old releases via the repo-1.x
+    branch, but no support is provided.
 
-## Overview
-
-We provide a branch for Python 2 users that is feature-frozen.
-Bugfixes may be added on a best-effort basis or from the community, but largely
-no new features will be added, nor is support guaranteed.
-
-Users can select this during `repo init` time via the [repo launcher].
-Otherwise the default branches (e.g. stable & main) will be used which will
-require Python 3.
-
-This means the [repo launcher] needs to support both Python 2 & Python 3, but
-since it doesn't import any other repo code, this shouldn't be too problematic.
-
-The main branch will require Python 3.6 at a minimum.
-If the system has an older version of Python 3, then users will have to select
-the legacy Python 2 branch instead.
-
-### repo hooks
+## repo hooks
 
 Projects that use [repo hooks] run on independent schedules.
-They might migrate to Python 3 earlier or later than us.
-To support them, we'll probe the shebang of the hook script and if we find an
-interpreter in there that indicates a different version than repo is currently
-running under, we'll attempt to reexec ourselves under that.
+Since it's not possible to detect what version of Python the hooks were written
+or tested against, we always import & exec them with the active Python version.
 
-For example, a hook with a header like `#!/usr/bin/python2` will have repo
-execute `/usr/bin/python2` to execute the hook code specifically if repo is
-currently running Python 3.
+If the user's Python is too new for the [repo hooks], then it is up to the hooks
+maintainer to update.
 
-For more details, consult the [repo hooks] documentation.
+## Repo launcher
+
+The [repo launcher] is an independent script that can support older versions of
+Python without holding back the rest of the codebase.
+If it detects the current version of Python is too old, it will try to reexec
+via a newer version of Python via standard `pythonX.Y` interpreter names.
+
+However, this is provided as a nicety when it is not onerous, and there is no
+official support for older versions of Python than the rest of the codebase.
+
+If your default python interpreters are too old to run the launcher even though
+you have newer versions installed, your choices are:
+
+*   Modify the [repo launcher]'s shebang to suite your environment.
+*   Download an older version of the [repo launcher] and don't upgrade it.
+    Be aware that there is no guarantee old repo launchers are WILL work with
+    current versions of repo.  Bug reports using old launchers will not be
+    accepted.
+
+## When to drop support
+
+So far, Python 3.6 has provided most of the interesting features that we want
+(e.g. typing & f-strings), and there haven't been features in newer versions
+that are critical to us.
+
+That said, let's assume we need functionality that only exists in Python 3.7.
+How do we decide when it's acceptable to drop Python 3.6?
+
+1.  Review the [Project References](./release-process.md#project-references) to
+    see what major distros are using the previous version of Python, and when
+    they go EOL.  Generally we care about Ubuntu LTS & current/previous Debian
+    stable versions.
+    *   If they're all EOL already, then go for it, drop support.
+    *   If they aren't EOL, start a thread on [repo-discuss] to see how the user
+        base feels about the proposal.
+1.  Update the "soft" versions in the codebase.  This will start warning users
+    that the older version is deprecated.
+    *   Update [repo](/repo) if the launcher needs updating.
+        This only helps with people who download newer launchers.
+    *   Update [main.py](/main.py) for the main codebase.
+        This warns for everyone regardless of [repo launcher] version.
+    *   Update [requirements.json](/requirements.json).
+        This allows [repo launcher] to display warnings/errors without having
+        to execute the new codebase.  This helps in case of syntax or module
+        changes where older versions won't even be able to import the new code.
+1.  After some grace period (ideally at least 2 quarters after the first release
+    with the updated soft requirements), update the "hard" versions, and then
+    start using the new functionality.
+
+## Python 2.7 & 3.0-3.5
+
+> **There is no support for these versions.**
+> **Do not file bugs if you are using old Python versions.**
+> **Any such reports will be marked invalid and ignored.**
+> **Upgrade your distro and/or runtime instead.**
+
+Fetch an old version of the [repo launcher]:
+
+```sh
+$ curl https://storage.googleapis.com/git-repo-downloads/repo-2.32 > ~/.bin/repo-2.32
+$ chmod a+rx ~/.bin/repo-2.32
+```
+
+Then initialize an old version of repo:
+
+```sh
+$ repo-2.32 init --repo-rev=repo-1 ...
+```
 
 
+[repo-discuss]: https://groups.google.com/forum/#!forum/repo-discuss
 [repo hooks]: ./repo-hooks.md
 [repo launcher]: ../repo

editor.py

@@ -14,15 +14,15 @@
 
 import os
 import re
-import sys
 import subprocess
+import sys
 import tempfile
 
 from error import EditorError
 import platform_utils
 
 
-class Editor(object):
+class Editor:
     """Manages the user's preferred text editor."""
 
     _editor = None
@@ -104,9 +104,7 @@ least one of these before using this command.""",  # noqa: E501
             try:
                 rc = subprocess.Popen(args, shell=shell).wait()
             except OSError as e:
-                raise EditorError(
-                    "editor failed, %s: %s %s" % (str(e), editor, path)
-                )
+                raise EditorError(f"editor failed, {str(e)}: {editor} {path}")
             if rc != 0:
                 raise EditorError(
                     "editor failed with exit status %d: %s %s"

error.py

@@ -12,8 +12,55 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from typing import List
 
-class ManifestParseError(Exception):
+
+class BaseRepoError(Exception):
+    """All repo specific exceptions derive from BaseRepoError."""
+
+
+class RepoError(BaseRepoError):
+    """Exceptions thrown inside repo that can be handled."""
+
+    def __init__(self, *args, project: str = None) -> None:
+        super().__init__(*args)
+        self.project = project
+
+
+class RepoExitError(BaseRepoError):
+    """Exception thrown that result in termination of repo program.
+    - Should only be handled in main.py
+    """
+
+    def __init__(
+        self,
+        *args,
+        exit_code: int = 1,
+        aggregate_errors: List[Exception] = None,
+        **kwargs,
+    ) -> None:
+        super().__init__(*args, **kwargs)
+        self.exit_code = exit_code
+        self.aggregate_errors = aggregate_errors
+
+
+class RepoUnhandledExceptionError(RepoExitError):
+    """Exception that maintains error as reason for program exit."""
+
+    def __init__(
+        self,
+        error: BaseException,
+        **kwargs,
+    ) -> None:
+        super().__init__(error, **kwargs)
+        self.error = error
+
+
+class SilentRepoExitError(RepoExitError):
+    """RepoExitError that should no include CLI logging of issue/issues."""
+
+
+class ManifestParseError(RepoExitError):
     """Failed to parse the manifest file."""
 
 
@@ -25,11 +72,11 @@ class ManifestInvalidPathError(ManifestParseError):
     """A path used in <copyfile> or <linkfile> is incorrect."""
 
 
-class NoManifestException(Exception):
+class NoManifestException(RepoExitError):
     """The required manifest does not exist."""
 
-    def __init__(self, path, reason):
-        super().__init__(path, reason)
+    def __init__(self, path, reason, **kwargs):
+        super().__init__(path, reason, **kwargs)
         self.path = path
         self.reason = reason
 
@@ -37,55 +84,72 @@ class NoManifestException(Exception):
         return self.reason
 
 
-class EditorError(Exception):
+class EditorError(RepoError):
     """Unspecified error from the user's text editor."""
 
-    def __init__(self, reason):
-        super().__init__(reason)
+    def __init__(self, reason, **kwargs):
+        super().__init__(reason, **kwargs)
         self.reason = reason
 
     def __str__(self):
         return self.reason
 
 
-class GitError(Exception):
-    """Unspecified internal error from git."""
+class GitError(RepoError):
+    """Unspecified git related error."""
 
-    def __init__(self, command):
-        super().__init__(command)
-        self.command = command
+    def __init__(self, message, command_args=None, **kwargs):
+        super().__init__(message, **kwargs)
+        self.message = message
+        self.command_args = command_args
 
     def __str__(self):
-        return self.command
+        return self.message
 
 
-class UploadError(Exception):
+class GitcUnsupportedError(RepoExitError):
+    """Gitc no longer supported."""
+
+
+class UploadError(RepoError):
     """A bundle upload to Gerrit did not succeed."""
 
-    def __init__(self, reason):
-        super().__init__(reason)
+    def __init__(self, reason, **kwargs):
+        super().__init__(reason, **kwargs)
         self.reason = reason
 
     def __str__(self):
         return self.reason
 
 
-class DownloadError(Exception):
+class DownloadError(RepoExitError):
     """Cannot download a repository."""
 
-    def __init__(self, reason):
-        super().__init__(reason)
+    def __init__(self, reason, **kwargs):
+        super().__init__(reason, **kwargs)
         self.reason = reason
 
     def __str__(self):
         return self.reason
 
 
-class NoSuchProjectError(Exception):
+class InvalidArgumentsError(RepoExitError):
+    """Invalid command Arguments."""
+
+
+class SyncError(RepoExitError):
+    """Cannot sync repo."""
+
+
+class UpdateManifestError(RepoExitError):
+    """Cannot update manifest."""
+
+
+class NoSuchProjectError(RepoExitError):
     """A specified project does not exist in the work tree."""
 
-    def __init__(self, name=None):
-        super().__init__(name)
+    def __init__(self, name=None, **kwargs):
+        super().__init__(**kwargs)
         self.name = name
 
     def __str__(self):
@@ -94,11 +158,11 @@ class NoSuchProjectError(Exception):
         return self.name
 
 
-class InvalidProjectGroupsError(Exception):
+class InvalidProjectGroupsError(RepoExitError):
     """A specified project is not suitable for the specified groups"""
 
-    def __init__(self, name=None):
-        super().__init__(name)
+    def __init__(self, name=None, **kwargs):
+        super().__init__(**kwargs)
         self.name = name
 
     def __str__(self):
@@ -107,7 +171,7 @@ class InvalidProjectGroupsError(Exception):
         return self.name
 
 
-class RepoChangedException(Exception):
+class RepoChangedException(BaseRepoError):
     """Thrown if 'repo sync' results in repo updating its internal
     repo or manifest repositories.  In this special case we must
     use exec to re-execute repo with the new code and manifest.
@@ -118,7 +182,7 @@ class RepoChangedException(Exception):
         self.extra_args = extra_args or []
 
 
-class HookError(Exception):
+class HookError(RepoError):
     """Thrown if a 'repo-hook' could not be run.
 
     The common case is that the file wasn't present when we tried to run it.

event_log.py

@@ -15,12 +15,13 @@
 import json
 import multiprocessing
 
+
 TASK_COMMAND = "command"
 TASK_SYNC_NETWORK = "sync-network"
 TASK_SYNC_LOCAL = "sync-local"
 
 
-class EventLog(object):
+class EventLog:
     """Event log that records events that occurred during a repo invocation.
 
     Events are written to the log as a consecutive JSON entries, one per line.

fetch.py

@@ -19,6 +19,12 @@ import sys
 from urllib.parse import urlparse
 from urllib.request import urlopen
 
+from error import RepoExitError
+
+
+class FetchFileError(RepoExitError):
+    """Exit error when fetch_file fails."""
+
 
 def fetch_file(url, verbose=False):
     """Fetch a file from the specified source using the appropriate protocol.
@@ -29,6 +35,7 @@ def fetch_file(url, verbose=False):
     scheme = urlparse(url).scheme
     if scheme == "gs":
         cmd = ["gsutil", "cat", url]
+        errors = []
         try:
             result = subprocess.run(
                 cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True
@@ -41,9 +48,10 @@ def fetch_file(url, verbose=False):
                 )
             return result.stdout
         except subprocess.CalledProcessError as e:
+            errors.append(e)
             print(
                 'fatal: error running "gsutil": %s' % e.stderr, file=sys.stderr
             )
-        sys.exit(1)
+        raise FetchFileError(aggregate_errors=errors)
     with urlopen(url) as f:
         return f.read()

git_command.py

@@ -13,17 +13,25 @@
 # limitations under the License.
 
 import functools
+import json
 import os
-import sys
+import re
 import subprocess
+import sys
 from typing import Any, Optional
 
 from error import GitError
+from error import RepoExitError
 from git_refs import HEAD
+from git_trace2_event_log_base import BaseEventLog
 import platform_utils
-from repo_trace import REPO_TRACE, IsTrace, Trace
+from repo_logging import RepoLogger
+from repo_trace import IsTrace
+from repo_trace import REPO_TRACE
+from repo_trace import Trace
 from wrapper import Wrapper
 
+
 GIT = "git"
 # NB: These do not need to be kept in sync with the repo launcher script.
 # These may be much newer as it allows the repo launcher to roll between
@@ -40,15 +48,24 @@ GIT_DIR = "GIT_DIR"
 
 LAST_GITDIR = None
 LAST_CWD = None
+DEFAULT_GIT_FAIL_MESSAGE = "git command failure"
+ERROR_EVENT_LOGGING_PREFIX = "RepoGitCommandError"
+# Common line length limit
+GIT_ERROR_STDOUT_LINES = 1
+GIT_ERROR_STDERR_LINES = 10
+INVALID_GIT_EXIT_CODE = 126
+
+logger = RepoLogger(__file__)
 
 
-class _GitCall(object):
+class _GitCall:
     @functools.lru_cache(maxsize=None)
     def version_tuple(self):
         ret = Wrapper().ParseGitVersion()
         if ret is None:
-            print("fatal: unable to detect git version", file=sys.stderr)
-            sys.exit(1)
+            msg = "fatal: unable to detect git version"
+            logger.error(msg)
+            raise GitRequireError(msg)
         return ret
 
     def __getattr__(self, name):
@@ -57,7 +74,7 @@ class _GitCall(object):
         def fun(*cmdv):
             command = [name]
             command.extend(cmdv)
-            return GitCommand(None, command).Wait() == 0
+            return GitCommand(None, command, add_event_log=False).Wait() == 0
 
         return fun
 
@@ -95,7 +112,43 @@ def RepoSourceVersion():
     return ver
 
 
-class UserAgent(object):
+@functools.lru_cache(maxsize=None)
+def GetEventTargetPath():
+    """Get the 'trace2.eventtarget' path from git configuration.
+
+    Returns:
+        path: git config's 'trace2.eventtarget' path if it exists, or None
+    """
+    path = None
+    cmd = ["config", "--get", "trace2.eventtarget"]
+    # TODO(https://crbug.com/gerrit/13706): Use GitConfig when it supports
+    # system git config variables.
+    p = GitCommand(
+        None,
+        cmd,
+        capture_stdout=True,
+        capture_stderr=True,
+        bare=True,
+        add_event_log=False,
+    )
+    retval = p.Wait()
+    if retval == 0:
+        # Strip trailing carriage-return in path.
+        path = p.stdout.rstrip("\n")
+    elif retval != 1:
+        # `git config --get` is documented to produce an exit status of `1`
+        # if the requested variable is not present in the configuration.
+        # Report any other return value as an error.
+        logger.error(
+            "repo: error: 'git config --get' call failed with return code: "
+            "%r, stderr: %r",
+            retval,
+            p.stderr,
+        )
+    return path
+
+
+class UserAgent:
     """Mange User-Agent settings when talking to external services
 
     We follow the style as documented here:
@@ -143,12 +196,10 @@ class UserAgent(object):
     def git(self):
         """The UA when running git."""
         if self._git_ua is None:
-            self._git_ua = "git/%s (%s) git-repo/%s" % (
-                git.version_tuple().full,
-                self.os,
-                RepoSourceVersion(),
+            self._git_ua = (
+                f"git/{git.version_tuple().full} ({self.os}) "
+                f"git-repo/{RepoSourceVersion()}"
             )
-
         return self._git_ua
 
 
@@ -163,10 +214,9 @@ def git_require(min_version, fail=False, msg=""):
         need = ".".join(map(str, min_version))
         if msg:
             msg = " for " + msg
-        print(
-            "fatal: git %s or later required%s" % (need, msg), file=sys.stderr
-        )
-        sys.exit(1)
+        error_msg = f"fatal: git {need} or later required{msg}"
+        logger.error(error_msg)
+        raise GitRequireError(error_msg)
     return False
 
 
@@ -191,7 +241,7 @@ def _build_env(
         env["GIT_SSH"] = ssh_proxy.proxy
         env["GIT_SSH_VARIANT"] = "ssh"
     if "http_proxy" in env and "darwin" == sys.platform:
-        s = "'http.proxy=%s'" % (env["http_proxy"],)
+        s = f"'http.proxy={env['http_proxy']}'"
         p = env.get("GIT_CONFIG_PARAMETERS")
         if p is not None:
             s = p + " " + s
@@ -220,7 +270,7 @@ def _build_env(
     return env
 
 
-class GitCommand(object):
+class GitCommand:
     """Wrapper around a single git invocation."""
 
     def __init__(
@@ -237,6 +287,9 @@ class GitCommand(object):
         cwd=None,
         gitdir=None,
         objdir=None,
+        verify_command=False,
+        add_event_log=True,
+        log_as_error=True,
     ):
         if project:
             if not cwd:
@@ -244,6 +297,11 @@ class GitCommand(object):
             if not gitdir:
                 gitdir = project.gitdir
 
+        self.project = project
+        self.cmdv = cmdv
+        self.verify_command = verify_command
+        self.stdout, self.stderr = None, None
+
         # Git on Windows wants its paths only using / for reliability.
         if platform_utils.isWindows():
             if objdir:
@@ -262,15 +320,67 @@ class GitCommand(object):
         command = [GIT]
         if bare:
             cwd = None
-        command.append(cmdv[0])
+        command_name = cmdv[0]
+        command.append(command_name)
         # Need to use the --progress flag for fetch/clone so output will be
         # displayed as by default git only does progress output if stderr is a
         # TTY.
-        if sys.stderr.isatty() and cmdv[0] in ("fetch", "clone"):
+        if sys.stderr.isatty() and command_name in ("fetch", "clone"):
             if "--progress" not in cmdv and "--quiet" not in cmdv:
                 command.append("--progress")
         command.extend(cmdv[1:])
 
+        event_log = (
+            BaseEventLog(env=env, add_init_count=True)
+            if add_event_log
+            else None
+        )
+
+        try:
+            self._RunCommand(
+                command,
+                env,
+                capture_stdout=capture_stdout,
+                capture_stderr=capture_stderr,
+                merge_output=merge_output,
+                ssh_proxy=ssh_proxy,
+                cwd=cwd,
+                input=input,
+            )
+            self.VerifyCommand()
+        except GitCommandError as e:
+            if event_log is not None:
+                error_info = json.dumps(
+                    {
+                        "ErrorType": type(e).__name__,
+                        "Project": e.project,
+                        "CommandName": command_name,
+                        "Message": str(e),
+                        "ReturnCode": str(e.git_rc)
+                        if e.git_rc is not None
+                        else None,
+                        "IsError": log_as_error,
+                    }
+                )
+                event_log.ErrorEvent(
+                    f"{ERROR_EVENT_LOGGING_PREFIX}:{error_info}"
+                )
+                event_log.Write(GetEventTargetPath())
+            if isinstance(e, GitPopenCommandError):
+                raise
+
+    def _RunCommand(
+        self,
+        command,
+        env,
+        capture_stdout=False,
+        capture_stderr=False,
+        merge_output=False,
+        ssh_proxy=None,
+        cwd=None,
+        input=None,
+    ):
+        # Set subprocess.PIPE for streams that need to be captured.
         stdin = subprocess.PIPE if input else None
         stdout = subprocess.PIPE if capture_stdout else None
         stderr = (
@@ -279,6 +389,30 @@ class GitCommand(object):
             else (subprocess.PIPE if capture_stderr else None)
         )
 
+        # tee_stderr acts like a tee command for stderr, in that, it captures
+        # stderr from the subprocess and streams it back to sys.stderr, while
+        # keeping a copy in-memory.
+        # This allows us to store stderr logs from the subprocess into
+        # GitCommandError.
+        # Certain git operations, such as `git push`, writes diagnostic logs,
+        # such as, progress bar for pushing, into stderr. To ensure we don't
+        # break git's UX, we need to write to sys.stderr as we read from the
+        # subprocess. Setting encoding or errors makes subprocess return
+        # io.TextIOWrapper, which is line buffered. To avoid line-buffering
+        # while tee-ing stderr, we unset these kwargs. See GitCommand._Tee
+        # for tee-ing between the streams.
+        # We tee stderr iff the caller doesn't want to capture any stream to
+        # not disrupt the existing flow.
+        # See go/tee-repo-stderr for more context.
+        tee_stderr = False
+        kwargs = {"encoding": "utf-8", "errors": "backslashreplace"}
+        if not (stdin or stdout or stderr):
+            tee_stderr = True
+            # stderr will be written back to sys.stderr even though it is
+            # piped here.
+            stderr = subprocess.PIPE
+            kwargs = {}
+
         dbg = ""
         if IsTrace():
             global LAST_CWD
@@ -325,14 +459,17 @@ class GitCommand(object):
                     command,
                     cwd=cwd,
                     env=env,
-                    encoding="utf-8",
-                    errors="backslashreplace",
                     stdin=stdin,
                     stdout=stdout,
                     stderr=stderr,
+                    **kwargs,
                 )
             except Exception as e:
-                raise GitError("%s: %s" % (command[1], e))
+                raise GitPopenCommandError(
+                    message=f"{command[1]}: {e}",
+                    project=self.project.name if self.project else None,
+                    command_args=self.cmdv,
+                )
 
             if ssh_proxy:
                 ssh_proxy.add_client(p)
@@ -340,12 +477,45 @@ class GitCommand(object):
             self.process = p
 
             try:
-                self.stdout, self.stderr = p.communicate(input=input)
+                if tee_stderr:
+                    # tee_stderr streams stderr to sys.stderr while capturing
+                    # a copy within self.stderr. tee_stderr is only enabled
+                    # when the caller wants to pipe no stream.
+                    self.stderr = self._Tee(p.stderr, sys.stderr)
+                else:
+                    self.stdout, self.stderr = p.communicate(input=input)
             finally:
                 if ssh_proxy:
                     ssh_proxy.remove_client(p)
             self.rc = p.wait()
 
+    @staticmethod
+    def _Tee(in_stream, out_stream):
+        """Writes text from in_stream to out_stream while recording in buffer.
+
+        Args:
+            in_stream: I/O stream to be read from.
+            out_stream: I/O stream to write to.
+
+        Returns:
+            A str containing everything read from the in_stream.
+        """
+        buffer = ""
+        read_size = 1024 if sys.version_info < (3, 7) else -1
+        chunk = in_stream.read1(read_size)
+        while chunk:
+            # Convert to str.
+            if not hasattr(chunk, "encode"):
+                chunk = chunk.decode("utf-8", "backslashreplace")
+
+            buffer += chunk
+            out_stream.write(chunk)
+            out_stream.flush()
+
+            chunk = in_stream.read1(read_size)
+
+        return buffer
+
     @staticmethod
     def _GetBasicEnv():
         """Return a basic env for running git under.
@@ -365,5 +535,122 @@ class GitCommand(object):
             env.pop(key, None)
         return env
 
+    def VerifyCommand(self):
+        if self.rc == 0:
+            return None
+        stdout = (
+            "\n".join(self.stdout.split("\n")[:GIT_ERROR_STDOUT_LINES])
+            if self.stdout
+            else None
+        )
+        stderr = (
+            "\n".join(self.stderr.split("\n")[:GIT_ERROR_STDERR_LINES])
+            if self.stderr
+            else None
+        )
+        project = self.project.name if self.project else None
+        raise GitCommandError(
+            project=project,
+            command_args=self.cmdv,
+            git_rc=self.rc,
+            git_stdout=stdout,
+            git_stderr=stderr,
+        )
+
     def Wait(self):
+        if self.verify_command:
+            self.VerifyCommand()
         return self.rc
+
+
+class GitRequireError(RepoExitError):
+    """Error raised when git version is unavailable or invalid."""
+
+    def __init__(self, message, exit_code: int = INVALID_GIT_EXIT_CODE):
+        super().__init__(message, exit_code=exit_code)
+
+
+class GitCommandError(GitError):
+    """
+    Error raised from a failed git command.
+    Note that GitError can refer to any Git related error (e.g. branch not
+    specified for project.py 'UploadForReview'), while GitCommandError is
+    raised exclusively from non-zero exit codes returned from git commands.
+    """
+
+    # Tuples with error formats and suggestions for those errors.
+    _ERROR_TO_SUGGESTION = [
+        (
+            re.compile("couldn't find remote ref .*"),
+            "Check if the provided ref exists in the remote.",
+        ),
+        (
+            re.compile("unable to access '.*': .*"),
+            (
+                "Please make sure you have the correct access rights and the "
+                "repository exists."
+            ),
+        ),
+        (
+            re.compile("'.*' does not appear to be a git repository"),
+            "Are you running this repo command outside of a repo workspace?",
+        ),
+        (
+            re.compile("not a git repository"),
+            "Are you running this repo command outside of a repo workspace?",
+        ),
+    ]
+
+    def __init__(
+        self,
+        message: str = DEFAULT_GIT_FAIL_MESSAGE,
+        git_rc: int = None,
+        git_stdout: str = None,
+        git_stderr: str = None,
+        **kwargs,
+    ):
+        super().__init__(
+            message,
+            **kwargs,
+        )
+        self.git_rc = git_rc
+        self.git_stdout = git_stdout
+        self.git_stderr = git_stderr
+
+    @property
+    @functools.lru_cache(maxsize=None)
+    def suggestion(self):
+        """Returns helpful next steps for the given stderr."""
+        if not self.git_stderr:
+            return self.git_stderr
+
+        for err, suggestion in self._ERROR_TO_SUGGESTION:
+            if err.search(self.git_stderr):
+                return suggestion
+
+        return None
+
+    def __str__(self):
+        args = "[]" if not self.command_args else " ".join(self.command_args)
+        error_type = type(self).__name__
+        string = f"{error_type}: '{args}' on {self.project} failed"
+
+        if self.message != DEFAULT_GIT_FAIL_MESSAGE:
+            string += f": {self.message}"
+
+        if self.git_stdout:
+            string += f"\nstdout: {self.git_stdout}"
+
+        if self.git_stderr:
+            string += f"\nstderr: {self.git_stderr}"
+
+        if self.suggestion:
+            string += f"\nsuggestion: {self.suggestion}"
+
+        return string
+
+
+class GitPopenCommandError(GitError):
+    """
+    Error raised when subprocess.Popen fails for a GitCommand
+    """

git_config.py

@@ -15,7 +15,7 @@
 import contextlib
 import datetime
 import errno
-from http.client import HTTPException
+import http.client
 import json
 import os
 import re
@@ -26,11 +26,15 @@ from typing import Union
 import urllib.error
 import urllib.request
 
-from error import GitError, UploadError
+from error import GitError
+from error import UploadError
+from git_command import GitCommand
+from git_refs import R_CHANGES
+from git_refs import R_HEADS
+from git_refs import R_TAGS
 import platform_utils
 from repo_trace import Trace
-from git_command import GitCommand
-from git_refs import R_CHANGES, R_HEADS, R_TAGS
+
 
 # Prefix that is prepended to all the keys of SyncAnalysisState's data
 # that is saved in the config.
@@ -66,7 +70,7 @@ def _key(name):
     return ".".join(parts)
 
 
-class GitConfig(object):
+class GitConfig:
     _ForUser = None
 
     _ForSystem = None
@@ -176,7 +180,7 @@ class GitConfig(object):
             config_dict[key] = self.GetString(key)
         return config_dict
 
-    def GetBoolean(self, name: str) -> Union[str, None]:
+    def GetBoolean(self, name: str) -> Union[bool, None]:
         """Returns a boolean from the configuration file.
 
         Returns:
@@ -366,7 +370,7 @@ class GitConfig(object):
             with Trace(": parsing %s", self.file):
                 with open(self._json) as fd:
                     return json.load(fd)
-        except (IOError, ValueError):
+        except (OSError, ValueError):
             platform_utils.remove(self._json, missing_ok=True)
             return None
 
@@ -374,7 +378,7 @@ class GitConfig(object):
         try:
             with open(self._json, "w") as fd:
                 json.dump(cache, fd, indent=2)
-        except (IOError, TypeError):
+        except (OSError, TypeError):
             platform_utils.remove(self._json, missing_ok=True)
 
     def _ReadGit(self):
@@ -414,7 +418,7 @@ class GitConfig(object):
         if p.Wait() == 0:
             return p.stdout
         else:
-            raise GitError("git config %s: %s" % (str(args), p.stderr))
+            raise GitError(f"git config {str(args)}: {p.stderr}")
 
 
 class RepoConfig(GitConfig):
@@ -426,7 +430,7 @@ class RepoConfig(GitConfig):
         return os.path.join(repo_config_dir, ".repoconfig/config")
 
 
-class RefSpec(object):
+class RefSpec:
     """A Git refspec line, split into its components:
 
     forced:  True if the line starts with '+'
@@ -537,7 +541,7 @@ def GetUrlCookieFile(url, quiet):
     yield cookiefile, None
 
 
-class Remote(object):
+class Remote:
     """Configuration options related to a remote."""
 
     def __init__(self, config, name):
@@ -647,13 +651,11 @@ class Remote(object):
                             userEmail, host, port
                         )
                 except urllib.error.HTTPError as e:
-                    raise UploadError("%s: %s" % (self.review, str(e)))
+                    raise UploadError(f"{self.review}: {str(e)}")
                 except urllib.error.URLError as e:
-                    raise UploadError("%s: %s" % (self.review, str(e)))
-                except HTTPException as e:
-                    raise UploadError(
-                        "%s: %s" % (self.review, e.__class__.__name__)
-                    )
+                    raise UploadError(f"{self.review}: {str(e)}")
+                except http.client.HTTPException as e:
+                    raise UploadError(f"{self.review}: {e.__class__.__name__}")
 
                 REVIEW_CACHE[u] = self._review_url
         return self._review_url + self.projectname
@@ -662,7 +664,7 @@ class Remote(object):
         username = self._config.GetString("review.%s.username" % self.review)
         if username is None:
             username = userEmail.split("@")[0]
-        return "ssh://%s@%s:%s/" % (username, host, port)
+        return f"ssh://{username}@{host}:{port}/"
 
     def ToLocal(self, rev):
         """Convert a remote revision string to something we have locally."""
@@ -711,15 +713,15 @@ class Remote(object):
         self._Set("fetch", list(map(str, self.fetch)))
 
     def _Set(self, key, value):
-        key = "remote.%s.%s" % (self.name, key)
+        key = f"remote.{self.name}.{key}"
         return self._config.SetString(key, value)
 
     def _Get(self, key, all_keys=False):
-        key = "remote.%s.%s" % (self.name, key)
+        key = f"remote.{self.name}.{key}"
         return self._config.GetString(key, all_keys=all_keys)
 
 
-class Branch(object):
+class Branch:
     """Configuration options related to a single branch."""
 
     def __init__(self, config, name):
@@ -758,11 +760,11 @@ class Branch(object):
                     fd.write("\tmerge = %s\n" % self.merge)
 
     def _Set(self, key, value):
-        key = "branch.%s.%s" % (self.name, key)
+        key = f"branch.{self.name}.{key}"
         return self._config.SetString(key, value)
 
     def _Get(self, key, all_keys=False):
-        key = "branch.%s.%s" % (self.name, key)
+        key = f"branch.{self.name}.{key}"
         return self._config.GetString(key, all_keys=all_keys)
 
 
@@ -791,8 +793,8 @@ class SyncAnalysisState:
                 to be logged.
         """
         self._config = config
-        now = datetime.datetime.utcnow()
-        self._Set("main.synctime", now.isoformat(timespec="microseconds") + "Z")
+        now = datetime.datetime.now(datetime.timezone.utc)
+        self._Set("main.synctime", now.isoformat(timespec="microseconds"))
         self._Set("main.version", "1")
         self._Set("sys.argv", sys.argv)
         for key, value in superproject_logging_data.items():

git_refs.py

@@ -13,8 +13,10 @@
 # limitations under the License.
 
 import os
-from repo_trace import Trace
+
 import platform_utils
+from repo_trace import Trace
+
 
 HEAD = "HEAD"
 R_CHANGES = "refs/changes/"
@@ -26,7 +28,7 @@ R_WORKTREE_M = R_WORKTREE + "m/"
 R_M = "refs/remotes/m/"
 
 
-class GitRefs(object):
+class GitRefs:
     def __init__(self, gitdir):
         self._gitdir = gitdir
         self._phyref = None
@@ -103,10 +105,8 @@ class GitRefs(object):
     def _ReadPackedRefs(self):
         path = os.path.join(self._gitdir, "packed-refs")
         try:
-            fd = open(path, "r")
+            fd = open(path)
             mtime = os.path.getmtime(path)
-        except IOError:
-            return
         except OSError:
             return
         try:

git_superproject.py

@@ -22,17 +22,19 @@ Examples:
   UpdateProjectsResult = superproject.UpdateProjectsRevisionId(projects)
 """
 
-import hashlib
 import functools
+import hashlib
 import os
 import sys
 import time
 from typing import NamedTuple
 
-from git_command import git_require, GitCommand
+from git_command import git_require
+from git_command import GitCommand
 from git_config import RepoConfig
 from git_refs import GitRefs
 
+
 _SUPERPROJECT_GIT_NAME = "superproject.git"
 _SUPERPROJECT_MANIFEST_NAME = "superproject_override.xml"
 
@@ -64,12 +66,12 @@ class UpdateProjectsResult(NamedTuple):
     fatal: bool
 
 
-class Superproject(object):
+class Superproject:
     """Get commit ids from superproject.
 
-    Initializes a local copy of a superproject for the manifest. This allows
-    lookup of commit ids for all projects. It contains _project_commit_ids which
-    is a dictionary with project/commit id entries.
+    Initializes a bare local copy of a superproject for the manifest. This
+    allows lookup of commit ids for all projects. It contains
+    _project_commit_ids which is a dictionary with project/commit id entries.
     """
 
     def __init__(
@@ -233,7 +235,8 @@ class Superproject(object):
         p = GitCommand(
             None,
             cmd,
-            cwd=self._work_git,
+            gitdir=self._work_git,
+            bare=True,
             capture_stdout=True,
             capture_stderr=True,
         )
@@ -269,7 +272,8 @@ class Superproject(object):
         p = GitCommand(
             None,
             cmd,
-            cwd=self._work_git,
+            gitdir=self._work_git,
+            bare=True,
             capture_stdout=True,
             capture_stderr=True,
         )
@@ -379,7 +383,7 @@ class Superproject(object):
         try:
             with open(manifest_path, "w", encoding="utf-8") as fp:
                 fp.write(manifest_str)
-        except IOError as e:
+        except OSError as e:
             self._LogError("cannot write manifest to : {} {}", manifest_path, e)
             return None
         return manifest_path

git_trace2_event_log.py

@@ -1,46 +1,9 @@
-# Copyright (C) 2020 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-"""Provide event logging in the git trace2 EVENT format.
-
-The git trace2 EVENT format is defined at:
-https://www.kernel.org/pub/software/scm/git/docs/technical/api-trace2.html#_event_format
-https://git-scm.com/docs/api-trace2#_the_event_format_target
-
-  Usage:
-
-  git_trace_log = EventLog()
-  git_trace_log.StartEvent()
-  ...
-  git_trace_log.ExitEvent()
-  git_trace_log.Write()
-"""
+from git_command import GetEventTargetPath
+from git_command import RepoSourceVersion
+from git_trace2_event_log_base import BaseEventLog
 
 
-import datetime
-import errno
-import json
-import os
-import socket
-import sys
-import tempfile
-import threading
-
-from git_command import GitCommand, RepoSourceVersion
-
-
-class EventLog(object):
+class EventLog(BaseEventLog):
     """Event log that records events that occurred during a repo invocation.
 
     Events are written to the log as a consecutive JSON entries, one per line.
@@ -57,318 +20,13 @@ class EventLog(object):
     https://git-scm.com/docs/api-trace2#_event_format
     """
 
-    def __init__(self, env=None):
-        """Initializes the event log."""
-        self._log = []
-        # Try to get session-id (sid) from environment (setup in repo launcher).
-        KEY = "GIT_TRACE2_PARENT_SID"
-        if env is None:
-            env = os.environ
+    def __init__(self, **kwargs):
+        super().__init__(repo_source_version=RepoSourceVersion(), **kwargs)
 
-        self.start = datetime.datetime.utcnow()
-
-        # Save both our sid component and the complete sid.
-        # We use our sid component (self._sid) as the unique filename prefix and
-        # the full sid (self._full_sid) in the log itself.
-        self._sid = "repo-%s-P%08x" % (
-            self.start.strftime("%Y%m%dT%H%M%SZ"),
-            os.getpid(),
-        )
-        parent_sid = env.get(KEY)
-        # Append our sid component to the parent sid (if it exists).
-        if parent_sid is not None:
-            self._full_sid = parent_sid + "/" + self._sid
-        else:
-            self._full_sid = self._sid
-
-        # Set/update the environment variable.
-        # Environment handling across systems is messy.
-        try:
-            env[KEY] = self._full_sid
-        except UnicodeEncodeError:
-            env[KEY] = self._full_sid.encode()
-
-        # Add a version event to front of the log.
-        self._AddVersionEvent()
-
-    @property
-    def full_sid(self):
-        return self._full_sid
-
-    def _AddVersionEvent(self):
-        """Adds a 'version' event at the beginning of current log."""
-        version_event = self._CreateEventDict("version")
-        version_event["evt"] = "2"
-        version_event["exe"] = RepoSourceVersion()
-        self._log.insert(0, version_event)
-
-    def _CreateEventDict(self, event_name):
-        """Returns a dictionary with common keys/values for git trace2 events.
-
-        Args:
-            event_name: The event name.
-
-        Returns:
-            Dictionary with the common event fields populated.
-        """
-        return {
-            "event": event_name,
-            "sid": self._full_sid,
-            "thread": threading.current_thread().name,
-            "time": datetime.datetime.utcnow().isoformat() + "Z",
-        }
-
-    def StartEvent(self):
-        """Append a 'start' event to the current log."""
-        start_event = self._CreateEventDict("start")
-        start_event["argv"] = sys.argv
-        self._log.append(start_event)
-
-    def ExitEvent(self, result):
-        """Append an 'exit' event to the current log.
-
-        Args:
-            result: Exit code of the event
-        """
-        exit_event = self._CreateEventDict("exit")
-
-        # Consider 'None' success (consistent with event_log result handling).
-        if result is None:
-            result = 0
-        exit_event["code"] = result
-        time_delta = datetime.datetime.utcnow() - self.start
-        exit_event["t_abs"] = time_delta.total_seconds()
-        self._log.append(exit_event)
-
-    def CommandEvent(self, name, subcommands):
-        """Append a 'command' event to the current log.
-
-        Args:
-            name: Name of the primary command (ex: repo, git)
-            subcommands: List of the sub-commands (ex: version, init, sync)
-        """
-        command_event = self._CreateEventDict("command")
-        command_event["name"] = name
-        command_event["subcommands"] = subcommands
-        self._log.append(command_event)
-
-    def LogConfigEvents(self, config, event_dict_name):
-        """Append a |event_dict_name| event for each config key in |config|.
-
-        Args:
-            config: Configuration dictionary.
-            event_dict_name: Name of the event dictionary for items to be logged
-                under.
-        """
-        for param, value in config.items():
-            event = self._CreateEventDict(event_dict_name)
-            event["param"] = param
-            event["value"] = value
-            self._log.append(event)
-
-    def DefParamRepoEvents(self, config):
-        """Append 'def_param' events for repo config keys to the current log.
-
-        This appends one event for each repo.* config key.
-
-        Args:
-            config: Repo configuration dictionary
-        """
-        # Only output the repo.* config parameters.
-        repo_config = {k: v for k, v in config.items() if k.startswith("repo.")}
-        self.LogConfigEvents(repo_config, "def_param")
-
-    def GetDataEventName(self, value):
-        """Returns 'data-json' if the value is an array else returns 'data'."""
-        return "data-json" if value[0] == "[" and value[-1] == "]" else "data"
-
-    def LogDataConfigEvents(self, config, prefix):
-        """Append a 'data' event for each entry in |config| to the current log.
-
-        For each keyX and valueX of the config, "key" field of the event is
-        '|prefix|/keyX' and the "value" of the "key" field is valueX.
-
-        Args:
-            config: Configuration dictionary.
-            prefix: Prefix for each key that is logged.
-        """
-        for key, value in config.items():
-            event = self._CreateEventDict(self.GetDataEventName(value))
-            event["key"] = f"{prefix}/{key}"
-            event["value"] = value
-            self._log.append(event)
-
-    def ErrorEvent(self, msg, fmt=None):
-        """Append a 'error' event to the current log."""
-        error_event = self._CreateEventDict("error")
-        if fmt is None:
-            fmt = msg
-        error_event["msg"] = msg
-        error_event["fmt"] = fmt
-        self._log.append(error_event)
-
-    def _GetEventTargetPath(self):
-        """Get the 'trace2.eventtarget' path from git configuration.
-
-        Returns:
-            path: git config's 'trace2.eventtarget' path if it exists, or None
-        """
-        path = None
-        cmd = ["config", "--get", "trace2.eventtarget"]
-        # TODO(https://crbug.com/gerrit/13706): Use GitConfig when it supports
-        # system git config variables.
-        p = GitCommand(
-            None, cmd, capture_stdout=True, capture_stderr=True, bare=True
-        )
-        retval = p.Wait()
-        if retval == 0:
-            # Strip trailing carriage-return in path.
-            path = p.stdout.rstrip("\n")
-        elif retval != 1:
-            # `git config --get` is documented to produce an exit status of `1`
-            # if the requested variable is not present in the configuration.
-            # Report any other return value as an error.
-            print(
-                "repo: error: 'git config --get' call failed with return code: "
-                "%r, stderr: %r" % (retval, p.stderr),
-                file=sys.stderr,
-            )
-        return path
-
-    def _WriteLog(self, write_fn):
-        """Writes the log out using a provided writer function.
-
-        Generate compact JSON output for each item in the log, and write it
-        using write_fn.
-
-        Args:
-            write_fn: A function that accepts byts and writes them to a
-                destination.
-        """
-
-        for e in self._log:
-            # Dump in compact encoding mode.
-            # See 'Compact encoding' in Python docs:
-            # https://docs.python.org/3/library/json.html#module-json
-            write_fn(
-                json.dumps(e, indent=None, separators=(",", ":")).encode(
-                    "utf-8"
-                )
-                + b"\n"
-            )
-
-    def Write(self, path=None):
-        """Writes the log out to a file or socket.
-
-        Log is only written if 'path' or 'git config --get trace2.eventtarget'
-        provide a valid path (or socket) to write logs to.
-
-        Logging filename format follows the git trace2 style of being a unique
-        (exclusive writable) file.
-
-        Args:
-            path: Path to where logs should be written. The path may have a
-                prefix of the form "af_unix:[{stream|dgram}:]", in which case
-                the path is treated as a Unix domain socket. See
-                https://git-scm.com/docs/api-trace2#_enabling_a_target for
-                details.
-
-        Returns:
-            log_path: Path to the log file or socket if log is written,
-                otherwise None
-        """
-        log_path = None
-        # If no logging path is specified, get the path from
-        # 'trace2.eventtarget'.
+    def Write(self, path=None, **kwargs):
         if path is None:
             path = self._GetEventTargetPath()
+        return super().Write(path=path, **kwargs)
 
-        # If no logging path is specified, exit.
-        if path is None:
-            return None
-
-        path_is_socket = False
-        socket_type = None
-        if isinstance(path, str):
-            parts = path.split(":", 1)
-            if parts[0] == "af_unix" and len(parts) == 2:
-                path_is_socket = True
-                path = parts[1]
-                parts = path.split(":", 1)
-                if parts[0] == "stream" and len(parts) == 2:
-                    socket_type = socket.SOCK_STREAM
-                    path = parts[1]
-                elif parts[0] == "dgram" and len(parts) == 2:
-                    socket_type = socket.SOCK_DGRAM
-                    path = parts[1]
-            else:
-                # Get absolute path.
-                path = os.path.abspath(os.path.expanduser(path))
-        else:
-            raise TypeError("path: str required but got %s." % type(path))
-
-        # Git trace2 requires a directory to write log to.
-
-        # TODO(https://crbug.com/gerrit/13706): Support file (append) mode also.
-        if not (path_is_socket or os.path.isdir(path)):
-            return None
-
-        if path_is_socket:
-            if socket_type == socket.SOCK_STREAM or socket_type is None:
-                try:
-                    with socket.socket(
-                        socket.AF_UNIX, socket.SOCK_STREAM
-                    ) as sock:
-                        sock.connect(path)
-                        self._WriteLog(sock.sendall)
-                    return f"af_unix:stream:{path}"
-                except OSError as err:
-                    # If we tried to connect to a DGRAM socket using STREAM,
-                    # ignore the attempt and continue to DGRAM below. Otherwise,
-                    # issue a warning.
-                    if err.errno != errno.EPROTOTYPE:
-                        print(
-                            f"repo: warning: git trace2 logging failed: {err}",
-                            file=sys.stderr,
-                        )
-                        return None
-            if socket_type == socket.SOCK_DGRAM or socket_type is None:
-                try:
-                    with socket.socket(
-                        socket.AF_UNIX, socket.SOCK_DGRAM
-                    ) as sock:
-                        self._WriteLog(lambda bs: sock.sendto(bs, path))
-                        return f"af_unix:dgram:{path}"
-                except OSError as err:
-                    print(
-                        f"repo: warning: git trace2 logging failed: {err}",
-                        file=sys.stderr,
-                    )
-                    return None
-            # Tried to open a socket but couldn't connect (SOCK_STREAM) or write
-            # (SOCK_DGRAM).
-            print(
-                "repo: warning: git trace2 logging failed: could not write to "
-                "socket",
-                file=sys.stderr,
-            )
-            return None
-
-        # Path is an absolute path
-        # Use NamedTemporaryFile to generate a unique filename as required by
-        # git trace2.
-        try:
-            with tempfile.NamedTemporaryFile(
-                mode="xb", prefix=self._sid, dir=path, delete=False
-            ) as f:
-                # TODO(https://crbug.com/gerrit/13706): Support writing events
-                # as they occur.
-                self._WriteLog(f.write)
-                log_path = f.name
-        except FileExistsError as err:
-            print(
-                "repo: warning: git trace2 logging failed: %r" % err,
-                file=sys.stderr,
-            )
-            return None
-        return log_path
+    def _GetEventTargetPath(self):
+        return GetEventTargetPath()

git_trace2_event_log_base.py

@@ -0,0 +1,351 @@
+# Copyright (C) 2020 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Provide event logging in the git trace2 EVENT format.
+
+The git trace2 EVENT format is defined at:
+https://www.kernel.org/pub/software/scm/git/docs/technical/api-trace2.html#_event_format
+https://git-scm.com/docs/api-trace2#_the_event_format_target
+
+  Usage:
+
+  git_trace_log = EventLog()
+  git_trace_log.StartEvent()
+  ...
+  git_trace_log.ExitEvent()
+  git_trace_log.Write()
+"""
+
+
+import datetime
+import errno
+import json
+import os
+import socket
+import sys
+import tempfile
+import threading
+
+
+# BaseEventLog __init__ Counter that is consistent within the same process
+p_init_count = 0
+
+
+class BaseEventLog:
+    """Event log that records events that occurred during a repo invocation.
+
+    Events are written to the log as a consecutive JSON entries, one per line.
+    Entries follow the git trace2 EVENT format.
+
+    Each entry contains the following common keys:
+    - event: The event name
+    - sid: session-id - Unique string to allow process instance to be
+          identified.
+    - thread: The thread name.
+    - time: is the UTC time of the event.
+
+    Valid 'event' names and event specific fields are documented here:
+    https://git-scm.com/docs/api-trace2#_event_format
+    """
+
+    def __init__(
+        self, env=None, repo_source_version=None, add_init_count=False
+    ):
+        """Initializes the event log."""
+        global p_init_count
+        p_init_count += 1
+        self._log = []
+        # Try to get session-id (sid) from environment (setup in repo launcher).
+        KEY = "GIT_TRACE2_PARENT_SID"
+        if env is None:
+            env = os.environ
+
+        self.start = datetime.datetime.now(datetime.timezone.utc)
+
+        # Save both our sid component and the complete sid.
+        # We use our sid component (self._sid) as the unique filename prefix and
+        # the full sid (self._full_sid) in the log itself.
+        self._sid = (
+            f"repo-{self.start.strftime('%Y%m%dT%H%M%SZ')}-P{os.getpid():08x}"
+        )
+
+        if add_init_count:
+            self._sid = f"{self._sid}-{p_init_count}"
+
+        parent_sid = env.get(KEY)
+        # Append our sid component to the parent sid (if it exists).
+        if parent_sid is not None:
+            self._full_sid = parent_sid + "/" + self._sid
+        else:
+            self._full_sid = self._sid
+
+        # Set/update the environment variable.
+        # Environment handling across systems is messy.
+        try:
+            env[KEY] = self._full_sid
+        except UnicodeEncodeError:
+            env[KEY] = self._full_sid.encode()
+
+        if repo_source_version is not None:
+            # Add a version event to front of the log.
+            self._AddVersionEvent(repo_source_version)
+
+    @property
+    def full_sid(self):
+        return self._full_sid
+
+    def _AddVersionEvent(self, repo_source_version):
+        """Adds a 'version' event at the beginning of current log."""
+        version_event = self._CreateEventDict("version")
+        version_event["evt"] = "2"
+        version_event["exe"] = repo_source_version
+        self._log.insert(0, version_event)
+
+    def _CreateEventDict(self, event_name):
+        """Returns a dictionary with common keys/values for git trace2 events.
+
+        Args:
+            event_name: The event name.
+
+        Returns:
+            Dictionary with the common event fields populated.
+        """
+        return {
+            "event": event_name,
+            "sid": self._full_sid,
+            "thread": threading.current_thread().name,
+            "time": datetime.datetime.now(datetime.timezone.utc).isoformat(),
+        }
+
+    def StartEvent(self):
+        """Append a 'start' event to the current log."""
+        start_event = self._CreateEventDict("start")
+        start_event["argv"] = sys.argv
+        self._log.append(start_event)
+
+    def ExitEvent(self, result):
+        """Append an 'exit' event to the current log.
+
+        Args:
+            result: Exit code of the event
+        """
+        exit_event = self._CreateEventDict("exit")
+
+        # Consider 'None' success (consistent with event_log result handling).
+        if result is None:
+            result = 0
+        exit_event["code"] = result
+        time_delta = datetime.datetime.now(datetime.timezone.utc) - self.start
+        exit_event["t_abs"] = time_delta.total_seconds()
+        self._log.append(exit_event)
+
+    def CommandEvent(self, name, subcommands):
+        """Append a 'command' event to the current log.
+
+        Args:
+            name: Name of the primary command (ex: repo, git)
+            subcommands: List of the sub-commands (ex: version, init, sync)
+        """
+        command_event = self._CreateEventDict("command")
+        command_event["name"] = name
+        command_event["subcommands"] = subcommands
+        self._log.append(command_event)
+
+    def LogConfigEvents(self, config, event_dict_name):
+        """Append a |event_dict_name| event for each config key in |config|.
+
+        Args:
+            config: Configuration dictionary.
+            event_dict_name: Name of the event dictionary for items to be logged
+                under.
+        """
+        for param, value in config.items():
+            event = self._CreateEventDict(event_dict_name)
+            event["param"] = param
+            event["value"] = value
+            self._log.append(event)
+
+    def DefParamRepoEvents(self, config):
+        """Append 'def_param' events for repo config keys to the current log.
+
+        This appends one event for each repo.* config key.
+
+        Args:
+            config: Repo configuration dictionary
+        """
+        # Only output the repo.* config parameters.
+        repo_config = {k: v for k, v in config.items() if k.startswith("repo.")}
+        self.LogConfigEvents(repo_config, "def_param")
+
+    def GetDataEventName(self, value):
+        """Returns 'data-json' if the value is an array else returns 'data'."""
+        return "data-json" if value[0] == "[" and value[-1] == "]" else "data"
+
+    def LogDataConfigEvents(self, config, prefix):
+        """Append a 'data' event for each entry in |config| to the current log.
+
+        For each keyX and valueX of the config, "key" field of the event is
+        '|prefix|/keyX' and the "value" of the "key" field is valueX.
+
+        Args:
+            config: Configuration dictionary.
+            prefix: Prefix for each key that is logged.
+        """
+        for key, value in config.items():
+            event = self._CreateEventDict(self.GetDataEventName(value))
+            event["key"] = f"{prefix}/{key}"
+            event["value"] = value
+            self._log.append(event)
+
+    def ErrorEvent(self, msg, fmt=None):
+        """Append a 'error' event to the current log."""
+        error_event = self._CreateEventDict("error")
+        if fmt is None:
+            fmt = msg
+        error_event["msg"] = f"RepoErrorEvent:{msg}"
+        error_event["fmt"] = f"RepoErrorEvent:{fmt}"
+        self._log.append(error_event)
+
+    def _WriteLog(self, write_fn):
+        """Writes the log out using a provided writer function.
+
+        Generate compact JSON output for each item in the log, and write it
+        using write_fn.
+
+        Args:
+            write_fn: A function that accepts byts and writes them to a
+                destination.
+        """
+
+        for e in self._log:
+            # Dump in compact encoding mode.
+            # See 'Compact encoding' in Python docs:
+            # https://docs.python.org/3/library/json.html#module-json
+            write_fn(
+                json.dumps(e, indent=None, separators=(",", ":")).encode(
+                    "utf-8"
+                )
+                + b"\n"
+            )
+
+    def Write(self, path=None):
+        """Writes the log out to a file or socket.
+
+        Log is only written if 'path' or 'git config --get trace2.eventtarget'
+        provide a valid path (or socket) to write logs to.
+
+        Logging filename format follows the git trace2 style of being a unique
+        (exclusive writable) file.
+
+        Args:
+            path: Path to where logs should be written. The path may have a
+                prefix of the form "af_unix:[{stream|dgram}:]", in which case
+                the path is treated as a Unix domain socket. See
+                https://git-scm.com/docs/api-trace2#_enabling_a_target for
+                details.
+
+        Returns:
+            log_path: Path to the log file or socket if log is written,
+                otherwise None
+        """
+        log_path = None
+        # If no logging path is specified, exit.
+        if path is None:
+            return None
+
+        path_is_socket = False
+        socket_type = None
+        if isinstance(path, str):
+            parts = path.split(":", 1)
+            if parts[0] == "af_unix" and len(parts) == 2:
+                path_is_socket = True
+                path = parts[1]
+                parts = path.split(":", 1)
+                if parts[0] == "stream" and len(parts) == 2:
+                    socket_type = socket.SOCK_STREAM
+                    path = parts[1]
+                elif parts[0] == "dgram" and len(parts) == 2:
+                    socket_type = socket.SOCK_DGRAM
+                    path = parts[1]
+            else:
+                # Get absolute path.
+                path = os.path.abspath(os.path.expanduser(path))
+        else:
+            raise TypeError("path: str required but got %s." % type(path))
+
+        # Git trace2 requires a directory to write log to.
+
+        # TODO(https://crbug.com/gerrit/13706): Support file (append) mode also.
+        if not (path_is_socket or os.path.isdir(path)):
+            return None
+
+        if path_is_socket:
+            if socket_type == socket.SOCK_STREAM or socket_type is None:
+                try:
+                    with socket.socket(
+                        socket.AF_UNIX, socket.SOCK_STREAM
+                    ) as sock:
+                        sock.connect(path)
+                        self._WriteLog(sock.sendall)
+                    return f"af_unix:stream:{path}"
+                except OSError as err:
+                    # If we tried to connect to a DGRAM socket using STREAM,
+                    # ignore the attempt and continue to DGRAM below. Otherwise,
+                    # issue a warning.
+                    if err.errno != errno.EPROTOTYPE:
+                        print(
+                            f"repo: warning: git trace2 logging failed: {err}",
+                            file=sys.stderr,
+                        )
+                        return None
+            if socket_type == socket.SOCK_DGRAM or socket_type is None:
+                try:
+                    with socket.socket(
+                        socket.AF_UNIX, socket.SOCK_DGRAM
+                    ) as sock:
+                        self._WriteLog(lambda bs: sock.sendto(bs, path))
+                        return f"af_unix:dgram:{path}"
+                except OSError as err:
+                    print(
+                        f"repo: warning: git trace2 logging failed: {err}",
+                        file=sys.stderr,
+                    )
+                    return None
+            # Tried to open a socket but couldn't connect (SOCK_STREAM) or write
+            # (SOCK_DGRAM).
+            print(
+                "repo: warning: git trace2 logging failed: could not write to "
+                "socket",
+                file=sys.stderr,
+            )
+            return None
+
+        # Path is an absolute path
+        # Use NamedTemporaryFile to generate a unique filename as required by
+        # git trace2.
+        try:
+            with tempfile.NamedTemporaryFile(
+                mode="xb", prefix=self._sid, dir=path, delete=False
+            ) as f:
+                # TODO(https://crbug.com/gerrit/13706): Support writing events
+                # as they occur.
+                self._WriteLog(f.write)
+                log_path = f.name
+        except FileExistsError as err:
+            print(
+                "repo: warning: git trace2 logging failed: %r" % err,
+                file=sys.stderr,
+            )
+            return None
+        return log_path

gitc_utils.py

@@ -1,166 +0,0 @@
-# Copyright (C) 2015 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-import os
-import multiprocessing
-import re
-import sys
-import time
-
-import git_command
-import git_config
-import wrapper
-
-from error import ManifestParseError
-
-NUM_BATCH_RETRIEVE_REVISIONID = 32
-
-
-def get_gitc_manifest_dir():
-    return wrapper.Wrapper().get_gitc_manifest_dir()
-
-
-def parse_clientdir(gitc_fs_path):
-    return wrapper.Wrapper().gitc_parse_clientdir(gitc_fs_path)
-
-
-def _get_project_revision(args):
-    """Worker for _set_project_revisions to lookup one project remote."""
-    (i, url, expr) = args
-    gitcmd = git_command.GitCommand(
-        None, ["ls-remote", url, expr], capture_stdout=True, cwd="/tmp"
-    )
-    rc = gitcmd.Wait()
-    return (i, rc, gitcmd.stdout.split("\t", 1)[0])
-
-
-def _set_project_revisions(projects):
-    """Sets the revisionExpr for a list of projects.
-
-    Because of the limit of open file descriptors allowed, length of projects
-    should not be overly large. Recommend calling this function multiple times
-    with each call not exceeding NUM_BATCH_RETRIEVE_REVISIONID projects.
-
-    Args:
-        projects: List of project objects to set the revionExpr for.
-    """
-    # Retrieve the commit id for each project based off of its current
-    # revisionExpr and it is not already a commit id.
-    with multiprocessing.Pool(NUM_BATCH_RETRIEVE_REVISIONID) as pool:
-        results_iter = pool.imap_unordered(
-            _get_project_revision,
-            (
-                (i, project.remote.url, project.revisionExpr)
-                for i, project in enumerate(projects)
-                if not git_config.IsId(project.revisionExpr)
-            ),
-            chunksize=8,
-        )
-        for i, rc, revisionExpr in results_iter:
-            project = projects[i]
-            if rc:
-                print(
-                    "FATAL: Failed to retrieve revisionExpr for %s"
-                    % project.name
-                )
-                pool.terminate()
-                sys.exit(1)
-            if not revisionExpr:
-                pool.terminate()
-                raise ManifestParseError(
-                    "Invalid SHA-1 revision project %s (%s)"
-                    % (project.remote.url, project.revisionExpr)
-                )
-            project.revisionExpr = revisionExpr
-
-
-def generate_gitc_manifest(gitc_manifest, manifest, paths=None):
-    """Generate a manifest for shafsd to use for this GITC client.
-
-    Args:
-        gitc_manifest: Current gitc manifest, or None if there isn't one yet.
-        manifest: A GitcManifest object loaded with the current repo manifest.
-        paths: List of project paths we want to update.
-    """
-
-    print(
-        "Generating GITC Manifest by fetching revision SHAs for each "
-        "project."
-    )
-    if paths is None:
-        paths = list(manifest.paths.keys())
-
-    groups = [x for x in re.split(r"[,\s]+", manifest.GetGroupsStr()) if x]
-
-    # Convert the paths to projects, and filter them to the matched groups.
-    projects = [manifest.paths[p] for p in paths]
-    projects = [p for p in projects if p.MatchesGroups(groups)]
-
-    if gitc_manifest is not None:
-        for path, proj in manifest.paths.items():
-            if not proj.MatchesGroups(groups):
-                continue
-
-            if not proj.upstream and not git_config.IsId(proj.revisionExpr):
-                proj.upstream = proj.revisionExpr
-
-            if path not in gitc_manifest.paths:
-                # Any new projects need their first revision, even if we weren't
-                # asked for them.
-                projects.append(proj)
-            elif path not in paths:
-                # And copy revisions from the previous manifest if we're not
-                # updating them now.
-                gitc_proj = gitc_manifest.paths[path]
-                if gitc_proj.old_revision:
-                    proj.revisionExpr = None
-                    proj.old_revision = gitc_proj.old_revision
-                else:
-                    proj.revisionExpr = gitc_proj.revisionExpr
-
-    _set_project_revisions(projects)
-
-    if gitc_manifest is not None:
-        for path, proj in gitc_manifest.paths.items():
-            if proj.old_revision and path in paths:
-                # If we updated a project that has been started, keep the
-                # old-revision updated.
-                repo_proj = manifest.paths[path]
-                repo_proj.old_revision = repo_proj.revisionExpr
-                repo_proj.revisionExpr = None
-
-    # Convert URLs from relative to absolute.
-    for _name, remote in manifest.remotes.items():
-        remote.fetchUrl = remote.resolvedFetchUrl
-
-    # Save the manifest.
-    save_manifest(manifest)
-
-
-def save_manifest(manifest, client_dir=None):
-    """Save the manifest file in the client_dir.
-
-    Args:
-        manifest: Manifest object to save.
-        client_dir: Client directory to save the manifest in.
-    """
-    if not client_dir:
-        manifest_file = manifest.manifestFile
-    else:
-        manifest_file = os.path.join(client_dir, ".manifest")
-    with open(manifest_file, "w") as f:
-        manifest.Save(f, groups=manifest.GetGroupsStr())
-    # TODO(sbasi/jorg): Come up with a solution to remove the sleep below.
-    # Give the GITC filesystem time to register the manifest changes.
-    time.sleep(3)

hooks.py

@@ -12,11 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import errno
-import json
 import os
 import re
-import subprocess
 import sys
 import traceback
 import urllib.parse
@@ -25,7 +22,7 @@ from error import HookError
 from git_refs import HEAD
 
 
-class RepoHook(object):
+class RepoHook:
     """A RepoHook contains information about a script to run as a hook.
 
     Hooks are used to run a python script before running an upload (for
@@ -183,7 +180,7 @@ class RepoHook(object):
                 abort_if_user_denies was passed to the consturctor.
         """
         hooks_config = self._hooks_project.config
-        git_approval_key = "repo.hooks.%s.%s" % (self._hook_type, subkey)
+        git_approval_key = f"repo.hooks.{self._hook_type}.{subkey}"
 
         # Get the last value that the user approved for this hook; may be None.
         old_val = hooks_config.GetString(git_approval_key)
@@ -196,7 +193,7 @@ class RepoHook(object):
             else:
                 # Give the user a reason why we're prompting, since they last
                 # told us to "never ask again".
-                prompt = "WARNING: %s\n\n" % (changed_prompt,)
+                prompt = f"WARNING: {changed_prompt}\n\n"
         else:
             prompt = ""
 
@@ -244,9 +241,8 @@ class RepoHook(object):
         return self._CheckForHookApprovalHelper(
             "approvedmanifest",
             self._manifest_url,
-            "Run hook scripts from %s" % (self._manifest_url,),
-            "Manifest URL has changed since %s was allowed."
-            % (self._hook_type,),
+            f"Run hook scripts from {self._manifest_url}",
+            f"Manifest URL has changed since {self._hook_type} was allowed.",
         )
 
     def _CheckForHookApprovalHash(self):
@@ -265,7 +261,7 @@ class RepoHook(object):
             "approvedhash",
             self._GetHash(),
             prompt % (self._GetMustVerb(), self._script_fullpath),
-            "Scripts have changed since %s was allowed." % (self._hook_type,),
+            f"Scripts have changed since {self._hook_type} was allowed.",
         )
 
     @staticmethod
@@ -298,43 +294,6 @@ class RepoHook(object):
 
         return interp
 
-    def _ExecuteHookViaReexec(self, interp, context, **kwargs):
-        """Execute the hook script through |interp|.
-
-        Note: Support for this feature should be dropped ~Jun 2021.
-
-        Args:
-            interp: The Python program to run.
-            context: Basic Python context to execute the hook inside.
-            kwargs: Arbitrary arguments to pass to the hook script.
-
-        Raises:
-            HookError: When the hooks failed for any reason.
-        """
-        # This logic needs to be kept in sync with _ExecuteHookViaImport below.
-        script = """
-import json, os, sys
-path = '''%(path)s'''
-kwargs = json.loads('''%(kwargs)s''')
-context = json.loads('''%(context)s''')
-sys.path.insert(0, os.path.dirname(path))
-data = open(path).read()
-exec(compile(data, path, 'exec'), context)
-context['main'](**kwargs)
-""" % {
-            "path": self._script_fullpath,
-            "kwargs": json.dumps(kwargs),
-            "context": json.dumps(context),
-        }
-
-        # We pass the script via stdin to avoid OS argv limits.  It also makes
-        # unhandled exception tracebacks less verbose/confusing for users.
-        cmd = [interp, "-c", "import sys; exec(sys.stdin.read())"]
-        proc = subprocess.Popen(cmd, stdin=subprocess.PIPE)
-        proc.communicate(input=script.encode("utf-8"))
-        if proc.returncode:
-            raise HookError("Failed to run %s hook." % (self._hook_type,))
-
     def _ExecuteHookViaImport(self, data, context, **kwargs):
         """Execute the hook code in |data| directly.
 
@@ -412,30 +371,13 @@ context['main'](**kwargs)
             # See what version of python the hook has been written against.
             data = open(self._script_fullpath).read()
             interp = self._ExtractInterpFromShebang(data)
-            reexec = False
             if interp:
                 prog = os.path.basename(interp)
-                if prog.startswith("python2") and sys.version_info.major != 2:
-                    reexec = True
-                elif prog.startswith("python3") and sys.version_info.major == 2:
-                    reexec = True
-
-            # Attempt to execute the hooks through the requested version of
-            # Python.
-            if reexec:
-                try:
-                    self._ExecuteHookViaReexec(interp, context, **kwargs)
-                except OSError as e:
-                    if e.errno == errno.ENOENT:
-                        # We couldn't find the interpreter, so fallback to
-                        # importing.
-                        reexec = False
-                    else:
-                        raise
+                if prog.startswith("python2"):
+                    raise HookError("Python 2 is not supported")
 
             # Run the hook by importing directly.
-            if not reexec:
-                self._ExecuteHookViaImport(data, context, **kwargs)
+            self._ExecuteHookViaImport(data, context, **kwargs)
         finally:
             # Restore sys.path and CWD.
             sys.path = orig_syspath

hooks/commit-msg

@@ -1,5 +1,5 @@
 #!/bin/sh
-# From Gerrit Code Review 3.6.1 c67916dbdc07555c44e32a68f92ffc484b9b34f0
+# From Gerrit Code Review 3.10.0 d5403dbf335ba7d48977fc95170c3f7027c34659
 #
 # Part of Gerrit Code Review (https://www.gerritcodereview.com/)
 #
@@ -31,14 +31,21 @@ if test ! -f "$1" ; then
 fi
 
 # Do not create a change id if requested
-if test "false" = "$(git config --bool --get gerrit.createChangeId)" ; then
-  exit 0
-fi
+create_setting=$(git config --get gerrit.createChangeId)
+case "$create_setting" in
+  false)
+    exit 0
+    ;;
+  always)
+    ;;
+  *)
+    # Do not create a change id for squash/fixup commits.
+    if head -n1 "$1" | LC_ALL=C grep -q '^[a-z][a-z]*! '; then
+      exit 0
+    fi
+    ;;
+esac
 
-# Do not create a change id for squash commits.
-if head -n1 "$1" | grep -q '^squash! '; then
-  exit 0
-fi
 
 if git rev-parse --verify HEAD >/dev/null 2>&1; then
   refhash="$(git rev-parse HEAD)"
@@ -51,7 +58,7 @@ dest="$1.tmp.${random}"
 
 trap 'rm -f "$dest" "$dest-2"' EXIT
 
-if ! git stripspace --strip-comments < "$1" > "${dest}" ; then
+if ! cat "$1" | sed -e '/>8/q' | git stripspace --strip-comments > "${dest}" ; then
    echo "cannot strip comments from $1"
    exit 1
 fi
@@ -65,7 +72,7 @@ reviewurl="$(git config --get gerrit.reviewUrl)"
 if test -n "${reviewurl}" ; then
   token="Link"
   value="${reviewurl%/}/id/I$random"
-  pattern=".*/id/I[0-9a-f]\{40\}$"
+  pattern=".*/id/I[0-9a-f]\{40\}"
 else
   token="Change-Id"
   value="I$random"
@@ -92,7 +99,7 @@ fi
 # Avoid the --where option which only appeared in Git 2.15
 if ! git -c trailer.where=before interpret-trailers \
          --trailer "Signed-off-by: $token: $value" < "$dest-2" |
-     sed -re "s/^Signed-off-by: ($token: )/\1/" \
+     sed -e "s/^Signed-off-by: \($token: \)/\1/" \
          -e "/^Signed-off-by: SENTINEL/d" > "$dest" ; then
   echo "cannot insert $token line in $1"
   exit 1

main.py

@@ -21,6 +21,7 @@ which takes care of execing this entry point.
 """
 
 import getpass
+import json
 import netrc
 import optparse
 import os
@@ -31,35 +32,47 @@ import textwrap
 import time
 import urllib.request
 
+from repo_logging import RepoLogger
+
+
 try:
     import kerberos
 except ImportError:
     kerberos = None
 
 from color import SetDefaultColoring
-import event_log
-from repo_trace import SetTrace, Trace, SetTraceToStderr
-from git_command import user_agent
-from git_config import RepoConfig
-from git_trace2_event_log import EventLog
 from command import InteractiveCommand
 from command import MirrorSafeCommand
-from command import GitcAvailableCommand, GitcClientCommand
-from subcmds.version import Version
 from editor import Editor
 from error import DownloadError
+from error import GitcUnsupportedError
 from error import InvalidProjectGroupsError
 from error import ManifestInvalidRevisionError
 from error import ManifestParseError
 from error import NoManifestException
 from error import NoSuchProjectError
 from error import RepoChangedException
-import gitc_utils
-from manifest_xml import GitcClient, RepoClient
-from pager import RunPager, TerminatePager
-from wrapper import WrapperPath, Wrapper
-
+from error import RepoError
+from error import RepoExitError
+from error import RepoUnhandledExceptionError
+from error import SilentRepoExitError
+import event_log
+from git_command import user_agent
+from git_config import RepoConfig
+from git_trace2_event_log import EventLog
+from manifest_xml import RepoClient
+from pager import RunPager
+from pager import TerminatePager
+from repo_trace import SetTrace
+from repo_trace import SetTraceToStderr
+from repo_trace import Trace
 from subcmds import all_commands
+from subcmds.version import Version
+from wrapper import Wrapper
+from wrapper import WrapperPath
+
+
+logger = RepoLogger(__file__)
 
 
 # NB: These do not need to be kept in sync with the repo launcher script.
@@ -74,29 +87,22 @@ from subcmds import all_commands
 MIN_PYTHON_VERSION_SOFT = (3, 6)
 MIN_PYTHON_VERSION_HARD = (3, 6)
 
-if sys.version_info.major < 3:
-    print(
-        "repo: error: Python 2 is no longer supported; "
-        "Please upgrade to Python {}.{}+.".format(*MIN_PYTHON_VERSION_SOFT),
-        file=sys.stderr,
+if sys.version_info < MIN_PYTHON_VERSION_HARD:
+    logger.error(
+        "repo: error: Python version is too old; "
+        "Please upgrade to Python %d.%d+.",
+        *MIN_PYTHON_VERSION_SOFT,
     )
     sys.exit(1)
-else:
-    if sys.version_info < MIN_PYTHON_VERSION_HARD:
-        print(
-            "repo: error: Python 3 version is too old; "
-            "Please upgrade to Python {}.{}+.".format(*MIN_PYTHON_VERSION_SOFT),
-            file=sys.stderr,
-        )
-        sys.exit(1)
-    elif sys.version_info < MIN_PYTHON_VERSION_SOFT:
-        print(
-            "repo: warning: your Python 3 version is no longer supported; "
-            "Please upgrade to Python {}.{}+.".format(*MIN_PYTHON_VERSION_SOFT),
-            file=sys.stderr,
-        )
+elif sys.version_info < MIN_PYTHON_VERSION_SOFT:
+    logger.error(
+        "repo: warning: your Python version is no longer supported; "
+        "Please upgrade to Python %d.%d+.",
+        *MIN_PYTHON_VERSION_SOFT,
+    )
 
 KEYBOARD_INTERRUPT_EXIT = 128 + signal.SIGINT
+MAX_PRINT_ERRORS = 5
 
 global_options = optparse.OptionParser(
     usage="repo [-p|--paginate|--no-pager] COMMAND [ARGS]",
@@ -181,7 +187,7 @@ global_options.add_option(
 )
 
 
-class _Repo(object):
+class _Repo:
     def __init__(self, repodir):
         self.repodir = repodir
         self.commands = all_commands
@@ -193,9 +199,8 @@ class _Repo(object):
         if short:
             commands = " ".join(sorted(self.commands))
             wrapped_commands = textwrap.wrap(commands, width=77)
-            print(
-                "Available commands:\n  %s" % ("\n  ".join(wrapped_commands),)
-            )
+            help_commands = "".join(f"\n  {x}" for x in wrapped_commands)
+            print(f"Available commands:{help_commands}")
             print("\nRun `repo help <command>` for command-specific details.")
             print("Bug reports:", Wrapper().BUG_URL)
         else:
@@ -231,7 +236,7 @@ class _Repo(object):
         if name in self.commands:
             return name, []
 
-        key = "alias.%s" % (name,)
+        key = f"alias.{name}"
         alias = RepoConfig.ForRepository(self.repodir).GetString(key)
         if alias is None:
             alias = RepoConfig.ForUser().GetString(key)
@@ -299,11 +304,10 @@ class _Repo(object):
                 submanifest_path=gopts.submanifest_path,
                 outer_client=outer_client,
             )
-        gitc_manifest = None
-        gitc_client_name = gitc_utils.parse_clientdir(os.getcwd())
-        if gitc_client_name:
-            gitc_manifest = GitcClient(self.repodir, gitc_client_name)
-            repo_client.isGitcClient = True
+
+        if Wrapper().gitc_parse_clientdir(os.getcwd()):
+            logger.error("GITC is not supported.")
+            raise GitcUnsupportedError()
 
         try:
             cmd = self.commands[name](
@@ -312,50 +316,27 @@ class _Repo(object):
                 manifest=repo_client.manifest,
                 outer_client=outer_client,
                 outer_manifest=outer_client.manifest,
-                gitc_manifest=gitc_manifest,
                 git_event_log=git_trace2_event_log,
             )
         except KeyError:
-            print(
-                "repo: '%s' is not a repo command.  See 'repo help'." % name,
-                file=sys.stderr,
+            logger.error(
+                "repo: '%s' is not a repo command.  See 'repo help'.", name
             )
             return 1
 
         Editor.globalConfig = cmd.client.globalConfig
 
         if not isinstance(cmd, MirrorSafeCommand) and cmd.manifest.IsMirror:
-            print(
-                "fatal: '%s' requires a working directory" % name,
-                file=sys.stderr,
-            )
-            return 1
-
-        if (
-            isinstance(cmd, GitcAvailableCommand)
-            and not gitc_utils.get_gitc_manifest_dir()
-        ):
-            print(
-                "fatal: '%s' requires GITC to be available" % name,
-                file=sys.stderr,
-            )
-            return 1
-
-        if isinstance(cmd, GitcClientCommand) and not gitc_client_name:
-            print("fatal: '%s' requires a GITC client" % name, file=sys.stderr)
+            logger.error("fatal: '%s' requires a working directory", name)
             return 1
 
         try:
             copts, cargs = cmd.OptionParser.parse_args(argv)
             copts = cmd.ReadEnvironmentOptions(copts)
         except NoManifestException as e:
-            print(
-                "error: in `%s`: %s" % (" ".join([name] + argv), str(e)),
-                file=sys.stderr,
-            )
-            print(
-                "error: manifest missing or unreadable -- please run init",
-                file=sys.stderr,
+            logger.error("error: in `%s`: %s", " ".join([name] + argv), e)
+            logger.error(
+                "error: manifest missing or unreadable -- please run init"
             )
             return 1
 
@@ -422,12 +403,36 @@ class _Repo(object):
             """
             try:
                 execute_command_helper()
-            except (KeyboardInterrupt, SystemExit, Exception) as e:
+            except (
+                KeyboardInterrupt,
+                SystemExit,
+                Exception,
+                RepoExitError,
+            ) as e:
                 ok = isinstance(e, SystemExit) and not e.code
+                exception_name = type(e).__name__
+                if isinstance(e, RepoUnhandledExceptionError):
+                    exception_name = type(e.error).__name__
+                if isinstance(e, RepoExitError):
+                    aggregated_errors = e.aggregate_errors or []
+                    for error in aggregated_errors:
+                        project = None
+                        if isinstance(error, RepoError):
+                            project = error.project
+                        error_info = json.dumps(
+                            {
+                                "ErrorType": type(error).__name__,
+                                "Project": project,
+                                "Message": str(error),
+                            }
+                        )
+                        git_trace2_event_log.ErrorEvent(
+                            f"AggregateExitError:{error_info}"
+                        )
                 if not ok:
-                    exception_name = type(e).__name__
                     git_trace2_event_log.ErrorEvent(
-                        f"RepoExitError:{exception_name}")
+                        f"RepoExitError:{exception_name}"
+                    )
                 raise
 
         try:
@@ -435,38 +440,33 @@ class _Repo(object):
         except (
             DownloadError,
             ManifestInvalidRevisionError,
+            ManifestParseError,
             NoManifestException,
         ) as e:
-            print(
-                "error: in `%s`: %s" % (" ".join([name] + argv), str(e)),
-                file=sys.stderr,
-            )
+            logger.error("error: in `%s`: %s", " ".join([name] + argv), e)
             if isinstance(e, NoManifestException):
-                print(
-                    "error: manifest missing or unreadable -- please run init",
-                    file=sys.stderr,
+                logger.error(
+                    "error: manifest missing or unreadable -- please run init"
                 )
-            result = 1
+            result = e.exit_code
         except NoSuchProjectError as e:
             if e.name:
-                print("error: project %s not found" % e.name, file=sys.stderr)
+                logger.error("error: project %s not found", e.name)
             else:
-                print("error: no project in current directory", file=sys.stderr)
-            result = 1
+                logger.error("error: no project in current directory")
+            result = e.exit_code
         except InvalidProjectGroupsError as e:
             if e.name:
-                print(
-                    "error: project group must be enabled for project %s"
-                    % e.name,
-                    file=sys.stderr,
+                logger.error(
+                    "error: project group must be enabled for project %s",
+                    e.name,
                 )
             else:
-                print(
+                logger.error(
                     "error: project group must be enabled for the project in "
-                    "the current directory",
-                    file=sys.stderr,
+                    "the current directory"
                 )
-            result = 1
+            result = e.exit_code
         except SystemExit as e:
             if e.code:
                 result = e.code
@@ -474,6 +474,9 @@ class _Repo(object):
         except KeyboardInterrupt:
             result = KEYBOARD_INTERRUPT_EXIT
             raise
+        except RepoExitError as e:
+            result = e.exit_code
+            raise
         except Exception:
             result = 1
             raise
@@ -528,7 +531,7 @@ def _CheckWrapperVersion(ver_str, repo_path):
         repo_path = "~/bin/repo"
 
     if not ver_str:
-        print("no --wrapper-version argument", file=sys.stderr)
+        logger.error("no --wrapper-version argument")
         sys.exit(1)
 
     # Pull out the version of the repo launcher we know about to compare.
@@ -537,7 +540,7 @@ def _CheckWrapperVersion(ver_str, repo_path):
 
     exp_str = ".".join(map(str, exp))
     if ver < MIN_REPO_VERSION:
-        print(
+        logger.error(
             """
 repo: error:
 !!! Your version of repo %s is too old.
@@ -546,42 +549,44 @@ repo: error:
 !!! You must upgrade before you can continue:
 
     cp %s %s
-"""
-            % (ver_str, min_str, exp_str, WrapperPath(), repo_path),
-            file=sys.stderr,
+""",
+            ver_str,
+            min_str,
+            exp_str,
+            WrapperPath(),
+            repo_path,
         )
         sys.exit(1)
 
     if exp > ver:
-        print(
-            "\n... A new version of repo (%s) is available." % (exp_str,),
-            file=sys.stderr,
+        logger.warning(
+            "\n... A new version of repo (%s) is available.", exp_str
         )
         if os.access(repo_path, os.W_OK):
-            print(
+            logger.warning(
                 """\
 ... You should upgrade soon:
     cp %s %s
-"""
-                % (WrapperPath(), repo_path),
-                file=sys.stderr,
+""",
+                WrapperPath(),
+                repo_path,
             )
         else:
-            print(
+            logger.warning(
                 """\
 ... New version is available at: %s
 ... The launcher is run from: %s
 !!! The launcher is not writable.  Please talk to your sysadmin or distro
 !!! to get an update installed.
-"""
-                % (WrapperPath(), repo_path),
-                file=sys.stderr,
+""",
+                WrapperPath(),
+                repo_path,
             )
 
 
 def _CheckRepoDir(repo_dir):
     if not repo_dir:
-        print("no --repo-dir argument", file=sys.stderr)
+        logger.error("no --repo-dir argument")
         sys.exit(1)
 
 
@@ -785,7 +790,7 @@ def init_http():
             mgr.add_password(p[1], "https://%s/" % host, p[0], p[2])
     except netrc.NetrcParseError:
         pass
-    except IOError:
+    except OSError:
         pass
     handlers.append(_BasicAuthHandler(mgr))
     handlers.append(_DigestAuthHandler(mgr))
@@ -840,19 +845,19 @@ def _Main(argv):
             SetTraceToStderr()
 
         result = repo._Run(name, gopts, argv) or 0
+    except RepoExitError as e:
+        if not isinstance(e, SilentRepoExitError):
+            logger.log_aggregated_errors(e)
+        result = e.exit_code
     except KeyboardInterrupt:
         print("aborted by user", file=sys.stderr)
         result = KEYBOARD_INTERRUPT_EXIT
-    except ManifestParseError as mpe:
-        print("fatal: %s" % mpe, file=sys.stderr)
-        result = 1
     except RepoChangedException as rce:
         # If repo changed, re-exec ourselves.
-        #
         argv = list(sys.argv)
         argv.extend(rce.extra_args)
         try:
-            os.execv(sys.executable, [__file__] + argv)
+            os.execv(sys.executable, [sys.executable, __file__] + argv)
         except OSError as e:
             print("fatal: cannot restart repo after upgrade", file=sys.stderr)
             print("fatal: %s" % e, file=sys.stderr)

manifest_xml.py

@@ -18,28 +18,25 @@ import os
 import platform
 import re
 import sys
-import xml.dom.minidom
 import urllib.parse
+import xml.dom.minidom
 
-import gitc_utils
+from error import ManifestInvalidPathError
+from error import ManifestInvalidRevisionError
+from error import ManifestParseError
 from git_config import GitConfig
-from git_refs import R_HEADS, HEAD
+from git_refs import HEAD
+from git_refs import R_HEADS
 from git_superproject import Superproject
 import platform_utils
-from project import (
-    Annotation,
-    RemoteSpec,
-    Project,
-    RepoProject,
-    ManifestProject,
-)
-from error import (
-    ManifestParseError,
-    ManifestInvalidPathError,
-    ManifestInvalidRevisionError,
-)
+from project import Annotation
+from project import ManifestProject
+from project import Project
+from project import RemoteSpec
+from project import RepoProject
 from wrapper import Wrapper
 
+
 MANIFEST_FILE_NAME = "manifest.xml"
 LOCAL_MANIFEST_NAME = "local_manifest.xml"
 LOCAL_MANIFESTS_DIR_NAME = "local_manifests"
@@ -117,12 +114,40 @@ def XmlInt(node, attr, default=None):
     try:
         return int(value)
     except ValueError:
-        raise ManifestParseError(
-            'manifest: invalid %s="%s" integer' % (attr, value)
-        )
+        raise ManifestParseError(f'manifest: invalid {attr}="{value}" integer')
 
 
-class _Default(object):
+def normalize_url(url: str) -> str:
+    """Mutate input 'url' into normalized form:
+
+    * remove trailing slashes
+    * convert SCP-like syntax to SSH URL
+
+    Args:
+        url: URL to modify
+
+    Returns:
+        The normalized URL.
+    """
+
+    url = url.rstrip("/")
+    parsed_url = urllib.parse.urlparse(url)
+
+    # This matches patterns like "git@github.com:foo/bar".
+    scp_like_url_re = r"^[^:]+@[^:]+:[^/]+/"
+
+    # If our URL is missing a schema and matches git's
+    # SCP-like syntax we should convert it to a proper
+    # SSH URL instead to make urljoin() happier.
+    #
+    # See: https://git-scm.com/docs/git-clone#URLS
+    if not parsed_url.scheme and re.match(scp_like_url_re, url):
+        return "ssh://" + url.replace(":", "/", 1)
+
+    return url
+
+
+class _Default:
     """Project defaults within the manifest."""
 
     revisionExpr = None
@@ -145,7 +170,7 @@ class _Default(object):
         return self.__dict__ != other.__dict__
 
 
-class _XmlRemote(object):
+class _XmlRemote:
     def __init__(
         self,
         name,
@@ -185,20 +210,22 @@ class _XmlRemote(object):
     def _resolveFetchUrl(self):
         if self.fetchUrl is None:
             return ""
-        url = self.fetchUrl.rstrip("/")
-        manifestUrl = self.manifestUrl.rstrip("/")
-        # urljoin will gets confused over quite a few things.  The ones we care
-        # about here are:
-        # * no scheme in the base url, like <hostname:port>
-        # We handle no scheme by replacing it with an obscure protocol, gopher
-        # and then replacing it with the original when we are done.
 
-        if manifestUrl.find(":") != manifestUrl.find("/") - 1:
-            url = urllib.parse.urljoin("gopher://" + manifestUrl, url)
-            url = re.sub(r"^gopher://", "", url)
+        fetch_url = normalize_url(self.fetchUrl)
+        manifest_url = normalize_url(self.manifestUrl)
+
+        # urljoin doesn't like URLs with no scheme in the base URL
+        # such as file paths.  We handle this by prefixing it with
+        # an obscure protocol, gopher, and replacing it with the
+        # original after urljoin
+        if manifest_url.find(":") != manifest_url.find("/") - 1:
+            fetch_url = urllib.parse.urljoin(
+                "gopher://" + manifest_url, fetch_url
+            )
+            fetch_url = re.sub(r"^gopher://", "", fetch_url)
         else:
-            url = urllib.parse.urljoin(manifestUrl, url)
-        return url
+            fetch_url = urllib.parse.urljoin(manifest_url, fetch_url)
+        return fetch_url
 
     def ToRemoteSpec(self, projectName):
         fetchUrl = self.resolvedFetchUrl.rstrip("/")
@@ -278,7 +305,7 @@ class _XmlSubmanifest:
             parent.repodir,
             linkFile,
             parent_groups=",".join(groups) or "",
-            submanifest_path=self.relpath,
+            submanifest_path=os.path.join(parent.path_prefix, self.relpath),
             outer_client=outer_client,
             default_groups=default_groups,
         )
@@ -357,7 +384,7 @@ class SubmanifestSpec:
         self.groups = groups or []
 
 
-class XmlManifest(object):
+class XmlManifest:
     """manages the repo configuration file"""
 
     def __init__(
@@ -730,10 +757,10 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
             self._output_manifest_project_extras(p, e)
 
             if p.subprojects:
-                subprojects = set(subp.name for subp in p.subprojects)
+                subprojects = {subp.name for subp in p.subprojects}
                 output_projects(p, e, list(sorted(subprojects)))
 
-        projects = set(p.name for p in self._paths.values() if not p.parent)
+        projects = {p.name for p in self._paths.values() if not p.parent}
         output_projects(None, root, list(sorted(projects)))
 
         if self._repo_hooks_project:
@@ -803,17 +830,17 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
             for child in node.childNodes:
                 if child.nodeType == xml.dom.Node.ELEMENT_NODE:
                     attrs = child.attributes
-                    element = dict(
-                        (attrs.item(i).localName, attrs.item(i).value)
+                    element = {
+                        attrs.item(i).localName: attrs.item(i).value
                         for i in range(attrs.length)
-                    )
+                    }
                     if child.nodeName in SINGLE_ELEMENTS:
                         ret[child.nodeName] = element
                     elif child.nodeName in MULTI_ELEMENTS:
                         ret.setdefault(child.nodeName, []).append(element)
                     else:
                         raise ManifestParseError(
-                            'Unhandled element "%s"' % (child.nodeName,)
+                            f'Unhandled element "{child.nodeName}"'
                         )
 
                     append_children(element, child)
@@ -860,8 +887,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         self._Load()
         outer = self._outer_client
         yield outer
-        for tree in outer.all_children:
-            yield tree
+        yield from outer.all_children
 
     @property
     def all_children(self):
@@ -870,8 +896,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         for child in self._submanifests.values():
             if child.repo_client:
                 yield child.repo_client
-                for tree in child.repo_client.all_children:
-                    yield tree
+                yield from child.repo_client.all_children
 
     @property
     def path_prefix(self):
@@ -990,7 +1015,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     @property
     def PartialCloneExclude(self):
         exclude = self.manifest.manifestProject.partial_clone_exclude or ""
-        return set(x.strip() for x in exclude.split(","))
+        return {x.strip() for x in exclude.split(",")}
 
     def SetManifestOverride(self, path):
         """Override manifestFile.  The caller must call Unload()"""
@@ -1263,18 +1288,19 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         try:
             root = xml.dom.minidom.parse(path)
         except (OSError, xml.parsers.expat.ExpatError) as e:
-            raise ManifestParseError(
-                "error parsing manifest %s: %s" % (path, e)
-            )
+            raise ManifestParseError(f"error parsing manifest {path}: {e}")
 
         if not root or not root.childNodes:
-            raise ManifestParseError("no root node in %s" % (path,))
+            raise ManifestParseError(f"no root node in {path}")
 
         for manifest in root.childNodes:
-            if manifest.nodeName == "manifest":
+            if (
+                manifest.nodeType == manifest.ELEMENT_NODE
+                and manifest.nodeName == "manifest"
+            ):
                 break
         else:
-            raise ManifestParseError("no <manifest> in %s" % (path,))
+            raise ManifestParseError(f"no <manifest> in {path}")
 
         nodes = []
         for node in manifest.childNodes:
@@ -1284,7 +1310,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                     msg = self._CheckLocalPath(name)
                     if msg:
                         raise ManifestInvalidPathError(
-                            '<include> invalid "name": %s: %s' % (name, msg)
+                            f'<include> invalid "name": {name}: {msg}'
                         )
                 include_groups = ""
                 if parent_groups:
@@ -1316,7 +1342,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                     raise
                 except Exception as e:
                     raise ManifestParseError(
-                        "failed parsing included manifest %s: %s" % (name, e)
+                        f"failed parsing included manifest {name}: {e}"
                     )
             else:
                 if parent_groups and node.nodeName == "project":
@@ -1767,13 +1793,13 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 msg = self._CheckLocalPath(name)
                 if msg:
                     raise ManifestInvalidPathError(
-                        '<submanifest> invalid "name": %s: %s' % (name, msg)
+                        f'<submanifest> invalid "name": {name}: {msg}'
                     )
         else:
             msg = self._CheckLocalPath(path)
             if msg:
                 raise ManifestInvalidPathError(
-                    '<submanifest> invalid "path": %s: %s' % (path, msg)
+                    f'<submanifest> invalid "path": {path}: {msg}'
                 )
 
         submanifest = _XmlSubmanifest(
@@ -1808,7 +1834,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         msg = self._CheckLocalPath(name, dir_ok=True)
         if msg:
             raise ManifestInvalidPathError(
-                '<project> invalid "name": %s: %s' % (name, msg)
+                f'<project> invalid "name": {name}: {msg}'
             )
         if parent:
             name = self._JoinName(parent.name, name)
@@ -1818,7 +1844,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
             remote = self._default.remote
         if remote is None:
             raise ManifestParseError(
-                "no remote for project %s within %s" % (name, self.manifestFile)
+                f"no remote for project {name} within {self.manifestFile}"
             )
 
         revisionExpr = node.getAttribute("revision") or remote.revision
@@ -1839,7 +1865,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
             msg = self._CheckLocalPath(path, dir_ok=True, cwd_dot_ok=True)
             if msg:
                 raise ManifestInvalidPathError(
-                    '<project> invalid "path": %s: %s' % (path, msg)
+                    f'<project> invalid "path": {path}: {msg}'
                 )
 
         rebase = XmlBool(node, "rebase", True)
@@ -2096,7 +2122,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         if not cwd_dot_ok or parts != ["."]:
             for part in set(parts):
                 if part in {".", "..", ".git"} or part.startswith(".repo"):
-                    return "bad component: %s" % (part,)
+                    return f"bad component: {part}"
 
         if not dir_ok and resep.match(path[-1]):
             return "dirs not allowed"
@@ -2132,7 +2158,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         msg = cls._CheckLocalPath(dest)
         if msg:
             raise ManifestInvalidPathError(
-                '<%s> invalid "dest": %s: %s' % (element, dest, msg)
+                f'<{element}> invalid "dest": {dest}: {msg}'
             )
 
         # |src| is the file we read from or path we point to for symlinks.
@@ -2143,7 +2169,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         )
         if msg:
             raise ManifestInvalidPathError(
-                '<%s> invalid "src": %s: %s' % (element, src, msg)
+                f'<{element}> invalid "src": {src}: {msg}'
             )
 
     def _ParseCopyFile(self, project, node):
@@ -2187,7 +2213,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         v = self._remotes.get(name)
         if not v:
             raise ManifestParseError(
-                "remote %s not defined in %s" % (name, self.manifestFile)
+                f"remote {name} not defined in {self.manifestFile}"
             )
         return v
 
@@ -2213,7 +2239,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         toProjects = manifest.paths
 
         fromKeys = sorted(fromProjects.keys())
-        toKeys = sorted(toProjects.keys())
+        toKeys = set(toProjects.keys())
 
         diff = {
             "added": [],
@@ -2224,13 +2250,13 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         }
 
         for proj in fromKeys:
+            fromProj = fromProjects[proj]
             if proj not in toKeys:
-                diff["removed"].append(fromProjects[proj])
-            elif not fromProjects[proj].Exists:
+                diff["removed"].append(fromProj)
+            elif not fromProj.Exists:
                 diff["missing"].append(toProjects[proj])
                 toKeys.remove(proj)
             else:
-                fromProj = fromProjects[proj]
                 toProj = toProjects[proj]
                 try:
                     fromRevId = fromProj.GetCommitRevisionId()
@@ -2242,27 +2268,11 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                         diff["changed"].append((fromProj, toProj))
                 toKeys.remove(proj)
 
-        for proj in toKeys:
-            diff["added"].append(toProjects[proj])
+        diff["added"].extend(toProjects[proj] for proj in sorted(toKeys))
 
         return diff
 
 
-class GitcManifest(XmlManifest):
-    """Parser for GitC (git-in-the-cloud) manifests."""
-
-    def _ParseProject(self, node, parent=None):
-        """Override _ParseProject and add support for GITC specific attributes."""  # noqa: E501
-        return super()._ParseProject(
-            node, parent=parent, old_revision=node.getAttribute("old-revision")
-        )
-
-    def _output_manifest_project_extras(self, p, e):
-        """Output GITC Specific Project attributes"""
-        if p.old_revision:
-            e.setAttribute("old-revision", str(p.old_revision))
-
-
 class RepoClient(XmlManifest):
     """Manages a repo client checkout."""
 
@@ -2315,19 +2325,3 @@ class RepoClient(XmlManifest):
 
         # TODO: Completely separate manifest logic out of the client.
         self.manifest = self
-
-
-class GitcClient(RepoClient, GitcManifest):
-    """Manages a GitC client checkout."""
-
-    def __init__(self, repodir, gitc_client_name):
-        """Initialize the GitcManifest object."""
-        self.gitc_client_name = gitc_client_name
-        self.gitc_client_dir = os.path.join(
-            gitc_utils.get_gitc_manifest_dir(), gitc_client_name
-        )
-
-        super().__init__(
-            repodir, os.path.join(self.gitc_client_dir, ".manifest")
-        )
-        self.isGitcClient = True

pager.py

@@ -19,6 +19,7 @@ import sys
 
 import platform_utils
 
+
 active = False
 pager_process = None
 old_stdout = None

platform_utils.py

@@ -57,8 +57,8 @@ def _validate_winpath(path):
     if _winpath_is_valid(path):
         return path
     raise ValueError(
-        'Path "{}" must be a relative path or an absolute '
-        "path starting with a drive letter".format(path)
+        f'Path "{path}" must be a relative path or an absolute '
+        "path starting with a drive letter"
     )
 
 
@@ -193,10 +193,9 @@ def _walk_windows_impl(top, topdown, onerror, followlinks):
     for name in dirs:
         new_path = os.path.join(top, name)
         if followlinks or not islink(new_path):
-            for x in _walk_windows_impl(
+            yield from _walk_windows_impl(
                 new_path, topdown, onerror, followlinks
-            ):
-                yield x
+            )
     if not topdown:
         yield top, dirs, nondirs
 

platform_utils_win32.py

@@ -12,12 +12,28 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from ctypes import addressof
+from ctypes import byref
+from ctypes import c_buffer
+from ctypes import c_ubyte
+from ctypes import FormatError
+from ctypes import get_last_error
+from ctypes import Structure
+from ctypes import Union
+from ctypes import WinDLL
+from ctypes import WinError
+from ctypes.wintypes import BOOL
+from ctypes.wintypes import BOOLEAN
+from ctypes.wintypes import DWORD
+from ctypes.wintypes import HANDLE
+from ctypes.wintypes import LPCWSTR
+from ctypes.wintypes import LPDWORD
+from ctypes.wintypes import LPVOID
+from ctypes.wintypes import ULONG
+from ctypes.wintypes import USHORT
+from ctypes.wintypes import WCHAR
 import errno
 
-from ctypes import WinDLL, get_last_error, FormatError, WinError, addressof
-from ctypes import c_buffer, c_ubyte, Structure, Union, byref
-from ctypes.wintypes import BOOL, BOOLEAN, LPCWSTR, DWORD, HANDLE
-from ctypes.wintypes import WCHAR, USHORT, LPVOID, ULONG, LPDWORD
 
 kernel32 = WinDLL("kernel32", use_last_error=True)
 
@@ -170,9 +186,7 @@ def _create_symlink(source, link_name, dwFlags):
             error_desc = FormatError(code).strip()
             if code == ERROR_PRIVILEGE_NOT_HELD:
                 raise OSError(errno.EPERM, error_desc, link_name)
-            _raise_winerror(
-                code, 'Error creating symbolic link "{}"'.format(link_name)
-            )
+            _raise_winerror(code, f'Error creating symbolic link "{link_name}"')
 
 
 def islink(path):
@@ -194,7 +208,7 @@ def readlink(path):
     )
     if reparse_point_handle == INVALID_HANDLE_VALUE:
         _raise_winerror(
-            get_last_error(), 'Error opening symbolic link "{}"'.format(path)
+            get_last_error(), f'Error opening symbolic link "{path}"'
         )
     target_buffer = c_buffer(MAXIMUM_REPARSE_DATA_BUFFER_SIZE)
     n_bytes_returned = DWORD()
@@ -211,7 +225,7 @@ def readlink(path):
     CloseHandle(reparse_point_handle)
     if not io_result:
         _raise_winerror(
-            get_last_error(), 'Error reading symbolic link "{}"'.format(path)
+            get_last_error(), f'Error reading symbolic link "{path}"'
         )
     rdb = REPARSE_DATA_BUFFER.from_buffer(target_buffer)
     if rdb.ReparseTag == IO_REPARSE_TAG_SYMLINK:
@@ -220,11 +234,11 @@ def readlink(path):
         return rdb.MountPointReparseBuffer.PrintName
     # Unsupported reparse point type.
     _raise_winerror(
-        ERROR_NOT_SUPPORTED, 'Error reading symbolic link "{}"'.format(path)
+        ERROR_NOT_SUPPORTED, f'Error reading symbolic link "{path}"'
     )
 
 
 def _raise_winerror(code, error_desc):
     win_error_desc = FormatError(code).strip()
-    error_desc = "{0}: {1}".format(error_desc, win_error_desc)
+    error_desc = f"{error_desc}: {win_error_desc}"
     raise WinError(code, error_desc)

progress.py

@@ -16,6 +16,7 @@ import os
 import sys
 import time
 
+
 try:
     import threading as _threading
 except ImportError:
@@ -23,6 +24,7 @@ except ImportError:
 
 from repo_trace import IsTraceToStderr
 
+
 _TTY = sys.stderr.isatty()
 
 # This will erase all content in the current line (wherever the cursor is).
@@ -50,11 +52,11 @@ def duration_str(total):
     uses microsecond resolution.  This makes for noisy output.
     """
     hours, mins, secs = convert_to_hms(total)
-    ret = "%.3fs" % (secs,)
+    ret = f"{secs:.3f}s"
     if mins:
-        ret = "%im%s" % (mins, ret)
+        ret = f"{mins}m{ret}"
     if hours:
-        ret = "%ih%s" % (hours, ret)
+        ret = f"{hours}h{ret}"
     return ret
 
 
@@ -80,7 +82,7 @@ def jobs_str(total):
     return f"{total} job{'s' if total > 1 else ''}"
 
 
-class Progress(object):
+class Progress:
     def __init__(
         self,
         title,

pyproject.toml

@@ -15,4 +15,4 @@
 [tool.black]
 line-length = 80
 # NB: Keep in sync with tox.ini.
-target-version = ['py36', 'py37', 'py38', 'py39', 'py310']
+target-version = ['py36', 'py37', 'py38', 'py39', 'py310', 'py311'] #, 'py312'

release/update-manpages

@@ -22,4 +22,5 @@ import sys
 
 import update_manpages
 
+
 sys.exit(update_manpages.main(sys.argv[1:]))

release/update_manpages.py

@@ -17,17 +17,18 @@
 Most code lives in this module so it can be unittested.
 """
 
-from pathlib import Path
-from functools import partial
 import argparse
+import functools
 import multiprocessing
 import os
+from pathlib import Path
 import re
 import shutil
 import subprocess
 import sys
 import tempfile
 
+
 TOPDIR = Path(__file__).resolve().parent.parent
 MANDIR = TOPDIR.joinpath("man")
 
@@ -112,7 +113,9 @@ def main(argv):
 
         # Run all cmd in parallel, and wait for them to finish.
         with multiprocessing.Pool() as pool:
-            pool.map(partial(worker, cwd=tempdir, check=True), cmdlist)
+            pool.map(
+                functools.partial(worker, cwd=tempdir, check=True), cmdlist
+            )
 
     for tmp_path in MANDIR.glob("*.1.tmp"):
         path = tmp_path.parent / tmp_path.stem

repo_logging.py

@@ -0,0 +1,93 @@
+# Copyright (C) 2023 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Logic for printing user-friendly logs in repo."""
+
+import logging
+
+from color import Coloring
+from error import RepoExitError
+
+
+SEPARATOR = "=" * 80
+MAX_PRINT_ERRORS = 5
+
+
+class _ConfigMock:
+    """Default coloring config to use when Logging.config is not set."""
+
+    def __init__(self):
+        self.default_values = {"color.ui": "auto"}
+
+    def GetString(self, x):
+        return self.default_values.get(x, None)
+
+
+class _LogColoring(Coloring):
+    """Coloring outstream for logging."""
+
+    def __init__(self, config):
+        super().__init__(config, "logs")
+        self.error = self.colorer("error", fg="red")
+        self.warning = self.colorer("warn", fg="yellow")
+        self.levelMap = {
+            "WARNING": self.warning,
+            "ERROR": self.error,
+        }
+
+
+class _LogColoringFormatter(logging.Formatter):
+    """Coloring formatter for logging."""
+
+    def __init__(self, config=None, *args, **kwargs):
+        self.config = config if config else _ConfigMock()
+        self.colorer = _LogColoring(self.config)
+        super().__init__(*args, **kwargs)
+
+    def format(self, record):
+        """Formats |record| with color."""
+        msg = super().format(record)
+        colorer = self.colorer.levelMap.get(record.levelname)
+        return msg if not colorer else colorer(msg)
+
+
+class RepoLogger(logging.Logger):
+    """Repo Logging Module."""
+
+    def __init__(self, name: str, config=None, **kwargs):
+        super().__init__(name, **kwargs)
+        handler = logging.StreamHandler()
+        handler.setFormatter(_LogColoringFormatter(config))
+        self.addHandler(handler)
+
+    def log_aggregated_errors(self, err: RepoExitError):
+        """Print all aggregated logs."""
+        self.error(SEPARATOR)
+
+        if not err.aggregate_errors:
+            self.error("Repo command failed: %s", type(err).__name__)
+            self.error("\t%s", str(err))
+            return
+
+        self.error(
+            "Repo command failed due to the following `%s` errors:",
+            type(err).__name__,
+        )
+        self.error(
+            "\n".join(str(e) for e in err.aggregate_errors[:MAX_PRINT_ERRORS])
+        )
+
+        diff = len(err.aggregate_errors) - MAX_PRINT_ERRORS
+        if diff > 0:
+            self.error("+%d additional errors...", diff)

repo_trace.py

@@ -20,14 +20,15 @@ Temporary: Tracing is always on. Set `REPO_TRACE=0` to turn off.
 To also include trace outputs in stderr do `repo --trace_to_stderr ...`
 """
 
-import sys
+import contextlib
 import os
-import time
+import sys
 import tempfile
-from contextlib import ContextDecorator
+import time
 
 import platform_utils
 
+
 # Env var to implicitly turn on tracing.
 REPO_TRACE = "REPO_TRACE"
 
@@ -68,7 +69,7 @@ def _SetTraceFile(quiet):
     _TRACE_FILE = _GetTraceFile(quiet)
 
 
-class Trace(ContextDecorator):
+class Trace(contextlib.ContextDecorator):
     """Used to capture and save git traces."""
 
     def _time(self):
@@ -141,7 +142,7 @@ def _GetTraceFile(quiet):
 def _ClearOldTraces():
     """Clear the oldest commands if trace file is too big."""
     try:
-        with open(_TRACE_FILE, "r", errors="ignore") as f:
+        with open(_TRACE_FILE, errors="ignore") as f:
             if os.path.getsize(f.name) / (1024 * 1024) <= _MAX_SIZE:
                 return
             trace_lines = f.readlines()

run_tests

@@ -18,6 +18,7 @@
 import os
 import subprocess
 import sys
+
 import pytest
 
 
@@ -26,8 +27,16 @@ ROOT_DIR = os.path.dirname(os.path.realpath(__file__))
 
 def run_black():
     """Returns the exit code from black."""
+    # Black by default only matches .py files.  We have to list standalone
+    # scripts manually.
+    extra_programs = [
+        "repo",
+        "run_tests",
+        "release/update-manpages",
+    ]
     return subprocess.run(
-        [sys.executable, "-m", "black", "--check", ROOT_DIR], check=False
+        [sys.executable, "-m", "black", "--check", ROOT_DIR] + extra_programs,
+        check=False,
     ).returncode
 
 
@@ -38,12 +47,20 @@ def run_flake8():
     ).returncode
 
 
+def run_isort():
+    """Returns the exit code from isort."""
+    return subprocess.run(
+        [sys.executable, "-m", "isort", "--check", ROOT_DIR], check=False
+    ).returncode
+
+
 def main(argv):
     """The main entry."""
     checks = (
         lambda: pytest.main(argv),
         run_black,
         run_flake8,
+        run_isort,
     )
     return 0 if all(not c() for c in checks) else 1
 

run_tests.vpython3

@@ -123,3 +123,8 @@ wheel: <
   name: "infra/python/wheels/pycodestyle-py2_py3"
   version: "version:2.10.0"
 >
+
+wheel: <
+  name: "infra/python/wheels/isort-py3"
+  version: "version:5.10.1"
+>

setup.py

@@ -16,6 +16,7 @@
 """Python packaging for repo."""
 
 import os
+
 import setuptools
 
 

ssh.py

@@ -165,7 +165,7 @@ class ProxyManager:
         # Check to see whether we already think that the master is running; if
         # we think it's already running, return right away.
         if port is not None:
-            key = "%s:%s" % (host, port)
+            key = f"{host}:{port}"
         else:
             key = host
 

subcmds/__init__.py

@@ -14,8 +14,10 @@
 
 import os
 
+
 # A mapping of the subcommand name to the class that implements it.
 all_commands = {}
+all_modules = []
 
 my_dir = os.path.dirname(__file__)
 for py in os.listdir(my_dir):
@@ -35,13 +37,12 @@ for py in os.listdir(my_dir):
         try:
             cmd = getattr(mod, clsn)
         except AttributeError:
-            raise SyntaxError(
-                "%s/%s does not define class %s" % (__name__, py, clsn)
-            )
+            raise SyntaxError(f"{__name__}/{py} does not define class {clsn}")
 
         name = name.replace("_", "-")
         cmd.NAME = name
         all_commands[name] = cmd
+        all_modules.append(mod)
 
 # Add 'branch' as an alias for 'branches'.
 all_commands["branch"] = all_commands["branches"]

subcmds/abandon.py

@@ -12,14 +12,24 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from collections import defaultdict
+import collections
 import functools
 import itertools
-import sys
 
-from command import Command, DEFAULT_LOCAL_JOBS
+from command import Command
+from command import DEFAULT_LOCAL_JOBS
+from error import RepoError
+from error import RepoExitError
 from git_command import git
 from progress import Progress
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
+
+
+class AbandonError(RepoExitError):
+    """Exit error when abandon command fails."""
 
 
 class Abandon(Command):
@@ -68,28 +78,37 @@ It is equivalent to "git branch -D <branchname>".
             branches = nb
 
         ret = {}
+        errors = []
         for name in branches:
-            status = project.AbandonBranch(name)
+            status = None
+            try:
+                status = project.AbandonBranch(name)
+            except RepoError as e:
+                status = False
+                errors.append(e)
             if status is not None:
                 ret[name] = status
-        return (ret, project)
+
+        return (ret, project, errors)
 
     def Execute(self, opt, args):
         nb = args[0].split()
-        err = defaultdict(list)
-        success = defaultdict(list)
+        err = collections.defaultdict(list)
+        success = collections.defaultdict(list)
+        aggregate_errors = []
         all_projects = self.GetProjects(
             args[1:], all_manifests=not opt.this_manifest_only
         )
         _RelPath = lambda p: p.RelPath(local=opt.this_manifest_only)
 
         def _ProcessResults(_pool, pm, states):
-            for results, project in states:
+            for results, project, errors in states:
                 for branch, status in results.items():
                     if status:
                         success[branch].append(project)
                     else:
                         err[branch].append(project)
+                aggregate_errors.extend(errors)
                 pm.update(msg="")
 
         self.ExecuteInParallel(
@@ -98,7 +117,7 @@ It is equivalent to "git branch -D <branchname>".
             all_projects,
             callback=_ProcessResults,
             output=Progress(
-                "Abandon %s" % (nb,), len(all_projects), quiet=opt.quiet
+                f"Abandon {nb}", len(all_projects), quiet=opt.quiet
             ),
         )
 
@@ -110,19 +129,13 @@ It is equivalent to "git branch -D <branchname>".
         if err:
             for br in err.keys():
                 err_msg = "error: cannot abandon %s" % br
-                print(err_msg, file=sys.stderr)
+                logger.error(err_msg)
                 for proj in err[br]:
-                    print(
-                        " " * len(err_msg) + " | %s" % _RelPath(proj),
-                        file=sys.stderr,
-                    )
-            sys.exit(1)
+                    logger.error(" " * len(err_msg) + " | %s", _RelPath(proj))
+            raise AbandonError(aggregate_errors=aggregate_errors)
         elif not success:
-            print(
-                "error: no project has local branch(es) : %s" % nb,
-                file=sys.stderr,
-            )
-            sys.exit(1)
+            logger.error("error: no project has local branch(es) : %s", nb)
+            raise AbandonError(aggregate_errors=aggregate_errors)
         else:
             # Everything below here is displaying status.
             if opt.quiet:
@@ -139,4 +152,4 @@ It is equivalent to "git branch -D <branchname>".
                             _RelPath(p) for p in success[br]
                         )
                     )
-                print("%s%s| %s\n" % (br, " " * (width - len(br)), result))
+                print(f"{br}{' ' * (width - len(br))}| {result}\n")

subcmds/branches.py

@@ -16,7 +16,8 @@ import itertools
 import sys
 
 from color import Coloring
-from command import Command, DEFAULT_LOCAL_JOBS
+from command import Command
+from command import DEFAULT_LOCAL_JOBS
 
 
 class BranchColoring(Coloring):
@@ -27,7 +28,7 @@ class BranchColoring(Coloring):
         self.notinproject = self.printer("notinproject", fg="red")
 
 
-class BranchInfo(object):
+class BranchInfo:
     def __init__(self, name):
         self.name = name
         self.current = 0
@@ -173,7 +174,7 @@ is shown, then the branch appears in all projects.
                         if _RelPath(p) not in have:
                             paths.append(_RelPath(p))
 
-                s = " %s %s" % (in_type, ", ".join(paths))
+                s = f" {in_type} {', '.join(paths)}"
                 if not i.IsSplitCurrent and (width + 7 + len(s) < 80):
                     fmt = out.current if i.IsCurrent else fmt
                     fmt(s)

subcmds/checkout.py

@@ -13,10 +13,33 @@
 # limitations under the License.
 
 import functools
-import sys
+from typing import NamedTuple
 
-from command import Command, DEFAULT_LOCAL_JOBS
+from command import Command
+from command import DEFAULT_LOCAL_JOBS
+from error import GitError
+from error import RepoExitError
 from progress import Progress
+from project import Project
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
+
+
+class CheckoutBranchResult(NamedTuple):
+    # Whether the Project is on the branch (i.e. branch exists and no errors)
+    result: bool
+    project: Project
+    error: Exception
+
+
+class CheckoutCommandError(RepoExitError):
+    """Exception thrown when checkout command fails."""
+
+
+class MissingBranchError(RepoExitError):
+    """Exception thrown when no project has specified branch."""
 
 
 class Checkout(Command):
@@ -41,23 +64,30 @@ The command is equivalent to:
 
     def _ExecuteOne(self, nb, project):
         """Checkout one project."""
-        return (project.CheckoutBranch(nb), project)
+        error = None
+        result = None
+        try:
+            result = project.CheckoutBranch(nb)
+        except GitError as e:
+            error = e
+        return CheckoutBranchResult(result, project, error)
 
     def Execute(self, opt, args):
         nb = args[0]
         err = []
+        err_projects = []
         success = []
         all_projects = self.GetProjects(
             args[1:], all_manifests=not opt.this_manifest_only
         )
 
         def _ProcessResults(_pool, pm, results):
-            for status, project in results:
-                if status is not None:
-                    if status:
-                        success.append(project)
-                    else:
-                        err.append(project)
+            for result in results:
+                if result.error is not None:
+                    err.append(result.error)
+                    err_projects.append(result.project)
+                elif result.result:
+                    success.append(result.project)
                 pm.update(msg="")
 
         self.ExecuteInParallel(
@@ -66,17 +96,15 @@ The command is equivalent to:
             all_projects,
             callback=_ProcessResults,
             output=Progress(
-                "Checkout %s" % (nb,), len(all_projects), quiet=opt.quiet
+                f"Checkout {nb}", len(all_projects), quiet=opt.quiet
             ),
         )
 
-        if err:
-            for p in err:
-                print(
-                    "error: %s/: cannot checkout %s" % (p.relpath, nb),
-                    file=sys.stderr,
-                )
-            sys.exit(1)
+        if err_projects:
+            for p in err_projects:
+                logger.error("error: %s/: cannot checkout %s", p.relpath, nb)
+            raise CheckoutCommandError(aggregate_errors=err)
         elif not success:
-            print("error: no project has branch %s" % nb, file=sys.stderr)
-            sys.exit(1)
+            msg = f"error: no project has branch {nb}"
+            logger.error(msg)
+            raise MissingBranchError(msg)

subcmds/cherry_pick.py

@@ -14,10 +14,15 @@
 
 import re
 import sys
+
 from command import Command
+from error import GitError
 from git_command import GitCommand
+from repo_logging import RepoLogger
+
 
 CHANGE_ID_RE = re.compile(r"^\s*Change-Id: I([0-9a-f]{40})\s*$")
+logger = RepoLogger(__file__)
 
 
 class CherryPick(Command):
@@ -44,18 +49,29 @@ change id will be added.
             ["rev-parse", "--verify", reference],
             capture_stdout=True,
             capture_stderr=True,
+            verify_command=True,
         )
-        if p.Wait() != 0:
-            print(p.stderr, file=sys.stderr)
-            sys.exit(1)
+        try:
+            p.Wait()
+        except GitError:
+            logger.error(p.stderr)
+            raise
+
         sha1 = p.stdout.strip()
 
-        p = GitCommand(None, ["cat-file", "commit", sha1], capture_stdout=True)
-        if p.Wait() != 0:
-            print(
-                "error: Failed to retrieve old commit message", file=sys.stderr
-            )
-            sys.exit(1)
+        p = GitCommand(
+            None,
+            ["cat-file", "commit", sha1],
+            capture_stdout=True,
+            verify_command=True,
+        )
+
+        try:
+            p.Wait()
+        except GitError:
+            logger.error("error: Failed to retrieve old commit message")
+            raise
+
         old_msg = self._StripHeader(p.stdout)
 
         p = GitCommand(
@@ -63,37 +79,43 @@ change id will be added.
             ["cherry-pick", sha1],
             capture_stdout=True,
             capture_stderr=True,
+            verify_command=True,
         )
-        status = p.Wait()
+
+        try:
+            p.Wait()
+        except GitError as e:
+            logger.error(e)
+            logger.warning(
+                "NOTE: When committing (please see above) and editing the "
+                "commit message, please remove the old Change-Id-line and "
+                "add:\n%s",
+                self._GetReference(sha1),
+            )
+            raise
 
         if p.stdout:
             print(p.stdout.strip(), file=sys.stdout)
         if p.stderr:
             print(p.stderr.strip(), file=sys.stderr)
 
-        if status == 0:
-            # The cherry-pick was applied correctly. We just need to edit the
-            # commit message.
-            new_msg = self._Reformat(old_msg, sha1)
+        # The cherry-pick was applied correctly. We just need to edit
+        # the commit message.
+        new_msg = self._Reformat(old_msg, sha1)
 
-            p = GitCommand(
-                None,
-                ["commit", "--amend", "-F", "-"],
-                input=new_msg,
-                capture_stdout=True,
-                capture_stderr=True,
-            )
-            if p.Wait() != 0:
-                print("error: Failed to update commit message", file=sys.stderr)
-                sys.exit(1)
-
-        else:
-            print(
-                "NOTE: When committing (please see above) and editing the "
-                "commit message, please remove the old Change-Id-line and add:"
-            )
-            print(self._GetReference(sha1), file=sys.stderr)
-            print(file=sys.stderr)
+        p = GitCommand(
+            None,
+            ["commit", "--amend", "-F", "-"],
+            input=new_msg,
+            capture_stdout=True,
+            capture_stderr=True,
+            verify_command=True,
+        )
+        try:
+            p.Wait()
+        except GitError:
+            logger.error("error: Failed to update commit message")
+            raise
 
     def _IsChangeId(self, line):
         return CHANGE_ID_RE.match(line)

subcmds/diff.py

@@ -15,7 +15,8 @@
 import functools
 import io
 
-from command import DEFAULT_LOCAL_JOBS, PagedCommand
+from command import DEFAULT_LOCAL_JOBS
+from command import PagedCommand
 
 
 class Diff(PagedCommand):

subcmds/diffmanifests.py

@@ -87,25 +87,17 @@ synced and their revisions won't be found.
     def _printRawDiff(self, diff, pretty_format=None, local=False):
         _RelPath = lambda p: p.RelPath(local=local)
         for project in diff["added"]:
-            self.printText(
-                "A %s %s" % (_RelPath(project), project.revisionExpr)
-            )
+            self.printText(f"A {_RelPath(project)} {project.revisionExpr}")
             self.out.nl()
 
         for project in diff["removed"]:
-            self.printText(
-                "R %s %s" % (_RelPath(project), project.revisionExpr)
-            )
+            self.printText(f"R {_RelPath(project)} {project.revisionExpr}")
             self.out.nl()
 
         for project, otherProject in diff["changed"]:
             self.printText(
-                "C %s %s %s"
-                % (
-                    _RelPath(project),
-                    project.revisionExpr,
-                    otherProject.revisionExpr,
-                )
+                f"C {_RelPath(project)} {project.revisionExpr} "
+                f"{otherProject.revisionExpr}"
             )
             self.out.nl()
             self._printLogs(
@@ -118,12 +110,8 @@ synced and their revisions won't be found.
 
         for project, otherProject in diff["unreachable"]:
             self.printText(
-                "U %s %s %s"
-                % (
-                    _RelPath(project),
-                    project.revisionExpr,
-                    otherProject.revisionExpr,
-                )
+                f"U {_RelPath(project)} {project.revisionExpr} "
+                f"{otherProject.revisionExpr}"
             )
             self.out.nl()
 

subcmds/download.py

@@ -16,9 +16,18 @@ import re
 import sys
 
 from command import Command
-from error import GitError, NoSuchProjectError
+from error import GitError
+from error import NoSuchProjectError
+from error import RepoExitError
+from repo_logging import RepoLogger
+
 
 CHANGE_RE = re.compile(r"^([1-9][0-9]*)(?:[/\.-]([1-9][0-9]*))?$")
+logger = RepoLogger(__file__)
+
+
+class DownloadCommandError(RepoExitError):
+    """Error raised when download command fails."""
 
 
 class Download(Command):
@@ -102,23 +111,18 @@ If no project is specified try to use current directory as a project.
                     except NoSuchProjectError:
                         project = None
                     if project not in projects:
-                        print(
+                        logger.error(
                             "error: %s matches too many projects; please "
-                            "re-run inside the project checkout." % (a,),
-                            file=sys.stderr,
+                            "re-run inside the project checkout.",
+                            a,
                         )
                         for project in projects:
-                            print(
-                                "  %s/ @ %s"
-                                % (
-                                    project.RelPath(
-                                        local=opt.this_manifest_only
-                                    ),
-                                    project.revisionExpr,
-                                ),
-                                file=sys.stderr,
+                            logger.error(
+                                "  %s/ @ %s",
+                                project.RelPath(local=opt.this_manifest_only),
+                                project.revisionExpr,
                             )
-                        sys.exit(1)
+                        raise NoSuchProjectError()
                 else:
                     project = projects[0]
                     print("Defaulting to cwd project", project.name)
@@ -137,29 +141,33 @@ If no project is specified try to use current directory as a project.
                 )
 
     def Execute(self, opt, args):
+        try:
+            self._ExecuteHelper(opt, args)
+        except Exception as e:
+            if isinstance(e, RepoExitError):
+                raise e
+            raise DownloadCommandError(aggregate_errors=[e])
+
+    def _ExecuteHelper(self, opt, args):
         for project, change_id, ps_id in self._ParseChangeIds(opt, args):
             dl = project.DownloadPatchSet(change_id, ps_id)
-            if not dl:
-                print(
-                    "[%s] change %d/%d not found"
-                    % (project.name, change_id, ps_id),
-                    file=sys.stderr,
-                )
-                sys.exit(1)
 
             if not opt.revert and not dl.commits:
-                print(
-                    "[%s] change %d/%d has already been merged"
-                    % (project.name, change_id, ps_id),
-                    file=sys.stderr,
+                logger.error(
+                    "[%s] change %d/%d has already been merged",
+                    project.name,
+                    change_id,
+                    ps_id,
                 )
                 continue
 
             if len(dl.commits) > 1:
-                print(
-                    "[%s] %d/%d depends on %d unmerged changes:"
-                    % (project.name, change_id, ps_id, len(dl.commits)),
-                    file=sys.stderr,
+                logger.error(
+                    "[%s] %d/%d depends on %d unmerged changes:",
+                    project.name,
+                    change_id,
+                    ps_id,
+                    len(dl.commits),
                 )
                 for c in dl.commits:
                     print("  %s" % (c), file=sys.stderr)
@@ -196,9 +204,10 @@ If no project is specified try to use current directory as a project.
                         project._Checkout(dl.commit)
 
             except GitError:
-                print(
-                    "[%s] Could not complete the %s of %s"
-                    % (project.name, mode, dl.commit),
-                    file=sys.stderr,
+                logger.error(
+                    "[%s] Could not complete the %s of %s",
+                    project.name,
+                    mode,
+                    dl.commit,
                 )
-                sys.exit(1)
+                raise

subcmds/forall.py

@@ -16,21 +16,22 @@ import errno
 import functools
 import io
 import multiprocessing
-import re
 import os
+import re
 import signal
-import sys
 import subprocess
+import sys
 
 from color import Coloring
-from command import (
-    DEFAULT_LOCAL_JOBS,
-    Command,
-    MirrorSafeCommand,
-    WORKER_BATCH_SIZE,
-)
+from command import Command
+from command import DEFAULT_LOCAL_JOBS
+from command import MirrorSafeCommand
+from command import WORKER_BATCH_SIZE
 from error import ManifestInvalidRevisionError
+from repo_logging import RepoLogger
 
+
+logger = RepoLogger(__file__)
 _CAN_COLOR = [
     "branch",
     "diff",
@@ -294,10 +295,10 @@ without iterating through the remaining projects.
             rc = rc or errno.EINTR
         except Exception as e:
             # Catch any other exceptions raised
-            print(
-                "forall: unhandled error, terminating the pool: %s: %s"
-                % (type(e).__name__, e),
-                file=sys.stderr,
+            logger.error(
+                "forall: unhandled error, terminating the pool: %s: %s",
+                type(e).__name__,
+                e,
             )
             rc = rc or getattr(e, "errno", 1)
         if rc != 0:

subcmds/gitc_delete.py

@@ -1,52 +0,0 @@
-# Copyright (C) 2015 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-import sys
-
-from command import Command, GitcClientCommand
-import platform_utils
-
-
-class GitcDelete(Command, GitcClientCommand):
-    COMMON = True
-    visible_everywhere = False
-    helpSummary = "Delete a GITC Client."
-    helpUsage = """
-%prog
-"""
-    helpDescription = """
-This subcommand deletes the current GITC client, deleting the GITC manifest
-and all locally downloaded sources.
-"""
-
-    def _Options(self, p):
-        p.add_option(
-            "-f",
-            "--force",
-            dest="force",
-            action="store_true",
-            help="force the deletion (no prompt)",
-        )
-
-    def Execute(self, opt, args):
-        if not opt.force:
-            prompt = (
-                "This will delete GITC client: %s\nAre you sure? (yes/no) "
-                % self.gitc_manifest.gitc_client_name
-            )
-            response = input(prompt).lower()
-            if not response == "yes":
-                print('Response was not "yes"\n Exiting...')
-                sys.exit(1)
-        platform_utils.rmtree(self.gitc_manifest.gitc_client_dir)

subcmds/gitc_init.py

@@ -1,87 +0,0 @@
-# Copyright (C) 2015 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-import os
-import sys
-
-import gitc_utils
-from command import GitcAvailableCommand
-from manifest_xml import GitcManifest
-from subcmds import init
-import wrapper
-
-
-class GitcInit(init.Init, GitcAvailableCommand):
-    COMMON = True
-    MULTI_MANIFEST_SUPPORT = False
-    helpSummary = "Initialize a GITC Client."
-    helpUsage = """
-%prog [options] [client name]
-"""
-    helpDescription = """
-The '%prog' command is ran to initialize a new GITC client for use
-with the GITC file system.
-
-This command will setup the client directory, initialize repo, just
-like repo init does, and then downloads the manifest collection
-and installs it in the .repo/directory of the GITC client.
-
-Once this is done, a GITC manifest is generated by pulling the HEAD
-SHA for each project and generates the properly formatted XML file
-and installs it as .manifest in the GITC client directory.
-
-The -c argument is required to specify the GITC client name.
-
-The optional -f argument can be used to specify the manifest file to
-use for this GITC client.
-"""
-
-    def _Options(self, p):
-        super()._Options(p, gitc_init=True)
-
-    def Execute(self, opt, args):
-        gitc_client = gitc_utils.parse_clientdir(os.getcwd())
-        if not gitc_client or (
-            opt.gitc_client and gitc_client != opt.gitc_client
-        ):
-            print(
-                "fatal: Please update your repo command. See go/gitc for "
-                "instructions.",
-                file=sys.stderr,
-            )
-            sys.exit(1)
-        self.client_dir = os.path.join(
-            gitc_utils.get_gitc_manifest_dir(), gitc_client
-        )
-        super().Execute(opt, args)
-
-        manifest_file = self.manifest.manifestFile
-        if opt.manifest_file:
-            if not os.path.exists(opt.manifest_file):
-                print(
-                    "fatal: Specified manifest file %s does not exist."
-                    % opt.manifest_file
-                )
-                sys.exit(1)
-            manifest_file = opt.manifest_file
-
-        manifest = GitcManifest(
-            self.repodir, os.path.join(self.client_dir, ".manifest")
-        )
-        manifest.Override(manifest_file)
-        gitc_utils.generate_gitc_manifest(None, manifest)
-        print(
-            "Please run `cd %s` to view your GITC client."
-            % os.path.join(wrapper.Wrapper().GITC_FS_ROOT_DIR, gitc_client)
-        )

subcmds/grep.py

@@ -14,11 +14,20 @@
 
 import functools
 import sys
+from typing import NamedTuple
 
 from color import Coloring
-from command import DEFAULT_LOCAL_JOBS, PagedCommand
+from command import DEFAULT_LOCAL_JOBS
+from command import PagedCommand
 from error import GitError
+from error import InvalidArgumentsError
+from error import SilentRepoExitError
 from git_command import GitCommand
+from project import Project
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
 
 
 class GrepColoring(Coloring):
@@ -28,6 +37,22 @@ class GrepColoring(Coloring):
         self.fail = self.printer("fail", fg="red")
 
 
+class ExecuteOneResult(NamedTuple):
+    """Result from an execute instance."""
+
+    project: Project
+    rc: int
+    stdout: str
+    stderr: str
+    error: GitError
+
+
+class GrepCommandError(SilentRepoExitError):
+    """Grep command failure. Since Grep command
+    output already outputs errors ensure that
+    aggregate errors exit silently."""
+
+
 class Grep(PagedCommand):
     COMMON = True
     helpSummary = "Print lines matching a pattern"
@@ -246,11 +271,18 @@ contain a line that matches both expressions:
                 bare=False,
                 capture_stdout=True,
                 capture_stderr=True,
+                verify_command=True,
             )
         except GitError as e:
-            return (project, -1, None, str(e))
+            return ExecuteOneResult(project, -1, None, str(e), e)
 
-        return (project, p.Wait(), p.stdout, p.stderr)
+        try:
+            error = None
+            rc = p.Wait()
+        except GitError as e:
+            rc = 1
+            error = e
+        return ExecuteOneResult(project, rc, p.stdout, p.stderr, error)
 
     @staticmethod
     def _ProcessResults(full_name, have_rev, opt, _pool, out, results):
@@ -258,31 +290,40 @@ contain a line that matches both expressions:
         bad_rev = False
         have_match = False
         _RelPath = lambda p: p.RelPath(local=opt.this_manifest_only)
+        errors = []
 
-        for project, rc, stdout, stderr in results:
-            if rc < 0:
+        for result in results:
+            if result.rc < 0:
                 git_failed = True
-                out.project("--- project %s ---" % _RelPath(project))
+                out.project("--- project %s ---" % _RelPath(result.project))
                 out.nl()
-                out.fail("%s", stderr)
+                out.fail("%s", result.stderr)
                 out.nl()
+                errors.append(result.error)
                 continue
 
-            if rc:
+            if result.rc:
                 # no results
-                if stderr:
-                    if have_rev and "fatal: ambiguous argument" in stderr:
+                if result.stderr:
+                    if (
+                        have_rev
+                        and "fatal: ambiguous argument" in result.stderr
+                    ):
                         bad_rev = True
                     else:
-                        out.project("--- project %s ---" % _RelPath(project))
+                        out.project(
+                            "--- project %s ---" % _RelPath(result.project)
+                        )
                         out.nl()
-                        out.fail("%s", stderr.strip())
+                        out.fail("%s", result.stderr.strip())
                         out.nl()
+                    if result.error is not None:
+                        errors.append(result.error)
                 continue
             have_match = True
 
             # We cut the last element, to avoid a blank line.
-            r = stdout.split("\n")
+            r = result.stdout.split("\n")
             r = r[0:-1]
 
             if have_rev and full_name:
@@ -290,13 +331,13 @@ contain a line that matches both expressions:
                     rev, line = line.split(":", 1)
                     out.write("%s", rev)
                     out.write(":")
-                    out.project(_RelPath(project))
+                    out.project(_RelPath(result.project))
                     out.write("/")
                     out.write("%s", line)
                     out.nl()
             elif full_name:
                 for line in r:
-                    out.project(_RelPath(project))
+                    out.project(_RelPath(result.project))
                     out.write("/")
                     out.write("%s", line)
                     out.nl()
@@ -304,7 +345,7 @@ contain a line that matches both expressions:
                 for line in r:
                     print(line)
 
-        return (git_failed, bad_rev, have_match)
+        return (git_failed, bad_rev, have_match, errors)
 
     def Execute(self, opt, args):
         out = GrepColoring(self.manifest.manifestProject.config)
@@ -333,16 +374,14 @@ contain a line that matches both expressions:
         have_rev = False
         if opt.revision:
             if "--cached" in cmd_argv:
-                print(
-                    "fatal: cannot combine --cached and --revision",
-                    file=sys.stderr,
-                )
-                sys.exit(1)
+                msg = "fatal: cannot combine --cached and --revision"
+                logger.error(msg)
+                raise InvalidArgumentsError(msg)
             have_rev = True
             cmd_argv.extend(opt.revision)
         cmd_argv.append("--")
 
-        git_failed, bad_rev, have_match = self.ExecuteInParallel(
+        git_failed, bad_rev, have_match, errors = self.ExecuteInParallel(
             opt.jobs,
             functools.partial(self._ExecuteOne, cmd_argv),
             projects,
@@ -354,12 +393,12 @@ contain a line that matches both expressions:
         )
 
         if git_failed:
-            sys.exit(1)
+            raise GrepCommandError(
+                "error: git failures", aggregate_errors=errors
+            )
         elif have_match:
             sys.exit(0)
         elif have_rev and bad_rev:
             for r in opt.revision:
-                print("error: can't search revision %s" % r, file=sys.stderr)
-            sys.exit(1)
-        else:
-            sys.exit(1)
+                logger.error("error: can't search revision %s", r)
+        raise GrepCommandError(aggregate_errors=errors)

subcmds/help.py

@@ -16,18 +16,18 @@ import re
 import sys
 import textwrap
 
-from subcmds import all_commands
 from color import Coloring
-from command import (
-    PagedCommand,
-    MirrorSafeCommand,
-    GitcAvailableCommand,
-    GitcClientCommand,
-)
-import gitc_utils
+from command import MirrorSafeCommand
+from command import PagedCommand
+from error import RepoExitError
+from subcmds import all_commands
 from wrapper import Wrapper
 
 
+class InvalidHelpCommand(RepoExitError):
+    """Invalid command passed into help."""
+
+
 class Help(PagedCommand, MirrorSafeCommand):
     COMMON = False
     helpSummary = "Display detailed help on a command"
@@ -74,26 +74,9 @@ Displays detailed usage information about a command.
     def PrintCommonCommandsBody(self):
         print("The most commonly used repo commands are:")
 
-        def gitc_supported(cmd):
-            if not isinstance(cmd, GitcAvailableCommand) and not isinstance(
-                cmd, GitcClientCommand
-            ):
-                return True
-            if self.client.isGitcClient:
-                return True
-            if isinstance(cmd, GitcClientCommand):
-                return False
-            if gitc_utils.get_gitc_manifest_dir():
-                return True
-            return False
-
         commandNames = list(
             sorted(
-                [
-                    name
-                    for name, command in all_commands.items()
-                    if command.COMMON and gitc_supported(command)
-                ]
+                name for name, command in all_commands.items() if command.COMMON
             )
         )
         self._PrintCommands(commandNames)
@@ -167,7 +150,7 @@ Displays detailed usage information about a command.
     def _PrintAllCommandHelp(self):
         for name in sorted(all_commands):
             cmd = all_commands[name](manifest=self.manifest)
-            self._PrintCommandHelp(cmd, header_prefix="[%s] " % (name,))
+            self._PrintCommandHelp(cmd, header_prefix=f"[{name}] ")
 
     def _Options(self, p):
         p.add_option(
@@ -202,7 +185,7 @@ Displays detailed usage information about a command.
                 print(
                     "repo: '%s' is not a repo command." % name, file=sys.stderr
                 )
-                sys.exit(1)
+                raise InvalidHelpCommand(name)
 
             self._PrintCommandHelp(cmd)
 

subcmds/info.py

@@ -14,9 +14,10 @@
 
 import optparse
 
-from command import PagedCommand
 from color import Coloring
-from git_refs import R_M, R_HEADS
+from command import PagedCommand
+from git_refs import R_HEADS
+from git_refs import R_M
 
 
 class _Coloring(Coloring):
@@ -96,7 +97,9 @@ class Info(PagedCommand):
             self.headtext(self.manifest.default.revisionExpr)
         self.out.nl()
         self.heading("Manifest merge branch: ")
-        self.headtext(mergeBranch)
+        # The manifest might not have a merge branch if it isn't in a git repo,
+        # e.g. if `repo init --standalone-manifest` is used.
+        self.headtext(mergeBranch or "")
         self.out.nl()
         self.heading("Manifest groups: ")
         self.headtext(manifestGroups)
@@ -247,7 +250,7 @@ class Info(PagedCommand):
 
             for commit in commits:
                 split = commit.split()
-                self.text("{0:38}{1} ".format("", "-"))
+                self.text(f"{'':38}{'-'} ")
                 self.sha(split[0] + " ")
                 self.text(" ".join(split[1:]))
                 self.out.nl()

subcmds/init.py

@@ -16,10 +16,19 @@ import os
 import sys
 
 from color import Coloring
-from command import InteractiveCommand, MirrorSafeCommand
-from git_command import git_require, MIN_GIT_VERSION_SOFT, MIN_GIT_VERSION_HARD
+from command import InteractiveCommand
+from command import MirrorSafeCommand
+from error import RepoUnhandledExceptionError
+from error import UpdateManifestError
+from git_command import git_require
+from git_command import MIN_GIT_VERSION_HARD
+from git_command import MIN_GIT_VERSION_SOFT
+from repo_logging import RepoLogger
 from wrapper import Wrapper
 
+
+logger = RepoLogger(__file__)
+
 _REPO_ALLOW_SHALLOW = os.environ.get("REPO_ALLOW_SHALLOW")
 
 
@@ -82,8 +91,8 @@ to update the working directory files.
     def _CommonOptions(self, p):
         """Disable due to re-use of Wrapper()."""
 
-    def _Options(self, p, gitc_init=False):
-        Wrapper().InitParser(p, gitc_init=gitc_init)
+    def _Options(self, p):
+        Wrapper().InitParser(p)
         m = p.add_option_group("Multi-manifest")
         m.add_option(
             "--outer-manifest",
@@ -156,7 +165,10 @@ to update the working directory files.
             git_event_log=self.git_event_log,
             manifest_name=opt.manifest_name,
         ):
-            sys.exit(1)
+            manifest_name = opt.manifest_name
+            raise UpdateManifestError(
+                f"Unable to sync manifest {manifest_name}"
+            )
 
     def _Prompt(self, prompt, value):
         print("%-10s [%s]: " % (prompt, value), end="", flush=True)
@@ -203,7 +215,7 @@ to update the working directory files.
 
             if not opt.quiet:
                 print()
-            print("Your identity is: %s <%s>" % (name, email))
+            print(f"Your identity is: {name} <{email}>")
             print("is this correct [y/N]? ", end="", flush=True)
             a = sys.stdin.readline().strip().lower()
             if a in ("yes", "y", "t", "true"):
@@ -321,11 +333,11 @@ to update the working directory files.
     def Execute(self, opt, args):
         git_require(MIN_GIT_VERSION_HARD, fail=True)
         if not git_require(MIN_GIT_VERSION_SOFT):
-            print(
-                "repo: warning: git-%s+ will soon be required; please upgrade "
-                "your version of git to maintain support."
-                % (".".join(str(x) for x in MIN_GIT_VERSION_SOFT),),
-                file=sys.stderr,
+            logger.warning(
+                "repo: warning: git-%s+ will soon be required; "
+                "please upgrade your version of git to maintain "
+                "support.",
+                ".".join(str(x) for x in MIN_GIT_VERSION_SOFT),
             )
 
         rp = self.manifest.repoProject
@@ -341,19 +353,17 @@ to update the working directory files.
             wrapper = Wrapper()
             try:
                 remote_ref, rev = wrapper.check_repo_rev(
-                    rp.gitdir,
+                    rp.worktree,
                     opt.repo_rev,
                     repo_verify=opt.repo_verify,
                     quiet=opt.quiet,
                 )
-            except wrapper.CloneFailure:
+            except wrapper.CloneFailure as e:
                 err_msg = "fatal: double check your --repo-rev setting."
-                print(
-                    err_msg,
-                    file=sys.stderr,
-                )
+                logger.error(err_msg)
                 self.git_event_log.ErrorEvent(err_msg)
-                sys.exit(1)
+                raise RepoUnhandledExceptionError(e)
+
             branch = rp.GetBranch("default")
             branch.merge = remote_ref
             rp.work_git.reset("--hard", rev)

subcmds/list.py

@@ -14,7 +14,8 @@
 
 import os
 
-from command import Command, MirrorSafeCommand
+from command import Command
+from command import MirrorSafeCommand
 
 
 class List(Command, MirrorSafeCommand):
@@ -130,7 +131,7 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
             elif opt.path_only and not opt.name_only:
                 lines.append("%s" % (_getpath(project)))
             else:
-                lines.append("%s : %s" % (_getpath(project), project.name))
+                lines.append(f"{_getpath(project)} : {project.name}")
 
         if lines:
             lines.sort()

subcmds/manifest.py

@@ -17,6 +17,10 @@ import os
 import sys
 
 from command import PagedCommand
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
 
 
 class Manifest(PagedCommand):
@@ -132,7 +136,7 @@ to indicate the remote ref to push changes to via 'repo upload'.
             manifest.SetUseLocalManifests(not opt.ignore_local_manifests)
 
             if opt.json:
-                print("warning: --json is experimental!", file=sys.stderr)
+                logger.warning("warning: --json is experimental!")
                 doc = manifest.ToDict(
                     peg_rev=opt.peg_rev,
                     peg_rev_upstream=opt.peg_rev_upstream,
@@ -159,13 +163,13 @@ to indicate the remote ref to push changes to via 'repo upload'.
             if output_file != "-":
                 fd.close()
                 if manifest.path_prefix:
-                    print(
-                        f"Saved {manifest.path_prefix} submanifest to "
-                        f"{output_file}",
-                        file=sys.stderr,
+                    logger.warning(
+                        "Saved %s submanifest to %s",
+                        manifest.path_prefix,
+                        output_file,
                     )
                 else:
-                    print(f"Saved manifest to {output_file}", file=sys.stderr)
+                    logger.warning("Saved manifest to %s", output_file)
 
     def ValidateOptions(self, opt, args):
         if args:

subcmds/prune.py

@@ -15,7 +15,8 @@
 import itertools
 
 from color import Coloring
-from command import DEFAULT_LOCAL_JOBS, PagedCommand
+from command import DEFAULT_LOCAL_JOBS
+from command import PagedCommand
 
 
 class Prune(PagedCommand):
@@ -82,9 +83,7 @@ class Prune(PagedCommand):
             )
 
             if not branch.base_exists:
-                print(
-                    "(ignoring: tracking branch is gone: %s)" % (branch.base,)
-                )
+                print(f"(ignoring: tracking branch is gone: {branch.base})")
             else:
                 commits = branch.commits
                 date = branch.date

subcmds/rebase.py

@@ -17,6 +17,10 @@ import sys
 from color import Coloring
 from command import Command
 from git_command import GitCommand
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
 
 
 class RebaseColoring(Coloring):
@@ -104,17 +108,15 @@ branch but need to incorporate new upstream changes "underneath" them.
         one_project = len(all_projects) == 1
 
         if opt.interactive and not one_project:
-            print(
-                "error: interactive rebase not supported with multiple "
-                "projects",
-                file=sys.stderr,
+            logger.error(
+                "error: interactive rebase not supported with multiple projects"
             )
+
             if len(args) == 1:
-                print(
-                    "note: project %s is mapped to more than one path"
-                    % (args[0],),
-                    file=sys.stderr,
+                logger.warning(
+                    "note: project %s is mapped to more than one path", args[0]
                 )
+
             return 1
 
         # Setup the common git rebase args that we use for all projects.
@@ -145,10 +147,9 @@ branch but need to incorporate new upstream changes "underneath" them.
             cb = project.CurrentBranch
             if not cb:
                 if one_project:
-                    print(
-                        "error: project %s has a detached HEAD"
-                        % _RelPath(project),
-                        file=sys.stderr,
+                    logger.error(
+                        "error: project %s has a detached HEAD",
+                        _RelPath(project),
                     )
                     return 1
                 # Ignore branches with detached HEADs.
@@ -157,10 +158,9 @@ branch but need to incorporate new upstream changes "underneath" them.
             upbranch = project.GetBranch(cb)
             if not upbranch.LocalMerge:
                 if one_project:
-                    print(
-                        "error: project %s does not track any remote branches"
-                        % _RelPath(project),
-                        file=sys.stderr,
+                    logger.error(
+                        "error: project %s does not track any remote branches",
+                        _RelPath(project),
                     )
                     return 1
                 # Ignore branches without remotes.

subcmds/selfupdate.py

@@ -12,12 +12,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from optparse import SUPPRESS_HELP
-import sys
+import optparse
 
-from command import Command, MirrorSafeCommand
-from subcmds.sync import _PostRepoUpgrade
+from command import Command
+from command import MirrorSafeCommand
+from error import RepoExitError
+from repo_logging import RepoLogger
 from subcmds.sync import _PostRepoFetch
+from subcmds.sync import _PostRepoUpgrade
+
+
+logger = RepoLogger(__file__)
+
+
+class SelfupdateError(RepoExitError):
+    """Exit error for failed selfupdate command."""
 
 
 class Selfupdate(Command, MirrorSafeCommand):
@@ -47,7 +56,7 @@ need to be performed by an end-user.
             "--repo-upgraded",
             dest="repo_upgraded",
             action="store_true",
-            help=SUPPRESS_HELP,
+            help=optparse.SUPPRESS_HELP,
         )
 
     def Execute(self, opt, args):
@@ -58,9 +67,10 @@ need to be performed by an end-user.
             _PostRepoUpgrade(self.manifest)
 
         else:
-            if not rp.Sync_NetworkHalf().success:
-                print("error: can't update repo", file=sys.stderr)
-                sys.exit(1)
+            result = rp.Sync_NetworkHalf()
+            if result.error:
+                logger.error("error: can't update repo")
+                raise SelfupdateError(aggregate_errors=[result.error])
 
             rp.bare_git.gc("--auto")
             _PostRepoFetch(rp, repo_verify=opt.repo_verify, verbose=True)

subcmds/stage.py

@@ -17,6 +17,10 @@ import sys
 from color import Coloring
 from command import InteractiveCommand
 from git_command import GitCommand
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
 
 
 class _ProjectList(Coloring):
@@ -62,7 +66,7 @@ The '%prog' command stages files to prepare the next commit.
             if p.IsDirty()
         ]
         if not all_projects:
-            print("no projects have uncommitted modifications", file=sys.stderr)
+            logger.error("no projects have uncommitted modifications")
             return
 
         out = _ProjectList(self.manifest.manifestProject.config)

subcmds/start.py

@@ -13,15 +13,28 @@
 # limitations under the License.
 
 import functools
-import os
-import sys
+from typing import NamedTuple
 
-from command import Command, DEFAULT_LOCAL_JOBS
-from git_config import IsImmutable
+from command import Command
+from command import DEFAULT_LOCAL_JOBS
+from error import RepoExitError
 from git_command import git
-import gitc_utils
+from git_config import IsImmutable
 from progress import Progress
-from project import SyncBuffer
+from project import Project
+from repo_logging import RepoLogger
+
+
+logger = RepoLogger(__file__)
+
+
+class ExecuteOneResult(NamedTuple):
+    project: Project
+    error: Exception
+
+
+class StartError(RepoExitError):
+    """Exit error for failed start command."""
 
 
 class Start(Command):
@@ -73,6 +86,7 @@ revision specified in the manifest.
         # a change, then we can't push back to it. Substitute with
         # dest_branch, if defined; or with manifest default revision instead.
         branch_merge = ""
+        error = None
         if IsImmutable(project.revisionExpr):
             if project.dest_branch:
                 branch_merge = project.dest_branch
@@ -80,19 +94,17 @@ revision specified in the manifest.
                 branch_merge = self.manifest.default.revisionExpr
 
         try:
-            ret = project.StartBranch(
+            project.StartBranch(
                 nb, branch_merge=branch_merge, revision=revision
             )
         except Exception as e:
-            print(
-                "error: unable to checkout %s: %s" % (project.name, e),
-                file=sys.stderr,
-            )
-            ret = False
-        return (ret, project)
+            logger.error("error: unable to checkout %s: %s", project.name, e)
+            error = e
+        return ExecuteOneResult(project, error)
 
     def Execute(self, opt, args):
         nb = args[0]
+        err_projects = []
         err = []
         projects = []
         if not opt.all:
@@ -102,53 +114,14 @@ revision specified in the manifest.
 
         all_projects = self.GetProjects(
             projects,
-            missing_ok=bool(self.gitc_manifest),
             all_manifests=not opt.this_manifest_only,
         )
 
-        # This must happen after we find all_projects, since GetProjects may
-        # need the local directory, which will disappear once we save the GITC
-        # manifest.
-        if self.gitc_manifest:
-            gitc_projects = self.GetProjects(
-                projects, manifest=self.gitc_manifest, missing_ok=True
-            )
-            for project in gitc_projects:
-                if project.old_revision:
-                    project.already_synced = True
-                else:
-                    project.already_synced = False
-                    project.old_revision = project.revisionExpr
-                project.revisionExpr = None
-            # Save the GITC manifest.
-            gitc_utils.save_manifest(self.gitc_manifest)
-
-            # Make sure we have a valid CWD.
-            if not os.path.exists(os.getcwd()):
-                os.chdir(self.manifest.topdir)
-
-            pm = Progress("Syncing %s" % nb, len(all_projects), quiet=opt.quiet)
-            for project in all_projects:
-                gitc_project = self.gitc_manifest.paths[project.relpath]
-                # Sync projects that have not been opened.
-                if not gitc_project.already_synced:
-                    proj_localdir = os.path.join(
-                        self.gitc_manifest.gitc_client_dir, project.relpath
-                    )
-                    project.worktree = proj_localdir
-                    if not os.path.exists(proj_localdir):
-                        os.makedirs(proj_localdir)
-                    project.Sync_NetworkHalf()
-                    sync_buf = SyncBuffer(self.manifest.manifestProject.config)
-                    project.Sync_LocalHalf(sync_buf)
-                    project.revisionId = gitc_project.old_revision
-                pm.update(msg="")
-            pm.end()
-
         def _ProcessResults(_pool, pm, results):
-            for result, project in results:
-                if not result:
-                    err.append(project)
+            for result in results:
+                if result.error:
+                    err_projects.append(result.project)
+                    err.append(result.error)
                 pm.update(msg="")
 
         self.ExecuteInParallel(
@@ -157,17 +130,19 @@ revision specified in the manifest.
             all_projects,
             callback=_ProcessResults,
             output=Progress(
-                "Starting %s" % (nb,), len(all_projects), quiet=opt.quiet
+                f"Starting {nb}", len(all_projects), quiet=opt.quiet
             ),
         )
 
-        if err:
-            for p in err:
-                print(
-                    "error: %s/: cannot start %s"
-                    % (p.RelPath(local=opt.this_manifest_only), nb),
-                    file=sys.stderr,
+        if err_projects:
+            for p in err_projects:
+                logger.error(
+                    "error: %s/: cannot start %s",
+                    p.RelPath(local=opt.this_manifest_only),
+                    nb,
                 )
             msg_fmt = "cannot start %d project(s)"
-            self.git_event_log.ErrorEvent(msg_fmt % (len(err)), msg_fmt)
-            sys.exit(1)
+            self.git_event_log.ErrorEvent(
+                msg_fmt % (len(err_projects)), msg_fmt
+            )
+            raise StartError(aggregate_errors=err)

subcmds/status.py

@@ -17,9 +17,9 @@ import glob
 import io
 import os
 
-from command import DEFAULT_LOCAL_JOBS, PagedCommand
-
 from color import Coloring
+from command import DEFAULT_LOCAL_JOBS
+from command import PagedCommand
 import platform_utils
 
 

subcmds/upload.py

@@ -19,16 +19,26 @@ import re
 import sys
 from typing import List
 
-from command import DEFAULT_LOCAL_JOBS, InteractiveCommand
+from command import DEFAULT_LOCAL_JOBS
+from command import InteractiveCommand
 from editor import Editor
+from error import GitError
+from error import SilentRepoExitError
 from error import UploadError
 from git_command import GitCommand
 from git_refs import R_HEADS
 from hooks import RepoHook
 from project import ReviewableBranch
+from repo_logging import RepoLogger
+from subcmds.sync import LocalSyncState
 
 
 _DEFAULT_UNUSUAL_COMMIT_THRESHOLD = 5
+logger = RepoLogger(__file__)
+
+
+class UploadExitError(SilentRepoExitError):
+    """Indicates that there is an upload command error requiring a sys exit."""
 
 
 def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool:
@@ -62,16 +72,16 @@ def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool:
     # If any branch has many commits, prompt the user.
     if many_commits:
         if len(branches) > 1:
-            print(
+            logger.warning(
                 "ATTENTION: One or more branches has an unusually high number "
                 "of commits."
             )
         else:
-            print(
+            logger.warning(
                 "ATTENTION: You are uploading an unusually high number of "
                 "commits."
             )
-        print(
+        logger.warning(
             "YOU PROBABLY DO NOT MEAN TO DO THIS. (Did you rebase across "
             "branches?)"
         )
@@ -85,8 +95,8 @@ def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool:
 
 def _die(fmt, *args):
     msg = fmt % args
-    print("error: %s" % msg, file=sys.stderr)
-    sys.exit(1)
+    logger.error("error: %s", msg)
+    raise UploadExitError(msg)
 
 
 def _SplitEmails(values):
@@ -528,137 +538,137 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
         except (AttributeError, IndexError):
             return ""
 
-    def _UploadAndReport(self, opt, todo, original_people):
+    def _UploadBranch(self, opt, branch, original_people):
+        """Upload Branch."""
+        people = copy.deepcopy(original_people)
+        self._AppendAutoList(branch, people)
+
+        # Check if there are local changes that may have been forgotten.
+        changes = branch.project.UncommitedFiles()
+        if opt.ignore_untracked_files:
+            untracked = set(branch.project.UntrackedFiles())
+            changes = [x for x in changes if x not in untracked]
+
+        if changes:
+            key = "review.%s.autoupload" % branch.project.remote.review
+            answer = branch.project.config.GetBoolean(key)
+
+            # If they want to auto upload, let's not ask because it
+            # could be automated.
+            if answer is None:
+                print()
+                print(
+                    "Uncommitted changes in %s (did you forget to "
+                    "amend?):" % branch.project.name
+                )
+                print("\n".join(changes))
+                print("Continue uploading? (y/N) ", end="", flush=True)
+                if opt.yes:
+                    print("<--yes>")
+                    a = "yes"
+                else:
+                    a = sys.stdin.readline().strip().lower()
+                if a not in ("y", "yes", "t", "true", "on"):
+                    print("skipping upload", file=sys.stderr)
+                    branch.uploaded = False
+                    branch.error = "User aborted"
+                    return
+
+        # Check if topic branches should be sent to the server during
+        # upload.
+        if opt.auto_topic is not True:
+            key = "review.%s.uploadtopic" % branch.project.remote.review
+            opt.auto_topic = branch.project.config.GetBoolean(key)
+
+        def _ExpandCommaList(value):
+            """Split |value| up into comma delimited entries."""
+            if not value:
+                return
+            for ret in value.split(","):
+                ret = ret.strip()
+                if ret:
+                    yield ret
+
+        # Check if hashtags should be included.
+        key = "review.%s.uploadhashtags" % branch.project.remote.review
+        hashtags = set(_ExpandCommaList(branch.project.config.GetString(key)))
+        for tag in opt.hashtags:
+            hashtags.update(_ExpandCommaList(tag))
+        if opt.hashtag_branch:
+            hashtags.add(branch.name)
+
+        # Check if labels should be included.
+        key = "review.%s.uploadlabels" % branch.project.remote.review
+        labels = set(_ExpandCommaList(branch.project.config.GetString(key)))
+        for label in opt.labels:
+            labels.update(_ExpandCommaList(label))
+
+        # Handle e-mail notifications.
+        if opt.notify is False:
+            notify = "NONE"
+        else:
+            key = "review.%s.uploadnotify" % branch.project.remote.review
+            notify = branch.project.config.GetString(key)
+
+        destination = opt.dest_branch or branch.project.dest_branch
+
+        if branch.project.dest_branch and not opt.dest_branch:
+            merge_branch = self._GetMergeBranch(
+                branch.project, local_branch=branch.name
+            )
+
+            full_dest = destination
+            if not full_dest.startswith(R_HEADS):
+                full_dest = R_HEADS + full_dest
+
+            # If the merge branch of the local branch is different from
+            # the project's revision AND destination, this might not be
+            # intentional.
+            if (
+                merge_branch
+                and merge_branch != branch.project.revisionExpr
+                and merge_branch != full_dest
+            ):
+                print(
+                    f"For local branch {branch.name}: merge branch "
+                    f"{merge_branch} does not match destination branch "
+                    f"{destination}"
+                )
+                print("skipping upload.")
+                print(
+                    f"Please use `--destination {destination}` if this "
+                    "is intentional"
+                )
+                branch.uploaded = False
+                return
+
+        branch.UploadForReview(
+            people,
+            dryrun=opt.dryrun,
+            auto_topic=opt.auto_topic,
+            hashtags=hashtags,
+            labels=labels,
+            private=opt.private,
+            notify=notify,
+            wip=opt.wip,
+            ready=opt.ready,
+            dest_branch=destination,
+            validate_certs=opt.validate_certs,
+            push_options=opt.push_options,
+        )
+
+        branch.uploaded = True
+
+    def _UploadAndReport(self, opt, todo, people):
         have_errors = False
+        aggregate_errors = []
         for branch in todo:
             try:
-                people = copy.deepcopy(original_people)
-                self._AppendAutoList(branch, people)
-
-                # Check if there are local changes that may have been forgotten.
-                changes = branch.project.UncommitedFiles()
-                if opt.ignore_untracked_files:
-                    untracked = set(branch.project.UntrackedFiles())
-                    changes = [x for x in changes if x not in untracked]
-
-                if changes:
-                    key = "review.%s.autoupload" % branch.project.remote.review
-                    answer = branch.project.config.GetBoolean(key)
-
-                    # If they want to auto upload, let's not ask because it
-                    # could be automated.
-                    if answer is None:
-                        print()
-                        print(
-                            "Uncommitted changes in %s (did you forget to "
-                            "amend?):" % branch.project.name
-                        )
-                        print("\n".join(changes))
-                        print("Continue uploading? (y/N) ", end="", flush=True)
-                        if opt.yes:
-                            print("<--yes>")
-                            a = "yes"
-                        else:
-                            a = sys.stdin.readline().strip().lower()
-                        if a not in ("y", "yes", "t", "true", "on"):
-                            print("skipping upload", file=sys.stderr)
-                            branch.uploaded = False
-                            branch.error = "User aborted"
-                            continue
-
-                # Check if topic branches should be sent to the server during
-                # upload.
-                if opt.auto_topic is not True:
-                    key = "review.%s.uploadtopic" % branch.project.remote.review
-                    opt.auto_topic = branch.project.config.GetBoolean(key)
-
-                def _ExpandCommaList(value):
-                    """Split |value| up into comma delimited entries."""
-                    if not value:
-                        return
-                    for ret in value.split(","):
-                        ret = ret.strip()
-                        if ret:
-                            yield ret
-
-                # Check if hashtags should be included.
-                key = "review.%s.uploadhashtags" % branch.project.remote.review
-                hashtags = set(
-                    _ExpandCommaList(branch.project.config.GetString(key))
-                )
-                for tag in opt.hashtags:
-                    hashtags.update(_ExpandCommaList(tag))
-                if opt.hashtag_branch:
-                    hashtags.add(branch.name)
-
-                # Check if labels should be included.
-                key = "review.%s.uploadlabels" % branch.project.remote.review
-                labels = set(
-                    _ExpandCommaList(branch.project.config.GetString(key))
-                )
-                for label in opt.labels:
-                    labels.update(_ExpandCommaList(label))
-
-                # Handle e-mail notifications.
-                if opt.notify is False:
-                    notify = "NONE"
-                else:
-                    key = (
-                        "review.%s.uploadnotify" % branch.project.remote.review
-                    )
-                    notify = branch.project.config.GetString(key)
-
-                destination = opt.dest_branch or branch.project.dest_branch
-
-                if branch.project.dest_branch and not opt.dest_branch:
-                    merge_branch = self._GetMergeBranch(
-                        branch.project, local_branch=branch.name
-                    )
-
-                    full_dest = destination
-                    if not full_dest.startswith(R_HEADS):
-                        full_dest = R_HEADS + full_dest
-
-                    # If the merge branch of the local branch is different from
-                    # the project's revision AND destination, this might not be
-                    # intentional.
-                    if (
-                        merge_branch
-                        and merge_branch != branch.project.revisionExpr
-                        and merge_branch != full_dest
-                    ):
-                        print(
-                            f"For local branch {branch.name}: merge branch "
-                            f"{merge_branch} does not match destination branch "
-                            f"{destination}"
-                        )
-                        print("skipping upload.")
-                        print(
-                            f"Please use `--destination {destination}` if this "
-                            "is intentional"
-                        )
-                        branch.uploaded = False
-                        continue
-
-                branch.UploadForReview(
-                    people,
-                    dryrun=opt.dryrun,
-                    auto_topic=opt.auto_topic,
-                    hashtags=hashtags,
-                    labels=labels,
-                    private=opt.private,
-                    notify=notify,
-                    wip=opt.wip,
-                    ready=opt.ready,
-                    dest_branch=destination,
-                    validate_certs=opt.validate_certs,
-                    push_options=opt.push_options,
-                )
-
-                branch.uploaded = True
-            except UploadError as e:
+                self._UploadBranch(opt, branch, people)
+            except (UploadError, GitError) as e:
                 self.git_event_log.ErrorEvent(f"upload error: {e}")
                 branch.error = e
+                aggregate_errors.append(e)
                 branch.uploaded = False
                 have_errors = True
 
@@ -697,7 +707,7 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
                 )
 
         if have_errors:
-            sys.exit(1)
+            raise UploadExitError(aggregate_errors=aggregate_errors)
 
     def _GetMergeBranch(self, project, local_branch=None):
         if local_branch is None:
@@ -740,16 +750,13 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             for result in results:
                 project, avail = result
                 if avail is None:
-                    print(
+                    logger.error(
                         'repo: error: %s: Unable to upload branch "%s". '
                         "You might be able to fix the branch by running:\n"
-                        "  git branch --set-upstream-to m/%s"
-                        % (
-                            project.RelPath(local=opt.this_manifest_only),
-                            project.CurrentBranch,
-                            project.manifest.branch,
-                        ),
-                        file=sys.stderr,
+                        "  git branch --set-upstream-to m/%s",
+                        project.RelPath(local=opt.this_manifest_only),
+                        project.CurrentBranch,
+                        project.manifest.branch,
                     )
                 elif avail:
                     pending.append(result)
@@ -764,14 +771,11 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
 
         if not pending:
             if opt.branch is None:
-                print(
-                    "repo: error: no branches ready for upload", file=sys.stderr
-                )
+                logger.error("repo: error: no branches ready for upload")
             else:
-                print(
-                    'repo: error: no branches named "%s" ready for upload'
-                    % (opt.branch,),
-                    file=sys.stderr,
+                logger.error(
+                    'repo: error: no branches named "%s" ready for upload',
+                    opt.branch,
                 )
             return 1
 
@@ -800,6 +804,11 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             if not hook.Run(
                 project_list=pending_proj_names, worktree_list=pending_worktrees
             ):
+                if LocalSyncState(manifest).IsPartiallySynced():
+                    logger.error(
+                        "Partially synced tree detected. Syncing all projects "
+                        "may resolve issues you're seeing."
+                    )
                 ret = 1
         if ret:
             return ret

subcmds/version.py

@@ -15,8 +15,11 @@
 import platform
 import sys
 
-from command import Command, MirrorSafeCommand
-from git_command import git, RepoSourceVersion, user_agent
+from command import Command
+from command import MirrorSafeCommand
+from git_command import git
+from git_command import RepoSourceVersion
+from git_command import user_agent
 from git_refs import HEAD
 from wrapper import Wrapper
 
@@ -39,35 +42,28 @@ class Version(Command, MirrorSafeCommand):
         # These might not be the same.  Report them both.
         src_ver = RepoSourceVersion()
         rp_ver = rp.bare_git.describe(HEAD)
-        print("repo version %s" % rp_ver)
-        print("       (from %s)" % rem.url)
-        print("       (tracking %s)" % branch.merge)
-        print("       (%s)" % rp.bare_git.log("-1", "--format=%cD", HEAD))
+        print(f"repo version {rp_ver}")
+        print(f"       (from {rem.url})")
+        print(f"       (tracking {branch.merge})")
+        print(f"       ({rp.bare_git.log('-1', '--format=%cD', HEAD)})")
 
         if self.wrapper_path is not None:
-            print("repo launcher version %s" % self.wrapper_version)
-            print("       (from %s)" % self.wrapper_path)
+            print(f"repo launcher version {self.wrapper_version}")
+            print(f"       (from {self.wrapper_path})")
 
             if src_ver != rp_ver:
-                print("       (currently at %s)" % src_ver)
+                print(f"       (currently at {src_ver})")
 
-        print("repo User-Agent %s" % user_agent.repo)
-        print("git %s" % git.version_tuple().full)
-        print("git User-Agent %s" % user_agent.git)
-        print("Python %s" % sys.version)
+        print(f"repo User-Agent {user_agent.repo}")
+        print(f"git {git.version_tuple().full}")
+        print(f"git User-Agent {user_agent.git}")
+        print(f"Python {sys.version}")
         uname = platform.uname()
         if sys.version_info.major < 3:
             # Python 3 returns a named tuple, but Python 2 is simpler.
             print(uname)
         else:
-            print(
-                "OS %s %s (%s)" % (uname.system, uname.release, uname.version)
-            )
-            print(
-                "CPU %s (%s)"
-                % (
-                    uname.machine,
-                    uname.processor if uname.processor else "unknown",
-                )
-            )
+            print(f"OS {uname.system} {uname.release} ({uname.version})")
+            processor = uname.processor if uname.processor else "unknown"
+            print(f"CPU {uname.machine} ({processor})")
         print("Bug reports:", Wrapper().BUG_URL)

tests/conftest.py

@@ -14,8 +14,11 @@
 
 """Common fixtures for pytests."""
 
+import pathlib
+
 import pytest
 
+import platform_utils
 import repo_trace
 
 
@@ -23,3 +26,49 @@ import repo_trace
 def disable_repo_trace(tmp_path):
     """Set an environment marker to relax certain strict checks for test code."""  # noqa: E501
     repo_trace._TRACE_FILE = str(tmp_path / "TRACE_FILE_from_test")
+
+
+# adapted from pytest-home 0.5.1
+def _set_home(monkeypatch, path: pathlib.Path):
+    """
+    Set the home dir using a pytest monkeypatch context.
+    """
+    win = platform_utils.isWindows()
+    vars = ["HOME"] + win * ["USERPROFILE"]
+    for var in vars:
+        monkeypatch.setenv(var, str(path))
+    return path
+
+
+# copied from
+# https://github.com/pytest-dev/pytest/issues/363#issuecomment-1335631998
+@pytest.fixture(scope="session")
+def monkeysession():
+    with pytest.MonkeyPatch.context() as mp:
+        yield mp
+
+
+@pytest.fixture(autouse=True, scope="session")
+def session_tmp_home_dir(tmp_path_factory, monkeysession):
+    """Set HOME to a temporary directory, avoiding user's .gitconfig.
+
+    b/302797407
+
+    Set home at session scope to take effect prior to
+    ``test_wrapper.GitCheckoutTestCase.setUpClass``.
+    """
+    return _set_home(monkeysession, tmp_path_factory.mktemp("home"))
+
+
+# adapted from pytest-home 0.5.1
+@pytest.fixture(autouse=True)
+def tmp_home_dir(monkeypatch, tmp_path_factory):
+    """Set HOME to a temporary directory.
+
+    Ensures that state doesn't accumulate in $HOME across tests.
+
+    Note that in conjunction with session_tmp_homedir, the HOME
+    dir is patched twice, once at session scope, and then again at
+    the function scope.
+    """
+    return _set_home(monkeypatch, tmp_path_factory.mktemp("home"))

tests/test_error.py

@@ -18,7 +18,21 @@ import inspect
 import pickle
 import unittest
 
+import command
 import error
+import fetch
+import git_command
+import project
+from subcmds import all_modules
+
+
+imports = all_modules + [
+    error,
+    project,
+    git_command,
+    fetch,
+    command,
+]
 
 
 class PickleTests(unittest.TestCase):
@@ -26,10 +40,11 @@ class PickleTests(unittest.TestCase):
 
     def getExceptions(self):
         """Return all our custom exceptions."""
-        for name in dir(error):
-            cls = getattr(error, name)
-            if isinstance(cls, type) and issubclass(cls, Exception):
-                yield cls
+        for entry in imports:
+            for name in dir(entry):
+                cls = getattr(entry, name)
+                if isinstance(cls, type) and issubclass(cls, Exception):
+                    yield cls
 
     def testExceptionLookup(self):
         """Make sure our introspection logic works."""

tests/test_git_command.py

@@ -14,14 +14,12 @@
 
 """Unittests for the git_command.py module."""
 
-import re
+import io
 import os
+import re
+import subprocess
 import unittest
-
-try:
-    from unittest import mock
-except ImportError:
-    import mock
+from unittest import mock
 
 import git_command
 import wrapper
@@ -65,6 +63,169 @@ class GitCommandTest(unittest.TestCase):
         )
 
 
+class GitCommandWaitTest(unittest.TestCase):
+    """Tests the GitCommand class .Wait()"""
+
+    def setUp(self):
+        class MockPopen:
+            rc = 0
+
+            def __init__(self):
+                self.stdout = io.BufferedReader(io.BytesIO())
+                self.stderr = io.BufferedReader(io.BytesIO())
+
+            def communicate(
+                self, input: str = None, timeout: float = None
+            ) -> [str, str]:
+                """Mock communicate fn."""
+                return ["", ""]
+
+            def wait(self, timeout=None):
+                return self.rc
+
+        self.popen = popen = MockPopen()
+
+        def popen_mock(*args, **kwargs):
+            return popen
+
+        def realpath_mock(val):
+            return val
+
+        mock.patch.object(subprocess, "Popen", side_effect=popen_mock).start()
+
+        mock.patch.object(
+            os.path, "realpath", side_effect=realpath_mock
+        ).start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_raises_when_verify_non_zero_result(self):
+        self.popen.rc = 1
+        r = git_command.GitCommand(None, ["status"], verify_command=True)
+        with self.assertRaises(git_command.GitCommandError):
+            r.Wait()
+
+    def test_returns_when_no_verify_non_zero_result(self):
+        self.popen.rc = 1
+        r = git_command.GitCommand(None, ["status"], verify_command=False)
+        self.assertEqual(1, r.Wait())
+
+    def test_default_returns_non_zero_result(self):
+        self.popen.rc = 1
+        r = git_command.GitCommand(None, ["status"])
+        self.assertEqual(1, r.Wait())
+
+
+class GitCommandStreamLogsTest(unittest.TestCase):
+    """Tests the GitCommand class stderr log streaming cases."""
+
+    def setUp(self):
+        self.mock_process = mock.MagicMock()
+        self.mock_process.communicate.return_value = (None, None)
+        self.mock_process.wait.return_value = 0
+
+        self.mock_popen = mock.MagicMock()
+        self.mock_popen.return_value = self.mock_process
+        mock.patch("subprocess.Popen", self.mock_popen).start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_does_not_stream_logs_when_input_is_set(self):
+        git_command.GitCommand(None, ["status"], input="foo")
+
+        self.mock_popen.assert_called_once_with(
+            ["git", "status"],
+            cwd=None,
+            env=mock.ANY,
+            encoding="utf-8",
+            errors="backslashreplace",
+            stdin=subprocess.PIPE,
+            stdout=None,
+            stderr=None,
+        )
+        self.mock_process.communicate.assert_called_once_with(input="foo")
+        self.mock_process.stderr.read1.assert_not_called()
+
+    def test_does_not_stream_logs_when_stdout_is_set(self):
+        git_command.GitCommand(None, ["status"], capture_stdout=True)
+
+        self.mock_popen.assert_called_once_with(
+            ["git", "status"],
+            cwd=None,
+            env=mock.ANY,
+            encoding="utf-8",
+            errors="backslashreplace",
+            stdin=None,
+            stdout=subprocess.PIPE,
+            stderr=None,
+        )
+        self.mock_process.communicate.assert_called_once_with(input=None)
+        self.mock_process.stderr.read1.assert_not_called()
+
+    def test_does_not_stream_logs_when_stderr_is_set(self):
+        git_command.GitCommand(None, ["status"], capture_stderr=True)
+
+        self.mock_popen.assert_called_once_with(
+            ["git", "status"],
+            cwd=None,
+            env=mock.ANY,
+            encoding="utf-8",
+            errors="backslashreplace",
+            stdin=None,
+            stdout=None,
+            stderr=subprocess.PIPE,
+        )
+        self.mock_process.communicate.assert_called_once_with(input=None)
+        self.mock_process.stderr.read1.assert_not_called()
+
+    def test_does_not_stream_logs_when_merge_output_is_set(self):
+        git_command.GitCommand(None, ["status"], merge_output=True)
+
+        self.mock_popen.assert_called_once_with(
+            ["git", "status"],
+            cwd=None,
+            env=mock.ANY,
+            encoding="utf-8",
+            errors="backslashreplace",
+            stdin=None,
+            stdout=None,
+            stderr=subprocess.STDOUT,
+        )
+        self.mock_process.communicate.assert_called_once_with(input=None)
+        self.mock_process.stderr.read1.assert_not_called()
+
+    @mock.patch("sys.stderr")
+    def test_streams_stderr_when_no_stream_is_set(self, mock_stderr):
+        logs = "\n".join(
+            [
+                "Enumerating objects: 5, done.",
+                "Counting objects: 100% (5/5), done.",
+                "Writing objects: 100% (3/3), 330 bytes | 330 KiB/s, done.",
+                "remote: Processing changes: refs: 1, new: 1, done ",
+                "remote: SUCCESS",
+            ]
+        )
+        self.mock_process.stderr = io.BufferedReader(
+            io.BytesIO(bytes(logs, "utf-8"))
+        )
+
+        cmd = git_command.GitCommand(None, ["push"])
+
+        self.mock_popen.assert_called_once_with(
+            ["git", "push"],
+            cwd=None,
+            env=mock.ANY,
+            stdin=None,
+            stdout=None,
+            stderr=subprocess.PIPE,
+        )
+        self.mock_process.communicate.assert_not_called()
+        mock_stderr.write.assert_called_once_with(logs)
+        self.assertEqual(cmd.stderr, logs)
+
+
 class GitCallUnitTest(unittest.TestCase):
     """Tests the _GitCall class (via git_command.git)."""
 
@@ -153,12 +314,31 @@ class GitRequireTests(unittest.TestCase):
 
     def test_older_fatal(self):
         """Test fatal require calls with old versions."""
-        with self.assertRaises(SystemExit) as e:
+        with self.assertRaises(git_command.GitRequireError) as e:
             git_command.git_require((2,), fail=True)
             self.assertNotEqual(0, e.code)
 
     def test_older_fatal_msg(self):
         """Test fatal require calls with old versions and message."""
-        with self.assertRaises(SystemExit) as e:
+        with self.assertRaises(git_command.GitRequireError) as e:
             git_command.git_require((2,), fail=True, msg="so sad")
             self.assertNotEqual(0, e.code)
+
+
+class GitCommandErrorTest(unittest.TestCase):
+    """Test for the GitCommandError class."""
+
+    def test_augument_stderr(self):
+        self.assertEqual(
+            git_command.GitCommandError(
+                git_stderr="couldn't find remote ref refs/heads/foo"
+            ).suggestion,
+            "Check if the provided ref exists in the remote.",
+        )
+
+        self.assertEqual(
+            git_command.GitCommandError(
+                git_stderr="'foobar' does not appear to be a git repository"
+            ).suggestion,
+            "Are you running this repo command outside of a repo workspace?",
+        )

tests/test_git_config.py

@@ -100,7 +100,7 @@ class GitConfigReadOnlyTests(unittest.TestCase):
             ("intg", 10737418240),
         )
         for key, value in TESTS:
-            self.assertEqual(value, self.config.GetInt("section.%s" % (key,)))
+            self.assertEqual(value, self.config.GetInt(f"section.{key}"))
 
 
 class GitConfigReadWriteTests(unittest.TestCase):

tests/test_git_superproject.py

@@ -21,10 +21,11 @@ import tempfile
 import unittest
 from unittest import mock
 
+from test_manifest_xml import sort_attributes
+
 import git_superproject
 import git_trace2_event_log
 import manifest_xml
-from test_manifest_xml import sort_attributes
 
 
 class SuperprojectTestCase(unittest.TestCase):
@@ -33,7 +34,7 @@ class SuperprojectTestCase(unittest.TestCase):
     PARENT_SID_KEY = "GIT_TRACE2_PARENT_SID"
     PARENT_SID_VALUE = "parent_sid"
     SELF_SID_REGEX = r"repo-\d+T\d+Z-.*"
-    FULL_SID_REGEX = r"^%s/%s" % (PARENT_SID_VALUE, SELF_SID_REGEX)
+    FULL_SID_REGEX = rf"^{PARENT_SID_VALUE}/{SELF_SID_REGEX}"
 
     def setUp(self):
         """Set up superproject every time."""
@@ -107,7 +108,9 @@ class SuperprojectTestCase(unittest.TestCase):
             self.assertRegex(log_entry["sid"], self.FULL_SID_REGEX)
         else:
             self.assertRegex(log_entry["sid"], self.SELF_SID_REGEX)
-        self.assertRegex(log_entry["time"], r"^\d+-\d+-\d+T\d+:\d+:\d+\.\d+Z$")
+        self.assertRegex(
+            log_entry["time"], r"^\d+-\d+-\d+T\d+:\d+:\d+\.\d+\+00:00$"
+        )
 
     def readLog(self, log_path):
         """Helper function to read log data into a list."""
@@ -246,7 +249,7 @@ class SuperprojectTestCase(unittest.TestCase):
         os.mkdir(self._superproject._superproject_path)
         manifest_path = self._superproject._WriteManifestFile()
         self.assertIsNotNone(manifest_path)
-        with open(manifest_path, "r") as fp:
+        with open(manifest_path) as fp:
             manifest_xml_data = fp.read()
         self.assertEqual(
             sort_attributes(manifest_xml_data),
@@ -281,7 +284,7 @@ class SuperprojectTestCase(unittest.TestCase):
                     )
                     self.assertIsNotNone(update_result.manifest_path)
                     self.assertFalse(update_result.fatal)
-                    with open(update_result.manifest_path, "r") as fp:
+                    with open(update_result.manifest_path) as fp:
                         manifest_xml_data = fp.read()
                     self.assertEqual(
                         sort_attributes(manifest_xml_data),
@@ -368,7 +371,7 @@ class SuperprojectTestCase(unittest.TestCase):
                     )
                     self.assertIsNotNone(update_result.manifest_path)
                     self.assertFalse(update_result.fatal)
-                    with open(update_result.manifest_path, "r") as fp:
+                    with open(update_result.manifest_path) as fp:
                         manifest_xml_data = fp.read()
                     # Verify platform/vendor/x's project revision hasn't
                     # changed.
@@ -433,7 +436,7 @@ class SuperprojectTestCase(unittest.TestCase):
                     )
                     self.assertIsNotNone(update_result.manifest_path)
                     self.assertFalse(update_result.fatal)
-                    with open(update_result.manifest_path, "r") as fp:
+                    with open(update_result.manifest_path) as fp:
                         manifest_xml_data = fp.read()
                     # Verify platform/vendor/x's project revision hasn't
                     # changed.
@@ -489,7 +492,9 @@ class SuperprojectTestCase(unittest.TestCase):
 
                     self.assertTrue(self._superproject._Fetch())
                     self.assertEqual(
-                        mock_git_command.call_args.args,
+                        # TODO: Once we require Python 3.8+,
+                        #  use 'mock_git_command.call_args.args'.
+                        mock_git_command.call_args[0],
                         (
                             None,
                             [
@@ -509,7 +514,9 @@ class SuperprojectTestCase(unittest.TestCase):
                     # If branch for revision exists, set as --negotiation-tip.
                     self.assertTrue(self._superproject._Fetch())
                     self.assertEqual(
-                        mock_git_command.call_args.args,
+                        # TODO: Once we require Python 3.8+,
+                        #  use 'mock_git_command.call_args.args'.
+                        mock_git_command.call_args[0],
                         (
                             None,
                             [

tests/test_git_trace2_event_log.py

@@ -61,7 +61,7 @@ class EventLogTestCase(unittest.TestCase):
     PARENT_SID_KEY = "GIT_TRACE2_PARENT_SID"
     PARENT_SID_VALUE = "parent_sid"
     SELF_SID_REGEX = r"repo-\d+T\d+Z-.*"
-    FULL_SID_REGEX = r"^%s/%s" % (PARENT_SID_VALUE, SELF_SID_REGEX)
+    FULL_SID_REGEX = rf"^{PARENT_SID_VALUE}/{SELF_SID_REGEX}"
 
     def setUp(self):
         """Load the event_log module every time."""
@@ -90,7 +90,9 @@ class EventLogTestCase(unittest.TestCase):
             self.assertRegex(log_entry["sid"], self.FULL_SID_REGEX)
         else:
             self.assertRegex(log_entry["sid"], self.SELF_SID_REGEX)
-        self.assertRegex(log_entry["time"], r"^\d+-\d+-\d+T\d+:\d+:\d+\.\d+Z$")
+        self.assertRegex(
+            log_entry["time"], r"^\d+-\d+-\d+T\d+:\d+:\d+\.\d+\+00:00$"
+        )
 
     def readLog(self, log_path):
         """Helper function to read log data into a list."""
@@ -338,8 +340,8 @@ class EventLogTestCase(unittest.TestCase):
         # Check for 'error' event specific fields.
         self.assertIn("msg", error_event)
         self.assertIn("fmt", error_event)
-        self.assertEqual(error_event["msg"], msg)
-        self.assertEqual(error_event["fmt"], fmt)
+        self.assertEqual(error_event["msg"], f"RepoErrorEvent:{msg}")
+        self.assertEqual(error_event["fmt"], f"RepoErrorEvent:{fmt}")
 
     def test_write_with_filename(self):
         """Test Write() with a path to a file exits with None."""

tests/test_hooks.py

@@ -14,9 +14,10 @@
 
 """Unittests for the hooks.py module."""
 
-import hooks
 import unittest
 
+import hooks
+
 
 class RepoHookShebang(unittest.TestCase):
     """Check shebang parsing in RepoHook."""

tests/test_manifest_xml.py

@@ -198,13 +198,13 @@ class ValueTests(unittest.TestCase):
     def test_bool_true(self):
         """Check XmlBool true values."""
         for value in ("yes", "true", "1"):
-            node = self._get_node('<node a="%s"/>' % (value,))
+            node = self._get_node(f'<node a="{value}"/>')
             self.assertTrue(manifest_xml.XmlBool(node, "a"))
 
     def test_bool_false(self):
         """Check XmlBool false values."""
         for value in ("no", "false", "0"):
-            node = self._get_node('<node a="%s"/>' % (value,))
+            node = self._get_node(f'<node a="{value}"/>')
             self.assertFalse(manifest_xml.XmlBool(node, "a"))
 
     def test_int_default(self):
@@ -220,7 +220,7 @@ class ValueTests(unittest.TestCase):
     def test_int_good(self):
         """Check XmlInt numeric handling."""
         for value in (-1, 0, 1, 50000):
-            node = self._get_node('<node a="%s"/>' % (value,))
+            node = self._get_node(f'<node a="{value}"/>')
             self.assertEqual(value, manifest_xml.XmlInt(node, "a"))
 
     def test_int_invalid(self):
@@ -385,6 +385,21 @@ class XmlManifestTests(ManifestParseTestCase):
             "</manifest>",
         )
 
+    def test_parse_with_xml_doctype(self):
+        """Check correct manifest parse with DOCTYPE node present."""
+        manifest = self.getXmlManifest(
+            """<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE manifest []>
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <project name="test-project" path="src/test-project"/>
+</manifest>
+"""
+        )
+        self.assertEqual(len(manifest.projects), 1)
+        self.assertEqual(manifest.projects[0].name, "test-project")
+
 
 class IncludeElementTests(ManifestParseTestCase):
     """Tests for <include>."""
@@ -1113,3 +1128,32 @@ class ExtendProjectElementTests(ManifestParseTestCase):
         )
         self.assertEqual(len(manifest.projects), 1)
         self.assertEqual(manifest.projects[0].upstream, "bar")
+
+
+class NormalizeUrlTests(ManifestParseTestCase):
+    """Tests for normalize_url() in manifest_xml.py"""
+
+    def test_has_trailing_slash(self):
+        url = "http://foo.com/bar/baz/"
+        self.assertEqual(
+            "http://foo.com/bar/baz", manifest_xml.normalize_url(url)
+        )
+
+    def test_has_no_scheme(self):
+        """Deal with cases where we have no scheme, but we also
+        aren't dealing with the git SCP-like syntax
+        """
+        url = "foo.com/baf/bat"
+        self.assertEqual(url, manifest_xml.normalize_url(url))
+
+        url = "git@foo.com/baf/bat"
+        self.assertEqual(url, manifest_xml.normalize_url(url))
+
+        url = "/file/path/here"
+        self.assertEqual(url, manifest_xml.normalize_url(url))
+
+    def test_has_no_scheme_matches_scp_like_syntax(self):
+        url = "git@foo.com:bar/baf"
+        self.assertEqual(
+            "ssh://git@foo.com/bar/baf", manifest_xml.normalize_url(url)
+        )

tests/test_project.py

@@ -22,9 +22,9 @@ import tempfile
 import unittest
 
 import error
-import manifest_xml
 import git_command
 import git_config
+import manifest_xml
 import platform_utils
 import project
 
@@ -48,7 +48,7 @@ def TempGitTree():
         yield tempdir
 
 
-class FakeProject(object):
+class FakeProject:
     """A fake for Project for basic functionality."""
 
     def __init__(self, worktree):
@@ -151,7 +151,7 @@ class CopyLinkTestCase(unittest.TestCase):
                     # "".
                     break
                 result = os.path.exists(path)
-                msg.append("\tos.path.exists(%s): %s" % (path, result))
+                msg.append(f"\tos.path.exists({path}): {result}")
                 if result:
                     msg.append("\tcontents: %r" % os.listdir(path))
                     break
@@ -507,7 +507,10 @@ class ManifestPropertiesFetchedCorrectly(unittest.TestCase):
             self.assertFalse(fakeproj.partial_clone)
 
             fakeproj.config.SetString("repo.depth", "48")
-            self.assertEqual(fakeproj.depth, "48")
+            self.assertEqual(fakeproj.depth, 48)
+
+            fakeproj.config.SetString("repo.depth", "invalid_depth")
+            self.assertEqual(fakeproj.depth, None)
 
             fakeproj.config.SetString("repo.clonefilter", "blob:limit=10M")
             self.assertEqual(fakeproj.clone_filter, "blob:limit=10M")

tests/test_repo_logging.py

@@ -0,0 +1,64 @@
+# Copyright (C) 2023 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unit test for repo_logging module."""
+import unittest
+from unittest import mock
+
+from error import RepoExitError
+from repo_logging import RepoLogger
+
+
+class TestRepoLogger(unittest.TestCase):
+    @mock.patch.object(RepoLogger, "error")
+    def test_log_aggregated_errors_logs_aggregated_errors(self, mock_error):
+        """Test if log_aggregated_errors logs a list of aggregated errors."""
+        logger = RepoLogger(__name__)
+        logger.log_aggregated_errors(
+            RepoExitError(
+                aggregate_errors=[
+                    Exception("foo"),
+                    Exception("bar"),
+                    Exception("baz"),
+                    Exception("hello"),
+                    Exception("world"),
+                    Exception("test"),
+                ]
+            )
+        )
+
+        mock_error.assert_has_calls(
+            [
+                mock.call("=" * 80),
+                mock.call(
+                    "Repo command failed due to the following `%s` errors:",
+                    "RepoExitError",
+                ),
+                mock.call("foo\nbar\nbaz\nhello\nworld"),
+                mock.call("+%d additional errors...", 1),
+            ]
+        )
+
+    @mock.patch.object(RepoLogger, "error")
+    def test_log_aggregated_errors_logs_single_error(self, mock_error):
+        """Test if log_aggregated_errors logs empty aggregated_errors."""
+        logger = RepoLogger(__name__)
+        logger.log_aggregated_errors(RepoExitError())
+
+        mock_error.assert_has_calls(
+            [
+                mock.call("=" * 80),
+                mock.call("Repo command failed: %s", "RepoExitError"),
+            ]
+        )

tests/test_subcmds_sync.py

@@ -14,12 +14,18 @@
 """Unittests for the subcmds/sync.py module."""
 
 import os
+import shutil
+import tempfile
+import time
 import unittest
 from unittest import mock
 
 import pytest
 
 import command
+from error import GitError
+from error import RepoExitError
+from project import SyncNetworkHalfResult
 from subcmds import sync
 
 
@@ -104,6 +110,200 @@ def test_cli_jobs(argv, jobs_manifest, jobs, jobs_net, jobs_check):
             assert opts.jobs_checkout == jobs_check
 
 
+class LocalSyncState(unittest.TestCase):
+    """Tests for LocalSyncState."""
+
+    _TIME = 10
+
+    def setUp(self):
+        """Common setup."""
+        self.topdir = tempfile.mkdtemp("LocalSyncState")
+        self.repodir = os.path.join(self.topdir, ".repo")
+        os.makedirs(self.repodir)
+
+        self.manifest = mock.MagicMock(
+            topdir=self.topdir,
+            repodir=self.repodir,
+            repoProject=mock.MagicMock(relpath=".repo/repo"),
+        )
+        self.state = self._new_state()
+
+    def tearDown(self):
+        """Common teardown."""
+        shutil.rmtree(self.topdir)
+
+    def _new_state(self, time=_TIME):
+        with mock.patch("time.time", return_value=time):
+            return sync.LocalSyncState(self.manifest)
+
+    def test_set(self):
+        """Times are set."""
+        p = mock.MagicMock(relpath="projA")
+        self.state.SetFetchTime(p)
+        self.state.SetCheckoutTime(p)
+        self.assertEqual(self.state.GetFetchTime(p), self._TIME)
+        self.assertEqual(self.state.GetCheckoutTime(p), self._TIME)
+
+    def test_update(self):
+        """Times are updated."""
+        with open(self.state._path, "w") as f:
+            f.write(
+                """
+            {
+              "projB": {
+                "last_fetch": 5,
+                "last_checkout": 7
+              }
+            }
+            """
+            )
+
+        # Initialize state to read from the new file.
+        self.state = self._new_state()
+        projA = mock.MagicMock(relpath="projA")
+        projB = mock.MagicMock(relpath="projB")
+        self.assertEqual(self.state.GetFetchTime(projA), None)
+        self.assertEqual(self.state.GetFetchTime(projB), 5)
+        self.assertEqual(self.state.GetCheckoutTime(projB), 7)
+
+        self.state.SetFetchTime(projA)
+        self.state.SetFetchTime(projB)
+        self.assertEqual(self.state.GetFetchTime(projA), self._TIME)
+        self.assertEqual(self.state.GetFetchTime(projB), self._TIME)
+        self.assertEqual(self.state.GetCheckoutTime(projB), 7)
+
+    def test_save_to_file(self):
+        """Data is saved under repodir."""
+        p = mock.MagicMock(relpath="projA")
+        self.state.SetFetchTime(p)
+        self.state.Save()
+        self.assertEqual(
+            os.listdir(self.repodir), [".repo_localsyncstate.json"]
+        )
+
+    def test_partial_sync(self):
+        """Partial sync state is detected."""
+        with open(self.state._path, "w") as f:
+            f.write(
+                """
+            {
+              "projA": {
+                "last_fetch": 5,
+                "last_checkout": 5
+              },
+              "projB": {
+                "last_fetch": 5,
+                "last_checkout": 5
+              }
+            }
+            """
+            )
+
+        # Initialize state to read from the new file.
+        self.state = self._new_state()
+        projB = mock.MagicMock(relpath="projB")
+        self.assertEqual(self.state.IsPartiallySynced(), False)
+
+        self.state.SetFetchTime(projB)
+        self.state.SetCheckoutTime(projB)
+        self.assertEqual(self.state.IsPartiallySynced(), True)
+
+    def test_ignore_repo_project(self):
+        """Sync data for repo project is ignored when checking partial sync."""
+        p = mock.MagicMock(relpath="projA")
+        self.state.SetFetchTime(p)
+        self.state.SetCheckoutTime(p)
+        self.state.SetFetchTime(self.manifest.repoProject)
+        self.state.Save()
+        self.assertEqual(self.state.IsPartiallySynced(), False)
+
+        self.state = self._new_state(self._TIME + 1)
+        self.state.SetFetchTime(self.manifest.repoProject)
+        self.assertEqual(
+            self.state.GetFetchTime(self.manifest.repoProject), self._TIME + 1
+        )
+        self.assertEqual(self.state.GetFetchTime(p), self._TIME)
+        self.assertEqual(self.state.IsPartiallySynced(), False)
+
+    def test_nonexistent_project(self):
+        """Unsaved projects don't have data."""
+        p = mock.MagicMock(relpath="projC")
+        self.assertEqual(self.state.GetFetchTime(p), None)
+        self.assertEqual(self.state.GetCheckoutTime(p), None)
+
+    def test_prune_removed_projects(self):
+        """Removed projects are pruned."""
+        with open(self.state._path, "w") as f:
+            f.write(
+                """
+            {
+              "projA": {
+                "last_fetch": 5
+              },
+              "projB": {
+                "last_fetch": 7
+              }
+            }
+            """
+            )
+
+        def mock_exists(path):
+            if "projA" in path:
+                return False
+            return True
+
+        projA = mock.MagicMock(relpath="projA")
+        projB = mock.MagicMock(relpath="projB")
+        self.state = self._new_state()
+        self.assertEqual(self.state.GetFetchTime(projA), 5)
+        self.assertEqual(self.state.GetFetchTime(projB), 7)
+        with mock.patch("os.path.exists", side_effect=mock_exists):
+            self.state.PruneRemovedProjects()
+        self.assertIsNone(self.state.GetFetchTime(projA))
+
+        self.state = self._new_state()
+        self.assertIsNone(self.state.GetFetchTime(projA))
+        self.assertEqual(self.state.GetFetchTime(projB), 7)
+
+    def test_prune_removed_and_symlinked_projects(self):
+        """Removed projects that still exists on disk as symlink are pruned."""
+        with open(self.state._path, "w") as f:
+            f.write(
+                """
+            {
+              "projA": {
+                "last_fetch": 5
+              },
+              "projB": {
+                "last_fetch": 7
+              }
+            }
+            """
+            )
+
+        def mock_exists(path):
+            return True
+
+        def mock_islink(path):
+            if "projB" in path:
+                return True
+            return False
+
+        projA = mock.MagicMock(relpath="projA")
+        projB = mock.MagicMock(relpath="projB")
+        self.state = self._new_state()
+        self.assertEqual(self.state.GetFetchTime(projA), 5)
+        self.assertEqual(self.state.GetFetchTime(projB), 7)
+        with mock.patch("os.path.exists", side_effect=mock_exists):
+            with mock.patch("os.path.islink", side_effect=mock_islink):
+                self.state.PruneRemovedProjects()
+        self.assertIsNone(self.state.GetFetchTime(projB))
+
+        self.state = self._new_state()
+        self.assertIsNone(self.state.GetFetchTime(projB))
+        self.assertEqual(self.state.GetFetchTime(projA), 5)
+
+
 class GetPreciousObjectsState(unittest.TestCase):
     """Tests for _GetPreciousObjectsState."""
 
@@ -158,3 +358,83 @@ class GetPreciousObjectsState(unittest.TestCase):
         self.assertFalse(
             self.cmd._GetPreciousObjectsState(self.project, self.opt)
         )
+
+
+class SyncCommand(unittest.TestCase):
+    """Tests for cmd.Execute."""
+
+    def setUp(self):
+        """Common setup."""
+        self.repodir = tempfile.mkdtemp(".repo")
+        self.manifest = manifest = mock.MagicMock(
+            repodir=self.repodir,
+        )
+
+        git_event_log = mock.MagicMock(ErrorEvent=mock.Mock(return_value=None))
+        self.outer_client = outer_client = mock.MagicMock()
+        outer_client.manifest.IsArchive = True
+        manifest.manifestProject.worktree = "worktree_path/"
+        manifest.repoProject.LastFetch = time.time()
+        self.sync_network_half_error = None
+        self.sync_local_half_error = None
+        self.cmd = sync.Sync(
+            manifest=manifest,
+            outer_client=outer_client,
+            git_event_log=git_event_log,
+        )
+
+        def Sync_NetworkHalf(*args, **kwargs):
+            return SyncNetworkHalfResult(True, self.sync_network_half_error)
+
+        def Sync_LocalHalf(*args, **kwargs):
+            if self.sync_local_half_error:
+                raise self.sync_local_half_error
+
+        self.project = p = mock.MagicMock(
+            use_git_worktrees=False,
+            UseAlternates=False,
+            name="project",
+            Sync_NetworkHalf=Sync_NetworkHalf,
+            Sync_LocalHalf=Sync_LocalHalf,
+            RelPath=mock.Mock(return_value="rel_path"),
+        )
+        p.manifest.GetProjectsWithName.return_value = [p]
+
+        mock.patch.object(
+            sync,
+            "_PostRepoFetch",
+            return_value=None,
+        ).start()
+
+        mock.patch.object(
+            self.cmd, "GetProjects", return_value=[self.project]
+        ).start()
+
+        opt, _ = self.cmd.OptionParser.parse_args([])
+        opt.clone_bundle = False
+        opt.jobs = 4
+        opt.quiet = True
+        opt.use_superproject = False
+        opt.current_branch_only = True
+        opt.optimized_fetch = True
+        opt.retry_fetches = 1
+        opt.prune = False
+        opt.auto_gc = False
+        opt.repo_verify = False
+        self.opt = opt
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_command_exit_error(self):
+        """Ensure unsuccessful commands raise expected errors."""
+        self.sync_network_half_error = GitError(
+            "sync_network_half_error error", project=self.project
+        )
+        self.sync_local_half_error = GitError(
+            "sync_local_half_error", project=self.project
+        )
+        with self.assertRaises(RepoExitError) as e:
+            self.cmd.Execute(self.opt, [])
+            self.assertIn(self.sync_local_half_error, e.aggregate_errors)
+            self.assertIn(self.sync_network_half_error, e.aggregate_errors)

tests/test_subcmds_upload.py

@@ -0,0 +1,70 @@
+# Copyright (C) 2023 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the subcmds/upload.py module."""
+
+import unittest
+from unittest import mock
+
+from error import GitError
+from error import UploadError
+from subcmds import upload
+
+
+class UnexpectedError(Exception):
+    """An exception not expected by upload command."""
+
+
+class UploadCommand(unittest.TestCase):
+    """Check registered all_commands."""
+
+    def setUp(self):
+        self.cmd = upload.Upload()
+        self.branch = mock.MagicMock()
+        self.people = mock.MagicMock()
+        self.opt, _ = self.cmd.OptionParser.parse_args([])
+        mock.patch.object(
+            self.cmd, "_AppendAutoList", return_value=None
+        ).start()
+        mock.patch.object(self.cmd, "git_event_log").start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_UploadAndReport_UploadError(self):
+        """Check UploadExitError raised when UploadError encountered."""
+        side_effect = UploadError("upload error")
+        with mock.patch.object(
+            self.cmd, "_UploadBranch", side_effect=side_effect
+        ):
+            with self.assertRaises(upload.UploadExitError):
+                self.cmd._UploadAndReport(self.opt, [self.branch], self.people)
+
+    def test_UploadAndReport_GitError(self):
+        """Check UploadExitError raised when GitError encountered."""
+        side_effect = GitError("some git error")
+        with mock.patch.object(
+            self.cmd, "_UploadBranch", side_effect=side_effect
+        ):
+            with self.assertRaises(upload.UploadExitError):
+                self.cmd._UploadAndReport(self.opt, [self.branch], self.people)
+
+    def test_UploadAndReport_UnhandledError(self):
+        """Check UnexpectedError passed through."""
+        side_effect = UnexpectedError("some os error")
+        with mock.patch.object(
+            self.cmd, "_UploadBranch", side_effect=side_effect
+        ):
+            with self.assertRaises(type(side_effect)):
+                self.cmd._UploadAndReport(self.opt, [self.branch], self.people)

tests/test_wrapper.py

@@ -14,7 +14,7 @@
 
 """Unittests for the wrapper.py module."""
 
-from io import StringIO
+import io
 import os
 import re
 import sys
@@ -47,8 +47,10 @@ class RepoWrapperUnitTest(RepoWrapperTestCase):
     def test_version(self):
         """Make sure _Version works."""
         with self.assertRaises(SystemExit) as e:
-            with mock.patch("sys.stdout", new_callable=StringIO) as stdout:
-                with mock.patch("sys.stderr", new_callable=StringIO) as stderr:
+            with mock.patch("sys.stdout", new_callable=io.StringIO) as stdout:
+                with mock.patch(
+                    "sys.stderr", new_callable=io.StringIO
+                ) as stderr:
                     self.wrapper._Version()
         self.assertEqual(0, e.exception.code)
         self.assertEqual("", stderr.getvalue())
@@ -76,11 +78,9 @@ class RepoWrapperUnitTest(RepoWrapperTestCase):
         self.assertIsNone(opts.manifest_url)
 
     def test_gitc_init_parser(self):
-        """Make sure 'gitc-init' GetParser works."""
-        parser = self.wrapper.GetParser(gitc_init=True)
-        opts, args = parser.parse_args([])
-        self.assertEqual([], args)
-        self.assertIsNone(opts.manifest_file)
+        """Make sure 'gitc-init' GetParser raises."""
+        with self.assertRaises(SystemExit):
+            self.wrapper.GetParser(gitc_init=True)
 
     def test_get_gitc_manifest_dir_no_gitc(self):
         """
@@ -418,7 +418,7 @@ class SetupGnuPG(RepoWrapperTestCase):
                 self.wrapper.home_dot_repo, "gnupg"
             )
             self.assertTrue(self.wrapper.SetupGnuPG(True))
-            with open(os.path.join(tempdir, "keyring-version"), "r") as fp:
+            with open(os.path.join(tempdir, "keyring-version")) as fp:
                 data = fp.read()
             self.assertEqual(
                 ".".join(str(x) for x in self.wrapper.KEYRING_VERSION),

tox.ini

@@ -15,7 +15,8 @@
 # https://tox.readthedocs.io/
 
 [tox]
-envlist = lint, py36, py37, py38, py39, py310
+envlist = lint, py36, py37, py38, py39, py310, py311, py312
+requires = virtualenv<20.22.0
 
 [gh-actions]
 python =
@@ -24,11 +25,14 @@ python =
     3.8: py38
     3.9: py39
     3.10: py310
+    3.11: py311
+    3.12: py312
 
 [testenv]
 deps =
     black
     flake8
+    isort
     pytest
     pytest-timeout
 commands = {envpython} run_tests {posargs}
@@ -54,6 +58,3 @@ deps =
 commands =
     black {posargs:.}
     flake8
-
-[pytest]
-timeout = 300