upload: fix error handling

There was a bug in error handeling code that caused an uncaught
exception to be raised.

Bug: b/296316540
Change-Id: I49c72f29c00f26ba60de552f958bc6eddf841162
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/383254
Reviewed-by: Mike Frysinger <vapier@google.com>
Commit-Queue: Jason Chang <jasonnc@google.com>
Tested-by: Jason Chang <jasonnc@google.com>

3 files changed, 200 insertions, 129 deletions

diff --git a/project.py b/project.py
index 6e6a605e..a3b6312e 100644
--- a/project.py
+++ b/project.py
@@ -1116,7 +1116,8 @@ class Project(object):
             if not re.match(r"^.+[+-][0-9]+$", label):
                 raise UploadError(
                     f'invalid label syntax "{label}": labels use forms like '
-                    "CodeReview+1 or Verified-1"
+                    "CodeReview+1 or Verified-1",
+                    project=self.name,
                 )
 
         if dest_branch is None:
@@ -1132,7 +1133,7 @@ class Project(object):
 
         url = branch.remote.ReviewUrl(self.UserEmail, validate_certs)
         if url is None:
-            raise UploadError("review not configured")
+            raise UploadError("review not configured", project=self.name)
         cmd = ["push"]
         if dryrun:
             cmd.append("-n")
@@ -1177,8 +1178,9 @@ class Project(object):
             ref_spec = ref_spec + "%" + ",".join(opts)
         cmd.append(ref_spec)
 
-        if GitCommand(self, cmd, bare=True).Wait() != 0:
-            raise UploadError("Upload failed")
+        GitCommand(
+            self, cmd, bare=True, capture_stderr=True, verify_command=True
+        ).Wait()
 
         if not dryrun:
             msg = "posted to %s for %s" % (branch.remote.review, dest_branch)
diff --git a/subcmds/upload.py b/subcmds/upload.py
index d0c028b9..040eaeb5 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, RepoExitError
+from error import UploadError, SilentRepoExitError, GitError
 from git_command import GitCommand
 from git_refs import R_HEADS
 from hooks import RepoHook
@@ -31,7 +31,7 @@ from project import ReviewableBranch
 _DEFAULT_UNUSUAL_COMMIT_THRESHOLD = 5
 
 
-class UploadExitError(RepoExitError):
+class UploadExitError(SilentRepoExitError):
     """Indicates that there is an upload command error requiring a sys exit."""
 
 
@@ -532,137 +532,137 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
         except (AttributeError, IndexError):
             return ""
 
-    def _UploadAndReport(self, opt, todo, original_people):
-        have_errors = False
-        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))
+    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
                 )
-                for label in opt.labels:
-                    labels.update(_ExpandCommaList(label))
-
-                # Handle e-mail notifications.
-                if opt.notify is False:
-                    notify = "NONE"
+                print("\n".join(changes))
+                print("Continue uploading? (y/N) ", end="", flush=True)
+                if opt.yes:
+                    print("<--yes>")
+                    a = "yes"
                 else:
-                    key = (
-                        "review.%s.uploadnotify" % branch.project.remote.review
-                    )
-                    notify = branch.project.config.GetString(key)
+                    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
+        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
-                    )
+        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,
+            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
 
-                branch.uploaded = True
-            except UploadError as e:
+    def _UploadAndReport(self, opt, todo, people):
+        have_errors = False
+        aggregate_errors = []
+        for branch in todo:
+            try:
+                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
 
@@ -701,7 +701,7 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
                 )
 
         if have_errors:
-            raise branch.error
+            raise UploadExitError(aggregate_errors=aggregate_errors)
 
     def _GetMergeBranch(self, project, local_branch=None):
         if local_branch is None:
diff --git a/tests/test_subcmds_upload.py b/tests/test_subcmds_upload.py
new file mode 100644
index 00000000..75811996
--- /dev/null
+++ b/tests/test_subcmds_upload.py
@@ -0,0 +1,69 @@
+# 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 subcmds import upload
+from error import UploadError, GitError
+
+
+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)