Fix writing GPG signatures with trailing newline

Make sure we don't produce a spurious empty line at the end.

Bug: 564428
Change-Id: Ib991d93fbd052baca65d32a7842f07f9ddeb8130
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

2 files changed, 65 insertions, 45 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/CommitBuilderTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/CommitBuilderTest.java
index 26294c7714..dee58f9cfc 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/CommitBuilderTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/CommitBuilderTest.java
@@ -13,7 +13,7 @@ import static java.nio.charset.StandardCharsets.US_ASCII;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.assertThrows;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -24,6 +24,32 @@ import org.junit.Test;
 
 public class CommitBuilderTest {
 
+	// @formatter:off
+	private static final String SIGNATURE = "-----BEGIN PGP SIGNATURE-----\n" +
+			"Version: BCPG v1.60\n" +
+			"\n" +
+			"iQEcBAABCAAGBQJb9cVhAAoJEKX+6Axg/6TZeFsH/0CY0WX/z7U8+7S5giFX4wH4\n" +
+			"opvBwqyt6OX8lgNwTwBGHFNt8LdmDCCmKoq/XwkNi3ARVjLhe3gBcKXNoavvPk2Z\n" +
+			"gIg5ChevGkU4afWCOMLVEYnkCBGw2+86XhrK1P7gTHEk1Rd+Yv1ZRDJBY+fFO7yz\n" +
+			"uSBuF5RpEY2sJiIvp27Gub/rY3B5NTR/feO/z+b9oiP/fMUhpRwG5KuWUsn9NPjw\n" +
+			"3tvbgawYpU/2UnS+xnavMY4t2fjRYjsoxndPLb2MUX8X7vC7FgWLBlmI/rquLZVM\n" +
+			"IQEKkjnA+lhejjK1rv+ulq4kGZJFKGYWYYhRDwFg5PTkzhudhN2SGUq5Wxq1Eg4=\n" +
+			"=b9OI\n" +
+			"-----END PGP SIGNATURE-----";
+
+	private static final String EXPECTED = "-----BEGIN PGP SIGNATURE-----\n" +
+			" Version: BCPG v1.60\n" +
+			" \n" +
+			" iQEcBAABCAAGBQJb9cVhAAoJEKX+6Axg/6TZeFsH/0CY0WX/z7U8+7S5giFX4wH4\n" +
+			" opvBwqyt6OX8lgNwTwBGHFNt8LdmDCCmKoq/XwkNi3ARVjLhe3gBcKXNoavvPk2Z\n" +
+			" gIg5ChevGkU4afWCOMLVEYnkCBGw2+86XhrK1P7gTHEk1Rd+Yv1ZRDJBY+fFO7yz\n" +
+			" uSBuF5RpEY2sJiIvp27Gub/rY3B5NTR/feO/z+b9oiP/fMUhpRwG5KuWUsn9NPjw\n" +
+			" 3tvbgawYpU/2UnS+xnavMY4t2fjRYjsoxndPLb2MUX8X7vC7FgWLBlmI/rquLZVM\n" +
+			" IQEKkjnA+lhejjK1rv+ulq4kGZJFKGYWYYhRDwFg5PTkzhudhN2SGUq5Wxq1Eg4=\n" +
+			" =b9OI\n" +
+			" -----END PGP SIGNATURE-----";
+	// @formatter:on
+
 	private void assertGpgSignatureStringOutcome(String signature,
 			String expectedOutcome) throws IOException {
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
@@ -33,47 +59,37 @@ public class CommitBuilderTest {
 	}
 
 	@Test
-	public void writeGpgSignatureString_1() throws Exception {
-		// @formatter:off
-		String signature = "-----BEGIN PGP SIGNATURE-----\n" +
-				"Version: BCPG v1.60\n" +
-				"\n" +
-				"iQEcBAABCAAGBQJb9cVhAAoJEKX+6Axg/6TZeFsH/0CY0WX/z7U8+7S5giFX4wH4\n" +
-				"opvBwqyt6OX8lgNwTwBGHFNt8LdmDCCmKoq/XwkNi3ARVjLhe3gBcKXNoavvPk2Z\n" +
-				"gIg5ChevGkU4afWCOMLVEYnkCBGw2+86XhrK1P7gTHEk1Rd+Yv1ZRDJBY+fFO7yz\n" +
-				"uSBuF5RpEY2sJiIvp27Gub/rY3B5NTR/feO/z+b9oiP/fMUhpRwG5KuWUsn9NPjw\n" +
-				"3tvbgawYpU/2UnS+xnavMY4t2fjRYjsoxndPLb2MUX8X7vC7FgWLBlmI/rquLZVM\n" +
-				"IQEKkjnA+lhejjK1rv+ulq4kGZJFKGYWYYhRDwFg5PTkzhudhN2SGUq5Wxq1Eg4=\n" +
-				"=b9OI\n" +
-				"-----END PGP SIGNATURE-----";
-		String expectedOutcome = "-----BEGIN PGP SIGNATURE-----\n" +
-				" Version: BCPG v1.60\n" +
-				" \n" +
-				" iQEcBAABCAAGBQJb9cVhAAoJEKX+6Axg/6TZeFsH/0CY0WX/z7U8+7S5giFX4wH4\n" +
-				" opvBwqyt6OX8lgNwTwBGHFNt8LdmDCCmKoq/XwkNi3ARVjLhe3gBcKXNoavvPk2Z\n" +
-				" gIg5ChevGkU4afWCOMLVEYnkCBGw2+86XhrK1P7gTHEk1Rd+Yv1ZRDJBY+fFO7yz\n" +
-				" uSBuF5RpEY2sJiIvp27Gub/rY3B5NTR/feO/z+b9oiP/fMUhpRwG5KuWUsn9NPjw\n" +
-				" 3tvbgawYpU/2UnS+xnavMY4t2fjRYjsoxndPLb2MUX8X7vC7FgWLBlmI/rquLZVM\n" +
-				" IQEKkjnA+lhejjK1rv+ulq4kGZJFKGYWYYhRDwFg5PTkzhudhN2SGUq5Wxq1Eg4=\n" +
-				" =b9OI\n" +
-				" -----END PGP SIGNATURE-----";
-		// @formatter:on
-		assertGpgSignatureStringOutcome(signature, expectedOutcome);
+	public void writeGpgSignatureString() throws Exception {
+		assertGpgSignatureStringOutcome(SIGNATURE, EXPECTED);
+	}
+
+	@Test
+	public void writeGpgSignatureStringTrailingLF() throws Exception {
+		assertGpgSignatureStringOutcome(SIGNATURE + '\n', EXPECTED);
+	}
+
+	@Test
+	public void writeGpgSignatureStringCRLF() throws Exception {
+		assertGpgSignatureStringOutcome(SIGNATURE.replaceAll("\n", "\r\n"),
+				EXPECTED);
+	}
+
+	@Test
+	public void writeGpgSignatureStringTrailingCRLF() throws Exception {
+		assertGpgSignatureStringOutcome(
+				SIGNATURE.replaceAll("\n", "\r\n") + "\r\n", EXPECTED);
 	}
 
 	@Test
 	public void writeGpgSignatureString_failsForNonAscii() throws Exception {
 		String signature = "Ü Ä";
-		try {
-			CommitBuilder.writeGpgSignatureString(signature,
-					new ByteArrayOutputStream());
-			fail("Exception expected");
-		} catch (IllegalArgumentException e) {
-			// good
-			String message = MessageFormat.format(JGitText.get().notASCIIString,
-					signature);
-			assertEquals(message, e.getMessage());
-		}
+		IllegalArgumentException e = assertThrows(
+				IllegalArgumentException.class,
+				() -> CommitBuilder.writeGpgSignatureString(signature,
+						new ByteArrayOutputStream()));
+		String message = MessageFormat.format(JGitText.get().notASCIIString,
+				signature);
+		assertEquals(message, e.getMessage());
 	}
 
 	@Test
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
index 66d7d51bdf..4f93fda49f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
@@ -361,7 +361,9 @@ public class CommitBuilder {
 	 * header</a>.
 	 * <p>
 	 * CRLF and CR will be sanitized to LF and signature will have a hanging
-	 * indent of one space starting with line two.
+	 * indent of one space starting with line two. A trailing line break is
+	 * <em>not</em> written; the caller is supposed to terminate the GPG
+	 * signature header by writing a single newline.
 	 * </p>
 	 *
 	 * @param in
@@ -375,22 +377,24 @@ public class CommitBuilder {
 	 */
 	static void writeGpgSignatureString(String in, OutputStream out)
 			throws IOException, IllegalArgumentException {
-		for (int i = 0; i < in.length(); ++i) {
+		int length = in.length();
+		for (int i = 0; i < length; ++i) {
 			char ch = in.charAt(i);
 			switch (ch) {
 			case '\r':
-				if (i + 1 < in.length() && in.charAt(i + 1) == '\n') {
-					out.write('\n');
-					out.write(' ');
+				if (i + 1 < length && in.charAt(i + 1) == '\n') {
 					++i;
-				} else {
+				}
+				if (i + 1 < length) {
 					out.write('\n');
 					out.write(' ');
 				}
 				break;
 			case '\n':
-				out.write('\n');
-				out.write(' ');
+				if (i + 1 < length) {
+					out.write('\n');
+					out.write(' ');
+				}
 				break;
 			default:
 				// sanity check