Now trychange can store patches in a Git repo

trychange.py

Index: trychange.py
diff --git a/trychange.py b/trychange.py
index f93d9a418778fd11c5cc52ac660952941ac88297..2a90a8c726b43c2b34c22351f75c4b07cd891480 100755
--- a/trychange.py
+++ b/trychange.py
@@ -4,10 +4,12 @@
 # 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
@@ -70,6 +72,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 +167,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

[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'),
     }
@@ -286,6 +292,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)
@@ -410,7 +417,9 @@ 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.
+  """
   values = [
     ('user', options.user),
     ('name', options.name),
@@ -480,6 +489,45 @@ def _SendChangeHTTP(bot_spec, options):
   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
@@ -497,45 +545,107 @@ 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:
-    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')
+  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:
       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):
@@ -651,7 +761,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",
@@ -737,6 +849,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
 
 
@@ -805,7 +929,6 @@ def TryChange(argv,
   try:
     changed_files = None
     # Always include os.getcwd() in the checkout settings.
-    checkouts = []
     path = os.getcwd()
 
     file_list = []
@@ -819,12 +942,29 @@ 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()
+    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.' %
@@ -833,9 +973,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.
@@ -913,23 +1054,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