Fix R= line rewriter to handle TBRs well.

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."""
 
 from distutils.version import LooseVersion
@@ skipping 830 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."""
   R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$'

[M-A Ruel, 2013/09/12 00:25:44]

Optionally,  presubmit_support.Change.TAG_LINE_RE could be used which is the
generic form to catch all tags. That's what add_footer() uses.

[agable, 2013/09/12 17:20:03]

Considered that. But since we're very explicitly only looking for R, TBR, and
BUG lines here I decided to keep these as-is.

+  BUG_LINE = r'^[ \t]*(BUG)[ \t]*=[ \t]*(.*?)[ \t]*$'
 
   def __init__(self, description):
-    self._description = (description or '').strip()
+    self._description_lines = (description or '').strip().splitlines()
 
-  @property
-  def description(self):
-    return self._description
+  @property               # www.logilab.org/ticket/89786
+  def description(self):  # pylint: disable=E0202
+    return '\n'.join(self._description_lines)
+
+  def set_description(self, desc):
+    if isinstance(desc, basestring):
+      lines = desc.splitlines()
+    else:
+      lines = [line.rstrip() for line in desc]
+    while lines and not lines[0]:
+      lines.pop(0)
+    while lines and not lines[-1]:
+      lines.pop(-1)
+    self._description_lines = lines
 
   def update_reviewers(self, reviewers):
-    """Rewrites the R=/TBR= line(s) as a single line."""
+    """Rewrites the R=/TBR= line(s) as a single line each."""
     assert isinstance(reviewers, list), reviewers
     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():])
 
-    if is_tbr:
-      new_r_line = 'TBR=' + ', '.join(reviewers)
+    # Get the set of R= and TBR= lines and remove them from the desciption.
+    regexp = re.compile(self.R_LINE)
+    matches = [regexp.match(line) for line in self._description_lines]
+    new_desc = [self._description_lines[i] for i in range(len(matches))

[M-A Ruel, 2013/09/12 00:25:44]

this would be slightly shorter:
new_desc = [l for i, l in enumerate(self._description_lines) if not matches[i]]

[agable, 2013/09/12 17:20:03]

Done.

+                if not matches[i]]
+    self.set_description(new_desc)
+
+    # Construct new unified R= and TBR= lines.
+    r_names = []
+    tbr_names = []
+    for match in matches:
+      if match is None:

[M-A Ruel, 2013/09/12 00:25:44]

if not match:

[agable, 2013/09/12 17:20:03]

Done.

+        continue
+      is_tbr = match.group(1) == 'TBR'
+      people = cleanup_list([match.group(2).strip()])
+      if is_tbr:

[M-A Ruel, 2013/09/12 00:25:44]

if match.group(1) == 'TBR':

[agable, 2013/09/12 17:20:03]

Done.

+        tbr_names.extend(people)
+      else:
+        r_names.extend(people)
+    r_final_names = reviewers[:]

[M-A Ruel, 2013/09/12 00:25:44]

I'd prefer for you to do it at the top of the file, right after the check, so
it's more obvious that the input is not modified within the function. It's fine
to keep the variable name reviewers and not alias it.

[agable, 2013/09/12 17:20:03]

Done.

+    for name in r_names:
+      if name not in r_final_names:
+        r_final_names.append(name)
+    new_r_line = 'R=' + ', '.join(r_final_names) if r_final_names else None
+    new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else None
+
+    # Put the new lines in the desciption where the old first R= line was.

[M-A Ruel, 2013/09/12 00:25:44]

description

[agable, 2013/09/12 17:20:03]

Done.

+    line_loc = next((i for i, match in enumerate(matches) if match), -1)
+    if line_loc != -1 and line_loc < len(self._description_lines):

[M-A Ruel, 2013/09/12 00:25:44]

if 0 <= line_loc < len(self._description_lines):

[agable, 2013/09/12 17:20:03]

Done.

+      if new_tbr_line:
+        self._description_lines.insert(line_loc, new_tbr_line)
+      if new_r_line:
+        self._description_lines.insert(line_loc, new_r_line)
     else:
-      new_r_line = 'R=' + ', '.join(reviewers)
-
-    if matches:
-      self._description = (
-        self._description[:matches[0].start()] + new_r_line +
-        self._description[matches[0].end():]).strip()
-    else:
-      self.append_footer(new_r_line)
+      if new_r_line:
+        self.append_footer(new_r_line)
+      if new_tbr_line:
+        self.append_footer(new_tbr_line)
 
   def prompt(self):
     """Asks the user to update the description."""
-    self._description = (
-      '# Enter a description of the change.\n'
-      '# This will be displayed on the codereview site.\n'
-      '# The first line will also be used as the subject of the review.\n'
+    self.set_description([
+      '# Enter a description of the change.',
+      '# This will be displayed on the codereview site.',
+      '# The first line will also be used as the subject of the review.',
       '#--------------------This line is 72 characters long'
-      '--------------------\n'
-    ) + self._description
+      '--------------------',
+    ] + self._description_lines)
 
-    if '\nBUG=' not in self._description:
+    regexp = re.compile(self.BUG_LINE)
+    if not any((regexp.match(line) for line in self._description_lines)):
       self.append_footer('BUG=')
-    content = gclient_utils.RunEditor(self._description, True,
+    content = gclient_utils.RunEditor(self.description, True,
                                       git_editor=settings.GetGitEditor())
     if not content:
       DieWithError('Running editor failed')
+    lines = content.splitlines()
 
     # Strip off comments.
-    content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
-    if not content:
+    clean_lines = [line.strip() for line in lines

[M-A Ruel, 2013/09/12 00:25:44]

rstrip()

[agable, 2013/09/12 17:20:03]

Done.

+                   if not re.match(r'^#.*$', line)]

[M-A Ruel, 2013/09/12 00:25:44]

if not line.startswith('#')

[agable, 2013/09/12 17:20:03]

Done.

+    if not clean_lines:
       DieWithError('No CL description, aborting')
-    self._description = content
+    self.set_description(clean_lines)
 
   def append_footer(self, line):
-    # 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
+    if self._description_lines:
+      # Add an empty line if either the last line or the new line isn't a tag.
+      last_line = self._description_lines[-1]
+      if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
+          not presubmit_support.Change.TAG_LINE_RE.match(line)):
+        self._description_lines.append('')
+    self._description_lines.append(line)
 
   def get_reviewers(self):
     """Retrieves the list of reviewers."""
-    regexp = re.compile(self.R_LINE, re.MULTILINE)
-    reviewers = [i.group(2).strip() for i in regexp.finditer(self._description)]
+    matches = [re.match(self.R_LINE, line) for line in self._description_lines]
+    reviewers = [match.group(2).strip() for match in matches if match]
     return cleanup_list(reviewers)
 
 
 def get_approving_reviewers(props):
   """Retrieves the reviewers that approved a CL from the issue properties with
   messages.
 
   Note that the list may contain reviewers that are not committer, thus are not
   considered by the CQ.
   """
@@ skipping 1261 matching lines @@
         ('AppEngine is misbehaving and returned HTTP %d, again. Keep faith '
           'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
 
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   colorama.init()
   sys.exit(main(sys.argv[1:]))