Switch ChangeDescription usage to be stricter.

git_cl.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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld."""
 
 import json
@@ skipping 36 matching lines @@
 
 
 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:
+  except subprocess2.CalledProcessError as e:
+    logging.debug('Failed running %s', args)
     if not error_ok:
       DieWithError(
           'Command "%s" failed.\n%s' % (
             ' '.join(args), error_message or e.stdout or ''))
     return e.stdout
 
 
 def RunGit(args, **kwargs):
   """Returns stdout."""
   return RunCommand(['git'] + args, **kwargs)
@@ skipping 722 matching lines @@
   SetProperty(settings.GetTreeStatusUrl(error_ok=True), 'Tree status URL',
               'tree-status-url', False)
   SetProperty(settings.GetViewVCUrl(), 'ViewVC URL', 'viewvc-url', True)
 
   # TODO: configure a default branch to diff against, rather than this
   # svn-based hackery.
 
 
 class ChangeDescription(object):
   """Contains a parsed form of the change description."""
-  def __init__(self, log_desc, reviewers):
-    self.log_desc = log_desc
-    self.reviewers = reviewers
-    self.description = self.log_desc
+  R_LINE = r'^\s*(TBR|R)\s*=\s*(.+)\s*$'
 
-  def Prompt(self):
-    content = """# Enter a description of the change.
-# This will displayed on the codereview site.
-# The first line will also be used as the subject of the review.
-"""
-    content += self.description
-    if ('\nR=' not in self.description and
-        '\nTBR=' not in self.description and
-        self.reviewers):
-      content += '\nR=' + self.reviewers
+  def __init__(self, description):
+    self._description = (description or '').strip()
+
+  @property
+  def description(self):
+    return self._description
+
+  def update_reviewers(self, reviewers):

[iannucci, 2013/04/08 22:49:40]

Can we make reviewers a list of reviewers instead of expecting the
comma-delimited string? That will make it easier to manipulate and makes this
interface more logical.

[Dirk Pranke, 2013/04/09 00:07:52]

I don't think we can easily do this without pushing logic into the client that
we don't want to be there.

An alternative might be to provide static methods that did the converstion
(splitting/joining).

If we did make reviewers a list internally, we'd also want to split it out as a
separate member variable distinct from the free-text part of the description.

There is a certain self-enforcing consistency with using self._description for
all of the fields, at least.

[M-A Ruel, 2013/04/11 00:01:38]

So reviewers is now a list all the time.

+    """Rewrites the R=/TBR= line(s) as a single line."""
+    if not reviewers:
+      return
+    regexp = re.compile(self.R_LINE, re.MULTILINE)
+    matches = list(regexp.finditer(self.description))
+    is_tbr = any(m.group(1) == 'TBR' for m in matches)
+    if len(matches) > 1:
+      # Erase all except the first one.
+      for i in xrange(len(matches) - 1, 0, -1):
+        self._description = (
+            self.description[:matches[i].start()] +
+            self.description[matches[i].end()+1:])

[Dirk Pranke, 2013/04/09 00:07:52]

switching back and forth between self._description and self.description is
confusing. You should use one or the other consistently.

[M-A Ruel, 2013/04/11 00:01:38]

Done.

+
+    if is_tbr:
+      new_r_line = 'TBR=' + reviewers
+    else:
+      new_r_line = 'R=' + reviewers
+
+    if matches:
+      self._description = (
+        self.description[:matches[0].start()] + new_r_line +
+        self.description[matches[0].end()+1:])
+    else:
+      self.append_line(new_r_line)
+
+  def prompt(self):
+    """Asks the user to update the description."""
+    self._description = (
+      '# Enter a description of the change.\n'
+      '# This will displayed on the codereview site.\n'
+      '# The first line will also be used as the subject of the review.\n'
+    ) + self.description
+
     if '\nBUG=' not in self.description:
-      content += '\nBUG='
-    content = content.rstrip('\n') + '\n'
-    content = gclient_utils.RunEditor(content, True)
+      self.append_line('BUG=')
+    content = gclient_utils.RunEditor(self.description, True)
     if not content:
       DieWithError('Running editor failed')
+
+    # Strip off comments.
     content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
-    if not content.strip():
+    if not content:
       DieWithError('No CL description, aborting')
-    self.description = content
+    self._description = content
 
-  def ParseDescription(self):
-    """Updates the list of reviewers and subject from the description."""
-    self.description = self.description.strip('\n') + '\n'
-    # Retrieves all reviewer lines
-    regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE)
-    reviewers = ','.join(
-        i.group(2).strip() for i in regexp.finditer(self.description))
-    if reviewers:
-      self.reviewers = reviewers
+  def append_line(self, line):

[Dirk Pranke, 2013/04/09 00:07:52]

I would probably call this something like add_keyword, since (I think?) we don't
want this to be used to generically append lines, right?

+    # Adds a LF if the last line did not have 'FOO=BAR' or if the new one isn't.
+    if self.description:
+      if '\n' not in self.description:
+        self._description += '\n'
+      else:
+        last_line = self.description.rsplit('\n', 1)[1]
+        if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
+            not presubmit_support.Change.TAG_LINE_RE.match(line)):
+          self._description += '\n'
+    self._description += '\n' + line
 
-  def IsEmpty(self):
-    return not self.description
+  def get_reviewers(self):
+    """Retrieves the list of reviewers."""
+    regexp = re.compile(self.R_LINE, re.MULTILINE)
+    reviewers = sum(
+        (i.group(2).split(',') for i in regexp.finditer(self.description)),
+        [])
+    reviewers = (i.strip() for i in reviewers)
+    return sorted(i for i in reviewers if i)
 
 
 def FindCodereviewSettingsFile(filename='codereview.settings'):
   """Finds the given file starting in the cwd and going up.
 
   Only looks up to the top of the repository unless an
   'inherit-review-settings-ok' file exists in the root of the repository.
   """
   inherit_ok_file = 'inherit-review-settings-ok'
   cwd = os.getcwd()
@@ skipping 247 matching lines @@
 
 def GerritUpload(options, args, cl):
   """upload the current branch to gerrit."""
   # We assume the remote called "origin" is the one we want.
   # It is probably not worthwhile to support different workflows.
   remote = 'origin'
   branch = 'master'
   if options.target_branch:
     branch = options.target_branch
 
-  log_desc = options.message or CreateDescriptionFromLog(args)
-  if CHANGE_ID not in log_desc:
+  change_desc = ChangeDescription(
+      options.message or CreateDescriptionFromLog(args))
+  if not change_desc.description:
+    print "Description is empty; aborting."
+    return 1
+  if CHANGE_ID not in change_desc.description:

[Dirk Pranke, 2013/04/09 00:07:52]

I wonder if we should restructure this so that we add the change id during the
constructor (optionally?)

[M-A Ruel, 2013/04/11 00:01:38]

I prefer to keep this out of this CL since it invokes testing with repo style
hooks and there is exactly 0 unit test covering this use case.

     AddChangeIdToCommitMessage(options, args)
   if options.reviewers:
-    log_desc += '\nR=' + options.reviewers
-  change_desc = ChangeDescription(log_desc, options.reviewers)
-  change_desc.ParseDescription()
-  if change_desc.IsEmpty():
-    print "Description is empty; aborting."
-    return 1
+    change_desc.update_reviewers(options.reviewers)
 
   receive_options = []
   cc = cl.GetCCList().split(',')
   if options.cc:
     cc += options.cc.split(',')
   cc = filter(None, cc)
   if cc:
     receive_options += ['--cc=' + email for email in cc]
-  if change_desc.reviewers:
-    reviewers = filter(None, change_desc.reviewers.split(','))
-    if reviewers:
-      receive_options += ['--reviewer=' + email for email in reviewers]
+  if change_desc.get_reviewers():
+    receive_options.extend(
+        '--reviewer=' + email for email in change_desc.get_reviewers())
 
   git_command = ['push']
   if receive_options:
     git_command.append('--receive-pack=git receive-pack %s' %
                        ' '.join(receive_options))
   git_command += [remote, 'HEAD:refs/for/' + branch]
   RunGit(git_command)
   # TODO(ukai): parse Change-Id: and set issue number?
   return 0
 
@@ skipping 15 matching lines @@
       # for upload.py.  Soon this will be changed to set the --message option.
       # Will wait until people are used to typing -t instead of -m.
       upload_args.extend(['--title', options.message])
     upload_args.extend(['--issue', str(cl.GetIssue())])
     print ("This branch is associated with issue %s. "
            "Adding patch to that issue." % cl.GetIssue())
   else:
     if options.title:
       upload_args.extend(['--title', options.title])
     message = options.title or options.message or CreateDescriptionFromLog(args)
-    change_desc = ChangeDescription(message, options.reviewers)
+    change_desc = ChangeDescription(message)
+    if options.reviewers:
+      change_desc.update_reviewers(options.reviewers)
     if not options.force:
-      change_desc.Prompt()
-    change_desc.ParseDescription()
+      change_desc.prompt()
 
-    if change_desc.IsEmpty():
+    if not change_desc.description:
       print "Description is empty; aborting."
       return 1
 
     upload_args.extend(['--message', change_desc.description])
-    if change_desc.reviewers:
-      upload_args.extend(['--reviewers', change_desc.reviewers])
+    if change_desc.get_reviewers():
+      upload_args.append('--reviewers=' + ','.join(change_desc.get_reviewers()))
     if options.send_mail:
-      if not change_desc.reviewers:
+      if not change_desc.get_reviewers():
         DieWithError("Must specify reviewers to send email.")
       upload_args.append('--send_mail')
     cc = ','.join(filter(None, (cl.GetCCList(), options.cc)))
     if cc:
       upload_args.extend(['--cc', cc])
 
   upload_args.extend(['--git_similarity', str(options.similarity)])
   if not options.find_copies:
     upload_args.extend(['--git_no_find_copies'])
 
@@ skipping 223 matching lines @@
       elif 'unknown' == status:
         print('Unable to determine tree status.  Please verify manually and '
               'use "git cl dcommit --bypass-hooks" to commit on a closed tree.')
   else:
     breakpad.SendStack(
         'GitClHooksBypassedCommit',
         'Issue %s/%s bypassed hook when committing' %
         (cl.GetRietveldServer(), cl.GetIssue()),
         verbose=False)
 
-  description = options.message
-  if not description and cl.GetIssue():
-    description = cl.GetDescription()
+  change_desc = ChangeDescription(options.message)
+  if not change_desc.description and cl.GetIssue():
+    change_desc = ChangeDescription(cl.GetDescription())
 
-  if not description:
+  if not change_desc.description:
     if not cl.GetIssue() and options.bypass_hooks:
-      description = CreateDescriptionFromLog([base_branch])
+      change_desc = ChangeDescription(CreateDescriptionFromLog([base_branch]))
     else:
       print 'No description set.'
       print 'Visit %s/edit to set it.' % (cl.GetIssueURL())
       return 1
 
+  # Keep a separate copy for the commit message.

[Dirk Pranke, 2013/04/09 00:07:52]

Can you clarify *why* we need a separate copy here?

[M-A Ruel, 2013/04/11 00:01:38]

Done.

+  commit_desc = ChangeDescription(change_desc.description)
   if cl.GetIssue():
-    description += "\n\nReview URL: %s" % cl.GetIssueURL()
+    commit_desc.append_line('Review URL: %s' % cl.GetIssueURL())
+  if options.contributor:
+    commit_desc.append_line('Patch from %s.' % options.contributor)
 
-  if options.contributor:
-    description += "\nPatch from %s." % options.contributor
-  print 'Description:', repr(description)
+  print 'Description:', repr(commit_desc.description)
 
   branches = [base_branch, cl.GetBranchRef()]
   if not options.force:
     print_stats(options.similarity, options.find_copies, branches)
     ask_for_data('About to commit; enter to confirm.')
 
   # We want to squash all this branch's commits into one commit with the proper
   # description. We do this by doing a "reset --soft" to the base branch (which
   # keeps the working copy the same), then dcommitting that.  If origin/master
   # has a submodule merge commit, we'll also need to cherry-pick the squashed
@@ skipping 15 matching lines @@
     os.chdir(rel_base_path)
 
   # Stuff our change into the merge branch.
   # We wrap in a try...finally block so if anything goes wrong,
   # we clean up the branches.
   retcode = -1
   try:
     RunGit(['checkout', '-q', '-b', MERGE_BRANCH])
     RunGit(['reset', '--soft', base_branch])
     if options.contributor:
-      RunGit(['commit', '--author', options.contributor, '-m', description])
+      RunGit(
+          [
+            'commit', '--author', options.contributor,
+            '-m', commit_desc.description,
+          ])
     else:
-      RunGit(['commit', '-m', description])
+      RunGit(['commit', '-m', commit_desc.description])
     if base_has_submodules:
       cherry_pick_commit = RunGit(['rev-list', 'HEAD^!']).rstrip()
       RunGit(['branch', CHERRY_PICK_BRANCH, svn_head])
       RunGit(['checkout', CHERRY_PICK_BRANCH])
       RunGit(['cherry-pick', cherry_pick_commit])
     if cmd == 'push':
       # push the merge branch.
       remote, branch = cl.FetchUpstreamTuple(cl.GetBranch())
       retcode, output = RunGitWithCode(
           ['push', '--porcelain', remote, 'HEAD:%s' % branch])
@@ skipping 18 matching lines @@
                for l in output.splitlines(False))
       match = filter(None, match)
       if len(match) != 1:
         DieWithError("Couldn't parse ouput to extract the committed hash:\n%s" %
             output)
       revision = match[0].group(2)
     else:
       return 1
     viewvc_url = settings.GetViewVCUrl()
     if viewvc_url and revision:
-      cl.description += ('\n\nCommitted: ' + viewvc_url + revision)
+      change_desc.append_line('Committed: ' + viewvc_url + revision)
     elif revision:
-      cl.description += ('\n\nCommitted: ' + revision)
+      change_desc.append_line('Committed: ' + revision)
     print ('Closing issue '
            '(you may be prompted for your codereview password)...')
-    cl.UpdateDescription(cl.description)
+    cl.UpdateDescription(change_desc.description)
     cl.CloseIssue()
     props = cl.RpcServer().get_issue_properties(cl.GetIssue(), False)
     patch_num = len(props['patchsets'])
     comment = "Committed patchset #%d manually as r%s" % (patch_num, revision)
     comment += ' (presubmit successful).' if not options.bypass_hooks else '.'
     cl.RpcServer().add_comment(cl.GetIssue(), comment)
     cl.SetIssue(0)
 
   if retcode == 0:
     hook = POSTUPSTREAM_HOOK_PATTERN % cmd
@@ skipping 376 matching lines @@
              'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
   # Not a known command. Default to help.
   GenUsage(parser, 'help')
   return CMDhelp(parser, argv)
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   sys.exit(main(sys.argv[1:]))