Add support for landing to pending ref.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 82e9208632860decd8ad16c546eb57e433f6482e..6d27621d3b4eeea6100bd987748d98160a32242e 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -74,6 +74,7 @@ def GetNoGitPagerEnv():
   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)
@@ -280,6 +281,7 @@ class Settings(object):
     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."""
@@ -325,9 +327,13 @@ class Settings(object):
   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):
@@ -446,6 +452,14 @@ class Settings(object):
       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)
 
@@ -1085,6 +1099,7 @@ def LoadCodereviewSettingsFromFile(fileobj):
   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']])
@@ -1847,7 +1862,7 @@ def SendUpstream(parser, args, cmd):
     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()]
 
@@ -1873,8 +1888,10 @@ def SendUpstream(parser, args, 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
@@ -1904,16 +1921,16 @@ def SendUpstream(parser, args, cmd):
     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',
@@ -1978,6 +1995,7 @@ def SendUpstream(parser, args, cmd):
   # 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])
@@ -1994,11 +2012,22 @@ def SendUpstream(parser, args, cmd):
       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.
@@ -2015,7 +2044,7 @@ def SendUpstream(parser, args, cmd):
   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)
@@ -2025,18 +2054,21 @@ def SendUpstream(parser, args, cmd):
       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:
@@ -2052,6 +2084,41 @@ def SendUpstream(parser, args, cmd):
   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."""
@@ -2075,7 +2142,7 @@ def CMDland(parser, args):
     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>')