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

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java

@@ -51,13 +51,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";

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());
+		parser.addCommand(pckIn.readString());
-		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
+		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());
+		parser.addCommand(pckIn.readString());
-		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
+		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();

org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java

@@ -1066,18 +1066,17 @@ public abstract class BaseReceivePack {
 		PushCertificateParser certParser = getPushCertificateParser();
 		FirstLine firstLine = null;
 		for (;;) {
-			String rawLine;
+			String line;
 			try {
-				rawLine = pckIn.readStringRaw();
+				line = pckIn.readString();
 			} catch (EOFException eof) {
 				if (commands.isEmpty())
 					return;
 				throw eof;
 			}
-			if (rawLine == PacketLineIn.END) {
+			if (line == PacketLineIn.END) {
 				break;
 			}
-			String line = chomp(rawLine);
 
 			if (line.length() >= 48 && line.startsWith("shallow ")) { //$NON-NLS-1$
 				clientShallowCommits.add(ObjectId.fromString(line.substring(8, 48)));
@@ -1095,7 +1094,7 @@ public abstract class BaseReceivePack {
 				}
 			}
 
-			if (rawLine.equals(PushCertificateParser.BEGIN_SIGNATURE)) {
+			if (line.equals(PushCertificateParser.BEGIN_SIGNATURE)) {
 				certParser.receiveSignature(pckIn);
 				continue;
 			}
@@ -1114,21 +1113,11 @@ public abstract class BaseReceivePack {
 			}
 			commands.add(cmd);
 			if (certParser.enabled()) {
-				// Must use raw line with optional newline so signed payload can be
+				certParser.addCommand(cmd);
-				// reconstructed.
-				certParser.addCommand(cmd, rawLine);
 			}
 		}
 	}
 
-	static String chomp(String line) {
-		if (line != null && !line.isEmpty()
-				&& line.charAt(line.length() - 1) == '\n') {
-			return line.substring(0, line.length() - 1);
-		}
-		return line;
-	}
-
 	static ReceiveCommand parseCommand(String line) throws PackProtocolException {
           if (line == null || line.length() < 83) {
 			throw new PackProtocolException(

org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java

@@ -85,12 +85,11 @@ public class PushCertificate {
 	private final String nonce;
 	private final NonceStatus nonceStatus;
 	private final List<ReceiveCommand> commands;
-	private final String rawCommands;
 	private final String signature;
 
 	PushCertificate(String version, PushCertificateIdent pusher, String pushee,
 			String nonce, NonceStatus nonceStatus, List<ReceiveCommand> commands,
-			String rawCommands, String signature) {
+			String signature) {
 		if (version == null || version.isEmpty()) {
 			throw new IllegalArgumentException(MessageFormat.format(
 					JGitText.get().pushCertificateInvalidField, VERSION));
@@ -112,8 +111,7 @@ public class PushCertificate {
 					JGitText.get().pushCertificateInvalidField,
 					"nonce status")); //$NON-NLS-1$
 		}
-		if (commands == null || commands.isEmpty()
+		if (commands == null || commands.isEmpty()) {
-				|| rawCommands == null || rawCommands.isEmpty()) {
 			throw new IllegalArgumentException(MessageFormat.format(
 					JGitText.get().pushCertificateInvalidField,
 					"command")); //$NON-NLS-1$
@@ -123,7 +121,7 @@ public class PushCertificate {
 					JGitText.get().pushCertificateInvalidSignature);
 		}
 		if (!signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE)
-				|| !signature.endsWith(PushCertificateParser.END_SIGNATURE)) {
+				|| !signature.endsWith(PushCertificateParser.END_SIGNATURE + '\n')) {
 			throw new IllegalArgumentException(
 					JGitText.get().pushCertificateInvalidSignature);
 		}
@@ -133,7 +131,6 @@ public class PushCertificate {
 		this.nonce = nonce;
 		this.nonceStatus = nonceStatus;
 		this.commands = commands;
-		this.rawCommands = rawCommands;
 		this.signature = signature;
 	}
 
@@ -209,14 +206,18 @@ public class PushCertificate {
 	 * @since 4.1
 	 */
 	public String toText() {
-		return new StringBuilder()
+		StringBuilder sb = new StringBuilder()
 				.append(VERSION).append(' ').append(version).append('\n')
 				.append(PUSHER).append(' ').append(getPusher())
 				.append('\n')
 				.append(PUSHEE).append(' ').append(pushee).append('\n')
 				.append(NONCE).append(' ').append(nonce).append('\n')
-				.append('\n')
+				.append('\n');
-				.append(rawCommands)
+		for (ReceiveCommand cmd : commands) {
-				.toString();
+			sb.append(cmd.getOldId().name())
+				.append(' ').append(cmd.getNewId().name())
+				.append(' ').append(cmd.getRefName()).append('\n');
+		}
+		return sb.toString();
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java

@@ -42,7 +42,6 @@
  */
 package org.eclipse.jgit.transport;
 
-import static org.eclipse.jgit.transport.BaseReceivePack.chomp;
 import static org.eclipse.jgit.transport.BaseReceivePack.parseCommand;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_PUSH_CERT;
 
@@ -66,9 +65,9 @@ import org.eclipse.jgit.transport.PushCertificate.NonceStatus;
  */
 public class PushCertificateParser {
 	static final String BEGIN_SIGNATURE =
-			"-----BEGIN PGP SIGNATURE-----\n"; //$NON-NLS-1$
+			"-----BEGIN PGP SIGNATURE-----"; //$NON-NLS-1$
 	static final String END_SIGNATURE =
-			"-----END PGP SIGNATURE-----\n"; //$NON-NLS-1$
+			"-----END PGP SIGNATURE-----"; //$NON-NLS-1$
 
 	static final String VERSION = "certificate version"; //$NON-NLS-1$
 
@@ -80,7 +79,7 @@ public class PushCertificateParser {
 
 	private static final String VERSION_0_1 = "0.1"; //$NON-NLS-1$
 
-	private static final String END_CERT = "push-cert-end\n"; //$NON-NLS-1$
+	private static final String END_CERT = "push-cert-end"; //$NON-NLS-1$
 
 	private boolean received;
 	private String version;
@@ -112,7 +111,6 @@ public class PushCertificateParser {
 
 	private final NonceGenerator nonceGenerator;
 	private final List<ReceiveCommand> commands;
-	private final StringBuilder rawCommands;
 
 	PushCertificateParser(Repository into, SignedPushConfig cfg) {
 		if (cfg != null) {
@@ -124,7 +122,6 @@ public class PushCertificateParser {
 		}
 		db = into;
 		commands = new ArrayList<>();
-		rawCommands = new StringBuilder();
 	}
 
 	/**
@@ -139,8 +136,7 @@ public class PushCertificateParser {
 		}
 		try {
 			return new PushCertificate(version, pusher, pushee, receivedNonce,
-					nonceStatus, Collections.unmodifiableList(commands),
+					nonceStatus, Collections.unmodifiableList(commands), signature);
-					rawCommands.toString(), signature);
 		} catch (IllegalArgumentException e) {
 			throw new IOException(e.getMessage(), e);
 		}
@@ -244,9 +240,9 @@ public class PushCertificateParser {
 	 * Read the PGP signature.
 	 * <p>
 	 * This method assumes the line
-	 * {@code "-----BEGIN PGP SIGNATURE-----\n"} has already been parsed,
+	 * {@code "-----BEGIN PGP SIGNATURE-----"} has already been parsed,
-	 * and continues parsing until an {@code "-----END PGP SIGNATURE-----\n"} is
+	 * and continues parsing until an {@code "-----END PGP SIGNATURE-----"} is
-	 * found, followed by {@code "push-cert-end\n"}.
+	 * found, followed by {@code "push-cert-end"}.
 	 *
 	 * @param pckIn
 	 *            where we read the signature from.
@@ -257,13 +253,13 @@ public class PushCertificateParser {
 	public void receiveSignature(PacketLineIn pckIn) throws IOException {
 		received = true;
 		try {
-			StringBuilder sig = new StringBuilder(BEGIN_SIGNATURE);
+			StringBuilder sig = new StringBuilder(BEGIN_SIGNATURE).append('\n');
 			String line;
-			while (!(line = pckIn.readStringRaw()).equals(END_SIGNATURE)) {
+			while (!(line = pckIn.readString()).equals(END_SIGNATURE)) {
-				sig.append(line);
+				sig.append(line).append('\n');
 			}
-			signature = sig.append(END_SIGNATURE).toString();
+			signature = sig.append(END_SIGNATURE).append('\n').toString();
-			if (!pckIn.readStringRaw().equals(END_CERT)) {
+			if (!pckIn.readString().equals(END_CERT)) {
 				throw new PackProtocolException(
 						JGitText.get().pushCertificateInvalidSignature);
 			}
@@ -278,28 +274,23 @@ public class PushCertificateParser {
 	 *
 	 * @param cmd
 	 *            the command.
-	 * @param rawLine
-	 *            the exact line read from the wire that produced this
-	 *            command, including trailing newline if present.
 	 * @since 4.1
 	 */
-	public void addCommand(ReceiveCommand cmd, String rawLine) {
+	public void addCommand(ReceiveCommand cmd) {
 		commands.add(cmd);
-		rawCommands.append(rawLine);
 	}
 
 	/**
 	 * Add a command to the signature.
 	 *
-	 * @param rawLine
+	 * @param line
-	 *            the exact line read from the wire that produced this
+	 *            the line read from the wire that produced this
-	 *            command, including trailing newline if present.
+	 *            command, with optional trailing newline already trimmed.
 	 * @throws PackProtocolException
 	 *             if the raw line cannot be parsed to a command.
 	 * @since 4.0
 	 */
-	public void addCommand(String rawLine) throws PackProtocolException {
+	public void addCommand(String line) throws PackProtocolException {
-		commands.add(parseCommand(chomp(rawLine)));
+		commands.add(parseCommand(line));
-		rawCommands.append(rawLine);
 	}
 }