opendev/gerrit
Code Issues Proposed changes

Index files affected by merge commits

Browse Source 
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
This commit is contained in:
Shawn Pearce
2017-08-02 08:40:51 -07:00
committed by Alice Kober-Sotzek
parent ab7b0a8f0c
commit 7ecae33e24
10 changed files with 118 additions and 167 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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<ReviewResult> 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<ReviewResult> 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 <T extends CommentInput> void checkComments(
RevisionResource revision, Map<String, List<T>> commentsPerPath)
throws OrmException, BadRequestException {
throws BadRequestException, PatchListNotAvailableException {
Set<String> revisionFilePaths = getAffectedFilePaths(revision);
for (Map.Entry<String, List<T>> entry : commentsPerPath.entrySet()) {
String path = entry.getKey();
@@ -569,9 +575,14 @@ public class PostReview
}
}
private Set<String> getAffectedFilePaths(RevisionResource revision) throws OrmException {
ChangeData changeData = changeDataFactory.create(db.get(), revision.getNotes());
return new HashSet<>(changeData.filePaths(revision.getPatchSet()));
private Set<String> 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<String, List<RobotCommentInput>> in)
throws BadRequestException, OrmException {
throws BadRequestException, PatchListNotAvailableException {
cleanUpComments(in);
for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) {
String commentPath = e.getKey();

2
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@@ -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");
}

9
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
View File

@@ -145,10 +145,13 @@ public class ChangeField {
.buildRepeatable(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of()));
public static Set<String> getFileParts(ChangeData cd) throws OrmException {
List<String> paths = cd.currentFilePaths();
if (paths == null) {
return ImmutableSet.of();
List<String> paths;
try {
paths = cd.currentFilePaths();
} catch (IOException e) {
throw new OrmException(e);
}
Splitter s = Splitter.on('/').omitEmptyStrings();
Set<String> r = new HashSet<>();
for (String path : paths) {

4
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
View File

@@ -86,7 +86,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
ChangeField.PENDING_REVIEWER,
ChangeField.PENDING_REVIEWER_BY_EMAIL);
static final Schema<ChangeData> V45 = schema(V44, ChangeField.REVERT_OF);
@Deprecated static final Schema<ChangeData> V45 = schema(V44, ChangeField.REVERT_OF);
static final Schema<ChangeData> V46 = schema(V45);
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();

3
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java
View File

@@ -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;
}

10
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@@ -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 {

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

@@ -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<PatchSet> patchSets;
private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals;
private List<PatchSetApproval> currentApprovals;
private Map<Integer, List<String>> files;
private Map<Integer, Optional<DiffSummary>> diffSummaries;
private List<String> currentFiles;
private Optional<DiffSummary> diffSummary;
private Collection<Comment> publishedComments;
private Collection<RobotComment> robotComments;
private CurrentUser visibleTo;
@@ -391,6 +394,7 @@ public class ChangeData {
private List<ReviewerStatusUpdate> 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<Integer, List<String>> initFiles() {
if (files == null) {
files = new HashMap<>();
}
return files;
}
public void setCurrentFilePaths(List<String> filePaths) throws OrmException {
PatchSet ps = currentPatchSet();
if (ps != null) {
initFiles().put(ps.getPatchSetId(), ImmutableList.copyOf(filePaths));
currentFiles = ImmutableList.copyOf(filePaths);
}
}
public List<String> currentFilePaths() throws OrmException {
PatchSet ps = currentPatchSet();
return ps != null ? filePaths(ps) : null;
}
public List<String> filePaths(PatchSet ps) throws OrmException {
Integer psId = ps.getPatchSetId();
List<String> r = initFiles().get(psId);
if (r == null) {
Change c = change();
if (c == null) {
return null;
public List<String> currentFilePaths() throws IOException, OrmException {
if (currentFiles == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
Optional<DiffSummary> p = getDiffSummary(c, ps);
if (!p.isPresent()) {
List<String> emptyFileList = Collections.emptyList();
if (lazyLoad) {
files.put(ps.getPatchSetId(), emptyFileList);
}
return emptyFileList;
}
r = p.get().getPaths();
files.put(psId, r);
Optional<DiffSummary> p = getDiffSummary();
currentFiles = p.map(DiffSummary::getPaths).orElse(Collections.emptyList());
}
return r;
return currentFiles;
}
private Optional<DiffSummary> getDiffSummary(Change c, PatchSet ps) {
Integer psId = ps.getId().get();
if (diffSummaries == null) {
diffSummaries = new HashMap<>();
}
Optional<DiffSummary> r = diffSummaries.get(psId);
if (r == null) {
private Optional<DiffSummary> 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<ChangedLines> computeChangedLines() throws OrmException {
Change c = change();
if (c == null) {
return Optional.empty();
}
PatchSet ps = currentPatchSet();
if (ps == null) {
return Optional.empty();
}
Optional<DiffSummary> ds = getDiffSummary(c, ps);
private Optional<ChangedLines> computeChangedLines() throws OrmException, IOException {
Optional<DiffSummary> ds = getDiffSummary();
if (ds.isPresent()) {
return Optional.of(ds.get().getChangedLines());
}
return Optional.empty();
}
public Optional<ChangedLines> changedLines() throws OrmException {
public Optional<ChangedLines> 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;
}

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

@@ -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<ChangeData> create(Arguments args, String value, Change c)
throws QueryParseException, OrmException {
ChangeDataCache changeDataCache =
new ChangeDataCache(c, args.db, args.changeDataFactory, args.projectCache);
List<String> files = listFiles(c, args, changeDataCache);
ChangeData cd;
List<String> 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<String> 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<String> 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<ReviewDb> 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<ObjectId> alreadyAccepted;
private ObjectId testAgainst;
private ProjectState projectState;
private Set<ObjectId> alreadyAccepted;
public ChangeDataCache(
Change change,
Provider<ReviewDb> 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;

10
gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java
View File

@@ -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<String> files = object.currentFilePaths();
return files != null && Collections.binarySearch(files, value) >= 0;
List<String> files;
try {
files = object.currentFilePaths();
} catch (IOException e) {
throw new OrmException(e);
}
return Collections.binarySearch(files, value) >= 0;
}
@Override

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

@@ -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<String> files = object.currentFilePaths();
if (files != null) {
return RegexListSearcher.ofStrings(getValue()).hasMatch(files);
List<String> 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