From f9aacd4087b02948da9a7878da48ea186ab99d5a Mon Sep 17 00:00:00 2001 From: Jason Chang Date: Thu, 3 Aug 2023 14:38:00 -0700 Subject: [PATCH] Raise repo exit errors in place of sys.exit Bug: b/293344017 Change-Id: I92d81c78eba8ff31b5252415f4c9a515a6c76411 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/381774 Tested-by: Jason Chang Reviewed-by: Joanna Wang Commit-Queue: Jason Chang --- command.py | 8 ++++++-- fetch.py | 9 ++++++++- git_command.py | 21 +++++++++++++++------ project.py | 22 ++++++++++------------ subcmds/abandon.py | 24 +++++++++++++++++++----- subcmds/init.py | 12 +++++++++--- subcmds/upload.py | 10 +++++++--- tests/test_error.py | 4 ++++ tests/test_git_command.py | 4 ++-- 9 files changed, 80 insertions(+), 34 deletions(-) diff --git a/command.py b/command.py index 939a4630..d2129381 100644 --- a/command.py +++ b/command.py @@ -16,11 +16,11 @@ import multiprocessing import os import optparse import re -import sys from event_log import EventLog from error import NoSuchProjectError from error import InvalidProjectGroupsError +from error import RepoExitError import progress @@ -42,6 +42,10 @@ WORKER_BATCH_SIZE = 32 DEFAULT_LOCAL_JOBS = min(os.cpu_count(), 8) +class UsageError(RepoExitError): + """Exception thrown with invalid command usage.""" + + class Command(object): """Base class for any command line action in repo.""" @@ -215,7 +219,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.""" diff --git a/fetch.py b/fetch.py index 31f8152f..cf8d1d7d 100644 --- a/fetch.py +++ b/fetch.py @@ -18,6 +18,11 @@ import subprocess 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): @@ -29,6 +34,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 +47,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() diff --git a/git_command.py b/git_command.py index 588a64fd..36fcfe7c 100644 --- a/git_command.py +++ b/git_command.py @@ -19,6 +19,7 @@ import subprocess from typing import Any, Optional from error import GitError +from error import RepoExitError from git_refs import HEAD import platform_utils from repo_trace import REPO_TRACE, IsTrace, Trace @@ -44,6 +45,7 @@ DEFAULT_GIT_FAIL_MESSAGE = "git command failure" # Common line length limit GIT_ERROR_STDOUT_LINES = 1 GIT_ERROR_STDERR_LINES = 1 +INVALID_GIT_EXIT_CODE = 126 class _GitCall(object): @@ -51,8 +53,9 @@ class _GitCall(object): 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" + print(msg, file=sys.stderr) + raise GitRequireError(msg) return ret def __getattr__(self, name): @@ -167,10 +170,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 = "fatal: git %s or later required%s" % (need, msg) + print(error_msg, file=sys.stderr) + raise GitRequireError(error_msg) return False @@ -403,6 +405,13 @@ class GitCommand(object): ) +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. diff --git a/project.py b/project.py index b268007d..c3eb09c9 100644 --- a/project.py +++ b/project.py @@ -2003,8 +2003,8 @@ class Project(object): name: The name of the branch to abandon. Returns: - True if the abandon succeeded; False if it didn't; None if the - branch didn't exist. + True if the abandon succeeded; Raises GitCommandError if it didn't; + None if the branch didn't exist. """ rev = R_HEADS + name all_refs = self.bare_ref.all @@ -2025,16 +2025,14 @@ class Project(object): ) else: self._Checkout(revid, quiet=True) - - return ( - GitCommand( - self, - ["branch", "-D", name], - capture_stdout=True, - capture_stderr=True, - ).Wait() - == 0 - ) + GitCommand( + self, + ["branch", "-D", name], + capture_stdout=True, + capture_stderr=True, + verify_command=True, + ).Wait() + return True def PruneHeads(self): """Prune any topic branches already merged into upstream.""" diff --git a/subcmds/abandon.py b/subcmds/abandon.py index ded287f6..896b348f 100644 --- a/subcmds/abandon.py +++ b/subcmds/abandon.py @@ -20,6 +20,11 @@ import sys from command import Command, DEFAULT_LOCAL_JOBS from git_command import git from progress import Progress +from error import RepoError, RepoExitError + + +class AbandonError(RepoExitError): + """Exit error when abandon command fails.""" class Abandon(Command): @@ -68,28 +73,37 @@ It is equivalent to "git branch -D ". 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) + 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( @@ -116,13 +130,13 @@ It is equivalent to "git branch -D ". " " * len(err_msg) + " | %s" % _RelPath(proj), file=sys.stderr, ) - sys.exit(1) + 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) + raise AbandonError(aggregate_errors=aggregate_errors) else: # Everything below here is displaying status. if opt.quiet: diff --git a/subcmds/init.py b/subcmds/init.py index 6d7fd857..868d339e 100644 --- a/subcmds/init.py +++ b/subcmds/init.py @@ -19,6 +19,8 @@ from color import Coloring from command import InteractiveCommand, MirrorSafeCommand from git_command import git_require, MIN_GIT_VERSION_SOFT, MIN_GIT_VERSION_HARD from wrapper import Wrapper +from error import UpdateManifestError +from error import RepoUnhandledExceptionError _REPO_ALLOW_SHALLOW = os.environ.get("REPO_ALLOW_SHALLOW") @@ -156,7 +158,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) @@ -346,14 +351,15 @@ to update the working directory files. 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, ) 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) diff --git a/subcmds/upload.py b/subcmds/upload.py index 8d949bef..d0c028b9 100644 --- a/subcmds/upload.py +++ b/subcmds/upload.py @@ -21,7 +21,7 @@ from typing import List from command import DEFAULT_LOCAL_JOBS, InteractiveCommand from editor import Editor -from error import UploadError +from error import UploadError, RepoExitError from git_command import GitCommand from git_refs import R_HEADS from hooks import RepoHook @@ -31,6 +31,10 @@ from project import ReviewableBranch _DEFAULT_UNUSUAL_COMMIT_THRESHOLD = 5 +class UploadExitError(RepoExitError): + """Indicates that there is an upload command error requiring a sys exit.""" + + def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool: """Perform basic safety checks on the given set of branches. @@ -86,7 +90,7 @@ def _VerifyPendingCommits(branches: List[ReviewableBranch]) -> bool: def _die(fmt, *args): msg = fmt % args print("error: %s" % msg, file=sys.stderr) - sys.exit(1) + raise UploadExitError(msg) def _SplitEmails(values): @@ -697,7 +701,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/ ) if have_errors: - sys.exit(1) + raise branch.error def _GetMergeBranch(self, project, local_branch=None): if local_branch is None: diff --git a/tests/test_error.py b/tests/test_error.py index 2733ab8c..2b28f5c2 100644 --- a/tests/test_error.py +++ b/tests/test_error.py @@ -21,12 +21,16 @@ import unittest import error import project import git_command +import fetch +import command from subcmds import all_modules imports = all_modules + [ error, project, git_command, + fetch, + command, ] diff --git a/tests/test_git_command.py b/tests/test_git_command.py index 1e8beabc..3dd31b29 100644 --- a/tests/test_git_command.py +++ b/tests/test_git_command.py @@ -204,12 +204,12 @@ 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)