From c2786d933bb6517a88ba6ff95e1606f656d89178 Mon Sep 17 00:00:00 2001 From: "tandrii@chromium.org" Date: Tue, 31 May 2016 19:53:50 +0000 Subject: [PATCH] Fix & refactor git cl patch. * Fixes a bug when issue was incorrectly over-written in another branch. Add a test case. * Refactor for better flow. * Update outdated errors on wrong arguments. BUG=616105 R=andybons@chromium.org,sergiyb@chromium.org Review-Url: https://codereview.chromium.org/2022183003 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@300681 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl.py | 49 ++++++++++++++++++++++---------------------- tests/git_cl_test.py | 23 ++++++++++----------- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/git_cl.py b/git_cl.py index 35785ebdf..241e20315 100755 --- a/git_cl.py +++ b/git_cl.py @@ -4092,14 +4092,18 @@ def CMDpatch(parser, args): _process_codereview_select_options(parser, options) auth_config = auth.extract_auth_config_from_options(options) - cl = Changelist(auth_config=auth_config, codereview=options.forced_codereview) - issue_arg = None if options.reapply : + if options.newbranch: + parser.error('--reapply works on the current branch only') if len(args) > 0: - parser.error('--reapply implies no additional arguments.') + parser.error('--reapply implies no additional arguments') + + cl = Changelist(auth_config=auth_config, + codereview=options.forced_codereview) + if not cl.GetIssue(): + parser.error('current branch must have an associated issue') - issue_arg = cl.GetIssue() upstream = cl.GetUpstreamBranch() if upstream == None: parser.error('No upstream branch specified. Cannot reset branch') @@ -4107,37 +4111,34 @@ def CMDpatch(parser, args): RunGit(['reset', '--hard', upstream]) if options.pull: RunGit(['pull']) - else: - if len(args) != 1: - parser.error('Must specify issue number or url') - issue_arg = args[0] - if not issue_arg: - parser.print_help() - return 1 + return cl.CMDPatchIssue(cl.GetIssue(), options.reject, options.nocommit, + options.directory) - if cl.IsGerrit(): - if options.reject: - parser.error('--reject is not supported with Gerrit codereview.') - if options.nocommit: - parser.error('--nocommit is not supported with Gerrit codereview.') - if options.directory: - parser.error('--directory is not supported with Gerrit codereview.') + if len(args) != 1 or not args[0]: + parser.error('Must specify issue number or url') # We don't want uncommitted changes mixed up with the patch. if git_common.is_dirty_git_tree('patch'): return 1 if options.newbranch: - if options.reapply: - parser.error("--reapply excludes any option other than --pull") if options.force: RunGit(['branch', '-D', options.newbranch], - stderr=subprocess2.PIPE, error_ok=True) - RunGit(['checkout', '-b', options.newbranch, - Changelist().GetUpstreamBranch()]) + stderr=subprocess2.PIPE, error_ok=True) + RunGit(['new-branch', options.newbranch]) + + cl = Changelist(auth_config=auth_config, codereview=options.forced_codereview) + + if cl.IsGerrit(): + if options.reject: + parser.error('--reject is not supported with Gerrit codereview.') + if options.nocommit: + parser.error('--nocommit is not supported with Gerrit codereview.') + if options.directory: + parser.error('--directory is not supported with Gerrit codereview.') - return cl.CMDPatchIssue(issue_arg, options.reject, options.nocommit, + return cl.CMDPatchIssue(args[0], options.reject, options.nocommit, options.directory) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 82a404ccc..3924b22d9 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1109,14 +1109,6 @@ class TestGitCl(TestCase): def test_patch_when_dirty(self): # Patch when local tree is dirty self.mock(git_common, 'is_dirty_git_tree', lambda x: True) - self.calls = [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), ''), - ((['git', 'config', 'branch.master.gerritissue'],), ''), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'gerrit.host'],), ''), - ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), - ] self.assertNotEqual(git_cl.main(['patch', '123456']), 0) def test_diff_when_dirty(self): @@ -1154,16 +1146,15 @@ class TestGitCl(TestCase): lambda *args: 'Description') self.mock(git_cl, 'IsGitVersionAtLeast', lambda *args: True) + self.calls = self.calls or [] if not force_codereview: # These calls detect codereview to use. - self.calls = [ + self.calls += [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.rietveldissue'],), ''), ((['git', 'config', 'branch.master.gerritissue'],), ''), ((['git', 'config', 'rietveld.autoupdate'],), ''), ] - else: - self.calls = [] if is_gerrit: if not force_codereview: @@ -1178,7 +1169,7 @@ class TestGitCl(TestCase): ((['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'],), ''), ] - def test_patch_successful(self): + def _common_patch_successful(self): self._patch_common() self.calls += [ ((['git', 'apply', '--index', '-p0', '--3way'],), ''), @@ -1192,8 +1183,16 @@ class TestGitCl(TestCase): 'https://codereview.example.com'],), ''), ((['git', 'config', 'branch.master.rietveldpatchset', '60001'],), ''), ] + + def test_patch_successful(self): + self._common_patch_successful() self.assertEqual(git_cl.main(['patch', '123456']), 0) + def test_patch_successful_new_branch(self): + self.calls = [ ((['git', 'new-branch', 'master'],), ''), ] + self._common_patch_successful() + self.assertEqual(git_cl.main(['patch', '-b', 'master', '123456']), 0) + def test_patch_conflict(self): self._patch_common() self.calls += [