ReceivePack: refactor push option parsing

Refactor all of the push option support code to allocate the list
immediately before parsing the options section off the stream.

Move option support down to ReceivePack instead of BaseReceivePack.
Push options are specific to the ReceivePack protocol and are not
likely to appear in the 4 year old subscription proposal.  These
changes are OK before JGit 4.5 ships as no consumer should be relying
on these new APIs.

Change-Id: Ib07d18c877628aba07da07cd91875f918d509c49

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

@@ -84,7 +84,7 @@ public class PushOptionsTest extends RepositoryTestCase {
 	private InMemoryRepository client;
 	private ObjectId obj1;
 	private ObjectId obj2;
-	private BaseReceivePack baseReceivePack;
+	private ReceivePack receivePack;
 
 	@Before
 	public void setUp() throws Exception {
@@ -96,13 +96,12 @@ public class PushOptionsTest extends RepositoryTestCase {
 		testProtocol = new TestProtocol<>(null,
 				new ReceivePackFactory<Object>() {
 					@Override
-					public ReceivePack create(Object req, Repository database)
+					public ReceivePack create(Object req, Repository git)
 							throws ServiceNotEnabledException,
 							ServiceNotAuthorizedException {
-						ReceivePack receivePack = new ReceivePack(database);
+						receivePack = new ReceivePack(git);
 						receivePack.setAllowPushOptions(true);
 						receivePack.setAtomic(true);
-						baseReceivePack = receivePack;
 						return receivePack;
 					}
 				});
@@ -118,7 +117,6 @@ public class PushOptionsTest extends RepositoryTestCase {
 
 	@After
 	public void tearDown() {
-		baseReceivePack = null;
 		Transport.unregister(testProtocol);
 	}
 
@@ -176,7 +174,7 @@ public class PushOptionsTest extends RepositoryTestCase {
 		assertSame(RemoteRefUpdate.Status.OK, one.getStatus());
 		assertSame(RemoteRefUpdate.Status.REJECTED_REMOTE_CHANGED,
 				two.getStatus());
-		assertEquals(pushOptions, baseReceivePack.getPushOptions());
+		assertEquals(pushOptions, receivePack.getPushOptions());
 	}
 
 	@Test
@@ -197,7 +195,7 @@ public class PushOptionsTest extends RepositoryTestCase {
 
 		assertSame(RemoteRefUpdate.Status.OK, one.getStatus());
 		assertSame(RemoteRefUpdate.Status.OK, two.getStatus());
-		assertEquals(pushOptions, baseReceivePack.getPushOptions());
+		assertEquals(pushOptions, receivePack.getPushOptions());
 	}
 
 	@Test
@@ -220,7 +218,7 @@ public class PushOptionsTest extends RepositoryTestCase {
 				one.getStatus());
 		assertSame(RemoteRefUpdate.Status.REJECTED_REMOTE_CHANGED,
 				two.getStatus());
-		assertNull(baseReceivePack.getPushOptions());
+		assertNull(receivePack.getPushOptions());
 	}
 
 	@Test
@@ -241,7 +239,7 @@ public class PushOptionsTest extends RepositoryTestCase {
 		assertSame(RemoteRefUpdate.Status.OK, one.getStatus());
 		assertSame(RemoteRefUpdate.Status.REJECTED_REMOTE_CHANGED,
 				two.getStatus());
-		assertEquals(pushOptions, baseReceivePack.getPushOptions());
+		assertEquals(pushOptions, receivePack.getPushOptions());
 	}
 
 	@Test
@@ -360,4 +358,4 @@ public class PushOptionsTest extends RepositoryTestCase {
 			fail("should already have thrown TransportException");
 		}
 	}
-}
+}

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

@@ -46,9 +46,9 @@ package org.eclipse.jgit.transport;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_ATOMIC;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_DELETE_REFS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_OFS_DELTA;
+import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_PUSH_OPTIONS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_QUIET;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REPORT_STATUS;
-import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_PUSH_OPTIONS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT;
 import static org.eclipse.jgit.transport.SideBandOutputStream.CH_DATA;
@@ -69,7 +69,6 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
-import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.PackProtocolException;
@@ -252,18 +251,6 @@ public abstract class BaseReceivePack {
 
 	private boolean quiet;
 
-	/**
-	 * A list of option strings associated with a push.
-	 * @since 4.5
-	 */
-	protected List<String> pushOptions;
-
-	/**
-	 * Whether the client intends to use push options.
-	 * @since 4.5
-	 */
-	protected boolean usePushOptions;
-
 	/** Lock around the received pack file, while updating refs. */
 	private PackLock packLock;
 
@@ -782,8 +769,7 @@ public abstract class BaseReceivePack {
 	 *             read.
 	 */
 	public boolean isSideBand() throws RequestNotYetReadException {
-		if (enabledCapabilities == null)
+		checkRequestWasRead();
-			throw new RequestNotYetReadException();
 		return enabledCapabilities.contains(CAPABILITY_SIDE_BAND_64K);
 	}
 
@@ -810,7 +796,7 @@ public abstract class BaseReceivePack {
 	}
 
 	/**
-	 * @return true if the server supports the receiving of push options.
+	 * @return true if the server supports receiving push options.
 	 * @since 4.5
 	 */
 	public boolean isAllowPushOptions() {
@@ -818,10 +804,10 @@ public abstract class BaseReceivePack {
 	}
 
 	/**
-	 * Configure if the server supports the receiving of push options.
+	 * Configure if the server supports receiving push options.
 	 *
 	 * @param allow
-	 *            true to permit option strings.
+	 *            true to optionally accept option strings from the client.
 	 * @since 4.5
 	 */
 	public void setAllowPushOptions(boolean allow) {
@@ -840,44 +826,10 @@ public abstract class BaseReceivePack {
 	 * @since 4.0
 	 */
 	public boolean isQuiet() throws RequestNotYetReadException {
-		if (enabledCapabilities == null)
+		checkRequestWasRead();
-			throw new RequestNotYetReadException();
 		return quiet;
 	}
 
-	/**
-	 * Gets an unmodifiable view of the option strings associated with the push.
-	 *
-	 * @return an unmodifiable view of pushOptions, or null (if pushOptions is).
-	 * @throws IllegalStateException
-	 *             if allowPushOptions has not been set to true.
-	 * @throws RequestNotYetReadException
-	 *             if the client's request has not yet been read from the wire,
-	 *             so we do not know if they expect push options. Note that the
-	 *             client may have already written the request, it just has not
-	 *             been read.
-	 * @since 4.5
-	 */
-	@Nullable
-	public List<String> getPushOptions() {
-		if (!allowPushOptions) {
-			// Reading push options without a prior setAllowPushOptions(true)
-			// call doesn't make sense.
-			throw new IllegalStateException();
-		}
-		if (enabledCapabilities == null) {
-			// Push options are not available until receive() has been called.
-			throw new RequestNotYetReadException();
-		}
-		if (pushOptions == null) {
-			// The client doesn't support push options. Return null to
-			// distinguish this from the case where the client declared support
-			// for push options and sent an empty list of them.
-			return null;
-		}
-		return Collections.unmodifiableList(pushOptions);
-	}
-
 	/**
 	 * Set the configuration for push certificate verification.
 	 *
@@ -1269,11 +1221,6 @@ public abstract class BaseReceivePack {
 	protected void enableCapabilities() {
 		sideBand = isCapabilityEnabled(CAPABILITY_SIDE_BAND_64K);
 		quiet = allowQuiet && isCapabilityEnabled(CAPABILITY_QUIET);
-		usePushOptions = allowPushOptions
-				&& isCapabilityEnabled(CAPABILITY_PUSH_OPTIONS);
-		if (usePushOptions) {
-			pushOptions = new ArrayList<>();
-		}
 		if (sideBand) {
 			OutputStream out = rawOut;
 
@@ -1286,17 +1233,6 @@ public abstract class BaseReceivePack {
 		}
 	}
 
-	/**
-	 * Sets the client's intention regarding push options.
-	 *
-	 * @param usePushOptions
-	 *            whether the client intends to use push options
-	 * @since 4.5
-	 */
-	public void setUsePushOptions(boolean usePushOptions) {
-		this.usePushOptions = usePushOptions;
-	}
-
 	/**
 	 * Check if the peer requested a capability.
 	 *
@@ -1308,6 +1244,11 @@ public abstract class BaseReceivePack {
 		return enabledCapabilities.contains(name);
 	}
 
+	void checkRequestWasRead() {
+		if (enabledCapabilities == null)
+			throw new RequestNotYetReadException();
+	}
+
 	/** @return true if a pack is expected based on the list of commands. */
 	protected boolean needPack() {
 		for (final ReceiveCommand cmd : commands) {

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

@@ -44,12 +44,17 @@
 package org.eclipse.jgit.transport;
 
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_ATOMIC;
+import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_PUSH_OPTIONS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REPORT_STATUS;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.UnpackException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
@@ -71,6 +76,10 @@ public class ReceivePack extends BaseReceivePack {
 
 	private boolean echoCommandFailures;
 
+	/** Whether the client intends to use push options. */
+	private boolean usePushOptions;
+	private List<String> pushOptions;
+
 	/**
 	 * Create a new pack receive for an open repository.
 	 *
@@ -83,6 +92,36 @@ public class ReceivePack extends BaseReceivePack {
 		postReceive = PostReceiveHook.NULL;
 	}
 
+	/**
+	 * Gets an unmodifiable view of the option strings associated with the push.
+	 *
+	 * @return an unmodifiable view of pushOptions, or null (if pushOptions is).
+	 * @throws IllegalStateException
+	 *             if allowPushOptions has not been set to true.
+	 * @throws RequestNotYetReadException
+	 *             if the client's request has not yet been read from the wire,
+	 *             so we do not know if they expect push options. Note that the
+	 *             client may have already written the request, it just has not
+	 *             been read.
+	 * @since 4.5
+	 */
+	@Nullable
+	public List<String> getPushOptions() {
+		if (!isAllowPushOptions()) {
+			// Reading push options without a prior setAllowPushOptions(true)
+			// call doesn't make sense.
+			throw new IllegalStateException();
+		}
+		checkRequestWasRead();
+		if (!usePushOptions) {
+			// The client doesn't support push options. Return null to
+			// distinguish this from the case where the client declared support
+			// for push options and sent an empty list of them.
+			return null;
+		}
+		return Collections.unmodifiableList(pushOptions);
+	}
+
 	/** @return the hook invoked before updates occur. */
 	public PreReceiveHook getPreReceiveHook() {
 		return preReceive;
@@ -171,15 +210,18 @@ public class ReceivePack extends BaseReceivePack {
 	@Override
 	protected void enableCapabilities() {
 		reportStatus = isCapabilityEnabled(CAPABILITY_REPORT_STATUS);
+		usePushOptions = isCapabilityEnabled(CAPABILITY_PUSH_OPTIONS);
 		super.enableCapabilities();
 	}
 
 	private void readPushOptions() throws IOException {
-		String pushOption = pckIn.readString();
+		pushOptions = new ArrayList<>(4);
-
+		for (;;) {
-		while (pushOption != PacketLineIn.END) {
+			String option = pckIn.readString();
-			pushOptions.add(pushOption);
+			if (option == PacketLineIn.END) {
-			pushOption = pckIn.readString();
+				break;
+			}
+			pushOptions.add(option);
 		}
 	}