Gerrit git cl: stop creating a shadow branch.

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from distutils.version import LooseVersion
@@ skipping 2506 matching lines @@
 def GerritUpload(options, args, cl, change):
   """upload the current branch to gerrit."""
   # We assume the remote called "origin" is the one we want.
   # It is probably not worthwhile to support different workflows.
   gerrit_remote = 'origin'
 
   remote, remote_branch = cl.GetRemoteBranch()
   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()))
+      # Issues already on Gerrit really must have exactly 1 Change-Id.

[Bernhard Bauer, 2016/03/29 08:22:49]

In practice this could fail (e.g. an old CL that was pushed without a Change-Id
in the description). I'm not really sure how to handle that case though... Maybe
just treat it as if there was no descripion?

[tandrii(chromium), 2016/03/29 08:37:41]

Didn't know it was possible, but let's assume so. Then how do you indicate to
Gerrit that you are uploading a new patchset for *specific* issue?

My take so far is that if this assert fails, a bug to infra will be filed, and
we'll deal with it once it happens.

[Bernhard Bauer, 2016/03/29 09:03:21]

If a commit is pushed that doesn't have a Change-Id (or it's not in the footer,
etc.), Gerrit internally assigns one, but it doesn't change the CL description.
You can see an example of that at
https://chromium-review.googlesource.com/#/c/329268; Gerrit shows the internal
Change-Id below the description text field. If you want to upload a new patch
set to that CL, it needs to have a commit message that uses that internal
Change-Id in its footer.
Hm.... I'm not super happy with putting additional work (even just filing a bug)
on the shoulders of a random developer who is just trying to upload a CL. Can we
at least expand the error message to say something like "Contact chrome-infra@"?
:)

[tandrii(chromium), 2016/03/29 13:06:24]

Ah, i see.
Ah, given that this is likely to happen, I agree, too much burden. So how do you
like the new version?

+      change_ids = git_footers.get_footer_change_id(message)
+      assert len(change_ids) == 1
+      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
       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]
+    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 old\n'

[Bernhard Bauer, 2016/03/29 08:22:49]

Nit: "[...] with an old version [...]".

[tandrii(chromium), 2016/03/29 13:06:24]

Done.

+            '      version of depot_tools. If so, then re-upload it with:\n'
+            '      git cl upload --squash\n' % upstream_branch_name)
     else:
       parent = cl.GetCommonAncestorWithUpstream()
 
     tree = RunGit(['rev-parse', 'HEAD:']).strip()
     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:
+      print "\nDescription is empty. Aborting..."

[ukai, 2016/03/29 02:16:16]

nit: DieWithError?

what is difference between DieWithError and return 1?

[tandrii(chromium), 2016/03/29 08:37:41]

Same thing, afaiu. This was copy-paste from above, but will change here too.

[tandrii(chromium), 2016/03/29 13:06:24]

Done.

+      return 1
+
     if not git_footers.get_footer_change_id(change_desc.description):
       DownloadGerritHook(False)
       change_desc.set_description(AddChangeIdToCommitMessage(options, args))
     ref_to_push = 'HEAD'
     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:
     print('WARNING: This will upload %d commits. Run the following command '
           'to see which commits will be uploaded: ' % len(commits))
     print('git log %s..%s' % (parent, ref_to_push))
     print('You can also use `git squash-branch` to squash these into a single '
           'commit.')
     ask_for_data('About to upload; enter to confirm.')
 
@@ skipping 26 matching lines @@
   if options.squash:
     regex = re.compile(r'remote:\s+https?://[\w\-\.\/]*/(\d+)\s.*')
     change_numbers = [m.group(1)
                       for m in map(regex.match, push_stdout.splitlines())
                       if m]
     if len(change_numbers) != 1:
       DieWithError(
         ('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
 
 
 def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
   """Computes the remote branch ref to use for the CL.
 
   Args:
     remote (str): The git remote for the CL.
     remote_branch (str): The git remote branch for the CL.
     target_branch (str): The target branch specified by the user.
@@ skipping 270 matching lines @@
   auth.add_auth_options(parser)
   (options, args) = parser.parse_args(args)
   auth_config = auth.extract_auth_config_from_options(options)
 
   if git_common.is_dirty_git_tree('upload'):
     return 1
 
   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?
     base_branch = args[0]
   else:
     if cl.GetBranch() is None:
       DieWithError('Can\'t upload from detached HEAD state. Get on a branch!')
 
     # Default to diffing against common ancestor of upstream branch
     base_branch = cl.GetCommonAncestorWithUpstream()
@@ skipping 1389 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   colorama.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)