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 831 matching lines @@
               '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]*$'
+  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)
+
+  @description.setter
+  def description(self, desc):  # pylint: disable=E0202

[M-A Ruel, 2013/08/30 15:52:36]

I must say I'm not a fan of keeping a setter here, it makes understand
foo.description = <something> much harder to understand and reduce the general
maintainability of this file. If someone wants to update the lines,
._description_lines is there.

Another option is an explicit set_description() member method.

[agable, 2013/09/11 21:08:05]

Yeah, you're right that the setter is confusing. I went the set_description()
route, as it is very clear and I wanted to keep the functionality around.

+    if isinstance(desc, basestring):
+      lines = desc.strip().splitlines()

[M-A Ruel, 2013/08/30 15:52:36]

.strip() is not necessary because of lines 867-870.

[agable, 2013/09/11 21:08:05]

Done.

+    else:
+      lines = [line.strip() for line in desc]

[M-A Ruel, 2013/08/30 15:52:36]

That's wrong, you are left-aligning everything.

[agable, 2013/09/11 21:08:05]

Done.

+    while lines and not lines[0]:
+      lines = lines[1:]

[M-A Ruel, 2013/08/30 15:52:36]

lines.pop(0)
will just the same faster.

[agable, 2013/09/11 21:08:05]

Done.

+    while lines and not lines[-1]:
+      lines = lines.pop()

[M-A Ruel, 2013/08/30 15:52:36]

That's wrong since you are keeping the last line; you want:
lines.pop()

[agable, 2013/09/11 21:08:05]

Done.

+    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))
+                if not matches[i]]
+    self.description = new_desc
+
+    # Construct new unified R= and TBR= lines.
+    r_names = []
+    tbr_names = []
+    for match in matches:
+      if match is None:
+        continue
+      is_tbr = match.group(1) == 'TBR'
+      people = cleanup_list([match.group(2).strip()])
+      if is_tbr:
+        tbr_names.extend(people)
+      else:
+        r_names.extend(people)
+    r_final_names = reviewers[:]
+    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.
+    line_loc = next((i for i, match in enumerate(matches) if match), -1)
+    if line_loc != -1 and line_loc < len(self._description_lines):
+      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.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
+                   if not re.match(r'^#.*$', line)]
+    if not clean_lines:
       DieWithError('No CL description, aborting')
-    self._description = content
+    self.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:]))