Add a helper method to fetch the commit message

The PRED_commit_message_1 rule doesn't need the whole PatchSetInfo
values to be loaded to function properly: only the commit message is
required.

This change improves this by only loading the commit message from the
repository.
Fix a test case: the commit_message predicate does not use the account
index anymore. Before that, it was indirectly used to gather
informations on the author and committer. These two informations were
loaded but never used.

Change-Id: I3a964bae3512ee6fba7895739f32fd592e8595bf
This commit is contained in:
Maxime Guerreiro 2018-05-03 15:50:22 +00:00
parent bcba0f92ed
commit 05ce4c2414
6 changed files with 50 additions and 9 deletions

View File

@ -32,8 +32,10 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
@ -49,6 +51,8 @@ import java.util.Collections;
import java.util.List;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
/** Utilities for manipulating patch sets. */
@ -58,17 +62,20 @@ public class PatchSetUtil {
private final Provider<ApprovalsUtil> approvalsUtilProvider;
private final ProjectCache projectCache;
private final Provider<ReviewDb> dbProvider;
private final GitRepositoryManager repoManager;
@Inject
PatchSetUtil(
NotesMigration migration,
Provider<ApprovalsUtil> approvalsUtilProvider,
ProjectCache projectCache,
Provider<ReviewDb> dbProvider) {
Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager) {
this.migration = migration;
this.approvalsUtilProvider = approvalsUtilProvider;
this.projectCache = projectCache;
this.dbProvider = dbProvider;
this.repoManager = repoManager;
}
public PatchSet current(ReviewDb db, ChangeNotes notes) throws OrmException {
@ -206,4 +213,15 @@ public class PatchSetUtil {
}
return false;
}
/** Returns the full commit message for the given project at the given patchset revision */
public String getFullCommitMessage(Project.NameKey project, PatchSet patchSet)
throws IOException {
try (Repository repo = repoManager.openRepository(project);
RevWalk rw = new RevWalk(repo)) {
RevCommit src = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
rw.parseBody(src);
return src.getFullMessage();
}
}
}

View File

@ -16,6 +16,7 @@ package com.google.gerrit.server.rules;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchListCache;
@ -175,6 +176,7 @@ public class PrologEnvironment extends BufferingPrologControl {
private final Provider<AnonymousUser> anonymousUser;
private final int reductionLimit;
private final int compileLimit;
private final PatchSetUtil patchsetUtil;
@Inject
Args(
@ -185,7 +187,8 @@ public class PrologEnvironment extends BufferingPrologControl {
PatchSetInfoFactory patchSetInfoFactory,
IdentifiedUser.GenericFactory userFactory,
Provider<AnonymousUser> anonymousUser,
@GerritServerConfig Config config) {
@GerritServerConfig Config config,
PatchSetUtil patchsetUtil) {
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.repositoryManager = repositoryManager;
@ -193,6 +196,7 @@ public class PrologEnvironment extends BufferingPrologControl {
this.patchSetInfoFactory = patchSetInfoFactory;
this.userFactory = userFactory;
this.anonymousUser = anonymousUser;
this.patchsetUtil = patchsetUtil;
int limit = config.getInt("rules", null, "reductionLimit", 100000);
reductionLimit = limit <= 0 ? Integer.MAX_VALUE : limit;
@ -240,5 +244,9 @@ public class PrologEnvironment extends BufferingPrologControl {
public AnonymousUser getAnonymousUser() {
return anonymousUser.get();
}
public PatchSetUtil getPatchsetUtil() {
return patchsetUtil;
}
}
}

View File

@ -25,6 +25,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.Emails;
@ -89,6 +90,22 @@ public final class StoredValues {
}
};
public static final StoredValue<String> COMMIT_MESSAGE =
new StoredValue<String>() {
@Override
public String createValue(Prolog engine) {
Change change = getChange(engine);
PatchSet ps = getPatchSet(engine);
PrologEnvironment env = (PrologEnvironment) engine.control;
PatchSetUtil patchSetUtil = env.getArgs().getPatchsetUtil();
try {
return patchSetUtil.getFullCommitMessage(change.getProject(), ps);
} catch (IOException e) {
throw new SystemException(e.getMessage());
}
}
};
public static final StoredValue<PatchList> PATCH_LIST =
new StoredValue<PatchList>() {
@Override

View File

@ -14,7 +14,6 @@
package gerrit;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.server.rules.StoredValues;
import com.googlecode.prolog_cafe.exceptions.PrologException;
import com.googlecode.prolog_cafe.lang.Operation;
@ -41,9 +40,9 @@ public class PRED_commit_message_1 extends Predicate.P1 {
engine.setB0();
Term a1 = arg1.dereference();
PatchSetInfo psInfo = StoredValues.PATCH_SET_INFO.get(engine);
String commitMessage = StoredValues.COMMIT_MESSAGE.get(engine);
SymbolTerm msg = SymbolTerm.create(psInfo.getMessage());
SymbolTerm msg = SymbolTerm.create(commitMessage);
if (!a1.unify(msg, engine.trail)) {
return engine.fail();
}

View File

@ -65,10 +65,8 @@ public class RulesIT extends AbstractDaemonTest {
@Test
public void testUnresolvedCommentsCount() throws Exception {
// This test results in a RULE_ERROR as Prolog tries to find accounts by email, using the index.
// TODO(maximeg) get OK results
modifySubmitRules("gerrit:commit_message_matches('.*')");
assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.RULE_ERROR);
assertThat(statusForRule()).isEqualTo(SubmitRecord.Status.OK);
}
@Test

View File

@ -48,7 +48,8 @@ public class GerritCommonTest extends PrologTestCase {
cfg.setInt("rules", null, "compileReductionLimit", (int) 1e6);
bind(PrologEnvironment.Args.class)
.toInstance(
new PrologEnvironment.Args(null, null, null, null, null, null, null, cfg));
new PrologEnvironment.Args(
null, null, null, null, null, null, null, cfg, null));
}
});
}