git-cl patch: set metadata before (failing to) apply patch

If the commit or cherry-pick command fails, git automatically drops
the user into a state where they can try to manually fix it. That's
great. But with the old call order, it would also leave the metadata
unset. This sets the issue and patchset number before doing the
potentially-failing heavy lifting, so that the branch will be in a
consistent state after the user finishes fixing up the failed command.

BUG=701130

Change-Id: I792b9fb9e61ba62626c19aa1837d21f8cd8f594e
Reviewed-on: https://chromium-review.googlesource.com/456039Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>

git_cl.py

@@ -2070,12 +2070,12 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
 
     # If we had an issue, commit the current state and register the issue.
     if not nocommit:
+      self.SetIssue(self.GetIssue())
+      self.SetPatchset(patchset)
       RunGit(['commit', '-m', (self.GetDescription() + '\n\n' +
                                'patch from issue %(i)s at patchset '
                                '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
                                % {'i': self.GetIssue(), 'p': patchset})])
-      self.SetIssue(self.GetIssue())
-      self.SetPatchset(patchset)
       print('Committed patch locally.')
     else:
       print('Patch applied to index.')
@@ -2635,9 +2635,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
     fetch_info = revision_info['fetch']['http']
     RunGit(['fetch', fetch_info['url'], fetch_info['ref']])
-    RunGit(['cherry-pick', 'FETCH_HEAD'])
     self.SetIssue(self.GetIssue())
     self.SetPatchset(patchset)
+    RunGit(['cherry-pick', 'FETCH_HEAD'])
     print('Committed patch for change %i patchset %i locally' %
           (self.GetIssue(), self.GetPatchset()))
     return 0

tests/git_cl_test.py

@@ -1815,16 +1815,16 @@ class TestGitCl(TestCase):
   def _common_patch_successful(self, new_branch=False):
     self._patch_common(new_branch=new_branch)
     self.calls += [
-      ((['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',
          'https://codereview.example.com'],), ''),
       ((['git', 'config', 'branch.master.rietveldpatchset', '60001'],),
        ''),
+      ((['git', 'commit', '-m',
+         'Description\n\n' +
+         'patch from issue 123456 at patchset 60001 ' +
+         '(http://crrev.com/123456#ps60001)'],), ''),
     ]
 
   def test_patch_successful(self):
@@ -1846,12 +1846,12 @@ class TestGitCl(TestCase):
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
-      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
       ((['git', 'config', 'branch.master.gerritissue', '123456'],),
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
       ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
     ]
     self.assertEqual(git_cl.main(['patch', '123456']), 0)
 
@@ -1861,12 +1861,12 @@ class TestGitCl(TestCase):
     self.calls += [
       ((['git', 'fetch', 'https://host.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
-      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
       ((['git', 'config', 'branch.master.gerritissue', '123456'],),
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://host-review.googlesource.com'],), ''),
       ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
     ]
     self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0)
 
@@ -1876,12 +1876,12 @@ class TestGitCl(TestCase):
     self.calls += [
       ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
-      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
       ((['git', 'config', 'branch.master.gerritissue', '123456'],),
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://else-review.googlesource.com'],), ''),
       ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
     ]
     self.assertEqual(git_cl.main(
       ['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0)
@@ -1892,6 +1892,11 @@ class TestGitCl(TestCase):
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
+      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
+       ''),
+      ((['git', 'config', 'branch.master.gerritserver',
+        'https://chromium-review.googlesource.com'],), ''),
+      ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
       ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
        SystemExitMock()),