git cl: refactor common user interactions.

R=machenbach@chromium.org
BUG=

Change-Id: Id3e3fe6223a875d997ac5569137db270818a08f4
Reviewed-on: https://chromium-review.googlesource.com/453439
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>

git_cl.py

@@ -183,6 +183,30 @@ def ask_for_data(prompt):
     sys.exit(1)
 
 
+def confirm_or_exit(prefix='', action='confirm'):
+  """Asks user to press enter to continue or press Ctrl+C to abort."""
+  if not prefix or prefix.endswith('\n'):
+    mid = 'Press'
+  elif prefix.endswith('.'):
+    mid = ' Press'
+  elif prefix.endswith(' '):
+    mid = 'press'
+  else:
+    mid = ' press'
+  ask_for_data('%s%s Enter to %s, or Ctrl+C to abort' % (prefix, mid, action))
+
+
+def ask_for_explicit_yes(prompt):
+  """Returns whether user typed 'y' or 'yes' to confirm the given prompt"""
+  result = ask_for_data(prompt + ' [Yes/No]: ').lower()
+  while True:
+    if 'yes'.startswith(result):
+      return True
+    if 'no'.startswith(result):
+      return False
+    result = ask_for_data('Please, type yes or no: ').lower()
+
+
 def _git_branch_config_key(branch, key):
   """Helper method to return Git config key for a branch."""
   assert branch, 'branch name is required to set git config for it'
@@ -1559,7 +1583,7 @@ class Changelist(object):
         print('Uploading will still work, but if you\'ve uploaded to this '
               'issue from another machine or branch the patch you\'re '
               'uploading now might not include those changes.')
-        ask_for_data('About to upload; enter to confirm.')
+        confirm_or_exit(action='upload')
 
     print_stats(options.similarity, options.find_copies, git_diff_args)
     ret = self.CMDUploadChange(options, git_diff_args, change)
@@ -2317,8 +2341,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
            git_host, self._gerrit_host,
            cookie_auth.get_new_password_message(git_host)))
       if not force:
-        ask_for_data('If you know what you are doing, press Enter to continue, '
-                     'Ctrl+C to abort.')
+        confirm_or_exit('If you know what you are doing', action='continue')
       return
     else:
       missing = (
@@ -2453,11 +2476,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
   def UpdateDescriptionRemote(self, description, force=False):
     if gerrit_util.HasPendingChangeEdit(self._GetGerritHost(), self.GetIssue()):
       if not force:
-        ask_for_data(
+        confirm_or_exit(
             'The description cannot be modified while the issue has a pending '
             'unpublished edit. Either publish the edit in the Gerrit web UI '
-            'or delete it.\n\n'
-            'Press Enter to delete the unpublished edit, Ctrl+C to abort.')
+            'or delete it.\n\n', action='delete the unpublished edit')
 
       gerrit_util.DeletePendingChangeEdit(self._GetGerritHost(),
                                           self.GetIssue())
@@ -2538,10 +2560,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     detail = self._GetChangeDetail(['CURRENT_REVISION', 'LABELS'])
     if u'Commit-Queue' in detail.get('labels', {}):
       if not force:
-        ask_for_data('\nIt seems this repository has a Commit Queue, '
+        confirm_or_exit('\nIt seems this repository has a Commit Queue, '
                         'which can test and land changes for you. '
-                     'Are you sure you wish to bypass it?\n'
-                     'Press Enter to continue, Ctrl+C to abort.')
+                        'Are you sure you wish to bypass it?\n',
+                        action='bypass CQ')
 
     differs = True
     last_upload = self._GitGetBranchConfigValue('gerritsquashhash')
@@ -2556,9 +2578,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
               'patchset')
     if differs:
       if not force:
-        ask_for_data(
-            'Do you want to submit latest Gerrit patchset and bypass hooks?\n'
-            'Press Enter to continue, Ctrl+C to abort.')
+        confirm_or_exit(
+            'Do you want to submit latest Gerrit patchset and bypass hooks?\n',
+            action='submit')
       print('WARNING: bypassing hooks and submitting latest uploaded patchset')
     elif not bypass_hooks:
       hook_results = self.RunHook(
@@ -2652,8 +2674,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
           'and may interfere with it in subtle ways.\n'
           'We recommend you remove the commit-msg hook.')
     if offer_removal:
-      reply = ask_for_data('Do you want to remove it now? [Yes/No]')
-      if reply.lower().startswith('y'):
+      if ask_for_explicit_yes('Do you want to remove it now?'):
         gclient_utils.rm_file_or_tree(hook)
         print('Gerrit commit-msg hook removed.')
       else:
@@ -2719,7 +2740,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
               'and edit it if necessary but keep the "Change-Id: %s" footer\n'
               % (self.GetIssue(), '\n  '.join(footer_change_ids), change_id,
                  change_id))
-          ask_for_data('Press enter to edit now, Ctrl+C to abort')
+          confirm_or_exit(action='edit')
           if not options.force:
             change_desc = ChangeDescription(message)
             change_desc.prompt(bug=options.bug)
@@ -2809,7 +2830,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       print('git log %s..%s' % (parent, ref_to_push))
       print('You can also use `git squash-branch` to squash these into a '
             'single commit.')
-      ask_for_data('About to upload; enter to confirm.')
+      confirm_or_exit(action='upload')
 
     if options.reviewers or options.tbr_owners:
       change_desc.update_reviewers(options.reviewers, options.tbr_owners,
@@ -3608,9 +3629,8 @@ def upload_branch_deps(cl, args):
     print('There are no dependent local branches for %s' % root_branch)
     return 0
 
-  print('This command will checkout all dependent branches and run '
-        '"git cl upload".')
-  ask_for_data('[Press enter to continue or ctrl-C to quit]')
+  confirm_or_exit('This command will checkout all dependent branches and run '
+                  '"git cl upload".', action='continue')
 
   # Add a default patchset title to all upload calls in Rietveld.
   if not cl.IsGerrit():

tests/git_cl_test.py

@@ -464,7 +464,8 @@ class TestGitCl(TestCase):
                   self._mocked_call(['get_or_create_merge_base']+list(a))))
     self.mock(git_cl, 'BranchExists', lambda _: True)
     self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: '')
-    self.mock(git_cl, 'ask_for_data', self._mocked_call)
+    self.mock(git_cl, 'ask_for_data', lambda *a, **k: self._mocked_call(
+        *(['ask_for_data'] + list(a)), **k))
     self.mock(git_cl, 'write_json', lambda path, contents:
         self._mocked_call('write_json', path, contents))
     self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock)
@@ -550,6 +551,20 @@ class TestGitCl(TestCase):
       raise result
     return result
 
+  def test_ask_for_explicit_yes_true(self):
+    self.calls = [
+        (('ask_for_data', 'prompt [Yes/No]: '), 'blah'),
+        (('ask_for_data', 'Please, type yes or no: '), 'ye'),
+    ]
+    self.assertTrue(git_cl.ask_for_explicit_yes('prompt'))
+
+  def test_ask_for_explicit_yes_true(self):
+    self.calls = [
+        (('ask_for_data', 'prompt [Yes/No]: '), 'yesish'),
+        (('ask_for_data', 'Please, type yes or no: '), 'nO'),
+    ]
+    self.assertFalse(git_cl.ask_for_explicit_yes('prompt'))
+
   def test_LoadCodereviewSettingsFromFile_gerrit(self):
     codereview_file = StringIO.StringIO('GERRIT_HOST: true')
     self.calls = [
@@ -1247,7 +1262,7 @@ class TestGitCl(TestCase):
       if not title:
         calls += [
             ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
-            (('Title for patchset []: ',), 'User input'),
+            (('ask_for_data', 'Title for patchset []: '), 'User input'),
         ]
         title = 'User_input'
     if not git_footers.get_footer_change_id(description) and not squash:
@@ -1562,7 +1577,9 @@ class TestGitCl(TestCase):
     self.mock(git_cl, 'CMDupload', mock_CMDupload)
 
     self.calls = [
-        (('[Press enter to continue or ctrl-C to quit]',), ''),
+        (('ask_for_data', 'This command will checkout all dependent branches '
+                          'and run "git cl upload". Press Enter to continue, '
+                          'or Ctrl+C to abort'), ''),
     ]
 
     class MockChangelist():
@@ -1950,8 +1967,6 @@ class TestGitCl(TestCase):
               CookiesAuthenticatorMockFactory(hosts_with_creds=auth))
     self.mock(git_cl, 'DieWithError',
               lambda msg, change=None: self._mocked_call(['DieWithError', msg]))
-    self.mock(git_cl, 'ask_for_data',
-              lambda msg: self._mocked_call(['ask_for_data', msg]))
     self.calls = self._gerrit_ensure_auth_calls(skip_auth_check=skip_auth_check)
     cl = git_cl.Changelist(codereview='gerrit')
     cl.branch = 'master'
@@ -1979,8 +1994,8 @@ class TestGitCl(TestCase):
       'chromium-review.googlesource.com': 'other',
     })
     self.calls.append(
-        ((['ask_for_data', 'If you know what you are doing, '
-                           'press Enter to continue, Ctrl+C to abort.'],), ''))
+        (('ask_for_data', 'If you know what you are doing '
+                          'press Enter to continue, or Ctrl+C to abort'), ''))
     self.assertIsNone(cl.EnsureAuthenticated(force=False))
 
   def test_gerrit_ensure_authenticated_ok(self):
@@ -2678,7 +2693,7 @@ class TestGitCl(TestCase):
         ((['exists', '/abs/git_repo_root/.git/hooks/commit-msg'],), True),
         ((['FileRead', '/abs/git_repo_root/.git/hooks/commit-msg'],),
          '...\n# From Gerrit Code Review\n...\nadd_ChangeId()\n'),
-        (('Do you want to remove it now? [Yes/No]',), 'Yes'),
+        (('ask_for_data', 'Do you want to remove it now? [Yes/No]: '), 'Yes'),
         ((['rm_file_or_tree', '/abs/git_repo_root/.git/hooks/commit-msg'],),
          ''),
     ]