From 29fc7e87c6c961605825e3d15c69ad11d8f33e51 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 12 Apr 2018 11:43:50 -0400 Subject: [PATCH] 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 --- .../jgit/transport/PushConnectionTest.java | 60 ++++++++++++++++++- .../eclipse/jgit/transport/PushProcess.java | 5 +- .../org/eclipse/jgit/transport/Transport.java | 4 +- 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 c16c1b2a9..63478f6f9 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 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() { @@ -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 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 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 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 remoteRefNames(Collection updates) { + List 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 3201732a9..0ade84a04 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 prepareRemoteUpdates() throws TransportException { boolean atomic = transport.isPushAtomic(); - final Map result = new HashMap<>(); + final Map 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 afefbff5d..7625ba734 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 specs) throws IOException { final Map localRefs = db.getRefDatabase().getRefs(ALL); - final Collection procRefs = new HashSet<>(); + final Collection procRefs = new LinkedHashSet<>(); for (final RefSpec spec : specs) { if (spec.isWildcard()) {