From e3a49aa40576da2427447f7fedb670d5d59216e5 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Tue, 24 Mar 2020 23:46:28 +0000 Subject: [PATCH] git-cl: Remove unused and duplicate functions. Remove unused functions, and use scm.GIT where possible to reduce duplication. Change-Id: I24f05e7b3ae04743e97b6665ee668f44d6acc294 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2116938 Reviewed-by: Anthony Polito Commit-Queue: Edward Lesmes --- gclient_scm.py | 12 -- git_cl.py | 277 +++++--------------------------------- scm.py | 19 +-- tests/gclient_scm_test.py | 3 - tests/git_cl_test.py | 195 +-------------------------- tests/scm_unittest.py | 17 +++ 6 files changed, 57 insertions(+), 466 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index d97c7e84a..4079acb32 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -230,18 +230,6 @@ class GitWrapper(SCMWrapper): filter_kwargs['predicate'] = self.out_cb self.filter = gclient_utils.GitFilter(**filter_kwargs) - @staticmethod - def BinaryExists(): - """Returns true if the command exists.""" - try: - # We assume git is newer than 1.7. See: crbug.com/114483 - result, version = scm.GIT.AssertVersion('1.7') - if not result: - raise gclient_utils.Error('Git version is older than 1.7: %s' % version) - return result - except OSError: - return False - def GetCheckoutRoot(self): return scm.GIT.GetCheckoutRoot(self.checkout_path) diff --git a/git_cl.py b/git_cl.py index 84eaa9726..3f8a571fb 100755 --- a/git_cl.py +++ b/git_cl.py @@ -9,7 +9,6 @@ from __future__ import print_function -from distutils.version import LooseVersion import base64 import collections import datetime @@ -116,6 +115,12 @@ DEFAULT_LINT_IGNORE_REGEX = r"$^" # File name for yapf style config files. YAPF_CONFIG_FILENAME = '.style.yapf' +# The issue, patchset and codereview server are stored on git config for each +# branch under branch... +ISSUE_CONFIG_KEY = 'gerritissue' +PATCHSET_CONFIG_KEY = 'gerritpatchset' +CODEREVIEW_SERVER_CONFIG_KEY = 'gerritserver' + # Shortcut since it quickly becomes repetitive. Fore = colorama.Fore @@ -201,20 +206,6 @@ def RunGitSilent(args): return RunGitWithCode(args, suppress_stderr=True)[1] -def IsGitVersionAtLeast(min_version): - prefix = 'git version ' - version = RunGit(['--version']).strip() - return (version.startswith(prefix) and - LooseVersion(version[len(prefix):]) >= LooseVersion(min_version)) - - -def BranchExists(branch): - """Return True if specified branch exists.""" - code, _ = RunGitWithCode(['rev-parse', '--verify', branch], - suppress_stderr=True) - return not code - - def time_sleep(seconds): # Use this so that it can be mocked in tests without interfering with python # system machinery. @@ -258,12 +249,6 @@ def ask_for_explicit_yes(prompt): result = gclient_utils.AskForData('Please, type yes or no: ').lower() -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) - - def _get_properties_from_options(options): prop_list = getattr(options, 'properties', []) properties = dict(x.split('=', 1) for x in prop_list) @@ -275,46 +260,6 @@ def _get_properties_from_options(options): return properties -# TODO(crbug.com/976104): Remove this function once git-cl try-results has -# migrated to use buildbucket v2 -def _buildbucket_retry(operation_name, http, *args, **kwargs): - """Retries requests to buildbucket service and returns parsed json content.""" - try_count = 0 - while True: - response, content = http.request(*args, **kwargs) - try: - content_json = json.loads(content) - except ValueError: - content_json = None - - # Buildbucket could return an error even if status==200. - if content_json and content_json.get('error'): - error = content_json.get('error') - if error.get('code') == 403: - raise BuildbucketResponseException( - 'Access denied: %s' % error.get('message', '')) - msg = 'Error in response. Reason: %s. Message: %s.' % ( - error.get('reason', ''), error.get('message', '')) - raise BuildbucketResponseException(msg) - - if response.status == 200: - if content_json is None: - raise BuildbucketResponseException( - 'Buildbucket returned invalid JSON content: %s.\n' - 'Please file bugs at http://crbug.com, ' - 'component "Infra>Platform>Buildbucket".' % - content) - return content_json - if response.status < 500 or try_count >= 2: - raise httplib2.HttpLib2Error(content) - - # status >= 500 means transient failures. - logging.debug('Transient errors when %s. Will retry.', operation_name) - time_sleep(0.5 + (1.5 * try_count)) - try_count += 1 - assert False, 'unreachable' - - def _call_buildbucket(http, buildbucket_host, method, request): """Calls a buildbucket v2 method and returns the parsed json response.""" headers = { @@ -786,7 +731,7 @@ class Settings(object): @staticmethod def GetRelativeRoot(): - return RunGit(['rev-parse', '--show-cdup']).strip() + return scm.GIT.GetCheckoutRoot(None) def GetRoot(self): if self.root is None: @@ -1031,12 +976,6 @@ class Changelist(object): self.cc = ','.join(filter(None, (base_cc, more_cc))) or '' return self.cc - def GetCCListWithoutDefault(self): - """Return the users cc'd on this CL excluding default ones.""" - if self.cc is None: - self.cc = ','.join(self.more_cc) - return self.cc - def ExtendCC(self, more_cc): """Extends the list of users to cc on this CL based on the changed files.""" self.more_cc.extend(more_cc) @@ -1056,10 +995,6 @@ class Changelist(object): self.GetBranch() # Poke the lazy loader. return self.branchref - def ClearBranch(self): - """Clears cached branch data of this object.""" - self.branch = self.branchref = None - def _GitGetBranchConfigValue(self, key, default=None): return scm.GIT.GetBranchConfig( settings.GetRoot(), self.GetBranch(), key, default) @@ -1091,7 +1026,7 @@ class Changelist(object): def GetCommonAncestorWithUpstream(self): upstream_branch = self.GetUpstreamBranch() - if not BranchExists(upstream_branch): + if not scm.GIT.IsValidRevision(settings.GetRoot(), upstream_branch): DieWithError('The upstream for the current branch (%s) does not exist ' 'anymore.\nPlease fix it and try again.' % self.GetBranch()) return git_common.get_or_create_merge_base(self.GetBranch(), @@ -1138,55 +1073,6 @@ class Changelist(object): self._remote = (remote, 'refs/remotes/%s/%s' % (remote, branch)) return self._remote - def GitSanityChecks(self, upstream_git_obj): - """Checks git repo status and ensures diff is from local commits.""" - - if upstream_git_obj is None: - if self.GetBranch() is None: - print('ERROR: Unable to determine current branch (detached HEAD?)', - file=sys.stderr) - else: - print('ERROR: No upstream branch.', file=sys.stderr) - return False - - # Verify the commit we're diffing against is in our current branch. - upstream_sha = RunGit(['rev-parse', '--verify', upstream_git_obj]).strip() - common_ancestor = RunGit(['merge-base', upstream_sha, 'HEAD']).strip() - if upstream_sha != common_ancestor: - print('ERROR: %s is not in the current branch. You may need to rebase ' - 'your tracking branch' % upstream_sha, file=sys.stderr) - return False - - # List the commits inside the diff, and verify they are all local. - commits_in_diff = RunGit( - ['rev-list', '^%s' % upstream_sha, 'HEAD']).splitlines() - code, remote_branch = RunGitWithCode(['config', 'gitcl.remotebranch']) - remote_branch = remote_branch.strip() - if code != 0: - _, remote_branch = self.GetRemoteBranch() - - commits_in_remote = RunGit( - ['rev-list', '^%s' % upstream_sha, remote_branch]).splitlines() - - common_commits = set(commits_in_diff) & set(commits_in_remote) - if common_commits: - print('ERROR: Your diff contains %d commits already in %s.\n' - 'Run "git log --oneline %s..HEAD" to get a list of commits in ' - 'the diff. If you are using a custom git flow, you can override' - ' the reference used for this check with "git config ' - 'gitcl.remotebranch ".' % ( - len(common_commits), remote_branch, upstream_git_obj), - file=sys.stderr) - return False - return True - - def GetGitBaseUrlFromConfig(self): - """Return the configured base URL from branch..baseurl. - - Returns None if it is not set. - """ - return self._GitGetBranchConfigValue('base-url') - def GetRemoteUrl(self): """Return the configured remote URL, e.g. 'git://example.org/foo.git/'. @@ -1237,7 +1123,7 @@ 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: - self.issue = self._GitGetBranchConfigValue(self.IssueConfigKey()) + self.issue = self._GitGetBranchConfigValue(ISSUE_CONFIG_KEY) if self.issue is not None: self.issue = int(self.issue) self.lookedup_issue = True @@ -1273,7 +1159,7 @@ 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: - self.patchset = self._GitGetBranchConfigValue(self.PatchsetConfigKey()) + self.patchset = self._GitGetBranchConfigValue(PATCHSET_CONFIG_KEY) if self.patchset is not None: self.patchset = int(self.patchset) self.lookedup_patchset = True @@ -1289,30 +1175,28 @@ class Changelist(object): self.patchset = None else: self.patchset = int(patchset) - self._GitSetBranchConfigValue( - self.PatchsetConfigKey(), str(self.patchset)) + self._GitSetBranchConfigValue(PATCHSET_CONFIG_KEY, str(self.patchset)) def SetIssue(self, issue=None): """Set this branch's issue. If issue isn't given, clears the issue.""" assert self.GetBranch() if issue: issue = int(issue) - self._GitSetBranchConfigValue( - self.IssueConfigKey(), str(issue)) + self._GitSetBranchConfigValue(ISSUE_CONFIG_KEY, str(issue)) self.issue = issue codereview_server = self.GetCodereviewServer() if codereview_server: self._GitSetBranchConfigValue( - self.CodereviewServerConfigKey(), - codereview_server) + CODEREVIEW_SERVER_CONFIG_KEY, codereview_server) else: # Reset all of these just to be clean. reset_suffixes = [ 'last-upload-hash', - self.IssueConfigKey(), - self.PatchsetConfigKey(), - self.CodereviewServerConfigKey(), - ] + self._PostUnsetIssueProperties() + ISSUE_CONFIG_KEY, + PATCHSET_CONFIG_KEY, + CODEREVIEW_SERVER_CONFIG_KEY, + 'gerritsquashhash', + ] for prop in reset_suffixes: try: self._GitSetBranchConfigValue(prop, None) @@ -1477,8 +1361,7 @@ class Changelist(object): return options.message.strip() # Use the subject of the last commit as title by default. - title = RunGit( - ['show', '-s', '--format=%s', 'HEAD']).strip() + title = RunGit(['show', '-s', '--format=%s', 'HEAD']).strip() if options.force: return title user_title = gclient_utils.AskForData('Title for patchset [%s]: ' % title) @@ -1526,7 +1409,7 @@ class Changelist(object): options, git_diff_args, custom_cl_base, change_desc) if not ret: self._GitSetBranchConfigValue( - 'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip()) + 'last-upload-hash', scm.GIT.ResolveCommit(settings.GetRoot(), 'HEAD')) # Run post upload hooks, if specified. if settings.GetRunPostUploadHook(): self.RunPostUploadHook( @@ -1605,7 +1488,7 @@ class Changelist(object): # with that branch. if self.GetIssue() and self.GetBranch(): self._gerrit_server = self._GitGetBranchConfigValue( - self.CodereviewServerConfigKey()) + CODEREVIEW_SERVER_CONFIG_KEY) if self._gerrit_server: self._gerrit_host = urllib.parse.urlparse(self._gerrit_server).netloc if not self._gerrit_server: @@ -1648,18 +1531,6 @@ class Changelist(object): # Fall back on still unique, but less efficient change number. return str(self.GetIssue()) - @classmethod - def IssueConfigKey(cls): - return 'gerritissue' - - @classmethod - def PatchsetConfigKey(cls): - return 'gerritpatchset' - - @classmethod - def CodereviewServerConfigKey(cls): - return 'gerritserver' - def EnsureAuthenticated(self, force, refresh=None): """Best effort check that user is authenticated with Gerrit server.""" if settings.GetGerritSkipEnsureAuthenticated(): @@ -1757,13 +1628,6 @@ class Changelist(object): (self.GetIssue(), self.GetIssueOwner(), details['email'])) confirm_or_exit(action='upload') - def _PostUnsetIssueProperties(self): - """Which branch-specific properties to erase when unsetting issue.""" - return ['gerritsquashhash'] - - def GetGerritObjForPresubmit(self): - return presubmit_support.GerritAccessor(self._GetGerritHost()) - def GetStatus(self): """Applies a rough heuristic to give a simple summary of an issue's review or CQ status, assuming adherence to a common workflow. @@ -2099,7 +1963,7 @@ class Changelist(object): if self.GetBranch(): self.SetIssue(parsed_issue_arg.issue) self.SetPatchset(patchset) - fetched_hash = RunGit(['rev-parse', 'FETCH_HEAD']).strip() + fetched_hash = scm.GIT.ResolveCommit(settings.GetRoot(), 'FETCH_HEAD') self._GitSetBranchConfigValue('last-upload-hash', fetched_hash) self._GitSetBranchConfigValue('gerritsquashhash', fetched_hash) else: @@ -2489,9 +2353,7 @@ class Changelist(object): # the tree hash of the parent branch. The upside is less likely bogus # requests to reupload parent change just because it's uploadhash is # missing, yet the downside likely exists, too (albeit unknown to me yet). - parent = RunGit(['config', - 'branch.%s.gerritsquashhash' % upstream_branch_name], - error_ok=True).strip() + parent = self._GitGetBranchConfigValue('gerritsquashhash', '') # Verify that the upstream branch has been uploaded too, otherwise # Gerrit will create additional CLs when uploading. if not parent or (RunGitSilent(['rev-parse', upstream_branch + ':']) != @@ -2842,76 +2704,6 @@ class ChangeDescription(object): """ return re.sub(cls.BAD_HASH_TAG_CHUNK, '-', tag).strip('-').lower() - def update_with_git_number_footers(self, parent_hash, parent_msg, dest_ref): - """Updates this commit description given the parent. - - This is essentially what Gnumbd used to do. - Consult https://goo.gl/WMmpDe for more details. - """ - assert parent_msg # No, orphan branch creation isn't supported. - assert parent_hash - assert dest_ref - parent_footer_map = git_footers.parse_footers(parent_msg) - # This will also happily parse svn-position, which GnumbD is no longer - # supporting. While we'd generate correct footers, the verifier plugin - # installed in Gerrit will block such commit (ie git push below will fail). - parent_position = git_footers.get_position(parent_footer_map) - - # Cherry-picks may have last line obscuring their prior footers, - # from git_footers perspective. This is also what Gnumbd did. - cp_line = None - if (self._description_lines and - re.match(self.CHERRY_PICK_LINE, self._description_lines[-1])): - cp_line = self._description_lines.pop() - - top_lines, footer_lines, _ = git_footers.split_footers(self.description) - - # Original-ify all Cr- footers, to avoid re-lands, cherry-picks, or just - # user interference with actual footers we'd insert below. - for i, line in enumerate(footer_lines): - k, v = git_footers.parse_footer(line) or (None, None) - if k and k.startswith('Cr-'): - footer_lines[i] = '%s: %s' % ('Cr-Original-' + k[len('Cr-'):], v) - - # Add Position and Lineage footers based on the parent. - lineage = list(reversed(parent_footer_map.get('Cr-Branched-From', []))) - if parent_position[0] == dest_ref: - # Same branch as parent. - number = int(parent_position[1]) + 1 - else: - number = 1 # New branch, and extra lineage. - lineage.insert(0, '%s-%s@{#%d}' % (parent_hash, parent_position[0], - int(parent_position[1]))) - - footer_lines.append('Cr-Commit-Position: %s@{#%d}' % (dest_ref, number)) - footer_lines.extend('Cr-Branched-From: %s' % v for v in lineage) - - self._description_lines = top_lines - if cp_line: - self._description_lines.append(cp_line) - if self._description_lines[-1] != '': - self._description_lines.append('') # Ensure footer separator. - self._description_lines.extend(footer_lines) - - -def get_approving_reviewers(props, disapproval=False): - """Retrieves the reviewers that approved a CL from the issue properties with - messages. - - Note that the list may contain reviewers that are not committer, thus are not - considered by the CQ. - - If disapproval is True, instead returns reviewers who 'not lgtm'd the CL. - """ - approval_type = 'disapproval' if disapproval else 'approval' - return sorted( - set( - message['sender'] - for message in props['messages'] - if message[approval_type] and message['sender'] in props['reviewers'] - ) - ) - def FindCodereviewSettingsFile(filename='codereview.settings'): """Finds the given file starting in the cwd and going up. @@ -2993,11 +2785,6 @@ def hasSheBang(fname): return f.read(2).startswith('#!') -# TODO(bpastene) Remove once a cleaner fix to crbug.com/600473 presents itself. -def DownloadHooks(*args, **kwargs): - pass - - def DownloadGerritHook(force): """Downloads and installs a Gerrit commit-msg hook. @@ -3331,7 +3118,7 @@ def CMDcreds_check(parser, args): @metrics.collector.collect_metrics('git cl baseurl') def CMDbaseurl(parser, args): """Gets or sets base-url for this branch.""" - branchref = RunGit(['symbolic-ref', 'HEAD']).strip() + branchref = scm.GIT.GetBranchRef(settings.GetRoot()) branch = scm.GIT.ShortBranchName(branchref) _, args = parser.parse_args(args) if not args: @@ -3803,9 +3590,8 @@ def CMDissue(parser, args): git_config[name] = val for branch in branches: - config_key = _git_branch_config_key(scm.GIT.ShortBranchName(branch), - Changelist.IssueConfigKey()) - issue = git_config.get(config_key) + issue = git_config.get( + 'branch.%s.%s' % (scm.GIT.ShortBranchName(branch), ISSUE_CONFIG_KEY)) if issue: issue_branch_map.setdefault(int(issue), []).append(branch) if not args: @@ -4849,9 +4635,8 @@ def CMDowners(parser, args): help='Show all owners for a particular file') options, args = parser.parse_args(args) - author = RunGit(['config', 'user.email']).strip() or None - cl = Changelist() + author = cl.GetAuthor() if options.show_all: for arg in args: @@ -5072,8 +4857,7 @@ def CMDformat(parser, args): dart_diff_files = [x for x in diff_files if MatchingFileType(x, ['.dart'])] gn_diff_files = [x for x in diff_files if MatchingFileType(x, GN_EXTS)] - top_dir = os.path.normpath( - RunGit(["rev-parse", "--show-toplevel"]).rstrip('\n')) + top_dir = settings.GetRoot() return_value = _RunClangFormatDiff(opts, clang_diff_files, top_dir, upstream_commit) @@ -5235,15 +5019,14 @@ def CMDcheckout(parser, args): target_issue = str(issue_arg.issue) - issueprefix = Changelist.IssueConfigKey() output = RunGit(['config', '--local', '--get-regexp', - r'branch\..*\.%s' % issueprefix], + r'branch\..*\.' + ISSUE_CONFIG_KEY], error_ok=True) branches = [] for key, issue in [x.split() for x in output.splitlines()]: if issue == target_issue: - branches.append(re.sub(r'branch\.(.*)\.%s' % issueprefix, r'\1', key)) + branches.append(re.sub(r'branch\.(.*)\.' + ISSUE_CONFIG_KEY, r'\1', key)) if len(branches) == 0: print('No branch found for issue %s.' % target_issue) diff --git a/scm.py b/scm.py index eb413a235..aa65dacf1 100644 --- a/scm.py +++ b/scm.py @@ -4,6 +4,7 @@ """SCM-specific utility classes.""" +import distutils.version import glob import io import os @@ -113,8 +114,8 @@ class GIT(object): def Capture(args, cwd, strip_out=True, **kwargs): env = GIT.ApplyEnvVars(kwargs) output = subprocess2.check_output( - ['git'] + args, cwd=cwd, stderr=subprocess2.PIPE, env=env, - **kwargs).decode('utf-8', 'replace') + ['git'] + args, cwd=cwd, stderr=subprocess2.PIPE, env=env, **kwargs) + output = output.decode('utf-8', 'replace') return output.strip() if strip_out else output @staticmethod @@ -406,13 +407,7 @@ class GIT(object): """Asserts git's version is at least min_version.""" if cls.current_version is None: current_version = cls.Capture(['--version'], '.') - matched = re.search(r'version ([0-9\.]+)', current_version) - cls.current_version = matched.group(1) - current_version_list = list(map(only_int, cls.current_version.split('.'))) - for min_ver in map(int, min_version.split('.')): - ver = current_version_list.pop(0) - if ver < min_ver: - return (False, cls.current_version) - elif ver > min_ver: - return (True, cls.current_version) - return (True, cls.current_version) + matched = re.search(r'git version (.+)', current_version) + cls.current_version = distutils.version.LooseVersion(matched.group(1)) + min_version = distutils.version.LooseVersion(min_version) + return (min_version <= cls.current_version, cls.current_version) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index b6b3cdfe9..81cc23329 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -214,9 +214,6 @@ from :3 self.relpath = '.' self.base_path = join(self.root_dir, self.relpath) self.enabled = self.CreateGitRepo(self.sample_git_import, self.base_path) - self._original_GitBinaryExists = gclient_scm.GitWrapper.BinaryExists - mock.patch('gclient_scm.GitWrapper.BinaryExists', - staticmethod(lambda : True)).start() mock.patch('sys.stdout', StringIO()).start() self.addCleanup(mock.patch.stopall) self.addCleanup(gclient_utils.rmtree, self.root_dir) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 1cf288058..3ab59ba4e 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -302,187 +302,6 @@ class TestGitClBasic(unittest.TestCase): self.assertEqual(f('v8', 'chromium:123,456,v8:123'), ['v8:456', 'chromium:123', 'v8:123']) - def _test_git_number(self, parent_msg, dest_ref, child_msg, - parent_hash='parenthash'): - desc = git_cl.ChangeDescription(child_msg) - desc.update_with_git_number_footers(parent_hash, parent_msg, dest_ref) - return desc.description - - def assertEqualByLine(self, actual, expected): - self.assertEqual(actual.splitlines(), expected.splitlines()) - - def test_git_number_bad_parent(self): - with self.assertRaises(ValueError): - self._test_git_number('Parent', 'refs/heads/master', 'Child') - - def test_git_number_bad_parent_footer(self): - with self.assertRaises(AssertionError): - self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: wrong', - 'refs/heads/master', 'Child') - - def test_git_number_bad_lineage_ignored(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#1}\n' - 'Cr-Branched-From: mustBeReal40CharHash-branch@{#pos}', - 'refs/heads/master', 'Child') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#2}\n' - 'Cr-Branched-From: mustBeReal40CharHash-branch@{#pos}') - - def test_git_number_same_branch(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#12}', - dest_ref='refs/heads/master', - child_msg='Child') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#13}') - - def test_git_number_same_branch_mixed_footers(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#12}', - dest_ref='refs/heads/master', - child_msg='Child\n' - '\n' - 'Broken-by: design\n' - 'BUG=123') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Broken-by: design\n' - 'BUG=123\n' - 'Cr-Commit-Position: refs/heads/master@{#13}') - - def test_git_number_same_branch_with_originals(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#12}', - dest_ref='refs/heads/master', - child_msg='Child\n' - '\n' - 'Some users are smart and insert their own footers\n' - '\n' - 'Cr-Whatever: value\n' - 'Cr-Commit-Position: refs/copy/paste@{#22}') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Some users are smart and insert their own footers\n' - '\n' - 'Cr-Original-Whatever: value\n' - 'Cr-Original-Commit-Position: refs/copy/paste@{#22}\n' - 'Cr-Commit-Position: refs/heads/master@{#13}') - - def test_git_number_new_branch(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#12}', - dest_ref='refs/heads/branch', - child_msg='Child') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Cr-Commit-Position: refs/heads/branch@{#1}\n' - 'Cr-Branched-From: parenthash-refs/heads/master@{#12}') - - def test_git_number_lineage(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/branch@{#1}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}', - dest_ref='refs/heads/branch', - child_msg='Child') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Cr-Commit-Position: refs/heads/branch@{#2}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}') - - def test_git_number_moooooooore_lineage(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/branch@{#5}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}', - dest_ref='refs/heads/mooore', - child_msg='Child') - self.assertEqualByLine( - actual, - 'Child\n' - '\n' - 'Cr-Commit-Position: refs/heads/mooore@{#1}\n' - 'Cr-Branched-From: parenthash-refs/heads/branch@{#5}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}') - - def test_git_number_ever_moooooooore_lineage(self): - self.maxDiff = 10000 # pylint: disable=attribute-defined-outside-init - actual = self._test_git_number( - 'CQ commit on fresh new branch + numbering.\n' - '\n' - 'NOTRY=True\n' - 'NOPRESUBMIT=True\n' - 'BUG=\n' - '\n' - 'Review-Url: https://codereview.chromium.org/2577703003\n' - 'Cr-Commit-Position: refs/heads/gnumb-test/br@{#1}\n' - 'Cr-Branched-From: 0749ff9edc-refs/heads/gnumb-test/cq@{#4}\n' - 'Cr-Branched-From: 5c49df2da6-refs/heads/master@{#41618}', - dest_ref='refs/heads/gnumb-test/cl', - child_msg='git cl on fresh new branch + numbering.\n' - '\n' - 'Review-Url: https://codereview.chromium.org/2575043003 .\n') - self.assertEqualByLine( - actual, - 'git cl on fresh new branch + numbering.\n' - '\n' - 'Review-Url: https://codereview.chromium.org/2575043003 .\n' - 'Cr-Commit-Position: refs/heads/gnumb-test/cl@{#1}\n' - 'Cr-Branched-From: parenthash-refs/heads/gnumb-test/br@{#1}\n' - 'Cr-Branched-From: 0749ff9edc-refs/heads/gnumb-test/cq@{#4}\n' - 'Cr-Branched-From: 5c49df2da6-refs/heads/master@{#41618}') - - def test_git_number_cherry_pick(self): - actual = self._test_git_number( - 'Parent\n' - '\n' - 'Cr-Commit-Position: refs/heads/branch@{#1}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}', - dest_ref='refs/heads/branch', - child_msg='Child, which is cherry-pick from master\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#100}\n' - '(cherry picked from commit deadbeef12345678deadbeef12345678deadbeef)') - self.assertEqualByLine( - actual, - 'Child, which is cherry-pick from master\n' - '\n' - '(cherry picked from commit deadbeef12345678deadbeef12345678deadbeef)\n' - '\n' - 'Cr-Original-Commit-Position: refs/heads/master@{#100}\n' - 'Cr-Commit-Position: refs/heads/branch@{#2}\n' - 'Cr-Branched-From: somehash-refs/heads/master@{#12}') - @mock.patch('gerrit_util.GetAccountDetails') def test_valid_accounts(self, mockGetAccountDetails): mock_per_account = { @@ -678,7 +497,6 @@ class TestGitCl(unittest.TestCase): mock.patch( 'git_common.get_or_create_merge_base', lambda *a: self._mocked_call('get_or_create_merge_base', *a)).start() - mock.patch('git_cl.BranchExists', return_value=True).start() mock.patch('git_cl.FindCodereviewSettingsFile', return_value='').start() mock.patch( 'git_cl.SaveDescriptionBackup', @@ -720,6 +538,8 @@ class TestGitCl(unittest.TestCase): self.mockGit = GitMocks() mock.patch('scm.GIT.GetBranchRef', self.mockGit.GetBranchRef).start() mock.patch('scm.GIT.GetConfig', self.mockGit.GetConfig).start() + mock.patch('scm.GIT.ResolveCommit', return_value='hash').start() + mock.patch('scm.GIT.IsValidRevision', return_value=True).start() mock.patch('scm.GIT.SetConfig', self.mockGit.SetConfig).start() mock.patch( 'git_new_branch.create_new_branch', self.mockGit.NewBranch).start() @@ -1137,9 +957,6 @@ class TestGitCl(unittest.TestCase): notify), ''), ] - calls += [ - ((['git', 'rev-parse', 'HEAD'],), 'hash'), - ] return calls def _run_gerrit_upload_test( @@ -1209,7 +1026,6 @@ class TestGitCl(unittest.TestCase): list(args))).start() mock.patch('os.path.isfile', lambda path: self._mocked_call(['os.path.isfile', path])).start() - mock.patch('git_cl.Changelist.GitSanityChecks', return_value=True).start() mock.patch( 'git_cl._create_description_from_log', return_value=description).start() mock.patch( @@ -1716,7 +1532,7 @@ class TestGitCl(unittest.TestCase): scm.GIT.GetBranchConfig('', branch, 'gerritserver')) def _patch_common(self, git_short_host='chromium'): - mock.patch('git_cl.IsGitVersionAtLeast', return_value=True).start() + mock.patch('scm.GIT.ResolveCommit', return_value='deadbeef').start() self.mockGit.config['remote.origin.url'] = ( 'https://%s.googlesource.com/my/repo' % git_short_host) gerrit_util.GetChangeDetail.return_value = { @@ -1745,7 +1561,6 @@ class TestGitCl(unittest.TestCase): ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', 'refs/changes/56/123456/7'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'), ] self.assertEqual(git_cl.main(['patch', '123456']), 0) self.assertIssueAndPatchset() @@ -1756,7 +1571,6 @@ class TestGitCl(unittest.TestCase): ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo', 'refs/changes/56/123456/7'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'), ] self.assertEqual(git_cl.main(['patch', '-b', 'feature', '123456']), 0) self.assertIssueAndPatchset(branch='feature') @@ -1767,7 +1581,6 @@ class TestGitCl(unittest.TestCase): ((['git', 'fetch', 'https://host.googlesource.com/my/repo', 'refs/changes/56/123456/7'],), ''), ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''), - ((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'), ] self.assertEqual(git_cl.main(['patch', '123456', '--force']), 0) self.assertIssueAndPatchset(git_short_host='host') @@ -1778,7 +1591,6 @@ class TestGitCl(unittest.TestCase): ((['git', 'fetch', 'https://else.googlesource.com/my/repo', 'refs/changes/56/123456/1'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'), ] self.assertEqual(git_cl.main( ['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0) @@ -1790,7 +1602,6 @@ class TestGitCl(unittest.TestCase): ((['git', 'fetch', 'https://else.googlesource.com/my/repo', 'refs/changes/56/123456/1'],), ''), ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''), - ((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'), ] self.assertEqual(git_cl.main( ['patch', 'https://else-review.googlesource.com/c/my/repo/+/123456/1']), diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index 41cc3b9ec..5584d5911 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -38,6 +38,23 @@ class GitWrapperTestCase(unittest.TestCase): self.assertEqual(scm.GIT.GetEmail(self.root_dir), 'mini@me.com') mockCapture.assert_called_with(['config', 'user.email'], cwd=self.root_dir) + @mock.patch('scm.GIT.Capture') + def testAssertVersion(self, mockCapture): + cases = [ + ('1.7', True), + ('1.7.9', True), + ('1.7.9.foo-bar-baz', True), + ('1.8', True), + ('1.6.9', False), + ] + for expected_version, expected_ok in cases: + class GIT(scm.GIT): + pass + mockCapture.return_value = 'git version ' + expected_version + ok, version = GIT.AssertVersion('1.7') + self.assertEqual(expected_ok, ok) + self.assertEqual(expected_version, version) + def testRefToRemoteRef(self): remote = 'origin' refs = {