Fix ReceiveCommitsAdvertiseRefsHook, add docs and test coverage
The tests for receive commits ref advertising were further away from
reality than they should have been. In essence, they tested
ReceiveCommitsAdvertiseRefsHook directly, which let room for bugs to
creep in.
Most prominently, we filter metadata for pushes. As metadata we see
refs/changes/* and refs/cache-automerge/* (suboptimal naming is a
concern not addressed by this commit).
When the previous ref filter has already filtered out change refs
then ReceiveCommitsAdvertiseRefsHook became a no-op and did not
advertise the additional .haves it should because of a check for
existence based on the previously advertised refs.
This commit:
- Fixes the aforementioned bug
- Brings tests closer to reality by including the whole advertising
chain, have JGit do the actual advertising and assert on that
- Document the desired behavior
Change-Id: I230e66eee5b94c6f62b0426e5c16849d993a1cfe
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();
}
}
