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')
 
 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

[ghost stip (do not use), 2014/03/26 19:16:36]

nit: Capitalize comments and end with a period.

+      '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 104 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 49 matching lines @@
       raise NoTryServerAccess('%s is unaccessible. Reason: %s' % (url,
                                                                   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.

[M-A Ruel, 2014/03/26 12:57:10]

Imperative verb tense means "Creates" instead of "Create".

[nodir, 2014/03/26 17:18:17]

A lot of methods in this file uses non-imperative tense. I will update them all
in another CL

+
+  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,

[ghost stip (do not use), 2014/03/26 19:16:36]

nit: """One line with short desciption < 80 chars.

More lines.
"""

+  and delete the files.
+
+

[M-A Ruel, 2014/03/26 12:57:10]

remove one line

[nodir, 2014/03/26 17:18:17]

Done.

+  __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":

[M-A Ruel, 2014/03/26 12:57:10]

This file uses single quotes, please keep it consistent.

[nodir, 2014/03/26 17:18:17]

There is a lot of double quotes in this file. I will update them all in a
separate CL.

+      # 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 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)

[agable, 2014/03/26 17:15:10]

The patch-specific refs are going to be non-branch refs (i.e.
refs/patches/<unique id>), so this should be okay. If you decide to put each
patch on its own branch (i.e. refs/heads/<unique id>) then the clone is a bad
idea.

+    email = scm.GIT.GetEmail(cwd=os.getcwd())
+    scm.GIT.Capture(['config', '--local', 'user.email', email], patch_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)

[M-A Ruel, 2014/03/26 12:57:10]

One issue I see here is that do you want the user to pull everyone else patches.
So you should have a branch with the empty commit to fetch.

[agable, 2014/03/26 17:15:10]

I think the plan is for origin/master to contain a single file that gets updated
once per patch, and for each patch to live on its own ref that isn't in the
default fetch refspec.

[nodir, 2014/03/26 17:18:17]

I have a CL locally that does something similar:

trychange.py creates a new branch with a unique name, puts one file there
"patch.diff". In order for GitPoller to notice the change, it also updates
special file "branch" in the master, so the buildbot master sees the new
patches. The "branch" file contains the name of the branch. As a result, the
developer machine never pulls other's patches.

The buidlbot master listens to changes in the master branch. For each new
change, it reads the branch file content at a certain commit, creates a new
build request and specifies the patch branch name in properties. As a result,
the buildbot master never pulls patches.

A slave reads the "branch" property and pulls only that branch, reads the
patch.diff and applies it. The slaves machine don't pull other's patches.

The problem with this approach that I faced yesterday, is that Gerrit didn't
allot me to push new branches.

+
+  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)
+
+  with _PrepareDescriptionAndPatchFiles(description, options) as (
+       patch_filename, description_filename):
+    logging.info('Committing patch')
+    target_filename = os.path.abspath(
+        os.path.join(patch_dir, os.path.basename(patch_filename)))
+    shutil.copyfile(patch_filename, target_filename)
+    try:

[agable, 2014/03/26 17:15:10]

This whole block needs to be changed to commit the patch on a new unique ref.

+      scm.GIT.Capture(['add', target_filename], cwd=patch_dir)
+      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', 'master'], cwd=patch_dir)

[M-A Ruel, 2014/03/26 12:57:10]

This is problematic, since it creates a race condition when two devs trigger try
jobs simultaneously. Each one should create a unique (or user name based) branch
so it doesn't interfere with others.

[nodir, 2014/03/26 17:18:17]

See above.

+    except subprocess2.CalledProcessError, e:
+      scm.GIT.Capture(['reset', '--hard', 'origin/master'], cwd=patch_dir)
+      raise NoTryServerAccess(str(e))
+
 
 
 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))