commit 19030eb700266b009646034c1121124605a045ce Author: Sven Selberg Date: Tue Oct 6 15:41:12 2020 +0200 ReceivePackRefCache, locate patch-sets instead of refs PatchSet.Id and change-ref are two representations of the same thing. All callers of ReceivePackRefCache#tipsFromObjectId(id, prefix) used prefix "refs/changes/" and parsed the refs to PatchSet.Ids. Replacing with ReceivePackRefCache#patchSetIdsFromObjectId(id) that returns a list of PatchSet.Ids makes for easier to read code, since the intention of the call is clear, and it releaves the callers of parsing and null-checks. Change-Id: If2e7cbc269d80d87e460bfeedda96ff76aa90fbc diff --git a/java/com/google/gerrit/server/git/GroupCollector.java b/java/com/google/gerrit/server/git/GroupCollector.java index 9e0f2ee..5bbe5e2 100644 --- a/java/com/google/gerrit/server/git/GroupCollector.java +++ b/java/com/google/gerrit/server/git/GroupCollector.java @@ -29,7 +29,6 @@ import com.google.common.collect.SortedSetMultimap; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; -import com.google.gerrit.entities.RefNames; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.git.receive.ReceivePackRefCache; @@ -43,7 +42,6 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.revwalk.RevCommit; /** @@ -231,7 +229,7 @@ public class GroupCollector { private boolean isGroupFromExistingPatchSet(RevCommit commit, String group) throws IOException { ObjectId id = parseGroup(commit, group); - return id != null && !receivePackRefCache.tipsFromObjectId(id, RefNames.REFS_CHANGES).isEmpty(); + return id != null && !receivePackRefCache.patchSetIdsFromObjectId(id).isEmpty(); } private Set resolveGroups(ObjectId forCommit, Collection candidates) @@ -273,17 +271,13 @@ public class GroupCollector { private Iterable resolveGroup(ObjectId forCommit, String group) throws IOException { ObjectId id = parseGroup(forCommit, group); if (id != null) { - Ref ref = - Iterables.getFirst(receivePackRefCache.tipsFromObjectId(id, RefNames.REFS_CHANGES), null); - if (ref != null) { - PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); - if (psId != null) { - List groups = groupLookup.lookup(psId); - // Group for existing patch set may be missing, e.g. if group has not - // been migrated yet. - if (groups != null && !groups.isEmpty()) { - return groups; - } + PatchSet.Id psId = Iterables.getFirst(receivePackRefCache.patchSetIdsFromObjectId(id), null); + if (psId != null) { + List groups = groupLookup.lookup(psId); + // Group for existing patch set may be missing, e.g. if group has not + // been migrated yet. + if (groups != null && !groups.isEmpty()) { + return groups; } } } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 69db066..9a4b301 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -2145,15 +2145,15 @@ class ReceiveCommits { receivePack.getRevWalk().parseBody(c); String name = c.name(); groupCollector.visit(c); - Collection existingRefs = - receivePackRefCache.tipsFromObjectId(c, RefNames.REFS_CHANGES); + Collection existingPatchSets = + receivePackRefCache.patchSetIdsFromObjectId(c); if (rejectImplicitMerges) { Collections.addAll(mergedParents, c.getParents()); mergedParents.remove(c); } - boolean commitAlreadyTracked = !existingRefs.isEmpty(); + boolean commitAlreadyTracked = !existingPatchSets.isEmpty(); if (commitAlreadyTracked) { alreadyTracked++; // Corner cases where an existing commit might need a new group: @@ -2169,9 +2169,7 @@ class ReceiveCommits { // A's group. // C) Commit is a PatchSet of a pre-existing change uploaded with a // different target branch. - existingRefs.stream() - .map(r -> PatchSet.Id.fromRef(r.getName())) - .filter(Objects::nonNull) + existingPatchSets.stream() .forEach(i -> updateGroups.add(new UpdateGroupsRequest(i, c))); if (!(newChangeForAllNotInTarget || magicBranch.base != null)) { continue; @@ -2311,8 +2309,7 @@ class ReceiveCommits { // In case the change look up from the index failed, // double check against the existing refs - if (foundInExistingRef( - receivePackRefCache.tipsFromObjectId(p.commit, RefNames.REFS_CHANGES))) { + if (foundInExistingPatchSets(receivePackRefCache.patchSetIdsFromObjectId(p.commit))) { if (pending.size() == 1) { reject(magicBranch.cmd, "commit(s) already exists (as current patchset)"); return Collections.emptyList(); @@ -2360,11 +2357,10 @@ class ReceiveCommits { } } - private boolean foundInExistingRef(Collection existingRefs) { - try (TraceTimer traceTimer = newTimer("foundInExistingRef")) { - for (Ref ref : existingRefs) { - ChangeNotes notes = - notesFactory.create(project.getNameKey(), Change.Id.fromRef(ref.getName())); + private boolean foundInExistingPatchSets(Collection existingPatchSets) { + try (TraceTimer traceTimer = newTimer("foundInExistingPatchSet")) { + for (PatchSet.Id psId : existingPatchSets) { + ChangeNotes notes = notesFactory.create(project.getNameKey(), psId.changeId()); Change change = notes.getChange(); if (change.getDest().equals(magicBranch.dest)) { logger.atFine().log("Found change %s from existing refs.", change.getKey()); @@ -2838,15 +2834,15 @@ class ReceiveCommits { return false; } - List existingChangesWithSameCommit = - receivePackRefCache.tipsFromObjectId(newCommit, RefNames.REFS_CHANGES); - if (!existingChangesWithSameCommit.isEmpty()) { + List existingPatchSetsWithSameCommit = + receivePackRefCache.patchSetIdsFromObjectId(newCommit); + if (!existingPatchSetsWithSameCommit.isEmpty()) { // TODO(hiesel, hanwen): Remove this check entirely when Gerrit requires change IDs // without the option to turn that off. reject( inputCommand, "commit already exists (in the project): " - + existingChangesWithSameCommit.get(0).getName()); + + existingPatchSetsWithSameCommit.get(0).toRefName()); return false; } @@ -3225,7 +3221,7 @@ class ReceiveCommits { "more than %d commits, and %s not set", limit, PUSH_OPTION_SKIP_VALIDATION)); return; } - if (!receivePackRefCache.tipsFromObjectId(c, RefNames.REFS_CHANGES).isEmpty()) { + if (!receivePackRefCache.patchSetIdsFromObjectId(c).isEmpty()) { continue; } @@ -3294,12 +3290,8 @@ class ReceiveCommits { // Check if change refs point to this commit. Usually there are 0-1 change // refs pointing to this commit. - for (Ref ref : - receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) { - if (!PatchSet.isChangeRef(ref.getName())) { - continue; - } - PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); + for (PatchSet.Id psId : + receivePackRefCache.patchSetIdsFromObjectId(c.copy())) { Optional notes = getChangeNotes(psId.changeId()); if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) { if (submissionId == null) { diff --git a/java/com/google/gerrit/server/git/receive/ReceivePackRefCache.java b/java/com/google/gerrit/server/git/receive/ReceivePackRefCache.java index 376ab2d..8568810 100644 --- a/java/com/google/gerrit/server/git/receive/ReceivePackRefCache.java +++ b/java/com/google/gerrit/server/git/receive/ReceivePackRefCache.java @@ -21,9 +21,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.RefNames; import java.io.IOException; import java.util.Map; +import java.util.Objects; import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; @@ -58,8 +60,8 @@ public interface ReceivePackRefCache { return new WithAdvertisedRefs(allRefsSupplier); } - /** Returns a list of refs whose name starts with {@code prefix} that point to {@code id}. */ - ImmutableList tipsFromObjectId(ObjectId id, @Nullable String prefix) throws IOException; + /** Returns a list of {@link com.google.gerrit.entities.PatchSet.Id}s that point to {@code id}. */ + ImmutableList patchSetIdsFromObjectId(ObjectId id) throws IOException; /** Returns all refs whose name starts with {@code prefix}. */ ImmutableList byPrefix(String prefix) throws IOException; @@ -76,10 +78,10 @@ public interface ReceivePackRefCache { } @Override - public ImmutableList tipsFromObjectId(ObjectId id, @Nullable String prefix) - throws IOException { + public ImmutableList patchSetIdsFromObjectId(ObjectId id) throws IOException { return delegate.getTipsWithSha1(id).stream() - .filter(r -> prefix == null || r.getName().startsWith(prefix)) + .map(r -> PatchSet.Id.fromRef(r.getName())) + .filter(Objects::nonNull) .collect(toImmutableList()); } @@ -113,10 +115,11 @@ public interface ReceivePackRefCache { } @Override - public ImmutableList tipsFromObjectId(ObjectId id, String prefix) { + public ImmutableList patchSetIdsFromObjectId(ObjectId id) { lazilyInitRefMaps(); return refsByObjectId.get(id).stream() - .filter(r -> prefix == null || r.getName().startsWith(prefix)) + .map(r -> PatchSet.Id.fromRef(r.getName())) + .filter(Objects::nonNull) .collect(toImmutableList()); } diff --git a/javatests/com/google/gerrit/server/git/receive/ReceivePackRefCacheTest.java b/javatests/com/google/gerrit/server/git/receive/ReceivePackRefCacheTest.java index 698acd8..7eb6bc7 100644 --- a/javatests/com/google/gerrit/server/git/receive/ReceivePackRefCacheTest.java +++ b/javatests/com/google/gerrit/server/git/receive/ReceivePackRefCacheTest.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.RefNames; import java.util.Map; import org.eclipse.jgit.lib.ObjectId; @@ -60,17 +61,18 @@ public class ReceivePackRefCacheTest { } @Test - public void noCache_tipsFromObjectIdDelegatesToRefDbAndFiltersByPrefix() throws Exception { + public void noCache_tipsFromObjectIdDelegatesToRefDb() throws Exception { Ref refBla = newRef("refs/bla", "badc0feebadc0feebadc0feebadc0feebadc0fee"); - Ref refheads = newRef(RefNames.REFS_HEADS, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + String patchSetRef = RefNames.REFS_CHANGES + "01/1/1"; + Ref patchSet = newRef(patchSetRef, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); RefDatabase mockRefDb = mock(RefDatabase.class); ReceivePackRefCache cache = ReceivePackRefCache.noCache(mockRefDb); when(mockRefDb.getTipsWithSha1(ObjectId.zeroId())) - .thenReturn(ImmutableSet.of(refBla, refheads)); + .thenReturn(ImmutableSet.of(refBla, patchSet)); - assertThat(cache.tipsFromObjectId(ObjectId.zeroId(), RefNames.REFS_HEADS)) - .containsExactly(refheads); + assertThat(cache.patchSetIdsFromObjectId(ObjectId.zeroId())) + .containsExactly(PatchSet.Id.fromRef(patchSetRef)); verify(mockRefDb).getTipsWithSha1(ObjectId.zeroId()); verifyNoMoreInteractions(mockRefDb); } @@ -107,25 +109,14 @@ public class ReceivePackRefCacheTest { } @Test - public void advertisedRefs_tipsFromObjectIdWithNoPrefix() throws Exception { + public void advertisedRefs_patchSetIdsFromObjectId() throws Exception { Map refs = setupTwoChanges(); ReceivePackRefCache cache = ReceivePackRefCache.withAdvertisedRefs(() -> refs); assertThat( - cache.tipsFromObjectId( - ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"), null)) - .containsExactly(refs.get("refs/changes/01/1/1")); - } - - @Test - public void advertisedRefs_tipsFromObjectIdWithPrefix() throws Exception { - Map refs = setupTwoChanges(); - ReceivePackRefCache cache = ReceivePackRefCache.withAdvertisedRefs(() -> refs); - - assertThat( - cache.tipsFromObjectId( - ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"), "/refs/some")) - .isEmpty(); + cache.patchSetIdsFromObjectId( + ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"))) + .containsExactly(PatchSet.Id.fromRef("refs/changes/01/1/1")); } private static Ref newRef(String name, String sha1) {