From aa5ced1815929e9c8cf9863d239b774adf5fde73 Mon Sep 17 00:00:00 2001 From: "tandrii@chromium.org" Date: Tue, 29 Mar 2016 09:41:14 +0000 Subject: [PATCH] git cl: Rework Changelist class for Rietveld/Gerrit use. This adds pluggable codereview-specific implementations into Changelist class. The specific implementation is chosen at Changelist automatically, with Rietveld being default for backwards compatibility. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds a few extra call to git config in order to determine which codereview to use, but but it shouldn't have any performance impact. These is a reland of these 4 CLs + with several fixes. patch from issue 1827523003 at patchset 20001 (http://crrev.com/1827523003#ps20001) patch from issue 1830703004 at patchset 1 (http://crrev.com/1830703004#ps1) patch from issue 1830923002 at patchset 60001 (http://crrev.com/1830923002#ps60001) patch from issue 1805193002 at patchset 380001 (http://crrev.com/1805193002#ps380001) This CL without a fix was also committed and reverted as patch from issue 1830973003 at patchset 40001 (http://crrev.com/1830973003#ps40001) R=machenbach@chromium.org,sergiyb@chromium.org,andybons@chromium.org BUG=579160,597638 Review URL: https://codereview.chromium.org/1838143002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@299530 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl.py | 525 ++++++++++++++++++++++++++++++++----------- tests/git_cl_test.py | 57 ++--- 2 files changed, 427 insertions(+), 155 deletions(-) diff --git a/git_cl.py b/git_cl.py index 59e412123..587fe6405 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5,7 +5,7 @@ # Copyright (C) 2008 Evan Martin -"""A git-command for integrating reviews on Rietveld.""" +"""A git-command for integrating reviews on Rietveld and Gerrit.""" from distutils.version import LooseVersion from multiprocessing.pool import ThreadPool @@ -47,6 +47,7 @@ import commit_queue import dart_format import fix_encoding import gclient_utils +import gerrit_util import git_cache import git_common import git_footers @@ -156,7 +157,7 @@ def ask_for_data(prompt): def git_set_branch_value(key, value): - branch = Changelist().GetBranch() + branch = GetCurrentBranch() if not branch: return @@ -168,7 +169,7 @@ def git_set_branch_value(key, value): def git_get_branch_default(key, default): - branch = Changelist().GetBranch() + branch = GetCurrentBranch() if branch: git_key = 'branch.%s.%s' % (branch, key) (_, stdout) = RunGitWithCode(['config', '--int', '--get', git_key]) @@ -813,23 +814,61 @@ class Settings(object): def ShortBranchName(branch): """Convert a name like 'refs/heads/foo' to just 'foo'.""" - return branch.replace('refs/heads/', '') + return branch.replace('refs/heads/', '', 1) + + +def GetCurrentBranchRef(): + """Returns branch ref (e.g., refs/heads/master) or None.""" + return RunGit(['symbolic-ref', 'HEAD'], + stderr=subprocess2.VOID, error_ok=True).strip() or None + + +def GetCurrentBranch(): + """Returns current branch or None. + + For refs/heads/* branches, returns just last part. For others, full ref. + """ + branchref = GetCurrentBranchRef() + if branchref: + return ShortBranchName(branchref) + return None class Changelist(object): - def __init__(self, branchref=None, issue=None, auth_config=None): + """Changelist works with one changelist in local branch. + + Supports two codereview backends: Rietveld or Gerrit, selected at object + creation. + + Not safe for concurrent multi-{thread,process} use. + """ + + def __init__(self, branchref=None, issue=None, codereview=None, **kwargs): + """Create a new ChangeList instance. + + If issue is given, the codereview must be given too. + + If `codereview` is given, it must be 'rietveld' or 'gerrit'. + Otherwise, it's decided based on current configuration of the local branch, + with default being 'rietveld' for backwards compatibility. + See _load_codereview_impl for more details. + + **kwargs will be passed directly to codereview implementation. + """ # Poke settings so we get the "configure your server" message if necessary. global settings if not settings: # Happens when git_cl.py is used as a utility library. settings = Settings() - settings.GetDefaultServerUrl() + + if issue: + assert codereview, 'codereview must be known, if issue is known' + self.branchref = branchref if self.branchref: self.branch = ShortBranchName(self.branchref) else: self.branch = None - self.rietveld_server = None self.upstream_branch = None self.lookedup_issue = False self.issue = issue or None @@ -839,14 +878,40 @@ class Changelist(object): self.patchset = None self.cc = None self.watchers = () - self._auth_config = auth_config - self._props = None self._remote = None - self._rpc_server = None - @property - def auth_config(self): - return self._auth_config + self._codereview_impl = None + self._load_codereview_impl(codereview, **kwargs) + + def _load_codereview_impl(self, codereview=None, **kwargs): + if codereview: + codereview = codereview.lower() + if codereview == 'gerrit': + self._codereview_impl = _GerritChangelistImpl(self, **kwargs) + elif codereview == 'rietveld': + self._codereview_impl = _RietveldChangelistImpl(self, **kwargs) + else: + assert codereview in ('rietveld', 'gerrit') + return + + # Automatic selection based on issue number set for a current branch. + # Rietveld takes precedence over Gerrit. + assert not self.issue + # Whether we find issue or not, we are doing the lookup. + self.lookedup_issue = True + for cls in [_RietveldChangelistImpl, _GerritChangelistImpl]: + setting = cls.IssueSetting(self.GetBranch()) + issue = RunGit(['config', setting], error_ok=True).strip() + if issue: + self._codereview_impl = cls(self, **kwargs) + self.issue = int(issue) + return + + # No issue is set for this branch, so decide based on repo-wide settings. + return self._load_codereview_impl( + codereview='gerrit' if settings.GetIsGerrit() else 'rietveld', + **kwargs) + def GetCCList(self): """Return the users cc'd on this CL. @@ -874,8 +939,7 @@ class Changelist(object): def GetBranch(self): """Returns the short branch name, e.g. 'master'.""" if not self.branch: - branchref = RunGit(['symbolic-ref', 'HEAD'], - stderr=subprocess2.VOID, error_ok=True).strip() + branchref = GetCurrentBranchRef() if not branchref: return None self.branchref = branchref @@ -919,11 +983,12 @@ class Changelist(object): remote = 'origin' upstream_branch = 'refs/heads/trunk' else: - DieWithError("""Unable to determine default branch to diff against. -Either pass complete "git diff"-style arguments, like - git cl upload origin/master -or verify this branch is set up to track another (via the --track argument to -"git checkout -b ...").""") + DieWithError( + 'Unable to determine default branch to diff against.\n' + 'Either pass complete "git diff"-style arguments, like\n' + ' git cl upload origin/master\n' + 'or verify this branch is set up to track another \n' + '(via the --track argument to "git checkout -b ...").') return remote, upstream_branch @@ -1065,65 +1130,24 @@ or verify this branch is set up to track another (via the --track argument to def GetIssue(self): """Returns the issue number as a int or None if not set.""" if self.issue is None and not self.lookedup_issue: - issue = RunGit(['config', self._IssueSetting()], error_ok=True).strip() + issue = RunGit(['config', + self._codereview_impl.IssueSetting(self.GetBranch())], + error_ok=True).strip() self.issue = int(issue) or None if issue else None self.lookedup_issue = True return self.issue - def GetRietveldServer(self): - if not self.rietveld_server: - # If we're on a branch then get the server potentially associated - # with that branch. - if self.GetIssue(): - rietveld_server_config = self._RietveldServer() - if rietveld_server_config: - self.rietveld_server = gclient_utils.UpgradeToHttps(RunGit( - ['config', rietveld_server_config], error_ok=True).strip()) - if not self.rietveld_server: - self.rietveld_server = settings.GetDefaultServerUrl() - return self.rietveld_server - - def GetGerritServer(self): - # We don't support multiple Gerrit servers, and assume it to be same as - # origin, except with a '-review' suffix for first subdomain. - parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.') - parts[0] = parts[0] + '-review' - return 'https://%s' % '.'.join(parts) - def GetIssueURL(self): """Get the URL for a particular issue.""" - if not self.GetIssue(): + issue = self.GetIssue() + if not issue: return None - if settings.GetIsGerrit(): - return '%s/%s' % (self.GetGerritServer(), self.GetIssue()) - return '%s/%s' % (self.GetRietveldServer(), self.GetIssue()) + return '%s/%s' % (self._codereview_impl.GetCodereviewServer(), issue) def GetDescription(self, pretty=False): if not self.has_description: if self.GetIssue(): - issue = self.GetIssue() - try: - self.description = self.RpcServer().get_description(issue).strip() - except urllib2.HTTPError as e: - if e.code == 404: - DieWithError( - ('\nWhile fetching the description for issue %d, received a ' - '404 (not found)\n' - 'error. It is likely that you deleted this ' - 'issue on the server. If this is the\n' - 'case, please run\n\n' - ' git cl issue 0\n\n' - 'to clear the association with the deleted issue. Then run ' - 'this command again.') % issue) - else: - DieWithError( - '\nFailed to fetch issue description. HTTP error %d' % e.code) - except urllib2.URLError as e: - print >> sys.stderr, ( - 'Warning: Failed to retrieve CL description due to network ' - 'failure.') - self.description = '' - + self.description = self._codereview_impl.FetchDescription() self.has_description = True if pretty: wrapper = textwrap.TextWrapper() @@ -1134,7 +1158,7 @@ or verify this branch is set up to track another (via the --track argument to def GetPatchset(self): """Returns the patchset number as a int or None if not set.""" if self.patchset is None and not self.lookedup_patchset: - patchset = RunGit(['config', self._PatchsetSetting()], + patchset = RunGit(['config', self._codereview_impl.PatchsetSetting()], error_ok=True).strip() self.patchset = int(patchset) or None if patchset else None self.lookedup_patchset = True @@ -1142,47 +1166,29 @@ or verify this branch is set up to track another (via the --track argument to def SetPatchset(self, patchset): """Set this branch's patchset. If patchset=0, clears the patchset.""" + patchset_setting = self._codereview_impl.PatchsetSetting() if patchset: - RunGit(['config', self._PatchsetSetting(), str(patchset)]) + RunGit(['config', patchset_setting, str(patchset)]) self.patchset = patchset else: - RunGit(['config', '--unset', self._PatchsetSetting()], + RunGit(['config', '--unset', patchset_setting], stderr=subprocess2.PIPE, error_ok=True) self.patchset = None - def GetMostRecentPatchset(self): - return self.GetIssueProperties()['patchsets'][-1] - - def GetPatchSetDiff(self, issue, patchset): - return self.RpcServer().get( - '/download/issue%s_%s.diff' % (issue, patchset)) - - def GetIssueProperties(self): - if self._props is None: - issue = self.GetIssue() - if not issue: - self._props = {} - else: - self._props = self.RpcServer().get_issue_properties(issue, True) - return self._props - - def GetApprovingReviewers(self): - return get_approving_reviewers(self.GetIssueProperties()) - - def AddComment(self, message): - return self.RpcServer().add_comment(self.GetIssue(), message) - def SetIssue(self, issue=None): """Set this branch's issue. If issue isn't given, clears the issue.""" + issue_setting = self._codereview_impl.IssueSetting(self.GetBranch()) + codereview_setting = self._codereview_impl.GetCodereviewServerSetting() if issue: self.issue = issue - RunGit(['config', self._IssueSetting(), str(issue)]) - if not settings.GetIsGerrit() and self.rietveld_server: - RunGit(['config', self._RietveldServer(), self.rietveld_server]) + RunGit(['config', issue_setting, str(issue)]) + codereview_server = self._codereview_impl.GetCodereviewServer() + if codereview_server: + RunGit(['config', codereview_setting, codereview_server]) else: current_issue = self.GetIssue() if current_issue: - RunGit(['config', '--unset', self._IssueSetting()]) + RunGit(['config', '--unset', issue_setting]) self.issue = None self.SetPatchset(None) @@ -1232,6 +1238,186 @@ or verify this branch is set up to track another (via the --track argument to author, upstream=upstream_branch) + def UpdateDescription(self, description): + self.description = description + return self._codereview_impl.UpdateDescriptionRemote(description) + + def RunHook(self, committing, may_prompt, verbose, change): + """Calls sys.exit() if the hook fails; returns a HookResults otherwise.""" + try: + return presubmit_support.DoPresubmitChecks(change, committing, + verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin, + default_presubmit=None, may_prompt=may_prompt, + rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit()) + except presubmit_support.PresubmitFailure, e: + DieWithError( + ('%s\nMaybe your depot_tools is out of date?\n' + 'If all fails, contact maruel@') % e) + + # Forward methods to codereview specific implementation. + + def CloseIssue(self): + return self._codereview_impl.CloseIssue() + + def GetStatus(self): + return self._codereview_impl.GetStatus() + + def GetCodereviewServer(self): + return self._codereview_impl.GetCodereviewServer() + + def GetApprovingReviewers(self): + return self._codereview_impl.GetApprovingReviewers() + + def GetMostRecentPatchset(self): + return self._codereview_impl.GetMostRecentPatchset() + + def __getattr__(self, attr): + # This is because lots of untested code accesses Rietveld-specific stuff + # directly, and it's hard to fix for sure. So, just let it work, and fix + # on a cases by case basis. + return getattr(self._codereview_impl, attr) + + +class _ChangelistCodereviewBase(object): + """Abstract base class encapsulating codereview specifics of a changelist.""" + def __init__(self, changelist): + self._changelist = changelist # instance of Changelist + + def __getattr__(self, attr): + # Forward methods to changelist. + # TODO(tandrii): maybe clean up _GerritChangelistImpl and + # _RietveldChangelistImpl to avoid this hack? + return getattr(self._changelist, attr) + + def GetStatus(self): + """Apply a rough heuristic to give a simple summary of an issue's review + or CQ status, assuming adherence to a common workflow. + + Returns None if no issue for this branch, or specific string keywords. + """ + raise NotImplementedError() + + def GetCodereviewServer(self): + """Returns server URL without end slash, like "https://codereview.com".""" + raise NotImplementedError() + + def FetchDescription(self): + """Fetches and returns description from the codereview server.""" + raise NotImplementedError() + + def GetCodereviewServerSetting(self): + """Returns git config setting for the codereview server.""" + raise NotImplementedError() + + @staticmethod + def IssueSetting(branch): + """Returns name of git config setting which stores issue number for a given + branch.""" + raise NotImplementedError() + + def PatchsetSetting(self): + """Returns name of git config setting which stores issue number.""" + raise NotImplementedError() + + def GetRieveldObjForPresubmit(self): + # This is an unfortunate Rietveld-embeddedness in presubmit. + # For non-Rietveld codereviews, this probably should return a dummy object. + raise NotImplementedError() + + def UpdateDescriptionRemote(self, description): + """Update the description on codereview site.""" + raise NotImplementedError() + + def CloseIssue(self): + """Closes the issue.""" + raise NotImplementedError() + + def GetApprovingReviewers(self): + """Returns a list of reviewers approving the change. + + Note: not necessarily committers. + """ + raise NotImplementedError() + + def GetMostRecentPatchset(self): + """Returns the most recent patchset number from the codereview site.""" + raise NotImplementedError() + + +class _RietveldChangelistImpl(_ChangelistCodereviewBase): + def __init__(self, changelist, auth_config=None, rietveld_server=None): + super(_RietveldChangelistImpl, self).__init__(changelist) + assert settings, 'must be initialized in _ChangelistCodereviewBase' + settings.GetDefaultServerUrl() + + self._rietveld_server = rietveld_server + self._auth_config = auth_config + self._props = None + self._rpc_server = None + + def GetAuthConfig(self): + return self._auth_config + + def GetCodereviewServer(self): + if not self._rietveld_server: + # If we're on a branch then get the server potentially associated + # with that branch. + if self.GetIssue(): + rietveld_server_setting = self.GetCodereviewServerSetting() + if rietveld_server_setting: + self._rietveld_server = gclient_utils.UpgradeToHttps(RunGit( + ['config', rietveld_server_setting], error_ok=True).strip()) + if not self._rietveld_server: + self._rietveld_server = settings.GetDefaultServerUrl() + return self._rietveld_server + + def FetchDescription(self): + issue = self.GetIssue() + assert issue + try: + return self.RpcServer().get_description(issue).strip() + except urllib2.HTTPError as e: + if e.code == 404: + DieWithError( + ('\nWhile fetching the description for issue %d, received a ' + '404 (not found)\n' + 'error. It is likely that you deleted this ' + 'issue on the server. If this is the\n' + 'case, please run\n\n' + ' git cl issue 0\n\n' + 'to clear the association with the deleted issue. Then run ' + 'this command again.') % issue) + else: + DieWithError( + '\nFailed to fetch issue description. HTTP error %d' % e.code) + except urllib2.URLError as e: + print >> sys.stderr, ( + 'Warning: Failed to retrieve CL description due to network ' + 'failure.') + return '' + + def GetMostRecentPatchset(self): + return self.GetIssueProperties()['patchsets'][-1] + + def GetPatchSetDiff(self, issue, patchset): + return self.RpcServer().get( + '/download/issue%s_%s.diff' % (issue, patchset)) + + def GetIssueProperties(self): + if self._props is None: + issue = self.GetIssue() + if not issue: + self._props = {} + else: + self._props = self.RpcServer().get_issue_properties(issue, True) + return self._props + + def GetApprovingReviewers(self): + return get_approving_reviewers(self.GetIssueProperties()) + + def AddComment(self, message): + return self.RpcServer().add_comment(self.GetIssue(), message) + def GetStatus(self): """Apply a rough heuristic to give a simple summary of an issue's review or CQ status, assuming adherence to a common workflow. @@ -1279,26 +1465,11 @@ or verify this branch is set up to track another (via the --track argument to return 'reply' return 'waiting' - def RunHook(self, committing, may_prompt, verbose, change): - """Calls sys.exit() if the hook fails; returns a HookResults otherwise.""" - - try: - return presubmit_support.DoPresubmitChecks(change, committing, - verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin, - default_presubmit=None, may_prompt=may_prompt, - rietveld_obj=self.RpcServer()) - except presubmit_support.PresubmitFailure, e: - DieWithError( - ('%s\nMaybe your depot_tools is out of date?\n' - 'If all fails, contact maruel@') % e) - - def UpdateDescription(self, description): - self.description = description + def UpdateDescriptionRemote(self, description): return self.RpcServer().update_description( self.GetIssue(), self.description) def CloseIssue(self): - """Updates the description and closes the issue.""" return self.RpcServer().close_issue(self.GetIssue()) def SetFlag(self, flag, value): @@ -1322,25 +1493,116 @@ or verify this branch is set up to track another (via the --track argument to """ if not self._rpc_server: self._rpc_server = rietveld.CachingRietveld( - self.GetRietveldServer(), + self.GetCodereviewServer(), self._auth_config or auth.make_auth_config()) return self._rpc_server - def _IssueSetting(self): - """Return the git setting that stores this change's issue.""" - return 'branch.%s.rietveldissue' % self.GetBranch() + @staticmethod + def IssueSetting(branch): + return 'branch.%s.rietveldissue' % branch - def _PatchsetSetting(self): + def PatchsetSetting(self): """Return the git setting that stores this change's most recent patchset.""" return 'branch.%s.rietveldpatchset' % self.GetBranch() - def _RietveldServer(self): + def GetCodereviewServerSetting(self): """Returns the git setting that stores this change's rietveld server.""" branch = self.GetBranch() if branch: return 'branch.%s.rietveldserver' % branch return None + def GetRieveldObjForPresubmit(self): + return self.RpcServer() + + +class _GerritChangelistImpl(_ChangelistCodereviewBase): + def __init__(self, changelist, auth_config=None): + # auth_config is Rietveld thing, kept here to preserve interface only. + super(_GerritChangelistImpl, self).__init__(changelist) + self._change_id = None + self._gerrit_server = None # e.g. https://chromium-review.googlesource.com + self._gerrit_host = None # e.g. chromium-review.googlesource.com + + def _GetGerritHost(self): + # Lazy load of configs. + self.GetCodereviewServer() + return self._gerrit_host + + def GetCodereviewServer(self): + if not self._gerrit_server: + # If we're on a branch then get the server potentially associated + # with that branch. + if self.GetIssue(): + gerrit_server_setting = self.GetCodereviewServerSetting() + if gerrit_server_setting: + self._gerrit_server = RunGit(['config', gerrit_server_setting], + error_ok=True).strip() + if self._gerrit_server: + self._gerrit_host = urlparse.urlparse(self._gerrit_server).netloc + if not self._gerrit_server: + # We assume repo to be hosted on Gerrit, and hence Gerrit server + # has "-review" suffix for lowest level subdomain. + parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.') + parts[0] = parts[0] + '-review' + self._gerrit_host = '.'.join(parts) + self._gerrit_server = 'https://%s' % self._gerrit_host + return self._gerrit_server + + @staticmethod + def IssueSetting(branch): + return 'branch.%s.gerritissue' % branch + + def PatchsetSetting(self): + """Return the git setting that stores this change's most recent patchset.""" + return 'branch.%s.gerritpatchset' % self.GetBranch() + + def GetCodereviewServerSetting(self): + """Returns the git setting that stores this change's Gerrit server.""" + branch = self.GetBranch() + if branch: + return 'branch.%s.gerritserver' % branch + return None + + def GetRieveldObjForPresubmit(self): + class ThisIsNotRietveldIssue(object): + def __nonzero__(self): + # This is a hack to make presubmit_support think that rietveld is not + # defined, yet still ensure that calls directly result in a decent + # exception message below. + return False + + def __getattr__(self, attr): + print( + 'You aren\'t using Rietveld at the moment, but Gerrit.\n' + 'Using Rietveld in your PRESUBMIT scripts won\'t work.\n' + 'Please, either change your PRESUBIT to not use rietveld_obj.%s,\n' + 'or use Rietveld for codereview.\n' + 'See also http://crbug.com/579160.' % attr) + raise NotImplementedError() + return ThisIsNotRietveldIssue() + + def GetStatus(self): + # TODO(tandrii) + raise NotImplementedError() + + def GetMostRecentPatchset(self): + data = gerrit_util.GetChangeDetail(self._GetGerritHost(), self.GetIssue(), + ['CURRENT_REVISION']) + return data['revisions'][data['current_revision']]['_number'] + + def FetchDescription(self): + data = gerrit_util.GetChangeDetail(self._GetGerritHost(), self.GetIssue(), + ['COMMIT_FOOTERS', 'CURRENT_REVISION']) + return data['revisions'][data['current_revision']]['commit_with_footers'] + + def UpdateDescriptionRemote(self, description): + # TODO(tandrii) + raise NotImplementedError() + + def CloseIssue(self): + gerrit_util.AbandonChange(self._GetGerritHost(), self.GetIssue(), msg='') + class ChangeDescription(object): """Contains a parsed form of the change description.""" @@ -1886,6 +2148,7 @@ def CMDstatus(parser, args): changes = ( Changelist(branchref=b, auth_config=auth_config) for b in branches.splitlines()) + # TODO(tandrii): refactor to use CLs list instead of branches list. branches = [c.GetBranch() for c in changes] alignment = max(5, max(len(b) for b in branches)) print 'Branches associated with reviews:' @@ -2001,7 +2264,7 @@ def CMDcomments(parser, args): except ValueError: DieWithError('A review issue id is expected to be a number') - cl = Changelist(issue=issue, auth_config=auth_config) + cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config) if options.comment: cl.AddComment(options.comment) @@ -2380,8 +2643,10 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix): def RietveldUpload(options, args, cl, change): """upload the patch to rietveld.""" upload_args = ['--assume_yes'] # Don't ask about untracked files. - upload_args.extend(['--server', cl.GetRietveldServer()]) - upload_args.extend(auth.auth_config_to_command_options(cl.auth_config)) + upload_args.extend(['--server', cl.GetCodereviewServer()]) + # TODO(tandrii): refactor this ugliness into _RietveldChangelistImpl. + upload_args.extend(auth.auth_config_to_command_options( + cl._codereview_impl.GetAuthConfig())) if options.emulate_svn_auto_props: upload_args.append('--emulate_svn_auto_props') @@ -2624,9 +2889,9 @@ def CMDupload(parser, args): # during the actual upload. if not settings.GetIsGerrit() and auth_config.use_oauth2: authenticator = auth.get_authenticator_for_host( - cl.GetRietveldServer(), auth_config) + cl.GetCodereviewServer(), auth_config) if not authenticator.has_cached_credentials(): - raise auth.LoginRequiredError(cl.GetRietveldServer()) + raise auth.LoginRequiredError(cl.GetCodereviewServer()) # Apply watchlists on upload. change = cl.GetChange(base_branch, None) @@ -3220,12 +3485,13 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config): # consequences of the caller not checking this could be dire. assert(not git_common.is_dirty_git_tree('apply')) + # TODO(tandrii): implement for Gerrit. if type(issue_arg) is int or issue_arg.isdigit(): # Input is an issue id. Figure out the URL. issue = int(issue_arg) - cl = Changelist(issue=issue, auth_config=auth_config) + cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config) patchset = cl.GetMostRecentPatchset() - patch_data = cl.GetPatchSetDiff(issue, patchset) + patch_data = cl._codereview_impl.GetPatchSetDiff(issue, patchset) else: # Assume it's a URL to the patch. Default to https. issue_url = gclient_utils.UpgradeToHttps(issue_arg) @@ -3234,8 +3500,8 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config): DieWithError('Must pass an issue ID or full URL for ' '\'Download raw patch set\'') issue = int(match.group(2)) - cl = Changelist(issue=issue, auth_config=auth_config) - cl.rietveld_server = match.group(1) + cl = Changelist(issue=issue, codereview='rietveld', + rietvled_server=match.group(1), auth_config=auth_config) patchset = int(match.group(3)) patch_data = urllib2.urlopen(issue_arg).read() @@ -3279,7 +3545,8 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config): 'patch from issue %(i)s at patchset ' '%(p)s (http://crrev.com/%(i)s#ps%(p)s)' % {'i': issue, 'p': patchset})]) - cl = Changelist(auth_config=auth_config) + cl = Changelist(codereview='rietveld', auth_config=auth_config, + rietveld_server=cl.GetCodereviewServer()) cl.SetIssue(issue) cl.SetPatchset(patchset) print "Committed patch locally." diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 7da5aaefc..e89b4cc1e 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -163,20 +163,21 @@ class TestGitCl(TestCase): '-M'+similarity, 'fake_ancestor_sha', 'HEAD'],), '+dat') return [ - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), - 'codereview.example.com'), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), similarity_call, ((['git', 'symbolic-ref', 'HEAD'],), 'master'), find_copies_call, ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.rietveldissue'],), ''), + ((['git', 'config', 'branch.master.gerritissue'],), ''), + ((['git', 'config', 'rietveld.autoupdate'],), ''), + ((['git', 'config', 'gerrit.host'],), ''), + ((['git', 'config', 'rietveld.server'],), + 'codereview.example.com'), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), ((['get_or_create_merge_base', 'master', 'master'],), 'fake_ancestor_sha'), - ((['git', 'config', 'gerrit.host'],), ''), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [ ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'rev-parse', 'HEAD'],), '12345'), @@ -279,8 +280,6 @@ class TestGitCl(TestCase): 'svn-remote.svn.fetch trunk/src:refs/remotes/origin/master'), None), 0)), - ((['git', - 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), ((['git', 'config', '--int', '--get', 'branch.working.git-cl-similarity'],), ''), @@ -288,6 +287,10 @@ class TestGitCl(TestCase): ((['git', 'config', '--int', '--get', 'branch.working.git-find-copies'],), ''), ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), + ((['git', + 'config', 'branch.working.rietveldissue'],), '12345'), + ((['git', + 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'config', 'branch.working.merge'],), 'refs/heads/master'), ((['git', 'config', 'branch.working.remote'],), 'origin'), @@ -321,8 +324,6 @@ class TestGitCl(TestCase): 'diff', '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), 'M\tPRESUBMIT.py'), - ((['git', - 'config', 'branch.working.rietveldissue'],), '12345'), ((['git', 'config', 'branch.working.rietveldpatchset'],), '31137'), ((['git', 'config', 'branch.working.rietveldserver'],), @@ -334,8 +335,6 @@ class TestGitCl(TestCase): @classmethod def _dcommit_calls_bypassed(cls): return [ - ((['git', - 'config', 'branch.working.rietveldissue'],), '12345'), ((['git', 'config', 'branch.working.rietveldserver'],), 'codereview.example.com'), ] @@ -343,7 +342,6 @@ class TestGitCl(TestCase): @classmethod def _dcommit_calls_3(cls): return [ - ((['git', 'config', 'gerrit.host'],), ''), ((['git', 'diff', '--no-ext-diff', '--stat', '--find-copies-harder', '-l100000', '-C50', 'fake_ancestor_sha', @@ -546,10 +544,6 @@ class TestGitCl(TestCase): @classmethod def _gerrit_base_calls(cls): return [ - ((['git', 'config', 'rietveld.autoupdate'],), - ''), - ((['git', - 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', '--int', '--get', 'branch.master.git-cl-similarity'],), ''), @@ -557,21 +551,22 @@ class TestGitCl(TestCase): ((['git', 'config', '--int', '--get', 'branch.master.git-find-copies'],), ''), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.rietveldissue'],), ''), + ((['git', 'config', 'branch.master.gerritissue'],), ''), + ((['git', 'config', 'rietveld.autoupdate'],), ''), + ((['git', 'config', 'gerrit.host'],), 'True'), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), ((['get_or_create_merge_base', 'master', 'master'],), 'fake_ancestor_sha'), - ((['git', 'config', 'gerrit.host'],), 'True'), - ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [ + ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [ ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'rev-parse', 'HEAD'],), '12345'), ((['git', 'diff', '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), 'M\t.gitignore\n'), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), - ((['git', - 'config', 'branch.master.rietveldpatchset'],), ''), + ((['git', 'config', 'branch.master.gerritpatchset'],), ''), ((['git', 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],), 'foo'), @@ -580,7 +575,7 @@ class TestGitCl(TestCase): 'diff', '--no-ext-diff', '--stat', '--find-copies-harder', '-l100000', '-C50', 'fake_ancestor_sha', 'HEAD'],), '+dat'), - ] + ] @classmethod def _gerrit_upload_calls(cls, description, reviewers, squash, @@ -662,7 +657,12 @@ class TestGitCl(TestCase): ] if squash: calls += [ - ((['git', 'config', 'branch.master.rietveldissue', '123456'],), ''), + ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), + ((['git', 'config', 'branch.master.gerritserver'],), ''), + ((['git', 'config', 'remote.origin.url'],), + 'https://chromium.googlesource.com/my/repo.git'), + ((['git', 'config', 'branch.master.gerritserver', + 'https://chromium-review.googlesource.com'],), ''), ((['git', 'rev-parse', 'HEAD'],), 'abcdef0123456789'), ((['git', 'update-ref', '-m', 'Uploaded abcdef0123456789', 'refs/heads/git_cl_uploads/master', 'abcdef0123456789'],), @@ -886,9 +886,12 @@ class TestGitCl(TestCase): self.assertNotEqual(git_cl.main(['diff']), 0) def _patch_common(self): - self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda x: '60001') - self.mock(git_cl.Changelist, 'GetPatchSetDiff', lambda *args: None) - self.mock(git_cl.Changelist, 'GetDescription', lambda *args: 'Description') + self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset', + lambda x: '60001') + self.mock(git_cl._RietveldChangelistImpl, 'GetPatchSetDiff', + lambda *args: None) + self.mock(git_cl.Changelist, 'GetDescription', + lambda *args: 'Description') self.mock(git_cl.Changelist, 'SetIssue', lambda *args: None) self.mock(git_cl.Changelist, 'SetPatchset', lambda *args: None) self.mock(git_cl, 'IsGitVersionAtLeast', lambda *args: True) @@ -908,6 +911,8 @@ class TestGitCl(TestCase): 'Description\n\n' + 'patch from issue 123456 at patchset 60001 ' + '(http://crrev.com/123456#ps60001)'],), ''), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.rietveldserver'],), ''), ] self.assertEqual(git_cl.main(['patch', '123456']), 0)