From 3e631421d257eec7478186fb92fae167be7e95f0 Mon Sep 17 00:00:00 2001
From: Andrii Shyshkalov <tandrii@chromium.org>
Date: Thu, 16 Feb 2017 17:46:44 +0100
Subject: [PATCH] Gerrit git cl upload: abort early if change is submitted or
 abandoned.

BUG=689652
R=agable@chromium.org,martiniss@chromium.org

Change-Id: Ib9f5b08f5b31a1519a5f5916042885967a263d88
Reviewed-on: https://chromium-review.googlesource.com/443325
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
---
 git_cl.py            | 36 +++++++++++++++++++++++++++++++++---
 tests/git_cl_test.py | 26 ++++++++++++++------------
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/git_cl.py b/git_cl.py
index 1deebe3d3f..c06fc87cda 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1503,10 +1503,11 @@ class Changelist(object):
       base_branch = self.GetCommonAncestorWithUpstream()
       git_diff_args = [base_branch, 'HEAD']
 
-    # Make sure authenticated to codereview before running potentially expensive
-    # hooks.  It is a fast, best efforts check. Codereview still can reject the
-    # authentication during the actual upload.
+    # Fast best-effort checks to abort before running potentially
+    # expensive hooks if uploading is likely to fail anyway. Passing these
+    # checks does not guarantee that uploading will not fail.
     self._codereview_impl.EnsureAuthenticated(force=options.force)
+    self._codereview_impl.EnsureCanUploadPatchset()
 
     # Apply watchlists on upload.
     change = self.GetChange(base_branch, None)
@@ -1757,6 +1758,15 @@ class _ChangelistCodereviewBase(object):
     """
     raise NotImplementedError()
 
+  def EnsureCanUploadPatchset(self):
+    """Best effort check that uploading isn't supposed to fail for predictable
+    reasons.
+
+    This method should raise informative exception if uploading shouldn't
+    proceed.
+    """
+    pass
+
   def CMDUploadChange(self, options, args, change):
     """Uploads a change to codereview."""
     raise NotImplementedError()
@@ -2310,6 +2320,26 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
                      cookie_auth.get_netrc_path(),
                      cookie_auth.get_new_password_message(git_host)))
 
+  def EnsureCanUploadPatchset(self):
+    """Best effort check that uploading isn't supposed to fail for predictable
+    reasons.
+
+    This method should raise informative exception if uploading shouldn't
+    proceed.
+    """
+    if not self.GetIssue():
+      return
+
+    # Warm change details cache now to avoid RPCs later, reducing latency for
+    # developers.
+    self.FetchDescription()
+
+    status = self._GetChangeDetail()['status']
+    if status in ('MERGED', 'ABANDONED'):
+      DieWithError('Change %s has been %s, new uploads are not allowed' %
+                   (self.GetIssueURL(),
+                    'submitted' if status == 'MERGED' else 'abandoned'))
+
   def _PostUnsetIssueProperties(self):
     """Which branch-specific properties to erase when unsetting issue."""
     return ['gerritsquashhash']
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index a2e37332cb..3d977268ca 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -1152,18 +1152,7 @@ class TestGitCl(TestCase):
            'refs/remotes/origin/master'],),
          'fake_ancestor_sha'),
         # Calls to verify branch point is ancestor
-      ] + (cls._gerrit_ensure_auth_calls(issue=issue) +
-           cls._git_sanity_checks('fake_ancestor_sha', 'master',
-                                  get_remote_branch=False)) + [
-        ((['git', 'rev-parse', '--show-cdup'],), ''),
-        ((['git', 'rev-parse', 'HEAD'],), '12345'),
-
-        ((['git',
-           'diff', '--name-status', '--no-renames', '-r',
-           'fake_ancestor_sha...', '.'],),
-         'M\t.gitignore\n'),
-        ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1),
-      ] + ([
+      ] + cls._gerrit_ensure_auth_calls(issue=issue) + ([
         (('GetChangeDetail', 'chromium-review.googlesource.com',
           '123456', ['CURRENT_REVISION', 'CURRENT_COMMIT']),
          {
@@ -1172,7 +1161,20 @@ class TestGitCl(TestCase):
            'revisions': { 'sha1_of_current_revision': {
              'commit': {'message': fetched_description},
            }},
+           'status': 'NEW',
          }),
+      ] if issue else [
+      ]) + cls._git_sanity_checks('fake_ancestor_sha', 'master',
+                                  get_remote_branch=False) + [
+        ((['git', 'rev-parse', '--show-cdup'],), ''),
+        ((['git', 'rev-parse', 'HEAD'],), '12345'),
+
+        ((['git',
+           'diff', '--name-status', '--no-renames', '-r',
+           'fake_ancestor_sha...', '.'],),
+         'M\t.gitignore\n'),
+        ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1),
+      ] + ([
       ] if issue else [
         ((['git',
            'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],),