Gerrit git cl: stop creating a shadow branch.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index c3b92903b7de07db6cedc99e15208d43f8c275d1..d722213a94847b761d810eb85b9f61da002e4ea4 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -2518,6 +2518,9 @@ def GenerateGerritChangeId(message):
 
 def GerritUpload(options, args, cl, change):
   """upload the current branch to gerrit."""
+  # TODO(tandrii): refactor this to be a method of _GerritChangelistImpl,
+  # to avoid private members accessors below.
+
   # We assume the remote called "origin" is the one we want.
   # It is probably not worthwhile to support different workflows.
   gerrit_remote = 'origin'
@@ -2526,52 +2529,92 @@ def GerritUpload(options, args, cl, change):
   branch = GetTargetRef(remote, remote_branch, options.target_branch,
                         pending_prefix='')
 
-  change_desc = ChangeDescription(
-      options.message or CreateDescriptionFromLog(args))
-  if not change_desc.description:
-    print "\nDescription is empty. Aborting..."
-    return 1
-
   if options.title:
     print "\nPatch titles (-t) are not supported in Gerrit. Aborting..."
     return 1
 
   if options.squash:
-    # Try to get the message from a previous upload.
-    shadow_branch = 'refs/heads/git_cl_uploads/' + cl.GetBranch()
-    message = RunGitSilent(['show', '--format=%B', '-s', shadow_branch])
-    if not message:
+    if cl.GetIssue():
+      # Try to get the message from a previous upload.
+      message = cl.GetDescription()
+      if not message:
+        DieWithError(
+            'failed to fetch description from current Gerrit issue %d\n'
+            '%s' % (cl.GetIssue(), cl.GetIssueURL()))
+      change_id = cl._codereview_impl._GetChangeDetail([])['change_id']
+      footer_change_ids = git_footers.get_footer_change_id(message)
+      if footer_change_ids != [change_id]:
+        while footer_change_ids:
+          # There is already a valid footer but with different or several ids.
+          # Doing this automatically is non-trivial as we don't want to loose
+          # existing other footers, yet we want to append just 1 desired
+          # Change-Id. That's non-trival, so just create new footer, but let
+          # user verify the new description.
+          message = '%s\n\nChange-Id: %s' % (message, change_id)
+          print((
+              'WARNING: issue %s has Change-Id footer(s):\n'
+              '  %s\n'
+              'but issue has Change-Id %s, according to Gerrit.\n'
+              'Please, check the proposed correction to the description,\n'
+              'and edit it if necessary but keep the "Change-Id: %s" footer\n'
+              ) % (cl.GetIssue(), '\n  '.join(footer_change_ids), change_id,
+                   change_id, cl.GetIssueUrl()))
+          ask_for_data('Press enter to edit now, Ctrl+C to abort')
+          if options.force:

[Bernhard Bauer, 2016/03/29 13:27:38]

Why do we do this only if force is _set_?

[tandrii(chromium), 2016/03/29 14:46:32]

good catch, missing "not".

+            change_desc = ChangeDescription(message)
+            change_desc.prompt()
+            message = change_desc.description
+            if not message:
+              DieWithError("Description is empty. Aborting...")
+          footer_change_ids = git_footers.get_footer_change_id(message)
+          if footer_change_ids == [change_id] or not footer_change_ids:
+            break
+          continue

[Bernhard Bauer, 2016/03/29 13:27:38]

This is the last statement in the body of a while loop, so it doesn't actually
do anything, no?

I think we could simplify the control flow in here a bit. How about this:

  change_id = cl._codereview_impl._GetChangeDetail([])['change_id']
  while True:
    footer_change_ids = git_footers.get_footer_change_id(message)
    if not footer_change_ids:
      message = git_footers.add_footer_change_id(message, change_id)
      print('WARNING: appended missing Change-Id to issue description')
      continue

    if footer_change_ids == [change_id]:
      break

    # There is already a valid footer [...]
    message = '%s\n\nChange-Id: %s' % (message, change_id)
    # ...

  # Sanity check [...]
  assert [change_id] == git_footers.get_footer_change_id(message)

[tandrii(chromium), 2016/03/29 14:46:32]

Yep, but I thought reading is easier that way.
Hey, I like that. Fixed.

+        if not footer_change_ids:
+          message = git_footers.add_footer_change_id(message, change_id)
+          print('WARNING: appended missing Change-Id to issue description')
+      # Sanity check of this code - we should end up with proper message footer.
+      assert [change_id] == git_footers.get_footer_change_id(message)
+      change_desc = ChangeDescription(message)
+    else:
+      change_desc = ChangeDescription(
+          options.message or CreateDescriptionFromLog(args))
       if not options.force:
         change_desc.prompt()
       if not change_desc.description:
-        print "Description is empty; aborting."
-        return 1
+        DieWithError("Description is empty. Aborting...")
       message = change_desc.description
       change_ids = git_footers.get_footer_change_id(message)
       if len(change_ids) > 1:
-        DieWithError('too many Change-Id footers in %s branch' % shadow_branch)
+        DieWithError('too many Change-Id footers, at most 1 allowed.')
       if not change_ids:
+        # Generate the Change-Id automatically.
         message = git_footers.add_footer_change_id(
             message, GenerateGerritChangeId(message))
         change_desc.set_description(message)
         change_ids = git_footers.get_footer_change_id(message)
         assert len(change_ids) == 1
-
       change_id = change_ids[0]
 
     remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
     if remote is '.':
       # If our upstream branch is local, we base our squashed commit on its
       # squashed version.
-      parent = ('refs/heads/git_cl_uploads/' +
-                scm.GIT.ShortBranchName(upstream_branch))
-
-      # Verify that the upstream branch has been uploaded too, otherwise Gerrit
-      # will create additional CLs when uploading.
-      if (RunGitSilent(['rev-parse', upstream_branch + ':']) !=
-          RunGitSilent(['rev-parse', parent + ':'])):
-        print 'Upload upstream branch ' + upstream_branch + ' first.'
-        return 1
+      upstream_branch_name = scm.GIT.ShortBranchName(upstream_branch)
+      # Check the squashed hash of the parent.
+      parent = RunGit(['config',
+                       'branch.%s.gerritsquashhash' % upstream_branch_name],
+                      error_ok=True).strip()
+      # Verify that the upstream branch has been uploaded too, otherwise
+      # Gerrit will create additional CLs when uploading.
+      if not parent or (RunGitSilent(['rev-parse', upstream_branch + ':']) !=
+                        RunGitSilent(['rev-parse', parent + ':'])):
+        # TODO(tandrii): remove "old depot_tools" part on April 12, 2016.
+        DieWithError(
+            'Upload upstream branch %s first.\n'
+            'Note: maybe you\'ve uploaded it with --no-squash or with an old\n'
+            '      version of depot_tools. If so, then re-upload it with:\n'
+            '      git cl upload --squash\n' % upstream_branch_name)
     else:
       parent = cl.GetCommonAncestorWithUpstream()
 
@@ -2579,6 +2622,11 @@ def GerritUpload(options, args, cl, change):
     ref_to_push = RunGit(['commit-tree', tree, '-p', parent,
                           '-m', message]).strip()
   else:
+    change_desc = ChangeDescription(
+        options.message or CreateDescriptionFromLog(args))
+    if not change_desc.description:
+      DieWithError("Description is empty. Aborting...")
+
     if not git_footers.get_footer_change_id(change_desc.description):
       DownloadGerritHook(False)
       change_desc.set_description(AddChangeIdToCommitMessage(options, args))
@@ -2586,6 +2634,7 @@ def GerritUpload(options, args, cl, change):
     parent = '%s/%s' % (gerrit_remote, branch)
     change_id = git_footers.get_footer_change_id(change_desc.description)[0]
 
+  assert change_desc
   commits = RunGitSilent(['rev-list', '%s..%s' % (parent,
                                                   ref_to_push)]).splitlines()
   if len(commits) > 1:
@@ -2632,8 +2681,8 @@ def GerritUpload(options, args, cl, change):
         ('Created|Updated %d issues on Gerrit, but only 1 expected.\n'
          'Change-Id: %s') % (len(change_numbers), change_id))
     cl.SetIssue(change_numbers[0])
-    head = RunGit(['rev-parse', 'HEAD']).strip()
-    RunGit(['update-ref', '-m', 'Uploaded ' + head, shadow_branch, ref_to_push])
+    RunGit(['config', 'branch.%s.gerritsquashhash' % cl.GetBranch(),
+            ref_to_push])
   return 0
 
 
@@ -2924,6 +2973,9 @@ def CMDupload(parser, args):
   options.reviewers = cleanup_list(options.reviewers)
   options.cc = cleanup_list(options.cc)
 
+  # For sanity of test expectations, do this otherwise lazy-loading *now*.
+  settings.GetIsGerrit()
+
   cl = Changelist(auth_config=auth_config)
   if args:
     # TODO(ukai): is it ok for gerrit case?