diff --git a/git_cl.py b/git_cl.py index 42af2a77f2..8a5dc73ec8 100755 --- a/git_cl.py +++ b/git_cl.py @@ -114,20 +114,19 @@ def RunGit(args, **kwargs): def RunGitWithCode(args, suppress_stderr=False): """Returns return code and stdout.""" + if suppress_stderr: + stderr = subprocess2.VOID + else: + stderr = sys.stderr try: - if suppress_stderr: - stderr = subprocess2.VOID - else: - stderr = sys.stderr - out, code = subprocess2.communicate(['git'] + args, - env=GetNoGitPagerEnv(), - stdout=subprocess2.PIPE, - stderr=stderr) - return code, out[0] - except ValueError: - # When the subprocess fails, it returns None. That triggers a ValueError - # when trying to unpack the return value into (out, code). - return 1, '' + (out, _), code = subprocess2.communicate(['git'] + args, + env=GetNoGitPagerEnv(), + stdout=subprocess2.PIPE, + stderr=stderr) + return code, out + except subprocess2.CalledProcessError as e: + logging.debug('Failed running %s', args) + return e.returncode, e.stdout def RunGitSilent(args): @@ -157,33 +156,72 @@ def ask_for_data(prompt): sys.exit(1) -def git_set_branch_value(key, value): - branch = GetCurrentBranch() - if not branch: - return +def _git_branch_config_key(branch, key): + """Helper method to return Git config key for a branch.""" + assert branch, 'branch name is required to set git config for it' + return 'branch.%s.%s' % (branch, key) - cmd = ['config'] - if isinstance(value, int): - cmd.append('--int') - git_key = 'branch.%s.%s' % (branch, key) - RunGit(cmd + [git_key, str(value)]) +def _git_get_branch_config_value(key, default=None, value_type=str, + branch=False): + """Returns git config value of given or current branch if any. -def git_get_branch_default(key, default): - branch = GetCurrentBranch() - if branch: - git_key = 'branch.%s.%s' % (branch, key) - (_, stdout) = RunGitWithCode(['config', '--int', '--get', git_key]) - try: - return int(stdout.strip()) - except ValueError: - pass + Returns default in all other cases. + """ + assert value_type in (int, str, bool) + if branch is False: # Distinguishing default arg value from None. + branch = GetCurrentBranch() + + if not branch: + return default + + args = ['config'] + if value_type == int: + args.append('--int') + elif value_type == bool: + args.append('--bool') + args.append(_git_branch_config_key(branch, key)) + code, out = RunGitWithCode(args) + if code == 0: + value = out.strip() + if value_type == int: + return int(value) + if value_type == bool: + return bool(value.lower() == 'true') + return value return default +def _git_set_branch_config_value(key, value, branch=None, **kwargs): + """Sets the value or unsets if it's None of a git branch config. + + Valid, though not necessarily existing, branch must be provided, + otherwise currently checked out branch is used. + """ + if not branch: + branch = GetCurrentBranch() + assert branch, 'a branch name OR currently checked out branch is required' + args = ['config'] + # Check for boolean first, becuase bool is int, but int is not bool. + if value is None: + args.append('--unset') + elif isinstance(value, bool): + args.append('--bool') + value = str(value).lower() + elif isinstance(value, int): + args.append('--int') + value = str(value) + else: + value = str(value) + args.append(_git_branch_config_key(branch, key)) + if value is not None: + args.append(value) + RunGit(args, **kwargs) + + def add_git_similarity(parser): parser.add_option( - '--similarity', metavar='SIM', type='int', action='store', + '--similarity', metavar='SIM', type=int, action='store', help='Sets the percentage that a pair of files need to match in order to' ' be considered copies (default 50)') parser.add_option( @@ -198,19 +236,20 @@ def add_git_similarity(parser): options, args = old_parser_args(args) if options.similarity is None: - options.similarity = git_get_branch_default('git-cl-similarity', 50) + options.similarity = _git_get_branch_config_value( + 'git-cl-similarity', default=50, value_type=int) else: print('Note: Saving similarity of %d%% in git config.' % options.similarity) - git_set_branch_value('git-cl-similarity', options.similarity) + _git_set_branch_config_value('git-cl-similarity', options.similarity) options.similarity = max(0, min(options.similarity, 100)) if options.find_copies is None: - options.find_copies = bool( - git_get_branch_default('git-find-copies', True)) + options.find_copies = _git_get_branch_config_value( + 'git-find-copies', default=True, value_type=bool) else: - git_set_branch_value('git-find-copies', int(options.find_copies)) + _git_set_branch_config_value('git-find-copies', bool(options.find_copies)) print('Using %d%% similarity for rename/copy detection. ' 'Override with --similarity.' % options.similarity) @@ -906,7 +945,7 @@ class Changelist(object): Notes: * Not safe for concurrent multi-{thread,process} use. * Caches values from current branch. Therefore, re-use after branch change - with care. + with great care. """ def __init__(self, branchref=None, issue=None, codereview=None, **kwargs): @@ -966,14 +1005,15 @@ class Changelist(object): assert not self.issue # Whether we find issue or not, we are doing the lookup. self.lookedup_issue = True - for codereview, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems(): - setting = cls.IssueSetting(self.GetBranch()) - issue = RunGit(['config', setting], error_ok=True).strip() - if issue: - self._codereview = codereview - self._codereview_impl = cls(self, **kwargs) - self.issue = int(issue) - return + if self.GetBranch(): + for codereview, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems(): + issue = _git_get_branch_config_value( + cls.IssueConfigKey(), value_type=int, branch=self.GetBranch()) + if issue: + self._codereview = codereview + 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( @@ -1025,16 +1065,31 @@ class Changelist(object): """Clears cached branch data of this object.""" self.branch = self.branchref = None + def _GitGetBranchConfigValue(self, key, default=None, **kwargs): + assert 'branch' not in kwargs, 'this CL branch is used automatically' + kwargs['branch'] = self.GetBranch() + return _git_get_branch_config_value(key, default, **kwargs) + + def _GitSetBranchConfigValue(self, key, value, **kwargs): + assert 'branch' not in kwargs, 'this CL branch is used automatically' + assert self.GetBranch(), ( + 'this CL must have an associated branch to %sset %s%s' % + ('un' if value is None else '', + key, + '' if value is None else ' to %r' % value)) + kwargs['branch'] = self.GetBranch() + return _git_set_branch_config_value(key, value, **kwargs) + @staticmethod def FetchUpstreamTuple(branch): """Returns a tuple containing remote and remote ref, e.g. 'origin', 'refs/heads/master' """ remote = '.' - upstream_branch = RunGit(['config', 'branch.%s.merge' % branch], - error_ok=True).strip() + upstream_branch = _git_get_branch_config_value('merge', branch=branch) + if upstream_branch: - remote = RunGit(['config', 'branch.%s.remote' % branch]).strip() + remote = _git_get_branch_config_value('remote', branch=branch) else: upstream_branch = RunGit(['config', 'rietveld.upstream-branch'], error_ok=True).strip() @@ -1168,8 +1223,7 @@ class Changelist(object): Returns None if it is not set. """ - return RunGit(['config', 'branch.%s.base-url' % self.GetBranch()], - error_ok=True).strip() + return self._GitGetBranchConfigValue('base-url') def GetGitSvnRemoteUrl(self): """Return the configured git-svn remote URL parsed from git svn info. @@ -1202,10 +1256,8 @@ class Changelist(object): 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._codereview_impl.IssueSetting(self.GetBranch())], - error_ok=True).strip() - self.issue = int(issue) or None if issue else None + self.issue = self._GitGetBranchConfigValue( + self._codereview_impl.IssueConfigKey(), value_type=int) self.lookedup_issue = True return self.issue @@ -1230,41 +1282,44 @@ class Changelist(object): 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._codereview_impl.PatchsetSetting()], - error_ok=True).strip() - self.patchset = int(patchset) or None if patchset else None + self.patchset = self._GitGetBranchConfigValue( + self._codereview_impl.PatchsetConfigKey(), value_type=int) self.lookedup_patchset = True return self.patchset 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', patchset_setting, str(patchset)]) - self.patchset = patchset - else: - RunGit(['config', '--unset', patchset_setting], - stderr=subprocess2.PIPE, error_ok=True) + """Set this branch's patchset. If patchset=0, clears the patchset.""" + assert self.GetBranch() + if not patchset: self.patchset = None + else: + self.patchset = int(patchset) + self._GitSetBranchConfigValue( + self._codereview_impl.PatchsetConfigKey(), self.patchset) 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() + """Set this branch's issue. If issue isn't given, clears the issue.""" + assert self.GetBranch() if issue: + issue = int(issue) + self._GitSetBranchConfigValue( + self._codereview_impl.IssueConfigKey(), issue) self.issue = issue - RunGit(['config', issue_setting, str(issue)]) codereview_server = self._codereview_impl.GetCodereviewServer() if codereview_server: - RunGit(['config', codereview_setting, codereview_server]) + self._GitSetBranchConfigValue( + self._codereview_impl.CodereviewServerConfigKey(), + codereview_server) else: - # Reset it regardless. It doesn't hurt. - config_settings = [issue_setting, self._codereview_impl.PatchsetSetting()] - for prop in (['last-upload-hash'] + - self._codereview_impl._PostUnsetIssueProperties()): - config_settings.append('branch.%s.%s' % (self.GetBranch(), prop)) - for setting in config_settings: - RunGit(['config', '--unset', setting], error_ok=True) + # Reset all of these just to be clean. + reset_suffixes = [ + 'last-upload-hash', + self._codereview_impl.IssueConfigKey(), + self._codereview_impl.PatchsetConfigKey(), + self._codereview_impl.CodereviewServerConfigKey(), + ] + self._PostUnsetIssueProperties() + for prop in reset_suffixes: + self._GitSetBranchConfigValue(prop, None, error_ok=True) self.issue = None self.patchset = None @@ -1408,8 +1463,8 @@ class Changelist(object): elif options.cq_dry_run: self.SetCQState(_CQState.DRY_RUN) - git_set_branch_value('last-upload-hash', - RunGit(['rev-parse', 'HEAD']).strip()) + _git_set_branch_config_value('last-upload-hash', + RunGit(['rev-parse', 'HEAD']).strip()) # Run post upload hooks, if specified. if settings.GetRunPostUploadHook(): presubmit_support.DoPostUploadExecuter( @@ -1496,27 +1551,24 @@ class _ChangelistCodereviewBase(object): """Fetches and returns description from the codereview server.""" raise NotImplementedError() - def GetCodereviewServerSetting(self): - """Returns git config setting for the codereview server.""" - raise NotImplementedError() - @classmethod - def IssueSetting(cls, branch): - return 'branch.%s.%s' % (branch, cls.IssueSettingSuffix()) + def IssueConfigKey(cls): + """Returns branch setting storing issue number.""" + raise NotImplementedError() @classmethod - def IssueSettingSuffix(cls): - """Returns name of git config setting which stores issue number for a given - branch.""" + def PatchsetConfigKey(cls): + """Returns branch setting storing patchset number.""" raise NotImplementedError() - def PatchsetSetting(self): - """Returns name of git config setting which stores issue number.""" + @classmethod + def CodereviewServerConfigKey(cls): + """Returns branch setting storing codereview server.""" raise NotImplementedError() def _PostUnsetIssueProperties(self): """Which branch-specific properties to erase when unsettin issue.""" - raise NotImplementedError() + return [] def GetRieveldObjForPresubmit(self): # This is an unfortunate Rietveld-embeddedness in presubmit. @@ -1603,10 +1655,8 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): # 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()) + self._rietveld_server = gclient_utils.UpgradeToHttps( + self._GitGetBranchConfigValue(self.CodereviewServerConfigKey())) if not self._rietveld_server: self._rietveld_server = settings.GetDefaultServerUrl() return self._rietveld_server @@ -1760,23 +1810,16 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): return self._rpc_server @classmethod - def IssueSettingSuffix(cls): + def IssueConfigKey(cls): return 'rietveldissue' - def PatchsetSetting(self): - """Return the git setting that stores this change's most recent patchset.""" - return 'branch.%s.rietveldpatchset' % self.GetBranch() - - 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 + @classmethod + def PatchsetConfigKey(cls): + return 'rietveldpatchset' - def _PostUnsetIssueProperties(self): - """Which branch-specific properties to erase when unsetting issue.""" - return ['rietveldserver'] + @classmethod + def CodereviewServerConfigKey(cls): + return 'rietveldserver' def GetRieveldObjForPresubmit(self): return self.RpcServer() @@ -2070,12 +2113,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # 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 + self._gerrit_server = self._GitGetBranchConfigValue( + self.CodereviewServerConfigKey()) + 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. @@ -2086,9 +2127,17 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): return self._gerrit_server @classmethod - def IssueSettingSuffix(cls): + def IssueConfigKey(cls): return 'gerritissue' + @classmethod + def PatchsetConfigKey(cls): + return 'gerritpatchset' + + @classmethod + def CodereviewServerConfigKey(cls): + return 'gerritserver' + def EnsureAuthenticated(self, force): """Best effort check that user is authenticated with Gerrit server.""" if settings.GetGerritSkipEnsureAuthenticated(): @@ -2134,24 +2183,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): cookie_auth.get_netrc_path(), cookie_auth.get_new_password_message(git_host))) - - 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 _PostUnsetIssueProperties(self): """Which branch-specific properties to erase when unsetting issue.""" - return [ - 'gerritserver', - 'gerritsquashhash', - ] + return ['gerritsquashhash'] def GetRieveldObjForPresubmit(self): class ThisIsNotRietveldIssue(object): @@ -2274,8 +2308,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'Press Enter to continue, Ctrl+C to abort.') differs = True - last_upload = RunGit(['config', - 'branch.%s.gerritsquashhash' % self.GetBranch()], + last_upload = RunGit(['config', self._GitBranchSetting('gerritsquashhash')], error_ok=True).strip() # Note: git diff outputs nothing if there is no diff. if not last_upload or RunGit(['diff', last_upload]).strip(): @@ -2578,8 +2611,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): ('Created|Updated %d issues on Gerrit, but only 1 expected.\n' 'Change-Id: %s') % (len(change_numbers), change_id)) self.SetIssue(change_numbers[0]) - RunGit(['config', 'branch.%s.gerritsquashhash' % self.GetBranch(), - ref_to_push]) + self._GitSetBranchConfigValue('gerritsquashhash', ref_to_push) return 0 def _AddChangeIdToCommitMessage(self, options, args): @@ -5079,7 +5111,7 @@ def CMDcheckout(parser, args): branches = [] for cls in _CODEREVIEW_IMPLEMENTATIONS.values(): - branches.extend(find_issues(cls.IssueSettingSuffix())) + branches.extend(find_issues(cls.IssueConfigKey())) if len(branches) == 0: print('No branch found for issue %s.' % target_issue) return 1 diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index afcaf6d377..dc689fdcc7 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -21,6 +21,13 @@ import git_common import git_footers import subprocess2 +def callError(code=1, cmd='', cwd='', stdout='', stderr=''): + return subprocess2.CalledProcessError(code, cmd, cwd, stdout, stderr) + + +CERR1 = callError(1) + + class ChangelistMock(object): # A class variable so we can access it when we don't have access to the # instance that's being set. @@ -34,6 +41,7 @@ class ChangelistMock(object): def UpdateDescription(self, desc): ChangelistMock.desc = desc + class PresubmitMock(object): def __init__(self, *args, **kwargs): self.reviewers = [] @@ -233,7 +241,8 @@ class TestGitCl(TestCase): self.mock(subprocess2, 'call', self._mocked_call) self.mock(subprocess2, 'check_call', self._mocked_call) self.mock(subprocess2, 'check_output', self._mocked_call) - self.mock(subprocess2, 'communicate', self._mocked_call) + self.mock(subprocess2, 'communicate', + lambda *a, **kw: ([self._mocked_call(*a, **kw), ''], 0)) self.mock(git_cl.gclient_utils, 'CheckCallAndFilter', self._mocked_call) self.mock(git_common, 'is_dirty_git_tree', lambda x: False) self.mock(git_common, 'get_or_create_merge_base', @@ -269,8 +278,6 @@ class TestGitCl(TestCase): self.calls, '@%d Expected: Actual: %r' % (len(self._calls_done), args)) top = self.calls.pop(0) - if len(top) > 2 and top[2]: - raise top[2] expected_args, result = top # Also logs otherwise it could get caught in a try/finally and be hard to @@ -298,6 +305,8 @@ class TestGitCl(TestCase): len(self._calls_done), expected_args, args)) self._calls_done.append(top) + if isinstance(result, Exception): + raise result return result @classmethod @@ -319,19 +328,19 @@ class TestGitCl(TestCase): def _git_base_calls(cls, similarity, find_copies): if similarity is None: similarity = '50' - similarity_call = ((['git', 'config', '--int', '--get', - 'branch.master.git-cl-similarity'],), '') + similarity_call = ((['git', 'config', '--int', + 'branch.master.git-cl-similarity'],), CERR1) else: similarity_call = ((['git', 'config', '--int', - 'branch.master.git-cl-similarity', similarity],), '') + 'branch.master.git-cl-similarity', similarity],), '') if find_copies is None: find_copies = True - find_copies_call = ((['git', 'config', '--int', '--get', - 'branch.master.git-find-copies'],), '') + find_copies_call = ((['git', 'config', '--bool', + 'branch.master.git-find-copies'],), CERR1) else: - val = str(int(find_copies)) - find_copies_call = ((['git', 'config', '--int', + val = str(find_copies).lower() + find_copies_call = ((['git', 'config', '--bool', 'branch.master.git-find-copies', val],), '') if find_copies: @@ -349,8 +358,8 @@ class TestGitCl(TestCase): find_copies_call, ] + cls._is_gerrit_calls() + [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), - ((['git', 'config', 'branch.master.gerritissue'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.master.gerritissue'],), CERR1), ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'config', 'branch.master.merge'],), 'master'), @@ -363,8 +372,7 @@ class TestGitCl(TestCase): ((['git', 'diff', '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), 'M\t.gitignore\n'), - ((['git', 'config', 'branch.master.rietveldpatchset'],), - ''), + ((['git', 'config', '--int', 'branch.master.rietveldpatchset'],), CERR1), ((['git', 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],), 'foo'), @@ -403,12 +411,11 @@ class TestGitCl(TestCase): ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'svn', 'info'],), ''), ((['git', 'config', 'rietveld.project'],), ''), - ((['git', - 'config', 'branch.master.rietveldissue', '1'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldissue', '1'],), ''), ((['git', 'config', 'branch.master.rietveldserver', 'https://codereview.example.com'],), ''), ((['git', - 'config', 'branch.master.rietveldpatchset', '2'],), ''), + 'config', '--int', 'branch.master.rietveldpatchset', '2'],), ''), ] + cls._git_post_upload_calls() @classmethod @@ -434,7 +441,7 @@ class TestGitCl(TestCase): 'rev-list', '^' + fake_ancestor, 'HEAD'],), fake_cl), # Mock a config miss (error code 1) ((['git', - 'config', 'gitcl.remotebranch'],), (('', None), 1)), + 'config', 'gitcl.remotebranch'],), CERR1), ] + ([ # Call to GetRemoteBranch() ((['git', @@ -461,14 +468,14 @@ class TestGitCl(TestCase): None), 0)), ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), - ((['git', 'config', '--int', '--get', - 'branch.working.git-cl-similarity'],), ''), + ((['git', 'config', '--int', + 'branch.working.git-cl-similarity'],), CERR1), ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), - ((['git', 'config', '--int', '--get', - 'branch.working.git-find-copies'],), ''), + ((['git', 'config', '--bool', + 'branch.working.git-find-copies'],), CERR1), ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), ((['git', - 'config', 'branch.working.rietveldissue'],), '12345'), + 'config', '--int', 'branch.working.rietveldissue'],), '12345'), ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', @@ -505,7 +512,7 @@ class TestGitCl(TestCase): '.'],), 'M\tPRESUBMIT.py'), ((['git', - 'config', 'branch.working.rietveldpatchset'],), '31137'), + 'config', '--int', 'branch.working.rietveldpatchset'],), '31137'), ((['git', 'config', 'branch.working.rietveldserver'],), 'codereview.example.com'), ((['git', 'config', 'user.email'],), 'author@example.com'), @@ -529,11 +536,10 @@ class TestGitCl(TestCase): (' PRESUBMIT.py | 2 +-\n' ' 1 files changed, 1 insertions(+), 1 deletions(-)\n')), ((['git', 'show-ref', '--quiet', '--verify', - 'refs/heads/git-cl-commit'],), - (('', None), 0)), + 'refs/heads/git-cl-commit'],), ''), ((['git', 'branch', '-D', 'git-cl-commit'],), ''), ((['git', 'show-ref', '--quiet', '--verify', - 'refs/heads/git-cl-cherry-pick'],), ''), + 'refs/heads/git-cl-cherry-pick'],), CERR1), ((['git', 'rev-parse', '--show-cdup'],), '\n'), ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''), ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''), @@ -738,7 +744,7 @@ class TestGitCl(TestCase): if skip_auth_check: return [((cmd, ), 'true')] - calls = [((cmd, ), '', subprocess2.CalledProcessError(1, '', '', '', ''))] + calls = [((cmd, ), CERR1)] if issue: calls.extend([ ((['git', 'config', 'branch.master.gerritserver'],), ''), @@ -757,16 +763,16 @@ class TestGitCl(TestCase): def _gerrit_base_calls(cls, issue=None): return [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', '--int', '--get', - 'branch.master.git-cl-similarity'],), ''), + ((['git', 'config', '--int', 'branch.master.git-cl-similarity'],), + CERR1), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', '--int', '--get', - 'branch.master.git-find-copies'],), ''), + ((['git', 'config', '--bool', 'branch.master.git-find-copies'],), + CERR1), ] + cls._is_gerrit_calls(True) + [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), - ((['git', 'config', 'branch.master.gerritissue'],), - '' if issue is None else str(issue)), + ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.master.gerritissue'],), + CERR1 if issue is None else str(issue)), ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), ((['get_or_create_merge_base', 'master', @@ -783,7 +789,7 @@ class TestGitCl(TestCase): 'diff', '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), 'M\t.gitignore\n'), - ((['git', 'config', 'branch.master.gerritpatchset'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritpatchset'],), CERR1), ] + ([] if issue else [ ((['git', 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],), @@ -902,9 +908,10 @@ class TestGitCl(TestCase): ] if squash: calls += [ - ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],), + ''), ((['git', 'config', 'branch.master.gerritserver', - 'https://chromium-review.googlesource.com'],), ''), + 'https://chromium-review.googlesource.com'],), ''), ((['git', 'config', 'branch.master.gerritsquashhash', 'abcdef0123456789'],), ''), ] @@ -1265,9 +1272,9 @@ class TestGitCl(TestCase): # These calls detect codereview to use. self.calls += [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), - ((['git', 'config', 'branch.master.gerritissue'],), ''), - ((['git', 'config', 'rietveld.autoupdate'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.master.gerritissue'],), CERR1), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), ] if is_gerrit: @@ -1277,7 +1284,7 @@ class TestGitCl(TestCase): ] else: self.calls += [ - ((['git', 'config', 'gerrit.host'],), ''), + ((['git', 'config', 'gerrit.host'],), CERR1), ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'rev-parse', '--show-cdup'],), ''), ((['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'],), ''), @@ -1291,11 +1298,13 @@ class TestGitCl(TestCase): 'Description\n\n' + 'patch from issue 123456 at patchset 60001 ' + '(http://crrev.com/123456#ps60001)'],), ''), - ((['git', 'config', 'branch.master.rietveldissue', '123456'],), ''), - ((['git', 'config', 'branch.master.rietveldserver'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldissue', '123456'],), + ''), + ((['git', 'config', 'branch.master.rietveldserver'],), CERR1), ((['git', 'config', 'branch.master.rietveldserver', 'https://codereview.example.com'],), ''), - ((['git', 'config', 'branch.master.rietveldpatchset', '60001'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldpatchset', '60001'],), + ''), ] def test_patch_successful(self): @@ -1310,8 +1319,7 @@ class TestGitCl(TestCase): def test_patch_conflict(self): self._patch_common() self.calls += [ - ((['git', 'apply', '--index', '-p0', '--3way'],), '', - subprocess2.CalledProcessError(1, '', '', '', '')), + ((['git', 'apply', '--index', '-p0', '--3way'],), CERR1), ] self.assertNotEqual(git_cl.main(['patch', '123456']), 0) @@ -1321,7 +1329,8 @@ class TestGitCl(TestCase): ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', 'refs/changes/56/123456/7'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],), + ''), ((['git', 'config', 'branch.master.gerritserver'],), ''), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), @@ -1329,7 +1338,7 @@ class TestGitCl(TestCase): 'https://chromium.googlesource.com/my/repo'), ((['git', 'config', 'branch.master.gerritserver', 'https://chromium-review.googlesource.com'],), ''), - ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritpatchset', '7'],), ''), ] self.assertEqual(git_cl.main(['patch', '123456']), 0) @@ -1340,7 +1349,8 @@ class TestGitCl(TestCase): 'refs/changes/56/123456/7'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],), + ''), ((['git', 'config', 'branch.master.gerritserver'],), ''), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), @@ -1348,7 +1358,7 @@ class TestGitCl(TestCase): 'https://chromium.googlesource.com/my/repo'), ((['git', 'config', 'branch.master.gerritserver', 'https://chromium-review.googlesource.com'],), ''), - ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritpatchset', '7'],), ''), ] self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0) @@ -1358,10 +1368,11 @@ class TestGitCl(TestCase): ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', 'refs/changes/56/123456/1'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],), + ''), ((['git', 'config', 'branch.master.gerritserver', 'https://chromium-review.googlesource.com'],), ''), - ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''), + ((['git', 'config', '--int', 'branch.master.gerritpatchset', '1'],), ''), ] self.assertEqual(git_cl.main( ['patch', 'https://chromium-review.googlesource.com/#/c/123456/1']), 0) @@ -1375,10 +1386,9 @@ class TestGitCl(TestCase): self.calls += [ ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', 'refs/changes/56/123456/1'],), ''), - ((['git', 'cherry-pick', 'FETCH_HEAD'],), - '', subprocess2.CalledProcessError(1, '', '', '', '')), - ((['DieWithError', 'git cherry-pick FETCH_HEAD" failed.\n'],), - '', SystemExitMock()), + ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1), + ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],), + SystemExitMock()), ] with self.assertRaises(SystemExitMock): git_cl.main(['patch', @@ -1419,12 +1429,9 @@ class TestGitCl(TestCase): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self.calls = [ ((['git', 'config', '--local', '--get-regexp', - 'branch\\..*\\.rietveldissue'], ), '', - subprocess2.CalledProcessError(1, '', '', '', '')), + 'branch\\..*\\.rietveldissue'], ), CERR1), ((['git', 'config', '--local', '--get-regexp', - 'branch\\..*\\.gerritissue'], ), '', - subprocess2.CalledProcessError(1, '', '', '', '')), - + 'branch\\..*\\.gerritissue'], ), CERR1), ] self.assertEqual(1, git_cl.main(['checkout', '99999'])) @@ -1484,7 +1491,7 @@ class TestGitCl(TestCase): lambda _, v: self._mocked_call(['SetFlags', v])) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), + ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), '123'), ((['git', 'config', 'rietveld.autoupdate'],), ''), ((['git', 'config', 'rietveld.server'],), ''), ((['git', 'config', 'rietveld.server'],), ''), @@ -1500,8 +1507,8 @@ class TestGitCl(TestCase): ['SetReview', h, i, labels])) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.rietveldissue'],), ''), - ((['git', 'config', 'branch.feature.gerritissue'],), '123'), + ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'), ((['git', 'config', 'branch.feature.gerritserver'],), 'https://chromium-review.googlesource.com'), ((['SetReview', 'chromium-review.googlesource.com', 123, @@ -1656,9 +1663,9 @@ class TestGitCl(TestCase): self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.gerritissue'],), '123'), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.bug-prefix'],), ''), + ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), + ((['git', 'config', 'rietveld.bug-prefix'],), CERR1), ((['git', 'config', 'core.editor'],), 'vi'), ] self.assertEqual(0, git_cl.main(['description', '--gerrit'])) @@ -1679,15 +1686,12 @@ class TestGitCl(TestCase): self.calls = \ [((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],), 'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'), - ((['git', 'config', 'branch.master.rietveldissue'],), '1'), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'branch.foo.rietveldissue'],), '456'), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'branch.bar.rietveldissue'],), ''), - ((['git', 'config', 'branch.bar.gerritissue'],), '789'), + ((['git', 'config', '--int', 'branch.master.rietveldissue'],), '1'), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'config', '--int', 'branch.foo.rietveldissue'],), '456'), + ((['git', 'config', '--int', 'branch.bar.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.bar.gerritissue'],), '789'), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'tag', 'git-cl-archived-456-foo', 'foo'],), ''), ((['git', 'branch', '-D', 'foo'],), '')] @@ -1714,10 +1718,9 @@ class TestGitCl(TestCase): self.calls = \ [((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],), 'refs/heads/master'), - ((['git', 'config', 'branch.master.rietveldissue'],), '1'), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), - ((['git', 'config', 'rietveld.server'],), ''), + ((['git', 'config', '--int', 'branch.master.rietveldissue'],), '1'), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), ((['git', 'symbolic-ref', 'HEAD'],), 'master')] class MockChangelist(): @@ -1740,13 +1743,13 @@ class TestGitCl(TestCase): self.mock(git_cl.sys, 'stdout', out) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.rietveldissue'],), ''), - ((['git', 'config', 'branch.feature.gerritissue'],), '123'), - ((['git', 'config', '--unset', 'branch.feature.gerritissue'],), ''), - ((['git', 'config', '--unset', 'branch.feature.gerritpatchset'],), ''), + ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), CERR1), + ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'), # Let this command raise exception (retcode=1) - it should be ignored. ((['git', 'config', '--unset', 'branch.feature.last-upload-hash'],), - '', subprocess2.CalledProcessError(1, '', '', '', '')), + CERR1), + ((['git', 'config', '--unset', 'branch.feature.gerritissue'],), ''), + ((['git', 'config', '--unset', 'branch.feature.gerritpatchset'],), ''), ((['git', 'config', '--unset', 'branch.feature.gerritserver'],), ''), ((['git', 'config', '--unset', 'branch.feature.gerritsquashhash'],), ''), @@ -1767,7 +1770,7 @@ class TestGitCl(TestCase): lambda _, s: self._mocked_call(['SetCQState', s])) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), + ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), '123'), ((['git', 'config', 'rietveld.autoupdate'],), ''), ((['git', 'config', 'rietveld.server'],), 'https://codereview.chromium.org'),