opendev/gerrit
Code Issues Proposed changes

Don't compare commit message against auto-merge commit

Browse Source 
When a merge commit is compared against its auto-merge commit, we want
to show the complete commit message as new in the diff screen (the
same as when the commit is compared against a parent commit).

Define a new class ComparisonType that encapsulates the information
about whether the comparison is done against a parent or against the
auto-merge.

If the comparison is done against a parent, include the parent number.
The parent number will be needed if we want to have magic files with
content that depends on the parent against which the comparison is
done. Include the parent number already now, so that we don't need to
invalidate the PatchListCache later once more.

Commenting on the auto-merge commit message is no longer possible.
Old comments that were done on the auto-merge commit message are
filtered out when comments are retrieved (because we don't return any
content to which they could be applied).

Change-Id: I8a75c32a2053f82f8a2f41e0d15747c7f50354c3
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-09-12 17:16:08 +02:00
parent 6647c6c7cb
commit c4ced8910f
11 changed files with 179 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -547,21 +547,26 @@ public abstract class AbstractDaemonTest {
protected PushOneCommit.Result createMergeCommitChange(String ref)
throws Exception {
return createMergeCommitChange(ref, "foo");
}
protected PushOneCommit.Result createMergeCommitChange(String ref, String file)
throws Exception {
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
PushOneCommit.Result p1 = pushFactory.create(db, admin.getIdent(),
testRepo, "parent 1", ImmutableMap.of("foo", "foo-1", "bar", "bar-1"))
testRepo, "parent 1", ImmutableMap.of(file, "foo-1", "bar", "bar-1"))
.to(ref);
// reset HEAD in order to create a sibling of the first change
testRepo.reset(initial);
PushOneCommit.Result p2 = pushFactory.create(db, admin.getIdent(),
testRepo, "parent 2", ImmutableMap.of("foo", "foo-2", "bar", "bar-2"))
testRepo, "parent 2", ImmutableMap.of(file, "foo-2", "bar", "bar-2"))
.to(ref);
PushOneCommit m = pushFactory.create(db, admin.getIdent(), testRepo, "merge",
ImmutableMap.of("foo", "foo-1", "bar", "bar-2"));
ImmutableMap.of(file, "foo-1", "bar", "bar-2"));
m.setParents(ImmutableList.of(p1.getCommit(), p2.getCommit()));
PushOneCommit.Result result = m.to(ref);
result.assertOkStatus();

39
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
View File

@@ -32,9 +32,11 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.PostReview;
@@ -148,8 +150,8 @@ public class CommentsIT extends AbstractDaemonTest {
@Test
public void postCommentOnMergeCommitChange() throws Exception {
for (Integer line : lines) {
final String file = "/COMMIT_MSG";
PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
String file = "foo";
PushOneCommit.Result r = createMergeCommitChange("refs/for/master", file);
String changeId = r.getChangeId();
String revId = r.getCommit().getName();
ReviewInput input = new ReviewInput();
@@ -165,6 +167,39 @@ public class CommentsIT extends AbstractDaemonTest {
assertThat(Lists.transform(result.get(file), infoToInput(file)))
.containsExactly(c1, c2, c3, c4);
}
// for the commit message comments on the auto-merge are not possible
for (Integer line : lines) {
String file = Patch.COMMIT_MSG;
PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
String changeId = r.getChangeId();
String revId = r.getCommit().getName();
ReviewInput input = new ReviewInput();
CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1");
CommentInput c2 = newCommentOnParent(file, 1, line, "parent-1 of ps-1");
CommentInput c3 = newCommentOnParent(file, 2, line, "parent-2 of ps-1");
input.comments = new HashMap<>();
input.comments.put(file, ImmutableList.of(c1, c2, c3));
revision(r).review(input);
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result).isNotEmpty();
assertThat(Lists.transform(result.get(file), infoToInput(file)))
.containsExactly(c1, c2, c3);
}
}
@Test
public void postCommentOnCommitMessageOnAutoMerge() throws Exception {
PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
ReviewInput input = new ReviewInput();
CommentInput c =
newComment(Patch.COMMIT_MSG, Side.PARENT, 0, "comment on auto-merge");
input.comments = new HashMap<>();
input.comments.put(Patch.COMMIT_MSG, ImmutableList.of(c));
exception.expect(BadRequestException.class);
exception.expectMessage(
"cannot comment on " + Patch.COMMIT_MSG + " on auto-merge");
revision(r).review(input);
}
@Test

26
gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server;
import static java.util.stream.Collectors.toList;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
@@ -28,6 +30,7 @@ import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -216,10 +219,27 @@ public class PatchLineCommentsUtil {
public List<PatchLineComment> publishedByPatchSet(ReviewDb db,
ChangeNotes notes, PatchSet.Id psId) throws OrmException {
if (!migration.readChanges()) {
return sort(
db.patchComments().publishedByPatchSet(psId).toList());
return removeCommentsOnAncestorOfCommitMessage(sort(
db.patchComments().publishedByPatchSet(psId).toList()));
}
return commentsOnPatchSet(notes.load().getComments().values(), psId);
return removeCommentsOnAncestorOfCommitMessage(
commentsOnPatchSet(notes.load().getComments().values(), psId));
}
/**
* For the commit message the A side in a diff view is always empty when a
* comparison against an ancestor is done, so there can't be any comments on
* this ancestor. However earlier we showed the auto-merge commit message on
* side A when for a merge commit a comparison against the auto-merge was
* done. From that time there may still be comments on the auto-merge commit
* message and those we want to filter out.
*/
private List<PatchLineComment> removeCommentsOnAncestorOfCommitMessage(
List<PatchLineComment> list) {
return list.stream()
.filter(c -> c.getSide() != 0
|| !Patch.COMMIT_MSG.equals(c.getKey().getParentKey().get()))
.collect(toList());
}
public List<PatchLineComment> draftByPatchSetAuthor(ReviewDb db,

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

@@ -329,6 +329,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
"file %s not found in revision %s",
path, revision.getChange().currentPatchSetId()));
}
if (Patch.COMMIT_MSG.equals(path)) {
for (CommentInput comment : ent.getValue()) {
if (comment.side == Side.PARENT && comment.parent == null) {
throw new BadRequestException(
String.format("cannot comment on %s on auto-merge", path));
}
}
}
List<CommentInput> list = ent.getValue();
if (list == null) {

71
gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java Normal file
View File

@@ -0,0 +1,71 @@
// Copyright (C) 2016 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.patch;
import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
public class ComparisonType {
/** 1-based parent */
private final Integer parentNum;
private final boolean autoMerge;
static ComparisonType againstOtherPatchSet() {
return new ComparisonType(null, false);
}
static ComparisonType againstParent(int parentNum) {
return new ComparisonType(parentNum, false);
}
static ComparisonType againstAutoMerge() {
return new ComparisonType(null, true);
}
private ComparisonType(Integer parentNum, boolean autoMerge) {
this.parentNum = parentNum;
this.autoMerge = autoMerge;
}
public boolean isAgainstParentOrAutoMerge() {
return isAgainstParent() || isAgainstAutoMerge();
}
public boolean isAgainstParent() {
return parentNum != null;
}
public boolean isAgainstAutoMerge() {
return autoMerge;
}
void writeTo(OutputStream out) throws IOException {
writeVarInt32(out, parentNum != null ? parentNum : 0);
writeVarInt32(out, autoMerge ? 1 : 0);
}
static ComparisonType readFrom(InputStream in) throws IOException {
int p = readVarInt32(in);
Integer parentNum = p > 0 ? p : null;
boolean autoMerge = readVarInt32(in) != 0;
return new ComparisonType(parentNum, autoMerge);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java
View File

@@ -55,7 +55,7 @@ public class PatchFile {
final RevCommit bCommit = rw.parseCommit(patchList.getNewId());
if (Patch.COMMIT_MSG.equals(fileName)) {
if (patchList.isAgainstParent()) {
if (patchList.getComparisonType().isAgainstParentOrAutoMerge()) {
a = Text.EMPTY;
} else {
// For the initial commit, we have an empty tree on Side A

18
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java
View File

@@ -58,16 +58,16 @@ public class PatchList implements Serializable {
@Nullable
private transient ObjectId oldId;
private transient ObjectId newId;
private transient boolean againstParent;
private transient ComparisonType comparisonType;
private transient int insertions;
private transient int deletions;
private transient PatchListEntry[] patches;
public PatchList(@Nullable final AnyObjectId oldId, final AnyObjectId newId,
final boolean againstParent, final PatchListEntry[] patches) {
public PatchList(@Nullable AnyObjectId oldId, AnyObjectId newId,
ComparisonType comparisonType, PatchListEntry[] patches) {
this.oldId = oldId != null ? oldId.copy() : null;
this.newId = newId.copy();
this.againstParent = againstParent;
this.comparisonType = comparisonType;
// We assume index 0 contains the magic commit message entry.
if (patches.length > 1) {
@@ -97,9 +97,9 @@ public class PatchList implements Serializable {
return Collections.unmodifiableList(Arrays.asList(patches));
}
/** @return true if {@link #getOldId} is {@link #getNewId}'s ancestor. */
public boolean isAgainstParent() {
return againstParent;
/** @return the comparison type */
public ComparisonType getComparisonType() {
return comparisonType;
}
/** @return total number of new lines added. */
@@ -166,7 +166,7 @@ public class PatchList implements Serializable {
try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) {
writeCanBeNull(out, oldId);
writeNotNull(out, newId);
writeVarInt32(out, againstParent ? 1 : 0);
comparisonType.writeTo(out);
writeVarInt32(out, insertions);
writeVarInt32(out, deletions);
writeVarInt32(out, patches.length);
@@ -182,7 +182,7 @@ public class PatchList implements Serializable {
try (InflaterInputStream in = new InflaterInputStream(buf)) {
oldId = readCanBeNull(in);
newId = readNotNull(in);
againstParent = readVarInt32(in) != 0;
comparisonType = ComparisonType.readFrom(in);
insertions = readVarInt32(in);
deletions = readVarInt32(in);
final int cnt = readVarInt32(in);

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java
View File

@@ -35,7 +35,7 @@ import java.io.Serializable;
import java.util.Objects;
public class PatchListKey implements Serializable {
public static final long serialVersionUID = 22L;
public static final long serialVersionUID = 23L;
public static final BiMap<Whitespace, Character> WHITESPACE_TYPES = ImmutableBiMap.of(
Whitespace.IGNORE_NONE, 'N',

23
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@@ -156,14 +156,17 @@ public class PatchListLoader implements Callable<PatchList> {
if (a == null) {
// TODO(sop) Remove this case.
// This is a merge commit, compared to its ancestor.
// This is an octopus merge commit which should be compared against the
// auto-merge. However since we don't support computing the auto-merge
// for octopus merge commits, we fall back to diffing against the first
// parent, even though this wasn't what was requested.
//
PatchListEntry[] entries = new PatchListEntry[1];
entries[0] = newCommitMessage(cmp, reader, null, b);
return new PatchList(a, b, true, entries);
return new PatchList(a, b, ComparisonType.againstParent(1), entries);
}
boolean againstParent = isAgainstParent(a, b);
ComparisonType comparisonType = getComparisonType(a, b);
RevCommit aCommit = a instanceof RevCommit ? (RevCommit) a : null;
RevTree aTree = rw.parseTree(a);
@@ -190,7 +193,7 @@ public class PatchListLoader implements Callable<PatchList> {
int cnt = diffEntries.size();
List<PatchListEntry> entries = new ArrayList<>();
entries.add(newCommitMessage(cmp, reader,
againstParent ? null : aCommit, b));
comparisonType.isAgainstParentOrAutoMerge() ? null : aCommit, b));
for (int i = 0; i < cnt; i++) {
DiffEntry e = diffEntries.get(i);
if (paths == null || paths.contains(e.getNewPath())
@@ -204,19 +207,23 @@ public class PatchListLoader implements Callable<PatchList> {
entries.add(newEntry(aTree, fh, newSize, newSize - oldSize));
}
}
return new PatchList(a, b, againstParent,
return new PatchList(a, b, comparisonType,
entries.toArray(new PatchListEntry[entries.size()]));
}
}
private boolean isAgainstParent(RevObject a, RevCommit b) {
private ComparisonType getComparisonType(RevObject a, RevCommit b) {
for (int i = 0; i < b.getParentCount(); i++) {
if (b.getParent(i).equals(a)) {
return true;
return ComparisonType.againstParent(i + 1);
}
}
return false;
if (key.getOldId() == null && b.getParentCount() > 0) {
return ComparisonType.againstAutoMerge();
}
return ComparisonType.againstOtherPatchSet();
}
private static long getFileSize(ObjectReader reader,

9
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
View File

@@ -66,7 +66,7 @@ class PatchScriptBuilder {
private ObjectReader reader;
private Change change;
private DiffPreferencesInfo diffPrefs;
private boolean againstParent;
private ComparisonType comparisonType;
private ObjectId aId;
private ObjectId bId;
@@ -106,8 +106,8 @@ class PatchScriptBuilder {
}
}
void setTrees(final boolean ap, final ObjectId a, final ObjectId b) {
againstParent = ap;
void setTrees(final ComparisonType ct, final ObjectId a, final ObjectId b) {
comparisonType = ct;
aId = a;
bId = b;
}
@@ -435,7 +435,8 @@ class PatchScriptBuilder {
try {
final boolean reuse;
if (Patch.COMMIT_MSG.equals(path)) {
if (againstParent && (aId == within || within.equals(aId))) {
if (comparisonType.isAgainstParentOrAutoMerge()
&& (aId == within || within.equals(aId))) {
id = ObjectId.zeroId();
src = Text.EMPTY;
srcContent = Text.NO_BYTES;

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java
View File

@@ -260,7 +260,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
b.setRepository(git, project);
b.setChange(change);
b.setDiffPrefs(diffPrefs);
b.setTrees(list.isAgainstParent(), list.getOldId(), list.getNewId());
b.setTrees(list.getComparisonType(), list.getOldId(), list.getNewId());
return b;
}