Update errors to extend BaseRepoError

In order to better analyze and track repo errors, repo command failures
need to be tied to specific errors in repo source code.

Additionally a new GitCommandError was added to differentiate between
general git related errors to failed git commands. Git commands that opt
into verification will raise a GitCommandError if the command failed.

The first step in this process is a general error refactoring

Bug: b/293344017
Change-Id: I46944b1825ce892757c8dd3f7e2fab7e460760c0
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/380994
Commit-Queue: Jason Chang <jasonnc@google.com>
Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
Tested-by: Jason Chang <jasonnc@google.com>
Reviewed-by: Joanna Wang <jojwang@google.com>

5 files changed, 218 insertions, 33 deletions

diff --git a/error.py b/error.py
index 3cf34d54e..ed4a90b78 100644
--- a/error.py
+++ b/error.py
@@ -12,8 +12,51 @@
 # 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 ManifestParseError(RepoExitError):
     """Failed to parse the manifest file."""
 
 
@@ -25,11 +68,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 +80,64 @@ 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 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 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 +146,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 +159,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 +170,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.
diff --git a/git_command.py b/git_command.py
index c7245adef..588a64fd0 100644
--- a/git_command.py
+++ b/git_command.py
@@ -40,6 +40,10 @@ GIT_DIR = "GIT_DIR"
 
 LAST_GITDIR = None
 LAST_CWD = None
+DEFAULT_GIT_FAIL_MESSAGE = "git command failure"
+# Common line length limit
+GIT_ERROR_STDOUT_LINES = 1
+GIT_ERROR_STDERR_LINES = 1
 
 
 class _GitCall(object):
@@ -237,6 +241,7 @@ class GitCommand(object):
         cwd=None,
         gitdir=None,
         objdir=None,
+        verify_command=False,
     ):
         if project:
             if not cwd:
@@ -244,6 +249,10 @@ class GitCommand(object):
             if not gitdir:
                 gitdir = project.gitdir
 
+        self.project = project
+        self.cmdv = cmdv
+        self.verify_command = verify_command
+
         # Git on Windows wants its paths only using / for reliability.
         if platform_utils.isWindows():
             if objdir:
@@ -332,7 +341,11 @@ class GitCommand(object):
                     stderr=stderr,
                 )
             except Exception as e:
-                raise GitError("%s: %s" % (command[1], e))
+                raise GitCommandError(
+                    message="%s: %s" % (command[1], e),
+                    project=project.name if project else None,
+                    command_args=cmdv,
+                )
 
             if ssh_proxy:
                 ssh_proxy.add_client(p)
@@ -366,4 +379,61 @@ class GitCommand(object):
         return env
 
     def Wait(self):
-        return self.rc
+        if not self.verify_command or self.rc == 0:
+            return self.rc
+
+        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,
+        )
+
+
+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.
+    """
+
+    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
+
+    def __str__(self):
+        args = "[]" if not self.command_args else " ".join(self.command_args)
+        error_type = type(self).__name__
+        return f"""{error_type}: {self.message}
+    Project: {self.project}
+    Args: {args}
+    Stdout:
+{self.git_stdout}
+    Stderr:
+{self.git_stderr}"""
diff --git a/subcmds/__init__.py b/subcmds/__init__.py
index 4e41afc03..0754f708a 100644
--- a/subcmds/__init__.py
+++ b/subcmds/__init__.py
@@ -16,6 +16,7 @@ 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):
@@ -42,6 +43,7 @@ for py in os.listdir(my_dir):
         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"]
diff --git a/tests/test_error.py b/tests/test_error.py
index 784e2d57c..2733ab8c3 100644
--- a/tests/test_error.py
+++ b/tests/test_error.py
@@ -19,6 +19,15 @@ import pickle
 import unittest
 
 import error
+import project
+import git_command
+from subcmds import all_modules
+
+imports = all_modules + [
+    error,
+    project,
+    git_command,
+]
 
 
 class PickleTests(unittest.TestCase):
@@ -26,10 +35,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."""
diff --git a/tests/test_git_command.py b/tests/test_git_command.py
index c4c3a4c5b..1e8beabcc 100644
--- a/tests/test_git_command.py
+++ b/tests/test_git_command.py
@@ -16,6 +16,7 @@
 
 import re
 import os
+import subprocess
 import unittest
 
 try:
@@ -65,6 +66,56 @@ class GitCommandTest(unittest.TestCase):
         )
 
 
+class GitCommandWaitTest(unittest.TestCase):
+    """Tests the GitCommand class .Wait()"""
+
+    def setUp(self):
+        class MockPopen(object):
+            rc = 0
+
+            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 GitCallUnitTest(unittest.TestCase):
     """Tests the _GitCall class (via git_command.git)."""