Merge "Improve footer parsing to allow multiline footers."

3 files changed, 81 insertions, 28 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FooterLineTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FooterLineTest.java
index 01f6a3a0a0..303aedcd00 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FooterLineTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FooterLineTest.java
@@ -318,6 +318,57 @@ public class FooterLineTest extends RepositoryTestCase {
 		assertFalse("not CC", line.matches(FooterKey.CC));
 	}
 
+	@Test
+	public void testMultilineFooters() {
+		String msg = buildMessage("subject\n\nbody of commit\n"
+				+ "Not-A-Footer-Line: this line must not be read as a footer\n"
+				+ "\n" // paragraph break, now footers appear in final block
+				+ "Notes: The change must not be merged until dependency ABC is\n"
+				+ " updated.\n"
+				+ "CC:            <some.mailing.list@example.com>\n"
+				+ "not really a footer line but we'll skip it anyway\n"
+				+ "Acked-by: Some Reviewer <sr@example.com>\n");
+		List<FooterLine> footers = FooterLine.fromMessage(msg);
+		FooterLine f;
+
+		assertNotNull(footers);
+		assertEquals(3, footers.size());
+
+		f = footers.get(0);
+		assertEquals("Notes", f.getKey());
+		assertEquals(
+				"The change must not be merged until dependency ABC is updated.",
+				f.getValue());
+
+		f = footers.get(1);
+		assertEquals("CC", f.getKey());
+		assertEquals("<some.mailing.list@example.com>", f.getValue());
+
+		f = footers.get(2);
+		assertEquals("Acked-by", f.getKey());
+		assertEquals("Some Reviewer <sr@example.com>", f.getValue());
+	}
+
+	@Test
+	public void testMultilineFooters_multipleWhitespaceAreAllowed() {
+		String msg = buildMessage("subject\n\nbody of commit\n"
+				+ "Not-A-Footer-Line: this line must not be read as a footer\n"
+				+ "\n" // paragraph break, now footers appear in final block
+				+ "Notes: The change must not be merged until dependency ABC is\n"
+				+ "    updated.\n");
+		List<FooterLine> footers = FooterLine.fromMessage(msg);
+		FooterLine f;
+
+		assertNotNull(footers);
+		assertEquals(1, footers.size());
+
+		f = footers.get(0);
+		assertEquals("Notes", f.getKey());
+		assertEquals(
+				"The change must not be merged until dependency ABC is updated.",
+				f.getValue());
+	}
+
 	private String buildMessage(String msg) {
 		StringBuilder buf = new StringBuilder();
 		buf.append("tree " + ObjectId.zeroId().name() + "\n");
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FooterLine.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FooterLine.java
index 2d396d57c5..cadeaec6e0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FooterLine.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/FooterLine.java
@@ -74,41 +74,42 @@ public final class FooterLine {
 	 */
 	public static List<FooterLine> fromMessage(
 			byte[] raw) {
-		int ptr = raw.length - 1;
-		while (raw[ptr] == '\n') // trim any trailing LFs, not interesting
-			ptr--;
+		// Find the end of the last paragraph.
+		int parEnd = raw.length;
+		for (; parEnd > 0 && (raw[parEnd - 1] == '\n' || raw[parEnd - 1] == ' '); --parEnd);
 
 		int msgB = RawParseUtils.commitMessage(raw, 0);
 		ArrayList<FooterLine> r = new ArrayList<>(4);
 		Charset enc = RawParseUtils.guessEncoding(raw);
-		for (;;) {
-			ptr = RawParseUtils.prevLF(raw, ptr);
-			if (ptr <= msgB)
-				break; // Don't parse commit headers as footer lines.
 
-			int keyStart = ptr + 2;
-			if (raw[keyStart] == '\n')
-				break; // Stop at first paragraph break, no footers above it.
+		// Search for the beginning of last paragraph
+		int parStart = parEnd;
+		for (; parStart > msgB && (raw[parStart - 1] != '\n' || raw[parStart - 2] != '\n'); --parStart);
 
-			int keyEnd = RawParseUtils.endOfFooterLineKey(raw, keyStart);
-			if (keyEnd < 0)
-				continue; // Not a well formed footer line, skip it.
+		for (int ptr = parStart; ptr < parEnd;) {
+			int keyStart = ptr;
+			int keyEnd = RawParseUtils.endOfFooterLineKey(raw, ptr);
+			if (keyEnd < 0) {
+				// Not a well-formed footer line, skip it.
+				ptr = RawParseUtils.nextLF(raw, ptr);
+				continue;
+			}
 
 			// Skip over the ': *' at the end of the key before the value.
-			//
-			int valStart = keyEnd + 1;
-			while (valStart < raw.length && raw[valStart] == ' ')
-				valStart++;
-
-			// Value ends at the LF, and does not include it.
-			//
-			int valEnd = RawParseUtils.nextLF(raw, valStart);
-			if (raw[valEnd - 1] == '\n')
-				valEnd--;
-
+			int valStart, valEnd;
+			for (valStart = keyEnd + 1; valStart < raw.length && raw[valStart] == ' '; ++valStart);
+
+			for(ptr = valStart;;) {
+				ptr = RawParseUtils.nextLF(raw, ptr);
+				// Next line starts with whitespace for a multiline footer.
+				if (ptr == raw.length || raw[ptr] != ' ') {
+					valEnd = raw[ptr - 1] == '\n' ? ptr - 1 : ptr;
+					break;
+				}
+			}
 			r.add(new FooterLine(raw, enc, keyStart, keyEnd, valStart, valEnd));
 		}
-		Collections.reverse(r);
+
 		return r;
 	}
 
@@ -199,7 +200,7 @@ public final class FooterLine {
 	 *         character encoding.
 	 */
 	public String getValue() {
-		return RawParseUtils.decode(enc, buffer, valStart, valEnd);
+		return RawParseUtils.decode(enc, buffer, valStart, valEnd).replaceAll("\n +", " ");
 	}
 
 	/**
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
index 0392ea428c..1f0f13152e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
@@ -570,8 +570,9 @@ public class RevCommit extends RevObject {
 	 * the order of the line's appearance in the commit message itself.
 	 * <p>
 	 * A footer line's key must match the pattern {@code ^[A-Za-z0-9-]+:}, while
-	 * the value is free-form, but must not contain an LF. Very common keys seen
-	 * in the wild are:
+	 * the value is free-form. The Value may be split over multiple lines with
+	 * each subsequent line starting with at least one whitespace. Very common
+	 * keys seen in the wild are:
 	 * <ul>
 	 * <li>{@code Signed-off-by} (agrees to Developer Certificate of Origin)
 	 * <li>{@code Acked-by} (thinks change looks sane in context)