mirror of
https://gerrit.googlesource.com/git-repo
synced 2024-12-21 07:16:21 +00:00
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>
This commit is contained in:
Jason Chang
LUCI
parent
11cb96030e
commit
5a3a5f7cec
10
project.py
10
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)
|
||||
|
||||
@ -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,10 +532,8 @@ 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:
|
||||
def _UploadBranch(self, opt, branch, original_people):
|
||||
"""Upload Branch."""
|
||||
people = copy.deepcopy(original_people)
|
||||
self._AppendAutoList(branch, people)
|
||||
|
||||
@ -568,7 +566,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
print("skipping upload", file=sys.stderr)
|
||||
branch.uploaded = False
|
||||
branch.error = "User aborted"
|
||||
continue
|
||||
return
|
||||
|
||||
# Check if topic branches should be sent to the server during
|
||||
# upload.
|
||||
@ -587,9 +585,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
|
||||
# Check if hashtags should be included.
|
||||
key = "review.%s.uploadhashtags" % branch.project.remote.review
|
||||
hashtags = set(
|
||||
_ExpandCommaList(branch.project.config.GetString(key))
|
||||
)
|
||||
hashtags = set(_ExpandCommaList(branch.project.config.GetString(key)))
|
||||
for tag in opt.hashtags:
|
||||
hashtags.update(_ExpandCommaList(tag))
|
||||
if opt.hashtag_branch:
|
||||
@ -597,9 +593,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
|
||||
# Check if labels should be included.
|
||||
key = "review.%s.uploadlabels" % branch.project.remote.review
|
||||
labels = set(
|
||||
_ExpandCommaList(branch.project.config.GetString(key))
|
||||
)
|
||||
labels = set(_ExpandCommaList(branch.project.config.GetString(key)))
|
||||
for label in opt.labels:
|
||||
labels.update(_ExpandCommaList(label))
|
||||
|
||||
@ -607,9 +601,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
if opt.notify is False:
|
||||
notify = "NONE"
|
||||
else:
|
||||
key = (
|
||||
"review.%s.uploadnotify" % branch.project.remote.review
|
||||
)
|
||||
key = "review.%s.uploadnotify" % branch.project.remote.review
|
||||
notify = branch.project.config.GetString(key)
|
||||
|
||||
destination = opt.dest_branch or branch.project.dest_branch
|
||||
@ -642,7 +634,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
"is intentional"
|
||||
)
|
||||
branch.uploaded = False
|
||||
continue
|
||||
return
|
||||
|
||||
branch.UploadForReview(
|
||||
people,
|
||||
@ -660,9 +652,17 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
||||
)
|
||||
|
||||
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:
|
||||
|
||||
69
tests/test_subcmds_upload.py
Normal file
69
tests/test_subcmds_upload.py
Normal file
@ -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)
|
||||
Loading…
Reference in New Issue
Block a user