opendev/gerrit
Code Issues Proposed changes

For the 'conflicts' operator return only changes that actually conflict

Browse Source 
So far the 'conflicts:<change>' query operator returns all changes that
touch at least one file that was also modified by the specified change.
This means all changes that potentially conflict with the specifed
change are returned.

Now we check the mergeability for the changes and only return those
changes that actually conflict. This means now the 'Automatically
resolve conflicts' project setting is taken into account and if the
content merge succeeds a change is not returned.

A new persistent cache is added to cache for two commits and a given
submit type whether the commits are conflicting. This ensures that the
expensive mergeability check for two changes is only done once.

Change-Id: I1d6a3c431ed679f786e589bbed3aed52fe439895
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
This commit is contained in:
Edwin Kempin 2013-07-04 09:47:13 +02:00
parent 75ebb3f04c
commit 86e8362313
9 changed files with 387 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/user-search.txt
View File

@ -77,10 +77,9 @@ Change-Id that was scraped out of the commit message.
[[conflicts]]
conflicts:'ID'::
+
Changes that potentially conflict with change 'ID' because they touch
at least one file that was also touched by change 'ID'. Change 'ID' can
be specified as a legacy numerical 'ID' such as 15183, or a newer style
Change-Id that was scraped out of the commit message.
Changes that conflict with change 'ID'. Change 'ID' can be specified
as a legacy numerical 'ID' such as 15183, or a newer style Change-Id
that was scraped out of the commit message.
[[owner]]
owner:'USER'::

4
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@ -111,6 +111,7 @@ import com.google.gerrit.server.project.ProjectNode;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SectionSortCache;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ConflictsCacheImpl;
import com.google.gerrit.server.ssh.SshAddressesModule;
import com.google.gerrit.server.tools.ToolsCatalog;
import com.google.gerrit.server.util.IdGenerator;
@ -143,13 +144,14 @@ public class GerritGlobalModule extends FactoryModule {
install(authModule);
install(AccountByEmailCacheImpl.module());
install(AccountCacheImpl.module());
install(ChangeCache.module());
install(ConflictsCacheImpl.module());
install(GroupCacheImpl.module());
install(GroupIncludeCacheImpl.module());
install(PatchListCacheImpl.module());
install(ProjectCacheImpl.module());
install(SectionSortCache.module());
install(TagCache.module());
install(ChangeCache.module());
install(new AccessControlModule());
install(new CmdLineParserModule());

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

@ -33,8 +33,8 @@ public abstract class BasicChangeRewrites 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, null, null, //
null, null, null, null, null, null, null), null);
static Schema<ChangeData> schema(@Nullable IndexCollection indexes) {
ChangeIndex index = indexes != null ? indexes.getSearchIndex() : null;

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

@ -32,6 +32,7 @@ import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.SubmitStrategyFactory;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.IndexCollection;
import com.google.gerrit.server.index.Schema;
@ -153,6 +154,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final ProjectCache projectCache;
final Provider<ListChildProjects> listChildProjects;
final IndexCollection indexes;
final SubmitStrategyFactory submitStrategyFactory;
final ConflictsCache conflictsCache;
@Inject
@VisibleForTesting
@ -169,7 +172,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
GitRepositoryManager repoManager,
ProjectCache projectCache,
Provider<ListChildProjects> listChildProjects,
IndexCollection indexes) {
IndexCollection indexes,
SubmitStrategyFactory submitStrategyFactory,
ConflictsCache conflictsCache) {
this.dbProvider = dbProvider;
this.rewriter = rewriter;
this.userFactory = userFactory;
@ -184,6 +189,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
this.projectCache = projectCache;
this.listChildProjects = listChildProjects;
this.indexes = indexes;
this.submitStrategyFactory = submitStrategyFactory;
this.conflictsCache = conflictsCache;
}
}
@ -317,8 +324,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public Predicate<ChangeData> conflicts(String value) throws OrmException,
QueryParseException {
requireIndex(FIELD_CONFLICTS, value);
return new ConflictsPredicate(args.dbProvider, args.patchListCache, value,
parseChange(value));
return new ConflictsPredicate(args.dbProvider, args.patchListCache,
args.submitStrategyFactory, args.changeControlGenericFactory,
args.userFactory, args.repoManager, args.projectCache,
args.conflictsCache, value, parseChange(value));
}
@Operator

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

@ -0,0 +1,78 @@
// 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.common.base.Objects;
import com.google.gerrit.reviewdb.client.Project.SubmitType;
import org.eclipse.jgit.lib.ObjectId;
import java.io.Serializable;
public class ConflictKey implements Serializable {
private static final long serialVersionUID = 1L;
private final ObjectId commit;
private final ObjectId otherCommit;
private final SubmitType submitType;
private final boolean contentMerge;
public ConflictKey(ObjectId commit, ObjectId otherCommit,
SubmitType submitType, boolean contentMerge) {
if (SubmitType.FAST_FORWARD_ONLY.equals(submitType)
|| commit.compareTo(otherCommit) < 0) {
this.commit = commit;
this.otherCommit = otherCommit;
} else {
this.commit = otherCommit;
this.otherCommit = commit;
}
this.submitType = submitType;
this.contentMerge = contentMerge;
}
public ObjectId getCommit() {
return commit;
}
public ObjectId getOtherCommit() {
return otherCommit;
}
public SubmitType getSubmitType() {
return submitType;
}
public boolean isContentMerge() {
return contentMerge;
}
@Override
public boolean equals(Object o) {
if (!(o instanceof ConflictKey)) {
return false;
}
ConflictKey other = (ConflictKey)o;
return commit.equals(other.commit)
&& otherCommit.equals(other.otherCommit)
&& submitType.equals(other.submitType)
&& contentMerge == other.contentMerge;
}
@Override
public int hashCode() {
return Objects.hashCode(commit, otherCommit, submitType, contentMerge);
}
}

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

@ -0,0 +1,25 @@
// 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.common.Nullable;
public interface ConflictsCache {
public void put(ConflictKey key, Boolean value);
@Nullable
public Boolean getIfPresent(ConflictKey key);
}

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

@ -0,0 +1,56 @@
// 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.common.cache.Cache;
import com.google.gerrit.server.cache.CacheModule;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Singleton;
import com.google.inject.name.Named;
@Singleton
public class ConflictsCacheImpl implements ConflictsCache {
public final static String NAME = "conflicts";
public static Module module() {
return new CacheModule() {
@Override
protected void configure() {
persist(NAME, ConflictKey.class, Boolean.class)
.maximumWeight(37400);
bind(ConflictsCache.class).to(ConflictsCacheImpl.class);
}
};
}
private final Cache<ConflictKey, Boolean> conflictsCache;
@Inject
public ConflictsCacheImpl(
@Named(NAME) Cache<ConflictKey, Boolean> conflictsCache) {
this.conflictsCache = conflictsCache;
}
@Override
public void put(ConflictKey key, Boolean value) {
conflictsCache.put(key, value);
}
@Override
public Boolean getIfPresent(ConflictKey key) {
return conflictsCache.getIfPresent(key);
}
}

214
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
View File

@ -15,30 +15,71 @@
package com.google.gerrit.server.query.change;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project.SubmitType;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.SubmitStrategy;
import com.google.gerrit.server.git.SubmitStrategyFactory;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.OperatorPredicate;
import com.google.gerrit.server.query.OrPredicate;
import com.google.gerrit.server.query.Predicate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.Constants;
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.RevFlag;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.util.List;
import java.util.Set;
class ConflictsPredicate extends OrPredicate<ChangeData> {
private final String value;
ConflictsPredicate(Provider<ReviewDb> db, PatchListCache plc, String value,
List<Change> changes) throws OrmException {
super(predicates(db, plc, changes));
ConflictsPredicate(Provider<ReviewDb> db, PatchListCache plc,
SubmitStrategyFactory submitStrategyFactory,
ChangeControl.GenericFactory changeControlFactory,
IdentifiedUser.GenericFactory identifiedUserFactory,
GitRepositoryManager repoManager, ProjectCache projectCache,
ConflictsCache conflictsCache, String value, List<Change> changes)
throws OrmException {
super(predicates(db, plc, submitStrategyFactory, changeControlFactory,
identifiedUserFactory, repoManager, projectCache, conflictsCache,
value, changes));
this.value = value;
}
private static List<Predicate<ChangeData>> predicates(Provider<ReviewDb> db,
PatchListCache plc, List<Change> changes) throws OrmException {
private static List<Predicate<ChangeData>> predicates(
final Provider<ReviewDb> db, final PatchListCache plc,
final SubmitStrategyFactory submitStrategyFactory,
final ChangeControl.GenericFactory changeControlFactory,
final IdentifiedUser.GenericFactory identifiedUserFactory,
final GitRepositoryManager repoManager, final ProjectCache projectCache,
final ConflictsCache conflictsCache, final String value,
List<Change> changes) throws OrmException {
List<Predicate<ChangeData>> changePredicates =
Lists.newArrayListWithCapacity(changes.size());
for (Change c : changes) {
for (final Change c : changes) {
final ChangeDataCache changeDataCache = new ChangeDataCache(c, db, projectCache);
List<String> files = new ChangeData(c).currentFilePaths(db, plc);
List<Predicate<ChangeData>> filePredicates =
Lists.newArrayListWithCapacity(files.size());
@ -47,7 +88,7 @@ class ConflictsPredicate extends OrPredicate<ChangeData> {
}
List<Predicate<ChangeData>> predicatesForOneChange =
Lists.newArrayListWithCapacity(4);
Lists.newArrayListWithCapacity(5);
predicatesForOneChange.add(
not(new LegacyChangeIdPredicate(db, c.getId())));
predicatesForOneChange.add(
@ -55,7 +96,115 @@ class ConflictsPredicate extends OrPredicate<ChangeData> {
predicatesForOneChange.add(
new RefPredicate(db, c.getDest().get()));
predicatesForOneChange.add(or(filePredicates));
predicatesForOneChange.add(new OperatorPredicate<ChangeData>(
ChangeQueryBuilder.FIELD_CONFLICTS, value) {
@Override
public boolean match(ChangeData object) throws OrmException {
Change otherChange = object.change(db);
if (otherChange == null) {
return false;
}
if (!otherChange.getDest().equals(c.getDest())) {
return false;
}
SubmitType submitType = getSubmitType(otherChange, object);
if (submitType == null) {
return false;
}
ObjectId other = ObjectId.fromString(
object.currentPatchSet(db).getRevision().get());
ConflictKey conflictsKey =
new ConflictKey(changeDataCache.getTestAgainst(), other, submitType,
changeDataCache.getProjectState().isUseContentMerge());
Boolean conflicts = conflictsCache.getIfPresent(conflictsKey);
if (conflicts != null) {
return conflicts;
}
try {
Repository repo =
repoManager.openRepository(otherChange.getProject());
try {
RevWalk rw = new RevWalk(repo) {
@Override
protected RevCommit createCommit(AnyObjectId id) {
return new CodeReviewCommit(id);
}
};
try {
RevFlag canMergeFlag = rw.newFlag("CAN_MERGE");
CodeReviewCommit commit =
(CodeReviewCommit) rw.parseCommit(changeDataCache.getTestAgainst());
SubmitStrategy strategy =
submitStrategyFactory.create(submitType,
db.get(), repo, rw, null, canMergeFlag,
getAlreadyAccepted(repo, rw, commit),
otherChange.getDest());
CodeReviewCommit otherCommit =
(CodeReviewCommit) rw.parseCommit(other);
otherCommit.add(canMergeFlag);
conflicts = !strategy.dryRun(commit, otherCommit);
conflictsCache.put(conflictsKey, conflicts);
return conflicts;
} catch (MergeException e) {
throw new IllegalStateException(e);
} catch (NoSuchProjectException e) {
throw new IllegalStateException(e);
} finally {
rw.release();
}
} finally {
repo.close();
}
} catch (IOException e) {
throw new IllegalStateException(e);
}
}
@Override
public int getCost() {
return 5;
}
private SubmitType getSubmitType(Change change, ChangeData cd) throws OrmException {
try {
final SubmitTypeRecord r =
changeControlFactory.controlFor(change,
identifiedUserFactory.create(change.getOwner()))
.getSubmitTypeRecord(db.get(), cd.currentPatchSet(db), cd);
if (r.status != SubmitTypeRecord.Status.OK) {
return null;
}
return r.type;
} catch (NoSuchChangeException e) {
return null;
}
}
private Set<RevCommit> getAlreadyAccepted(Repository repo, RevWalk rw,
CodeReviewCommit tip) throws MergeException {
Set<RevCommit> alreadyAccepted = Sets.newHashSet();
if (tip != null) {
alreadyAccepted.add(tip);
}
try {
for (ObjectId id : changeDataCache.getAlreadyAccepted(repo)) {
try {
alreadyAccepted.add(rw.parseCommit(id));
} catch (IncorrectObjectTypeException iote) {
// Not a commit? Skip over it.
}
}
} catch (IOException e) {
throw new MergeException(
"Failed to determine already accepted commits.", e);
}
return alreadyAccepted;
}
});
changePredicates.add(and(predicatesForOneChange));
}
return changePredicates;
@ -65,4 +214,55 @@ class ConflictsPredicate extends OrPredicate<ChangeData> {
public String toString() {
return ChangeQueryBuilder.FIELD_CONFLICTS + ":" + value;
}
private static class ChangeDataCache {
private final Change change;
private final Provider<ReviewDb> db;
private final ProjectCache projectCache;
private ObjectId testAgainst;
private ProjectState projectState;
private Set<ObjectId> alreadyAccepted;
ChangeDataCache(Change change, Provider<ReviewDb> db, ProjectCache projectCache) {
this.change = change;
this.db = db;
this.projectCache = projectCache;
}
ObjectId getTestAgainst()
throws OrmException {
if (testAgainst == null) {
testAgainst = ObjectId.fromString(
new ChangeData(change).currentPatchSet(db).getRevision().get());
}
return testAgainst;
}
ProjectState getProjectState() {
if (projectState == null) {
projectState = projectCache.get(change.getProject());
if (projectState == null) {
throw new IllegalStateException(
new NoSuchProjectException(change.getProject()));
}
}
return projectState;
}
Set<ObjectId> getAlreadyAccepted(Repository repo) {
if (alreadyAccepted == null) {
alreadyAccepted = Sets.newHashSet();
for (Ref r : repo.getAllRefs().values()) {
if (r.getName().startsWith(Constants.R_HEADS)
|| r.getName().startsWith(Constants.R_TAGS)) {
if (r.getObjectId() != null) {
alreadyAccepted.add(r.getObjectId());
}
}
}
}
return alreadyAccepted;
}
}
}

2
gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
View File

@ -26,7 +26,7 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
new FakeQueryBuilder.Definition<ChangeData, FakeQueryBuilder>(
FakeQueryBuilder.class),
new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
null, null, null, null, null, null, null, indexes),
null, null, null, null, null, null, null, indexes, null, null),
null);
}