diff --git a/git_cl.py b/git_cl.py index 98a2f01b4..15f44a99d 100755 --- a/git_cl.py +++ b/git_cl.py @@ -894,27 +894,22 @@ class Changelist(object): assert codereview in ('rietveld', 'gerrit') return - # Automatic selection. + # Automatic selection based on issue number set for a current branch. + # Rietveld takes precedence over Gerrit. assert not self.issue - # Check if this branch is associated with Rietveld => Rieveld. - self._codereview_impl = _RietveldChangelistImpl(self, **kwargs) - if self.GetIssue(force_lookup=True): - return - - tmp_rietveld = self._codereview_impl # Save Rietveld object. - - # Check if this branch has Gerrit issue associated => Gerrit. - self._codereview_impl = _GerritChangelistImpl(self, **kwargs) - if self.GetIssue(force_lookup=True): - return - - # OK, no issue is set for this branch. - # If Gerrit is set repo-wide => Gerrit. - if settings.GetIsGerrit(): - return + # 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 - self._codereview_impl = tmp_rietveld - 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') def GetCCList(self): @@ -1131,10 +1126,11 @@ class Changelist(object): cwd=url).strip() return url - def GetIssue(self, force_lookup=False): + def GetIssue(self): """Returns the issue number as a int or None if not set.""" - if force_lookup or (self.issue is None and not self.lookedup_issue): - issue = RunGit(['config', self._codereview_impl.IssueSetting()], + if self.issue is None and not self.lookedup_issue: + 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 @@ -1180,7 +1176,7 @@ class Changelist(object): def SetIssue(self, issue=None): """Set this branch's issue. If issue isn't given, clears the issue.""" - issue_setting = self._codereview_impl.IssueSetting() + issue_setting = self._codereview_impl.IssueSetting(self.GetBranch()) codereview_setting = self._codereview_impl.GetCodereviewServerSetting() if issue: self.issue = issue @@ -1312,8 +1308,10 @@ class _ChangelistCodereviewBase(object): """Returns git config setting for the codereview server.""" raise NotImplementedError() - def IssueSetting(self): - """Returns name of git config setting which stores issue number.""" + @staticmethod + def IssueSetting(branch): + """Returns name of git config setting which stores issue number for a given + branch.""" raise NotImplementedError() def PatchsetSetting(self): @@ -1498,9 +1496,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): 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): """Return the git setting that stores this change's most recent patchset.""" @@ -1550,9 +1548,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): self._gerrit_server = 'https://%s' % self._gerrit_host return self._gerrit_server - def IssueSetting(self): - """Return the git setting that stores this change's issue.""" - return 'branch.%s.gerritissue' % self.GetBranch() + @staticmethod + def IssueSetting(branch): + return 'branch.%s.gerritissue' % branch def PatchsetSetting(self): """Return the git setting that stores this change's most recent patchset.""" diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 5c116e6ad..e89b4cc1e 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -167,13 +167,13 @@ class TestGitCl(TestCase): similarity_call, ((['git', 'symbolic-ref', 'HEAD'],), 'master'), find_copies_call, - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), - 'codereview.example.com'), ((['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'],), @@ -286,11 +286,11 @@ class TestGitCl(TestCase): ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), ((['git', 'config', '--int', '--get', 'branch.working.git-find-copies'],), ''), - ((['git', - 'config', 'rietveld.server'],), 'codereview.example.com'), ((['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'), @@ -550,18 +550,16 @@ class TestGitCl(TestCase): ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', '--int', '--get', 'branch.master.git-find-copies'],), ''), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), ((['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'), - ] + 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', @@ -577,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,