Now trychange can store patches in a Git repo

trychange.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.
 
 """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
 import errno
 import getpass
 import itertools
 import json
 import logging
 import optparse
 import os
@@ skipping 44 matching lines @@
   premade diff file on the local drive:
     %(prog)s --email user@example.com
             --svn_repo svn://svn.chromium.org/chrome-try/try --diff foo.diff
 
   Running only on a 'mac' slave with revision 123 and clobber first; specify
   manually the 3 source files to use for the try job:
     %(prog)s --bot mac --revision 123 --clobber -f src/a.cc -f src/a.h
             -f include/b.h
 """
 
+GIT_PATCH_DIR_BASENAME = os.path.join('git-try', 'patches-git')
 
 def DieWithError(message):
   print >> sys.stderr, message
   sys.exit(1)
 
 
 def RunCommand(args, error_ok=False, error_message=None, **kwargs):
   try:
     return subprocess2.check_output(args, shell=False, **kwargs)
   except subprocess2.CalledProcessError, e:
@@ skipping 74 matching lines @@
     """Set default settings based on the gcl-style settings from the repository.
 
     The settings in the self.options object will only be set if no previous
     value exists (i.e. command line flags to the try command will override the
     settings in codereview.settings).
     """
     settings = {
       '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'),
     }
     logging.info('\n'.join(['%s: %s' % (k, v)
                             for (k, v) in settings.iteritems() if v]))
     for (k, v) in settings.iteritems():
       # Avoid overwriting options already set using command line flags.
       if v and getattr(self.options, k) is None:
         setattr(self.options, k, v)
 
@@ skipping 101 matching lines @@
 
     The files in the list should either be absolute paths or relative to the
     given root.
     """
     return scm.SVN.GenerateDiff(self.files, self.checkout_root, full_move=True,
                                 revision=self.diff_against)
 
 
 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)
     if not self.options.name:
       self.options.name = scm.GIT.GetPatchName(self.checkout_root)
     if not self.options.email:
       self.options.email = scm.GIT.GetEmail(self.checkout_root)
     if not self.diff_against:
       self.diff_against = scm.GIT.GetUpstreamBranch(self.checkout_root)
       if not self.diff_against:
@@ skipping 95 matching lines @@
 
       bot_spec.extend(_ApplyTestFilter(options.testfilter, new_style))
 
   except ImportError:
     pass
 
   return bot_spec
 
 
 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),
   ]
   if options.email:
     values.append(('email', options.email))
   if options.revision:
     values.append(('revision', options.revision))
   if options.clobber:
     values.append(('clobber', 'true'))
@@ skipping 50 matching lines @@
                                                                   str(e.args)))
   if not connection:
     raise NoTryServerAccess('%s is unaccessible.' % url)
   logging.info('Reading response...')
   response = connection.read()
   logging.info('Done')
   if response != 'OK':
     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."""
   if not options.svn_repo:
     raise NoTryServerAccess('Please use the --svn_repo option to specify the'
                             ' try server svn repository to connect to.')
 
   values = _ParseSendChangeOptions(bot_spec, options)
   description = ''.join("%s=%s\n" % (k, v) for k, v in values)
   logging.info('Sending by SVN')
   logging.info(description)
   logging.info(options.svn_repo)
   logging.info(options.diff)
   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()
-
-      # 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)
-
-      # 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)
     except subprocess2.CalledProcessError, e:
       raise NoTryServerAccess(str(e))
-  finally:
-    temp_file.close()
-    shutil.rmtree(temp_dir, True)
+
+  _PrepareDescriptionAndPatchFiles(description, options, send)
+
+
+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))
+
+  _PrepareDescriptionAndPatchFiles(description, options, send)
 
 
 def PrintSuccess(bot_spec, options):
   if not options.dry_run:
     text = 'Patch \'%s\' sent to try server' % options.name
     if bot_spec:
       text += ': %s' % ', '.join(
           '%s:%s' % (b[0], ','.join(b[1])) for b in bot_spec)
     print(text)
 
@@ skipping 95 matching lines @@
             "times to specify multiple builders. ex: "
             "'-bwin_rel:ui_tests,webkit_unit_tests -bwin_layout'. See "
             "the try server waterfall for the builders name and the tests "
             "available. Can also be used to specify gtest_filter, e.g. "
             "-bwin_rel:base_unittests:ValuesTest.*Value"))
   group.add_option("-B", "--print_bots", action="store_true",
                     help="Print bots we would use (e.g. from PRESUBMIT.py)"
                          " 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",
                     help="Force a clobber before building; e.g. don't do an "
                          "incremental build")
   # TODO(maruel): help="Select a specific configuration, usually 'debug' or "
   #                    "'release'"
   group.add_option("--target", help=optparse.SUPPRESS_HELP)
 
   group.add_option("--project",
@@ skipping 65 matching lines @@
   group.add_option("--use_svn",
                    action="store_const",
                    const=_SendChangeSVN,
                    dest="send_patch",
                    help="Use SVN to talk to the try server")
   group.add_option("-S", "--svn_repo",
                    metavar="SVN_URL",
                    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
 
 
 def TryChange(argv,
               change,
               swallow_exception,
               prog=None,
               extra_epilog=None):
   """
   Args:
@@ skipping 62 matching lines @@
     if options.files:
       file_list = options.files
     elif change:
       file_list = [f.LocalPath() for f in change.AffectedFiles()]
 
     if options.upstream_branch:
       path += '@' + options.upstream_branch
       # 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.' %
                      checkout.checkout_root)
       checkouts.append(checkout)
 
     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.
     if options.url:
       if options.files:
         parser.error('You cannot specify files and --url at the same time.')
       options.diff = urllib2.urlopen(options.url).read()
     elif options.diff:
       if options.files:
         parser.error('You cannot specify files and --diff at the same time.')
@@ skipping 57 matching lines @@
 
     if options.print_bots:
       print 'Bots which would be used:'
       for bot in bot_spec:
         if bot[1]:
           print '  %s:%s' % (bot[0], ','.join(bot[1]))
         else:
           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
     print >> sys.stderr, e
     return 1
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, e
     return 1
   return 0
 
 
 if __name__ == "__main__":
   fix_encoding.fix_encoding()
   sys.exit(TryChange(None, None, False))