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

git_cl.py

Index: git_cl.py
===================================================================
--- git_cl.py	(revision 141621)
+++ git_cl.py	(working copy)
@@ -69,8 +69,11 @@
 
 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):
@@ -1135,6 +1138,14 @@
   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
 
@@ -1166,6 +1177,7 @@
       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)
@@ -1183,11 +1195,18 @@
     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()))
@@ -1246,15 +1265,19 @@
     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
@@ -1274,6 +1297,11 @@
       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()
@@ -1288,6 +1316,8 @@
     # 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: