diff --git a/git_cl.py b/git_cl.py index 0a94470c3f..2f485b159d 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1050,7 +1050,7 @@ class _ParsedIssueNumberArgument(object): return self.issue is not None -def ParseIssueNumberArgument(arg): +def ParseIssueNumberArgument(arg, codereview=None): """Parses the issue argument and returns _ParsedIssueNumberArgument.""" fail_result = _ParsedIssueNumberArgument() @@ -1064,6 +1064,10 @@ def ParseIssueNumberArgument(arg): except ValueError: return fail_result + if codereview is not None: + parsed = _CODEREVIEW_IMPLEMENTATIONS[codereview].ParseIssueURL(parsed_url) + return parsed or fail_result + results = {} for name, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems(): parsed = cls.ParseIssueURL(parsed_url) @@ -1074,7 +1078,11 @@ def ParseIssueNumberArgument(arg): return fail_result if len(results) == 1: return results.values()[0] - # Choose Rietveld if there are two. + + if parsed_url.netloc and parsed_url.netloc.split('.')[0].endswith('-review'): + # This is likely Gerrit. + return results['gerrit'] + # Choose Rietveld as before if URL can parsed by either. return results['rietveld'] @@ -4422,10 +4430,10 @@ def CMDdescription(parser, args): target_issue_arg = None if len(args) > 0: - target_issue_arg = ParseIssueNumberArgument(args[0]) + target_issue_arg = ParseIssueNumberArgument(args[0], + options.forced_codereview) if not target_issue_arg.valid: parser.error('invalid codereview url or CL id') - return 1 auth_config = auth.extract_auth_config_from_options(options) @@ -4433,14 +4441,23 @@ def CMDdescription(parser, args): 'auth_config': auth_config, 'codereview': options.forced_codereview, } + detected_codereview_from_url = False if target_issue_arg: kwargs['issue'] = target_issue_arg.issue kwargs['codereview_host'] = target_issue_arg.hostname + if target_issue_arg.codereview and not options.forced_codereview: + detected_codereview_from_url = True + kwargs['codereview'] = target_issue_arg.codereview cl = Changelist(**kwargs) - if not cl.GetIssue(): + assert not detected_codereview_from_url DieWithError('This branch has no associated changelist.') + + if detected_codereview_from_url: + logging.info('canonical issue/change URL: %s (type: %s)\n', + cl.GetIssueURL(), target_issue_arg.codereview) + description = ChangeDescription(cl.GetDescription()) if options.display: @@ -5121,7 +5138,6 @@ def CMDpatch(parser, args): _process_codereview_select_options(parser, options) auth_config = auth.extract_auth_config_from_options(options) - if options.reapply: if options.newbranch: parser.error('--reapply works on the current branch only') @@ -5147,6 +5163,22 @@ def CMDpatch(parser, args): if len(args) != 1 or not args[0]: parser.error('Must specify issue number or url') + target_issue_arg = ParseIssueNumberArgument(args[0], + options.forced_codereview) + if not target_issue_arg.valid: + parser.error('invalid codereview url or CL id') + + cl_kwargs = { + 'auth_config': auth_config, + 'codereview_host': target_issue_arg.hostname, + 'codereview': options.forced_codereview, + } + detected_codereview_from_url = False + if target_issue_arg.codereview and not options.forced_codereview: + detected_codereview_from_url = True + cl_kwargs['codereview'] = target_issue_arg.codereview + cl_kwargs['issue'] = target_issue_arg.issue + # We don't want uncommitted changes mixed up with the patch. if git_common.is_dirty_git_tree('patch'): return 1 @@ -5159,7 +5191,7 @@ def CMDpatch(parser, args): elif not GetCurrentBranch(): DieWithError('A branch is required to apply patch. Hint: use -b option.') - cl = Changelist(auth_config=auth_config, codereview=options.forced_codereview) + cl = Changelist(**cl_kwargs) if cl.IsGerrit(): if options.reject: @@ -5169,8 +5201,12 @@ def CMDpatch(parser, args): if options.directory: parser.error('--directory is not supported with Gerrit codereview.') - return cl.CMDPatchIssue(args[0], options.reject, options.nocommit, - options.directory) + if detected_codereview_from_url: + print('canonical issue/change URL: %s (type: %s)\n' % + (cl.GetIssueURL(), target_issue_arg.codereview)) + + return cl.CMDPatchWithParsedIssue(target_issue_arg, options.reject, + options.nocommit, options.directory) def GetTreeStatus(url=None): @@ -5754,7 +5790,7 @@ def CMDcheckout(parser, args): issue_arg = ParseIssueNumberArgument(args[0]) if not issue_arg.valid: parser.error('invalid codereview url or CL id') - return 1 + target_issue = str(issue_arg.issue) def find_issues(issueprefix): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 9c179c6883..70cb28da92 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -469,7 +469,9 @@ class TestParseIssueURL(unittest.TestCase): def test_ParseIssueNumberArgument(self): def test(arg, *args, **kwargs): - self._validate(git_cl.ParseIssueNumberArgument(arg), *args, **kwargs) + codereview_hint = kwargs.pop('hint', None) + self._validate(git_cl.ParseIssueNumberArgument(arg, codereview_hint), + *args, **kwargs) test('123', 123) test('', fail=True) @@ -479,9 +481,13 @@ class TestParseIssueURL(unittest.TestCase): test('ssh://chrome-review.source.com/#/c/123/4/', fail=True) # Rietveld. test('https://codereview.source.com/www123', fail=True) - # Matches both, but we take Rietveld now. + # Matches both, but we take Rietveld now by default. test('https://codereview.source.com/123', 123, None, 'codereview.source.com', 'rietveld') + # Matches both, but we take Gerrit if specifically requested. + test('https://codereview.source.com/123', + 123, None, 'codereview.source.com', 'gerrit', + hint='gerrit') # Gerrrit. test('https://chrome-review.source.com/c/123/4', 123, 4, 'chrome-review.source.com', 'gerrit') @@ -1638,7 +1644,7 @@ class TestGitCl(TestCase): squash=False, squash_mode='override_nosquash') - def test_gerrit_patch_bad_chars(self): + def test_gerrit_patchset_title_bad_chars(self): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self._run_gerrit_upload_test( ['-f', '-t', 'Don\'t put bad cha,.rs'], @@ -1940,9 +1946,10 @@ class TestGitCl(TestCase): ] return calls - def _patch_common(self, is_gerrit=False, force_codereview=False, + def _patch_common(self, default_codereview=None, force_codereview=False, new_branch=False, git_short_host='host', - detect_gerrit_server=True): + detect_gerrit_server=True, + actual_codereview=None): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset', lambda x: '60001') @@ -1955,25 +1962,43 @@ class TestGitCl(TestCase): else: self.calls = [((['git', 'symbolic-ref', 'HEAD'],), 'master')] - if not force_codereview: - # These calls detect codereview to use. - self.calls += [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), CERR1), - ((['git', 'config', 'branch.master.gerritissue'],), CERR1), - ((['git', 'config', 'rietveld.autoupdate'],), CERR1), - ] - - if is_gerrit: + if default_codereview: if not force_codereview: + # These calls detect codereview to use. + self.calls += [ + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.rietveldissue'],), CERR1), + ((['git', 'config', 'branch.master.gerritissue'],), CERR1), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), + ] + if default_codereview == 'gerrit': + if not force_codereview: + self.calls += [ + ((['git', 'config', 'gerrit.host'],), 'true'), + ] + if detect_gerrit_server: + self.calls += self._get_gerrit_codereview_server_calls( + 'master', git_short_host=git_short_host, + detect_branch=not new_branch and force_codereview) + actual_codereview = 'gerrit' + + elif default_codereview == 'rietveld': self.calls += [ - ((['git', 'config', 'gerrit.host'],), 'true'), + ((['git', 'config', 'gerrit.host'],), CERR1), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'config', 'branch.master.rietveldserver',],), CERR1), ] - if detect_gerrit_server: - self.calls += self._get_gerrit_codereview_server_calls( - 'master', git_short_host=git_short_host, - detect_branch=not new_branch and force_codereview) + actual_codereview = 'rietveld' + if actual_codereview == 'rietveld': + self.calls += [ + ((['git', 'rev-parse', '--show-cdup'],), ''), + ] + if not default_codereview: + self.calls += [ + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ] + elif actual_codereview == 'gerrit': self.calls += [ (('GetChangeDetail', git_short_host + '-review.googlesource.com', '123456', ['ALL_REVISIONS', 'CURRENT_COMMIT']), @@ -1997,16 +2022,9 @@ class TestGitCl(TestCase): }, }), ] - else: - self.calls += [ - ((['git', 'config', 'gerrit.host'],), CERR1), - ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), - ((['git', 'config', 'branch.master.rietveldserver',],), CERR1), - ((['git', 'rev-parse', '--show-cdup'],), ''), - ] - def _common_patch_successful(self, new_branch=False): - self._patch_common(new_branch=new_branch) + def _common_patch_rietveld_successful(self, **kwargs): + self._patch_common(**kwargs) self.calls += [ ((['git', 'config', 'branch.master.rietveldissue', '123456'],), ''), @@ -2020,21 +2038,29 @@ class TestGitCl(TestCase): '(http://crrev.com/123456#ps60001)'],), ''), ] - def test_patch_successful(self): - self._common_patch_successful() + def test_patch_rietveld_default(self): + self._common_patch_rietveld_successful(default_codereview='rietveld') self.assertEqual(git_cl.main(['patch', '123456']), 0) - def test_patch_successful_new_branch(self): - self._common_patch_successful(new_branch=True) + def test_patch_rietveld_successful_new_branch(self): + self._common_patch_rietveld_successful(default_codereview='rietveld', + new_branch=True) self.assertEqual(git_cl.main(['patch', '-b', 'master', '123456']), 0) - def test_patch_conflict(self): - self._patch_common() + def test_patch_rietveld_guess_by_url(self): + self._common_patch_rietveld_successful( + default_codereview=None, # It doesn't matter. + actual_codereview='rietveld') + self.assertEqual(git_cl.main( + ['patch', 'https://codereview.example.com/123456']), 0) + + def test_patch_rietveld_conflict(self): + self._patch_common(default_codereview='rietveld') GitCheckoutMock.conflict = True self.assertNotEqual(git_cl.main(['patch', '123456']), 0) - def test_gerrit_patch_successful(self): - self._patch_common(is_gerrit=True, git_short_host='chromium', + def test_patch_gerrit_default(self): + self._patch_common(default_codereview='gerrit', git_short_host='chromium', detect_gerrit_server=True) self.calls += [ ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', @@ -2048,8 +2074,8 @@ class TestGitCl(TestCase): ] self.assertEqual(git_cl.main(['patch', '123456']), 0) - def test_patch_force_codereview(self): - self._patch_common(is_gerrit=True, force_codereview=True, + def test_patch_gerrit_force(self): + self._patch_common(default_codereview='gerrit', force_codereview=True, git_short_host='host', detect_gerrit_server=True) self.calls += [ ((['git', 'fetch', 'https://host.googlesource.com/my/repo', @@ -2063,12 +2089,15 @@ class TestGitCl(TestCase): ] self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0) - def test_gerrit_patch_url_successful(self): - self._patch_common(is_gerrit=True, git_short_host='else', - detect_gerrit_server=False) + def test_patch_gerrit_guess_by_url(self): + self._patch_common( + default_codereview=None, # It doesn't matter what's default. + actual_codereview='gerrit', + git_short_host='else', detect_gerrit_server=False) self.calls += [ ((['git', 'fetch', 'https://else.googlesource.com/my/repo', 'refs/changes/56/123456/1'],), ''), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), ((['git', 'config', 'branch.master.gerritserver', @@ -2079,32 +2108,49 @@ class TestGitCl(TestCase): self.assertEqual(git_cl.main( ['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0) - def test_gerrit_patch_conflict(self): - self._patch_common(is_gerrit=True, detect_gerrit_server=False, + def test_patch_gerrit_guess_by_url_like_rietveld(self): + self._patch_common( + default_codereview=None, # It doesn't matter what's default. + actual_codereview='gerrit', + git_short_host='else', detect_gerrit_server=False) + self.calls += [ + ((['git', 'fetch', 'https://else.googlesource.com/my/repo', + 'refs/changes/56/123456/1'],), ''), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.gerritissue', '123456'],), + ''), + ((['git', 'config', 'branch.master.gerritserver', + 'https://else-review.googlesource.com'],), ''), + ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''), + ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), + ] + self.assertEqual(git_cl.main( + ['patch', 'https://else-review.googlesource.com/123456/1']), 0) + + def test_patch_gerrit_conflict(self): + self._patch_common(default_codereview='gerrit', detect_gerrit_server=True, git_short_host='chromium') self.calls += [ ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', - 'refs/changes/56/123456/1'],), ''), + 'refs/changes/56/123456/7'],), ''), ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), ((['git', 'config', 'branch.master.gerritserver', 'https://chromium-review.googlesource.com'],), ''), - ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''), + ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1), ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],), SystemExitMock()), ] with self.assertRaises(SystemExitMock): - git_cl.main(['patch', - 'https://chromium-review.googlesource.com/#/c/123456/1']) + git_cl.main(['patch', '123456']) - def test_gerrit_patch_not_exists(self): + def test_patch_gerrit_not_exists(self): def notExists(_issue, *_, **kwargs): self.assertFalse(kwargs['ignore_404']) raise git_cl.gerrit_util.GerritError(404, '') self.mock(git_cl.gerrit_util, 'GetChangeDetail', notExists) - url = 'https://chromium-review.googlesource.com' self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), @@ -2112,11 +2158,17 @@ class TestGitCl(TestCase): ((['git', 'config', 'branch.master.gerritissue'],), CERR1), ((['git', 'config', 'rietveld.autoupdate'],), CERR1), ((['git', 'config', 'gerrit.host'],), 'true'), - ((['DieWithError', 'change 123456 at ' + url + ' does not exist ' - 'or you have no access to it'],), SystemExitMock()), + ((['git', 'config', 'branch.master.gerritserver'],), CERR1), + ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'), + ((['git', 'config', 'branch.master.remote'],), 'origin'), + ((['git', 'config', 'remote.origin.url'],), + 'https://chromium.googlesource.com/my/repo'), + ((['DieWithError', + 'change 123456 at https://chromium-review.googlesource.com does not ' + 'exist or you have no access to it'],), SystemExitMock()), ] with self.assertRaises(SystemExitMock): - self.assertEqual(1, git_cl.main(['patch', url + '/#/c/123456/1'])) + self.assertEqual(1, git_cl.main(['patch', '123456'])) def _checkout_calls(self): return [