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

@@ -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)
 
 

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 += [