Refactor git_footers for later use in git cl.

git_footers.py

Index: git_footers.py
diff --git a/git_footers.py b/git_footers.py
index c5f69b01333e6995b55a698bf8b3f19e591025c7..d049dfdc24cf18e4e2d2b41b6836b9a7aac9aabc 100755
--- a/git_footers.py
+++ b/git_footers.py
@@ -32,21 +32,38 @@ def parse_footer(line):
 
 def parse_footers(message):
   """Parses a git commit message into a multimap of footers."""
+  _, _, parsed_footers = split_footers(message)
+  footer_map = defaultdict(list)
+  if parsed_footers:
+    # Read footers from bottom to top.

[Sergiy Byelozyorov, 2016/06/02 23:16:26]

Why so?

[tandrii(chromium), 2016/06/03 09:16:17]

that's how git_footers always worked. But added a better comment. WDYT?

[Sergiy Byelozyorov, 2016/06/03 12:43:07]

Thanks.

+    for (k, v) in reversed(parsed_footers):
+      footer_map[normalize_name(k)].append(v.strip())
+  return footer_map
+
+
+def split_footers(message):
+  """Returns (non_footer_lines, footer_lines, parsed footers).
+
+  Guarantees that:
+    (non_footer_lines + footer_lines) == message.splitlines().
+    parsed_footers correspond each line in footer_lines.

[Sergiy Byelozyorov, 2016/06/02 23:16:26]

Document the structure of each entry in parsed_footers or better yet - document
return of value of the parse_footer function and here simply mention that it
matches its output.

[tandrii(chromium), 2016/06/03 09:16:17]

Done.

+  """
+  message_lines = list(message.splitlines())
   footer_lines = []
-  for line in reversed(message.splitlines()):
+  for line in reversed(message_lines):
     if line == '' or line.isspace():

[Yoshisato Yanagisawa, 2016/06/03 07:00:00]

Is non footer and footer are always split with empty line?

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

Yes

       break
     footer_lines.append(line)
+  else:
+    # The whole description was consisting of footers,
+    # which means those aren't footers.
+    footer_lines = []
 
+  footer_lines.reverse()
   footers = map(parse_footer, footer_lines)
-  if not all(footers):
-    return defaultdict(list)
-
-  footer_map = defaultdict(list)
-  for (k, v) in footers:
-    footer_map[normalize_name(k)].append(v.strip())
-
-  return footer_map
+  if not footer_lines or not all(footers):
+    return message_lines, [], []
+  return message_lines[:-len(footer_lines)], footer_lines, footers
 
 
 def get_footer_svn_id(branch=None):
@@ -67,42 +84,51 @@ def get_footer_change_id(message):
   """Returns a list of Gerrit's ChangeId from given commit message."""
   return parse_footers(message).get(normalize_name('Change-Id'), [])
 
-

[Sergiy Byelozyorov, 2016/06/02 23:16:27]

nit: keep the second line, these are top-level defintions

[tandrii(chromium), 2016/06/03 09:16:17]

Done.

 def add_footer_change_id(message, change_id):
   """Returns message with Change-ID footer in it.
 
-  Assumes that Change-Id is not yet in footers, which is then
-  inserted after any of these footers: Bug|Issue|Test|Feature.
+  Assumes that Change-Id is not yet in footers, which is then inserted at
+  earliest footer line which is after all of these footers:
+    Bug|Issue|Test|Feature.

[Sergiy Byelozyorov, 2016/06/02 23:16:27]

(optional) nit: Bug, Issue, Test, Feature

   """
-  assert 0 == len(get_footer_change_id(message))
-  change_id_line = 'Change-Id: %s' % change_id
-  # This code does the same as parse_footers, but keeps track of line
-  # numbers so that ChangeId is inserted in the right place.
-  lines = message.splitlines()
-  footer_lines = []
-  for line in reversed(lines):
-    if line == '' or line.isspace():
-      break
-    footer_lines.append(line)
-  else:
-    # The whole description was consisting of footers,
-    # which means those aren't footers.
-    footer_lines = []
-  # footers order is from end to start of the message.
-  footers = map(parse_footer, footer_lines)
-  if not footers or not all(footers):
-    lines.append('')
-    lines.append(change_id_line)
+  assert 'Change-Id' not in parse_footers(message)

[Sergiy Byelozyorov, 2016/06/02 23:16:26]

(optional) Should we really use assert here? This function looks like it's part
of the public API of this module and AFAIK, exceptions is the right way to
report incorrect usage by the callers.

[tandrii(chromium), 2016/06/03 09:16:17]

True, but 
NO! Assert is perfect for https://en.wikipedia.org/wiki/Design_by_contract
The whole point is that assert can be skipped in optimized build, just like
DCHECK in C++.

[Sergiy Byelozyorov, 2016/06/03 12:43:07]

Acknowledged.

+  return add_footer(message, 'Change-Id', change_id,
+                    after_keys=['Bug', 'Issue', 'Test', 'Feature'])
+
+def add_footer(message, key, value, after_keys=None):
+  """Returns a message with given footer appended.
+
+  If after_keys is None (default), appends footer last.
+  Otherwise, after_keys must be iterable of footer keys, then the new footer
+  would be inserted at the topmost position such there would be no footer lines
+  after it with key matching one of after_keys.
+  For example, given
+      message='Header.\n\nAdded: 2016\nBug: 123\nVerified-By: CQ'
+      after_keys=['Bug', 'Issue']
+  the new footer will be inserted between Bug and Verified-By existing footers.
+  """
+  assert key == normalize_name(key), 'Use normalized key'

[Sergiy Byelozyorov, 2016/06/02 23:16:26]

ditto (also optional)

+  new_footer = '%s: %s' % (key, value)
+
+  top_lines, footer_lines, parsed_footers = split_footers(message)
+  if not footer_lines:
+    if not top_lines:
+      top_lines = ['', '']

[Sergiy Byelozyorov, 2016/06/02 23:16:27]

Why two empty lines here?

[tandrii(chromium), 2016/06/03 09:16:17]

At the time of writing this, I was thinking of minor UI optimization, but you
are right, not needed.

[Sergiy Byelozyorov, 2016/06/03 12:43:07]

Acknowledged.

+    else:
+      top_lines.append('')

[Sergiy Byelozyorov, 2016/06/02 23:16:26]

What if there was already an empty line in the end? Wouldn't we be adding too
many empty lines?

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

Good point! added test for that.

[Sergiy Byelozyorov, 2016/06/03 12:43:07]

Acknowledged.

+    footer_lines = [new_footer]
+  elif not after_keys:
+    footer_lines.append(new_footer)
   else:
-    after = set(map(normalize_name, ['Bug', 'Issue', 'Test', 'Feature']))
-    for i, (key, _) in enumerate(footers):
-      if normalize_name(key) in after:
-        insert_at = len(lines) - i
+    after_keys = set(map(normalize_name, after_keys))
+    # Iterate from last to first footer till we find the footer keys above.
+    for i, (key, _) in reversed(list(enumerate(parsed_footers))):

[Sergiy Byelozyorov, 2016/06/02 23:16:27]

How about this?

  for i, key in enumerate(reversed(parsed_footers)):

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

meaning of `i` would be different.

[Sergiy Byelozyorov, 2016/06/03 12:43:07]

Actually yes, I forgot to delete my comment after testing it myself.

+      if normalize_name(key) in after_keys:
+        footer_lines.insert(i + 1, new_footer)
         break
     else:
-      insert_at = len(lines) - len(footers)
-    lines.insert(insert_at, change_id_line)
-  return '\n'.join(lines)
+      footer_lines.insert(0, new_footer)
+  return '\n'.join(top_lines + footer_lines)
 
 
 def get_unique(footers, key):