Merge "Fix show +10↑ to not expand all"
diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java
index 1f11921..3854310 100644
--- a/java/com/google/gerrit/reviewdb/client/RefNames.java
+++ b/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -123,6 +123,11 @@
return shard(id.get(), r).append(META_SUFFIX).toString();
}
+ public static String patchSetRef(PatchSet.Id id) {
+ StringBuilder r = newStringBuilder().append(REFS_CHANGES);
+ return shard(id.changeId().get(), r).append('/').append(id.get()).toString();
+ }
+
public static String robotCommentsRef(Change.Id id) {
StringBuilder r = newStringBuilder().append(REFS_CHANGES);
return shard(id.get(), r).append(ROBOT_COMMENTS_SUFFIX).toString();
diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index 66e66ca..b0f8e54 100644
--- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
@@ -35,12 +35,10 @@
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.ReceiveCommitsExecutor;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
import com.google.gerrit.server.git.MultiProgressMonitor;
import com.google.gerrit.server.git.ProjectRunnable;
import com.google.gerrit.server.git.TransferConfig;
import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.ContributorAgreementsChecker;
@@ -58,7 +56,6 @@
import com.google.inject.name.Named;
import java.io.IOException;
import java.io.OutputStream;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.ExecutionException;
@@ -66,8 +63,6 @@
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.transport.AdvertiseRefsHook;
-import org.eclipse.jgit.transport.AdvertiseRefsHookChain;
import org.eclipse.jgit.transport.PreReceiveHook;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.transport.ReceiveCommand.Result;
@@ -297,15 +292,10 @@
receiveConfig.checkReferencedObjectsAreReachable);
}
- List<AdvertiseRefsHook> advHooks = new ArrayList<>(4);
allRefsWatcher = new AllRefsWatcher();
- advHooks.add(allRefsWatcher);
- advHooks.add(
- new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build()));
- advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName));
- advHooks.add(new HackPushNegotiateHook());
- receivePack.setAdvertiseRefsHook(AdvertiseRefsHookChain.newChain(advHooks));
-
+ receivePack.setAdvertiseRefsHook(
+ ReceiveCommitsAdvertiseRefsHookChain.create(
+ allRefsWatcher, perm, queryProvider, projectName));
resultChangeIds = new ResultChangeIds();
receiveCommits =
factory.create(
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
index f7e0078..2ea417e 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
@@ -14,9 +14,8 @@
package com.google.gerrit.server.git.receive;
-import com.google.auto.value.AutoValue;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.Maps;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
@@ -29,28 +28,44 @@
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.util.MagicBranch;
import com.google.inject.Provider;
+import java.io.IOException;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.AdvertiseRefsHook;
import org.eclipse.jgit.transport.BaseReceivePack;
import org.eclipse.jgit.transport.ServiceMayNotContinueException;
import org.eclipse.jgit.transport.UploadPack;
-/** Exposes only the non refs/changes/ reference names. */
+/**
+ * Exposes only the non refs/changes/ reference names and provide additional haves.
+ *
+ * <p>Negotiation on Git push is suboptimal in that it tends to send more objects to the server than
+ * it should. This results in increased latencies for {@code git push}.
+ *
+ * <p>Ref advertisement for Git pushes still works in a "the server speaks first fashion" as Git
+ * Protocol V2 only addressed fetches Therefore the server needs to send all available references.
+ * For large repositories, this can be in the tens of megabytes to send to the client. We therefore
+ * remove all refs in refs/changes/* to scale down that footprint. Trivially, this would increase
+ * the unnecessary objects that the client has to send to the server because the common ancestor it
+ * finds in negotiation might be further back in history.
+ *
+ * <p>To work around this, we advertise the last 32 changes in that repository as additional {@code
+ * .haves}. This is a heuristical approach that aims at scaling down the number of unnecessary
+ * objects that client sends to the server. Unnecessary here refers to objects that the server
+ * already has.
+ *
+ * <p>For some code paths in {@link com.google.gerrit.server.git.DefaultAdvertiseRefsHook}, we
+ * already removed refs/changes, so the logic to skip these in this class become a no-op.
+ *
+ * <p>TODO(hiesel): Instrument this heuristic and proof its value.
+ */
public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- @VisibleForTesting
- @AutoValue
- public abstract static class Result {
- public abstract Map<String, Ref> allRefs();
-
- public abstract Set<ObjectId> additionalHaves();
- }
-
private final Provider<InternalChangeQuery> queryProvider;
private final Project.NameKey projectName;
@@ -68,28 +83,16 @@
@Override
public void advertiseRefs(BaseReceivePack rp) throws ServiceMayNotContinueException {
- Result r = advertiseRefs(HookUtil.ensureAllRefsAdvertised(rp));
- rp.setAdvertisedRefs(r.allRefs(), r.additionalHaves());
+ Map<String, Ref> advertisedRefs = HookUtil.ensureAllRefsAdvertised(rp);
+ advertisedRefs.keySet().stream()
+ .filter(ReceiveCommitsAdvertiseRefsHook::skip)
+ .collect(toImmutableList())
+ .forEach(r -> advertisedRefs.remove(r));
+ rp.setAdvertisedRefs(advertisedRefs, advertiseOpenChanges(rp.getRepository()));
}
- @VisibleForTesting
- public Result advertiseRefs(Map<String, Ref> oldRefs) {
- Map<String, Ref> r = Maps.newHashMapWithExpectedSize(oldRefs.size());
- Set<ObjectId> allPatchSets = Sets.newHashSetWithExpectedSize(oldRefs.size());
- for (Map.Entry<String, Ref> e : oldRefs.entrySet()) {
- String name = e.getKey();
- if (!skip(name)) {
- r.put(name, e.getValue());
- }
- if (name.startsWith(RefNames.REFS_CHANGES)) {
- allPatchSets.add(e.getValue().getObjectId());
- }
- }
- return new AutoValue_ReceiveCommitsAdvertiseRefsHook_Result(
- r, advertiseOpenChanges(allPatchSets));
- }
-
- private Set<ObjectId> advertiseOpenChanges(Set<ObjectId> allPatchSets) {
+ private Set<ObjectId> advertiseOpenChanges(Repository repo)
+ throws ServiceMayNotContinueException {
// Advertise some recent open changes, in case a commit is based on one.
int limit = 32;
try {
@@ -111,11 +114,17 @@
// Ensure we actually observed a patch set ref pointing to this
// object, in case the database is out of sync with the repo and the
// object doesn't actually exist.
- if (allPatchSets.contains(ps.commitId())) {
- r.add(ps.commitId());
+ try {
+ Ref psRef = repo.getRefDatabase().exactRef(RefNames.patchSetRef(ps.id()));
+ if (psRef != null) {
+ r.add(ps.commitId());
+ }
+ } catch (IOException e) {
+ throw new ServiceMayNotContinueException(e);
}
}
}
+
return r;
} catch (StorageException err) {
logger.atSevere().withCause(err).log("Cannot list open changes of %s", projectName);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java
new file mode 100644
index 0000000..d10fc9a
--- /dev/null
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java
@@ -0,0 +1,76 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git.receive;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.inject.Provider;
+import java.util.ArrayList;
+import java.util.List;
+import org.eclipse.jgit.transport.AdvertiseRefsHook;
+import org.eclipse.jgit.transport.AdvertiseRefsHookChain;
+
+/**
+ * Helper to ensure that the chain for advertising refs is the same in tests and production code.
+ */
+public class ReceiveCommitsAdvertiseRefsHookChain {
+
+ /**
+ * Returns a single {@link AdvertiseRefsHook} that encompasses a chain of {@link
+ * AdvertiseRefsHook} to be used for advertising when processing a Git push.
+ */
+ public static AdvertiseRefsHook create(
+ AllRefsWatcher allRefsWatcher,
+ PermissionBackend.ForProject perm,
+ Provider<InternalChangeQuery> queryProvider,
+ Project.NameKey projectName) {
+ return create(allRefsWatcher, perm, queryProvider, projectName, false);
+ }
+
+ /**
+ * Returns a single {@link AdvertiseRefsHook} that encompasses a chain of {@link
+ * AdvertiseRefsHook} to be used for advertising when processing a Git push. Omits {@link
+ * HackPushNegotiateHook} as that does not advertise refs on it's own but adds {@code .have} based
+ * on history which is not relevant for the tests we have.
+ */
+ @VisibleForTesting
+ public static AdvertiseRefsHook createForTest(
+ PermissionBackend.ForProject perm,
+ Provider<InternalChangeQuery> queryProvider,
+ Project.NameKey projectName) {
+ return create(new AllRefsWatcher(), perm, queryProvider, projectName, true);
+ }
+
+ private static AdvertiseRefsHook create(
+ AllRefsWatcher allRefsWatcher,
+ PermissionBackend.ForProject perm,
+ Provider<InternalChangeQuery> queryProvider,
+ Project.NameKey projectName,
+ boolean skipHackPushNegotiateHook) {
+ List<AdvertiseRefsHook> advHooks = new ArrayList<>();
+ advHooks.add(allRefsWatcher);
+ advHooks.add(
+ new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build()));
+ advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName));
+ if (!skipHackPushNegotiateHook) {
+ advHooks.add(new HackPushNegotiateHook());
+ }
+ return AdvertiseRefsHookChain.newChain(advHooks);
+ }
+}
diff --git a/java/com/google/gerrit/server/git/receive/testing/BUILD b/java/com/google/gerrit/server/git/receive/testing/BUILD
new file mode 100644
index 0000000..82cd14b
--- /dev/null
+++ b/java/com/google/gerrit/server/git/receive/testing/BUILD
@@ -0,0 +1,12 @@
+java_library(
+ name = "testing",
+ testonly = 1,
+ srcs = glob(["**/*.java"]),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//lib:guava",
+ "//lib/auto:auto-value",
+ "//lib/auto:auto-value-annotations",
+ "//lib/jgit/org.eclipse.jgit:jgit",
+ ],
+)
diff --git a/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java b/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java
new file mode 100644
index 0000000..c54ab25
--- /dev/null
+++ b/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java
@@ -0,0 +1,87 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git.receive.testing;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Splitter;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.StreamSupport;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.RefAdvertiser;
+
+/** Helper to collect advertised refs and additonal haves and verify them in tests. */
+public class TestRefAdvertiser extends RefAdvertiser {
+
+ @VisibleForTesting
+ @AutoValue
+ public abstract static class Result {
+ public abstract Map<String, Ref> allRefs();
+
+ public abstract Set<ObjectId> additionalHaves();
+
+ public static Result create(Map<String, Ref> allRefs, Set<ObjectId> additionalHaves) {
+ return new AutoValue_TestRefAdvertiser_Result(allRefs, additionalHaves);
+ }
+ }
+
+ private final Map<String, Ref> advertisedRefs;
+ private final Set<ObjectId> additionalHaves;
+ private final Repository repo;
+
+ public TestRefAdvertiser(Repository repo) {
+ advertisedRefs = new HashMap<>();
+ additionalHaves = new HashSet<>();
+ this.repo = repo;
+ }
+
+ @Override
+ protected void writeOne(CharSequence line) throws IOException {
+ List<String> lineParts =
+ StreamSupport.stream(Splitter.on(' ').split(line).spliterator(), false)
+ .map(String::trim)
+ .collect(toImmutableList());
+ if (".have".equals(lineParts.get(1))) {
+ additionalHaves.add(ObjectId.fromString(lineParts.get(0)));
+ } else {
+ ObjectId id = ObjectId.fromString(lineParts.get(0));
+ Ref ref =
+ repo.getRefDatabase().getRefs().stream()
+ .filter(r -> r.getObjectId().equals(id))
+ .findAny()
+ .orElseThrow(
+ () ->
+ new RuntimeException(
+ line.toString() + " does not conform to expected pattern"));
+ advertisedRefs.put(lineParts.get(1), ref);
+ }
+ }
+
+ @Override
+ protected void end() {}
+
+ public Result result() {
+ return Result.create(advertisedRefs, additionalHaves);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/git/BUILD b/javatests/com/google/gerrit/acceptance/git/BUILD
index 96710e2..0de307a 100644
--- a/javatests/com/google/gerrit/acceptance/git/BUILD
+++ b/javatests/com/google/gerrit/acceptance/git/BUILD
@@ -8,6 +8,7 @@
":push_for_review",
":submodule_util",
"//java/com/google/gerrit/git",
+ "//java/com/google/gerrit/server/git/receive/testing",
"//lib/commons:lang",
],
) for f in glob(["*IT.java"])]
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 811ef35..a3017b6 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -50,7 +50,8 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.git.receive.ReceiveCommitsAdvertiseRefsHook;
+import com.google.gerrit.server.git.receive.ReceiveCommitsAdvertiseRefsHookChain;
+import com.google.gerrit.server.git.receive.testing.TestRefAdvertiser;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.Sequences;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -75,6 +76,8 @@
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.AdvertiseRefsHook;
+import org.eclipse.jgit.transport.ReceivePack;
import org.junit.Before;
import org.junit.Test;
@@ -544,11 +547,10 @@
@Test
public void receivePackListsOpenChangesAsAdditionalHaves() throws Exception {
- ReceiveCommitsAdvertiseRefsHook.Result r = getReceivePackRefs();
+ TestRefAdvertiser.Result r = getReceivePackRefs(admin);
assertThat(r.allRefs().keySet())
.containsExactly(
// meta refs are excluded
- "HEAD",
"refs/heads/branch",
"refs/heads/master",
"refs/meta/config",
@@ -568,7 +570,7 @@
.update();
requestScopeOperations.setApiUser(user.id());
- assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 1));
+ assertThat(getReceivePackRefs(user).additionalHaves()).containsExactly(obj(cd3, 1));
}
@Test
@@ -577,7 +579,8 @@
PushOneCommit.Result r = amendChange(cd3.change().getKey().get());
r.assertOkStatus();
cd3 = r.getChange();
- assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 2), obj(cd4, 1));
+ assertThat(getReceivePackRefs(admin).additionalHaves())
+ .containsExactly(obj(cd3, 2), obj(cd4, 1));
}
@Test
@@ -615,7 +618,7 @@
indexer.index(c.getProject(), c.getId());
}
- assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd4, 1));
+ assertThat(getReceivePackRefs(admin).additionalHaves()).containsExactly(obj(cd4, 1));
}
@Test
@@ -961,11 +964,16 @@
}
}
- private ReceiveCommitsAdvertiseRefsHook.Result getReceivePackRefs() throws Exception {
- ReceiveCommitsAdvertiseRefsHook hook =
- new ReceiveCommitsAdvertiseRefsHook(queryProvider, project);
+ private TestRefAdvertiser.Result getReceivePackRefs(TestAccount u) throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
- return hook.advertiseRefs(getAllRefs(repo));
+ AdvertiseRefsHook adv =
+ ReceiveCommitsAdvertiseRefsHookChain.createForTest(
+ newFilter(project, u), queryProvider, project);
+ ReceivePack rp = new ReceivePack(repo);
+ rp.setAdvertiseRefsHook(adv);
+ TestRefAdvertiser advertiser = new TestRefAdvertiser(repo);
+ rp.sendAdvertisedRefs(advertiser);
+ return advertiser.result();
}
}
