From 7ecae33e24723964b354a829fde21131477a58db Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 2 Aug 2017 08:40:51 -0700 Subject: [PATCH] Index files affected by merge commits This commit has the effect of changing ChangeData.currentFilePaths() to return the diff of a merge commit to be between the merge commit's new tree, and its 1st parent. That allows ChangeField.PATH to index the files affected by a merge commit, which supports the conflicts operator to identify open merges that may conflict with a change. The only caller of PatchListCache.getDiffSummary() is ChangeData.currentFilePaths(). The only caller of ChangeData.currentFilePaths() is ChangeField.PATH (for indexing) and query operators that test against a change. A side-effect of this commit is the changed lines (added, deleted, delta) for merge commits will now report the delta introduced by the merge, rather than the delta from the automerge. The conflicts operator now detects a few less cases of conflicts. The most obvious change is a merge uploaded to the wrong branch may not show a conflict with other changes on its current branch when its 1st parent (from the other branch) introduces a conflicting change. The conflicts operator now assumes merges are made "correctly", such that `git log --first-parent` will traverse the main-branch history, and not the side-branch. This requires users to do something like: git checkout -b my-merge origin/master git merge side-branch git push origin my-merge:refs/for/master When creating a merge commit to merge "side-branch" into "master". Since this is a typical workflow with git, most merges should already fit this expected pattern. A new change index schema version 46 is required to ensure changes are reindexed with this new computation for the PATH field. Bug: issue 3052 Change-Id: Iaa854447ed9abb77b3f8fe6c818723e804e0a8d4 --- .../gerrit/server/change/PostReview.java | 25 ++-- .../git/validators/MergeValidators.java | 2 +- .../server/index/change/ChangeField.java | 9 +- .../index/change/ChangeSchemaDefinitions.java | 4 +- .../gerrit/server/patch/PatchListCache.java | 3 - .../server/patch/PatchListCacheImpl.java | 10 -- .../server/query/change/ChangeData.java | 98 +++++++--------- .../query/change/ConflictsPredicate.java | 109 ++++++------------ .../query/change/EqualsPathPredicate.java | 10 +- .../query/change/RegexPathPredicate.java | 15 ++- 10 files changed, 118 insertions(+), 167 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 634cea06f9..58634a5a52 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -50,6 +50,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.api.changes.ReviewResult; import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.client.Comment.Range; +import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.AccountInfo; @@ -92,7 +93,11 @@ import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.patch.DiffSummary; +import com.google.gerrit.server.patch.DiffSummaryKey; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.LabelPermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -129,6 +134,7 @@ import java.util.OptionalInt; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; @Singleton public class PostReview @@ -206,14 +212,14 @@ public class PostReview protected Response applyImpl( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException, ConfigInvalidException { + PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException { return apply(updateFactory, revision, input, TimeUtil.nowTs()); } public Response apply( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input, Timestamp ts) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException, ConfigInvalidException { + PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException { // Respect timestamp, but truncate at change created-on time. ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn()); if (revision.getEdit().isPresent()) { @@ -553,7 +559,7 @@ public class PostReview private void checkComments( RevisionResource revision, Map> commentsPerPath) - throws OrmException, BadRequestException { + throws BadRequestException, PatchListNotAvailableException { Set revisionFilePaths = getAffectedFilePaths(revision); for (Map.Entry> entry : commentsPerPath.entrySet()) { String path = entry.getKey(); @@ -569,9 +575,14 @@ public class PostReview } } - private Set getAffectedFilePaths(RevisionResource revision) throws OrmException { - ChangeData changeData = changeDataFactory.create(db.get(), revision.getNotes()); - return new HashSet<>(changeData.filePaths(revision.getPatchSet())); + private Set getAffectedFilePaths(RevisionResource revision) + throws PatchListNotAvailableException { + ObjectId newId = ObjectId.fromString(revision.getPatchSet().getRevision().get()); + DiffSummaryKey key = + DiffSummaryKey.fromPatchListKey( + PatchListKey.againstDefaultBase(newId, Whitespace.IGNORE_NONE)); + DiffSummary ds = patchListCache.getDiffSummary(key, revision.getProject()); + return new HashSet<>(ds.getPaths()); } private static void ensurePathRefersToAvailableOrMagicFile( @@ -600,7 +611,7 @@ public class PostReview private void checkRobotComments( RevisionResource revision, Map> in) - throws BadRequestException, OrmException { + throws BadRequestException, PatchListNotAvailableException { cleanUpComments(in); for (Map.Entry> e : in.entrySet()) { String commentPath = e.getKey(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java index 51e47a0605..8ccf081e3f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java @@ -267,7 +267,7 @@ public class MergeValidators { if (!cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) { return; } - } catch (OrmException e) { + } catch (IOException | OrmException e) { log.error("Cannot validate account update", e); throw new MergeValidationException("account validation unavailable"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index a1c7f141a5..24bde800a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java @@ -145,10 +145,13 @@ public class ChangeField { .buildRepeatable(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of())); public static Set getFileParts(ChangeData cd) throws OrmException { - List paths = cd.currentFilePaths(); - if (paths == null) { - return ImmutableSet.of(); + List paths; + try { + paths = cd.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); } + Splitter s = Splitter.on('/').omitEmptyStrings(); Set r = new HashSet<>(); for (String path : paths) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index 129c8ac90e..41e8d1096b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java @@ -86,7 +86,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { ChangeField.PENDING_REVIEWER, ChangeField.PENDING_REVIEWER_BY_EMAIL); - static final Schema V45 = schema(V44, ChangeField.REVERT_OF); + @Deprecated static final Schema V45 = schema(V44, ChangeField.REVERT_OF); + + static final Schema V46 = schema(V45); public static final String NAME = "changes"; public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java index c32a3f6254..728d227123 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java @@ -30,9 +30,6 @@ public interface PatchListCache { IntraLineDiff getIntraLineDiff(IntraLineDiffKey key, IntraLineDiffArgs args); - DiffSummary getDiffSummary(Change change, PatchSet patchSet) - throws PatchListNotAvailableException; - DiffSummary getDiffSummary(DiffSummaryKey key, Project.NameKey project) throws PatchListNotAvailableException; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index b985723e6d..77774004d3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -163,16 +163,6 @@ public class PatchListCacheImpl implements PatchListCache { return new IntraLineDiff(IntraLineDiff.Status.DISABLED); } - @Override - public DiffSummary getDiffSummary(Change change, PatchSet patchSet) - throws PatchListNotAvailableException { - Project.NameKey project = change.getProject(); - ObjectId b = ObjectId.fromString(patchSet.getRevision().get()); - Whitespace ws = Whitespace.IGNORE_NONE; - return getDiffSummary( - DiffSummaryKey.fromPatchListKey(PatchListKey.againstDefaultBase(b, ws)), project); - } - @Override public DiffSummary getDiffSummary(DiffSummaryKey key, Project.NameKey project) throws PatchListNotAvailableException { 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 f8443ff2c6..30bb7605c6 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 @@ -35,6 +35,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; +import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Account; @@ -67,7 +68,9 @@ import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.DiffSummary; +import com.google.gerrit.server.patch.DiffSummaryKey; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; @@ -367,8 +370,8 @@ public class ChangeData { private Collection patchSets; private ListMultimap allApprovals; private List currentApprovals; - private Map> files; - private Map> diffSummaries; + private List currentFiles; + private Optional diffSummary; private Collection publishedComments; private Collection robotComments; private CurrentUser visibleTo; @@ -391,6 +394,7 @@ public class ChangeData { private List reviewerUpdates; private PersonIdent author; private PersonIdent committer; + private int parentCount; private Integer unresolvedCommentCount; private LabelTypes labelTypes; @@ -466,86 +470,61 @@ public class ChangeData { return allUsersName; } - private Map> initFiles() { - if (files == null) { - files = new HashMap<>(); - } - return files; - } - public void setCurrentFilePaths(List filePaths) throws OrmException { PatchSet ps = currentPatchSet(); if (ps != null) { - initFiles().put(ps.getPatchSetId(), ImmutableList.copyOf(filePaths)); + currentFiles = ImmutableList.copyOf(filePaths); } } - public List currentFilePaths() throws OrmException { - PatchSet ps = currentPatchSet(); - return ps != null ? filePaths(ps) : null; - } - - public List filePaths(PatchSet ps) throws OrmException { - Integer psId = ps.getPatchSetId(); - List r = initFiles().get(psId); - if (r == null) { - Change c = change(); - if (c == null) { - return null; + public List currentFilePaths() throws IOException, OrmException { + if (currentFiles == null) { + if (!lazyLoad) { + return Collections.emptyList(); } - - Optional p = getDiffSummary(c, ps); - if (!p.isPresent()) { - List emptyFileList = Collections.emptyList(); - if (lazyLoad) { - files.put(ps.getPatchSetId(), emptyFileList); - } - return emptyFileList; - } - - r = p.get().getPaths(); - files.put(psId, r); + Optional p = getDiffSummary(); + currentFiles = p.map(DiffSummary::getPaths).orElse(Collections.emptyList()); } - return r; + return currentFiles; } - private Optional getDiffSummary(Change c, PatchSet ps) { - Integer psId = ps.getId().get(); - if (diffSummaries == null) { - diffSummaries = new HashMap<>(); - } - Optional r = diffSummaries.get(psId); - if (r == null) { + private Optional getDiffSummary() throws OrmException, IOException { + if (diffSummary == null) { if (!lazyLoad) { return Optional.empty(); } - try { - r = Optional.of(patchListCache.getDiffSummary(c, ps)); - } catch (PatchListNotAvailableException e) { - r = Optional.empty(); + + Change c = change(); + PatchSet ps = currentPatchSet(); + if (c == null || ps == null || !loadCommitData()) { + return Optional.empty(); + } + + ObjectId id = ObjectId.fromString(ps.getRevision().get()); + Whitespace ws = Whitespace.IGNORE_NONE; + PatchListKey pk = + parentCount > 1 + ? PatchListKey.againstParentNum(1, id, ws) + : PatchListKey.againstDefaultBase(id, ws); + DiffSummaryKey key = DiffSummaryKey.fromPatchListKey(pk); + try { + diffSummary = Optional.of(patchListCache.getDiffSummary(key, c.getProject())); + } catch (PatchListNotAvailableException e) { + diffSummary = Optional.empty(); } - diffSummaries.put(psId, r); } - return r; + return diffSummary; } - private Optional computeChangedLines() throws OrmException { - Change c = change(); - if (c == null) { - return Optional.empty(); - } - PatchSet ps = currentPatchSet(); - if (ps == null) { - return Optional.empty(); - } - Optional ds = getDiffSummary(c, ps); + private Optional computeChangedLines() throws OrmException, IOException { + Optional ds = getDiffSummary(); if (ds.isPresent()) { return Optional.of(ds.get().getChangedLines()); } return Optional.empty(); } - public Optional changedLines() throws OrmException { + public Optional changedLines() throws OrmException, IOException { if (changedLines == null) { if (!lazyLoad) { return Optional.empty(); @@ -744,6 +723,7 @@ public class ChangeData { commitFooters = c.getFooterLines(); author = c.getAuthorIdent(); committer = c.getCommitterIdent(); + parentCount = c.getParentCount(); } return true; } 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 7890eddd45..064e08e17e 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 @@ -17,8 +17,8 @@ package com.google.gerrit.server.query.change; import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.index.query.Predicate; import com.google.gerrit.index.query.QueryParseException; +import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.IntegrationException; @@ -28,20 +28,15 @@ import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; import com.google.gwtorm.server.OrmException; -import com.google.inject.Provider; import java.io.IOException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; -import org.eclipse.jgit.revwalk.filter.RevFilter; -import org.eclipse.jgit.treewalk.TreeWalk; -import org.eclipse.jgit.treewalk.filter.TreeFilter; public class ConflictsPredicate { // UI code may depend on this string, so use caution when changing. @@ -51,9 +46,15 @@ public class ConflictsPredicate { public static Predicate create(Arguments args, String value, Change c) throws QueryParseException, OrmException { - ChangeDataCache changeDataCache = - new ChangeDataCache(c, args.db, args.changeDataFactory, args.projectCache); - List files = listFiles(c, args, changeDataCache); + ChangeData cd; + List files; + try { + cd = args.changeDataFactory.create(args.db.get(), c); + files = cd.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); + } + if (3 + files.size() > args.indexConfig.maxTerms()) { // Short-circuit with a nice error message if we exceed the index // backend's term limit. This assumes that "conflicts:foo" is the entire @@ -73,61 +74,29 @@ public class ConflictsPredicate { and.add(new RefPredicate(c.getDest().get())); and.add(Predicate.not(new LegacyChangeIdPredicate(c.getId()))); and.add(Predicate.or(filePredicates)); + + ChangeDataCache changeDataCache = new ChangeDataCache(cd, args.projectCache); and.add(new CheckConflict(ChangeQueryBuilder.FIELD_CONFLICTS, value, args, c, changeDataCache)); return Predicate.and(and); } - private static List listFiles(Change c, Arguments args, ChangeDataCache changeDataCache) - throws OrmException { - try (Repository repo = args.repoManager.openRepository(c.getProject()); - RevWalk rw = new RevWalk(repo)) { - RevCommit ps = rw.parseCommit(changeDataCache.getTestAgainst()); - if (ps.getParentCount() > 1) { - String dest = c.getDest().get(); - Ref destBranch = repo.getRefDatabase().exactRef(dest); - rw.setRevFilter(RevFilter.MERGE_BASE); - rw.markStart(rw.parseCommit(destBranch.getObjectId())); - rw.markStart(ps); - RevCommit base = rw.next(); - // TODO(zivkov): handle the case with multiple merge bases - - List files = new ArrayList<>(); - try (TreeWalk tw = new TreeWalk(repo)) { - if (base != null) { - tw.setFilter(TreeFilter.ANY_DIFF); - tw.addTree(base.getTree()); - } - tw.addTree(ps.getTree()); - tw.setRecursive(true); - while (tw.next()) { - files.add(tw.getPathString()); - } - } - return files; - } - return args.changeDataFactory.create(args.db.get(), c).currentFilePaths(); - } catch (IOException e) { - throw new OrmException(e); - } - } - private static final class CheckConflict extends ChangeOperatorPredicate { private final Arguments args; - private final Change c; + private final Branch.NameKey dest; private final ChangeDataCache changeDataCache; CheckConflict( String field, String value, Arguments args, Change c, ChangeDataCache changeDataCache) { super(field, value); this.args = args; - this.c = c; + this.dest = c.getDest(); this.changeDataCache = changeDataCache; } @Override public boolean match(ChangeData object) throws OrmException { Change otherChange = object.change(); - if (otherChange == null || !otherChange.getDest().equals(c.getDest())) { + if (otherChange == null || !otherChange.getDest().equals(dest)) { return false; } @@ -136,13 +105,17 @@ public class ConflictsPredicate { return false; } + ProjectState projectState; + try { + projectState = changeDataCache.getProjectState(); + } catch (NoSuchProjectException | OrmException e) { + return false; + } + ObjectId other = ObjectId.fromString(object.currentPatchSet().getRevision().get()); ConflictKey conflictsKey = new ConflictKey( - changeDataCache.getTestAgainst(), - other, - str.type, - changeDataCache.getProjectState().isUseContentMerge()); + changeDataCache.getTestAgainst(), other, str.type, projectState.isUseContentMerge()); Boolean conflicts = args.conflictsCache.getIfPresent(conflictsKey); if (conflicts != null) { return conflicts; @@ -187,41 +160,31 @@ public class ConflictsPredicate { } } - public static class ChangeDataCache { - protected final Change change; - protected final Provider db; - protected final ChangeData.Factory changeDataFactory; - protected final ProjectCache projectCache; + private static class ChangeDataCache { + private final ChangeData cd; + private final ProjectCache projectCache; - protected ObjectId testAgainst; - protected ProjectState projectState; - protected Set alreadyAccepted; + private ObjectId testAgainst; + private ProjectState projectState; + private Set alreadyAccepted; - public ChangeDataCache( - Change change, - Provider db, - ChangeData.Factory changeDataFactory, - ProjectCache projectCache) { - this.change = change; - this.db = db; - this.changeDataFactory = changeDataFactory; + ChangeDataCache(ChangeData cd, ProjectCache projectCache) { + this.cd = cd; this.projectCache = projectCache; } - protected ObjectId getTestAgainst() throws OrmException { + ObjectId getTestAgainst() throws OrmException { if (testAgainst == null) { - testAgainst = - ObjectId.fromString( - changeDataFactory.create(db.get(), change).currentPatchSet().getRevision().get()); + testAgainst = ObjectId.fromString(cd.currentPatchSet().getRevision().get()); } return testAgainst; } - protected ProjectState getProjectState() { + ProjectState getProjectState() throws OrmException, NoSuchProjectException { if (projectState == null) { - projectState = projectCache.get(change.getProject()); + projectState = projectCache.get(cd.project()); if (projectState == null) { - throw new IllegalStateException(new NoSuchProjectException(change.getProject())); + throw new NoSuchProjectException(cd.project()); } } return projectState; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java index 56ed7972a5..fc00283e87 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.query.change; import com.google.gerrit.server.index.change.ChangeField; import com.google.gwtorm.server.OrmException; +import java.io.IOException; import java.util.Collections; import java.util.List; @@ -26,8 +27,13 @@ public class EqualsPathPredicate extends ChangeIndexPredicate { @Override public boolean match(ChangeData object) throws OrmException { - List files = object.currentFilePaths(); - return files != null && Collections.binarySearch(files, value) >= 0; + List files; + try { + files = object.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); + } + return Collections.binarySearch(files, value) >= 0; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java index ca21247ac7..46b4cd5860 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.query.change; import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.util.RegexListSearcher; import com.google.gwtorm.server.OrmException; +import java.io.IOException; import java.util.List; public class RegexPathPredicate extends ChangeRegexPredicate { @@ -26,15 +27,13 @@ public class RegexPathPredicate extends ChangeRegexPredicate { @Override public boolean match(ChangeData object) throws OrmException { - List files = object.currentFilePaths(); - if (files != null) { - return RegexListSearcher.ofStrings(getValue()).hasMatch(files); + List files; + try { + files = object.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); } - // 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; + return RegexListSearcher.ofStrings(getValue()).hasMatch(files); } @Override