commit 19030eb700266b009646034c1121124605a045ce
Author: Sven Selberg <svense@axis.com>
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<String> resolveGroups(ObjectId forCommit, Collection<String> candidates)
@@ -273,17 +271,13 @@ public class GroupCollector {
private Iterable<String> 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<String> 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<String> 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<Ref> existingRefs =
- receivePackRefCache.tipsFromObjectId(c, RefNames.REFS_CHANGES);
+ Collection<PatchSet.Id> 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<Ref> 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<PatchSet.Id> 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<Ref> existingChangesWithSameCommit =
- receivePackRefCache.tipsFromObjectId(newCommit, RefNames.REFS_CHANGES);
- if (!existingChangesWithSameCommit.isEmpty()) {
+ List<PatchSet.Id> 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<ChangeNotes> 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<Ref> 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<PatchSet.Id> patchSetIdsFromObjectId(ObjectId id) throws IOException;
/** Returns all refs whose name starts with {@code prefix}. */
ImmutableList<Ref> byPrefix(String prefix) throws IOException;
@@ -76,10 +78,10 @@ public interface ReceivePackRefCache {
}
@Override
- public ImmutableList<Ref> tipsFromObjectId(ObjectId id, @Nullable String prefix)
- throws IOException {
+ public ImmutableList<PatchSet.Id> 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<Ref> tipsFromObjectId(ObjectId id, String prefix) {
+ public ImmutableList<PatchSet.Id> 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<String, Ref> 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<String, Ref> 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) {