From 64e0b0762f342193fee71f3b03e847987be73130 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 27 Dec 2013 11:38:01 -0800 Subject: [PATCH] Support exact match on file parts in file: operator Keep the old "file" field around (using that name in the index for backwards compatibility), but call it PATH in the code to reflect the fact that it matches the full path. Add a new "filepart" repeated field that splits the filename on '/' and adds each unique component as a field value. Use "path:" to search only the path, and "file:" to search either the file or the full path. There is only support for single path components or the full path, not arbitrary sequences of subcomponents. The latter is possible but would result in combinatorial blowup of terms. If users really want to search on that, they can use multiple "file:" terms (discarding ordering) or use regex search. Regex search is still over the entire path, but this is implemented at the ChangeQueryBuilder level and may be changed in the future. Change-Id: I4f6e06bc3c07989f96e4e74e64113f521c05b9e3 --- Documentation/user-search.txt | 16 ++++++- .../gerrit/server/index/ChangeField.java | 30 +++++++++++-- .../gerrit/server/index/ChangeSchemas.java | 32 ++++++++++++-- .../gerrit/server/index/RegexPredicate.java | 4 ++ .../query/change/ChangeQueryBuilder.java | 14 ++++++- .../query/change/ConflictsPredicate.java | 3 +- .../query/change/EqualsFilePredicate.java | 30 ++++++------- .../query/change/EqualsPathPredicate.java | 42 +++++++++++++++++++ ...Predicate.java => RegexPathPredicate.java} | 6 +-- .../google/gerrit/server/index/FakeIndex.java | 2 +- .../change/AbstractQueryChangesTest.java | 42 +++++++++++++++++-- ...eTest.java => RegexPathPredicateTest.java} | 16 +++---- 12 files changed, 194 insertions(+), 43 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java rename gerrit-server/src/main/java/com/google/gerrit/server/query/change/{RegexFilePredicate.java => RegexPathPredicate.java} (94%) rename gerrit-server/src/test/java/com/google/gerrit/server/query/change/{RegexFilePredicateTest.java => RegexPathPredicateTest.java} (85%) diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index 81df88383a..cd79812b06 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -182,8 +182,8 @@ comment:'TEXT':: + Changes that match 'TEXT' string in any comment left by a reviewer. -[[file]] -file:'PATH', f:'PATH':: +[[path]] +path:'PATH':: + Matches any change touching file at 'PATH'. By default exact path matching is used, but regular expressions can be enabled by starting @@ -202,6 +202,18 @@ ones using a bracket expression). For example, to match all XML files named like 'name1.xml', 'name2.xml', and 'name3.xml' use `file:"^name[1-3].xml"`. +[[file]] +file:'NAME', f:'NAME':: ++ +Matches any change touching a file containing the path component +'NAME'. For example a `file:src` will match changes that modify +files named `gerrit-server/src/main/java/Foo.java`. Name matching +is exact match, `file:Foo.java` finds any change touching a file +named exactly `Foo.java` and does not match `AbstractFoo.java`. ++ +Regular expression matching can be enabled by starting the string +with `^`. In this mode `file:` is an alias of `path:` (see above). + [[has]] has:draft:: + diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java index 36208c337a..9fa46fbd93 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.index; +import com.google.common.base.Splitter; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Account; @@ -160,10 +161,11 @@ public class ChangeField { } }; - /** List of filenames modified in the current patch set. */ - public static final FieldDef> FILE = + /** List of full file paths modified in the current patch set. */ + public static final FieldDef> PATH = new FieldDef.Repeatable( - ChangeQueryBuilder.FIELD_FILE, FieldType.EXACT, false) { + // Named for backwards compatibility. + "file", FieldType.EXACT, false) { @Override public Iterable get(ChangeData input, FillArgs args) throws OrmException { @@ -171,6 +173,28 @@ public class ChangeField { } }; + public static Set getFileParts(ChangeData cd) throws OrmException { + Splitter s = Splitter.on('/').omitEmptyStrings(); + Set r = Sets.newHashSet(); + for (String path : cd.currentFilePaths()) { + for (String part : s.split(path)) { + r.add(part); + } + } + return r; + } + + /** Components of each file path modified in the current patch set. */ + public static final FieldDef> FILE_PART = + new FieldDef.Repeatable( + "filepart", FieldType.EXACT, false) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + return getFileParts(input); + } + }; + /** Owner/creator of the change. */ public static final FieldDef OWNER = new FieldDef.Single( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java index eb2c527519..475e7d1b54 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java @@ -40,7 +40,7 @@ public class ChangeSchemas { ChangeField.TOPIC, ChangeField.UPDATED, ChangeField.LEGACY_SORTKEY, - ChangeField.FILE, + ChangeField.PATH, ChangeField.OWNER, ChangeField.REVIEWER, ChangeField.COMMIT, @@ -60,7 +60,7 @@ public class ChangeSchemas { ChangeField.TOPIC, ChangeField.UPDATED, ChangeField.LEGACY_SORTKEY, - ChangeField.FILE, + ChangeField.PATH, ChangeField.OWNER, ChangeField.REVIEWER, ChangeField.COMMIT, @@ -81,7 +81,7 @@ public class ChangeSchemas { ChangeField.TOPIC, ChangeField.UPDATED, ChangeField.SORTKEY, - ChangeField.FILE, + ChangeField.PATH, ChangeField.OWNER, ChangeField.REVIEWER, ChangeField.COMMIT, @@ -105,7 +105,7 @@ public class ChangeSchemas { ChangeField.TOPIC, ChangeField.UPDATED, ChangeField.SORTKEY, - ChangeField.FILE, + ChangeField.PATH, ChangeField.OWNER, ChangeField.REVIEWER, ChangeField.COMMIT, @@ -121,6 +121,30 @@ public class ChangeSchemas { // For upgrade to Lucene 4.6.0 index format only. static final Schema V6 = release(V5.getFields().values()); + static final Schema V7 = release( + ChangeField.LEGACY_ID, + ChangeField.ID, + ChangeField.STATUS, + ChangeField.PROJECT, + ChangeField.REF, + ChangeField.TOPIC, + ChangeField.UPDATED, + ChangeField.SORTKEY, + ChangeField.FILE_PART, + ChangeField.PATH, + ChangeField.OWNER, + ChangeField.REVIEWER, + ChangeField.COMMIT, + ChangeField.TR, + ChangeField.LABEL, + ChangeField.REVIEWED, + ChangeField.COMMIT_MESSAGE, + ChangeField.COMMENT, + ChangeField.CHANGE, + ChangeField.APPROVAL, + ChangeField.MERGEABLE); + + private static Schema release(Collection> fields) { return new Schema(true, fields); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/RegexPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/RegexPredicate.java index 198c7b091b..b73674dd95 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/RegexPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/RegexPredicate.java @@ -18,4 +18,8 @@ public abstract class RegexPredicate extends IndexPredicate { protected RegexPredicate(FieldDef def, String value) { super(def, value); } + + protected RegexPredicate(FieldDef def, String name, String value) { + super(def, name, value); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 99dd2e73df..c0b8a638ec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -99,6 +99,7 @@ public class ChangeQueryBuilder extends QueryBuilder { public static final String FIELD_OWNER = "owner"; public static final String FIELD_OWNERIN = "ownerin"; public static final String FIELD_PARENTPROJECT = "parentproject"; + public static final String FIELD_PATH = "path"; public static final String FIELD_PROJECT = "project"; public static final String FIELD_REF = "ref"; public static final String FIELD_REVIEWER = "reviewer"; @@ -384,9 +385,18 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate file(String file) throws QueryParseException { if (file.startsWith("^")) { - return new RegexFilePredicate(file); + return new RegexPathPredicate(FIELD_FILE, file); } else { - return new EqualsFilePredicate(file); + return EqualsFilePredicate.create(args, file); + } + } + + @Operator + public Predicate path(String path) throws QueryParseException { + if (path.startsWith("^")) { + return new RegexPathPredicate(FIELD_PATH, path); + } else { + return new EqualsPathPredicate(FIELD_PATH, path); } } 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 21025416e3..3c8b2c064e 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 @@ -70,7 +70,8 @@ class ConflictsPredicate extends OrPredicate { List> filePredicates = Lists.newArrayListWithCapacity(files.size()); for (String file : files) { - filePredicates.add(new EqualsFilePredicate(file)); + filePredicates.add( + new EqualsPathPredicate(ChangeQueryBuilder.FIELD_PATH, file)); } List> predicatesForOneChange = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsFilePredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsFilePredicate.java index 13bd08bbf8..e5fc51d950 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsFilePredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsFilePredicate.java @@ -16,31 +16,31 @@ package com.google.gerrit.server.query.change; import com.google.gerrit.server.index.ChangeField; import com.google.gerrit.server.index.IndexPredicate; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; import com.google.gwtorm.server.OrmException; -import java.util.Collections; -import java.util.List; - class EqualsFilePredicate extends IndexPredicate { + static Predicate create(Arguments args, String value) { + Predicate eqPath = + new EqualsPathPredicate(ChangeQueryBuilder.FIELD_FILE, value); + if (!args.indexes.getSearchIndex().getSchema().getFields().containsKey( + ChangeField.FILE_PART.getName())) { + return eqPath; + } + return Predicate.or(eqPath, new EqualsFilePredicate(value)); + } + private final String value; - EqualsFilePredicate(String value) { - super(ChangeField.FILE, value); + private EqualsFilePredicate(String value) { + super(ChangeField.FILE_PART, ChangeQueryBuilder.FIELD_FILE, value); this.value = value; } @Override public boolean match(ChangeData object) throws OrmException { - List files = object.currentFilePaths(); - if (files != null) { - return Collections.binarySearch(files, value) >= 0; - } 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; - } + return ChangeField.getFileParts(object).contains(value); } @Override 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 new file mode 100644 index 0000000000..055b6d5dd6 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java @@ -0,0 +1,42 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.query.change; + +import com.google.gerrit.server.index.ChangeField; +import com.google.gerrit.server.index.IndexPredicate; +import com.google.gwtorm.server.OrmException; + +import java.util.Collections; +import java.util.List; + +class EqualsPathPredicate extends IndexPredicate { + private final String value; + + EqualsPathPredicate(String fieldName, String value) { + super(ChangeField.PATH, fieldName, value); + this.value = value; + } + + @Override + public boolean match(ChangeData object) throws OrmException { + List files = object.currentFilePaths(); + return files != null && Collections.binarySearch(files, value) >= 0; + } + + @Override + public int getCost() { + return 1; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexFilePredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java similarity index 94% rename from gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexFilePredicate.java rename to gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java index f39dc1486f..d073002cbc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexFilePredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java @@ -25,7 +25,7 @@ import dk.brics.automaton.RunAutomaton; import java.util.Collections; import java.util.List; -class RegexFilePredicate extends RegexPredicate { +class RegexPathPredicate extends RegexPredicate { private final RunAutomaton pattern; private final String prefixBegin; @@ -33,8 +33,8 @@ class RegexFilePredicate extends RegexPredicate { private final int prefixLen; private final boolean prefixOnly; - RegexFilePredicate(String re) { - super(ChangeField.FILE, re); + RegexPathPredicate(String fieldName, String re) { + super(ChangeField.PATH, re); if (re.startsWith("^")) { re = re.substring(1); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java index d8a0f65a69..339a1bb163 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeIndex.java @@ -30,7 +30,7 @@ class FakeIndex implements ChangeIndex { static Schema V2 = new Schema(2, false, ImmutableList.of( ChangeField.STATUS, - ChangeField.FILE, + ChangeField.PATH, ChangeField.SORTKEY)); private static class Source implements ChangeDataSource { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index ec485bba29..321a4a0f3c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -574,13 +574,16 @@ public abstract class AbstractQueryChangesTest { TestRepository repo = createProject("repo"); RevCommit commit = repo.parseBody( repo.commit().message("one") - .add("file1", "contents1").add("file2", "contents2") + .add("dir/file1", "contents1").add("dir/file2", "contents2") .create()); Change change = newChange(repo, commit, null, null, null).insert(); assertTrue(query("file:file").isEmpty()); + assertResultEquals(change, queryOne("file:dir")); assertResultEquals(change, queryOne("file:file1")); assertResultEquals(change, queryOne("file:file2")); + assertResultEquals(change, queryOne("file:dir/file1")); + assertResultEquals(change, queryOne("file:dir/file2")); } @Test @@ -588,12 +591,43 @@ public abstract class AbstractQueryChangesTest { TestRepository repo = createProject("repo"); RevCommit commit = repo.parseBody( repo.commit().message("one") - .add("file1", "contents1").add("file2", "contents2") + .add("dir/file1", "contents1").add("dir/file2", "contents2") .create()); Change change = newChange(repo, commit, null, null, null).insert(); - assertTrue(query("file:file.*").isEmpty()); - assertResultEquals(change, queryOne("file:^file.*")); + assertTrue(query("file:.*file.*").isEmpty()); + assertTrue(query("file:^file.*").isEmpty()); // Whole path only. + assertResultEquals(change, queryOne("file:^dir.file.*")); + } + + @Test + public void byPathExact() throws Exception { + TestRepository repo = createProject("repo"); + RevCommit commit = repo.parseBody( + repo.commit().message("one") + .add("dir/file1", "contents1").add("dir/file2", "contents2") + .create()); + Change change = newChange(repo, commit, null, null, null).insert(); + + assertTrue(query("path:file").isEmpty()); + assertTrue(query("path:dir").isEmpty()); + assertTrue(query("path:file1").isEmpty()); + assertTrue(query("path:file2").isEmpty()); + assertResultEquals(change, queryOne("path:dir/file1")); + assertResultEquals(change, queryOne("path:dir/file2")); + } + + @Test + public void byPathRegex() throws Exception { + TestRepository repo = createProject("repo"); + RevCommit commit = repo.parseBody( + repo.commit().message("one") + .add("dir/file1", "contents1").add("dir/file2", "contents2") + .create()); + Change change = newChange(repo, commit, null, null, null).insert(); + + assertTrue(query("path:.*file.*").isEmpty()); + assertResultEquals(change, queryOne("path:^dir.file.*")); } @Test diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexFilePredicateTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java similarity index 85% rename from gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexFilePredicateTest.java rename to gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java index 363fa26c76..4e5dcddb78 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexFilePredicateTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java @@ -23,10 +23,10 @@ import java.util.Arrays; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -public class RegexFilePredicateTest { +public class RegexPathPredicateTest { @Test public void testPrefixOnlyOptimization() throws OrmException { - RegexFilePredicate p = predicate("^a/b/.*"); + RegexPathPredicate p = predicate("^a/b/.*"); assertTrue(p.match(change("a/b/source.c"))); assertFalse(p.match(change("source.c"))); @@ -36,7 +36,7 @@ public class RegexFilePredicateTest { @Test public void testPrefixReducesSearchSpace() throws OrmException { - RegexFilePredicate p = predicate("^a/b/.*\\.[ch]"); + RegexPathPredicate p = predicate("^a/b/.*\\.[ch]"); assertTrue(p.match(change("a/b/source.c"))); assertFalse(p.match(change("a/b/source.res"))); assertFalse(p.match(change("source.res"))); @@ -46,7 +46,7 @@ public class RegexFilePredicateTest { @Test public void testFileExtension_Constant() throws OrmException { - RegexFilePredicate p = predicate("^.*\\.res"); + RegexPathPredicate p = predicate("^.*\\.res"); assertTrue(p.match(change("test.res"))); assertTrue(p.match(change("foo/bar/test.res"))); assertFalse(p.match(change("test.res.bar"))); @@ -54,7 +54,7 @@ public class RegexFilePredicateTest { @Test public void testFileExtension_CharacterGroup() throws OrmException { - RegexFilePredicate p = predicate("^.*\\.[ch]"); + RegexPathPredicate p = predicate("^.*\\.[ch]"); assertTrue(p.match(change("test.c"))); assertTrue(p.match(change("test.h"))); assertFalse(p.match(change("test.C"))); @@ -71,14 +71,14 @@ public class RegexFilePredicateTest { @Test public void testExactMatch() throws OrmException { - RegexFilePredicate p = predicate("^foo.c"); + RegexPathPredicate p = predicate("^foo.c"); assertTrue(p.match(change("foo.c"))); assertFalse(p.match(change("foo.cc"))); assertFalse(p.match(change("bar.c"))); } - private static RegexFilePredicate predicate(String pattern) { - return new RegexFilePredicate(pattern); + private static RegexPathPredicate predicate(String pattern) { + return new RegexPathPredicate(ChangeQueryBuilder.FIELD_PATH, pattern); } private static ChangeData change(String... files) {