git cl patch: handle missing/wrong Gerrit issue better.

No stacktrace is printed, instead just a user-friendly string is shown.
Add test + some tests refactoring.

This CL also improve missing Gerrit issue exception elsewhere by raising
user-friendly exception.

BUG=654360
R=sergiyb@chromium.org
TEST=manual

Review-Url: https://codereview.chromium.org/2403793002

git_cl.py

@@ -977,6 +977,17 @@ def ParseIssueNumberArgument(arg):
   return fail_result
 
 
+class GerritIssueNotExists(Exception):
+  def __init__(self, issue, url):
+    self.issue = issue
+    self.url = url
+    super(GerritIssueNotExists, self).__init__()
+
+  def __str__(self):
+    return 'issue %s at %s does not exist or you have no access to it' % (
+        self.issue, self.url)
+
+
 class Changelist(object):
   """Changelist works with one changelist in local branch.
 
@@ -2258,8 +2269,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       * 'unsent'   - no reviewers added
       * 'waiting'  - waiting for review
       * 'reply'    - waiting for owner to reply to review
-      * 'not lgtm' - Code-Review -2 from at least one approved reviewer
-      * 'lgtm'     - Code-Review +2 from at least one approved reviewer
+      * 'not lgtm' - Code-Review disaproval from at least one valid reviewer
+      * 'lgtm'     - Code-Review approval from at least one valid reviewer
       * 'commit'   - in the commit queue
       * 'closed'   - abandoned
     """
@@ -2268,7 +2279,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
     try:
       data = self._GetChangeDetail(['DETAILED_LABELS', 'CURRENT_REVISION'])
-    except httplib.HTTPException:
+    except (httplib.HTTPException, GerritIssueNotExists):
       return 'error'
 
     if data['status'] in ('ABANDONED', 'MERGED'):
@@ -2343,9 +2354,12 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
   def _GetChangeDetail(self, options=None, issue=None):
     options = options or []
     issue = issue or self.GetIssue()
-    assert issue, 'issue required to query Gerrit'
-    return gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue),
+    assert issue, 'issue is required to query Gerrit'
+    data = gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue),
                                        options)
+    if not data:
+      raise GerritIssueNotExists(issue, self.GetCodereviewServer())
+    return data
 
   def CMDLand(self, force, bypass_hooks, verbose):
     if git_common.is_dirty_git_tree('land'):
@@ -2400,7 +2414,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       self._gerrit_host = parsed_issue_arg.hostname
       self._gerrit_server = 'https://%s' % self._gerrit_host
 
+    try:
       detail = self._GetChangeDetail(['ALL_REVISIONS'])
+    except GerritIssueNotExists as e:
+      DieWithError(str(e))
 
     if not parsed_issue_arg.patchset:
       # Use current revision by default.

tests/git_cl_test.py

@@ -137,6 +137,11 @@ class MockChangelistWithBranchAndIssue():
   def GetIssue(self):
     return self.issue
 
+
+class SystemExitMock(Exception):
+  pass
+
+
 class TestGitClBasic(unittest.TestCase):
   def _test_ParseIssueUrl(self, func, url, issue, patchset, hostname, fail):
     parsed = urlparse.urlparse(url)
@@ -271,6 +276,8 @@ class TestGitCl(TestCase):
     self.mock(git_cl.auth, 'get_authenticator_for_host', AuthenticatorMock)
     self.mock(git_cl.gerrit_util.GceAuthenticator, 'is_gce',
               classmethod(lambda _: False))
+    self.mock(git_cl, 'DieWithError',
+              lambda msg: self._mocked_call(['DieWithError', msg]))
     # It's important to reset settings to not have inter-tests interference.
     git_cl.settings = None
 
@@ -706,23 +713,21 @@ class TestGitCl(TestCase):
   def test_reviewer_send_mail_no_rev(self):
     # Fails without a reviewer.
     stdout = StringIO.StringIO()
-    stderr = StringIO.StringIO()
-    try:
-      self.calls = self._upload_no_rev_calls(None, None)
+    self.calls = self._upload_no_rev_calls(None, None) + [
+        ((['DieWithError', 'Must specify reviewers to send email.'],),
+          SystemExitMock())
+    ]
+
     def RunEditor(desc, _, **kwargs):
       return desc
     self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
     self.mock(sys, 'stdout', stdout)
-      self.mock(sys, 'stderr', stderr)
+    with self.assertRaises(SystemExitMock):
       git_cl.main(['upload', '--send-mail'])
-      self.fail()
-    except SystemExit:
     self.assertEqual(
         'Using 50% similarity for rename/copy detection. Override with '
         '--similarity.\n',
         stdout.getvalue())
-      self.assertEqual(
-          'Must specify reviewers to send email.\n', stderr.getvalue())
 
   def test_bug_on_cmd(self):
     self._run_reviewer_test(
@@ -1399,10 +1404,6 @@ class TestGitCl(TestCase):
 
   def test_gerrit_patch_conflict(self):
     self._patch_common(is_gerrit=True)
-    self.mock(git_cl, 'DieWithError',
-              lambda msg: self._mocked_call(['DieWithError', msg]))
-    class SystemExitMock(Exception):
-      pass
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
@@ -1414,6 +1415,22 @@ class TestGitCl(TestCase):
       git_cl.main(['patch',
                    'https://chromium-review.googlesource.com/#/c/123456/1'])
 
+  def test_gerrit_patch_not_exists(self):
+    url = 'https://chromium-review.googlesource.com'
+    self.mock(git_cl.gerrit_util, 'GetChangeDetail', lambda _, __, ___: None)
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),
+      ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
+      ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+      ((['git', 'config', 'gerrit.host'],), 'true'),
+      ((['DieWithError', 'issue 123456 at ' + url + ' does not exist '
+                         'or you have no access to it'],), SystemExitMock()),
+    ]
+    with self.assertRaises(SystemExitMock):
+      self.assertEqual(1, git_cl.main(['patch', url + '/#/c/123456/1']))
+
   def _checkout_calls(self):
     return [
         ((['git', 'config', '--local', '--get-regexp',