Now trychange can store patches in a Git repo

trychange.py

Index: trychange.py
diff --git a/trychange.py b/trychange.py
index 99070612648ad46b8d2197c0e0ea7499bc94f0b7..4cb8ed89cca68e63886ca1a773371962f388451d 100755
--- a/trychange.py
+++ b/trychange.py
@@ -4,8 +4,8 @@
 # found in the LICENSE file.
 
 """Client-side script to send a try job to the try server. It communicates to
-the try server by either writting to a svn repository or by directly connecting
-to the server by HTTP.
+the try server by either writting to a svn/git repository or by directly
+connecting to the server by HTTP.
 """
 
 import datetime
@@ -70,6 +70,7 @@ Examples:
             -f include/b.h
 """
 
+GIT_PATCH_DIR_BASENAME = os.path.join('git-try', 'patches-git')
 
 def DieWithError(message):
   print >> sys.stderr, message
@@ -164,7 +165,10 @@ class SCM(object):
       'port': self.GetCodeReviewSetting('TRYSERVER_HTTP_PORT'),
       'host': self.GetCodeReviewSetting('TRYSERVER_HTTP_HOST'),
       'svn_repo': self.GetCodeReviewSetting('TRYSERVER_SVN_URL'),
+      'git_repo': self.GetCodeReviewSetting('TRYSERVER_GIT_URL'),
       'project': self.GetCodeReviewSetting('TRYSERVER_PROJECT'),
+      # primarily for revision=auto
+      'revision': self.GetCodeReviewSetting('TRYSERVER_REVISION'),
       'root': self.GetCodeReviewSetting('TRYSERVER_ROOT'),
       'patchlevel': self.GetCodeReviewSetting('TRYSERVER_PATCHLEVEL'),
     }
@@ -286,6 +290,7 @@ class SVN(SCM):
 
 class GIT(SCM):
   """Gathers the options and diff for a git checkout."""
+
   def __init__(self, *args, **kwargs):
     SCM.__init__(self, *args, **kwargs)
     self.checkout_root = scm.GIT.GetCheckoutRoot(self.checkout_root)
@@ -401,7 +406,8 @@ def _GenTSBotSpec(checkouts, change, changed_files, options):
 
 
 def _ParseSendChangeOptions(bot_spec, options):
-  """Parse common options passed to _SendChangeHTTP and _SendChangeSVN."""
+  """Parse common options passed to _SendChangeHTTP, _SendChangeSVN and
+  _SendChangeGit."""

[agable, 2014/03/04 21:45:59]

nit: multiline docstring formatting.

[nodir, 2014/03/04 22:51:19]

Done.

   values = [
     ('user', options.user),
     ('name', options.name),
@@ -472,6 +478,36 @@ def _SendChangeHTTP(bot_spec, options):
     raise NoTryServerAccess('%s is unaccessible. Got:\n%s' % (url, response))
 
 
+def _PrepareDescriptionAndPatchFiles(description, options, callback):
+  """Create temporary files with description and patch, execute the callback
+  and delete the files.
+
+  Args:

[agable, 2014/03/04 21:45:59]

Should be a complete list of args, not just the confusing ones.

[nodir, 2014/03/04 22:51:19]

Done.

+    callback: a function with signature (patch_filename, description_filename),
+      executed between birth and death of temporary files.
+  """
+
+  # Create a temporary file and put description into it

[agable, 2014/03/04 21:45:59]

nit: sentence-like comments should have periods.

[nodir, 2014/03/04 22:51:19]

Done.

+  with tempfile.NamedTemporaryFile() as description_file:
+    description_file.write(description)
+    description_file.flush()
+
+    # Create a temporary directory, put a uniquely named file in it with the
+    # diff content
+    temp_dir = tempfile.mkdtemp()
+    try:
+      current_time = str(datetime.datetime.now()).replace(':', '.')
+      file_name = '%s.%s.%s.diff' % (Escape(options.user),
+                                     Escape(options.name), current_time)
+      full_patch_filename = os.path.join(temp_dir, file_name)
+      with open(full_patch_filename, 'wb') as f:
+        f.write(options.diff)
+
+      callback(full_patch_filename, description_file.name)
+    finally:
+      shutil.rmtree(temp_dir, True)
+
+
 def _SendChangeSVN(bot_spec, options):
   """Send a change to the try server by committing a diff file on a subversion
   server."""
@@ -488,45 +524,111 @@ def _SendChangeSVN(bot_spec, options):
   if options.dry_run:
     return
 
-  # Create a temporary directory, put a uniquely named file in it with the diff
-  # content and svn import that.
-  temp_dir = tempfile.mkdtemp()
-  temp_file = tempfile.NamedTemporaryFile()
-  try:
+  def send(patch_filename, description_filename):
+    """Send the patch file to SVN repo with commit message from the
+    description file"""
+    if sys.platform == "cygwin":
+      # Small chromium-specific issue here:
+      # git-try uses /usr/bin/python on cygwin but svn.bat will be used
+      # instead of /usr/bin/svn by default. That causes bad things(tm) since
+      # Windows' svn.exe has no clue about cygwin paths. Hence force to use
+      # the cygwin version in this particular context.
+      exe = "/usr/bin/svn"
+    else:
+      exe = "svn"
+    patch_dir = os.path.dirname(patch_filename)
+    command = [exe, 'import', '-q', patch_dir, options.svn_repo, '--file',
+               description_filename]
+    if scm.SVN.AssertVersion("1.5")[0]:
+      command.append('--no-ignore')
+
     try:
-      # Description
-      temp_file.write(description)
-      temp_file.flush()
+      subprocess2.check_call(command)
+    except subprocess2.CalledProcessError, e:
+      raise NoTryServerAccess(str(e))
 
-      # Diff file
-      current_time = str(datetime.datetime.now()).replace(':', '.')
-      file_name = (Escape(options.user) + '.' + Escape(options.name) +
-                   '.%s.diff' % current_time)
-      full_path = os.path.join(temp_dir, file_name)
-      with open(full_path, 'wb') as f:
-        f.write(options.diff)
+  _PrepareDescriptionAndPatchFiles(description, options, send)
 
-      # Committing it will trigger a try job.
-      if sys.platform == "cygwin":
-        # Small chromium-specific issue here:
-        # git-try uses /usr/bin/python on cygwin but svn.bat will be used
-        # instead of /usr/bin/svn by default. That causes bad things(tm) since
-        # Windows' svn.exe has no clue about cygwin paths. Hence force to use
-        # the cygwin version in this particular context.
-        exe = "/usr/bin/svn"
-      else:
-        exe = "svn"
-      command = [exe, 'import', '-q', temp_dir, options.svn_repo, '--file',
-                 temp_file.name]
-      if scm.SVN.AssertVersion("1.5")[0]:
-        command.append('--no-ignore')
 
-      subprocess2.check_call(command)
+def _GetPatchGitRepo(git_url):
+  """Get a path to a Git repo with patches with latest changes,
+  associated with the given url.
+
+  Store patches in .git/git-try/patches-git directory, a git repo. If it
+  doesn't exist yet or its origin URL is different, then clean up and clone it.
+  If it existed before, then pull changes.
+
+  Does not support SVN repo.
+
+  Returns a path to the directory with patches.
+  """
+  git_dir = scm.GIT.GetGitDir(os.getcwd())
+  patch_dir = os.path.join(git_dir, GIT_PATCH_DIR_BASENAME)
+
+  logging.info('Looking for git repo for patches')
+  # Is there already a repo with the expected url or should we clone?
+  clone = True
+  if os.path.exists(patch_dir) and scm.GIT.IsInsideWorkTree(patch_dir):
+    existing_url = scm.GIT.Capture(
+        ['config', '--local', 'remote.origin.url'],
+        cwd=patch_dir)
+    clone = existing_url != git_url
+
+  if clone:
+    if os.path.exists(patch_dir):
+      logging.info('Cleaning up')
+      shutil.rmtree(patch_dir, True)
+    logging.info('Cloning patch repo')
+    scm.GIT.Capture(['clone', git_url, GIT_PATCH_DIR_BASENAME], cwd=git_dir)
+  else:
+    if scm.GIT.IsWorkTreeDirty(patch_dir):
+      logging.info('Work dir is dirty: hard reset!')
+      scm.GIT.Capture(['reset', '--hard'], cwd=patch_dir)
+    logging.info('Updating patch repo')
+    scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir)
+
+  return patch_dir
+
+
+def _SendChangeGit(bot_spec, options):
+  """Send a change to the try server by committing a diff file to a GIT repo"""
+  if not options.git_repo:
+    raise NoTryServerAccess('Please use the --git_repo option to specify the '
+                            'try server git repository to connect to.')
+
+  values = _ParseSendChangeOptions(bot_spec, options)
+  comment_subject = '%s.%s' % (options.user, options.name)
+  comment_body = ''.join("%s=%s\n" % (k, v) for k, v in values)
+  description = '%s\n\n%s' % (comment_subject, comment_body)
+  logging.info('Sending by GIT')
+  logging.info(description)
+  logging.info(options.git_repo)
+  logging.info(options.diff)
+  if options.dry_run:
+    return
+
+  patch_dir = _GetPatchGitRepo(options.git_repo)
+  assert scm.GIT.IsInsideWorkTree(patch_dir)
+  assert not scm.GIT.IsWorkTreeDirty(patch_dir)
+
+  def send(patch_filename, description_filename):
+    """Sends the patch file to GIT repo with the description file contents as
+    commit message"""
+    logging.info('Committing patch')
+    target_filename = os.path.join(patch_dir, os.path.basename(patch_filename))
+    target_filename = os.path.abspath(target_filename)

[agable, 2014/03/04 21:45:59]

Combine this into one statement, with indentation at +4 on the second line.

[nodir, 2014/03/04 22:51:19]

Done.

+    shutil.copyfile(patch_filename, target_filename)
+    try:
+      scm.GIT.Capture(['add', '-f', target_filename], cwd=patch_dir)

[agable, 2014/03/04 21:45:59]

the generated patch file that is being added into the repo should never be in
.gitignore. Why '-f'?

[nodir, 2014/03/04 22:51:19]

Done.

+      scm.GIT.Capture(['commit', '-F', description_filename], cwd=patch_dir)
+      assert not scm.GIT.IsWorkTreeDirty(patch_dir)
+      logging.info('Pushing patch')
+      scm.GIT.Capture(['push', 'origin', 'HEAD'], cwd=patch_dir)
     except subprocess2.CalledProcessError, e:
+      scm.GIT.Capture(['reset', '--hard', 'origin/master'], cwd=patch_dir)
       raise NoTryServerAccess(str(e))
-  finally:
-    temp_file.close()
-    shutil.rmtree(temp_dir, True)
+
+  _PrepareDescriptionAndPatchFiles(description, options, send)
 
 
 def PrintSuccess(bot_spec, options):
@@ -642,7 +744,9 @@ def gen_parser(prog):
                          " and exit.  Do not send patch.  Like --dry_run"
                          " but less verbose.")
   group.add_option("-r", "--revision",
-                    help="Revision to use for the try job; default: the "
+                    help="Revision to use for the try job. If 'auto' is "
+                         "specified, it is resolved to the revision a patch is "
+                         "generated against (Git only). Default: the "
                          "revision will be determined by the try server; see "
                          "its waterfall for more info")
   group.add_option("-c", "--clobber", action="store_true",
@@ -728,6 +832,18 @@ def gen_parser(prog):
                    help="SVN url to use to write the changes in; --use_svn is "
                         "implied when using --svn_repo")
   parser.add_option_group(group)
+
+  group = optparse.OptionGroup(parser, "Access the try server with Git")
+  group.add_option("--use_git",
+                   action="store_const",
+                   const=_SendChangeGit,
+                   dest="send_patch",
+                   help="Use GIT to talk to the try server")
+  group.add_option("-G", "--git_repo",
+                   metavar="GIT_URL",
+                   help="GIT url to use to write the changes in; --use_git is "
+                        "implied when using --git_repo")
+  parser.add_option_group(group)
   return parser
 
 
@@ -810,12 +926,28 @@ def TryChange(argv,
       # Clear file list so that the correct list will be retrieved from the
       # upstream branch.
       file_list = []
-    checkouts.append(GuessVCS(options, path, file_list))
-    checkouts[0].AutomagicalSettings()
+
+    current_vcs = GuessVCS(options, path, file_list)
+    current_vcs.AutomagicalSettings()
+    vcs_is_git = type(current_vcs) is GIT
+
+    # So far, git_repo doesn't work with SVN
+    if options.git_repo and not vcs_is_git:

[agable, 2014/03/04 21:45:59]

current_vcs.options.git_repo, to ensure settings from AutomagicalSettings() are
picked up.

[nodir, 2014/03/04 22:51:19]

AFAIU, SCM/GIT classes do not clone options, so `options is current_vcs.options`

[agable, 2014/03/05 19:49:50]

Yes, that's correct. current_vcs.options === options, and the current code is
correct. I mostly want to make the code more *obviously* correct, by not relying
on magical side-effects of AutomagicalSettings() affecting things outside the
current_vcs object. 

[nodir, 2014/03/05 20:07:00]

Done.

+      parser.error('--git_repo option is supported only for GIT repositories')
+
+    # If revision==auto, resolve it
+    if options.revision and options.revision.lower() == 'auto':
+      if not vcs_is_git:
+        parser.error('--revision=auto is supported only for GIT repositories')
+      options.revision = scm.GIT.Capture(
+          ['rev-parse', current_vcs.diff_against],
+          cwd=path)
+
+    checkouts.append(current_vcs)
     for item in options.sub_rep:
       # Pass file_list=None because we don't know the sub repo's file list.
       checkout = GuessVCS(options,
-                          os.path.join(checkouts[0].checkout_root, item),
+                          os.path.join(current_vcs.checkout_root, item),
                           None)
       if checkout.checkout_root in [c.checkout_root for c in checkouts]:
         parser.error('Specified the root %s two times.' %
@@ -824,9 +956,10 @@ def TryChange(argv,
 
     can_http = options.port and options.host
     can_svn = options.svn_repo
+    can_git = options.git_repo
     # If there was no transport selected yet, now we must have enough data to
     # select one.
-    if not options.send_patch and not (can_http or can_svn):
+    if not options.send_patch and not (can_http or can_svn or can_git):
       parser.error('Please specify an access method.')
 
     # Convert options.diff into the content of the diff.
@@ -904,23 +1037,30 @@ def TryChange(argv,
           print '  %s' % (bot[0])
       return 0
 
-    # Send the patch.
+    # Determine sending protocol
     if options.send_patch:
       # If forced.
-      options.send_patch(bot_spec, options)
-      PrintSuccess(bot_spec, options)
-      return 0
-    try:
-      if can_http:
-        _SendChangeHTTP(bot_spec, options)
+      senders = [options.send_patch]
+    else:
+      # Try sending patch using avaialble protocols
+      all_senders = [
+        (_SendChangeHTTP, can_http),
+        (_SendChangeSVN, can_svn),
+        (_SendChangeGit, can_git)
+      ]
+      senders = [sender for sender, can in all_senders if can]
+
+    # Send the patch.
+    for sender in senders:
+      try:
+        sender(bot_spec, options)
         PrintSuccess(bot_spec, options)
         return 0
-    except NoTryServerAccess:
-      if not can_svn:
-        raise
-    _SendChangeSVN(bot_spec, options)
-    PrintSuccess(bot_spec, options)
-    return 0
+      except NoTryServerAccess:
+        is_last = sender == senders[-1]
+        if is_last:
+          raise
+    assert False, "Unreachable code"
   except (InvalidScript, NoTryServerAccess), e:
     if swallow_exception:
       return 1