add_codereview_message script to append messages to a CL

tools/add_codereview_message.py

+#!/usr/bin/python2
+
+# Copyright 2014 Google Inc.
+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Skia's Chromium DEPS Roll Message Script.
+
+This script takes a codereview URL as its argument and a (possibly
+multi-line) message on stdin.  It then calls `git cl upload` to append
+the message to the given codereview issue.
+
+Usage:
+  echo MESSAGE | %prog -c CHROMIUM_PATH CODEREVIEW_URL
+or:
+  export CHROMIUM_CHECKOUT_PATH="/path/to/chromium/checkout"
+  %prog CODEREVIEW_URL <<EOF
+  MESSAGE
+  EOF
+or:
+  %prog --help
+"""
+
+import collections
+import optparse
+import os
+import sys
+
+import git_utils
+import misc_utils
+
+
+# pylint: disable=I0011,C0103
+# Class-like object.
+DepsRollOptions = collections.namedtuple(
+    'DepsRollOptions',
+    ['chromium_path', 'skip_cl_upload', 'verbose',
+     'git_path', 'reviewers', 'cc'])
+
+
+def add_codereview_message(codereview_url, message, options):
+    """Function that does the meat of the work.

[borenet, 2014/01/30 14:36:35]

This docstring is not informative...

[hal.canary, 2014/01/30 20:11:02]

Done.

+
+    Args:
+        codereview_url: (string) we will extract the issue number from
+            this url.

[borenet, 2014/01/30 14:36:35]

Why not just take an issue number?  Asking for the full URL requires the caller
to determine which codereview server is in use, build the URL, etc, and we
aren't even using it here.

[hal.canary, 2014/01/30 20:11:02]

As the code is written, it will take either.  I'll document that.

+        message: (string) will be passed to `git cl upload -m $MESSAGE`
+        options: (object) a duck-typed object with the following fields:
+            options.skip_cl_upload (boolean): if true, will keep the
+                temporary branch around.
+            options.verbose (boolean): print out details useful for debugging.
+            options.git_path (string): git executable
+            options.reviewers (string): comma-separated list of reviewers
+            options.cc (string): comma-separated list of addresses to be
+                carbon-copied

[borenet, 2014/01/30 14:36:35]

I'd prefer having these as separate arguments. I think it's much clearer that
way (eg. you use options.chromium_path below, but it's not listed here), and it
doesn't force the caller to create an options object.

[hal.canary, 2014/01/30 20:11:02]

Done.

+    """
+    git = options.git_path
+    issue = codereview_url.strip('/').split('/')[-1]
+    vsp = misc_utils.VerboseSubprocess(options.verbose)
+    branch_name = 'issue_%s' % issue
+    upstream = 'origin/master'
+
+    with misc_utils.ChangeDir(options.chromium_path, options.verbose):
+        vsp.check_call([git, 'fetch', '-q', 'origin'])
+
+        with git_utils.ChangeGitBranch(
+            git, branch_name, upstream, options.verbose):
+            branch_name = git_utils.git_branch_name(git, options.verbose)
+
+            vsp.check_call([git, 'cl', 'patch', issue])
+            git_upload = [
+                git, 'cl', 'upload', '-t', 'bot report', '-m', message]
+            if options.cc:
+                git_upload.append('--cc=' + options.cc)
+            if options.reviewers:
+                git_upload.append('--reviewers=' + options.reviewers)
+
+            if options.skip_cl_upload:
+                space = '    '
+                print 'You should call:'
+                misc_utils.print_subprocess_args(space, ['cd', os.getcwd()])
+                misc_utils.print_subprocess_args(
+                    space, [git, 'checkout', branch_name])
+                misc_utils.print_subprocess_args(space, git_upload)
+            else:
+                vsp.check_call(git_upload)
+                print vsp.check_output([git, 'cl', 'issue'])
+
+        if not options.skip_cl_upload:
+            vsp.check_call([git, 'branch', '-D', branch_name])
+
+
+DEFAULT_REVIEWERS = ','.join([
+    'rmistry@google.com',
+    'reed@google.com',
+    'bsalomon@google.com',
+    'robertphillips@google.com',
+    ])
+
+
+DEFAULT_CC_LIST = ','.join([
+    'skia-team@google.com',
+    ])

[borenet, 2014/01/30 14:36:35]

I'd prefer these global variables declared before any functions.

[hal.canary, 2014/01/30 20:11:02]

Done.

+
+
+def main(argv):
+    """main function; see module-level docstring and GetOptionParser help.
+
+    Args:
+        argv: sys.argv[1:]-type argument list.
+    """
+    option_parser = optparse.OptionParser(usage=__doc__)
+    option_parser.add_option(
+        '-c', '--chromium_path',
+        default=os.environ.get('CHROMIUM_CHECKOUT_PATH'),
+        help='Path to local Chromium Git repository checkout,'
+        ' defaults to CHROMIUM_CHECKOUT_PATH if that environment'
+        ' variable is set.')

[borenet, 2014/01/30 14:36:35]

Personally, I think that this script could be useful outside of the DEPS roll
process and therefore would like to see this option changed to "checkout_path"
or something, and maybe default to CWD.

[hal.canary, 2014/01/30 20:11:02]

Done.

+    option_parser.add_option(
+        '', '--skip_cl_upload', action='store_true', default=False,
+        help='Skip the cl upload step; useful for testing.')
+    option_parser.add_option(
+        '', '--verbose', action='store_true', dest='verbose', default=False,
+        help='Do not suppress the output from `git cl`.',)
+    option_parser.add_option(
+        '', '--git_path', default='git',
+        help='Git executable, defaults to "git".',)

[borenet, 2014/01/30 14:36:35]

Now that we have a git_utils module, we could add a "find_git" function which
does something like:

for git in ('git', 'git.exe', 'git.bat'):
  if test_git_executable(git):
    return git
raise Exception(...)

It could even take a "hint" parameter for those who have git in some weird place
not in PATH, although I think it makes sense not to care about that case.

[hal.canary, 2014/01/30 20:11:02]

Done.  git_utils.git_executable()

+    option_parser.add_option(
+        '', '--reviewers', default=DEFAULT_REVIEWERS,
+        help=('Comma-separated list of reviewers.  Default is "%s".'
+              % DEFAULT_REVIEWERS))
+    option_parser.add_option(
+        '', '--cc', default=DEFAULT_CC_LIST,
+        help=('Comma-separated list of addresses to be carbon-copied.'
+              '  Default is "%s".' %  DEFAULT_CC_LIST))
+
+    options, arguments = option_parser.parse_args(argv)
+
+    if not options.chromium_path:
+        option_parser.error('Must specify chromium_path.')
+    if not git_utils.test_git_executable(options.git_path):
+        option_parser.error('Invalid git executable.')
+    if len(arguments) > 1:
+        option_parser.error('Extra arguments.')
+    if len(arguments) != 1:
+        option_parser.error('Missing Codereview URL.')
+
+    message = sys.stdin.read()
+    add_codereview_message(arguments[0], message, options)
+
+
+if __name__ == '__main__':
+    main(sys.argv[1:])
+