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

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index b958d0023e77614b76a9f4f2893ca8d6d8fcb226..9ba619bd10d07e7629583c56a1ade6361ee14bc3 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -2692,13 +2692,39 @@ class ChangeDescription(object):
     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."""