From 04b51d6de0c1eb267f2446126a682fc747620439 Mon Sep 17 00:00:00 2001
From: Andrii Shyshkalov <tandrii@chromium.org>
Date: Thu, 11 May 2017 13:21:30 +0200
Subject: [PATCH] Reland of Relax git_footers parsing to match that of Gerrit
 (JGit).

Previously landed as:
> Change-Id: Ieb6415d55e85b91f11f9052b0fd08cf982b64d51
> Reviewed-on: https://chromium-review.googlesource.com/501849

R=agable@chromium.org,machenbach@chromium.org

Bug: 717504
Change-Id: Iede5c29ff4c317b68d8c2bca319edec140a176f5
Reviewed-on: https://chromium-review.googlesource.com/502908
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
---
 git_footers.py                                | 21 ++++++++++---
 .../bot_update/resources/bot_update.py        |  6 ++--
 tests/git_footers_test.py                     | 31 +++++++++++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/git_footers.py b/git_footers.py
index 8e9aaff9b..e03c71c19 100755
--- a/git_footers.py
+++ b/git_footers.py
@@ -42,12 +42,25 @@ def parse_footers(message):
   return footer_map
 
 
+def matches_footer_key(line, key):
+  """Returns whether line is a valid footer whose key matches a given one.
+
+  Keys are compared in normalized form.
+  """
+  r = parse_footer(line)
+  if r is None:
+    return None
+  return normalize_name(r[0]) == normalize_name(key)
+
+
 def split_footers(message):
   """Returns (non_footer_lines, footer_lines, parsed footers).
 
   Guarantees that:
     (non_footer_lines + footer_lines) == message.splitlines().
     parsed_footers is parse_footer applied on each line of footer_lines.
+      There could be fewer parsed_footers than footer lines if some lines in
+      last paragraph are malformed.
   """
   message_lines = list(message.splitlines())
   footer_lines = []
@@ -61,8 +74,8 @@ def split_footers(message):
     footer_lines = []
 
   footer_lines.reverse()
-  footers = map(parse_footer, footer_lines)
-  if not footer_lines or not all(footers):
+  footers = filter(None, map(parse_footer, footer_lines))
+  if not footers:
     return message_lines, [], []
   return message_lines[:-len(footer_lines)], footer_lines, footers
 
@@ -111,11 +124,11 @@ def add_footer(message, key, value, after_keys=None, before_keys=None):
     after_keys = set(map(normalize_name, after_keys or []))
     after_indices = [
         footer_lines.index(x) for x in footer_lines for k in after_keys
-        if normalize_name(parse_footer(x)[0]) == k]
+        if matches_footer_key(x, k)]
     before_keys = set(map(normalize_name, before_keys or []))
     before_indices = [
         footer_lines.index(x) for x in footer_lines for k in before_keys
-        if normalize_name(parse_footer(x)[0]) == k]
+        if matches_footer_key(x, k)]
     if after_indices:
       # after_keys takes precedence, even if there's a conflict.
       insert_idx = max(after_indices) + 1
diff --git a/recipes/recipe_modules/bot_update/resources/bot_update.py b/recipes/recipe_modules/bot_update/resources/bot_update.py
index 729f3337b..baf8aeea1 100755
--- a/recipes/recipe_modules/bot_update/resources/bot_update.py
+++ b/recipes/recipe_modules/bot_update/resources/bot_update.py
@@ -411,9 +411,9 @@ def get_commit_message_footer_map(message):
   for line in lines:
     m = COMMIT_FOOTER_ENTRY_RE.match(line)
     if not m:
-      # If any single line isn't valid, the entire footer is invalid.
-      footers.clear()
-      return footers
+      # If any single line isn't valid, continue anyway for compatibility with
+      # Gerrit (which itself uses JGit for this).
+      continue
     footers[m.group(1)] = m.group(2).strip()
   return footers
 
diff --git a/tests/git_footers_test.py b/tests/git_footers_test.py
index 8bc06bb2c..1d537cee3 100755
--- a/tests/git_footers_test.py
+++ b/tests/git_footers_test.py
@@ -59,6 +59,33 @@ My commit message is my best friend. It is my life. I must master it.
         { 'Bug': [''],
           'Cr-Commit-Position': [ self._position ] })
 
+  def testSkippingBadFooterLines(self):
+    message = ('Title.\n'
+               '\n'
+               'Last: paragraph starts\n'
+               'It-may: contain\n'
+               'bad lines, which should be skipped\n'
+               'For: example\n'
+               '(cherry picked from)\n'
+               'And-only-valid: footers taken')
+    self.assertEqual(git_footers.split_footers(message),
+                     (['Title.',
+                       ''],
+                      ['Last: paragraph starts',
+                       'It-may: contain',
+                       'bad lines, which should be skipped',
+                       'For: example',
+                       '(cherry picked from)',
+                       'And-only-valid: footers taken'],
+                      [('Last', 'paragraph starts'),
+                       ('It-may', 'contain'),
+                       ('For', 'example'),
+                       ('And-only-valid', 'footers taken')]))
+    self.assertEqual(git_footers.parse_footers(message),
+                     {'Last': ['paragraph starts'],
+                      'It-May': ['contain'],
+                      'For': ['example'],
+                      'And-Only-Valid': ['footers taken']})
 
   def testGetFooterChangeId(self):
     msg = '\n'.join(['whatever',
@@ -101,6 +128,10 @@ My commit message is my best friend. It is my life. I must master it.
         git_footers.add_footer_change_id('header: like footer', 'Ixxx'),
         'header: like footer\n\nChange-Id: Ixxx')
 
+    self.assertEqual(
+        git_footers.add_footer_change_id('Header.\n\nBug: v8\nN=t\nT=z', 'Ix'),
+        'Header.\n\nBug: v8\nChange-Id: Ix\nN=t\nT=z')
+
   def testAddFooter(self):
     self.assertEqual(
         git_footers.add_footer('', 'Key', 'Value'),