From 6468b9068b721bd466d0e802c73fdc94dd9d949a Mon Sep 17 00:00:00 2001 From: skobes Date: Mon, 24 Oct 2016 08:45:10 -0700 Subject: [PATCH] Make git cl patch work with binary files. BUG=557512 Review-Url: https://codereview.chromium.org/2445543002 --- git_cl.py | 68 +++++++------------------------------------- tests/git_cl_test.py | 39 +++++++++++++++---------- 2 files changed, 35 insertions(+), 72 deletions(-) diff --git a/git_cl.py b/git_cl.py index 33517b96d..a1cc0fa42 100755 --- a/git_cl.py +++ b/git_cl.py @@ -40,6 +40,7 @@ from third_party import colorama from third_party import httplib2 from third_party import upload import auth +import checkout import clang_format import commit_queue import dart_format @@ -990,12 +991,6 @@ class _ParsedIssueNumberArgument(object): return self.issue is not None -class _RietveldParsedIssueNumberArgument(_ParsedIssueNumberArgument): - def __init__(self, *args, **kwargs): - self.patch_url = kwargs.pop('patch_url', None) - super(_RietveldParsedIssueNumberArgument, self).__init__(*args, **kwargs) - - def ParseIssueNumberArgument(arg): """Parses the issue argument and returns _ParsedIssueNumberArgument.""" fail_result = _ParsedIssueNumberArgument() @@ -1812,10 +1807,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): def GetMostRecentPatchset(self): return self.GetIssueProperties()['patchsets'][-1] - def GetPatchSetDiff(self, issue, patchset): - return self.RpcServer().get( - '/download/issue%s_%s.diff' % (issue, patchset)) - def GetIssueProperties(self): if self._props is None: issue = self.GetIssue() @@ -1972,8 +1963,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit, directory): - # TODO(maruel): Use apply_issue.py - # PatchIssue should never be called with a dirty tree. It is up to the # caller to check this, but just in case we assert here since the # consequences of the caller not checking this could be dire. @@ -1983,47 +1972,13 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): if parsed_issue_arg.hostname: self._rietveld_server = 'https://%s' % parsed_issue_arg.hostname - if (isinstance(parsed_issue_arg, _RietveldParsedIssueNumberArgument) and - parsed_issue_arg.patch_url): - assert parsed_issue_arg.patchset - patchset = parsed_issue_arg.patchset - patch_data = urllib2.urlopen(parsed_issue_arg.patch_url).read() - else: - patchset = parsed_issue_arg.patchset or self.GetMostRecentPatchset() - patch_data = self.GetPatchSetDiff(self.GetIssue(), patchset) - - # Switch up to the top-level directory, if necessary, in preparation for - # applying the patch. - top = settings.GetRelativeRoot() - if top: - os.chdir(top) - - # Git patches have a/ at the beginning of source paths. We strip that out - # with a sed script rather than the -p flag to patch so we can feed either - # Git or svn-style patches into the same apply command. - # re.sub() should be used but flags=re.MULTILINE is only in python 2.7. + patchset = parsed_issue_arg.patchset or self.GetMostRecentPatchset() + patchset_object = self.RpcServer().get_patch(self.GetIssue(), patchset) + scm_obj = checkout.GitCheckout(settings.GetRoot(), None, None, None, None) try: - patch_data = subprocess2.check_output( - ['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'], stdin=patch_data) - except subprocess2.CalledProcessError: - DieWithError('Git patch mungling failed.') - logging.info(patch_data) - - # We use "git apply" to apply the patch instead of "patch" so that we can - # pick up file adds. - # The --index flag means: also insert into the index (so we catch adds). - cmd = ['git', 'apply', '--index', '-p0'] - if directory: - cmd.extend(('--directory', directory)) - if reject: - cmd.append('--reject') - elif IsGitVersionAtLeast('1.7.12'): - cmd.append('--3way') - try: - subprocess2.check_call(cmd, env=GetNoGitPagerEnv(), - stdin=patch_data, stdout=subprocess2.VOID) - except subprocess2.CalledProcessError: - print('Failed to apply the patch') + scm_obj.apply_patch(patchset_object) + except Exception as e: + print(str(e)) return 1 # If we had an issue, commit the current state and register the issue. @@ -2047,24 +2002,23 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): match = re.match(r'/(\d+)/$', parsed_url.path) match2 = re.match(r'ps(\d+)$', parsed_url.fragment) if match and match2: - return _RietveldParsedIssueNumberArgument( + return _ParsedIssueNumberArgument( issue=int(match.group(1)), patchset=int(match2.group(1)), hostname=parsed_url.netloc) # Typical url: https://domain/[/[other]] match = re.match('/(\d+)(/.*)?$', parsed_url.path) if match: - return _RietveldParsedIssueNumberArgument( + return _ParsedIssueNumberArgument( issue=int(match.group(1)), hostname=parsed_url.netloc) # Rietveld patch: https://domain/download/issue_.diff match = re.match(r'/download/issue(\d+)_(\d+).diff$', parsed_url.path) if match: - return _RietveldParsedIssueNumberArgument( + return _ParsedIssueNumberArgument( issue=int(match.group(1)), patchset=int(match.group(2)), - hostname=parsed_url.netloc, - patch_url=gclient_utils.UpgradeToHttps(parsed_url.geturl())) + hostname=parsed_url.netloc) return None def CMDUploadChange(self, options, args, change): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index b61385396..64b211083 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -74,6 +74,23 @@ class RietveldMock(object): def close_issue(_issue): return 'Closed' + @staticmethod + def get_patch(issue, patchset): + return 'patch set from issue %s patchset %s' % (issue, patchset) + + +class GitCheckoutMock(object): + def __init__(self, *args, **kwargs): + pass + + @staticmethod + def reset(): + GitCheckoutMock.conflict = False + + def apply_patch(self, p): + if GitCheckoutMock.conflict: + raise Exception('failed') + class WatchlistsMock(object): def __init__(self, _): @@ -156,13 +173,10 @@ class TestGitClBasic(unittest.TestCase): return result def test_ParseIssueURL_rietveld(self): - def test(url, issue=None, patchset=None, hostname=None, patch_url=None, - fail=None): - result = self._test_ParseIssueUrl( + def test(url, issue=None, patchset=None, hostname=None, fail=None): + self._test_ParseIssueUrl( git_cl._RietveldChangelistImpl.ParseIssueURL, url, issue, patchset, hostname, fail) - if not fail: - self.assertEqual(result.patch_url, patch_url) test('http://codereview.chromium.org/123', 123, None, 'codereview.chromium.org') @@ -175,8 +189,7 @@ class TestGitClBasic(unittest.TestCase): test('https://codereview.chromium.org/123/#ps20001', 123, 20001, 'codereview.chromium.org') test('http://codereview.chromium.org/download/issue123_4.diff', - 123, 4, 'codereview.chromium.org', - patch_url='https://codereview.chromium.org/download/issue123_4.diff') + 123, 4, 'codereview.chromium.org') # This looks like bad Gerrit, but is actually valid Rietveld. test('https://chrome-review.source.com/123/4/', 123, None, 'chrome-review.source.com') @@ -271,6 +284,8 @@ class TestGitCl(TestCase): self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock) self.mock(git_cl.rietveld, 'Rietveld', RietveldMock) self.mock(git_cl.rietveld, 'CachingRietveld', RietveldMock) + self.mock(git_cl.checkout, 'GitCheckout', GitCheckoutMock) + GitCheckoutMock.reset() self.mock(git_cl.upload, 'RealMain', self.fail) self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock) self.mock(git_cl.auth, 'get_authenticator_for_host', AuthenticatorMock) @@ -1297,8 +1312,6 @@ class TestGitCl(TestCase): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset', lambda x: '60001') - self.mock(git_cl._RietveldChangelistImpl, 'GetPatchSetDiff', - lambda *args: None) self.mock(git_cl._GerritChangelistImpl, '_GetChangeDetail', lambda *args: { 'current_revision': '7777777777', @@ -1345,21 +1358,19 @@ class TestGitCl(TestCase): self.calls += [ ((['git', 'config', 'gerrit.host'],), CERR1), ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'config', 'branch.master.rietveldserver',],), CERR1), ((['git', 'rev-parse', '--show-cdup'],), ''), - ((['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'],), ''), ] def _common_patch_successful(self, new_branch=False): self._patch_common(new_branch=new_branch) self.calls += [ - ((['git', 'apply', '--index', '-p0', '--3way'],), ''), ((['git', 'commit', '-m', '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'],), CERR1), ((['git', 'config', 'branch.master.rietveldserver', 'https://codereview.example.com'],), ''), ((['git', 'config', 'branch.master.rietveldpatchset', '60001'],), @@ -1376,9 +1387,7 @@ class TestGitCl(TestCase): def test_patch_conflict(self): self._patch_common() - self.calls += [ - ((['git', 'apply', '--index', '-p0', '--3way'],), CERR1), - ] + GitCheckoutMock.conflict = True self.assertNotEqual(git_cl.main(['patch', '123456']), 0) def test_gerrit_patch_successful(self):