diff --git a/git_cl.py b/git_cl.py index 141364bc0..d26302fb7 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1294,6 +1294,8 @@ class Changelist(object): gerrit_url = self.GetCodereviewServer() issue = self.GetIssue() patchset = self.GetPatchset() + remote, remote_branch = self.GetRemoteBranch() + target_ref = GetTargetRef(remote, remote_branch, None) if author: args.extend(['--author', author]) if gerrit_url: @@ -1302,6 +1304,8 @@ class Changelist(object): args.extend(['--issue', str(issue)]) if patchset: args.extend(['--patchset', str(patchset)]) + if target_ref: + args.extend(['--target_ref', target_ref]) return args diff --git a/owners_client.py b/owners_client.py index 14c1bf5ef..915412e51 100644 --- a/owners_client.py +++ b/owners_client.py @@ -228,6 +228,6 @@ def GetCodeOwnersClient(root, host, project, branch): Defaults to GerritClient, and falls back to DepotToolsClient if code-owners plugin is not available.""" - if gerrit_util.IsCodeOwnersEnabled(host): + if host and gerrit_util.IsCodeOwnersEnabled(host): return GerritClient(host, project, branch) return DepotToolsClient(root, branch) diff --git a/presubmit_support.py b/presubmit_support.py index 7b0396276..70b348948 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -376,8 +376,10 @@ class GerritAccessor(object): To avoid excessive Gerrit calls, caches the results. """ - def __init__(self, host): + def __init__(self, host, project=None, branch=None): self.host = host + self.project = project + self.branch = branch self.cache = {} def _FetchChangeDetail(self, issue): @@ -650,10 +652,19 @@ class InputApi(object): # Temporary files we must manually remove at the end of a run. self._named_temporary_files = [] - # TODO(dpranke): figure out a list of all approved owners for a repo - # in order to be able to handle wildcard OWNERS files? - self.owners_client = owners_client.DepotToolsClient( - change.RepositoryRoot(), change.UpstreamBranch(), os_path=self.os_path) + host = '' + project = '' + branch = '' + if gerrit_obj: + host = gerrit_obj.host or '' + project = gerrit_obj.project or '' + branch = gerrit_obj.branch or '' + + self.owners_client = owners_client.GetCodeOwnersClient( + root=change.RepositoryRoot(), + host=host, + project=project, + branch=branch) self.owners_db = owners_db.Database( change.RepositoryRoot(), fopen=open, os_path=self.os_path) self.owners_finder = owners_finder.OwnersFinder @@ -1507,7 +1518,7 @@ def DoPostUploadExecuter(change, class PresubmitExecuter(object): def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None, - thread_pool=None, parallel=False, gerrit_project=None): + thread_pool=None, parallel=False): """ Args: change: The Change object. @@ -1525,7 +1536,6 @@ class PresubmitExecuter(object): self.more_cc = [] self.thread_pool = thread_pool self.parallel = parallel - self.gerrit_project = gerrit_project def ExecPresubmitScript(self, script_text, presubmit_path): """Executes a single presubmit script. @@ -1568,9 +1578,10 @@ class PresubmitExecuter(object): # Get the URL of git remote origin and use it to identify host and project host = '' - if self.gerrit and self.gerrit.host: - host = self.gerrit.host - project = self.gerrit_project or '' + project = '' + if self.gerrit: + host = self.gerrit.host or '' + project = self.gerrit.project or '' # Prefix for test names prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path) @@ -1669,8 +1680,7 @@ def DoPresubmitChecks(change, gerrit_obj, dry_run=None, parallel=False, - json_output=None, - gerrit_project=None): + json_output=None): """Runs all presubmit checks that apply to the files in the change. This finds all PRESUBMIT.py files in directories enclosing the files in the @@ -1713,7 +1723,7 @@ def DoPresubmitChecks(change, results = [] thread_pool = ThreadPool() executer = PresubmitExecuter(change, committing, verbose, gerrit_obj, - dry_run, thread_pool, parallel, gerrit_project) + dry_run, thread_pool, parallel) if default_presubmit: if verbose: sys.stdout.write('Running default presubmit script.\n') @@ -1870,11 +1880,15 @@ def _parse_gerrit_options(parser, options): 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. + A GerritAccessor object if options.gerrit_url or options.gerrit_project are + set, or None otherwise. """ gerrit_obj = None - if options.gerrit_url: - gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) + if options.gerrit_url or options.gerrit_project: + gerrit_obj = GerritAccessor( + urlparse.urlparse(options.gerrit_url or '').netloc, + options.gerrit_project, + options.target_ref) if not options.gerrit_fetch: return gerrit_obj @@ -1960,6 +1974,8 @@ def main(argv=None): 'executing presubmit or post-upload hooks. fnmatch ' 'wildcards can also be used.') parser.add_argument('--gerrit_project', help=argparse.SUPPRESS) + parser.add_argument('--target_ref', + help='The remote branch ref to use for the change.') options = parser.parse_args(argv) log_level = logging.ERROR @@ -1992,8 +2008,7 @@ def main(argv=None): gerrit_obj, options.dry_run, options.parallel, - options.json_output, - options.gerrit_project) + options.json_output) except PresubmitFailure as e: print(e, file=sys.stderr) print('Maybe your depot_tools is out of date?', file=sys.stderr) diff --git a/recipes/recipe_modules/presubmit/api.py b/recipes/recipe_modules/presubmit/api.py index 3fbdd2d59..0093a258c 100644 --- a/recipes/recipe_modules/presubmit/api.py +++ b/recipes/recipe_modules/presubmit/api.py @@ -98,6 +98,7 @@ class PresubmitApi(recipe_api.RecipeApi): '--issue', self.m.tryserver.gerrit_change.change, '--patchset', self.m.tryserver.gerrit_change.patchset, '--gerrit_url', 'https://%s' % self.m.tryserver.gerrit_change.host, + '--target_ref', self.m.tryserver.gerrit_change_target_ref, '--gerrit_fetch', ] if self.m.cq.state == self.m.cq.DRY: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index fdc884cad..730084e91 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2958,6 +2958,9 @@ class ChangelistTest(unittest.TestCase): mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start() mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start() mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start() + mock.patch('git_cl.Changelist.GetRemoteBranch', + return_value=('foo', 'bar')).start() + mock.patch('git_cl.GetTargetRef', return_value='ref').start() mock.patch('git_cl.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start() mock.patch('git_cl.Settings.GetRoot', return_value='root').start() mock.patch('git_cl.time_time').start() @@ -3000,6 +3003,7 @@ class ChangelistTest(unittest.TestCase): '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', + '--target_ref', 'ref', '--commit', '--may_prompt', '--parallel', @@ -3048,6 +3052,7 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', + '--target_ref', 'ref', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', @@ -3095,6 +3100,7 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', + '--target_ref', 'ref', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', @@ -3135,6 +3141,7 @@ class ChangelistTest(unittest.TestCase): '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', + '--target_ref', 'ref', '--post_upload', '--description_file', '/tmp/fake-temp1', ]) diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 7695f9ccd..6f6369f4c 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -275,6 +275,13 @@ class GetCodeOwnersClientTest(unittest.TestCase): mock.patch('gerrit_util.IsCodeOwnersEnabled').start() self.addCleanup(mock.patch.stopall) + def testGetCodeOwnersClient_NoHost(self): + owners_client.GetCodeOwnersClient('root', None, 'project', 'branch') + gerrit_util.IsCodeOwnersEnabled.assert_not_called() + + owners_client.GetCodeOwnersClient('root', '', 'project', 'branch') + gerrit_util.IsCodeOwnersEnabled.assert_not_called() + def testGetCodeOwnersClient_GerritClient(self): gerrit_util.IsCodeOwnersEnabled.return_value = True self.assertIsInstance( diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d745c7c40..dad2c4e0d 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -184,6 +184,7 @@ index fe3de7b..54ae6e1 100755 mock.patch('os.path.abspath', lambda f: f).start() mock.patch('os.path.isfile').start() mock.patch('os.remove').start() + mock.patch('owners_client.GetCodeOwnersClient').start() mock.patch('presubmit_support._parse_files').start() mock.patch('presubmit_support.rdb_wrapper.client', return_value=self.rdb_client).start() @@ -1144,9 +1145,10 @@ def CheckChangeOnCommit(input_api, output_api): upstream=options.upstream) scm.GIT.GetAllFiles.assert_called_once_with(options.root) - def testParseGerritOptions_NoGerritUrl(self): + def testParseGerritOptions_NoGerritUrlOrProject(self): options = mock.Mock( gerrit_url=None, + gerrit_project=None, gerrit_fetch=False, author='author', description='description') @@ -1160,14 +1162,16 @@ def CheckChangeOnCommit(input_api, output_api): def testParseGerritOptions_NoGerritFetch(self): options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_project='baz/project', gerrit_fetch=False, + target_ref='refs/heads/foobar', author='author', description='description') gerrit_obj = presubmit._parse_gerrit_options(None, options) presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com') + 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar') self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) self.assertEqual('author', options.author) self.assertEqual('description', options.description) @@ -1181,14 +1185,16 @@ def CheckChangeOnCommit(input_api, output_api): options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_project='baz/project', gerrit_fetch=True, + target_ref='refs/heads/foobar', issue=123, patchset=4) gerrit_obj = presubmit._parse_gerrit_options(None, options) presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com') + 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar') self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) self.assertEqual('new owner', options.author) self.assertEqual('new description', options.description)