Add a presubmit check for accidental checkins of files under a SVN modified directory.

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

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

presubmit_canned_checks.py

@@ -88,6 +88,42 @@ def CheckChangeHasNoCR(input_api, output_api, source_file_filter=None):
   return []
 
 
+def CheckSvnModifiedDirectories(input_api, output_api, source_file_filter=None):
+  """Checks for files in svn modified directories.
+
+  They will get submitted on accident because svn commits recursively by
+  default, and that's very dangerous.
+  """
+  if input_api.change.scm != 'svn':
+    return []
+
+  errors = []
+  current_cl_files = input_api.change.GetModifiedFiles()
+  all_modified_files = input_api.change.GetAllModifiedFiles()
+  # Filter out files in the current CL.
+  modified_files = [f for f in all_modified_files if f not in current_cl_files]
+  modified_abspaths = [input_api.os_path.abspath(f) for f in modified_files]
+
+  for f in input_api.AffectedFiles(source_file_filter):
+    if f.Action() == 'M' and f.IsDirectory():
+      curpath = f.AbsoluteLocalPath()
+      bad_files = []
+      # Check if any of the modified files in other CLs are under curpath.
+      for i in xrange(len(modified_files)):
+        abspath = modified_abspaths[i]
+        if input_api.os_path.commonprefix([curpath, abspath]) == curpath:
+          bad_files.append(modified_files[i])
+      if bad_files:
+        if input_api.is_committing:
+          error_type = output_api.PresubmitPromptWarning
+        else:
+          error_type = output_api.PresubmitNotifyResult
+        errors.append(error_type(
+            "Potential accidental commits in changelist %s:" % f.LocalPath(),
+            items=bad_files))
+  return errors
+
+
 def CheckChangeHasOnlyOneEol(input_api, output_api, source_file_filter=None):
   """Checks the files ends with one and only one \n (LF)."""
   eof_files = []
@@ -220,8 +256,10 @@ def CheckSvnForCommonMimeTypes(input_api, output_api):
 
 def CheckSvnProperty(input_api, output_api, prop, expected, affected_files):
   """Checks that affected_files files have prop=expected."""
-  bad = filter(lambda f: f.scm == 'svn' and f.Property(prop) != expected,
-               affected_files)
+  if input_api.change.scm != 'svn':
+    return []
+
+  bad = filter(lambda f: f.Property(prop) != expected, affected_files)
   if bad:
     if input_api.is_committing:
       type = output_api.PresubmitError

presubmit_support.py

@@ -343,7 +343,7 @@ class InputApi(object):
 
   def ReadFile(self, file, mode='r'):
     """Reads an arbitrary file.
-    
+
     Deny reading anything outside the repository.
     """
     if isinstance(file, AffectedFile):
@@ -372,7 +372,6 @@ class AffectedFile(object):
     self._local_root = repository_root
     self._is_directory = None
     self._properties = {}
-    self.scm = ''
 
   def ServerPath(self):
     """Returns a path string that identifies the file in the SCM system.
@@ -413,7 +412,7 @@ class AffectedFile(object):
 
   def IsTextFile(self):
     """Returns True if the file is a text file and not a binary file.
-    
+
     Deleted files are not text file."""
     raise NotImplementedError()  # Implement when needed
 
@@ -458,7 +457,6 @@ class SvnAffectedFile(AffectedFile):
     AffectedFile.__init__(self, *args, **kwargs)
     self._server_path = None
     self._is_text_file = None
-    self.scm = 'svn'
 
   def ServerPath(self):
     if self._server_path is None:
@@ -505,7 +503,6 @@ class GitAffectedFile(AffectedFile):
     AffectedFile.__init__(self, *args, **kwargs)
     self._server_path = None
     self._is_text_file = None
-    self.scm = 'git'
 
   def ServerPath(self):
     if self._server_path is None:
@@ -547,7 +544,7 @@ class Change(object):
 
   Used directly by the presubmit scripts to query the current change being
   tested.
-  
+
   Instance members:
     tags: Dictionnary of KEY=VALUE pairs found in the change description.
     self.KEY: equivalent to tags['KEY']
@@ -567,6 +564,7 @@ class Change(object):
     self._local_root = local_root
     self.issue = issue
     self.patchset = patchset
+    self.scm = ''
 
     # From the description text, build up a dictionary of key/value pairs
     # plus the description minus all key/value or "tag" lines.
@@ -681,10 +679,31 @@ class Change(object):
 class SvnChange(Change):
   _AFFECTED_FILES = SvnAffectedFile
 
+  def __init__(self, *args, **kwargs):
+    Change.__init__(self, *args, **kwargs)
+    self.scm = 'svn'
+
+  def GetAllModifiedFiles(self):
+    """Get all modified files."""
+    changelists = gcl.GetModifiedFiles()
+    all_modified_files = []
+    for cl in changelists.values():
+      all_modified_files.extend([f[1] for f in cl])
+    return all_modified_files
+
+  def GetModifiedFiles(self):
+    """Get modified files in the current CL."""
+    changelists = gcl.GetModifiedFiles()
+    return [f[1] for f in changelists[self.Name()]]
+
 
 class GitChange(Change):
   _AFFECTED_FILES = GitAffectedFile
 
+  def __init__(self, *args, **kwargs):
+    Change.__init__(self, *args, **kwargs)
+    self.scm = 'git'
+
 
 def ListRelevantPresubmitFiles(files, root):
   """Finds all presubmit files that apply to a given set of source files.

tests/presubmit_unittest.py

@@ -35,10 +35,12 @@ def CheckChangeOnUpload(input_api, output_api):
   def setUp(self):
     super_mox.SuperMoxTestBase.setUp(self)
     self.mox.StubOutWithMock(presubmit, 'warnings')
-    # Stub out 'os' but keep os.path.dirname/join/normpath/splitext and os.sep.
+    # Stub out 'os' but keep os.path.commonprefix/dirname/join/normpath/splitext
+    # and os.sep.
     os_sep = presubmit.os.sep
-    os_path_join = presubmit.os.path.join
+    os_path_commonprefix = presubmit.os.path.commonprefix
     os_path_dirname = presubmit.os.path.dirname
+    os_path_join = presubmit.os.path.join
     os_path_normpath = presubmit.os.path.normpath
     os_path_splitext = presubmit.os.path.splitext
     self.mox.StubOutWithMock(presubmit, 'os')
@@ -55,6 +57,7 @@ def CheckChangeOnUpload(input_api, output_api):
     def MockAbsPath(f):
       return f
     presubmit.os.path.abspath = MockAbsPath
+    presubmit.os.path.commonprefix = os_path_commonprefix
     self.fake_root_dir = self.RootDir()
     self.mox.StubOutWithMock(presubmit.gclient, 'CaptureSVNInfo')
     self.mox.StubOutWithMock(presubmit.gcl, 'GetSVNFileProperty')
@@ -394,7 +397,7 @@ def CheckChangeOnCommit(input_api, output_api):
                                   'PRESUBMIT.py')).AndReturn(False)
     presubmit.random.randint(0, 4).AndReturn(0)
     self.mox.ReplayAll()
-    
+
     output = StringIO.StringIO()
     input = StringIO.StringIO('y\n')
     # Always fail.
@@ -451,7 +454,7 @@ def CheckChangeOnUpload(input_api, output_api):
   if 'TEST' in input_api.change.tags:
     return [output_api.PresubmitError('Tag parsing failed. 3')]
   if input_api.change.DescriptionText() != 'Blah Blah':
-    return [output_api.PresubmitError('Tag parsing failed. 4 ' + 
+    return [output_api.PresubmitError('Tag parsing failed. 4 ' +
                                       input_api.change.DescriptionText())]
   if (input_api.change.FullDescriptionText() !=
       'Blah Blah\\n\\nSTORY=http://tracker.com/42\\nBUG=boo\\n'):
@@ -490,7 +493,7 @@ def CheckChangeOnCommit(input_api, output_api):
                                 mox.IgnoreArg(),
                                 None, False).AndReturn(False)
     self.mox.ReplayAll()
-    
+
     self.assertEquals(True,
                       presubmit.Main(['presubmit', '--root',
                                       self.fake_root_dir]))
@@ -897,7 +900,6 @@ class AffectedFileUnittest(PresubmitTestsBase):
     members = [
       'AbsoluteLocalPath', 'Action', 'IsDirectory', 'IsTextFile', 'LocalPath',
       'NewContents', 'OldContents', 'OldFileTempPath', 'Property', 'ServerPath',
-      'scm',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit.AffectedFile('a', 'b'), members)
@@ -974,7 +976,7 @@ class GclChangeUnittest(PresubmitTestsBase):
         'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTextFiles',
         'DescriptionText', 'FullDescriptionText', 'LocalPaths', 'Name',
         'RepositoryRoot', 'RightHandSideLines', 'ServerPaths',
-        'issue', 'patchset', 'tags',
+        'issue', 'patchset', 'scm', 'tags',
     ]
     # If this test fails, you should add the relevant test.
     self.mox.ReplayAll()
@@ -993,6 +995,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
   def MockInputApi(self, change, committing):
     input_api = self.mox.CreateMock(presubmit.InputApi)
     input_api.cStringIO = presubmit.cStringIO
+    input_api.os_path = presubmit.os.path
     input_api.re = presubmit.re
     input_api.traceback = presubmit.traceback
     input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2)
@@ -1013,6 +1016,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'CheckChangeHasNoCrAndHasOnlyOneEol', 'CheckChangeHasNoTabs',
       'CheckChangeHasQaField', 'CheckChangeHasTestedField',
       'CheckChangeHasTestField', 'CheckChangeSvnEolStyle',
+      'CheckSvnModifiedDirectories',
       'CheckSvnForCommonMimeTypes', 'CheckSvnProperty',
       'CheckDoNotSubmit',
       'CheckDoNotSubmitInDescription', 'CheckDoNotSubmitInFiles',
@@ -1091,7 +1095,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def SvnPropertyTest(self, check, property, value1, value2, committing,
                       error_type, use_source_file):
-    input_api1 = self.MockInputApi(None, committing)
+    change1 = presubmit.SvnChange('mychange', '', self.fake_root_dir, [], 0, 0)
+    input_api1 = self.MockInputApi(change1, committing)
     files1 = [
       presubmit.SvnAffectedFile('foo/bar.cc', 'A'),
       presubmit.SvnAffectedFile('foo.cc', 'M'),
@@ -1104,7 +1109,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
                                      property).AndReturn(value1)
     presubmit.gcl.GetSVNFileProperty(presubmit.normpath('foo.cc'),
                                      property).AndReturn(value1)
-    input_api2 = self.MockInputApi(None, committing)
+    change2 = presubmit.SvnChange('mychange', '', self.fake_root_dir, [], 0, 0)
+    input_api2 = self.MockInputApi(change2, committing)
     files2 = [
       presubmit.SvnAffectedFile('foo/bar.cc', 'A'),
       presubmit.SvnAffectedFile('foo.cc', 'M'),
@@ -1113,7 +1119,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       input_api2.AffectedSourceFiles(None).AndReturn(files2)
     else:
       input_api2.AffectedFiles(include_deleted=False).AndReturn(files2)
-    
+
     presubmit.gcl.GetSVNFileProperty(presubmit.normpath('foo/bar.cc'),
                                      property).AndReturn(value2)
     presubmit.gcl.GetSVNFileProperty(presubmit.normpath('foo.cc'),
@@ -1142,7 +1148,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
                          'Bleh', '',
                          presubmit.OutputApi.PresubmitError,
                          True)
-  
+
   def testCannedCheckChangeHasTestField(self):
     self.DescriptionTest(presubmit_canned_checks.CheckChangeHasTestField,
                          'Foo\nTEST=did some stuff', 'Foo\n',
@@ -1179,7 +1185,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
             presubmit_canned_checks.CheckChangeHasNoStrayWhitespace(x, y),
         'Foo', 'Foo ',
         presubmit.OutputApi.PresubmitPromptWarning)
-    
+
 
   def testCheckChangeHasOnlyOneEol(self):
     self.ReadFileTest(presubmit_canned_checks.CheckChangeHasOnlyOneEol,
@@ -1190,7 +1196,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
     self.ReadFileTest(presubmit_canned_checks.CheckChangeHasNoCR,
                       "Hey!\nHo!\n", "Hey!\r\nHo!\r\n",
                       presubmit.OutputApi.PresubmitPromptWarning)
- 
+
   def testCheckChangeHasNoCrAndHasOnlyOneEol(self):
     self.ReadFileTest(
         presubmit_canned_checks.CheckChangeHasNoCrAndHasOnlyOneEol,
@@ -1223,6 +1229,32 @@ class CannedChecksUnittest(PresubmitTestsBase):
                          'svn:eol-style', 'LF', '', False,
                          presubmit.OutputApi.PresubmitNotifyResult, True)
 
+  def testCannedCheckSvnAccidentalSubmission(self):
+    modified_dir_file = 'foo/'
+    accidental_submssion_file = 'foo/bar.cc'
+
+    change = self.mox.CreateMock(presubmit.SvnChange)
+    change.scm = 'svn'
+    change.GetModifiedFiles().AndReturn([modified_dir_file])
+    change.GetAllModifiedFiles().AndReturn([modified_dir_file,
+                                            accidental_submssion_file])
+    input_api = self.MockInputApi(change, True)
+
+    affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
+    affected_file.Action().AndReturn('M')
+    affected_file.IsDirectory().AndReturn(True)
+    affected_file.AbsoluteLocalPath().AndReturn(accidental_submssion_file)
+    affected_file.LocalPath().AndReturn(accidental_submssion_file)
+    input_api.AffectedFiles(None).AndReturn([affected_file])
+
+    self.mox.ReplayAll()
+
+    check = presubmit_canned_checks.CheckSvnModifiedDirectories
+    results = check(input_api, presubmit.OutputApi, None)
+    self.assertEquals(len(results), 1)
+    self.assertEquals(results[0].__class__,
+                      presubmit.OutputApi.PresubmitPromptWarning)
+
   def testCheckSvnForCommonMimeTypes(self):
     self.mox.StubOutWithMock(presubmit_canned_checks, 'CheckSvnProperty')
     input_api = self.MockInputApi(None, False)