diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index a0942df8c..467286765 100755 --- a/presubmit_canned_checks.py +++ b/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 diff --git a/presubmit_support.py b/presubmit_support.py index 8ab05c133..bef76e928 100755 --- a/presubmit_support.py +++ b/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. diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 9f6a57e1a..aa0e40433 100755 --- a/tests/presubmit_unittest.py +++ b/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)