opendev/gerrit
Code Issues Proposed changes

Allow file:^regex to match affected files

Browse Source 
For performance reasons we only permit the file: operator in the
watched project filters.  We have the data on hand in memory for
formatting in the message, so its reasonably cheap to scan through
filters using the file operator.  But doing it against a database
query is just too slow, odds are the file list isn't in the diff
cache and thus has to be computed on the fly.  At 200ms up to as many
as 4 seconds per change, this is simply unacceptable performance.

Bug: issue 70
Change-Id: I15ceb262419132a32c828e4051853b15ab963876
Signed-off-by: Shawn O. Pearce <sop@google.com>
This commit is contained in:
Shawn O. Pearce
2010-07-19 17:58:25 -07:00
parent ad695110ed
commit 50756b9c1b
8 changed files with 147 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
Documentation/user-search.txt
View File

@@ -130,6 +130,15 @@ Matches changes where the approval score 'VALUE' has been set during
a review. See <<labels,labels>> below for more detail on the format
of the argument.
[[file]]
file:\^'REGEX'::
+
Matches any change where REGEX matches a file that was affected
by the change. The regular expression pattern must start with
'\^'. For example, to match all XML files use `file:^.*\.xml$`.
Currently this operator is only available on a watched project
and may not be used in the search bar.
[[has]]
has:draft::
+

4
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountServiceImpl.java
View File

@@ -154,7 +154,9 @@ class AccountServiceImpl extends BaseServiceImplementation implements
if (filter != null) {
try {
queryBuilder.create(currentUser.get()).parse(filter);
ChangeQueryBuilder builder = queryBuilder.create(currentUser.get());
builder.setAllowFile(true);
builder.parse(filter);
} catch (QueryParseException badFilter) {
throw new InvalidQueryException(badFilter.getMessage(), filter);
}

9
gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java
View File

@@ -27,7 +27,9 @@ import org.eclipse.jgit.lib.Repository;
import java.io.IOException;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
/** Send comments, after the author of them hit used Publish Comments in the UI. */
public class CommentSender extends ReplyToChangeSender {
@@ -44,6 +46,13 @@ public class CommentSender extends ReplyToChangeSender {
public void setPatchLineComments(final List<PatchLineComment> plc) {
inlineComments = plc;
Set<String> paths = new HashSet<String>();
for (PatchLineComment c : plc) {
Patch.Key p = c.getKey().getParentKey();
paths.add(p.getFileName());
}
changeData.setCurrentFilePaths(paths);
}
@Override

6
gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
View File

@@ -81,11 +81,12 @@ public abstract class OutgoingEmail {
protected ChangeMessage changeMessage;
private ProjectState projectState;
private ChangeData changeData;
protected ChangeData changeData;
protected OutgoingEmail(EmailArguments ea, final Change c, final String mc) {
args = ea;
change = c;
changeData = change != null ? new ChangeData(change) : null;
messageClass = mc;
headers = new LinkedHashMap<String, EmailHeader>();
}
@@ -223,12 +224,10 @@ public abstract class OutgoingEmail {
/** Setup the message headers and envelope (TO, CC, BCC). */
protected void init() {
if (change != null && args.projectCache != null) {
changeData = new ChangeData(change);
projectState = args.projectCache.get(change.getProject());
projectName =
projectState != null ? projectState.getProject().getName() : null;
} else {
changeData = null;
projectState = null;
projectName = null;
}
@@ -646,6 +645,7 @@ public abstract class OutgoingEmail {
Predicate<ChangeData> p = qb.is_visible();
if (w.getFilter() != null) {
try {
qb.setAllowFile(true);
p = Predicate.and(qb.parse(w.getFilter()), p);
p = args.queryRewriter.get().rewrite(p);
if (p.match(changeData)) {

43
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -21,6 +21,9 @@ import com.google.gerrit.reviewdb.PatchSetApproval;
import com.google.gerrit.reviewdb.ReviewDb;
import com.google.gerrit.reviewdb.TrackingId;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListEntry;
import com.google.gwtorm.client.OrmException;
import com.google.inject.Provider;
@@ -35,6 +38,7 @@ public class ChangeData {
private Collection<PatchSet> patches;
private Collection<PatchSetApproval> approvals;
private Collection<PatchSetApproval> currentApprovals;
private Collection<String> currentFiles;
private Collection<PatchLineComment> comments;
private Collection<TrackingId> trackingIds;
private CurrentUser visibleTo;
@@ -48,6 +52,45 @@ public class ChangeData {
change = c;
}
public void setCurrentFilePaths(Collection<String> filePaths) {
currentFiles = filePaths;
}
public Collection<String> currentFilePaths(Provider<ReviewDb> db,
PatchListCache cache) throws OrmException {
if (currentFiles == null) {
Change c = change(db);
if (c == null) {
return null;
}
PatchSet ps = currentPatchSet(db);
if (ps == null) {
return null;
}
PatchList p = cache.get(c, ps);
List<String> r = new ArrayList<String>(p.getPatches().size());
for (PatchListEntry e : p.getPatches()) {
switch (e.getChangeType()) {
case ADDED:
case MODIFIED:
case COPIED:
r.add(e.getNewName());
break;
case DELETED:
r.add(e.getOldName());
break;
case RENAMED:
r.add(e.getOldName());
r.add(e.getNewName());
break;
}
}
currentFiles = r;
}
return currentFiles;
}
public Change.Id getId() {
return legacyId;
}

25
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.config.WildProjectName;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.IntPredicate;
import com.google.gerrit.server.query.Predicate;
@@ -66,6 +67,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public static final String FIELD_CHANGE = "change";
public static final String FIELD_COMMIT = "commit";
public static final String FIELD_DRAFTBY = "draftby";
public static final String FIELD_FILE = "file";
public static final String FIELD_IS = "is";
public static final String FIELD_HAS = "has";
public static final String FIELD_LABEL = "label";
@@ -96,6 +98,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final AuthConfig authConfig;
final ApprovalTypes approvalTypes;
final Project.NameKey wildProjectName;
final PatchListCache patchListCache;
@Inject
Arguments(Provider<ReviewDb> dbProvider,
@@ -105,7 +108,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
ChangeControl.GenericFactory changeControlGenericFactory,
AccountResolver accountResolver, GroupCache groupCache,
AuthConfig authConfig, ApprovalTypes approvalTypes,
@WildProjectName Project.NameKey wildProjectName) {
@WildProjectName Project.NameKey wildProjectName,
PatchListCache patchListCache) {
this.dbProvider = dbProvider;
this.rewriter = rewriter;
this.userFactory = userFactory;
@@ -116,6 +120,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
this.authConfig = authConfig;
this.approvalTypes = approvalTypes;
this.wildProjectName = wildProjectName;
this.patchListCache = patchListCache;
}
}
@@ -125,6 +130,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
private final Arguments args;
private final CurrentUser currentUser;
private boolean allowsFile;
@Inject
ChangeQueryBuilder(Arguments args, @Assisted CurrentUser currentUser) {
@@ -133,6 +139,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
this.currentUser = currentUser;
}
public void setAllowFile(boolean on) {
allowsFile = on;
}
@Operator
public Predicate<ChangeData> age(String value) {
return new AgePredicate(args.dbProvider, value);
@@ -243,6 +253,19 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return new RefPredicate(args.dbProvider, ref);
}
@Operator
public Predicate<ChangeData> file(String file) throws QueryParseException {
if (!allowsFile) {
throw error("operator not permitted here: file:" + file);
}
if (file.startsWith("^")) {
return new RegexFilePredicate(args.dbProvider, args.patchListCache, file);
}
throw new IllegalArgumentException();
}
@Operator
public Predicate<ChangeData> label(String name) {
return new LabelPredicate(args.changeControlGenericFactory,

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java
View File

@@ -38,7 +38,7 @@ public class ChangeQueryRewriter extends QueryRewriter<ChangeData> {
new ChangeQueryBuilder.Arguments( //
new InvalidProvider<ReviewDb>(), //
new InvalidProvider<ChangeQueryRewriter>(), //
null, null, null, null, null, null, null, null),
null, null, null, null, null, null, null, null, null),
null));
private final Provider<ReviewDb> dbProvider;

55
gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexFilePredicate.java Normal file
View File

@@ -0,0 +1,55 @@
// Copyright 2010 Google Inc. All Rights Reserved.
package com.google.gerrit.server.query.change;
import com.google.gerrit.reviewdb.ReviewDb;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.query.OperatorPredicate;
import com.google.gwtorm.client.OrmException;
import com.google.inject.Provider;
import java.util.Collection;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
class RegexFilePredicate extends OperatorPredicate<ChangeData> {
private final Provider<ReviewDb> db;
private final PatchListCache cache;
private final Pattern pattern;
RegexFilePredicate(Provider<ReviewDb> db, PatchListCache plc, String re) {
super(ChangeQueryBuilder.FIELD_FILE, re);
this.db = db;
this.cache = plc;
try {
this.pattern = Pattern.compile(re);
} catch (PatternSyntaxException e) {
throw new IllegalArgumentException(e.getMessage());
}
}
@Override
public boolean match(ChangeData object) throws OrmException {
Collection<String> files = object.currentFilePaths(db, cache);
if (files != null) {
for (String path : files) {
if (pattern.matcher(path).find()) {
return true;
}
}
return false;
} else {
// The ChangeData can't do expensive lookups right now. Bypass
// them and include the result anyway. We might be able to do
// a narrow later on to a smaller set.
//
return true;
}
}
@Override
public int getCost() {
return 1;
}
}