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..3ba88c3efbdc3739bc3a5ab0fe1389258d33beb5 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -849,80 +849,99 @@ def GetCodereviewSettingsInteractively():
 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 = (description or '').strip().splitlines()

[M-A Ruel, 2013/08/23 18:53:39]

Rename the variable to be sure.
self._description_lines

[agable, 2013/08/26 17:16:45]

Done.

 
   @property
   def description(self):
-    return self._description
+    return '\n'.join(self._description).strip()

[M-A Ruel, 2013/08/23 18:53:39]

I'd prefer to not strip here and store it stripped for consistency.

[agable, 2013/08/26 17:16:45]

Done.

 
   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]
+    new_desc = [self._description[i] for i in range(len(matches))
+                if not matches[i]]
+    self._description = new_desc

[M-A Ruel, 2013/08/23 18:53:39]

You also want to strip off ending lines, e.g.
while self._description and not self._description[-1]:
  self._description.pop()

[agable, 2013/08/26 17:16:45]

Done, via a properties.setter that handles all stripping every time we write
directly to self.description.

+
+    # 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 += people

[M-A Ruel, 2013/08/23 18:53:39]

I prefer using explicitly .append() or .extend(). Otherwise it occasionally
create hard-to-figure out bugs.

[agable, 2013/08/26 17:16:45]

Done.

+      else:
+        r_names += people
+    for name in r_names:

[M-A Ruel, 2013/08/23 18:53:39]

Using a set() for reviewers would make this trivial.
Also, I'd prefer for this code to not modify its input. It can create
hard-to-track bugs.

[agable, 2013/08/26 17:16:45]

Yes, but I'd like to keep the reviewers ordered (first the cmd-line reviewers,
in order, then everyone else from the description, in order) because people
often encode priority in reviewer orders.
Done.

+      if name not in reviewers:
+        reviewers.append(name)
+    new_r_line = 'R=' + ', '.join(reviewers) if reviewers else None
+    new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else None

[M-A Ruel, 2013/08/23 18:53:39]

This one is a problem, so people put "TBR=" on their CL description, it should
not be removed on commit. This should be unit tested.

[agable, 2013/08/26 17:16:45]

Not sure what you mean. If someone puts "TBR=" in their CL description (with no
one listed after the =), what are we supposed to do with that? It certainly
shouldn't count as valid review. I think that TAG=VALUE lines with no VALUE
should be removed.

+
+    # 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:
+      if new_tbr_line:
+        self._description.insert(line_loc, new_tbr_line)
+      if new_r_line:
+        self._description.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._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
 
-    if '\nBUG=' not in self._description:
+    regexp = re.compile(self.BUG_LINE)
+    if not any((regexp.match(line) for line in self._description)):
       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')
+    content = content.strip().splitlines()

[M-A Ruel, 2013/08/23 18:53:39]

lines =

[agable, 2013/08/26 17:16:45]

Done.

 
     # Strip off comments.
-    content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
+    content = [line for line in content if not re.match(r'^#.*$', line)]
     if not content:
       DieWithError('No CL description, aborting')
     self._description = content
 
   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 not self._description:

[M-A Ruel, 2013/08/23 18:53:39]

if self._description:
  <lines 933-937>

[agable, 2013/08/26 17:16:45]

Done.

+      self._description = ['']
+    # Add an empty line if either the last line or the new line isn't a tag.
+    last_line = self._description[-1]
+    if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
+        not presubmit_support.Change.TAG_LINE_RE.match(line)):
+      self._description.append('')
+    self._description.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)]
+    regexp = re.compile(self.R_LINE)
+    matches = [regexp.match(line) for line in self._description]

[M-A Ruel, 2013/08/23 18:53:39]

it's fine to use re.match(self.R_LINE, line). The re module keeps a cache, so
when you iterate over the same re, it's fast well enough for this use case.

[agable, 2013/08/26 17:16:45]

Done. Was just following the same pattern as REs elsewhere in this class.

+    reviewers = [match.group(2).strip() for match in matches if match]
     return cleanup_list(reviewers)