git cl description: avoid appending BUG= after Change-Id.

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 and Gerrit."""
 
 from distutils.version import LooseVersion
@@ skipping 2579 matching lines @@
   elif options.rietveld:
     options.forced_codereview = 'rietveld'
 
 
 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_lines = (description or '').strip().splitlines()

[tandrii(chromium), 2016/06/03 09:34:59]

see this?

[Sergiy Byelozyorov, 2016/06/03 12:35:16]

Acknowledged.

 
   @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]
@@ skipping 74 matching lines @@
       DieWithError('Running editor failed')
     lines = content.splitlines()
 
     # Strip off comments.
     clean_lines = [line.rstrip() for line in lines if not line.startswith('#')]
     if not clean_lines:
       DieWithError('No CL description, aborting')
     self.set_description(clean_lines)
 
   def append_footer(self, 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)
+    """Adds a footer line to the description.
+
+    Differentiates legacy "KEY=xxx" footers (used to be called tags) and
+    Gerrit's footers in the form of "Footer-Key: footer any value" and ensures
+    that Gerrit footers are always at the end.
+    """
+    parsed_footer_line = git_footers.parse_footer(line)
+    if parsed_footer_line:
+      # Line is a gerrit footer in the form: Footer-Key: any value.
+      # Thus, must be appended observing Gerrit footer rules.
+      self.set_description(
+          git_footers.add_footer(self.description,
+                                 key=parsed_footer_line[0],
+                                 value=parsed_footer_line[1]))
+      return
+
+    if not self._description_lines:

[Sergiy Byelozyorov, 2016/06/02 22:59:53]

Why are you accessing self._description_lines here and not using
self.description and self.set_description instead? Please add a comment with an
explanation.

[tandrii(chromium), 2016/06/03 09:34:59]

because _description_lines is actual primary storage of information. Why does
that need a comment?

[Sergiy Byelozyorov, 2016/06/03 12:35:16]

I was confused that you use both self._description_lines and self.description in
the same function, but now I see that they have different semantic: first is a
list of lines , second is just entire description. It's all good now :-).

+      self._description_lines.append(line)
+      return
+
+    top_lines, gerrit_footers, _ = git_footers.split_footers(self.description)
+    if gerrit_footers:
+      assert top_lines[-1] == '' and len(top_lines) >= 2

[Sergiy Byelozyorov, 2016/06/02 22:59:53]

Please mention that this is also guaranteed in the git_footers.split_footers
documentation. When reading this code, first thing that came to my mind was that
CL description is specified during upload in which case this assumption may be
violated by our users. And I needed to read the entire split_footers function to
see that indeed this assumption holds.

Also after further reading, I've noticed that top_lines may be shorter than 2.
For example, for description
  description = '\nMy-Footer: foo"
the function git.footers.split_footers(description) will return:
  ([''], ['My-Footer: foo'], [('My-Footer', 'foo')])

[tandrii(chromium), 2016/06/03 09:34:59]

Added comment.
So, previously that wasn't the case - top_lines was always at least 2. But after
I've modified the previous CL for split_footers implementation, yes, you are
right. refactored.

[Sergiy Byelozyorov, 2016/06/03 12:35:16]

Acknowledged.

+      insert_index = len(top_lines) - 1
+    else:
+      insert_index = len(top_lines)

[Sergiy Byelozyorov, 2016/06/02 22:59:53]

Shouldn't you also check and if necessary add an empty line here to guarantee
that footers are separated from the actual descriptions? The whole insert_index
logic is quite confusing... please add some comments or even better - try to
rewrite in a more elegant way. It seems to me that logic in this function is
deeply intervened and depends on undocumented logic in
gerrit_footers.split_footers and that makes it really confusing.

[tandrii(chromium), 2016/06/03 09:34:59]

OK, fair. Rewrote it. WDYT?

[Sergiy Byelozyorov, 2016/06/03 12:35:16]

Excellent. Much easier to understand now.

+    prev_line = top_lines[insert_index-1] if top_lines else None
+    if (not presubmit_support.Change.TAG_LINE_RE.match(prev_line) or
+        not presubmit_support.Change.TAG_LINE_RE.match(line)):
+        top_lines.insert(insert_index, '')
+        insert_index += 1
+    top_lines.insert(insert_index, line)
+    self._description_lines = top_lines + gerrit_footers
 
   def get_reviewers(self):
     """Retrieves the list of reviewers."""
     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
@@ skipping 2195 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   setup_color.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)