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 contextlib
 import datetime
 import errno
 import getpass
 import itertools
 import json
 import logging
 import optparse
 import os
 import posixpath
 import re
@@ skipping 42 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')
+GIT_BRANCH_FILE = 'branch'
 
 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 225 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.
+  """
   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))
 
 
+@contextlib.contextmanager
+def _TempFilename(name, contents=None):
+  """Create a temporary directory, append the specified name and yield.
+
+  In contrast to NamedTemporaryFile, does not keep the file open.
+  Deletes the file on __exit__.
+  """
+  temp_dir = tempfile.mkdtemp(prefix=name)
+  try:
+    path = os.path.join(temp_dir, name)
+    if contents:
+      with open(path, 'w') as f:
+        f.write(contents)
+    yield path
+  finally:
+    shutil.rmtree(temp_dir, True)
+
+
+@contextlib.contextmanager
+def _PrepareDescriptionAndPatchFiles(description, options):
+  """Create temporary files with description and patch, yield execution,
+  and delete the files.
+
+  __enter__ called on the return value returns a tuple of patch_filename and
+  description_filename.
+
+  Args:
+    description: contents of description file.
+    options: patchset options object. Must have attributes: user,
+        name (of patch) and diff (contents of patch).
+  """
+  current_time = str(datetime.datetime.now()).replace(':', '.')
+  patch_basename = '%s.%s.%s.diff' % (Escape(options.user),
+                                      Escape(options.name), current_time)
+  with _TempFilename('description', description) as description_filename:
+    with _TempFilename(patch_basename, options.diff) as patch_filename:
+      yield patch_filename, description_filename
+
+
 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:
+  with _PrepareDescriptionAndPatchFiles(description, options) as (
+       patch_filename, description_filename):
+    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)
+
+
+def _GetPatchGitRepo(git_url):
+  """Get a path to a Git repo with patches with latest changes,

[agable, 2014/03/26 20:27:04]

Just a single line for the first line of a docstring.

[nodir, 2014/03/26 20:57:02]

Done.

+  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)
+  def do_git(*args, **kwargs):

[agable, 2014/03/26 20:27:04]

the name of this function doesn't clarify that it is "doing git" *inside the
patch dir*. I'd argue that this isn't necessary or helpful enough to be worth
it. Especially since you then explicitly override the cwd the first time you
call it.

[nodir, 2014/03/26 20:57:02]

Removed the function

+    kwargs.setdefault('cwd', patch_dir)
+    return scm.GIT.Capture(list(args), **kwargs)
+
+  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')
+    do_git('clone', git_url, GIT_PATCH_DIR_BASENAME, cwd=git_dir)
+    email = scm.GIT.GetEmail(cwd=os.getcwd())
+    do_git('config', '--local', 'user.email', email)
+  else:
+    if scm.GIT.IsWorkTreeDirty(patch_dir):
+      logging.info('Work dir is dirty: hard reset!')
+      do_git('reset', '--hard')
+    logging.info('Updating patch repo')
+    do_git('pull', 'origin', 'master')
+
+  return os.path.abspath(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)
+  def do_git(*args):

[agable, 2014/03/26 20:27:04]

If you're defining the same inner function multiple times, it should be defined
once on the outside. Again, I think this isn't worth it. GIT.capture is already
the right level of abstraction.

[nodir, 2014/03/26 20:57:02]

Renamed this one to patch_git. Still, trying to avoid repetition.

The reasons why do_git was in two different places were 1) it closes over
patch_dir 2) it is small

+    return scm.GIT.Capture(list(args), cwd=patch_dir)
+  def add_and_commit(filename, comment_filename):
+    do_git('add', filename)
+    do_git('commit', '-F', comment_filename)
+
+  assert scm.GIT.IsInsideWorkTree(patch_dir)
+  assert not scm.GIT.IsWorkTreeDirty(patch_dir)
+
+  with _PrepareDescriptionAndPatchFiles(description, options) as (
+       patch_filename, description_filename):
+    logging.info('Committing patch')
+    target_branch = ('refs/tryserver/' +

[agable, 2014/03/26 20:27:04]

I'd say refs/patches/, since this system could potentially be in use beyond the
tryserver.

[nodir, 2014/03/26 20:57:02]

Done.

+                     os.path.basename(patch_filename).replace(' ','-'))
+    target_filename = os.path.join(patch_dir, 'patch.diff')
+    branch_file = os.path.join(patch_dir, GIT_BRANCH_FILE)
+    try:

[agable, 2014/03/26 20:27:04]

This try is around a lot of different calls, only some of which actually touch
the network. The try/except block should have much finer granularity. For
example, a CalledProcessError raised by 'git reset' should not result in a
NoTryServerAccess error.

[nodir, 2014/03/31 20:25:13]

Done.

+      # Crete a new branch and put the patch there
+      do_git('checkout', '--orphan', target_branch)
+      do_git('reset')
+      do_git('clean', '-f')
+      shutil.copyfile(patch_filename, target_filename)
+      add_and_commit(target_filename, description_filename)
+      assert not scm.GIT.IsWorkTreeDirty(patch_dir)
+
+      # Update the branch file in the master
+      do_git('checkout', 'master')
+
+      def update_branch():
+        with open(branch_file, 'w') as f:
+          f.write(target_branch)
+        add_and_commit(branch_file, description_filename)
+      def push():
+        do_git('push', 'origin', 'master', target_branch)
+      def push_fixup():
+        do_git('fetch', 'origin')
+        do_git('reset', '--hard', 'origin/master')
+        update_branch()
+
+      update_branch()
+      logging.info('Pushing patch')
+      _retry(push, push_fixup)
+    except subprocess2.CalledProcessError, e:
+      do_git('checkout', 'master')
+      do_git('reset', '--hard', 'origin/master')
+      raise NoTryServerAccess(str(e))
+
+
+def _retry(action, fixup=None, attempts=3):

[agable, 2014/03/26 20:27:04]

There are already other retry mechanisms floating around, there's probably not a
need to actually implement a new one. And, this method is only called once, so I
would just inline it. In fact, the whole mess of inner methods in _SendChangeGit
only exist so that they can be passed to this function. Remove it and you can
streamline _SendChangeGit and make it much more straight-line code.

[nodir, 2014/03/31 20:25:13]

Done.

+  """Calls action until it passes without exceptions"""
+  for attempt in xrange(attempts):
+    try:
+      action()
+    except subprocess2.CalledProcessError:
+      is_last = attempt == attempts - 1
+      if is_last:
+        raise
+      if fixup:
+        fixup()
 
 
 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 48 matching lines @@
     match = re.match(r'^(.*)/(\d+)/?$', options.rietveld_url)
     if match:
       if options.issue or options.patchset:
         parser.error('Cannot use both --issue and use a review number url')
       options.issue = int(match.group(2))
       options.rietveld_url = match.group(1)
 
   try:
     changed_files = None
     # Always include os.getcwd() in the checkout settings.
-    checkouts = []
     path = os.getcwd()
 
     file_list = []
     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()
+    options = current_vcs.options
+    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:
+      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 = [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))