Merge changes Id81c291a,I15d97ec7

* changes:
  VisibleRefFilter: break out special cases to methods
  Optimize edit handling in VisibleRefFilter

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java

@@ -164,29 +164,17 @@ public class RefNames {
    * @return reference prefix for this change edit
    */
   public static String refsEditPrefix(Account.Id accountId, Change.Id changeId) {
-    return new StringBuilder(refsUsers(accountId))
+    return refsEditPrefix(accountId) + changeId.get() + '/';
-      .append('/')
+  }
-      .append(EDIT_PREFIX)
+
-      .append(changeId.get())
+  public static String refsEditPrefix(Account.Id accountId) {
-      .append('/')
+    return refsUsers(accountId) + '/' + EDIT_PREFIX;
-      .toString();
   }
 
   public static boolean isRefsEdit(String ref) {
     return ref.startsWith(REFS_USERS) && ref.contains(EDIT_PREFIX);
   }
 
-  public static boolean isRefsEditOf(String ref, Account.Id accountId) {
-    if (accountId == null) {
-      return false;
-    }
-    String prefix = new StringBuilder(refsUsers(accountId))
-        .append('/')
-        .append(EDIT_PREFIX)
-        .toString();
-    return ref.startsWith(prefix);
-  }
-
   static Integer parseShardedRefPart(String name) {
     if (name == null) {
       return null;

gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java

@@ -80,30 +80,6 @@ public class RefNamesTest {
     assertThat(RefNames.isRefsEdit("refs/heads/master")).isFalse();
   }
 
-  @Test
-  public void isRefsEditOf() throws Exception {
-    assertThat(
-        RefNames.isRefsEditOf("refs/users/23/1011123/edit-67473/42", accountId))
-            .isTrue();
-
-    // other user
-    assertThat(
-        RefNames.isRefsEditOf("refs/users/20/1078620/edit-67473/42", accountId))
-            .isFalse();
-
-    // user ref, but no edit ref
-    assertThat(RefNames.isRefsEditOf("refs/users/23/1011123", accountId))
-        .isFalse();
-
-    // bad user shard
-    assertThat(
-        RefNames.isRefsEditOf("refs/users/77/1011123/edit-67473/42", accountId))
-            .isFalse();
-
-    // other ref
-    assertThat(RefNames.isRefsEditOf("refs/heads/master", accountId)).isFalse();
-  }
-
   @Test
   public void testParseShardedRefsPart() throws Exception {
     assertThat(parseShardedRefPart("01/1")).isEqualTo(1);

gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java

@@ -14,8 +14,12 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_SELF;
+
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -60,6 +64,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
   private final ProjectControl projectCtl;
   private final ReviewDb reviewDb;
   private final boolean showMetadata;
+  private String userEditPrefix;
   private Set<Change.Id> visibleChanges;
 
   public VisibleRefFilter(
@@ -81,86 +86,62 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
   }
 
   public Map<String, Ref> filter(Map<String, Ref> refs, boolean filterTagsSeparately) {
-    if (projectCtl.getProjectState().isAllUsers()
+    if (projectCtl.getProjectState().isAllUsers()) {
-        && projectCtl.getUser().isIdentifiedUser()) {
+      refs = addUsersSelfSymref(refs);
-      Ref userRef =
-          refs.get(RefNames.refsUsers(projectCtl.getUser().getAccountId()));
-      if (userRef != null) {
-        SymbolicRef refsUsersSelf =
-            new SymbolicRef(RefNames.REFS_USERS_SELF, userRef);
-        refs = new HashMap<>(refs);
-        refs.put(refsUsersSelf.getName(), refsUsersSelf);
-      }
     }
 
-    if (projectCtl.allRefsAreVisible(ImmutableSet.of(RefNames.REFS_CONFIG))) {
+    if (projectCtl.allRefsAreVisible(ImmutableSet.of(REFS_CONFIG))) {
-      Map<String, Ref> r = Maps.newHashMap(refs);
+      return fastHideRefsMetaConfig(refs);
-      if (!projectCtl.controlForRef(RefNames.REFS_CONFIG).isVisible()) {
-        r.remove(RefNames.REFS_CONFIG);
-      }
-      return r;
     }
 
-    Account.Id currAccountId;
+    Account.Id userId;
-    boolean canViewMetadata;
+    boolean viewMetadata;
     if (projectCtl.getUser().isIdentifiedUser()) {
       IdentifiedUser user = projectCtl.getUser().asIdentifiedUser();
-      currAccountId = user.getAccountId();
+      userId = user.getAccountId();
-      canViewMetadata = user.getCapabilities().canAccessDatabase();
+      viewMetadata = user.getCapabilities().canAccessDatabase();
+      userEditPrefix = RefNames.refsEditPrefix(userId);
     } else {
-      currAccountId = null;
+      userId = null;
-      canViewMetadata = false;
+      viewMetadata = false;
     }
 
     Map<String, Ref> result = new HashMap<>();
     List<Ref> deferredTags = new ArrayList<>();
 
     for (Ref ref : refs.values()) {
+      String name = ref.getName();
       Change.Id changeId;
       Account.Id accountId;
-      if (ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
+      if (name.startsWith(REFS_CACHE_AUTOMERGE)
+          || (!showMetadata && isMetadata(name))) {
         continue;
-      } else if (showMetadata
+      } else if (RefNames.isRefsEdit(name)) {
-          && (RefNames.isRefsEditOf(ref.getLeaf().getName(), currAccountId)
+        // Edits are visible only to the owning user, if change is visible.
-              || (RefNames.isRefsEdit(ref.getLeaf().getName())
+        if (viewMetadata || visibleEdit(name)) {
-                  && canViewMetadata))) {
+          result.put(name, ref);
-        // Change edit reference related is visible to the account that owns the
+        }
-        // change edit.
+      } else if ((changeId = Change.Id.fromRef(name)) != null) {
-        //
+        // Change ref is visible only if the change is visible.
-        // TODO(dborowitz): Verify if change is visible (to exclude edits on
+        if (viewMetadata || visible(changeId)) {
-        // changes that the user has lost access to).
+          result.put(name, ref);
-        result.put(ref.getName(), ref);
+        }
-
+      } else if ((accountId = Account.Id.fromRef(name)) != null) {
-      } else if ((changeId = Change.Id.fromRef(ref.getName())) != null) {
+        // Account ref is visible only to corresponding account.
-        // Reference related to a change is visible if the change is visible.
+        if (viewMetadata || (accountId.equals(userId)
-        if (showMetadata && (canViewMetadata || visible(changeId))) {
+            && projectCtl.controlForRef(name).isVisible())) {
-          result.put(ref.getName(), ref);
+          result.put(name, ref);
         }
-
       } else if (isTag(ref)) {
         // If its a tag, consider it later.
-        //
         if (ref.getObjectId() != null) {
           deferredTags.add(ref);
         }
-
       } else if (projectCtl.controlForRef(ref.getLeaf().getName()).isVisible()) {
         // Use the leaf to lookup the control data. If the reference is
         // symbolic we want the control around the final target. If its
         // not symbolic then getLeaf() is a no-op returning ref itself.
-        //
+        result.put(name, ref);
-
-        if ((accountId =
-            Account.Id.fromRef(ref.getLeaf().getName())) != null) {
-          // Reference related to an account is visible only for the current
-          // account.
-          if (showMetadata
-              && (canViewMetadata || accountId.equals(currAccountId))) {
-            result.put(ref.getName(), ref);
-          }
-        } else {
-          result.put(ref.getName(), ref);
-        }
       }
     }
 
@@ -182,6 +163,28 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
     return result;
   }
 
+  private Map<String, Ref> fastHideRefsMetaConfig(Map<String, Ref> refs) {
+    if (refs.containsKey(REFS_CONFIG)
+        && !projectCtl.controlForRef(REFS_CONFIG).isVisible()) {
+      Map<String, Ref> r = new HashMap<>(refs);
+      r.remove(REFS_CONFIG);
+      return r;
+    }
+    return refs;
+  }
+
+  private Map<String, Ref> addUsersSelfSymref(Map<String, Ref> refs) {
+    if (projectCtl.getUser().isIdentifiedUser()) {
+      Ref r = refs.get(RefNames.refsUsers(projectCtl.getUser().getAccountId()));
+      if (r != null) {
+        SymbolicRef s = new SymbolicRef(REFS_USERS_SELF, r);
+        refs = new HashMap<>(refs);
+        refs.put(s.getName(), s);
+      }
+    }
+    return refs;
+  }
+
   @Override
   protected Map<String, Ref> getAdvertisedRefs(Repository repository,
       RevWalk revWalk) throws ServiceMayNotContinueException {
@@ -211,6 +214,16 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
     return visibleChanges.contains(changeId);
   }
 
+  private boolean visibleEdit(String name) {
+    if (userEditPrefix != null && name.startsWith(userEditPrefix)) {
+      Change.Id id = Change.Id.fromEditRefPart(name);
+      if (id != null) {
+        return visible(id);
+      }
+    }
+    return false;
+  }
+
   private Set<Change.Id> visibleChangesBySearch() {
     Project project = projectCtl.getProject();
     try {
@@ -247,6 +260,10 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
     }
   }
 
+  private static boolean isMetadata(String name) {
+    return name.startsWith(REFS_CHANGES) || RefNames.isRefsEdit(name);
+  }
+
   private static boolean isTag(Ref ref) {
     return ref.getLeaf().getName().startsWith(Constants.R_TAGS);
   }