Merge "Add a RefFilterOption to always return all most recent ref changes"

java/com/google/gerrit/server/permissions/DefaultRefFilter.java

@@ -27,7 +27,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RefNames;
@@ -57,9 +56,12 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectIdRef;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Ref.Storage;
 import org.eclipse.jgit.lib.Repository;
 
 class DefaultRefFilter {
@@ -82,7 +84,7 @@ class DefaultRefFilter {
   private final Counter0 skipFilterCount;
   private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
 
-  private Map<Change.Id, BranchNameKey> visibleChanges;
+  private Map<Change.Id, ChangeNotes> visibleChanges;
 
   @Inject
   DefaultRefFilter(
@@ -135,6 +137,15 @@ class DefaultRefFilter {
         "Project state %s permits read = %s",
         projectState.getProject().getState(), projectState.statePermitsRead());
 
+    // If we anyway always return all available (most recent) changes in the change index and cache,
+    // we shouldn't care about refs/changes.
+    if (opts.returnMostRecentRefChanges()) {
+      refs =
+          refs.stream()
+              .filter(r -> !RefNames.isRefsChanges(r.getName()))
+              .collect(Collectors.toList());
+    }
+
     // See if we can get away with a single, cheap ref evaluation.
     if (refs.size() == 1) {
       String refName = Iterables.getOnlyElement(refs).getName();
@@ -142,7 +153,7 @@ class DefaultRefFilter {
         logger.atFinest().log("Filter out metadata ref %s", refName);
         return ImmutableList.of();
       }
-      if (RefNames.isRefsChanges(refName)) {
+      if (RefNames.isRefsChanges(refName) && !opts.returnMostRecentRefChanges()) {
         boolean isChangeRefVisisble = canSeeSingleChangeRef(refName);
         if (isChangeRefVisisble) {
           logger.atFinest().log("Change ref %s is visible", refName);
@@ -185,10 +196,31 @@ class DefaultRefFilter {
       }
     }
 
+    if (opts.returnMostRecentRefChanges()) {
+      visibleChanges(repo).values().stream()
+          .forEach(n -> addAllChangeAndPatchsetRefs(visibleRefs, n));
+    }
+
     logger.atFinest().log("visible refs = %s", visibleRefs);
     return visibleRefs;
   }
 
+  private void addAllChangeAndPatchsetRefs(Collection<Ref> refs, ChangeNotes changeNotes) {
+    refs.add(
+        new ObjectIdRef.PeeledNonTag(
+            Storage.PACKED,
+            RefNames.changeMetaRef(changeNotes.getChangeId()),
+            changeNotes.getMetaId()));
+    changeNotes
+        .getPatchSets()
+        .values()
+        .forEach(
+            p ->
+                refs.add(
+                    new ObjectIdRef.PeeledNonTag(
+                        Storage.PACKED, RefNames.patchSetRef(p.id()), p.commitId())));
+  }
+
   /**
    * Filters refs by visibility. Returns tags where visibility can't be trivially computed
    * separately for later rev-walk-based visibility computation. Tags where visibility is trivial to
@@ -256,7 +288,8 @@ class DefaultRefFilter {
       } else if ((changeId = Change.Id.fromRef(refName)) != null) {
         // This is a mere performance optimization. RefVisibilityControl could determine the
         // visibility of these refs just fine. But instead, we use highly-optimized logic that
-        // looks only on the last 10k most recent changes using the change index and a cache.
+        // looks only on the available changes in the change index and cache (which are the
+        // most recent changes).
         if (hasAccessDatabase) {
           resultRefs.add(ref);
         } else if (!visible(repo, changeId)) {
@@ -315,6 +348,11 @@ class DefaultRefFilter {
   }
 
   private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
+    return visibleChanges(repo).containsKey(changeId);
+  }
+
+  private Map<Change.Id, ChangeNotes> visibleChanges(Repository repo)
+      throws PermissionBackendException {
     if (visibleChanges == null) {
       if (changeCache == null) {
         visibleChanges = visibleChangesByScan(repo);
@@ -323,7 +361,7 @@ class DefaultRefFilter {
       }
       logger.atFinest().log("Visible changes: %s", visibleChanges.keySet());
     }
-    return visibleChanges.containsKey(changeId);
+    return visibleChanges;
   }
 
   private boolean visibleEdit(Repository repo, String name) throws PermissionBackendException {
@@ -340,15 +378,11 @@ class DefaultRefFilter {
       return true;
     }
 
-    // Initialize visibleChanges if it wasn't initialized yet.
+    if (visible(repo, id)) {
-    if (visibleChanges == null) {
-      visible(repo, id);
-    }
-    if (visibleChanges.containsKey(id)) {
       try {
         // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
         permissionBackendForProject
-            .ref(visibleChanges.get(id).branch())
+            .ref(visibleChanges(repo).get(id).getChange().getDest().branch())
             .check(RefPermission.READ_PRIVATE_CHANGES);
         logger.atFinest().log("Foreign change edit ref is visible: %s", name);
         return true;
@@ -362,17 +396,17 @@ class DefaultRefFilter {
     return false;
   }
 
-  private Map<Change.Id, BranchNameKey> visibleChangesBySearch() throws PermissionBackendException {
+  private Map<Change.Id, ChangeNotes> visibleChangesBySearch() throws PermissionBackendException {
     Project.NameKey project = projectState.getNameKey();
     try {
-      Map<Change.Id, BranchNameKey> visibleChanges = new HashMap<>();
+      Map<Change.Id, ChangeNotes> visibleChanges = new HashMap<>();
       for (ChangeData cd : changeCache.getChangeData(project)) {
         if (!projectState.statePermitsRead()) {
           continue;
         }
         try {
           permissionBackendForProject.change(cd).check(ChangePermission.READ);
-          visibleChanges.put(cd.getId(), cd.change().getDest());
+          visibleChanges.put(cd.getId(), cd.notes());
         } catch (AuthException e) {
           // Do nothing.
         }
@@ -385,7 +419,7 @@ class DefaultRefFilter {
     }
   }
 
-  private Map<Change.Id, BranchNameKey> visibleChangesByScan(Repository repo)
+  private Map<Change.Id, ChangeNotes> visibleChangesByScan(Repository repo)
       throws PermissionBackendException {
     Project.NameKey p = projectState.getNameKey();
     ImmutableList<ChangeNotesResult> changes;
@@ -397,11 +431,11 @@ class DefaultRefFilter {
       return Collections.emptyMap();
     }
 
-    Map<Change.Id, BranchNameKey> result = Maps.newHashMapWithExpectedSize(changes.size());
+    Map<Change.Id, ChangeNotes> result = Maps.newHashMapWithExpectedSize(changes.size());
     for (ChangeNotesResult notesResult : changes) {
       ChangeNotes notes = toNotes(notesResult);
       if (notes != null) {
-        result.put(notes.getChangeId(), notes.getChange().getDest());
+        result.put(notes.getChangeId(), notes);
       }
     }
     return result;

java/com/google/gerrit/server/permissions/PermissionBackend.java

@@ -329,6 +329,13 @@ public abstract class PermissionBackend {
     /** Remove all NoteDb refs (refs/changes/*, refs/users/*, edit refs) from the result. */
     public abstract boolean filterMeta();
 
+    /**
+     * Return all of the visible change refs that are available in the change index (which are the
+     * most recent changes), even if they are not part of the List<Ref> passed. This allows the
+     * caller not to send all the refs/changes.
+     */
+    public abstract boolean returnMostRecentRefChanges();
+
     /**
      * Select only refs with names matching prefixes per {@link
      * org.eclipse.jgit.lib.RefDatabase#getRefsByPrefix}.
@@ -340,6 +347,7 @@ public abstract class PermissionBackend {
     public static Builder builder() {
       return new AutoValue_PermissionBackend_RefFilterOptions.Builder()
           .setFilterMeta(false)
+          .setReturnMostRecentRefChanges(false)
           .setPrefixes(Collections.singletonList(""));
     }
 
@@ -347,6 +355,8 @@ public abstract class PermissionBackend {
     public abstract static class Builder {
       public abstract Builder setFilterMeta(boolean val);
 
+      public abstract Builder setReturnMostRecentRefChanges(boolean val);
+
       public abstract Builder setPrefixes(List<String> prefixes);
 
       public abstract RefFilterOptions build();

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

@@ -1422,6 +1422,180 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void advertiseMostRecentRefChangesEvenWhenNotInInputWithRefStarPermission()
+      throws Exception {
+    // admin has refs/* permission.
+    requestScopeOperations.setApiUser(admin.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      // set empty list of refs to filter
+                      new ArrayList<>(),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
+    }
+  }
+
+  @Test
+  public void advertiseMostRecentRefChangesEvenWhenNotInInputWithoutRefStarPermission()
+      throws Exception {
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+        .update();
+
+    // user doesn't have refs/* permission.
+    requestScopeOperations.setApiUser(user.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      // set empty list of refs to filter
+                      new ArrayList<>(),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
+    }
+  }
+
+  @Test
+  public void advertiseMostRecentRefChangesOnlyOnceWithRefStarPermission() throws Exception {
+    // admin has refs/* permission.
+    requestScopeOperations.setApiUser(admin.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      repo.getRefDatabase().getRefs(),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true. Make
+          // sure they are only returned once.
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  "HEAD",
+                  psRef1,
+                  metaRef1,
+                  psRef2,
+                  metaRef2,
+                  psRef3,
+                  metaRef3,
+                  psRef4,
+                  metaRef4,
+                  "refs/heads/branch",
+                  "refs/heads/master",
+                  "refs/meta/config",
+                  "refs/tags/branch-tag",
+                  "refs/tags/master-tag",
+                  "refs/tags/tree-tag"));
+    }
+  }
+
+  @Test
+  public void advertiseMostRecentRefChangesOnlyOnceWithoutRefStarPermission() throws Exception {
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+        .update();
+
+    // user doesn't have refs/* permission.
+    requestScopeOperations.setApiUser(user.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      repo.getRefDatabase().getRefs(),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true. Make
+          // sure they are only returned once.
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  "HEAD",
+                  psRef1,
+                  metaRef1,
+                  psRef2,
+                  metaRef2,
+                  psRef3,
+                  metaRef3,
+                  psRef4,
+                  metaRef4,
+                  "refs/heads/branch",
+                  "refs/heads/master",
+                  "refs/meta/config",
+                  "refs/tags/branch-tag",
+                  "refs/tags/master-tag",
+                  "refs/tags/tree-tag"));
+    }
+  }
+
+  @Test
+  public void advertiseMostRecentRefChangesWithSingleRequestedRefWithRefStarPermission()
+      throws Exception {
+    // admin has refs/* permission.
+    requestScopeOperations.setApiUser(admin.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      ImmutableList.of(repo.exactRef("HEAD")),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true.
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  "HEAD", psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
+    }
+  }
+
+  @Test
+  public void advertiseMostRecentRefChangesWithSingleRefRequetedWithoutRefStarPermission()
+      throws Exception {
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+        .update();
+
+    // user doesn't have refs/* permission.
+    requestScopeOperations.setApiUser(user.id());
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      PermissionBackend.ForProject forProject = newFilter(project, admin);
+      assertThat(
+              names(
+                  forProject.filter(
+                      ImmutableList.of(repo.exactRef("HEAD")),
+                      repo,
+                      RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
+          // all the change refs are still returned since returnMostRecentRefChanges = true.
+          .containsExactlyElementsIn(
+              ImmutableList.of(
+                  "HEAD", psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
+    }
+  }
+
   @Test
   public void fetchSingleChangeWithoutIndexAccess() throws Exception {
     PushOneCommit.Result change = createChange();