Merge changes I6333a288,I8f4d2f20,I51c8950e,I11a76eda

* changes:
  TagSet: encapsulate reachability propagation
  TagSet: Track tag commits with a RevFlag
  Only create RoaringBitmaps for commits which are tags
  RefAdvertisementIT: Test tags reachable only from leaf branch

2 files changed, 104 insertions, 18 deletions

diff --git a/java/com/google/gerrit/server/git/TagSet.java b/java/com/google/gerrit/server/git/TagSet.java
index a528c8fd60..bffc479c4f 100644
--- a/java/com/google/gerrit/server/git/TagSet.java
+++ b/java/com/google/gerrit/server/git/TagSet.java
@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableSet;
@@ -41,6 +43,7 @@ import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevSort;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.roaringbitmap.RoaringBitmap;
@@ -209,6 +212,7 @@ class TagSet {
 
     try (TagWalk rw = new TagWalk(git)) {
       rw.setRetainBody(false);
+      RevFlag isTag = rw.newFlag("tag");
       for (Ref ref :
           git.getRefDatabase()
               .getRefsByPrefixWithExclusions(RefDatabase.ALL, SKIPPABLE_REF_PREFIXES)) {
@@ -218,9 +222,9 @@ class TagSet {
         } else if (isTag(ref)) {
           // For a tag, remember where it points to.
           try {
-            addTag(rw, git.getRefDatabase().peel(ref));
+            addTag(rw, git.getRefDatabase().peel(ref), isTag);
           } catch (IOException e) {
-            addTag(rw, ref);
+            addTag(rw, ref, isTag);
           }
 
         } else {
@@ -229,17 +233,10 @@ class TagSet {
         }
       }
 
-      // Traverse the complete history. Copy any flags from a commit to
-      // all of its ancestors. This automatically updates any Tag object
-      // as the TagCommit and the stored Tag object share the same
-      // underlying bit set.
+      // Traverse the complete history and propagate reachability to parents.
       TagCommit c;
       while ((c = (TagCommit) rw.next()) != null) {
-        RoaringBitmap mine = c.refFlags;
-        int pCnt = c.getParentCount();
-        for (int pIdx = 0; pIdx < pCnt; pIdx++) {
-          ((TagCommit) c.getParent(pIdx)).refFlags.or(mine);
-        }
+        c.propagateReachabilityToParents(isTag);
       }
     } catch (IOException e) {
       logger.atWarning().withCause(e).log("Error building tags for repository %s", projectName);
@@ -356,9 +353,7 @@ class TagSet {
     refs.putAll(old.refs);
 
     for (Tag srcTag : old.tags) {
-      RoaringBitmap mine = new RoaringBitmap();
-      mine.or(srcTag.refFlags);
-      tags.add(new Tag(srcTag, mine));
+      tags.add(new Tag(srcTag));
     }
 
     for (TagMatcher.LostRef lost : m.lostRefs) {
@@ -369,7 +364,7 @@ class TagSet {
     }
   }
 
-  private void addTag(TagWalk rw, Ref ref) {
+  private void addTag(TagWalk rw, Ref ref, RevFlag isTag) {
     ObjectId id = ref.getPeeledObjectId();
     if (id == null) {
       id = ref.getObjectId();
@@ -378,7 +373,12 @@ class TagSet {
     if (!tags.contains(id)) {
       RoaringBitmap flags;
       try {
-        flags = ((TagCommit) rw.parseCommit(id)).refFlags;
+        TagCommit commit = ((TagCommit) rw.parseCommit(id));
+        commit.add(isTag);
+        if (commit.refFlags == null) {
+          commit.refFlags = new RoaringBitmap();
+        }
+        flags = commit.refFlags;
       } catch (IncorrectObjectTypeException notCommit) {
         flags = new RoaringBitmap();
       } catch (IOException e) {
@@ -395,6 +395,9 @@ class TagSet {
       rw.markStart(commit);
 
       int flag = refs.size();
+      if (commit.refFlags == null) {
+        commit.refFlags = new RoaringBitmap();
+      }
       commit.refFlags.add(flag);
       refs.put(ref.getName(), new CachedRef(ref, flag));
     } catch (IncorrectObjectTypeException notCommit) {
@@ -432,8 +435,13 @@ class TagSet {
     // RoaringBitmap in TagCommit.refFlags.
     @VisibleForTesting final RoaringBitmap refFlags;
 
+    Tag(Tag src) {
+      this(src, src.refFlags.clone());
+    }
+
     Tag(AnyObjectId id, RoaringBitmap flags) {
       super(id);
+      checkNotNull(flags);
       this.refFlags = flags;
     }
 
@@ -485,13 +493,50 @@ class TagSet {
   }
 
   // TODO(hanwen): this would be better named as CommitWithReachability, as it also holds non-tags.
+  // However, non-tags will have a null refFlags field.
   private static final class TagCommit extends RevCommit {
     /** CachedRef.flag => isVisible, indicating if this commit is reachable from the ref. */
-    final RoaringBitmap refFlags;
+    RoaringBitmap refFlags;
 
     TagCommit(AnyObjectId id) {
       super(id);
-      refFlags = new RoaringBitmap();
+    }
+
+    /**
+     * Copy any flags from this commit to all of its ancestors.
+     *
+     * <p>Do not maintain a reference to the flags on non-tag commits after copying their flags to
+     * their ancestors. The flag copying automatically updates any Tag object as the TagCommit and
+     * the stored Tag object share the same underlying RoaringBitmap.
+     *
+     * @param isTag {@code RevFlag} indicating if this TagCommit is a tag
+     */
+    void propagateReachabilityToParents(RevFlag isTag) {
+      RoaringBitmap mine = refFlags;
+      if (mine != null) {
+        boolean canMoveBitmap = false;
+        if (!has(isTag)) {
+          refFlags = null;
+          canMoveBitmap = true;
+        }
+        int pCnt = getParentCount();
+        for (int pIdx = 0; pIdx < pCnt; pIdx++) {
+          TagCommit commit = (TagCommit) getParent(pIdx);
+          RoaringBitmap parentFlags = commit.refFlags;
+          if (parentFlags == null) {
+            if (canMoveBitmap) {
+              // This commit is not itself a Tag, so in order to reduce cloning overhead, migrate
+              // its refFlags object to its first parent with null refFlags
+              commit.refFlags = mine;
+              canMoveBitmap = false;
+            } else {
+              commit.refFlags = mine.clone();
+            }
+          } else {
+            parentFlags.or(mine);
+          }
+        }
+      }
     }
   }
 }
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 9e85d8ce73..c0531e513b 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -875,6 +875,47 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
         "refs/tags/new-tag");
   }
 
+  // rcMaster (c1 master master-tag) <- rcBranch (c2 branch branch-tag) <- rcBranch (c2 branch) <-
+  // newcommit1 <- newcommit2 (new-branch)
+  @Test
+  public void uploadPackReachableTagVisibleFromLeafBranch() throws Exception {
+    try (Repository repo = repoManager.openRepository(project)) {
+      // rcBranch (c2 branch) <- newcommit1 (new-branch)
+      PushOneCommit.Result r =
+          pushFactory
+              .create(admin.newIdent(), testRepo)
+              .setParent(rcBranch)
+              .to("refs/heads/new-branch");
+      r.assertOkStatus();
+      RevCommit branchRc = r.getCommit();
+
+      // rcBranch (c2) <- newcommit1 <- newcommit2 (new-branch)
+      r =
+          pushFactory
+              .create(admin.newIdent(), testRepo)
+              .setParent(branchRc)
+              .to("refs/heads/new-branch");
+      r.assertOkStatus();
+    }
+
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(deny(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+        .add(deny(Permission.READ).ref("refs/heads/branch").group(REGISTERED_USERS))
+        .add(allow(Permission.READ).ref("refs/heads/new-branch").group(REGISTERED_USERS))
+        .update();
+
+    requestScopeOperations.setApiUser(user.id());
+    assertUploadPackRefs(
+        "refs/heads/new-branch",
+        // 'master' and 'branch' branches are not visible but 'master-tag' and 'branch-tag' are
+        // reachable from new-branch (since PushOneCommit always bases changes on each other).
+        "refs/tags/branch-tag",
+        "refs/tags/master-tag");
+    // tree-tag not visible. See comment in subsetOfBranchesVisibleIncludingHead.
+  }
+
   // first  ls-remote: rcBranch (c2 branch)               <- newcommit1 (updated-tag)
   // second ls-remote: rcBranch (c2 branch updated-tag)
   @Test