opendev/gerrit
Code Issues Proposed changes

Pass PatchSet not PatchSet.Id to canSubmit()

Browse Source 
All but one of the callers of canSubmit() already had the PatchSet
loaded in memory. Since canSubmit() generally needs the PatchSet
object, pass it through, avoiding a secondary database query.

Change-Id: I96fe842b3698deeb41612c61a78d54e9a6dbc4d6
This commit is contained in:
Shawn O. Pearce
2012-05-01 17:21:03 -07:00
parent f93b3eecec
commit ff29dca4de
8 changed files with 41 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
View File

@@ -134,7 +134,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
detail.setCanEdit(control.getRefControl().canWrite());
if (detail.getChange().getStatus().isOpen()) {
List<SubmitRecord> submitRecords = control.canSubmit(db, patch.getId());
List<SubmitRecord> submitRecords = control.canSubmit(db, patch);
for (SubmitRecord rec : submitRecords) {
if (rec.labels != null) {
for (SubmitRecord.Label lbl : rec.labels) {

5
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
View File

@@ -83,7 +83,8 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
final Change.Id changeId = patchSetId.getParentKey();
final ChangeControl control = changeControlFactory.validateFor(changeId);
change = control.getChange();
patchSetInfo = infoFactory.get(db, patchSetId);
PatchSet patchSet = db.patchSets().get(patchSetId);
patchSetInfo = infoFactory.get(change, patchSet);
drafts = db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList();
aic.want(change.getOwner());
@@ -119,7 +120,7 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
.toList();
boolean couldSubmit = false;
List<SubmitRecord> submitRecords = control.canSubmit(db, patchSetId);
List<SubmitRecord> submitRecords = control.canSubmit(db, patchSet);
for (SubmitRecord rec : submitRecords) {
if (rec.status == SubmitRecord.Status.OK) {
couldSubmit = true;

7
gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
View File

@@ -40,18 +40,19 @@ import org.eclipse.jgit.lib.Repository;
public final class StoredValues {
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<Change> CHANGE = create(Change.class);
public static final StoredValue<PatchSet.Id> PATCH_SET_ID = create(PatchSet.Id.class);
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);
public static final StoredValue<PatchSetInfo> PATCH_SET_INFO = new StoredValue<PatchSetInfo>() {
@Override
public PatchSetInfo createValue(Prolog engine) {
PatchSet.Id patchSetId = StoredValues.PATCH_SET_ID.get(engine);
Change change = StoredValues.CHANGE.get(engine);
PatchSet ps = StoredValues.PATCH_SET.get(engine);
PrologEnvironment env = (PrologEnvironment) engine.control;
PatchSetInfoFactory patchInfoFactory =
env.getInjector().getInstance(PatchSetInfoFactory.class);
try {
return patchInfoFactory.get(REVIEW_DB.get(engine), patchSetId);
return patchInfoFactory.get(change, ps);
} catch (PatchSetInfoNotAvailableException e) {
throw new SystemException(e.getMessage());
}

2
gerrit-server/src/main/java/com/google/gerrit/server/changedetail/Submit.java
View File

@@ -80,7 +80,7 @@ public class Submit implements Callable<ReviewResult> {
throw new NoSuchChangeException(changeId);
}
List<SubmitRecord> submitResult = control.canSubmit(db, patchSetId);
List<SubmitRecord> submitResult = control.canSubmit(db, patch);
if (submitResult.isEmpty()) {
throw new IllegalStateException(
"ChangeControl.canSubmit returned empty list");

30
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
View File

@@ -18,7 +18,6 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -29,6 +28,7 @@ import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
@@ -68,31 +68,39 @@ public class PatchSetInfoFactory {
}
public PatchSetInfo get(ReviewDb db, PatchSet.Id patchSetId)
throws PatchSetInfoNotAvailableException {
Repository repo = null;
throws PatchSetInfoNotAvailableException {
try {
final PatchSet patchSet = db.patchSets().get(patchSetId);
final Change change = db.changes().get(patchSet.getId().getParentKey());
final Project.NameKey projectKey = change.getProject();
repo = repoManager.openRepository(projectKey);
return get(change, patchSet);
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
}
}
public PatchSetInfo get(Change change, PatchSet patchSet)
throws PatchSetInfoNotAvailableException {
Repository repo;
try {
repo = repoManager.openRepository(change.getProject());
} catch (RepositoryNotFoundException e) {
throw new PatchSetInfoNotAvailableException(e);
}
try {
final RevWalk rw = new RevWalk(repo);
try {
final RevCommit src =
rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
PatchSetInfo info = get(src, patchSetId);
PatchSetInfo info = get(src, patchSet.getId());
info.setParents(toParentInfos(src.getParents(), rw));
return info;
} finally {
rw.release();
}
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
} catch (IOException e) {
throw new PatchSetInfoNotAvailableException(e);
} finally {
if (repo != null) {
repo.close();
}
repo.close();
}
}

18
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -278,34 +278,34 @@ public class ChangeControl {
return false;
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) {
if (change.getStatus().isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
}
if (!patchSetId.equals(change.currentPatchSetId())) {
return ruleError("Patch set " + patchSetId + " is not current");
if (!patchSet.getId().equals(change.currentPatchSetId())) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " is not current");
}
try {
if (change.getStatus() == Change.Status.DRAFT){
if (change.getStatus() == Change.Status.DRAFT) {
if (!isVisible(db)) {
return ruleError("Patch set " + patchSetId + " not found");
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft changes");
}
}
if (isDraftPatchSet(patchSetId, db)) {
if (patchSet.isDraft()) {
if (!isVisible(db)) {
return ruleError("Patch set " + patchSetId + " not found");
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft patch sets");
}
}
} catch (OrmException err) {
return logRuleError("Cannot read patch set " + patchSetId, err);
return logRuleError("Cannot read patch set " + patchSet.getId(), err);
}
List<Term> results = new ArrayList<Term>();
@@ -323,7 +323,7 @@ public class ChangeControl {
try {
env.set(StoredValues.REVIEW_DB, db);
env.set(StoredValues.CHANGE, change);
env.set(StoredValues.PATCH_SET_ID, patchSetId);
env.set(StoredValues.PATCH_SET, patchSet);
env.set(StoredValues.CHANGE_CONTROL, this);
submitRule = env.once(

4
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ListChanges.java
View File

@@ -229,7 +229,7 @@ public class ListChanges {
}
}
PatchSet.Id ps = in.currentPatchSetId();
PatchSet ps = cd.currentPatchSet(db);
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps)) {
if (rec.labels == null) {
@@ -273,7 +273,7 @@ public class ListChanges {
}
if (approvals == null) {
approvals = db.get().patchSetApprovals().byPatchSet(ps).toList();
approvals = db.get().patchSetApprovals().byPatchSet(ps.getId()).toList();
}
for (PatchSetApproval psa : approvals) {
short val = psa.getValue();

4
gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java
View File

@@ -44,10 +44,10 @@ class PRED__load_commit_labels_1 extends Predicate.P1 {
try {
PrologEnvironment env = (PrologEnvironment) engine.control;
ReviewDb db = StoredValues.REVIEW_DB.get(engine);
PatchSet.Id patchSetId = StoredValues.PATCH_SET_ID.get(engine);
PatchSet patchSet = StoredValues.PATCH_SET.get(engine);
ApprovalTypes types = env.getInjector().getInstance(ApprovalTypes.class);
for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(patchSetId)) {
for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(patchSet.getId())) {
if (a.getValue() == 0) {
continue;
}