From be4d7a7c9d0ae043d7131534336a421b18e58609 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 17 Dec 2015 12:49:37 -0500 Subject: [PATCH] Store SubmitTypeRecord lazily in ChangeData Add convenience method isOk() to SubmitTypeRecord to make it easier for callers to check whether this result was successful. Change-Id: I1551d5fdce208631f4d97d815fa882ff44502467 --- .../gerrit/common/data/SubmitTypeRecord.java | 4 ++++ .../com/google/gerrit/server/git/MergeOp.java | 13 ++++-------- .../gerrit/server/git/MergeSuperSet.java | 10 +++++----- .../server/query/change/ChangeData.java | 20 +++++++++++++------ .../query/change/ConflictsPredicate.java | 18 ++++------------- 5 files changed, 31 insertions(+), 34 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitTypeRecord.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitTypeRecord.java index ff10b2e215..cd226effb7 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitTypeRecord.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitTypeRecord.java @@ -56,6 +56,10 @@ public class SubmitTypeRecord { this.errorMessage = errorMessage; } + public boolean isOk() { + return status == Status.OK; + } + @Override public String toString() { StringBuilder sb = new StringBuilder(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 931b13be9d..2ddeba65fb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -697,7 +697,7 @@ public class MergeOp implements AutoCloseable { continue; } - SubmitType st = getSubmitType(cd, ps); + SubmitType st = getSubmitType(cd); if (st == null) { logProblem(changeId, "No submit type for change"); continue; @@ -742,15 +742,10 @@ public class MergeOp implements AutoCloseable { } } - private SubmitType getSubmitType(ChangeData cd, PatchSet ps) { + private SubmitType getSubmitType(ChangeData cd) { try { - SubmitTypeRecord r = new SubmitRuleEvaluator(cd).setPatchSet(ps) - .getSubmitType(); - if (r.status != SubmitTypeRecord.Status.OK) { - logError("Failed to get submit type for " + cd.getId()); - return null; - } - return r.type; + SubmitTypeRecord str = cd.submitTypeRecord(); + return str.isOk() ? str.type : null; } catch (OrmException e) { logError("Failed to get submit type for " + cd.getId(), e); return null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index c9a5c3e850..0c9ddee427 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -25,7 +25,6 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; @@ -104,11 +103,12 @@ public class MergeSuperSet { for (Change.Id cId : pc.get(project)) { ChangeData cd = changeDataFactory.create(db, cId); - SubmitTypeRecord r = new SubmitRuleEvaluator(cd).getSubmitType(); - if (r.status != SubmitTypeRecord.Status.OK) { - logErrorAndThrow("Failed to get submit type for " + cd.getId()); + SubmitTypeRecord str = cd.submitTypeRecord(); + if (!str.isOk()) { + logErrorAndThrow("Failed to get submit type for " + cd.getId() + + ": " + str.errorMessage); } - if (r.type == SubmitType.CHERRY_PICK) { + if (str.type == SubmitType.CHERRY_PICK) { ret.add(cd); continue; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index baf0b91283..f12ad85f80 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -316,6 +316,7 @@ public class ChangeData { private List messages; private List submitRecords; private ChangedLines changedLines; + private SubmitTypeRecord submitTypeRecord; private Boolean mergeable; private Set editsByUser; private Set reviewedBy; @@ -753,6 +754,13 @@ public class ChangeData { return submitRecords; } + public SubmitTypeRecord submitTypeRecord() throws OrmException { + if (submitTypeRecord == null) { + submitTypeRecord = new SubmitRuleEvaluator(this).getSubmitType(); + } + return submitTypeRecord; + } + public void setMergeable(Boolean mergeable) { this.mergeable = mergeable; } @@ -772,18 +780,18 @@ public class ChangeData { } try (Repository repo = repoManager.openRepository(c.getProject())) { Ref ref = repo.getRefDatabase().exactRef(c.getDest().get()); - SubmitTypeRecord rec = new SubmitRuleEvaluator(this) - .getSubmitType(); - if (rec.status != SubmitTypeRecord.Status.OK) { - throw new OrmException( - "Error in mergeability check: " + rec.errorMessage); + SubmitTypeRecord str = submitTypeRecord(); + if (!str.isOk()) { + // If submit type rules are broken, it's definitely not mergeable. + // No need to log, as SubmitRuleEvaluator already did it for us. + return false; } String mergeStrategy = mergeUtilFactory .create(projectCache.get(c.getProject())) .mergeStrategyName(); mergeable = mergeabilityCache.get( ObjectId.fromString(ps.getRevision().get()), - ref, rec.type, mergeStrategy, c.getDest(), repo); + ref, str.type, mergeStrategy, c.getDest(), repo); } catch (IOException e) { throw new OrmException(e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index 3c06198e03..1977847d11 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.query.change; import com.google.common.collect.Lists; import com.google.gerrit.common.data.SubmitTypeRecord; -import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.CodeReviewCommit; @@ -26,7 +25,6 @@ import com.google.gerrit.server.git.strategy.SubmitDryRun; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; -import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.query.OperatorPredicate; import com.google.gerrit.server.query.OrPredicate; import com.google.gerrit.server.query.Predicate; @@ -98,14 +96,14 @@ class ConflictsPredicate extends OrPredicate { if (!otherChange.getDest().equals(c.getDest())) { return false; } - SubmitType submitType = getSubmitType(object); - if (submitType == null) { + SubmitTypeRecord str = object.submitTypeRecord(); + if (!str.isOk()) { return false; } ObjectId other = ObjectId.fromString( object.currentPatchSet().getRevision().get()); ConflictKey conflictsKey = - new ConflictKey(changeDataCache.getTestAgainst(), other, submitType, + new ConflictKey(changeDataCache.getTestAgainst(), other, str.type, changeDataCache.getProjectState().isUseContentMerge()); Boolean conflicts = args.conflictsCache.getIfPresent(conflictsKey); if (conflicts != null) { @@ -115,7 +113,7 @@ class ConflictsPredicate extends OrPredicate { args.repoManager.openRepository(otherChange.getProject()); CodeReviewRevWalk rw = CodeReviewCommit.newRevWalk(repo)) { conflicts = !args.submitDryRun.run( - submitType, repo, rw, otherChange.getDest(), + str.type, repo, rw, otherChange.getDest(), changeDataCache.getTestAgainst(), other, getAlreadyAccepted(repo, rw)); args.conflictsCache.put(conflictsKey, conflicts); @@ -131,14 +129,6 @@ class ConflictsPredicate extends OrPredicate { return 5; } - private SubmitType getSubmitType(ChangeData cd) throws OrmException { - SubmitTypeRecord r = new SubmitRuleEvaluator(cd).getSubmitType(); - if (r.status != SubmitTypeRecord.Status.OK) { - return null; - } - return r.type; - } - private Set getAlreadyAccepted(Repository repo, RevWalk rw) throws IntegrationException { try {