Add support for landing to pending ref.

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."""
 
 from distutils.version import LooseVersion
@@ skipping 56 matching lines @@
   print >> sys.stderr, message
   sys.exit(1)
 
 
 def GetNoGitPagerEnv():
   env = os.environ.copy()
   # 'cat' is a magical git string that disables pagers on all platforms.
   env['GIT_PAGER'] = 'cat'
   return env
 
+
 def RunCommand(args, error_ok=False, error_message=None, **kwargs):
   try:
     return subprocess2.check_output(args, shell=False, **kwargs)
   except subprocess2.CalledProcessError as e:
     logging.debug('Failed running %s', args)
     if not error_ok:
       DieWithError(
           'Command "%s" failed.\n%s' % (
             ' '.join(args), error_message or e.stdout or ''))
     return e.stdout
@@ skipping 186 matching lines @@
     self.cc = None
     self.root = None
     self.is_git_svn = None
     self.svn_branch = None
     self.tree_status_url = None
     self.viewvc_url = None
     self.updated = False
     self.is_gerrit = None
     self.git_editor = None
     self.project = None
+    self.pending_ref_prefix = None
 
   def LazyUpdateIfNeeded(self):
     """Updates the settings from a codereview.settings file, if available."""
     if not self.updated:
       # The only value that actually changes the behavior is
       # autoupdate = "false". Everything else means "true".
       autoupdate = RunGit(['config', 'rietveld.autoupdate'],
                           error_ok=True
                           ).strip().lower()
 
@@ skipping 25 matching lines @@
     return RunGit(['rev-parse', '--show-cdup']).strip()
 
   def GetRoot(self):
     if self.root is None:
       self.root = os.path.abspath(self.GetRelativeRoot())
     return self.root
 
   def GetIsGitSvn(self):
     """Return true if this repo looks like it's using git-svn."""
     if self.is_git_svn is None:
-      # If you have any "svn-remote.*" config keys, we think you're using svn.
-      self.is_git_svn = RunGitWithCode(
-          ['config', '--local', '--get-regexp', r'^svn-remote\.'])[0] == 0
+      if self.GetPendingRefPrefix():
+        # If PENDING_REF_PREFIX is set then it's a pure git repo no matter what.
+        self.is_git_svn = False
+      else:
+        # If you have any "svn-remote.*" config keys, we think you're using svn.
+        self.is_git_svn = RunGitWithCode(
+            ['config', '--local', '--get-regexp', r'^svn-remote\.'])[0] == 0
     return self.is_git_svn
 
   def GetSVNBranch(self):
     if self.svn_branch is None:
       if not self.GetIsGitSvn():
         DieWithError('Repo doesn\'t appear to be a git-svn repo.')
 
       # Try to figure out which remote branch we're based on.
       # Strategy:
       # 1) iterate through our branch history and find the svn URL.
@@ skipping 98 matching lines @@
 
   def GetLintIgnoreRegex(self):
     return (self._GetRietveldConfig('cpplint-ignore-regex', error_ok=True) or
             DEFAULT_LINT_IGNORE_REGEX)
 
   def GetProject(self):
     if not self.project:
       self.project = self._GetRietveldConfig('project', error_ok=True)
     return self.project
 
+  def GetPendingRefPrefix(self):
+    if not self.pending_ref_prefix:
+      self.pending_ref_prefix = self._GetRietveldConfig(
+          'pending-ref-prefix', error_ok=True)
+      if self.pending_ref_prefix:
+        self.pending_ref_prefix = self.pending_ref_prefix.rstrip('/') + '/'

[iannucci, 2014/08/21 20:32:20]

why not just take it as-is and then commit the right value?

[Vadim Sh., 2014/08/21 21:14:35]

Okay... just defensive programming.

+    return self.pending_ref_prefix
+
   def _GetRietveldConfig(self, param, **kwargs):
     return self._GetConfig('rietveld.' + param, **kwargs)
 
   def _GetConfig(self, param, **kwargs):
     self.LazyUpdateIfNeeded()
     return RunGit(['config', param], **kwargs).strip()
 
 
 def ShortBranchName(branch):
   """Convert a name like 'refs/heads/foo' to just 'foo'."""
@@ skipping 619 matching lines @@
   # Only server setting is required. Other settings can be absent.
   # In that case, we ignore errors raised during option deletion attempt.
   SetProperty('cc', 'CC_LIST', unset_error_ok=True)
   SetProperty('private', 'PRIVATE', unset_error_ok=True)
   SetProperty('tree-status-url', 'STATUS', unset_error_ok=True)
   SetProperty('viewvc-url', 'VIEW_VC', unset_error_ok=True)
   SetProperty('bug-prefix', 'BUG_PREFIX', unset_error_ok=True)
   SetProperty('cpplint-regex', 'LINT_REGEX', unset_error_ok=True)
   SetProperty('cpplint-ignore-regex', 'LINT_IGNORE_REGEX', unset_error_ok=True)
   SetProperty('project', 'PROJECT', unset_error_ok=True)
+  SetProperty('pending-ref-prefix', 'PENDING_REF_PREFIX', unset_error_ok=True)
 
   if 'GERRIT_HOST' in keyvals:
     RunGit(['config', 'gerrit.host', keyvals['GERRIT_HOST']])
 
   if 'PUSH_URL_CONFIG' in keyvals and 'ORIGIN_URL_CONFIG' in keyvals:
     #should be of the form
     #PUSH_URL_CONFIG: url.ssh://gitrw.chromium.org.pushinsteadof
     #ORIGIN_URL_CONFIG: http://src.chromium.org/git
     RunGit(['config', keyvals['PUSH_URL_CONFIG'],
             keyvals['ORIGIN_URL_CONFIG']])
@@ skipping 742 matching lines @@
     print
     print 'Either reparent this branch on top of origin/master:'
     print '  git reparent-branch --root'
     print
     print 'OR run `git rebase-update` if you think the parent branch is already'
     print 'committed.'
     print
     print '  Current parent: %r' % upstream_branch
     return 1
 
-  if not args or cmd == 'push':
+  if not args or cmd == 'land':

[iannucci, 2014/08/21 20:32:20]

good catch

     # Default to merging against our best guess of the upstream branch.
     args = [cl.GetUpstreamBranch()]
 
   if options.contributor:
     if not re.match('^.*\s<\S+@\S+>$', options.contributor):
       print "Please provide contibutor as 'First Last <email@example.com>'"
       return 1
 
   base_branch = args[0]
   base_has_submodules = IsSubmoduleMergeCommit(base_branch)
 
   if is_dirty_git_tree(cmd):
     return 1
 
   # This rev-list syntax means "show all commits not in my branch that
   # are in base_branch".
   upstream_commits = RunGit(['rev-list', '^' + cl.GetBranchRef(),
                              base_branch]).splitlines()
   if upstream_commits:
     print ('Base branch "%s" has %d commits '
            'not in this branch.' % (base_branch, len(upstream_commits)))
     print 'Run "git merge %s" before attempting to %s.' % (base_branch, cmd)
     return 1
 
   # This is the revision `svn dcommit` will commit on top of.
-  svn_head = RunGit(['log', '--grep=^git-svn-id:', '-1',
-                     '--pretty=format:%H'])
+  svn_head = None
+  if cmd == 'dcommit' or base_has_submodules:

[iannucci, 2014/08/21 20:32:20]

dcommit?

+    svn_head = RunGit(['log', '--grep=^git-svn-id:', '-1',

[Vadim Sh., 2014/08/21 19:31:22]

This stuff finds svn HEADs in pure git repos... For example in infra/infra
(because we have a chunk of history imported from git-svn repo). I decided it's
safer to have svn_head to be no None only when it's used in the code.

+                       '--pretty=format:%H'])
 
   if cmd == 'dcommit':
     # If the base_head is a submodule merge commit, the first parent of the
     # base_head should be a git-svn commit, which is what we're interested in.
     base_svn_head = base_branch
     if base_has_submodules:
       base_svn_head += '^1'
 
     extra_commits = RunGit(['rev-list', '^' + svn_head, base_svn_head])
     if extra_commits:
       print ('This branch has %d additional commits not upstreamed yet.'
              % len(extra_commits.splitlines()))
       print ('Upstream "%s" or rebase this branch on top of the upstream trunk '
              'before attempting to %s.' % (base_branch, cmd))
       return 1
 
   base_branch = RunGit(['merge-base', base_branch, 'HEAD']).strip()
   if not options.bypass_hooks:
     author = None
     if options.contributor:
       author = re.search(r'\<(.*)\>', options.contributor).group(1)
     hook_results = cl.RunHook(
         committing=True,
         may_prompt=not options.force,
         verbose=options.verbose,
         change=cl.GetChange(base_branch, author))
     if not hook_results.should_continue():
       return 1
 
-    if cmd == 'dcommit':

[Vadim Sh., 2014/08/21 19:31:22]

Now checks with 'git cl land' too.

-      # Check the tree status if the tree status URL is set.
-      status = GetTreeStatus()
-      if 'closed' == status:
-        print('The tree is closed.  Please wait for it to reopen. Use '
-              '"git cl dcommit --bypass-hooks" to commit on a closed tree.')
-        return 1
-      elif 'unknown' == status:
-        print('Unable to determine tree status.  Please verify manually and '
-              'use "git cl dcommit --bypass-hooks" to commit on a closed tree.')
+    # Check the tree status if the tree status URL is set.
+    status = GetTreeStatus()
+    if 'closed' == status:
+      print('The tree is closed.  Please wait for it to reopen. Use '
+            '"git cl %s --bypass-hooks" to commit on a closed tree.' % cmd)
+      return 1
+    elif 'unknown' == status:
+      print('Unable to determine tree status.  Please verify manually and '
+            'use "git cl %s --bypass-hooks" to commit on a closed tree.' % cmd)
+      return 1

[Vadim Sh., 2014/08/21 19:31:22]

Now aborts if tree status is unknown, as error message implied.

   else:
     breakpad.SendStack(
         'GitClHooksBypassedCommit',
         'Issue %s/%s bypassed hook when committing (tree status was "%s")' %
         (cl.GetRietveldServer(), cl.GetIssue(), GetTreeStatus()),
         verbose=False)
 
   change_desc = ChangeDescription(options.message)
   if not change_desc.description and cl.GetIssue():
     change_desc = ChangeDescription(cl.GetDescription())
@@ skipping 44 matching lines @@
   # trunk.  Move up to the top of the tree so that git commands that expect a
   # valid CWD won't fail after we check out the merge branch.
   rel_base_path = settings.GetRelativeRoot()
   if rel_base_path:
     os.chdir(rel_base_path)
 
   # Stuff our change into the merge branch.
   # We wrap in a try...finally block so if anything goes wrong,
   # we clean up the branches.
   retcode = -1
+  used_pending = False
   try:
     RunGit(['checkout', '-q', '-b', MERGE_BRANCH])
     RunGit(['reset', '--soft', base_branch])
     if options.contributor:
       RunGit(
           [
             'commit', '--author', options.contributor,
             '-m', commit_desc.description,
           ])
     else:
       RunGit(['commit', '-m', commit_desc.description])
     if base_has_submodules:
       cherry_pick_commit = RunGit(['rev-list', 'HEAD^!']).rstrip()
       RunGit(['branch', CHERRY_PICK_BRANCH, svn_head])
       RunGit(['checkout', CHERRY_PICK_BRANCH])
       RunGit(['cherry-pick', cherry_pick_commit])
-    if cmd == 'push':
-      # push the merge branch.
+    if cmd == 'land':
       remote, branch = cl.FetchUpstreamTuple(cl.GetBranch())
-      retcode, output = RunGitWithCode(
-          ['push', '--porcelain', remote, 'HEAD:%s' % branch])
+      pending_prefix = settings.GetPendingRefPrefix()
+      if not pending_prefix or branch.startswith(pending_prefix):
+        # If not using refs/pending/heads/* at all, or target ref is already set
+        # to pending, then push to the target ref directly.
+        retcode, output = RunGitWithCode(
+            ['push', '--porcelain', remote, 'HEAD:%s' % branch])
+        used_pending = pending_prefix and branch.startswith(pending_prefix)
+      else:
+        # Cherry-pick the change on top of pending ref and then push it.
+        assert branch.startswith('refs/'), branch
+        assert pending_prefix[-1] == '/', pending_prefix
+        pending_ref = pending_prefix + branch[len('refs/'):]
+        retcode, output = PushToGitPending(remote, pending_ref)
+        used_pending = True
       logging.debug(output)
     else:
       # dcommit the merge branch.
       retcode, output = RunGitWithCode(['svn', 'dcommit',
                                         '-C%s' % options.similarity,
                                         '--no-rebase', '--rmdir'])
   finally:
     # And then swap back to the original branch and clean up.
     RunGit(['checkout', '-q', cl.GetBranch()])
     RunGit(['branch', '-D', MERGE_BRANCH])
     if base_has_submodules:
       RunGit(['branch', '-D', CHERRY_PICK_BRANCH])
 
   if cl.GetIssue():
     if cmd == 'dcommit' and 'Committed r' in output:
       revision = re.match('.*?\nCommitted r(\\d+)', output, re.DOTALL).group(1)
-    elif cmd == 'push' and retcode == 0:
+    elif cmd == 'land' and retcode == 0:
       match = (re.match(r'.*?([a-f0-9]{7,})\.\.([a-f0-9]{7,})$', l)
                for l in output.splitlines(False))
       match = filter(None, match)
       if len(match) != 1:
         DieWithError("Couldn't parse ouput to extract the committed hash:\n%s" %
             output)
       revision = match[0].group(2)
     else:
       return 1
+    to_pending = ' to pending' if used_pending else ''
     viewvc_url = settings.GetViewVCUrl()
     if viewvc_url and revision:
-      change_desc.append_footer('Committed: ' + viewvc_url + revision)
+      change_desc.append_footer(
+          'Committed%s: %s%s' % (to_pending, viewvc_url, revision))
     elif revision:
-      change_desc.append_footer('Committed: ' + revision)
+      change_desc.append_footer('Committed%s: %s' % (to_pending, revision))
     print ('Closing issue '
            '(you may be prompted for your codereview password)...')
     cl.UpdateDescription(change_desc.description)
     cl.CloseIssue()
     props = cl.GetIssueProperties()
     patch_num = len(props['patchsets'])
-    comment = "Committed patchset #%d manually as %s" % (patch_num, revision)
+    comment = "Committed patchset #%d%s manually as %s" % (
+        patch_num, to_pending, revision)
     if options.bypass_hooks:
       comment += ' (tree was closed).' if GetTreeStatus() == 'closed' else '.'
     else:
       comment += ' (presubmit successful).'
     cl.RpcServer().add_comment(cl.GetIssue(), comment)
     cl.SetIssue(None)
 
   if retcode == 0:
     hook = POSTUPSTREAM_HOOK_PATTERN % cmd
     if os.path.isfile(hook):
       RunCommand([hook, base_branch], error_ok=True)
 
   return 0
 
 
+def PushToGitPending(remote, pending_ref):
+  """Fetches pending_ref, cherry-picks current HEAD on top of it, pushes.
+
+  Returns:
+    (retcode of last operation, output log of last operation).
+  """
+  cherry = RunGit(['rev-parse', 'HEAD']).strip()
+  code = 0
+  out = ''
+  attempt = 0
+  while attempt < 5:
+    attempt += 1
+
+    # Fetch. Retry fetch errors.
+    code, out = RunGitWithCode(
+        ['fetch', remote, '+%s:refs/git-cl/pending' % pending_ref])
+    if code:
+      continue
+
+    # Try to cherry pick. Abort on merge conflicts.
+    RunGitWithCode(['checkout', 'refs/git-cl/pending'])

[iannucci, 2014/08/21 20:37:03]

let's make this

refs/git-cl/pending/heads/master
refs/git-cl/pending/branch-heads/12345

etc.

[Vadim Sh., 2014/08/21 21:14:35]

Done.

+    code, out = RunGitWithCode(['cherry-pick', cherry])

[Vadim Sh., 2014/08/21 19:31:22]

cherry-pick supports different merge strategies. Any ideas what's the best one
for this case?

+    if code:
+      RunGitWithCode(['cherry-pick', '--abort'])

[Vadim Sh., 2014/08/21 19:31:22]

Not sure it's needed...

[iannucci, 2014/08/21 20:32:20]

yep

+      return code, out
+
+    # Applied cleanly, try to push now. Retry on error (flake or non-ff push).
+    code, out = RunGitWithCode(['push', remote, 'HEAD:%s' % pending_ref])

[Vadim Sh., 2014/08/21 19:31:22]

This can fail in 3 cases:
1. Network\git flake.
2. Non FF push.
3. ACLs.

(1) and (2) should be retried, but not (3). Unfortunately, detecting (3) is not
easy (git push logs all sorts of messages in case ACL\account is misconfigured).
Hardcode these strings here? Or screw it and retry ACL errors too?

[iannucci, 2014/08/21 20:32:20]

I think we should just put 'retry' before 'push'

[Vadim Sh., 2014/08/21 21:14:35]

Done.

+    if not code:

[iannucci, 2014/08/21 20:32:20]

let's do `code == 0`

[Vadim Sh., 2014/08/21 21:14:35]

Done.

+      # Success.
+      return code, out
+
+  return code, out
+
+
 @subcommand.usage('[upstream branch to apply against]')
 def CMDdcommit(parser, args):
   """Commits the current changelist via git-svn."""
   if not settings.GetIsGitSvn():
     message = """This doesn't appear to be an SVN repository.
 If your project has a git mirror with an upstream SVN master, you probably need
 to run 'git svn init', see your project's git mirror documentation.
 If your project has a true writeable upstream repository, you probably want
 to run 'git cl land' instead.
 Choose wisely, if you get this wrong, your commit might appear to succeed but
 will instead be silently ignored."""
     print(message)
     ask_for_data('[Press enter to dcommit or ctrl-C to quit]')
   return SendUpstream(parser, args, 'dcommit')
 
 
 @subcommand.usage('[upstream branch to apply against]')
 def CMDland(parser, args):
   """Commits the current changelist via git."""
   if settings.GetIsGitSvn():
     print('This appears to be an SVN repository.')
     print('Are you sure you didn\'t mean \'git cl dcommit\'?')
     ask_for_data('[Press enter to push or ctrl-C to quit]')
-  return SendUpstream(parser, args, 'push')
+  return SendUpstream(parser, args, 'land')
 
 
 @subcommand.usage('<patch url or issue id>')
 def CMDpatch(parser, args):
   """Patches in a code review."""
   parser.add_option('-b', dest='newbranch',
                     help='create a new branch off trunk for the patch')
   parser.add_option('-f', '--force', action='store_true',
                     help='with -b, clobber any existing branch')
   parser.add_option('-d', '--directory', action='store', metavar='DIR',
@@ skipping 584 matching lines @@
         ('AppEngine is misbehaving and returned HTTP %d, again. Keep faith '
           'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
 
 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()
   sys.exit(main(sys.argv[1:]))