Add support for new repo topology, with submodule-specific merge commits

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."""
 
 import json
@@ skipping 51 matching lines @@
     return e.stdout
 
 
 def RunGit(args, **kwargs):
   """Returns stdout."""
   return RunCommand(['git'] + args, **kwargs)
 
 
 def RunGitWithCode(args):
   """Returns return code and stdout."""
-  out, code = subprocess2.communicate(['git'] + args, stdout=subprocess2.PIPE)
-  return code, out[0]
+  try:
+    out, code = subprocess2.communicate(['git'] + args, stdout=subprocess2.PIPE)
+    return code, out[0]
+  except ValueError, e:
+    return 1, ''

[M-A Ruel, 2012/06/12 16:27:36]

Can you add a comment when it's happening?

[szager1, 2012/06/13 13:54:46]

When the subprocess fails, communicate() returns None.  That triggers a
ValueError when trying to unpack the return value into (out, code).

I added the above comment to the code.

 
 
 def usage(more):
   def hook(fn):
     fn.usage_more = more
     return fn
   return hook
 
 
 def ask_for_data(prompt):
@@ skipping 1044 matching lines @@
   if 'GIT_EXTERNAL_DIFF' in env:
     del env['GIT_EXTERNAL_DIFF']
   subprocess2.call(
       ['git', 'diff', '--no-ext-diff', '--stat', '-M'] + args, env=env)
 
   if settings.GetIsGerrit():
     return GerritUpload(options, args, cl)
   return RietveldUpload(options, args, cl)
 
 
+def IsSubmoduleMergeCommit(ref):
+  # When submodules are added to the repo, we expect there to be a single
+  # non-git-svn merge commit at remote HEAD with a signature comment.
+  pattern = '^SVN changes up to revision [0-9]*$'
+  cmd = ['rev-list', '--merges', '--grep="%s"' % pattern, '%s^!' % ref]
+  return RunGit(cmd) != ''
+
+
 def SendUpstream(parser, args, cmd):
   """Common code for CmdPush and CmdDCommit
 
   Squashed commit into a single.
   Updates changelog with metadata (e.g. pointer to review).
   Pushes/dcommits the code upstream.
   Updates review and closes.
   """
   parser.add_option('--bypass-hooks', action='store_true', dest='bypass_hooks',
                     help='bypass upload presubmit hook')
@@ skipping 11 matching lines @@
   if not args or cmd == 'push':
     # 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)
 
   # Make sure index is up-to-date before running diff-index.
   RunGit(['update-index', '--refresh', '-q'], error_ok=True)
   if RunGit(['diff-index', 'HEAD']):
     print 'Cannot %s with a dirty tree.  You must commit locally first.' % 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'])
+
   if cmd == 'dcommit':
-    # This is the revision `svn dcommit` will commit on top of.
-    svn_head = RunGit(['log', '--grep=^git-svn-id:', '-1',
-                       '--pretty=format:%H'])
-    extra_commits = RunGit(['rev-list', '^' + svn_head, base_branch])
+    # If the base_head is a submodule merge commit, the second parent of the

[M-A Ruel, 2012/06/12 16:27:36]

I still think it should be the primary parent (and not the second parent) in the
repository. e.g.

# On every new commit in trunk, run all of the following:
# Update refs/heads/trunk
git svn fetch
# Checkout detached HEAD
git checkout $(git rev-parse refs/heads/trunk)
git merge refs/heads/master
# Note NEW_HASH
git checkout refs/heads/master
git reset --hard NEW_HASH

Not tested, but I think it can fast forward even if it is not the primary
parent. I don't think it's exactly the right flow of commands either, especially
that it requires a full checkout and wouldn't work on a bare checkout.

[szager1, 2012/06/13 13:54:46]

Done.

+    # 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 += '^2'
+
+    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
 
   if not options.bypass_hooks:
     author = None
     if options.contributor:
@@ skipping 38 matching lines @@
 
   if options.contributor:
     description += "\nPatch from %s." % options.contributor
   print 'Description:', repr(description)
 
   branches = [base_branch, cl.GetBranchRef()]
   if not options.force:
     subprocess2.call(['git', 'diff', '--stat'] + branches)
     ask_for_data('About to commit; enter to confirm.')
 
-  # We want to squash all this branch's commits into one commit with the
-  # proper description.
-  # We do this by doing a "reset --soft" to the base branch (which keeps
-  # the working copy the same), then dcommitting that.
+  # We want to squash all this branch's commits into one commit with the proper
+  # description. We do this by doing a "reset --soft" to the base branch (which
+  # keeps the working copy the same), then dcommitting that.  If origin/master
+  # has a submodule merge commit, we'll also need to cherry-pick the squashed
+  # commit onto a branch based on the git-svn head.
   MERGE_BRANCH = 'git-cl-commit'
-  # Delete the merge branch if it already exists.
-  if RunGitWithCode(['show-ref', '--quiet', '--verify',
-                     'refs/heads/' + MERGE_BRANCH])[0] == 0:
-    RunGit(['branch', '-D', MERGE_BRANCH])
+  CHERRY_PICK_BRANCH = 'git-cl-cherry-pick'
+  # Delete the branches if they exist

[cmp, 2012/06/12 16:05:46]

nit: append a period to make this a first class sentence.

[szager1, 2012/06/13 13:54:46]

Done.

+  for branch in [MERGE_BRANCH, CHERRY_PICK_BRANCH]:
+    cmd = ['show-ref', '--quiet', '--verify', 'refs/heads/%s' % branch]
+    result = RunGitWithCode(cmd)
+    if result[0] == 0:
+      RunGit(['branch', '-D', branch])
 
   # We might be in a directory that's present in this branch but not in the
   # 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 = RunGit(['rev-parse', '--show-cdup']).strip()
   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
   try:
     RunGit(['checkout', '-q', '-b', MERGE_BRANCH])
     RunGit(['reset', '--soft', base_branch])
     if options.contributor:
       RunGit(['commit', '--author', options.contributor, '-m', description])
     else:
       RunGit(['commit', '-m', 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.
       remote, branch = cl.FetchUpstreamTuple()
       retcode, output = RunGitWithCode(
           ['push', '--porcelain', remote, 'HEAD:%s' % branch])
       logging.debug(output)
     else:
       # dcommit the merge branch.
       retcode, output = RunGitWithCode(['svn', 'dcommit',
                                         '--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:
       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" %
@@ skipping 266 matching lines @@
              'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
   # Not a known command. Default to help.
   GenUsage(parser, 'help')
   return CMDhelp(parser, argv)
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   sys.exit(main(sys.argv[1:]))