trychange.py: Gerrit protocol

trychange.py

Index: trychange.py
diff --git a/trychange.py b/trychange.py
index d181e141baff4c312c8fd1c8343fc98c8918991c..b9d43e1802d1f79aca75b3f7b423293132937f96 100755
--- a/trychange.py
+++ b/trychange.py
@@ -24,12 +24,14 @@ import sys
 import tempfile
 import urllib
 import urllib2
+import urlparse
 
 import breakpad  # pylint: disable=W0611
 
-import gcl
 import fix_encoding
+import gcl
 import gclient_utils
+import gerrit_util
 import scm
 import subprocess2
 
@@ -95,17 +97,21 @@ def RunGit(args, **kwargs):
   """Returns stdout."""
   return RunCommand(['git'] + args, **kwargs)
 
+class Error(Exception):
+  """An error during a try job submission.
+
+  For this error, trychange.py does not display stack trace, only message
+  """
 
-class InvalidScript(Exception):
+class InvalidScript(Error):
   def __str__(self):
     return self.args[0] + '\n' + HELP_STRING
 
 
-class NoTryServerAccess(Exception):
+class NoTryServerAccess(Error):
   def __str__(self):
     return self.args[0] + '\n' + HELP_STRING
 
-
 def Escape(name):
   """Escapes characters that could interfere with the file system or try job
   parsing.
@@ -168,6 +174,7 @@ class SCM(object):
       'port': self.GetCodeReviewSetting('TRYSERVER_HTTP_PORT'),
       'host': self.GetCodeReviewSetting('TRYSERVER_HTTP_HOST'),
       'svn_repo': self.GetCodeReviewSetting('TRYSERVER_SVN_URL'),
+      'gerrit_url': self.GetCodeReviewSetting('TRYSERVER_GERRIT_URL'),
       'git_repo': self.GetCodeReviewSetting('TRYSERVER_GIT_URL'),
       'project': self.GetCodeReviewSetting('TRYSERVER_PROJECT'),
       # Primarily for revision=auto
@@ -706,6 +713,73 @@ def _SendChangeGit(bot_spec, options):
       patch_git('reset', '--hard', 'origin/master')
       raise
 
+def _SendChangeGerrit(bot_spec, options):
+  """Posts a try job to a Gerrit change.
+
+  Reads Change-Id from the HEAD commit, resolves the current revision, checks
+  that local revision matches the uploaded one, posts a try job in form of a
+  message, sets Tryjob label to 1.
+
+  Gerrit message format: starts with !tryjob, optionally followed by a tryjob
+  definition in JSON format:
+      buildNames: list of strings specifying build names.
+  """
+
+  logging.info('Sending by Gerrit')
+  if not options.gerrit_url:
+    raise NoTryServerAccess('Please use --gerrit_url option to specify the '
+                            'Gerrit instance url to connect to')
+  gerrit_host = urlparse.urlparse(options.gerrit_url).hostname
+  logging.debug('Gerrit host: %s' % gerrit_host)
+
+  def get_change_id():

[Vadim Sh., 2014/04/30 00:40:48]

looks like this file is using GetChangeId naming style

+    """Finds Change-ID of the HEAD commit."""
+    CHANGE_ID_RGX = '^Change-Id: (I[a-f0-9]{10,})'
+    comment = scm.GIT.Capture(['log', '-1', '--format=%b'], cwd=os.getcwd())
+    change_id_match = re.search(CHANGE_ID_RGX, comment, re.I | re.M)
+    if not change_id_match:
+      raise Error('Change-Id was not found in the HEAD commit.')

[Vadim Sh., 2014/04/30 00:40:48]

It would be helpful to explain what to do in that case. For example explain that
this try mechanism works only Gerrit projects that have commit hooks in place. 

+    change_id = change_id_match.group(1)
+    assert change_id and change_id.startswith('I')

[Vadim Sh., 2014/04/30 00:40:48]

I think this assert is redundant since your regexp is pretty strict already.

+    return change_id
+
+  def format_message():
+    # Build job definition.
+    job_def = {}
+    builderNames = [builder for builder, _ in bot_spec]
+    if builderNames:
+      job_def['builderNames'] = builderNames
+
+    # Format message.
+    msg = '!tryjob'
+    if job_def:
+      msg = '%s %s' % (msg, json.dumps(job_def))

[Vadim Sh., 2014/04/30 00:40:48]

json.dumps(job_def, sort_keys=True)

+    return msg

[Vadim Sh., 2014/04/30 00:40:48]

Does Gerrit append '\n' for you here? I think it's better to explicitly append
it, since your regexp on buildbot side expects it.

[nodir, 2014/04/30 18:57:27]

The regexp expects EOL or EOF  after job definition. I've tested that:

re.search('!tryjob(.*)$', 'aaa\n!tryjob xxx', re.I).group() == '!tryjob xxx'

+
+  head_sha = scm.GIT.Capture(['log', '-1', '--format=%H'],  cwd=os.getcwd())
+
+  change_id = get_change_id()

[Vadim Sh., 2014/04/30 00:40:48]

I think it would be cleaner to pass |head_sha| to get_change_id() and extract
Change-Id from |head_sha| commit, rather then requery the history again.

[nodir, 2014/04/30 18:57:27]

Done.

+  assert change_id

[Vadim Sh., 2014/04/30 00:40:48]

I think this is redundant too..

+
+  # Check that the uploaded revision matches the local one.
+  changes = gerrit_util.GetChangeCurrentRevision(gerrit_host, change_id)
+  assert len(changes) in (0, 1), 'Multiple changes with id %s' % change_id

[Vadim Sh., 2014/04/30 00:40:48]

hm.. len(changes) <= 1?

[nodir, 2014/04/30 18:57:27]

Wanted to be more explicit, it's probably looks weird

+  if not changes:
+    raise Error('A change %s was not found on the server. Was it uploaded?' %
+                change_id)
+  logging.debug('Found Gerrit change: %s' % changes[0])
+  if changes[0]['current_revision'] != head_sha:
+    raise Error('Some changes were made since last upload to Gerrit, '

[Vadim Sh., 2014/04/30 00:40:48]

I'd rephrase it somehow. Maybe "Please reupload your latest changes to Gerrit"
or something like that. English is hard :(

You should also probably check that index and working directory are clean? I.e.
both 'git diff' and 'git diff --cached' return empty strings.

[nodir, 2014/04/30 18:57:27]

Modified the string a bit.
This is done by other part of trychange.py

+                'so stopping.')
+
+  # Post a try job.
+  message = format_message()
+  logging.info('Posting gerrit message: %s' % message)
+  if not options.dry_run:
+    # Post a message and set TryJob=1 label

[Vadim Sh., 2014/04/30 00:40:48]

nit: append '.' at the end of a sentence :)

+    gerrit_util.SetReview(gerrit_host, change_id, msg=message,
+                          labels={'Tryjob': 1})

[Vadim Sh., 2014/04/30 00:40:48]

What happens here if project doesn't have Tryjob label configured?

[nodir, 2014/04/30 18:57:27]

It will report:

Bad Request: label "Tryjb" is not a configured label

+
 
 def PrintSuccess(bot_spec, options):
   if not options.dry_run:
@@ -920,6 +994,19 @@ def gen_parser(prog):
                    help="GIT url to use to write the changes in; --use_git is "
                         "implied when using --git_repo")
   parser.add_option_group(group)
+
+  group = optparse.OptionGroup(parser, "Access the try server with Gerrit")
+  group.add_option("--use_gerrit",
+                   action="store_const",
+                   const=_SendChangeGerrit,
+                   dest="send_patch",
+                   help="Use Gerrit to talk to the try server")
+  group.add_option("--gerrit_url",
+                   metavar="GERRIT_URL",
+                   help="Gerrit url to post a tryjob to; --use_gerrit is "
+                        "implied when using --gerrit_url")
+  parser.add_option_group(group)
+
   return parser
 
 
@@ -1033,9 +1120,11 @@ def TryChange(argv,
     can_http = options.port and options.host
     can_svn = options.svn_repo
     can_git = options.git_repo
+    can_gerrit = options.gerrit_url
+    can_anything = can_http or can_svn or can_git or can_gerrit

[Vadim Sh., 2014/04/30 00:40:48]

I'm far from english expert, but shouldn't it be "can_something" ? :)

[nodir, 2014/04/30 18:57:27]

Done

     # 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 or can_git):
+    if not options.send_patch and not can_anything:
       parser.error('Please specify an access method.')
 
     # Convert options.diff into the content of the diff.
@@ -1122,7 +1211,8 @@ def TryChange(argv,
       all_senders = [
         (_SendChangeHTTP, can_http),
         (_SendChangeSVN, can_svn),
-        (_SendChangeGit, can_git)
+        (_SendChangeGerrit, can_gerrit),
+        (_SendChangeGit, can_git),
       ]
       senders = [sender for sender, can in all_senders if can]
 
@@ -1137,7 +1227,7 @@ def TryChange(argv,
         if is_last:
           raise
     assert False, "Unreachable code"
-  except (InvalidScript, NoTryServerAccess), e:
+  except Error, e:
     if swallow_exception:
       return 1
     print >> sys.stderr, e