Make sure warnings fail the check when may_prompt=False.

Split presubmit_support.Main() in two to make it simpler.

Handle better the case with unversioned files and add a unit test.

R=dpranke@chromium.org

Review URL: http://codereview.chromium.org/6674059

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@78700 0039d316-1c4b-4281-b951-d872f2087c98

presubmit_support.py

@@ -6,7 +6,7 @@
 """Enables directory-specific presubmit checks to run at upload and/or commit.
 """
 
-__version__ = '1.3.5'
+__version__ = '1.4'
 
 # TODO(joi) Add caching where appropriate/needed. The API is designed to allow
 # caching (between all different invocations of presubmit scripts for a given
@@ -1083,9 +1083,12 @@ def DoPresubmitChecks(change,
   if total_time > 1.0:
     output.write("Presubmit checks took %.1fs to calculate.\n" % total_time)
 
-  if not errors and warnings and may_prompt:
-    output.prompt_yes_no('There were presubmit warnings. '
-                         'Are you sure you wish to continue? (y/N): ')
+  if not errors and warnings:
+    if may_prompt:
+      output.prompt_yes_no('There were presubmit warnings. '
+                          'Are you sure you wish to continue? (y/N): ')
+    else:
+      output.fail()
 
   global _ASKED_FOR_FEEDBACK
   # Ask for feedback one time out of 5.
@@ -1120,8 +1123,31 @@ def ParseFiles(args, recursive):
   return files
 
 
+def load_files(options, args):
+  """Tries to determine the SCM."""
+  change_scm = scm.determine_scm(options.root)
+  files = []
+  if change_scm == 'svn':
+    change_class = SvnChange
+    status_fn = scm.SVN.CaptureStatus
+  elif change_scm == 'git':
+    change_class = GitChange
+    status_fn = scm.GIT.CaptureStatus
+  else:
+    logging.info('Doesn\'t seem under source control. Got %d files' % len(args))
+    if not args:
+      return None, None
+    change_class = Change
+  if args:
+    files = ParseFiles(args, options.recursive)
+  else:
+    # Grab modified files.
+    files = status_fn([options.root])
+  return change_class, files
+
+
 def Main(argv):
-  parser = optparse.OptionParser(usage="%prog [options]",
+  parser = optparse.OptionParser(usage="%prog [options] <files...>",
                                  version="%prog " + str(__version__))
   parser.add_option("-c", "--commit", action="store_true", default=False,
                    help="Use commit instead of upload checks")
@@ -1131,7 +1157,6 @@ def Main(argv):
                    help="Act recursively")
   parser.add_option("-v", "--verbose", action="store_true", default=False,
                    help="Verbose output")
-  parser.add_option("--files")
   parser.add_option("--name", default='no name')
   parser.add_option("--description", default='')
   parser.add_option("--issue", type='int', default=0)
@@ -1146,43 +1171,15 @@ def Main(argv):
   options, args = parser.parse_args(argv)
   if options.verbose:
     logging.basicConfig(level=logging.DEBUG)
-  if os.path.isdir(os.path.join(options.root, '.svn')):
-    change_class = SvnChange
-    if not options.files:
-      if args:
-        options.files = ParseFiles(args, options.recursive)
-      else:
-        # Grab modified files.
-        options.files = scm.SVN.CaptureStatus([options.root])
-  else:
-    is_git = os.path.isdir(os.path.join(options.root, '.git'))
-    if not is_git:
-      is_git = (0 == subprocess.call(
-          ['git', 'rev-parse', '--show-cdup'],
-          stdout=subprocess.PIPE, cwd=options.root))
-    if is_git:
-      # Only look at the subdirectories below cwd.
-      change_class = GitChange
-      if not options.files:
-        if args:
-          options.files = ParseFiles(args, options.recursive)
-        else:
-          # Grab modified files.
-          options.files = scm.GIT.CaptureStatus([options.root])
-    else:
-      logging.info('Doesn\'t seem under source control.')
-      change_class = Change
+  change_class, files = load_files(options, args)
+  if not change_class:
+    parser.error('For unversioned directory, <files> is not optional.')
   if options.verbose:
-    if not options.files:
-      print "Found no files."
-    elif len(options.files) != 1:
-      print "Found %d files." % len(options.files)
-    else:
-      print "Found 1 file."
+    print "Found %d file(s)." % len(files)
   results = DoPresubmitChecks(change_class(options.name,
                                            options.description,
                                            options.root,
-                                           options.files,
+                                           files,
                                            options.issue,
                                            options.patchset),
                               options.commit,

scm.py

@@ -65,6 +65,24 @@ def GenFakeDiff(filename):
   return result
 
 
+def determine_scm(root):
+  """Similar to upload.py's version but much simpler.
+
+  Returns 'svn', 'git' or None.
+  """
+  if os.path.isdir(os.path.join(root, '.svn')):
+    return 'svn'
+  elif os.path.isdir(os.path.join(root, '.svn')):
+    return 'git'
+  else:
+    if (0 == subprocess.call(
+          ['git', 'rev-parse', '--show-cdup'],
+          stdout=subprocess.PIPE, cwd=root)):
+      return 'git'
+    else:
+      return None
+
+
 class GIT(object):
   @staticmethod
   def Capture(args, **kwargs):

tests/presubmit_unittest.py

@@ -119,6 +119,7 @@ def GetPreferredTrySlaves():
     presubmit.os.path.abspath = MockAbsPath
     presubmit.os.getcwd = self.RootDir
     presubmit.os.chdir = MockChdir
+    self.mox.StubOutWithMock(presubmit.scm, 'determine_scm')
     self.mox.StubOutWithMock(presubmit.scm.SVN, 'CaptureInfo')
     self.mox.StubOutWithMock(presubmit.scm.SVN, 'GetFileProperty')
     self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileRead')
@@ -135,12 +136,12 @@ class PresubmitUnittest(PresubmitTestsBase):
     self.mox.ReplayAll()
     members = [
       'AffectedFile', 'Change', 'DoGetTrySlaves', 'DoPresubmitChecks',
-      'GetTrySlavesExecuter', 'GitAffectedFile', 'GitChange',
-      'InputApi', 'ListRelevantPresubmitFiles', 'Main',
+      'GetTrySlavesExecuter', 'GitAffectedFile',
+      'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'Main',
       'NotImplementedException', 'OutputApi', 'ParseFiles',
       'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs',
       'SvnAffectedFile', 'SvnChange', 'cPickle', 'cStringIO',
-      'exceptions', 'fnmatch', 'gclient_utils', 'glob', 'json',
+      'exceptions', 'fnmatch', 'gclient_utils', 'glob', 'json', 'load_files',
       'logging', 'marshal', 'normpath', 'optparse', 'os', 'owners', 'pickle',
       'presubmit_canned_checks', 'random', 're', 'scm', 'subprocess',
       'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2',
@@ -662,19 +663,14 @@ def CheckChangeOnCommit(input_api, output_api):
                                               self.fake_root_dir, None, False,
                                               output))
 
-  def testMain(self):
+  def testMainUnversioned(self):
     # OptParser calls presubmit.os.path.exists and is a pain when mocked.
     self.UnMock(presubmit.os.path, 'exists')
     self.mox.StubOutWithMock(presubmit, 'DoPresubmitChecks')
     self.mox.StubOutWithMock(presubmit, 'ParseFiles')
-    presubmit.os.path.isdir(presubmit.os.path.join(self.fake_root_dir, '.svn')
-        ).AndReturn(False)
-    presubmit.os.path.isdir(presubmit.os.path.join(self.fake_root_dir, '.git')
-        ).AndReturn(False)
-    presubmit.subprocess.call(
-        ['git', 'rev-parse', '--show-cdup'],
-        cwd=self.fake_root_dir,
-        stdout=presubmit.subprocess.PIPE).AndReturn(1)
+    presubmit.scm.determine_scm(self.fake_root_dir).AndReturn(None)
+    presubmit.ParseFiles(['random_file.txt'], None
+        ).AndReturn(['random_file.txt'])
     output = self.mox.CreateMock(presubmit.PresubmitOutput)
     output.should_continue().AndReturn(False)
 
@@ -684,9 +680,30 @@ def CheckChangeOnCommit(input_api, output_api):
                                 None, False).AndReturn(output)
     self.mox.ReplayAll()
 
-    self.assertEquals(True,
-                      presubmit.Main(['presubmit', '--root',
-                                      self.fake_root_dir]))
+    self.assertEquals(
+        True,
+        presubmit.Main(['--root', self.fake_root_dir, 'random_file.txt']))
+
+  def testMainUnversionedFail(self):
+    # OptParser calls presubmit.os.path.exists and is a pain when mocked.
+    self.UnMock(presubmit.os.path, 'exists')
+    self.mox.StubOutWithMock(presubmit, 'DoPresubmitChecks')
+    self.mox.StubOutWithMock(presubmit, 'ParseFiles')
+    presubmit.scm.determine_scm(self.fake_root_dir).AndReturn(None)
+    self.mox.StubOutWithMock(presubmit.sys, 'stderr')
+    presubmit.sys.stderr.write(
+        'Usage: presubmit_unittest.py [options] <files...>\n')
+    presubmit.sys.stderr.write('\n')
+    presubmit.sys.stderr.write(
+        'presubmit_unittest.py: error: For unversioned directory, <files> is '
+        'not optional.\n')
+    self.mox.ReplayAll()
+
+    try:
+      presubmit.Main(['--root', self.fake_root_dir])
+      self.fail()
+    except SystemExit, e:
+      self.assertEquals(2, e.code)
 
 
 class InputApiUnittest(PresubmitTestsBase):

tests/scm_unittest.py

@@ -39,8 +39,8 @@ class RootTestCase(BaseSCMTestCase):
     self.mox.ReplayAll()
     members = [
         'GetCasedPath', 'GenFakeDiff', 'GIT', 'SVN', 'ValidateEmail',
-        'cStringIO', 'gclient_utils', 'glob',  'logging', 'os', 're', 'shutil',
-        'subprocess', 'sys', 'tempfile', 'time', 'xml',
+        'cStringIO', 'determine_scm', 'gclient_utils', 'glob', 'logging', 'os',
+        're', 'shutil', 'subprocess', 'sys', 'tempfile', 'time', 'xml',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(scm, members)