opendev/gerrit
Code Issues Proposed changes

Extract an ApprovalCopier from ApprovalsUtil

Browse Source 
Rewrite in such a way that we can do an implicit "copy" between
arbitrary patch sets without necessarily storing the results. For the
notedb, we will use this (with cached ChangeKinds) rather than storing
copied labels.

This introduces a few minor behavior changes. First, copied approvals
are now normalized according to permissions. This may result in
different values stored in the database when copying, but these values
were already normalized out by ChangeJson and the submit rule
evaluator, so this should not result in a semantic change.

Second, we no longer pass in a "source" patch set; all prior patch
sets are considered. This has the advantage that it does not rely on
labels having been properly copied to intermediate patch sets. It also
means, for example, if patch set 3 has a code change relative to patch
set 2 but not patch set 1, approvals on a copyAllScoresOnNoCodeChange
label will be copied from patch set 1, bypassing patch set 2. We think
this is a more correct state of affairs: if patch set 1 is good enough
to submit, so should patch set 3, regardless of the fact that there is
an intermediate patch set.

Change-Id: I56ae707c34ac7b978b06a0624c1353e8f28286a7
This commit is contained in:
Dave Borowitz
2014-01-08 13:00:29 -08:00
parent a8f07cc3ee
commit c73bcab298
5 changed files with 203 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

176
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java Normal file
View File

@@ -0,0 +1,176 @@
// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.change.ChangeKind.NO_CODE_CHANGE;
import static com.google.gerrit.server.change.ChangeKind.TRIVIAL_REBASE;
import com.google.common.base.Objects;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Maps;
import com.google.common.collect.Table;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.ChangeKind;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
import java.util.NavigableSet;
import java.util.SortedSet;
import java.util.TreeMap;
/**
* Copies approvals between patch sets.
* <p>
* The result of a copy may either be stored, as when stamping approvals in the
* database at submit time, or refreshed on demand, as when reading approvals
* from the notedb.
*/
public class ApprovalCopier {
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
private final ChangeKindCache changeKindCache;
private final LabelNormalizer labelNormalizer;
private final ChangeData.Factory changeDataFactory;
@Inject
ApprovalCopier(GitRepositoryManager repoManager,
ProjectCache projectCache,
ChangeKindCache changeKindCache,
LabelNormalizer labelNormalizer,
ChangeData.Factory changeDataFactory) {
this.repoManager = repoManager;
this.projectCache = projectCache;
this.changeKindCache = changeKindCache;
this.labelNormalizer = labelNormalizer;
this.changeDataFactory = changeDataFactory;
}
public void copy(ReviewDb db, ChangeControl ctl, PatchSet.Id psId)
throws OrmException {
db.patchSetApprovals().insert(getForPatchSet(db, ctl, psId));
}
private List<PatchSetApproval> getForPatchSet(ReviewDb db, ChangeControl ctl,
PatchSet.Id psId) throws OrmException {
ChangeData cd = changeDataFactory.create(db, ctl);
try {
ProjectState project =
projectCache.checkedGet(cd.change().getDest().getParentKey());
ListMultimap<PatchSet.Id, PatchSetApproval> all = cd.allApprovalsMap();
Table<String, Account.Id, PatchSetApproval> byUser =
HashBasedTable.create();
for (PatchSetApproval psa : all.get(psId)) {
byUser.put(psa.getLabel(), psa.getAccountId(), psa);
}
TreeMap<Integer, PatchSet> patchSets = getPatchSets(cd);
NavigableSet<Integer> allPsIds = patchSets.navigableKeySet();
PatchSet currPs = patchSets.get(psId.get());
if (currPs == null) {
throw new OrmException("missing patch set " + psId);
}
Repository repo =
repoManager.openRepository(project.getProject().getNameKey());
try {
// Walk patch sets strictly less than psId in descending order.
for (PatchSet priorPs
: patchSets.descendingMap().tailMap(psId.get(), false).values()) {
List<PatchSetApproval> priorApprovals = all.get(priorPs.getId());
if (priorApprovals.isEmpty()) {
continue;
}
ChangeKind kind = changeKindCache.getChangeKind(project, repo,
ObjectId.fromString(priorPs.getRevision().get()),
ObjectId.fromString(currPs.getRevision().get()));
for (PatchSetApproval psa : priorApprovals) {
if (!byUser.contains(psa.getLabel(), psa.getAccountId())
&& canCopy(project, psa, psId, allPsIds, kind)) {
byUser.put(psa.getLabel(), psa.getAccountId(), copy(psa, psId));
}
}
}
return labelNormalizer.normalize(ctl, byUser.values());
} finally {
repo.close();
}
} catch (IOException e) {
throw new OrmException(e);
}
}
private static TreeMap<Integer, PatchSet> getPatchSets(ChangeData cd)
throws OrmException {
Collection<PatchSet> patchSets = cd.patches();
TreeMap<Integer, PatchSet> result = Maps.newTreeMap();
for (PatchSet ps : patchSets) {
result.put(ps.getId().get(), ps);
}
return result;
}
private static boolean canCopy(ProjectState project, PatchSetApproval psa,
PatchSet.Id psId, NavigableSet<Integer> allPsIds, ChangeKind kind)
throws OrmException {
int n = psa.getKey().getParentKey().get();
checkArgument(n != psId.get());
LabelType type = project.getLabelTypes().byLabel(psa.getLabelId());
if (type == null) {
return false;
} else if (Objects.equal(n, previous(allPsIds, psId.get())) && (
type.isCopyMinScore() && type.isMaxNegative(psa)
|| type.isCopyMaxScore() && type.isMaxPositive(psa))) {
// Copy min/max score only from the immediately preceding patch set (which
// may not be psId.get() - 1).
return true;
}
return (type.isCopyAllScoresOnTrivialRebase() && kind == TRIVIAL_REBASE)
|| (type.isCopyAllScoresIfNoCodeChange() && kind == NO_CODE_CHANGE);
}
private static PatchSetApproval copy(PatchSetApproval src, PatchSet.Id psId) {
if (src.getKey().getParentKey().equals(psId)) {
return src;
}
return new PatchSetApproval(psId, src);
}
private static <T> T previous(NavigableSet<T> s, T v) {
SortedSet<T> head = s.headSet(v);
return !head.isEmpty() ? head.last() : null;
}
}

43
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
View File

@@ -39,7 +39,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.ChangeKind;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
@@ -150,48 +149,6 @@ public class ApprovalsUtil {
return ImmutableSetMultimap.copyOf(reviewers);
}
/**
* Copy min/max scores from one patch set to another.
*
* @throws OrmException
*/
public void copyLabels(ReviewDb db, ChangeNotes notes, LabelTypes labelTypes,
PatchSet.Id source, PatchSet dest, ChangeKind changeKind)
throws OrmException {
copyLabels(db, labelTypes, byPatchSet(db, notes, source), source, dest,
changeKind);
}
/**
* Copy a set's min/max scores from one patch set to another.
*
* @throws OrmException
*/
public void copyLabels(ReviewDb db, LabelTypes labelTypes,
Iterable<PatchSetApproval> sourceApprovals, PatchSet.Id source,
PatchSet dest, ChangeKind changeKind) throws OrmException {
List<PatchSetApproval> copied = Lists.newArrayList();
for (PatchSetApproval a : sourceApprovals) {
if (source.equals(a.getPatchSetId())) {
LabelType type = labelTypes.byLabel(a.getLabelId());
if (type == null) {
continue;
} else if (type.isCopyMinScore() && type.isMaxNegative(a)) {
copied.add(new PatchSetApproval(dest.getId(), a));
} else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
copied.add(new PatchSetApproval(dest.getId(), a));
} else if (type.isCopyAllScoresOnTrivialRebase()
&& ChangeKind.TRIVIAL_REBASE.equals(changeKind)) {
copied.add(new PatchSetApproval(dest.getId(), a));
} else if (type.isCopyAllScoresIfNoCodeChange()
&& ChangeKind.NO_CODE_CHANGE.equals(changeKind)) {
copied.add(new PatchSetApproval(dest.getId(), a));
}
}
}
db.patchSetApprovals().insert(copied);
}
public List<PatchSetApproval> addReviewers(ReviewDb db,
ChangeUpdate update, LabelTypes labelTypes, Change change, PatchSet ps,
PatchSetInfo info, Iterable<Account.Id> wantReviewers,

20
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalCopier;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
@@ -40,7 +41,6 @@ import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.ssh.SshInfo;
import com.google.gerrit.server.util.TimeUtil;
@@ -88,7 +88,7 @@ public class PatchSetInserter {
private final MergeabilityChecker mergeabilityChecker;
private final ReplacePatchSetSender.Factory replacePatchSetFactory;
private final ApprovalsUtil approvalsUtil;
private final ChangeKindCache changeKindCache;
private final ApprovalCopier approvalCopier;
private final Repository git;
private final RevWalk revWalk;
@@ -110,12 +110,12 @@ public class PatchSetInserter {
ReviewDb db,
ChangeUpdate.Factory updateFactory,
ApprovalsUtil approvalsUtil,
ApprovalCopier approvalCopier,
PatchSetInfoFactory patchSetInfoFactory,
GitReferenceUpdated gitRefUpdated,
CommitValidators.Factory commitValidatorsFactory,
MergeabilityChecker mergeabilityChecker,
ReplacePatchSetSender.Factory replacePatchSetFactory,
ChangeKindCache changeKindCache,
@Assisted Repository git,
@Assisted RevWalk revWalk,
@Assisted ChangeControl ctl,
@@ -127,12 +127,12 @@ public class PatchSetInserter {
this.db = db;
this.updateFactory = updateFactory;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.patchSetInfoFactory = patchSetInfoFactory;
this.gitRefUpdated = gitRefUpdated;
this.commitValidatorsFactory = commitValidatorsFactory;
this.mergeabilityChecker = mergeabilityChecker;
this.replacePatchSetFactory = replacePatchSetFactory;
this.changeKindCache = changeKindCache;
this.git = git;
this.revWalk = revWalk;
@@ -272,17 +272,7 @@ public class PatchSetInserter {
}
if (copyLabels) {
PatchSet priorPatchSet = db.patchSets().get(currentPatchSetId);
ObjectId priorCommitId =
ObjectId.fromString(priorPatchSet.getRevision().get());
RevCommit priorCommit = revWalk.parseCommit(priorCommitId);
ProjectState projectState =
ctl.getProjectControl().getProjectState();
ChangeKind changeKind = changeKindCache.getChangeKind(
projectState, git, priorCommit, commit);
approvalsUtil.copyLabels(db, ctl.getNotes(),
ctl.getProjectControl().getLabelTypes(), currentPatchSetId,
patchSet, changeKind);
approvalCopier.copy(db, ctl, patchSet.getId());
}
db.commit();
update.commit();

9
gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
View File

@@ -33,6 +33,15 @@ import com.google.inject.Inject;
import java.util.Collection;
import java.util.List;
/**
* Normalizes votes on labels according to project config and permissions.
* <p>
* Votes are recorded in the database for a user based on the state of the
* project at that time: what labels are defined for the project, and what the
* user is allowed to vote on. Both of those can change between the time a vote
* is originally made and a later point, for example when a change is submitted.
* This class normalizes old votes against current project configuration.
*/
public class LabelNormalizer {
private final ChangeControl.GenericFactory changeFactory;
private final IdentifiedUser.GenericFactory userFactory;

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

@@ -18,7 +18,6 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.lib.RefDatabase.ALL;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED;
@@ -57,12 +56,12 @@ import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalCopier;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
@@ -70,8 +69,6 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeKind;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.MergeabilityChecker;
import com.google.gerrit.server.change.RevisionResource;
@@ -89,15 +86,14 @@ import com.google.gerrit.server.mail.CreateChangeSender;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.MergedSender;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.ssh.SshInfo;
import com.google.gerrit.server.util.MagicBranch;
import com.google.gerrit.server.util.RequestScopePropagator;
@@ -257,7 +253,7 @@ public class ReceiveCommits {
private final IdentifiedUser currentUser;
private final ReviewDb db;
private final ChangeNotes.Factory notesFactory;
private final ChangeData.Factory changeDataFactory;
private final ChangeUpdate.Factory updateFactory;
private final SchemaFactory<ReviewDb> schemaFactory;
private final AccountResolver accountResolver;
@@ -269,6 +265,7 @@ public class ReceiveCommits {
private final PatchSetInfoFactory patchSetInfoFactory;
private final ChangeHooks hooks;
private final ApprovalsUtil approvalsUtil;
private final ApprovalCopier approvalCopier;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
private final String canonicalWebUrl;
@@ -285,7 +282,6 @@ public class ReceiveCommits {
private final SshInfo sshInfo;
private final AllProjectsName allProjectsName;
private final ReceiveConfig receiveConfig;
private final ChangeKindCache changeKindCache;
private final ProjectControl projectControl;
private final Project project;
@@ -322,7 +318,7 @@ public class ReceiveCommits {
@Inject
ReceiveCommits(final ReviewDb db,
final SchemaFactory<ReviewDb> schemaFactory,
final ChangeNotes.Factory notesFactory,
final ChangeData.Factory changeDataFactory,
final ChangeUpdate.Factory updateFactory,
final AccountResolver accountResolver,
final CmdLineParser.Factory optionParserFactory,
@@ -333,6 +329,7 @@ public class ReceiveCommits {
final PatchSetInfoFactory patchSetInfoFactory,
final ChangeHooks hooks,
final ApprovalsUtil approvalsUtil,
final ApprovalCopier approvalCopier,
final ProjectCache projectCache,
final GitRepositoryManager repoManager,
final TagCache tagCache,
@@ -355,11 +352,10 @@ public class ReceiveCommits {
@Assisted final Repository repo,
final SubmoduleOp.Factory subOpFactory,
final Provider<Submit> submitProvider,
final MergeQueue mergeQueue,
final ChangeKindCache changeKindCache) throws IOException {
final MergeQueue mergeQueue) throws IOException {
this.currentUser = (IdentifiedUser) projectControl.getCurrentUser();
this.db = db;
this.notesFactory = notesFactory;
this.changeDataFactory = changeDataFactory;
this.updateFactory = updateFactory;
this.schemaFactory = schemaFactory;
this.accountResolver = accountResolver;
@@ -371,6 +367,7 @@ public class ReceiveCommits {
this.patchSetInfoFactory = patchSetInfoFactory;
this.hooks = hooks;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.projectCache = projectCache;
this.repoManager = repoManager;
this.canonicalWebUrl = canonicalWebUrl;
@@ -387,7 +384,6 @@ public class ReceiveCommits {
this.sshInfo = sshInfo;
this.allProjectsName = allProjectsName;
this.receiveConfig = config;
this.changeKindCache = changeKindCache;
this.projectControl = projectControl;
this.labelTypes = projectControl.getLabelTypes();
@@ -1680,7 +1676,6 @@ public class ReceiveCommits {
ChangeMessage msg;
String mergedIntoRef;
boolean skip;
ChangeKind changeKind;
private PatchSet.Id priorPatchSet;
ReplaceRequest(final Change.Id toChange, final RevCommit newCommit,
@@ -1689,7 +1684,6 @@ public class ReceiveCommits {
this.newCommit = newCommit;
this.inputCommand = cmd;
this.checkMergedInto = checkMergedInto;
this.changeKind = ChangeKind.REWORK;
revisions = HashBiMap.create();
for (Ref ref : refs(toChange)) {
@@ -1791,9 +1785,6 @@ public class ReceiveCommits {
}
}
changeKind = changeKindCache.getChangeKind(
projectControl.getProjectState(), repo, priorCommit, newCommit);
PatchSet.Id id =
ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId());
newPatchSet = new PatchSet(id);
@@ -1868,13 +1859,10 @@ public class ReceiveCommits {
mergedIntoRef = mergedInto != null ? mergedInto.getName() : null;
}
Collection<PatchSetApproval> oldChangeApprovals =
approvalsUtil.byChange(db, notesFactory.create(change)).values();
SetMultimap<ReviewerState, Account.Id> reviewers =
ApprovalsUtil.getReviewers(oldChangeApprovals);
MailRecipients oldRecipients = getRecipientsFromReviewers(reviewers);
approvalsUtil.copyLabels(db, labelTypes, oldChangeApprovals,
priorPatchSet, newPatchSet, changeKind);
ChangeData cd = changeDataFactory.create(db, changeCtl);
MailRecipients oldRecipients =
getRecipientsFromReviewers(cd.reviewers());
approvalCopier.copy(db, changeCtl, newPatchSet.getId());
approvalsUtil.addReviewers(db, update, labelTypes, change, newPatchSet,
info, recipients.getReviewers(), oldRecipients.getAll());
recipients.add(oldRecipients);