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

R=yyanagisawa@chromium.org,sergiyb@chromium.org
BUG=614587

Review-Url: https://codereview.chromium.org/2038673002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@300692 0039d316-1c4b-4281-b951-d872f2087c98

git_cl.py

@@ -2692,13 +2692,41 @@ 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:
+      self._description_lines.append(line)
+      return
+
+    top_lines, gerrit_footers, _ = git_footers.split_footers(self.description)
+    if gerrit_footers:
+      # git_footers.split_footers ensures that there is an empty line before
+      # actual (gerrit) footers, if any. We have to keep it that way.
+      assert top_lines and top_lines[-1] == ''
+      top_lines, separator = top_lines[:-1], top_lines[-1:]
+    else:
+      separator = []  # No need for separator if there are no gerrit_footers.
+
+    prev_line = top_lines[-1] if top_lines else ''
+    if (not presubmit_support.Change.TAG_LINE_RE.match(prev_line) or
+        not presubmit_support.Change.TAG_LINE_RE.match(line)):
+      top_lines.append('')
+    top_lines.append(line)
+    self._description_lines = top_lines + separator + gerrit_footers
 
   def get_reviewers(self):
     """Retrieves the list of reviewers."""

tests/git_cl_test.py

@@ -1022,6 +1022,25 @@ class TestGitCl(TestCase):
     change_id = git_cl.GenerateGerritChangeId('line1\nline2\n')
     self.assertEqual(change_id, 'Ihashchange')
 
+  def test_desecription_append_footer(self):
+    for init_desc, footer_line, expected_desc in [
+      # Use unique desc first lines for easy test failure identification.
+      ('foo', 'R=one', 'foo\n\nR=one'),
+      ('foo\n\nR=one', 'BUG=', 'foo\n\nR=one\nBUG='),
+      ('foo\n\nR=one', 'Change-Id: Ixx', 'foo\n\nR=one\n\nChange-Id: Ixx'),
+      ('foo\n\nChange-Id: Ixx', 'R=one', 'foo\n\nR=one\n\nChange-Id: Ixx'),
+      ('foo\n\nR=one\n\nChange-Id: Ixx', 'TBR=two',
+       'foo\n\nR=one\nTBR=two\n\nChange-Id: Ixx'),
+      ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz',
+       'foo\n\nR=one\n\nChange-Id: Ixx\nFoo-Bar: baz'),
+      ('foo\n\nChange-Id: Ixx', 'Foo-Bak: baz',
+       'foo\n\nChange-Id: Ixx\nFoo-Bak: baz'),
+      ('foo', 'Change-Id: Ixx', 'foo\n\nChange-Id: Ixx'),
+    ]:
+      desc = git_cl.ChangeDescription(init_desc)
+      desc.append_footer(footer_line)
+      self.assertEqual(desc.description, expected_desc)
+
   def test_update_reviewers(self):
     data = [
       ('foo', [], 'foo'),
@@ -1444,7 +1463,7 @@ class TestGitCl(TestCase):
     self.assertEqual('hihi', ChangelistMock.desc)
 
   def test_description_appends_bug_line(self):
-    current_desc = 'Some\n\nChange-Id: xxx'
+    current_desc = 'Some.\n\nChange-Id: xxx'
 
     def RunEditor(desc, _, **kwargs):
       self.assertEquals(
@@ -1452,15 +1471,14 @@ class TestGitCl(TestCase):
           '# This will be displayed on the codereview site.\n'
           '# The first line will also be used as the subject of the review.\n'
           '#--------------------This line is 72 characters long'
-          '--------------------\n' +
-          # TODO(tandrii): fix this http://crbug.com/614587.
-          current_desc + '\n\nBUG=',
+          '--------------------\n'
+          'Some.\n\nBUG=\n\nChange-Id: xxx',
           desc)
-      return current_desc + '\n\nBUG='
+      # Simulate user changing something.
+      return 'Some.\n\nBUG=123\n\nChange-Id: xxx'
 
     def UpdateDescriptionRemote(_, desc):
-      # TODO(tandrii): fix this http://crbug.com/614587.
-      self.assertEquals(desc, current_desc + '\n\nBUG=')
+      self.assertEquals(desc, 'Some.\n\nBUG=123\n\nChange-Id: xxx')
 
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self.mock(git_cl.Changelist, 'GetDescription',