diff --git a/presubmit_support.py b/presubmit_support.py index f33bd3310..3b89215fa 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1725,7 +1725,7 @@ def DoPresubmitChecks(change, os.environ = old_environ -def _scan_sub_dirs(mask, recursive): +def ScanSubDirs(mask, recursive): if not recursive: return [x for x in glob.glob(mask) if x not in ('.svn', '.git')] @@ -1741,83 +1741,32 @@ def _scan_sub_dirs(mask, recursive): return results -def _parse_files(args, recursive): +def ParseFiles(args, recursive): logging.debug('Searching for %s', args) files = [] for arg in args: - files.extend([('M', f) for f in _scan_sub_dirs(arg, recursive)]) + files.extend([('M', f) for f in ScanSubDirs(arg, recursive)]) return files -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' and not options.files: - parser.error(' is not optional for unversioned directories.') - +def load_files(options): + """Tries to determine the SCM.""" + files = [] 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)] + files = ParseFiles(options.files, options.recursive) + 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) else: - 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 + 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 @contextlib.contextmanager @@ -1875,8 +1824,6 @@ 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 ' @@ -1884,6 +1831,21 @@ 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: @@ -1891,13 +1853,22 @@ def main(argv=None): else: logging.basicConfig(level=logging.ERROR) - change = _parse_change(parser, options) - gerrit_obj = _parse_gerrit_options(parser, options) + 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)) try: with canned_check_filter(options.skip_canned): results = DoPresubmitChecks( - change, + change_class(options.name, + options.description, + options.root, + files, + options.issue, + options.patchset, + options.author, + upstream=options.upstream), options.commit, options.verbose, sys.stdout, diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 0f6c519d0..46293812a 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -163,25 +163,24 @@ index fe3de7b..54ae6e1 100755 mock.patch('gclient_utils.FileRead').start() mock.patch('gclient_utils.FileWrite').start() mock.patch('json.load').start() - mock.patch('multiprocessing.cpu_count', lambda: 2) - mock.patch('os.chdir').start() + mock.patch('os.path.abspath', lambda f: f).start() mock.patch('os.getcwd', self.RootDir) + mock.patch('os.chdir').start() 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('presubmit_support._parse_files').start() + mock.patch('random.randint').start() + mock.patch('presubmit_support.warn').start() mock.patch('presubmit_support.sigint_handler').start() mock.patch('presubmit_support.time_time', return_value=0).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('scm.GIT.GenerateDiff').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: @@ -879,9 +878,10 @@ def CheckChangeOnCommit(input_api, output_api): False, output)) @mock.patch('presubmit_support.DoPresubmitChecks') - def testMainUnversioned(self, mockDoPresubmitChecks): + @mock.patch('presubmit_support.ParseFiles') + def testMainUnversioned(self, mockParseFiles, mockDoPresubmitChecks): scm.determine_scm.return_value = None - presubmit._parse_files.return_value = [('M', 'random_file.txt')] + mockParseFiles.return_value = [('M', 'random_file.txt')] mockDoPresubmitChecks().should_continue.return_value = False self.assertEqual( @@ -898,202 +898,8 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual( sys.stderr.getvalue(), 'usage: presubmit_unittest.py [options] \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.') + 'presubmit_unittest.py: error: For unversioned directory, is ' + 'not optional.\n') class InputApiUnittest(PresubmitTestsBase):