RefControl: Use consistent rev-walking logic in canCreate

The intent of I4e580e2f was to allow creation of new branches along
with pushing new commits only if the user has push permissions, but
the implementation was buggy. For this change, we define "new commits"
as anything not reachable from a visible branch or tag (which
excludes, for example, commits of visible but not merged patch sets).

We previously defined a more correct walk in ProjectControl for
determining if a commit is merged into a visible branch, so reuse that
logic from RefControl.

Change-Id: I2e6181de5bf2a990ee58fb8e9d72b3370eba5cb8

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

@@ -992,7 +992,7 @@ public class ReceiveCommits {
 
     RefControl ctl = projectControl.controlForRef(cmd.getRefName());
     rp.getRevWalk().reset();
-    if (ctl.canCreate(db, rp.getRevWalk(), obj, allRefs.values().contains(obj))) {
+    if (ctl.canCreate(db, rp.getRevWalk(), obj)) {
       validateNewCommits(ctl, cmd);
       batch.addCommand(cmd);
     } else {

gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java

@@ -134,7 +134,7 @@ public class CreateBranch implements RestModifyView<ProjectResource, Input> {
       }
 
       rw.reset();
-      if (!refControl.canCreate(db.get(), rw, object, true)) {
+      if (!refControl.canCreate(db.get(), rw, object)) {
         throw new AuthException("Cannot create \"" + ref + "\"");
       }
 

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java

@@ -523,15 +523,9 @@ public class ProjectControl {
 
   public boolean canReadCommit(ReviewDb db, RevWalk rw, RevCommit commit) {
     try {
-      Repository repo = repoManager.openRepository(getProject().getNameKey());
+      Repository repo = openRepository();
       try {
-        VisibleRefFilter filter =
+        return isMergedIntoVisibleRef(repo, db, rw, commit, repo.getAllRefs());
-            new VisibleRefFilter(tagCache, changeCache, repo, this, db, true);
-        Map<String, Ref> visibleRefs = filter.filter(repo.getAllRefs(), true);
-        if (!visibleRefs.isEmpty() && IncludedInResolver.includedInOne(
-            repo, rw, commit, visibleRefs.values())) {
-          return true;
-        }
       } finally {
         repo.close();
       }
@@ -540,7 +534,20 @@ public class ProjectControl {
           "Cannot verify permissions to commit object %s in repository %s",
           commit.name(), getProject().getNameKey());
       log.error(msg, e);
+      return false;
     }
-    return false;
+  }
+
+  boolean isMergedIntoVisibleRef(Repository repo, ReviewDb db, RevWalk rw,
+      RevCommit commit, Map<String, Ref> unfilteredRefs) throws IOException {
+    VisibleRefFilter filter =
+        new VisibleRefFilter(tagCache, changeCache, repo, this, db, true);
+    Map<String, Ref> refs = filter.filter(unfilteredRefs, true);
+    return !refs.isEmpty()
+        && IncludedInResolver.includedInOne(repo, rw, commit, refs.values());
+  }
+
+  Repository openRepository() throws IOException {
+    return repoManager.openRepository(getProject().getNameKey());
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java

@@ -32,12 +32,16 @@ import com.google.gerrit.server.group.SystemGroupBackend;
 
 import dk.brics.automaton.RegExp;
 
+import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -50,6 +54,8 @@ import java.util.Set;
 
 /** Manages access control for Git references (aka branches, tags). */
 public class RefControl {
+  private static final Logger log = LoggerFactory.getLogger(RefControl.class);
+
   private final ProjectControl projectControl;
   private final String refName;
 
@@ -236,11 +242,9 @@ public class RefControl {
    * @param rw revision pool {@code object} was parsed in; must be reset before
    *     calling this method.
    * @param object the object the user will start the reference with.
-   * @param existsOnServer the object exists on server or not.
    * @return {@code true} if the user specified can create a new Git ref
    */
-  public boolean canCreate(ReviewDb db, RevWalk rw, RevObject object,
+  public boolean canCreate(ReviewDb db, RevWalk rw, RevObject object) {
-      boolean existsOnServer) {
     if (!canWrite()) {
       return false;
     }
@@ -265,13 +269,15 @@ public class RefControl {
       } else if (!canPerform(Permission.CREATE)) {
         // No create permissions.
         return false;
-      } else if (!existsOnServer && canUpdate()) {
+      } else if (canUpdate()) {
-        // If the object doesn't exist on the server, check that the user has
+        // If the user has push permissions, they can create the ref regardless
-        // push permissions.
+        // of whether they are pushing any new objects along with the create.
         return true;
-      } else if (projectControl.canReadCommit(db, rw, (RevCommit) object)) {
+      } else if (isMergedIntoBranchOrTag(db, rw, (RevCommit) object)) {
-        // The object exists on the server and is readable by this user, so they
+        // If the user has no push permissions, check whether the object is
-        // do not require push permission create this ref.
+        // merged into a branch or tag readable by this user. If so, they are
+        // not effectively "pushing" more objects, so they can create the ref
+        // even if they don't have push permission.
         return true;
       }
       return false;
@@ -313,6 +319,28 @@ public class RefControl {
     }
   }
 
+  private boolean isMergedIntoBranchOrTag(ReviewDb db, RevWalk rw,
+      RevCommit commit) {
+    try {
+      Repository repo = projectControl.openRepository();
+      try {
+        Map<String, Ref> refs =
+            repo.getRefDatabase().getRefs(Constants.R_HEADS);
+        refs.putAll(repo.getRefDatabase().getRefs(Constants.R_TAGS));
+        return projectControl.isMergedIntoVisibleRef(
+            repo, db, rw, commit, refs);
+      } finally {
+        repo.close();
+      }
+    } catch (IOException e) {
+      String msg = String.format(
+          "Cannot verify permissions to commit object %s in repository %s",
+          commit.name(), projectControl.getProject().getNameKey());
+      log.error(msg, e);
+    }
+    return false;
+  }
+
   /**
    * Determines whether the user can delete the Git ref controlled by this
    * object.