BaseReceivePack: Treat all LFs as optional

Discussion on the git mailing list has concluded[1] that the intended
behavior for all (non-sideband) portions of the receive-pack protocol
is for trailing LFs in pkt-lines to be optional. Go back to using
PacketLineIn#readString() everywhere.

For push certificates specifically, we agreed that the payload signed
by the client is always concatenated with LFs even though the client
MAY omit LFs when framing the certificate for the wire. This is still
reflected in the implementation of PushCertificate#toText().

[1] http://thread.gmane.org/gmane.comp.version-control.git/273175/focus=273412

Change-Id: I817231c4d4defececb8722142fea18ff42e06e44

2 files changed, 82 insertions, 18 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java
index 1b35192063..7578c6e3eb 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java
@@ -52,13 +52,6 @@ import org.junit.Test;
 /** Tests for base receive-pack utilities. */
 public class BaseReceivePackTest {
 	@Test
-	public void chomp() {
-		assertEquals("foo", BaseReceivePack.chomp("foo"));
-		assertEquals("foo", BaseReceivePack.chomp("foo\n"));
-		assertEquals("foo\n", BaseReceivePack.chomp("foo\n\n"));
-	}
-
-	@Test
 	public void parseCommand() throws Exception {
 		String o = "0000000000000000000000000000000000000000";
 		String n = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
index 6e49f47ff5..8fdf386ecc 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
@@ -87,6 +87,30 @@ public class PushCertificateParserTest {
 			+ "0020-----END PGP SIGNATURE-----\n"
 			+ "0012push-cert-end\n";
 
+	// Same push certificate, with all trailing newlines stripped.
+	// (Note that the canonical signed payload is the same, so the same signature
+	// is still valid.)
+	private static final String INPUT_NO_NEWLINES = "001bcertificate version 0.1"
+			+ "0040pusher Dave Borowitz <dborowitz@google.com> 1433954361 -0700"
+			+ "0023pushee git://localhost/repo.git"
+			+ "0029nonce 1433954361-bde756572d665bba81d8"
+			+ "0004"
+			+ "00670000000000000000000000000000000000000000"
+			+ " 6c2b981a177396fb47345b7df3e4d3f854c6bea7"
+			+ " refs/heads/master"
+			+ "0021-----BEGIN PGP SIGNATURE-----"
+			+ "0015Version: GnuPG v1"
+			+ "0004"
+			+ "0044iQEcBAABAgAGBQJVeGg5AAoJEPfTicJkUdPkUggH/RKAeI9/i/LduuiqrL/SSdIa"
+			+ "00449tYaSqJKLbXz63M/AW4Sp+4u+dVCQvnAt/a35CVEnpZz6hN4Kn/tiswOWVJf4CO7"
+			+ "0044htNubGs5ZMwvD6sLYqKAnrM3WxV/2TbbjzjZW6Jkidz3jz/WRT4SmjGYiEO7aA+V"
+			+ "00444ZdIS9f7sW5VsHHYlNThCA7vH8Uu48bUovFXyQlPTX0pToSgrWV3JnTxDNxfn3iG"
+			+ "0044IL0zTY/qwVCdXgFownLcs6J050xrrBWIKqfcWr3u4D2aCLyR0v+S/KArr7ulZygY"
+			+ "0044+SOklImn8TAZiNxhWtA6ens66IiammUkZYFv7SSzoPLFZT4dC84SmGPWgf94NoQ="
+			+ "0009=XFeC"
+			+ "001f-----END PGP SIGNATURE-----"
+			+ "0011push-cert-end";
+
 	private Repository db;
 
 	@Before
@@ -115,11 +139,10 @@ public class PushCertificateParserTest {
 		ObjectId newId =
 				ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
 		String line = oldId.name() + " " + newId.name() + " refs/heads/master";
-		String rawLine = line + "\n";
-
 		ReceiveCommand cmd = BaseReceivePack.parseCommand(line);
-		parser.addCommand(cmd, rawLine);
-		parser.addCommand(rawLine);
+
+		parser.addCommand(cmd);
+		parser.addCommand(line);
 		assertNull(parser.build());
 	}
 
@@ -132,8 +155,8 @@ public class PushCertificateParserTest {
 		assertNull(parser.build());
 
 		parser.receiveHeader(pckIn, false);
-		parser.addCommand(pckIn.readStringRaw());
-		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
+		parser.addCommand(pckIn.readString());
+		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString());
 		parser.receiveSignature(pckIn);
 		assertNull(parser.build());
 	}
@@ -162,8 +185,46 @@ public class PushCertificateParserTest {
 		PushCertificateParser parser =
 				new PushCertificateParser(db, newEnabledConfig());
 		parser.receiveHeader(pckIn, false);
-		parser.addCommand(pckIn.readStringRaw());
-		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
+		parser.addCommand(pckIn.readString());
+		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString());
+		parser.receiveSignature(pckIn);
+
+		PushCertificate cert = parser.build();
+		assertEquals("0.1", cert.getVersion());
+		assertEquals("Dave Borowitz", cert.getPusherIdent().getName());
+		assertEquals("dborowitz@google.com",
+				cert.getPusherIdent().getEmailAddress());
+		assertEquals(1433954361000L, cert.getPusherIdent().getWhen().getTime());
+		assertEquals(-7 * 60, cert.getPusherIdent().getTimeZoneOffset());
+		assertEquals("git://localhost/repo.git", cert.getPushee());
+		assertEquals("1433954361-bde756572d665bba81d8", cert.getNonce());
+
+		assertNotEquals(cert.getNonce(), parser.getAdvertiseNonce());
+		assertEquals(PushCertificate.NonceStatus.BAD, cert.getNonceStatus());
+
+		assertEquals(1, cert.getCommands().size());
+		ReceiveCommand cmd = cert.getCommands().get(0);
+		assertEquals("refs/heads/master", cmd.getRefName());
+		assertEquals(ObjectId.zeroId(), cmd.getOldId());
+		assertEquals("6c2b981a177396fb47345b7df3e4d3f854c6bea7",
+				cmd.getNewId().name());
+
+		assertEquals(concatPacketLines(INPUT, 0, 6), cert.toText());
+
+		String signature = concatPacketLines(INPUT, 6, 17);
+		assertTrue(signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE));
+		assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE + "\n"));
+		assertEquals(signature, cert.getSignature());
+	}
+
+	@Test
+	public void parseCertFromPktLineNoNewlines() throws Exception {
+		PacketLineIn pckIn = newPacketLineIn(INPUT_NO_NEWLINES);
+		PushCertificateParser parser =
+				new PushCertificateParser(db, newEnabledConfig());
+		parser.receiveHeader(pckIn, false);
+		parser.addCommand(pckIn.readString());
+		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString());
 		parser.receiveSignature(pckIn);
 
 		PushCertificate cert = parser.build();
@@ -186,11 +247,12 @@ public class PushCertificateParserTest {
 		assertEquals("6c2b981a177396fb47345b7df3e4d3f854c6bea7",
 				cmd.getNewId().name());
 
+		// Canonical signed payload has reinserted newlines.
 		assertEquals(concatPacketLines(INPUT, 0, 6), cert.toText());
 
 		String signature = concatPacketLines(INPUT, 6, 17);
 		assertTrue(signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE));
-		assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE));
+		assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE + "\n"));
 		assertEquals(signature, cert.getSignature());
 	}
 
@@ -203,6 +265,15 @@ public class PushCertificateParserTest {
 		assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 4));
 	}
 
+	@Test
+	public void testConcatPacketLinesInsertsNewlines() throws Exception {
+		String input = "000bline 1\n000aline 2000bline 3\n";
+		assertEquals("line 1\n", concatPacketLines(input, 0, 1));
+		assertEquals("line 1\nline 2\n", concatPacketLines(input, 0, 2));
+		assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 3));
+		assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 4));
+	}
+
 	private static String concatPacketLines(String input, int begin, int end)
 			throws IOException {
 		StringBuilder result = new StringBuilder();
@@ -211,12 +282,12 @@ public class PushCertificateParserTest {
 		while (i < end) {
 			String line;
 			try {
-				line = pckIn.readStringRaw();
+				line = pckIn.readString();
 			} catch (EOFException e) {
 				break;
 			}
 			if (++i > begin) {
-				result.append(line);
+				result.append(line).append('\n');
 			}
 		}
 		return result.toString();