diff --git a/presubmit_support.py b/presubmit_support.py index 7dd36ba26..16dd377ee 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1725,7 +1725,7 @@ def DoPresubmitChecks(change, os.environ = old_environ -def ScanSubDirs(mask, recursive): +def _scan_sub_dirs(mask, recursive): if not recursive: return [x for x in glob.glob(mask) if x not in ('.svn', '.git')] @@ -1741,32 +1741,83 @@ def ScanSubDirs(mask, recursive): return results -def ParseFiles(args, recursive): +def _parse_files(args, recursive): logging.debug('Searching for %s', args) files = [] for arg in args: - files.extend([('M', f) for f in ScanSubDirs(arg, recursive)]) + files.extend([('M', f) for f in _scan_sub_dirs(arg, recursive)]) return files -def load_files(options): - """Tries to determine the SCM.""" - files = [] - if options.files: - files = ParseFiles(options.files, options.recursive) +def _parse_change(parser, options): + """Process change options. + + Args: + parser: The parser used to parse the arguments from command line. + options: The arguments parsed from command line. + Returns: + A GitChange if the change root is a git repository, or a Change otherwise. + """ + if options.files and options.all_files: + parser.error(' cannot be specified when --all-files is set.') + change_scm = scm.determine_scm(options.root) - if change_scm == 'git': - change_class = GitChange - upstream = options.upstream or None - if not files: - files = scm.GIT.CaptureStatus([], options.root, upstream) + if change_scm != 'git' and not options.files: + parser.error(' is not optional for unversioned directories.') + + if options.files: + change_files = _parse_files(options.files, options.recursive) + elif options.all_files: + change_files = [('M', f) for f in scm.GIT.GetAllFiles(options.root)] else: - logging.info( - 'Doesn\'t seem under source control. Got %d files', len(options.files)) - if not files: - return None, None - change_class = Change - return change_class, files + change_files = scm.GIT.CaptureStatus( + [], options.root, options.upstream or None) + + logging.info('Found %d file(s).', len(change_files)) + + change_class = GitChange if change_scm == 'git' else Change + return change_class( + options.name, + options.description, + options.root, + change_files, + options.issue, + options.patchset, + options.author, + upstream=options.upstream) + + +def _parse_gerrit_options(parser, options): + """Process gerrit options. + + SIDE EFFECTS: Modifies options.author and options.description from Gerrit if + options.gerrit_fetch is set. + + Args: + parser: The parser used to parse the arguments from command line. + options: The arguments parsed from command line. + Returns: + A GerritAccessor object if options.gerrit_url is set, or None otherwise. + """ + gerrit_obj = None + if options.gerrit_url: + gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) + + if not options.gerrit_fetch: + return gerrit_obj + + if not options.gerrit_url or not options.issue or not options.patchset: + parser.error( + '--gerrit_fetch requires --gerrit_url, --issue and --patchset.') + + options.author = gerrit_obj.GetChangeOwner(options.issue) + options.description = gerrit_obj.GetChangeDescription( + options.issue, options.patchset) + + logging.info('Got author: "%s"', options.author) + logging.info('Got description: """\n%s\n"""', options.description) + + return gerrit_obj @contextlib.contextmanager @@ -1824,6 +1875,8 @@ def main(argv=None): 'all PRESUBMIT files in parallel.') parser.add_argument('--json_output', help='Write presubmit errors to json output.') + parser.add_argument('--all-files', action='store_true', + help='Mark all files under source control as modified.') parser.add_argument('files', nargs='*', help='List of files to be marked as modified when ' 'executing presubmit or post-upload hooks. fnmatch ' @@ -1831,21 +1884,6 @@ def main(argv=None): options = parser.parse_args(argv) - gerrit_obj = None - if options.gerrit_url: - gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) - if options.gerrit_fetch: - if not options.gerrit_url or not options.issue or not options.patchset: - parser.error( - '--gerrit_fetch requires --gerrit_url, --issue and --patchset.') - - options.author = gerrit_obj.GetChangeOwner(options.issue) - options.description = gerrit_obj.GetChangeDescription( - options.issue, options.patchset) - - logging.info('Got author: "%s"', options.author) - logging.info('Got description: """\n%s\n"""', options.description) - if options.verbose >= 2: logging.basicConfig(level=logging.DEBUG) elif options.verbose: @@ -1853,22 +1891,13 @@ def main(argv=None): else: logging.basicConfig(level=logging.ERROR) - change_class, files = load_files(options) - if not change_class: - parser.error('For unversioned directory, is not optional.') - logging.info('Found %d file(s).', len(files)) + change = _parse_change(parser, options) + gerrit_obj = _parse_gerrit_options(parser, options) try: with canned_check_filter(options.skip_canned): results = DoPresubmitChecks( - change_class(options.name, - options.description, - options.root, - files, - options.issue, - options.patchset, - options.author, - upstream=options.upstream), + change, options.commit, options.verbose, sys.stdout, diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 46293812a..0f6c519d0 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -163,24 +163,25 @@ index fe3de7b..54ae6e1 100755 mock.patch('gclient_utils.FileRead').start() mock.patch('gclient_utils.FileWrite').start() mock.patch('json.load').start() - mock.patch('os.path.abspath', lambda f: f).start() - mock.patch('os.getcwd', self.RootDir) + mock.patch('multiprocessing.cpu_count', lambda: 2) mock.patch('os.chdir').start() + mock.patch('os.getcwd', self.RootDir) mock.patch('os.listdir').start() + mock.patch('os.path.abspath', lambda f: f).start() mock.patch('os.path.isfile').start() mock.patch('os.remove').start() - mock.patch('random.randint').start() - mock.patch('presubmit_support.warn').start() + mock.patch('presubmit_support._parse_files').start() mock.patch('presubmit_support.sigint_handler').start() mock.patch('presubmit_support.time_time', return_value=0).start() - mock.patch('scm.determine_scm').start() + mock.patch('presubmit_support.warn').start() + mock.patch('random.randint').start() mock.patch('scm.GIT.GenerateDiff').start() + mock.patch('scm.determine_scm').start() mock.patch('subprocess2.Popen').start() mock.patch('sys.stderr', StringIO()).start() mock.patch('sys.stdout', StringIO()).start() mock.patch('tempfile.NamedTemporaryFile').start() mock.patch('threading.Timer').start() - mock.patch('multiprocessing.cpu_count', lambda: 2) if sys.version_info.major == 2: mock.patch('urllib2.urlopen').start() else: @@ -878,10 +879,9 @@ def CheckChangeOnCommit(input_api, output_api): False, output)) @mock.patch('presubmit_support.DoPresubmitChecks') - @mock.patch('presubmit_support.ParseFiles') - def testMainUnversioned(self, mockParseFiles, mockDoPresubmitChecks): + def testMainUnversioned(self, mockDoPresubmitChecks): scm.determine_scm.return_value = None - mockParseFiles.return_value = [('M', 'random_file.txt')] + presubmit._parse_files.return_value = [('M', 'random_file.txt')] mockDoPresubmitChecks().should_continue.return_value = False self.assertEqual( @@ -898,8 +898,202 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual( sys.stderr.getvalue(), 'usage: presubmit_unittest.py [options] \n' - 'presubmit_unittest.py: error: For unversioned directory, is ' - 'not optional.\n') + 'presubmit_unittest.py: error: is not optional for unversioned ' + 'directories.\n') + + @mock.patch('presubmit_support.Change', mock.Mock()) + def testParseChange_Files(self): + presubmit._parse_files.return_value=[('M', 'random_file.txt')] + scm.determine_scm.return_value = None + options = mock.Mock(all_files=False) + + change = presubmit._parse_change(None, options) + self.assertEqual(presubmit.Change.return_value, change) + presubmit.Change.assert_called_once_with( + options.name, + options.description, + options.root, + [('M', 'random_file.txt')], + options.issue, + options.patchset, + options.author, + upstream=options.upstream) + presubmit._parse_files.assert_called_once_with( + options.files, options.recursive) + + def testParseChange_NoFilesAndNoScm(self): + presubmit._parse_files.return_value = [] + scm.determine_scm.return_value = None + parser = mock.Mock() + parser.error.side_effect = [SystemExit] + options = mock.Mock(files=[], all_files=False) + + with self.assertRaises(SystemExit): + presubmit._parse_change(parser, options) + parser.error.assert_called_once_with( + ' is not optional for unversioned directories.') + + def testParseChange_FilesAndAllFiles(self): + parser = mock.Mock() + parser.error.side_effect = [SystemExit] + options = mock.Mock(files=['foo'], all_files=True) + + with self.assertRaises(SystemExit): + presubmit._parse_change(parser, options) + parser.error.assert_called_once_with( + ' cannot be specified when --all-files is set.') + + @mock.patch('presubmit_support.GitChange', mock.Mock()) + def testParseChange_FilesAndGit(self): + scm.determine_scm.return_value = 'git' + presubmit._parse_files.return_value = [('M', 'random_file.txt')] + options = mock.Mock(all_files=False) + + change = presubmit._parse_change(None, options) + self.assertEqual(presubmit.GitChange.return_value, change) + presubmit.GitChange.assert_called_once_with( + options.name, + options.description, + options.root, + [('M', 'random_file.txt')], + options.issue, + options.patchset, + options.author, + upstream=options.upstream) + presubmit._parse_files.assert_called_once_with( + options.files, options.recursive) + + @mock.patch('presubmit_support.GitChange', mock.Mock()) + @mock.patch('scm.GIT.CaptureStatus', mock.Mock()) + def testParseChange_NoFilesAndGit(self): + scm.determine_scm.return_value = 'git' + scm.GIT.CaptureStatus.return_value = [('A', 'added.txt')] + options = mock.Mock(all_files=False, files=[]) + + change = presubmit._parse_change(None, options) + self.assertEqual(presubmit.GitChange.return_value, change) + presubmit.GitChange.assert_called_once_with( + options.name, + options.description, + options.root, + [('A', 'added.txt')], + options.issue, + options.patchset, + options.author, + upstream=options.upstream) + scm.GIT.CaptureStatus.assert_called_once_with( + [], options.root, options.upstream) + + @mock.patch('presubmit_support.GitChange', mock.Mock()) + @mock.patch('scm.GIT.GetAllFiles', mock.Mock()) + def testParseChange_AllFilesAndGit(self): + scm.determine_scm.return_value = 'git' + scm.GIT.GetAllFiles.return_value = ['foo.txt', 'bar.txt'] + options = mock.Mock(all_files=True, files=[]) + + change = presubmit._parse_change(None, options) + self.assertEqual(presubmit.GitChange.return_value, change) + presubmit.GitChange.assert_called_once_with( + options.name, + options.description, + options.root, + [('M', 'foo.txt'), ('M', 'bar.txt')], + options.issue, + options.patchset, + options.author, + upstream=options.upstream) + scm.GIT.GetAllFiles.assert_called_once_with(options.root) + + def testParseGerritOptions_NoGerritUrl(self): + options = mock.Mock( + gerrit_url=None, + gerrit_fetch=False, + author='author', + description='description') + gerrit_obj = presubmit._parse_gerrit_options(None, options) + + self.assertIsNone(gerrit_obj) + self.assertEqual('author', options.author) + self.assertEqual('description', options.description) + + @mock.patch('presubmit_support.GerritAccessor', mock.Mock()) + def testParseGerritOptions_NoGerritFetch(self): + options = mock.Mock( + gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_fetch=False, + author='author', + description='description') + + gerrit_obj = presubmit._parse_gerrit_options(None, options) + + presubmit.GerritAccessor.assert_called_once_with( + 'foo-review.googlesource.com') + self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) + self.assertEqual('author', options.author) + self.assertEqual('description', options.description) + + @mock.patch('presubmit_support.GerritAccessor', mock.Mock()) + def testParseGerritOptions_GerritFetch(self): + accessor = mock.Mock() + accessor.GetChangeOwner.return_value = 'new owner' + accessor.GetChangeDescription.return_value = 'new description' + presubmit.GerritAccessor.return_value = accessor + + options = mock.Mock( + gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_fetch=True, + issue=123, + patchset=4) + + gerrit_obj = presubmit._parse_gerrit_options(None, options) + + presubmit.GerritAccessor.assert_called_once_with( + 'foo-review.googlesource.com') + self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) + self.assertEqual('new owner', options.author) + self.assertEqual('new description', options.description) + + def testParseGerritOptions_GerritFetchNoUrl(self): + parser = mock.Mock() + parser.error.side_effect = [SystemExit] + options = mock.Mock( + gerrit_url=None, + gerrit_fetch=True, + issue=123, + patchset=4) + + with self.assertRaises(SystemExit): + presubmit._parse_gerrit_options(parser, options) + parser.error.assert_called_once_with( + '--gerrit_fetch requires --gerrit_url, --issue and --patchset.') + + def testParseGerritOptions_GerritFetchNoIssue(self): + parser = mock.Mock() + parser.error.side_effect = [SystemExit] + options = mock.Mock( + gerrit_url='https://example.com', + gerrit_fetch=True, + issue=None, + patchset=4) + + with self.assertRaises(SystemExit): + presubmit._parse_gerrit_options(parser, options) + parser.error.assert_called_once_with( + '--gerrit_fetch requires --gerrit_url, --issue and --patchset.') + + def testParseGerritOptions_GerritFetchNoPatchset(self): + parser = mock.Mock() + parser.error.side_effect = [SystemExit] + options = mock.Mock( + gerrit_url='https://example.com', + gerrit_fetch=True, + issue=123, + patchset=None) + + with self.assertRaises(SystemExit): + presubmit._parse_gerrit_options(parser, options) + parser.error.assert_called_once_with( + '--gerrit_fetch requires --gerrit_url, --issue and --patchset.') class InputApiUnittest(PresubmitTestsBase):