Push: Ensure ref updates are processed in input order

Various places on the client side of the push were creating unordered
maps and sets of ref names, resulting in ReceivePack processing commands
in an order other than what the client provided. This is normally not
problematic for clients, who don't typically care about the order in
which ref updates are applied to the storage layer.

However, it does make it difficult to write deterministic tests of
ReceivePack or hooks whose output depends on the order in which commands
are processed, for example if informational per-ref messages are written
to a sideband.[1]

Add a test that ensures the ordering of commands both internally in
ReceivePack and in the output PushResult.

[1] Real-world example:
    https://gerrit-review.googlesource.com/c/gerrit/+/171871/1/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java#149

Change-Id: I7f1254b4ebf202d4dcfc8e59d7120427542d0d9e

3 files changed, 64 insertions, 5 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
index c16c1b2a93..63478f6f92 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
@@ -51,12 +51,16 @@ import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
@@ -76,6 +80,7 @@ public class PushConnectionTest {
 	private Object ctx = new Object();
 	private InMemoryRepository server;
 	private InMemoryRepository client;
+	private List<String> processedRefs;
 	private ObjectId obj1;
 	private ObjectId obj2;
 	private ObjectId obj3;
@@ -85,6 +90,7 @@ public class PushConnectionTest {
 	public void setUp() throws Exception {
 		server = newRepo("server");
 		client = newRepo("client");
+		processedRefs = new ArrayList<>();
 		testProtocol = new TestProtocol<>(
 				null,
 				new ReceivePackFactory<Object>() {
@@ -92,7 +98,18 @@ public class PushConnectionTest {
 					public ReceivePack create(Object req, Repository db)
 							throws ServiceNotEnabledException,
 							ServiceNotAuthorizedException {
-						return new ReceivePack(db);
+						ReceivePack rp = new ReceivePack(db);
+						rp.setPreReceiveHook(
+								new PreReceiveHook() {
+									@Override
+									public void onPreReceive(ReceivePack receivePack,
+											Collection<ReceiveCommand> cmds) {
+										for (ReceiveCommand cmd : cmds) {
+											processedRefs.add(cmd.getRefName());
+										}
+									}
+								});
+						return rp;
 					}
 				});
 		uri = testProtocol.register(ctx, server);
@@ -196,4 +213,45 @@ public class PushConnectionTest {
 			}
 		}
 	}
+
+	@Test
+	public void commandOrder() throws Exception {
+		TestRepository<?> tr = new TestRepository<>(client);
+		List<RemoteRefUpdate> updates = new ArrayList<>();
+		// Arbitrary non-sorted order.
+		for (int i = 9; i >= 0; i--) {
+			String name = "refs/heads/b" + i;
+			tr.branch(name).commit().create();
+			RemoteRefUpdate rru = new RemoteRefUpdate(client, name, name, false, null,
+					ObjectId.zeroId());
+			updates.add(rru);
+		}
+
+		PushResult result;
+		try (Transport tn = testProtocol.open(uri, client, "server")) {
+			result = tn.push(NullProgressMonitor.INSTANCE, updates);
+		}
+
+		for (RemoteRefUpdate remoteUpdate : result.getRemoteUpdates()) {
+			assertEquals(
+					"update should succeed on " + remoteUpdate.getRemoteName(),
+					RemoteRefUpdate.Status.OK, remoteUpdate.getStatus());
+		}
+
+		List<String> expected = remoteRefNames(updates);
+		assertEquals(
+				"ref names processed by ReceivePack should match input ref names in order",
+				expected, processedRefs);
+		assertEquals(
+				"remote ref names should match input ref names in order",
+				expected, remoteRefNames(result.getRemoteUpdates()));
+	}
+
+	private static List<String> remoteRefNames(Collection<RemoteRefUpdate> updates) {
+		List<String> result = new ArrayList<>();
+		for (RemoteRefUpdate u : updates) {
+			result.add(u.getRemoteName());
+		}
+		return result;
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
index 3201732a98..0ade84a048 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
@@ -49,6 +49,7 @@ import java.text.MessageFormat;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -124,7 +125,7 @@ class PushProcess {
 			throws TransportException {
 		this.walker = new RevWalk(transport.local);
 		this.transport = transport;
-		this.toPush = new HashMap<>();
+		this.toPush = new LinkedHashMap<>();
 		this.out = out;
 		this.pushOptions = transport.getPushOptions();
 		for (final RemoteRefUpdate rru : toPush) {
@@ -190,7 +191,7 @@ class PushProcess {
 	private Map<String, RemoteRefUpdate> prepareRemoteUpdates()
 			throws TransportException {
 		boolean atomic = transport.isPushAtomic();
-		final Map<String, RemoteRefUpdate> result = new HashMap<>();
+		final Map<String, RemoteRefUpdate> result = new LinkedHashMap<>();
 		for (final RemoteRefUpdate rru : toPush.values()) {
 			final Ref advertisedRef = connection.getRef(rru.getRemoteName());
 			ObjectId advertisedOld = null;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java
index afefbff5d8..7625ba7346 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java
@@ -64,7 +64,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -690,7 +690,7 @@ public abstract class Transport implements AutoCloseable {
 			final Repository db, final Collection<RefSpec> specs)
 			throws IOException {
 		final Map<String, Ref> localRefs = db.getRefDatabase().getRefs(ALL);
-		final Collection<RefSpec> procRefs = new HashSet<>();
+		final Collection<RefSpec> procRefs = new LinkedHashSet<>();
 
 		for (final RefSpec spec : specs) {
 			if (spec.isWildcard()) {