diff --git a/gclient_scm.py b/gclient_scm.py index 4079acb32..d97c7e84a 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -230,6 +230,18 @@ 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 3f8a571fb..84eaa9726 100755 --- a/git_cl.py +++ b/git_cl.py @@ -9,6 +9,7 @@ from __future__ import print_function +from distutils.version import LooseVersion import base64 import collections import datetime @@ -115,12 +116,6 @@ 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 @@ -206,6 +201,20 @@ 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. @@ -249,6 +258,12 @@ 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) @@ -260,6 +275,46 @@ 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 = { @@ -731,7 +786,7 @@ class Settings(object): @staticmethod def GetRelativeRoot(): - return scm.GIT.GetCheckoutRoot(None) + return RunGit(['rev-parse', '--show-cdup']).strip() def GetRoot(self): if self.root is None: @@ -976,6 +1031,12 @@ 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) @@ -995,6 +1056,10 @@ 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) @@ -1026,7 +1091,7 @@ class Changelist(object): def GetCommonAncestorWithUpstream(self): upstream_branch = self.GetUpstreamBranch() - if not scm.GIT.IsValidRevision(settings.GetRoot(), upstream_branch): + if not BranchExists(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(), @@ -1073,6 +1138,55 @@ 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/'. @@ -1123,7 +1237,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(ISSUE_CONFIG_KEY) + self.issue = self._GitGetBranchConfigValue(self.IssueConfigKey()) if self.issue is not None: self.issue = int(self.issue) self.lookedup_issue = True @@ -1159,7 +1273,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(PATCHSET_CONFIG_KEY) + self.patchset = self._GitGetBranchConfigValue(self.PatchsetConfigKey()) if self.patchset is not None: self.patchset = int(self.patchset) self.lookedup_patchset = True @@ -1175,28 +1289,30 @@ class Changelist(object): self.patchset = None else: self.patchset = int(patchset) - self._GitSetBranchConfigValue(PATCHSET_CONFIG_KEY, str(self.patchset)) + self._GitSetBranchConfigValue( + self.PatchsetConfigKey(), 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(ISSUE_CONFIG_KEY, str(issue)) + self._GitSetBranchConfigValue( + self.IssueConfigKey(), str(issue)) self.issue = issue codereview_server = self.GetCodereviewServer() if codereview_server: self._GitSetBranchConfigValue( - CODEREVIEW_SERVER_CONFIG_KEY, codereview_server) + self.CodereviewServerConfigKey(), + codereview_server) else: # Reset all of these just to be clean. reset_suffixes = [ 'last-upload-hash', - ISSUE_CONFIG_KEY, - PATCHSET_CONFIG_KEY, - CODEREVIEW_SERVER_CONFIG_KEY, - 'gerritsquashhash', - ] + self.IssueConfigKey(), + self.PatchsetConfigKey(), + self.CodereviewServerConfigKey(), + ] + self._PostUnsetIssueProperties() for prop in reset_suffixes: try: self._GitSetBranchConfigValue(prop, None) @@ -1361,7 +1477,8 @@ 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) @@ -1409,7 +1526,7 @@ class Changelist(object): options, git_diff_args, custom_cl_base, change_desc) if not ret: self._GitSetBranchConfigValue( - 'last-upload-hash', scm.GIT.ResolveCommit(settings.GetRoot(), 'HEAD')) + 'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip()) # Run post upload hooks, if specified. if settings.GetRunPostUploadHook(): self.RunPostUploadHook( @@ -1488,7 +1605,7 @@ class Changelist(object): # with that branch. if self.GetIssue() and self.GetBranch(): self._gerrit_server = self._GitGetBranchConfigValue( - CODEREVIEW_SERVER_CONFIG_KEY) + self.CodereviewServerConfigKey()) if self._gerrit_server: self._gerrit_host = urllib.parse.urlparse(self._gerrit_server).netloc if not self._gerrit_server: @@ -1531,6 +1648,18 @@ 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(): @@ -1628,6 +1757,13 @@ 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. @@ -1963,7 +2099,7 @@ class Changelist(object): if self.GetBranch(): self.SetIssue(parsed_issue_arg.issue) self.SetPatchset(patchset) - fetched_hash = scm.GIT.ResolveCommit(settings.GetRoot(), 'FETCH_HEAD') + fetched_hash = RunGit(['rev-parse', 'FETCH_HEAD']).strip() self._GitSetBranchConfigValue('last-upload-hash', fetched_hash) self._GitSetBranchConfigValue('gerritsquashhash', fetched_hash) else: @@ -2353,7 +2489,9 @@ 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 = self._GitGetBranchConfigValue('gerritsquashhash', '') + parent = RunGit(['config', + 'branch.%s.gerritsquashhash' % upstream_branch_name], + error_ok=True).strip() # 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 + ':']) != @@ -2704,6 +2842,76 @@ 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. @@ -2785,6 +2993,11 @@ 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. @@ -3118,7 +3331,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 = scm.GIT.GetBranchRef(settings.GetRoot()) + branchref = RunGit(['symbolic-ref', 'HEAD']).strip() branch = scm.GIT.ShortBranchName(branchref) _, args = parser.parse_args(args) if not args: @@ -3590,8 +3803,9 @@ def CMDissue(parser, args): git_config[name] = val for branch in branches: - issue = git_config.get( - 'branch.%s.%s' % (scm.GIT.ShortBranchName(branch), ISSUE_CONFIG_KEY)) + config_key = _git_branch_config_key(scm.GIT.ShortBranchName(branch), + Changelist.IssueConfigKey()) + issue = git_config.get(config_key) if issue: issue_branch_map.setdefault(int(issue), []).append(branch) if not args: @@ -4635,8 +4849,9 @@ 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: @@ -4857,7 +5072,8 @@ 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 = settings.GetRoot() + top_dir = os.path.normpath( + RunGit(["rev-parse", "--show-toplevel"]).rstrip('\n')) return_value = _RunClangFormatDiff(opts, clang_diff_files, top_dir, upstream_commit) @@ -5019,14 +5235,15 @@ def CMDcheckout(parser, args): target_issue = str(issue_arg.issue) + issueprefix = Changelist.IssueConfigKey() output = RunGit(['config', '--local', '--get-regexp', - r'branch\..*\.' + ISSUE_CONFIG_KEY], + r'branch\..*\.%s' % issueprefix], 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\.(.*)\.' + ISSUE_CONFIG_KEY, r'\1', key)) + branches.append(re.sub(r'branch\.(.*)\.%s' % issueprefix, 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 aa65dacf1..eb413a235 100644 --- a/scm.py +++ b/scm.py @@ -4,7 +4,6 @@ """SCM-specific utility classes.""" -import distutils.version import glob import io import os @@ -114,8 +113,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) - output = output.decode('utf-8', 'replace') + ['git'] + args, cwd=cwd, stderr=subprocess2.PIPE, env=env, + **kwargs).decode('utf-8', 'replace') return output.strip() if strip_out else output @staticmethod @@ -407,7 +406,13 @@ 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'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) + 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) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 81cc23329..b6b3cdfe9 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -214,6 +214,9 @@ 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 3ab59ba4e..1cf288058 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -302,6 +302,187 @@ 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 = { @@ -497,6 +678,7 @@ 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', @@ -538,8 +720,6 @@ 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() @@ -957,6 +1137,9 @@ class TestGitCl(unittest.TestCase): notify), ''), ] + calls += [ + ((['git', 'rev-parse', 'HEAD'],), 'hash'), + ] return calls def _run_gerrit_upload_test( @@ -1026,6 +1209,7 @@ 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( @@ -1532,7 +1716,7 @@ class TestGitCl(unittest.TestCase): scm.GIT.GetBranchConfig('', branch, 'gerritserver')) def _patch_common(self, git_short_host='chromium'): - mock.patch('scm.GIT.ResolveCommit', return_value='deadbeef').start() + mock.patch('git_cl.IsGitVersionAtLeast', return_value=True).start() self.mockGit.config['remote.origin.url'] = ( 'https://%s.googlesource.com/my/repo' % git_short_host) gerrit_util.GetChangeDetail.return_value = { @@ -1561,6 +1745,7 @@ 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() @@ -1571,6 +1756,7 @@ 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') @@ -1581,6 +1767,7 @@ 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') @@ -1591,6 +1778,7 @@ 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) @@ -1602,6 +1790,7 @@ 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 5584d5911..41cc3b9ec 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -38,23 +38,6 @@ 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 = {