Fix R= line rewriter to handle TBRs well.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index dc351a2123a9f1bb29c2c7cf95806e7b4dcf7d1e..fafd44cb0ff4316aa03b3a2b3f861efe85f05c61 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -849,80 +849,110 @@ def GetCodereviewSettingsInteractively():
 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)
-    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()
+    # 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:
-      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)