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

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 c16c1b2a93e052bcebd89c23ae16a1b9cb1d526c..63478f6f92bb7988a6b3341f1a5b2e18ebfacd78 100644 (file)
--- 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 3201732a989ec2a6ba4acf2edf2b3fe2c8ba0da0..0ade84a048505f708283582b656712e3cb45a3b3 100644 (file)
--- 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 afefbff5d83d9b81b62dc0cd76867424977831e4..7625ba734635bdc4fa2bcaa3ea4e3eaebe8c42b1 100644 (file)
--- 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()) {