Merge "Fix ReceiveCommitsAdvertiseRefsHook, add docs and test coverage"

java/com/google/gerrit/reviewdb/client/RefNames.java

@@ -123,6 +123,11 @@ public class RefNames {
     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();

java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java

@@ -35,12 +35,10 @@ import com.google.gerrit.server.IdentifiedUser;
 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.assistedinject.FactoryModuleBuilder;
 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.ExecutorService;
 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 @@ public class AsyncReceiveCommits implements PreReceiveHook {
           receiveConfig.checkReferencedObjectsAreReachable);
     }
 
-    List<AdvertiseRefsHook> advHooks = new ArrayList<>(4);
     allRefsWatcher = new AllRefsWatcher();
-    advHooks.add(allRefsWatcher);
+    receivePack.setAdvertiseRefsHook(
-    advHooks.add(
+        ReceiveCommitsAdvertiseRefsHookChain.create(
-        new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build()));
+            allRefsWatcher, perm, queryProvider, projectName));
-    advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName));
-    advHooks.add(new HackPushNegotiateHook());
-    receivePack.setAdvertiseRefsHook(AdvertiseRefsHookChain.newChain(advHooks));
-
     resultChangeIds = new ResultChangeIds();
     receiveCommits =
         factory.create(

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 static com.google.common.collect.ImmutableList.toImmutableList;
-import com.google.common.annotations.VisibleForTesting;
+
-import com.google.common.collect.Maps;
 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.ChangeData;
 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 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
 
   @Override
   public void advertiseRefs(BaseReceivePack rp) throws ServiceMayNotContinueException {
-    Result r = advertiseRefs(HookUtil.ensureAllRefsAdvertised(rp));
+    Map<String, Ref> advertisedRefs = HookUtil.ensureAllRefsAdvertised(rp);
-    rp.setAdvertisedRefs(r.allRefs(), r.additionalHaves());
+    advertisedRefs.keySet().stream()
+        .filter(ReceiveCommitsAdvertiseRefsHook::skip)
+        .collect(toImmutableList())
+        .forEach(r -> advertisedRefs.remove(r));
+    rp.setAdvertisedRefs(advertisedRefs, advertiseOpenChanges(rp.getRepository()));
   }
 
-  @VisibleForTesting
+  private Set<ObjectId> advertiseOpenChanges(Repository repo)
-  public Result advertiseRefs(Map<String, Ref> oldRefs) {
+      throws ServiceMayNotContinueException {
-    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) {
     // Advertise some recent open changes, in case a commit is based on one.
     int limit = 32;
     try {
@@ -111,11 +114,17 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
           // 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())) {
+          try {
-            r.add(ps.commitId());
+            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);

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);
+  }
+}

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",
+    ],
+)

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);
+  }
+}

javatests/com/google/gerrit/acceptance/git/BUILD

@@ -8,6 +8,7 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
         ":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"])]

javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -50,7 +50,8 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 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.Ref;
 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 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
 
   @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 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
         .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 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
     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 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
       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 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
     }
   }
 
-  private ReceiveCommitsAdvertiseRefsHook.Result getReceivePackRefs() throws Exception {
+  private TestRefAdvertiser.Result getReceivePackRefs(TestAccount u) throws Exception {
-    ReceiveCommitsAdvertiseRefsHook hook =
-        new ReceiveCommitsAdvertiseRefsHook(queryProvider, project);
     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();
     }
   }