opendev/gerrit
Code Issues Proposed changes

Merge "ReceivePackRefCache, locate patch-sets instead of refs"

Browse Source
This commit is contained in:
Patrick Hiesel
2020-10-07 12:51:04 +00:00
committed by Gerrit Code Review
parent 0b568f3dba 19030eb700
commit 491d90755d
4 changed files with 45 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
java/com/google/gerrit/server/git/GroupCollector.java
View File

@@ -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;
}
}
}

40
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -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;
@@ -2312,8 +2310,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();
@@ -2361,11 +2358,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());
@@ -2839,15 +2835,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;
}
@@ -3226,7 +3222,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;
}
@@ -3295,12 +3291,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) {

17
java/com/google/gerrit/server/git/receive/ReceivePackRefCache.java
View File

@@ -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());
}

31
javatests/com/google/gerrit/server/git/receive/ReceivePackRefCacheTest.java
View File

@@ -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) {