From c4ced8910f45d9bf7ad4d3044278dc6343c3d156 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 12 Sep 2016 17:16:08 +0200 Subject: [PATCH] Don't compare commit message against auto-merge commit 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 --- .../gerrit/acceptance/AbstractDaemonTest.java | 11 ++- .../acceptance/server/change/CommentsIT.java | 39 +++++++++- .../gerrit/server/PatchLineCommentsUtil.java | 26 ++++++- .../gerrit/server/change/PostReview.java | 8 +++ .../gerrit/server/patch/ComparisonType.java | 71 +++++++++++++++++++ .../google/gerrit/server/patch/PatchFile.java | 2 +- .../google/gerrit/server/patch/PatchList.java | 18 ++--- .../gerrit/server/patch/PatchListKey.java | 2 +- .../gerrit/server/patch/PatchListLoader.java | 23 +++--- .../server/patch/PatchScriptBuilder.java | 9 +-- .../server/patch/PatchScriptFactory.java | 2 +- 11 files changed, 179 insertions(+), 32 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 1afb374cb3..018358126a 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -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(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java index 67af6e21e9..5b6f3f9f3f 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -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> 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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index 603f528520..061b9bd93b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -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 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 removeCommentsOnAncestorOfCommitMessage( + List list) { + return list.stream() + .filter(c -> c.getSide() != 0 + || !Patch.COMMIT_MSG.equals(c.getKey().getParentKey().get())) + .collect(toList()); } public List draftByPatchSetAuthor(ReviewDb db, 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 3828041496..d6a4ef9b39 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 @@ -329,6 +329,14 @@ public class PostReview implements RestModifyView "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 list = ent.getValue(); if (list == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java new file mode 100644 index 0000000000..343d2e2c8c --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java @@ -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); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java index f8c8b49abc..71129f6bfc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java @@ -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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java index 2a4afb337d..fa40bf05da 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java @@ -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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java index 43e3dcec09..e732560718 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java @@ -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_TYPES = ImmutableBiMap.of( Whitespace.IGNORE_NONE, 'N', diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 0ae9a99ffb..fc05630070 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -156,14 +156,17 @@ public class PatchListLoader implements Callable { 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 { int cnt = diffEntries.size(); List 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 { 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, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index e09d26fac9..5b30f8c768 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 754d9950a3..91f8cf803d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -260,7 +260,7 @@ public class PatchScriptFactory implements Callable { 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; }