Support merging with conflicts with git-drover.

git_drover.py

Index: git_drover.py
diff --git a/git_drover.py b/git_drover.py
index 73c92004aa66eb613f6ea2f31179f95044bd7e77..6fad0a3bfbdffed62a5668be5efff158513217e7 100755
--- a/git_drover.py
+++ b/git_drover.py
@@ -5,6 +5,7 @@
 """git drover: A tool for merging changes to release branches."""
 
 import argparse
+import cPickle
 import functools
 import logging
 import os
@@ -20,6 +21,28 @@ class Error(Exception):
   pass
 
 
+_PATCH_ERROR_MESSAGE = """Patch failed to apply.
+
+A workdir for this cherry-pick has been created in
+  {0}
+
+To continue, resolve the conflicts there and run
+  git drover --continue {0}
+
+To abort this cherry-pick run
+  git drover --abort {0}
+"""
+
+
+class PatchError(Error):
+  """An error indicating that the patch failed to apply."""
+
+  def __init__(self, workdir):
+    super(PatchError, self).__init__(_PATCH_ERROR_MESSAGE.format(workdir))
+
+
+_DEV_NULL_FILE = open(os.devnull, 'w')
+
 if os.name == 'nt':
   # This is a just-good-enough emulation of os.symlink for drover to work on
   # Windows. It uses junctioning of directories (most of the contents of
@@ -43,7 +66,7 @@ else:
 
 class _Drover(object):
 
-  def __init__(self, branch, revision, parent_repo, dry_run):
+  def __init__(self, branch, revision, parent_repo, dry_run, verbose):
     self._branch = branch
     self._branch_ref = 'refs/remotes/branch-heads/%s' % branch
     self._revision = revision
@@ -51,7 +74,55 @@ class _Drover(object):
     self._dry_run = dry_run
     self._workdir = None
     self._branch_name = None
-    self._dev_null_file = open(os.devnull, 'w')
+    self._needs_cleanup = True
+    self._verbose = verbose
+
+  @classmethod
+  def resume(cls, workdir):
+    """Continues a cherry-pick that required manual resolution.
+
+    Args:
+      workdir: A string containing the path to the workdir used by drover.
+    """
+    drover = cls._restore_drover(workdir)
+    drover._continue()
+
+  @classmethod
+  def abort(cls, workdir):
+    """Aborts a cherry-pick that required manual resolution.
+
+    Args:
+      workdir: A string containing the path to the workdir used by drover.
+    """
+    drover = cls._restore_drover(workdir)
+    drover._cleanup()

[iannucci, 2015/11/02 20:37:58]

will this work if they're cd'd into the directory?

[Sam McNally, 2015/11/03 00:53:44]

Yes, except on Windows. On Windows it reports it to the user and asks them to
manually delete it. It seems to delete the contents at least, so it cleans up
most of the way.

+
+  @staticmethod
+  def _restore_drover(workdir):
+    """Restores a saved drover state contained within a workdir.
+
+    Args:
+      workdir: A string containing the path to the workdir used by drover.
+    """
+    try:
+      with open(os.path.join(workdir, '.git', 'drover'), 'rb') as f:
+        drover = cPickle.load(f)
+        if drover._verbose:
+          logging.getLogger().setLevel(logging.DEBUG)

[iannucci, 2015/11/02 20:37:58]

weird, but I see what you did there. Would be nice if this type of functionality
(resuming original CLI params) had it's own function instead of be inlined here.
I suspect it'll get extended in-line and will become quite messy.

[Sam McNally, 2015/11/03 00:53:44]

Done.

+        return drover
+    except (IOError, cPickle.UnpicklingError):
+      raise Error('%r is not git drover workdir' % workdir)
+
+  def _continue(self):
+    if os.path.exists(os.path.join(self._workdir, '.git', 'CHERRY_PICK_HEAD')):
+      self._run_git_command(
+          ['commit', '--no-edit'],
+          error_message='All conflicts must be resolved before continuing')
+
+    if self._upload_and_land():
+      # Only clean up the workdir on success. The manually resolved cherry-pick
+      # can be reused if the user cancels before landing.
+      self._cleanup()
 
   def run(self):
     """Runs this Drover instance.
@@ -70,35 +141,29 @@ class _Drover(object):
         self._run_git_command(['show', '-s', self._revision]), self._branch)):
       return
     self._create_checkout()
-    self._prepare_cherry_pick()
-    if self._dry_run:
-      logging.info('--dry_run enabled; not landing.')
-      return
-
-    self._run_git_command(['cl', 'upload'],
-                          error_message='Upload failed',
-                          interactive=True)
+    self._perform_cherry_pick()
+    self._upload_and_land()
 
-    if not self._confirm('About to land on %s.' % self._branch):
+  def _cleanup(self):
+    if not self._needs_cleanup:
       return
-    self._run_git_command(['cl', 'land', '--bypass-hooks'], interactive=True)
 
-  def _cleanup(self):
-    if self._branch_name:
-      try:
-        self._run_git_command(['cherry-pick', '--abort'])
-      except Error:
-        pass
-      self._run_git_command(['checkout', '--detach'])
-      self._run_git_command(['branch', '-D', self._branch_name])
     if self._workdir:
       logging.debug('Deleting %s', self._workdir)
       if os.name == 'nt':
-        # Use rmdir to properly handle the junctions we created.
-        subprocess.check_call(['rmdir', '/s', '/q', self._workdir], shell=True)
+        try:
+          # Use rmdir to properly handle the junctions we created.
+          subprocess.check_call(
+              ['rmdir', '/s', '/q', self._workdir], shell=True)
+        except subprocess.CalledProcessError:
+          logging.error(
+              'Failed to delete workdir %r. Please remove it manually.',
+              self._workdir)
       else:
         shutil.rmtree(self._workdir)
-    self._dev_null_file.close()
+    self._workdir = None
+    if self._branch_name:
+      self._run_git_command(['branch', '-D', self._branch_name])
 
   @staticmethod
   def _confirm(message):
@@ -166,25 +231,63 @@ class _Drover(object):
                                    self.FILES_TO_COPY, mk_symlink)
     self._run_git_command(['config', 'core.sparsecheckout', 'true'])
     with open(os.path.join(git_dir, 'info', 'sparse-checkout'), 'w') as f:
-      f.write('codereview.settings')
+      f.write('/codereview.settings')
 
     branch_name = os.path.split(self._workdir)[-1]
     self._run_git_command(['checkout', '-b', branch_name, self._branch_ref])
     self._branch_name = branch_name
 
-  def _prepare_cherry_pick(self):
-    self._run_git_command(['cherry-pick', '-x', self._revision],
-                          error_message='Patch failed to apply')
+  def _perform_cherry_pick(self):
+    try:
+      self._run_git_command(['cherry-pick', '-x', self._revision],
+                            error_message='Patch failed to apply')
+    except Error:
+      self._prepare_manual_resolve()
+      self._save_state()
+      self._needs_cleanup = False
+      raise PatchError(self._workdir)
+
+  def _save_state(self):
+    """Saves the state of this Drover instances to the workdir."""
+    with open(os.path.join(self._workdir, '.git', 'drover'), 'wb') as f:
+      cPickle.dump(self, f)
+
+  def _prepare_manual_resolve(self):
+    """Prepare the workdir for the user to manually resolve the cherry-pick."""
+    # Files that have been deleted between branch and cherry-pick will not have
+    # their skip-worktree bit set so set it manually for those files to avoid
+    # git status incorrectly listing them as unstaged deletes.
+    repo_status = self._run_git_command(['status', '--porcelain']).splitlines()
+    extra_files = [f[3:] for f in repo_status if f[:2] == ' D']
+    if extra_files:
+      self._run_git_command(['update-index', '--skip-worktree', '--'] +
+                            extra_files)
+
+  def _upload_and_land(self):
+    if self._dry_run:
+      logging.info('--dry_run enabled; not landing.')
+      return True
+
     self._run_git_command(['reset', '--hard'])
+    self._run_git_command(['cl', 'upload'],
+                          error_message='Upload failed',
+                          interactive=True)
+
+    if not self._confirm('About to land on %s.' % self._branch):
+      return False
+    self._run_git_command(['cl', 'land', '--bypass-hooks'], interactive=True)
+    return True
 
   def _run_git_command(self, args, error_message=None, interactive=False):
     """Runs a git command.
 
     Args:
       args: A list of strings containing the args to pass to git.
-      interactive:
       error_message: A string containing the error message to report if the
           command fails.
+      interactive: A bool containing whether the command requires user
+          interaction. If false, the command will be provided with no input and
+          the output is captured.
 
     Raises:
       Error: The command failed to complete successfully.
@@ -195,11 +298,11 @@ class _Drover(object):
 
     run = subprocess.check_call if interactive else subprocess.check_output
 
+    # Discard stderr unless verbose is enabled.
+    stderr = None if self._verbose else _DEV_NULL_FILE
+
     try:
-      return run(['git'] + args,
-                 shell=False,
-                 cwd=cwd,
-                 stderr=self._dev_null_file)
+      return run(['git'] + args, shell=False, cwd=cwd, stderr=stderr)
     except (OSError, subprocess.CalledProcessError) as e:
       if error_message:
         raise Error(error_message)
@@ -207,7 +310,7 @@ class _Drover(object):
         raise Error('Command %r failed: %s' % (' '.join(args), e))
 
 
-def cherry_pick_change(branch, revision, parent_repo, dry_run):
+def cherry_pick_change(branch, revision, parent_repo, dry_run, verbose=False):
   """Cherry-picks a change into a branch.
 
   Args:
@@ -218,31 +321,64 @@ def cherry_pick_change(branch, revision, parent_repo, dry_run):
         revision.
     parent_repo: A string containing the path to the parent repo to use for this
         cherry-pick.
-    dry_run: A boolean containing whether to stop before uploading the
+    dry_run: A bool containing whether to stop before uploading the
         cherry-pick cl.
+    verbose: A bool containing whether to print verbose logging.
 
   Raises:
     Error: An error occurred while attempting to cherry-pick |cl| to |branch|.
   """
-  drover = _Drover(branch, revision, parent_repo, dry_run)
+  drover = _Drover(branch, revision, parent_repo, dry_run, verbose)
   drover.run()
 
 
+def continue_cherry_pick(workdir):
+  """Continues a cherry-pick that required manual resolution.
+
+  Args:
+    workdir: A string containing the path to the workdir used by drover.
+  """
+  _Drover.resume(workdir)
+
+
+def abort_cherry_pick(workdir):
+  """Aborts a cherry-pick that required manual resolution.
+
+  Args:
+    workdir: A string containing the path to the workdir used by drover.
+  """
+  _Drover.abort(workdir)
+
+
 def main():
   parser = argparse.ArgumentParser(
       description='Cherry-pick a change into a release branch.')
+  group = parser.add_mutually_exclusive_group(required=True)
   parser.add_argument(
       '--branch',
       type=str,
-      required=True,
       metavar='<branch>',
       help='the name of the branch to which to cherry-pick; e.g. 1234')
-  parser.add_argument('--cherry-pick',
-                      type=str,
-                      required=True,
-                      metavar='<change>',
-                      help=('the change to cherry-pick; this can be any string '
-                            'that unambiguously refers to a revision'))
+  group.add_argument(
+      '--cherry-pick',
+      type=str,
+      metavar='<change>',
+      help=('the change to cherry-pick; this can be any string '
+            'that unambiguously refers to a revision not involving HEAD'))
+  group.add_argument(
+      '--continue',
+      type=str,
+      nargs='?',
+      dest='resume',
+      const=os.path.abspath('.'),
+      metavar='path_to_workdir',
+      help='Continue a drover cherry-pick after resolving conflicts')
+  group.add_argument('--abort',
+                     type=str,
+                     nargs='?',
+                     const=os.path.abspath('.'),
+                     metavar='path_to_workdir',
+                     help='Abort a drover cherry-pick')
   parser.add_argument(
       '--parent_checkout',
       type=str,
@@ -266,10 +402,18 @@ def main():
   if options.verbose:
     logging.getLogger().setLevel(logging.DEBUG)
   try:
-    cherry_pick_change(options.branch, options.cherry_pick,
-                       options.parent_checkout, options.dry_run)
+    if options.resume:
+      _Drover.resume(options.resume)
+    elif options.abort:
+      _Drover.abort(options.abort)
+    else:
+      if not options.branch:
+        parser.error('argument --branch is required for --cherry-pick')
+      cherry_pick_change(options.branch, options.cherry_pick,
+                         options.parent_checkout, options.dry_run,
+                         options.verbose)
   except Error as e:
-    logging.error(e.message)
+    print 'Error:', e.message
     sys.exit(128)