From c4ced8910f45d9bf7ad4d3044278dc6343c3d156 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 12 Sep 2016 17:16:08 +0200 Subject: [PATCH 1/4] 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; } From b5b60f3aedf7149d0410a1aaa8d20d54879f168c Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 12 Sep 2016 17:55:50 +0200 Subject: [PATCH 2/4] Patch: Add static method to check if file is magic Many code places differentiate between regular files and magic files (files that are generated and included by Gerrit). So far we only have one magic file for the commit message and there is no big benefit of having the static method to check for magic files. However we may add other magic files and this is a preparation for this. Change-Id: I1f7d77fc4e526916140e69deacdbde24cbe09c7c Signed-off-by: Edwin Kempin --- .../com/google/gerrit/client/change/FileTable.java | 10 +++++----- .../com/google/gerrit/client/change/ReplyBox.java | 2 +- .../gerrit/client/diff/PatchSetSelectBox.java | 4 ++-- .../com/google/gerrit/reviewdb/client/Patch.java | 13 +++++++++++++ .../com/google/gerrit/server/change/PostReview.java | 4 ++-- .../google/gerrit/server/events/EventFactory.java | 2 +- .../com/google/gerrit/server/mail/ChangeEmail.java | 2 +- .../google/gerrit/server/mail/CommentSender.java | 2 +- .../server/project/FilesInCommitCollection.java | 2 +- .../gerrit/server/query/change/ChangeData.java | 2 +- 10 files changed, 28 insertions(+), 15 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java index c0879e76c2..940cb8f0ce 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java @@ -538,7 +538,7 @@ public class FileTable extends FlowPanel { bytesDeleted = 0; for (int i = 0; i < list.length(); i++) { FileInfo info = list.get(i); - if (!Patch.COMMIT_MSG.equals(info.path())) { + if (!Patch.isMagic(info.path())) { if (!info.binary()) { hasNonBinaryFile = true; inserted += info.linesInserted(); @@ -628,7 +628,7 @@ public class FileTable extends FlowPanel { private void columnDeleteRestore(SafeHtmlBuilder sb, FileInfo info) { sb.openTd().setStyleName(R.css().restoreDelete()); if (hasUser) { - if (!Patch.COMMIT_MSG.equals(info.path())) { + if (!Patch.isMagic(info.path())) { boolean editable = isEditable(info); sb.openDiv() .openElement("button") @@ -659,7 +659,7 @@ public class FileTable extends FlowPanel { private void columnStatus(SafeHtmlBuilder sb, FileInfo info) { sb.openTd().setStyleName(R.css().status()); - if (!Patch.COMMIT_MSG.equals(info.path()) + if (!Patch.isMagic(info.path()) && info.status() != null && !ChangeType.MODIFIED.matches(info.status())) { sb.append(info.status()); @@ -784,7 +784,7 @@ public class FileTable extends FlowPanel { private void columnDelta1(SafeHtmlBuilder sb, FileInfo info) { sb.openTd().setStyleName(R.css().deltaColumn1()); - if (!Patch.COMMIT_MSG.equals(info.path()) && !info.binary()) { + if (!Patch.isMagic(info.path()) && !info.binary()) { if (showChangeSizeBars) { sb.append(info.linesInserted() + info.linesDeleted()); } else if (!ChangeType.DELETED.matches(info.status())) { @@ -813,7 +813,7 @@ public class FileTable extends FlowPanel { private void columnDelta2(SafeHtmlBuilder sb, FileInfo info) { sb.openTd().setStyleName(R.css().deltaColumn2()); if (showChangeSizeBars - && !Patch.COMMIT_MSG.equals(info.path()) && !info.binary() + && !Patch.isMagic(info.path()) && !info.binary() && (info.linesInserted() != 0 || info.linesDeleted() != 0)) { int w = 80; int t = inserted + deleted; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java index e29048ac06..fc583f2d8b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java @@ -427,7 +427,7 @@ public class ReplyBox extends Composite { Collections.sort(paths); for (String path : paths) { - if (!path.equals(Patch.COMMIT_MSG)) { + if (!Patch.isMagic(path)) { comments.add(new FileComments(clp, psId, path, copyPath(path, m.get(path)))); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java index bc37abbb61..d45b8d8ab8 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java @@ -128,7 +128,7 @@ class PatchSetSelectBox extends Composite { if (meta == null) { return; } - if (!Patch.COMMIT_MSG.equals(path)) { + if (!Patch.isMagic(path)) { linkPanel.add(createDownloadLink()); } if (!binary && open && idActive != null && Gerrit.isSignedIn()) { @@ -147,7 +147,7 @@ class PatchSetSelectBox extends Composite { void setUpBlame(final CodeMirror cm, final boolean isBase, final PatchSet.Id rev, final String path) { - if (!Patch.COMMIT_MSG.equals(path) && Gerrit.isSignedIn() + if (!Patch.isMagic(path) && Gerrit.isSignedIn() && Gerrit.info().change().allowBlame()) { Anchor blameIcon = createBlameIcon(); blameIcon.addClickHandler(new ClickHandler() { diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java index 6a559651ed..82c22cda94 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java @@ -22,6 +22,19 @@ public final class Patch { /** Magical file name which represents the commit message. */ public static final String COMMIT_MSG = "/COMMIT_MSG"; + /** + * Checks if the given path represents a magic file. A magic file is a + * generated file that is automatically included into changes. It does not + * exist in the commit of the patch set. + * + * @param path the file path + * @return {@code true} if the path represents a magic file, otherwise + * {@code false}. + */ + public static boolean isMagic(String path) { + return COMMIT_MSG.equals(path); + } + public static class Key extends StringKey { private static final long serialVersionUID = 1L; 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 d6a4ef9b39..830f9823f0 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 @@ -324,12 +324,12 @@ public class PostReview implements RestModifyView while (mapItr.hasNext()) { Map.Entry> ent = mapItr.next(); String path = ent.getKey(); - if (!filePaths.contains(path) && !Patch.COMMIT_MSG.equals(path)) { + if (!filePaths.contains(path) && !Patch.isMagic(path)) { throw new BadRequestException(String.format( "file %s not found in revision %s", path, revision.getChange().currentPatchSetId())); } - if (Patch.COMMIT_MSG.equals(path)) { + if (Patch.isMagic(path)) { for (CommentInput comment : ent.getValue()) { if (comment.side == Side.PARENT && comment.parent == null) { throw new BadRequestException( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index 56dacccf24..a926f3dda0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java @@ -500,7 +500,7 @@ public class EventFactory { List list = patchListCache.get(change, patchSet).toPatchList(pId); for (Patch pe : list) { - if (!Patch.COMMIT_MSG.equals(pe.getFileName())) { + if (!Patch.isMagic(pe.getFileName())) { p.sizeDeletions -= pe.getDeletions(); p.sizeInsertions += pe.getInsertions(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index cbd2ec9a00..df0f018f02 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -259,7 +259,7 @@ public abstract class ChangeEmail extends NotificationEmail { detail.append("---\n"); PatchList patchList = getPatchList(); for (PatchListEntry p : patchList.getPatches()) { - if (Patch.COMMIT_MSG.equals(p.getNewName())) { + if (Patch.isMagic(p.getNewName())) { continue; } detail.append(p.getChangeType().getCode()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 2424a6d08a..7cd26e12ff 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -75,7 +75,7 @@ public class CommentSender extends ReplyToChangeSender { Set paths = new HashSet<>(); for (PatchLineComment c : plc) { Patch.Key p = c.getKey().getParentKey(); - if (!Patch.COMMIT_MSG.equals(p.getFileName())) { + if (!Patch.isMagic(p.getFileName())) { paths.add(p.getFileName()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/FilesInCommitCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/FilesInCommitCollection.java index 64a5fb22cc..0f44a48b03 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/FilesInCommitCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/FilesInCommitCollection.java @@ -47,7 +47,7 @@ public class FilesInCommitCollection implements @Override public FileResource parse(CommitResource parent, IdString id) throws ResourceNotFoundException, IOException { - if (Patch.COMMIT_MSG.equals(id.get())) { + if (Patch.isMagic(id.get())) { return new FileResource(parent.getProject(), parent.getCommit(), id.get()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 035f97482d..aeb09c50a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -599,7 +599,7 @@ public class ChangeData { r = new ArrayList<>(p.get().getPatches().size()); for (PatchListEntry e : p.get().getPatches()) { - if (Patch.COMMIT_MSG.equals(e.getNewName())) { + if (Patch.isMagic(e.getNewName())) { continue; } switch (e.getChangeType()) { From c6ea7bb81c1d9e80317fa760a4846372eea3929a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 13 Sep 2016 12:33:08 +0200 Subject: [PATCH 3/4] Include a magic /MERGE_LIST file for merge commits The /MERGE_LIST file is generated by Gerrit and is automatically included in all changes for merge commits. It contains a list with the commits that are integrated by accepting the merge commit. When comparing against the auto-merge or a previous patch set it is assumed that the first parent is uninteresting, so that the file lists all commits which are reachable by the other parents, but not by the first parent. If a comparison against a selected parent is done, that parent is marked as uninteresting. This means the content of the file depends on the selection in 'Diff Against' drop-down box. By having the /MERGE_LIST file reviewers can immediately see which commits get integrated by this merge commit. This is important since for merge commits reviewers are supposed to review and approve these commits. Having a file for this allow reviewers to comment on the list and also see diffs between patch sets. In edit mode the /MERGE_LIST file is not editable. Change-Id: Iafcfe3f274ed334e9a40c13de5040a7509389e27 Signed-off-by: Edwin Kempin --- .../gerrit/acceptance/AbstractDaemonTest.java | 46 ++++ .../acceptance/api/change/GetMergeListIT.java | 80 ------- .../acceptance/api/change/MergeListIT.java | 209 ++++++++++++++++++ .../acceptance/api/revision/RevisionIT.java | 59 ++--- .../google/gerrit/client/info/FileInfo.java | 18 +- .../com/google/gerrit/client/Dispatcher.java | 6 +- .../gerrit/client/change/FileTable.java | 59 +++-- .../google/gerrit/client/change/Message.java | 4 + .../google/gerrit/client/change/ReplyBox.java | 5 + .../client/changes/ChangeConstants.java | 1 + .../client/changes/ChangeConstants.properties | 1 + .../com/google/gerrit/client/diff/Header.java | 2 + .../google/gerrit/reviewdb/client/Patch.java | 5 +- .../gerrit/server/change/ChangeJson.java | 1 + .../gerrit/server/change/FileContentUtil.java | 4 + .../gerrit/server/change/GetContent.java | 26 +++ .../gerrit/server/change/GetMergeList.java | 33 ++- .../gerrit/server/mail/CommentSender.java | 5 +- .../gerrit/server/patch/ComparisonType.java | 12 +- .../gerrit/server/patch/MergeListBuilder.java | 52 +++++ .../google/gerrit/server/patch/PatchFile.java | 14 +- .../google/gerrit/server/patch/PatchList.java | 12 +- .../gerrit/server/patch/PatchListKey.java | 6 +- .../gerrit/server/patch/PatchListLoader.java | 81 ++++--- .../server/patch/PatchScriptBuilder.java | 24 +- .../com/google/gerrit/server/patch/Text.java | 30 +++ .../main/java/gerrit/PRED_commit_stats_3.java | 20 +- 27 files changed, 610 insertions(+), 205 deletions(-) delete mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/GetMergeListIT.java create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/MergeListIT.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/MergeListBuilder.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 018358126a..e125a31b4d 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 @@ -17,6 +17,8 @@ package com.google.gerrit.acceptance; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.GitUtil.initSsh; import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES; +import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG; +import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static org.eclipse.jgit.lib.Constants.HEAD; @@ -49,6 +51,8 @@ import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeType; +import com.google.gerrit.extensions.common.DiffInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.IdString; @@ -68,6 +72,7 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.change.Abandon; import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.change.FileContentUtil; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Revisions; import com.google.gerrit.server.config.AllProjectsName; @@ -1098,4 +1103,45 @@ public abstract class AbstractDaemonTest { } assertThat(refValues.keySet()).containsAnyIn(trees.keySet()); } + + protected void assertDiffForNewFile(DiffInfo diff, RevCommit commit, + String path, String expectedContentSideB) throws Exception { + List expectedLines = new ArrayList<>(); + for (String line : expectedContentSideB.split("\n")) { + expectedLines.add(line); + } + + assertThat(diff.binary).isNull(); + assertThat(diff.changeType).isEqualTo(ChangeType.ADDED); + assertThat(diff.diffHeader).isNotNull(); + assertThat(diff.intralineStatus).isNull(); + assertThat(diff.webLinks).isNull(); + + assertThat(diff.metaA).isNull(); + assertThat(diff.metaB).isNotNull(); + assertThat(diff.metaB.commitId).isEqualTo(commit.name()); + + String expectedContentType = "text/plain"; + if (COMMIT_MSG.equals(path)) { + expectedContentType = FileContentUtil.TEXT_X_GERRIT_COMMIT_MESSAGE; + } else if (MERGE_LIST.equals(path)) { + expectedContentType = FileContentUtil.TEXT_X_GERRIT_MERGE_LIST; + } + assertThat(diff.metaB.contentType).isEqualTo(expectedContentType); + + assertThat(diff.metaB.lines).isEqualTo(expectedLines.size()); + assertThat(diff.metaB.name).isEqualTo(path); + assertThat(diff.metaB.webLinks).isNull(); + + assertThat(diff.content).hasSize(1); + DiffInfo.ContentEntry contentEntry = diff.content.get(0); + assertThat(contentEntry.b).containsExactlyElementsIn(expectedLines) + .inOrder(); + assertThat(contentEntry.a).isNull(); + assertThat(contentEntry.ab).isNull(); + assertThat(contentEntry.common).isNull(); + assertThat(contentEntry.editA).isNull(); + assertThat(contentEntry.editB).isNull(); + assertThat(contentEntry.skip).isNull(); + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/GetMergeListIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/GetMergeListIT.java deleted file mode 100644 index 0cb7d89c9d..0000000000 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/GetMergeListIT.java +++ /dev/null @@ -1,80 +0,0 @@ -// 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.acceptance.api.change; - -import static com.google.common.truth.Truth.assertThat; -import static org.eclipse.jgit.lib.Constants.HEAD; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.gerrit.acceptance.AbstractDaemonTest; -import com.google.gerrit.acceptance.NoHttpd; -import com.google.gerrit.acceptance.PushOneCommit; -import com.google.gerrit.extensions.common.CommitInfo; - -import org.eclipse.jgit.lib.ObjectId; -import org.junit.Test; - -import java.util.List; - -@NoHttpd -public class GetMergeListIT extends AbstractDaemonTest { - - @Test - public void getMergeList() throws Exception { - ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId(); - - PushOneCommit.Result gp1 = pushFactory - .create(db, admin.getIdent(), testRepo, "grand parent 1", - ImmutableMap.of("foo", "foo-1.1", "bar", "bar-1.1")) - .to("refs/for/master"); - - PushOneCommit.Result p1 = pushFactory - .create(db, admin.getIdent(), testRepo, "parent 1", - ImmutableMap.of("foo", "foo-1.2", "bar", "bar-1.2")) - .to("refs/for/master"); - - // reset HEAD in order to create a sibling of the first change - testRepo.reset(initial); - - PushOneCommit.Result gp2 = pushFactory - .create(db, admin.getIdent(), testRepo, "grand parent 1", - ImmutableMap.of("foo", "foo-2.1", "bar", "bar-2.1")) - .to("refs/for/master"); - - PushOneCommit.Result p2 = pushFactory - .create(db, admin.getIdent(), testRepo, "parent 2", - ImmutableMap.of("foo", "foo-2.2", "bar", "bar-2.2")) - .to("refs/for/master"); - - PushOneCommit m = pushFactory.create(db, admin.getIdent(), testRepo, - "merge", ImmutableMap.of("foo", "foo-1", "bar", "bar-2")); - m.setParents(ImmutableList.of(p1.getCommit(), p2.getCommit())); - PushOneCommit.Result result = m.to("refs/for/master"); - result.assertOkStatus(); - - List mergeList = - gApi.changes().id(result.getChangeId()).current().getMergeList().get(); - assertThat(mergeList).hasSize(2); - assertThat(mergeList.get(0).commit).isEqualTo(p2.getCommit().name()); - assertThat(mergeList.get(1).commit).isEqualTo(gp2.getCommit().name()); - - mergeList = gApi.changes().id(result.getChangeId()).current().getMergeList() - .withUninterestingParent(2).get(); - assertThat(mergeList).hasSize(2); - assertThat(mergeList.get(0).commit).isEqualTo(p1.getCommit().name()); - assertThat(mergeList.get(1).commit).isEqualTo(gp1.getCommit().name()); - } -} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/MergeListIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/MergeListIT.java new file mode 100644 index 0000000000..481df31618 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/MergeListIT.java @@ -0,0 +1,209 @@ +// 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.acceptance.api.change; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.eclipse.jgit.lib.Constants.HEAD; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.common.RawInputUtil; +import com.google.gerrit.extensions.api.changes.RevisionApi; +import com.google.gerrit.extensions.common.CommitInfo; +import com.google.gerrit.extensions.common.DiffInfo; +import com.google.gerrit.extensions.restapi.BinaryResult; +import com.google.gerrit.server.edit.ChangeEditModifier; +import com.google.gerrit.server.edit.ChangeEditUtil; +import com.google.gerrit.server.project.InvalidChangeOperationException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.inject.Inject; + +import org.eclipse.jgit.dircache.InvalidPathException; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.Before; +import org.junit.Test; + +import java.io.ByteArrayOutputStream; +import java.util.List; +import java.util.Set; + +@NoHttpd +public class MergeListIT extends AbstractDaemonTest { + + private String changeId; + private RevCommit merge; + private RevCommit parent1; + private RevCommit grandParent1; + private RevCommit parent2; + private RevCommit grandParent2; + + @Inject + private ChangeEditModifier modifier; + + @Inject + private ChangeEditUtil editUtil; + + @Before + public void setup() throws Exception { + ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId(); + + PushOneCommit.Result gp1 = pushFactory + .create(db, admin.getIdent(), testRepo, "grand parent 1", + ImmutableMap.of("foo", "foo-1.1", "bar", "bar-1.1")) + .to("refs/for/master"); + grandParent1 = gp1.getCommit(); + + PushOneCommit.Result p1 = pushFactory + .create(db, admin.getIdent(), testRepo, "parent 1", + ImmutableMap.of("foo", "foo-1.2", "bar", "bar-1.2")) + .to("refs/for/master"); + parent1 = p1.getCommit(); + + // reset HEAD in order to create a sibling of the first change + testRepo.reset(initial); + + PushOneCommit.Result gp2 = pushFactory + .create(db, admin.getIdent(), testRepo, "grand parent 2", + ImmutableMap.of("foo", "foo-2.1", "bar", "bar-2.1")) + .to("refs/for/master"); + grandParent2 = gp2.getCommit(); + + PushOneCommit.Result p2 = pushFactory + .create(db, admin.getIdent(), testRepo, "parent 2", + ImmutableMap.of("foo", "foo-2.2", "bar", "bar-2.2")) + .to("refs/for/master"); + parent2 = p2.getCommit(); + + PushOneCommit m = pushFactory.create(db, admin.getIdent(), testRepo, + "merge", ImmutableMap.of("foo", "foo-1", "bar", "bar-2")); + m.setParents(ImmutableList.of(p1.getCommit(), p2.getCommit())); + PushOneCommit.Result result = m.to("refs/for/master"); + result.assertOkStatus(); + merge = result.getCommit(); + changeId = result.getChangeId(); + } + + @Test + public void getMergeList() throws Exception { + List mergeList = current(changeId).getMergeList().get(); + assertThat(mergeList).hasSize(2); + assertThat(mergeList.get(0).commit).isEqualTo(parent2.name()); + assertThat(mergeList.get(1).commit).isEqualTo(grandParent2.name()); + + mergeList = current(changeId).getMergeList() + .withUninterestingParent(2).get(); + assertThat(mergeList).hasSize(2); + assertThat(mergeList.get(0).commit).isEqualTo(parent1.name()); + assertThat(mergeList.get(1).commit).isEqualTo(grandParent1.name()); + } + + @Test + public void getMergeListContent() throws Exception { + BinaryResult bin = current(changeId).file(MERGE_LIST).content(); + ByteArrayOutputStream os = new ByteArrayOutputStream(); + bin.writeTo(os); + String content = new String(os.toByteArray(), UTF_8); + assertThat(content).isEqualTo( + getMergeListContent(parent2, grandParent2)); + } + + @Test + public void getFileList() throws Exception { + assertThat(getFiles(changeId)).contains(MERGE_LIST); + assertThat(getFiles(changeId, 1)).contains(MERGE_LIST); + assertThat(getFiles(changeId, 2)).contains(MERGE_LIST); + + assertThat(getFiles(createChange().getChangeId())) + .doesNotContain(MERGE_LIST); + } + + @Test + public void getDiffForMergeList() throws Exception { + DiffInfo diff = getMergeListDiff(changeId); + assertDiffForNewFile(diff, merge, MERGE_LIST, + getMergeListContent(parent2, grandParent2)); + + diff = getMergeListDiff(changeId, 1); + assertDiffForNewFile(diff, merge, MERGE_LIST, + getMergeListContent(parent2, grandParent2)); + + diff = getMergeListDiff(changeId, 2); + assertDiffForNewFile(diff, merge, MERGE_LIST, + getMergeListContent(parent1, grandParent1)); + } + + @Test + public void editMergeList() throws Exception { + ChangeData cd = getOnlyElement(queryProvider.get().byKeyPrefix(changeId)); + modifier.createEdit(cd.change(), cd.currentPatchSet()); + + exception.expect(InvalidPathException.class); + exception.expectMessage("Invalid path: " + MERGE_LIST); + modifier.modifyFile(editUtil.byChange(cd.change()).get(), MERGE_LIST, + RawInputUtil.create("new content")); + } + + @Test + public void deleteMergeList() throws Exception { + ChangeData cd = getOnlyElement(queryProvider.get().byKeyPrefix(changeId)); + modifier.createEdit(cd.change(), cd.currentPatchSet()); + + exception.expect(InvalidChangeOperationException.class); + exception.expectMessage("no changes were made"); + modifier.deleteFile(editUtil.byChange(cd.change()).get(), MERGE_LIST); + } + + private String getMergeListContent(RevCommit... commits) { + StringBuilder mergeList = new StringBuilder("Merge List:\n\n"); + for (RevCommit c : commits) { + mergeList.append("* ") + .append(c.abbreviate(8).name()) + .append(" ") + .append(c.getShortMessage()) + .append("\n"); + } + return mergeList.toString(); + } + + private Set getFiles(String changeId) throws Exception { + return current(changeId).files().keySet(); + } + + private Set getFiles(String changeId, int parent) throws Exception { + return current(changeId).files(parent).keySet(); + } + + private DiffInfo getMergeListDiff(String changeId) throws Exception { + return current(changeId).file(MERGE_LIST).diff(); + } + + private DiffInfo getMergeListDiff(String changeId, int parent) + throws Exception { + return current(changeId).file(MERGE_LIST).diff(parent); + } + + private RevisionApi current(String changeId) throws Exception { + return gApi.changes() + .id(changeId) + .current(); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 322cd4e702..de0c3c17a5 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -20,11 +20,13 @@ import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.PATCH; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG; +import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.nio.charset.StandardCharsets.UTF_8; import static org.eclipse.jgit.lib.Constants.HEAD; import static org.junit.Assert.fail; +import com.google.common.base.Joiner; import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -45,7 +47,6 @@ import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; -import com.google.gerrit.extensions.common.ChangeType; import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.DiffInfo; import com.google.gerrit.extensions.common.FileInfo; @@ -539,7 +540,7 @@ public class RevisionIT extends AbstractDaemonTest { .revision(r.getCommit().name()) .files() .keySet() - ).containsExactly(COMMIT_MSG, "foo", "bar"); + ).containsExactly(COMMIT_MSG, MERGE_LIST, "foo", "bar"); // list files against parent 1 assertThat(gApi.changes() @@ -547,7 +548,7 @@ public class RevisionIT extends AbstractDaemonTest { .revision(r.getCommit().name()) .files(1) .keySet() - ).containsExactly(COMMIT_MSG, "bar"); + ).containsExactly(COMMIT_MSG, MERGE_LIST, "bar"); // list files against parent 2 assertThat(gApi.changes() @@ -555,7 +556,7 @@ public class RevisionIT extends AbstractDaemonTest { .revision(r.getCommit().name()) .files(2) .keySet() - ).containsExactly(COMMIT_MSG, "foo"); + ).containsExactly(COMMIT_MSG, MERGE_LIST, "foo"); } @Test @@ -853,66 +854,40 @@ public class RevisionIT extends AbstractDaemonTest { .file(path) .diff(); - List expectedLines = new ArrayList<>(); + List headers = new ArrayList<>(); if (path.equals(COMMIT_MSG)) { RevCommit c = pushResult.getCommit(); RevCommit parentCommit = c.getParents()[0]; String parentCommitId = testRepo.getRevWalk().getObjectReader() .abbreviate(parentCommit.getId(), 8).name(); - expectedLines.add("Parent: " + parentCommitId + " (" + headers.add("Parent: " + parentCommitId + " (" + parentCommit.getShortMessage() + ")"); SimpleDateFormat dtfmt = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss Z", Locale.US); PersonIdent author = c.getAuthorIdent(); dtfmt.setTimeZone(author.getTimeZone()); - expectedLines.add("Author: " + author.getName() + " <" + headers.add("Author: " + author.getName() + " <" + author.getEmailAddress() + ">"); - expectedLines.add("AuthorDate: " + headers.add("AuthorDate: " + dtfmt.format(Long.valueOf(author.getWhen().getTime()))); PersonIdent committer = c.getCommitterIdent(); dtfmt.setTimeZone(committer.getTimeZone()); - expectedLines.add("Commit: " + committer.getName() + " <" + headers.add("Commit: " + committer.getName() + " <" + committer.getEmailAddress() + ">"); - expectedLines.add("CommitDate: " + headers.add("CommitDate: " + dtfmt.format(Long.valueOf(committer.getWhen().getTime()))); - expectedLines.add(""); + headers.add(""); } - for (String line : expectedContentSideB.split("\n")) { - expectedLines.add(line); + if (!headers.isEmpty()) { + String header = Joiner.on("\n").join(headers); + expectedContentSideB = header + "\n" + expectedContentSideB; } - assertThat(diff.binary).isNull(); - assertThat(diff.changeType).isEqualTo(ChangeType.ADDED); - assertThat(diff.diffHeader).isNotNull(); - assertThat(diff.intralineStatus).isNull(); - assertThat(diff.webLinks).isNull(); - - assertThat(diff.metaA).isNull(); - assertThat(diff.metaB).isNotNull(); - assertThat(diff.metaB.commitId).isEqualTo(pushResult.getCommit().name()); - assertThat(diff.metaB.contentType).isEqualTo( - path.equals(COMMIT_MSG) - ? "text/x-gerrit-commit-message" - : "text/plain"); - assertThat(diff.metaB.lines).isEqualTo(expectedLines.size()); - assertThat(diff.metaB.name).isEqualTo(path); - assertThat(diff.metaB.webLinks).isNull(); - - assertThat(diff.content).hasSize(1); - DiffInfo.ContentEntry contentEntry = diff.content.get(0); - assertThat(contentEntry.b).hasSize(expectedLines.size()); - for (int i = 0; i < contentEntry.b.size(); i++) { - assertThat(contentEntry.b.get(i)).isEqualTo(expectedLines.get(i)); - } - assertThat(contentEntry.a).isNull(); - assertThat(contentEntry.ab).isNull(); - assertThat(contentEntry.common).isNull(); - assertThat(contentEntry.editA).isNull(); - assertThat(contentEntry.editB).isNull(); - assertThat(contentEntry.skip).isNull(); + assertDiffForNewFile(diff, pushResult.getCommit(), path, + expectedContentSideB); } } diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java index 9b290a5d21..b21126d5d8 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java @@ -56,6 +56,12 @@ public class FileInfo extends JavaScriptObject { } else if (Patch.COMMIT_MSG.equals(b.path())) { return 1; } + if (Patch.MERGE_LIST.equals(a.path())) { + return -1; + } else if (Patch.MERGE_LIST.equals(b.path())) { + return 1; + } + // Look at file suffixes to check if it makes sense to use a different order int s1 = a.path().lastIndexOf('.'); int s2 = b.path().lastIndexOf('.'); @@ -76,9 +82,15 @@ public class FileInfo extends JavaScriptObject { } public static String getFileName(String path) { - String fileName = Patch.COMMIT_MSG.equals(path) - ? "Commit Message" - : path; + String fileName; + if (Patch.COMMIT_MSG.equals(path)) { + fileName = "Commit Message"; + } else if (Patch.MERGE_LIST.equals(path)) { + fileName = "Merge List"; + } else { + fileName = path; + } + int s = fileName.lastIndexOf('/'); return s >= 0 ? fileName.substring(s + 1) : fileName; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java index 2b4c821662..cb2ac07dbc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java @@ -486,7 +486,11 @@ public class Dispatcher { } else if ("unified".equals(panel)) { unified(token, baseId, id, side, line); } else if ("edit".equals(panel)) { - codemirrorForEdit(token, id, line); + if (!Patch.isMagic(id.get()) || Patch.COMMIT_MSG.equals(id.get())) { + codemirrorForEdit(token, id, line); + } else { + Gerrit.display(token, new NotFoundScreen()); + } } else { Gerrit.display(token, new NotFoundScreen()); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java index 940cb8f0ce..3a85b2655d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java @@ -267,11 +267,18 @@ public class FileTable extends FlowPanel { if (table != null) { String self = Gerrit.selfRedirect(null); for (FileInfo info : Natives.asList(table.list)) { - Window.open(self + "#" + url(info), "_blank", null); + if (canOpen(info.path())) { + Window.open(self + "#" + url(info), "_blank", null); + } } } } + private boolean canOpen(String path) { + return mode != Mode.EDIT || !Patch.isMagic(path) + || Patch.COMMIT_MSG.equals(path); + } + private void setTable(MyTable table) { clear(); add(table); @@ -429,7 +436,10 @@ public class FileTable extends FlowPanel { @Override protected void onOpenRow(int row) { if (1 <= row && row <= list.length()) { - Gerrit.display(url(list.get(row - 1))); + FileInfo info = list.get(row - 1); + if (canOpen(info.path())) { + Gerrit.display(url(info)); + } } } @@ -452,7 +462,10 @@ public class FileTable extends FlowPanel { @Override public void onKeyPress(KeyPressEvent event) { - Gerrit.display(url(list.get(index))); + FileInfo info = list.get(index); + if (canOpen(info.path())) { + Gerrit.display(url(info)); + } } } } @@ -668,20 +681,43 @@ public class FileTable extends FlowPanel { } private void columnPath(SafeHtmlBuilder sb, FileInfo info) { - sb.openTd() - .setStyleName(R.css().pathColumn()) - .openAnchor(); - String path = info.path(); + + sb.openTd() + .setStyleName(R.css().pathColumn()); + + if (!canOpen(path)) { + sb.openDiv(); + appendPath(path); + sb.closeDiv(); + sb.closeTd(); + return; + } + + sb.openAnchor(); + if (mode == Mode.EDIT && !isEditable(info)) { sb.setAttribute("onclick", RESTORE + "(event," + info._row() + ")"); } else { sb.setAttribute("href", "#" + url(info)) .setAttribute("onclick", OPEN + "(event," + info._row() + ")"); } + appendPath(path); + sb.closeAnchor(); + if (info.oldPath() != null) { + sb.br(); + sb.openSpan().setStyleName(R.css().renameCopySource()) + .append(info.oldPath()) + .closeSpan(); + } + sb.closeTd(); + } + private void appendPath(String path) { if (Patch.COMMIT_MSG.equals(path)) { sb.append(Util.C.commitMessage()); + } else if (Patch.MERGE_LIST.equals(path)) { + sb.append(Util.C.mergeList()); } else if (Gerrit.getUserPreferences().muteCommonPathPrefixes()) { int commonPrefixLen = commonPrefix(path); if (commonPrefixLen > 0) { @@ -694,15 +730,6 @@ public class FileTable extends FlowPanel { } else { sb.append(path); } - - sb.closeAnchor(); - if (info.oldPath() != null) { - sb.br(); - sb.openSpan().setStyleName(R.css().renameCopySource()) - .append(info.oldPath()) - .closeSpan(); - } - sb.closeTd(); } private int commonPrefix(String path) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java index 6c27ed9435..c8735b738f 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java @@ -176,6 +176,10 @@ class Message extends Composite { if (l != null) { comments.add(new FileComments(clp, ps, Util.C.commitMessage(), l)); } + l = m.remove(Patch.MERGE_LIST); + if (l != null) { + comments.add(new FileComments(clp, ps, Util.C.mergeList(), l)); + } for (Map.Entry> e : m.entrySet()) { comments.add(new FileComments(clp, ps, e.getKey(), e.getValue())); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java index fc583f2d8b..b985f4320c 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java @@ -422,6 +422,11 @@ public class ReplyBox extends Composite { comments.add(new FileComments(clp, psId, Util.C.commitMessage(), copyPath(Patch.COMMIT_MSG, l))); } + l = m.get(Patch.MERGE_LIST); + if (l != null) { + comments.add(new FileComments(clp, psId, Util.C.commitMessage(), + copyPath(Patch.MERGE_LIST, l))); + } List paths = new ArrayList<>(m.keySet()); Collections.sort(paths); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java index b2334d1d18..4a9eea4e0e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java @@ -63,6 +63,7 @@ public interface ChangeConstants extends Constants { String patchTableColumnComments(); String patchTableColumnSize(); String commitMessage(); + String mergeList(); String patchTablePrev(); String patchTableNext(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties index b7e267752b..675cd072fd 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties @@ -45,6 +45,7 @@ patchTableColumnName = File Path patchTableColumnComments = Comments patchTableColumnSize = Size commitMessage = Commit Message +mergeList = Merge List patchTablePrev = Previous file patchTableNext = Next file diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java index f3770382fa..3033cbb1f8 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java @@ -122,6 +122,8 @@ public class Header extends Composite { SafeHtmlBuilder b = new SafeHtmlBuilder(); if (Patch.COMMIT_MSG.equals(path)) { return b.append(Util.C.commitMessage()); + } else if (Patch.MERGE_LIST.equals(path)) { + return b.append(Util.C.mergeList()); } int s = path.lastIndexOf('/') + 1; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java index 82c22cda94..309bda4286 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Patch.java @@ -22,6 +22,9 @@ public final class Patch { /** Magical file name which represents the commit message. */ public static final String COMMIT_MSG = "/COMMIT_MSG"; + /** Magical file name which represents the merge list of a merge commit. */ + public static final String MERGE_LIST = "/MERGE_LIST"; + /** * Checks if the given path represents a magic file. A magic file is a * generated file that is automatically included into changes. It does not @@ -32,7 +35,7 @@ public final class Patch { * {@code false}. */ public static boolean isMagic(String path) { - return COMMIT_MSG.equals(path); + return COMMIT_MSG.equals(path) || MERGE_LIST.equals(path); } public static class Key extends StringKey { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index e950d9993d..95e9043ece 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -1064,6 +1064,7 @@ public class ChangeJson { if (has(ALL_FILES) || (out.isCurrent && has(CURRENT_FILES))) { out.files = fileInfoJson.toFileInfoMap(c, in); out.files.remove(Patch.COMMIT_MSG); + out.files.remove(Patch.MERGE_LIST); } if ((out.isCurrent || (out.draft != null && out.draft)) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java index d145ddf7ae..d617a70d28 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java @@ -54,6 +54,7 @@ import java.util.zip.ZipOutputStream; @Singleton public class FileContentUtil { public static final String TEXT_X_GERRIT_COMMIT_MESSAGE = "text/x-gerrit-commit-message"; + public static final String TEXT_X_GERRIT_MERGE_LIST = "text/x-gerrit-merge-list"; private static final String X_GIT_SYMLINK = "x-git/symlink"; private static final String X_GIT_GITLINK = "x-git/gitlink"; private static final int MAX_SIZE = 5 << 20; @@ -264,6 +265,9 @@ public class FileContentUtil { if (Patch.COMMIT_MSG.equals(path)) { return TEXT_X_GERRIT_COMMIT_MESSAGE; } + if (Patch.MERGE_LIST.equals(path)) { + return TEXT_X_GERRIT_MERGE_LIST; + } if (project != null) { for (ProjectState p : project.tree()) { String t = p.getConfig().getMimeTypes().getMimeType(path); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetContent.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetContent.java index 5a546f39df..c7044e1ebc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetContent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetContent.java @@ -24,6 +24,8 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.patch.ComparisonType; +import com.google.gerrit.server.patch.Text; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -68,6 +70,12 @@ public class GetContent implements RestReadView { return BinaryResult.create(msg) .setContentType(FileContentUtil.TEXT_X_GERRIT_COMMIT_MESSAGE) .base64(); + } else if (Patch.MERGE_LIST.equals(path)) { + byte[] mergeList = getMergeList( + rsrc.getRevision().getChangeResource().getNotes()); + return BinaryResult.create(mergeList) + .setContentType(FileContentUtil.TEXT_X_GERRIT_MERGE_LIST) + .base64(); } return fileContentUtil.getContent( rsrc.getRevision().getControl().getProjectControl().getProjectState(), @@ -92,4 +100,22 @@ public class GetContent implements RestReadView { throw new NoSuchChangeException(changeId, e); } } + + private byte[] getMergeList(ChangeNotes notes) + throws NoSuchChangeException, OrmException, IOException { + Change.Id changeId = notes.getChangeId(); + PatchSet ps = psUtil.current(db.get(), notes); + if (ps == null) { + throw new NoSuchChangeException(changeId); + } + + try (Repository git = gitManager.openRepository(notes.getProjectName()); + RevWalk revWalk = new RevWalk(git)) { + return Text.forMergeList(ComparisonType.againstAutoMerge(), + revWalk.getObjectReader(), + ObjectId.fromString(ps.getRevision().get())).getContent(); + } catch (RepositoryNotFoundException e) { + throw new NoSuchChangeException(changeId, e); + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java index 4e94935853..b40daa6d77 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java @@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.patch.MergeListBuilder; import com.google.inject.Inject; import org.eclipse.jgit.lib.ObjectId; @@ -46,7 +47,8 @@ public class GetMergeList implements RestReadView { private boolean addLinks; @Inject - GetMergeList(GitRepositoryManager repoManager, ChangeJson.Factory json) { + GetMergeList(GitRepositoryManager repoManager, + ChangeJson.Factory json) { this.repoManager = repoManager; this.json = json; } @@ -62,7 +64,6 @@ public class GetMergeList implements RestReadView { @Override public Response> apply(RevisionResource rsrc) throws BadRequestException, IOException { - List result = new ArrayList<>(); Project.NameKey p = rsrc.getChange().getProject(); try (Repository repo = repoManager.openRepository(p); RevWalk rw = new RevWalk(repo)) { @@ -79,27 +80,19 @@ public class GetMergeList implements RestReadView { return Response.> ok(ImmutableList. of()); } - for (int parent = 0; parent < commit.getParentCount(); parent++) { - if (parent == uninterestingParent - 1) { - rw.markUninteresting(commit.getParent(parent)); - } else { - rw.markStart(commit.getParent(parent)); - } - } - + List commits = + MergeListBuilder.build(rw, commit, uninterestingParent); + List result = new ArrayList<>(commits.size()); ChangeJson changeJson = json.create(ChangeJson.NO_OPTIONS); - RevCommit c; - while ((c = rw.next()) != null) { - CommitInfo info = - changeJson.toCommit(rsrc.getControl(), rw, c, addLinks, true); - result.add(info); + for (RevCommit c : commits) { + result.add(changeJson.toCommit(rsrc.getControl(), rw, c, addLinks, true)); } - } - Response> r = Response.ok(result); - if (rsrc.isCacheable()) { - r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS)); + Response> r = Response.ok(result); + if (rsrc.isCacheable()) { + r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS)); + } + return r; } - return r; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 7cd26e12ff..c4b6869fbc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -137,6 +137,8 @@ public class CommentSender extends ReplyToChangeSender { } if (Patch.COMMIT_MSG.equals(pk.get())) { cmts.append("Commit Message:\n\n"); + } else if (Patch.MERGE_LIST.equals(pk.get())) { + cmts.append("Merge List:\n\n"); } else { cmts.append("File ").append(pk.get()).append(":\n\n"); } @@ -144,8 +146,7 @@ public class CommentSender extends ReplyToChangeSender { if (patchList != null) { try { - currentFileData = - new PatchFile(repo, patchList, pk.get()); + currentFileData = new PatchFile(repo, patchList, pk.get()); } catch (IOException e) { log.warn(String.format( "Cannot load %s from %s in %s", 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 index 343d2e2c8c..abbb680ee6 100644 --- 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 @@ -14,6 +14,7 @@ package com.google.gerrit.server.patch; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32; import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32; @@ -28,15 +29,15 @@ public class ComparisonType { private final boolean autoMerge; - static ComparisonType againstOtherPatchSet() { + public static ComparisonType againstOtherPatchSet() { return new ComparisonType(null, false); } - static ComparisonType againstParent(int parentNum) { + public static ComparisonType againstParent(int parentNum) { return new ComparisonType(parentNum, false); } - static ComparisonType againstAutoMerge() { + public static ComparisonType againstAutoMerge() { return new ComparisonType(null, true); } @@ -57,6 +58,11 @@ public class ComparisonType { return autoMerge; } + public int getParentNum() { + checkNotNull(parentNum); + return parentNum; + } + void writeTo(OutputStream out) throws IOException { writeVarInt32(out, parentNum != null ? parentNum : 0); writeVarInt32(out, autoMerge ? 1 : 0); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/MergeListBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/MergeListBuilder.java new file mode 100644 index 0000000000..8f54e48ffa --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/MergeListBuilder.java @@ -0,0 +1,52 @@ +// 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 com.google.common.collect.ImmutableList; + +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +public class MergeListBuilder { + public static List build(RevWalk rw, RevCommit merge, + int uninterestingParent) throws IOException { + rw.reset(); + rw.parseBody(merge); + if (merge.getParentCount() < 2) { + return ImmutableList.of(); + } + + for (int parent = 0; parent < merge.getParentCount(); parent++) { + RevCommit parentCommit = merge.getParent(parent); + rw.parseBody(parentCommit); + if (parent == uninterestingParent - 1) { + rw.markUninteresting(parentCommit); + } else { + rw.markStart(parentCommit); + } + } + + List result = new ArrayList<>(); + RevCommit c; + while ((c = rw.next()) != null) { + result.add(c); + } + return result; + } +} 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 71129f6bfc..d2a6d2b473 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 @@ -44,9 +44,8 @@ public class PatchFile { private Text a; private Text b; - public PatchFile(final Repository repo, final PatchList patchList, - final String fileName) throws MissingObjectException, - IncorrectObjectTypeException, IOException { + public PatchFile(Repository repo, PatchList patchList, String fileName) + throws MissingObjectException, IncorrectObjectTypeException, IOException { this.repo = repo; this.entry = patchList.get(fileName); @@ -68,7 +67,16 @@ public class PatchFile { aTree = null; bTree = null; + } else if (Patch.MERGE_LIST.equals(fileName)) { + // For the initial commit, we have an empty tree on Side A + RevObject object = rw.parseAny(patchList.getOldId()); + a = object instanceof RevCommit + ? Text.forMergeList(patchList.getComparisonType(), reader, object) + : Text.EMPTY; + b = Text.forMergeList(patchList.getComparisonType(), reader, bCommit); + aTree = null; + bTree = null; } else { if (patchList.getOldId() != null) { aTree = rw.parseTree(patchList.getOldId()); 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 fa40bf05da..2cfd0074d8 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,15 +58,18 @@ public class PatchList implements Serializable { @Nullable private transient ObjectId oldId; private transient ObjectId newId; + private transient boolean isMerge; private transient ComparisonType comparisonType; private transient int insertions; private transient int deletions; private transient PatchListEntry[] patches; public PatchList(@Nullable AnyObjectId oldId, AnyObjectId newId, - ComparisonType comparisonType, PatchListEntry[] patches) { + boolean isMerge, ComparisonType comparisonType, + PatchListEntry[] patches) { this.oldId = oldId != null ? oldId.copy() : null; this.newId = newId.copy(); + this.isMerge = isMerge; this.comparisonType = comparisonType; // We assume index 0 contains the magic commit message entry. @@ -144,9 +147,12 @@ public class PatchList implements Serializable { if (Patch.COMMIT_MSG.equals(fileName)) { return 0; } + if (isMerge && Patch.MERGE_LIST.equals(fileName)) { + return 1; + } int high = patches.length; - int low = 1; + int low = isMerge ? 2 : 1; while (low < high) { final int mid = (low + high) >>> 1; final int cmp = patches[mid].getNewName().compareTo(fileName); @@ -166,6 +172,7 @@ public class PatchList implements Serializable { try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) { writeCanBeNull(out, oldId); writeNotNull(out, newId); + writeVarInt32(out, isMerge ? 1 : 0); comparisonType.writeTo(out); writeVarInt32(out, insertions); writeVarInt32(out, deletions); @@ -182,6 +189,7 @@ public class PatchList implements Serializable { try (InflaterInputStream in = new InflaterInputStream(buf)) { oldId = readCanBeNull(in); newId = readNotNull(in); + isMerge = readVarInt32(in) != 0; comparisonType = ComparisonType.readFrom(in); insertions = readVarInt32(in); deletions = 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 e732560718..22f7bf31b6 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 = 23L; + public static final long serialVersionUID = 24L; public static final BiMap WHITESPACE_TYPES = ImmutableBiMap.of( Whitespace.IGNORE_NONE, 'N', @@ -138,6 +138,10 @@ public class PatchListKey implements Serializable { n.append(".."); n.append(newId.name()); n.append(" "); + if (parentNum != null) { + n.append(parentNum); + n.append(" "); + } n.append(whitespace.name()); n.append("]"); return n.toString(); 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 fc05630070..9616fc85fa 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 @@ -16,7 +16,6 @@ package com.google.gerrit.server.patch; import static com.google.common.base.Preconditions.checkArgument; - import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toSet; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; @@ -161,9 +160,11 @@ public class PatchListLoader implements Callable { // 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]; + ComparisonType comparisonType = ComparisonType.againstParent(1); + PatchListEntry[] entries = new PatchListEntry[2]; entries[0] = newCommitMessage(cmp, reader, null, b); - return new PatchList(a, b, ComparisonType.againstParent(1), entries); + entries[1] = newMergeList(cmp, reader, null, b, comparisonType); + return new PatchList(a, b, true, comparisonType, entries); } ComparisonType comparisonType = getComparisonType(a, b); @@ -194,6 +195,12 @@ public class PatchListLoader implements Callable { List entries = new ArrayList<>(); entries.add(newCommitMessage(cmp, reader, comparisonType.isAgainstParentOrAutoMerge() ? null : aCommit, b)); + boolean isMerge = b.getParentCount() > 1; + if (isMerge) { + entries.add(newMergeList(cmp, reader, + comparisonType.isAgainstParentOrAutoMerge() ? null : aCommit, b, + comparisonType)); + } for (int i = 0; i < cnt; i++) { DiffEntry e = diffEntries.get(i); if (paths == null || paths.contains(e.getNewPath()) @@ -207,7 +214,7 @@ public class PatchListLoader implements Callable { entries.add(newEntry(aTree, fh, newSize, newSize - oldSize)); } } - return new PatchList(a, b, comparisonType, + return new PatchList(a, b, isMerge, comparisonType, entries.toArray(new PatchListEntry[entries.size()])); } } @@ -285,32 +292,30 @@ public class PatchListLoader implements Callable { return diffFormatter.toFileHeader(diffEntry); } - private PatchListEntry newCommitMessage(final RawTextComparator cmp, - final ObjectReader reader, - final RevCommit aCommit, final RevCommit bCommit) throws IOException { - StringBuilder hdr = new StringBuilder(); - - hdr.append("diff --git"); - if (aCommit != null) { - hdr.append(" a/").append(Patch.COMMIT_MSG); - } else { - hdr.append(" ").append(FileHeader.DEV_NULL); - } - hdr.append(" b/").append(Patch.COMMIT_MSG); - hdr.append("\n"); - - if (aCommit != null) { - hdr.append("--- a/").append(Patch.COMMIT_MSG).append("\n"); - } else { - hdr.append("--- ").append(FileHeader.DEV_NULL).append("\n"); - } - hdr.append("+++ b/").append(Patch.COMMIT_MSG).append("\n"); - - Text aText = - aCommit != null ? Text.forCommit(reader, aCommit) : Text.EMPTY; + private PatchListEntry newCommitMessage(RawTextComparator cmp, + ObjectReader reader, RevCommit aCommit, RevCommit bCommit) + throws IOException { + Text aText = aCommit != null + ? Text.forCommit(reader, aCommit) + : Text.EMPTY; Text bText = Text.forCommit(reader, bCommit); + return createPatchListEntry(cmp, aCommit, aText, bText, Patch.COMMIT_MSG); + } - byte[] rawHdr = hdr.toString().getBytes(UTF_8); + private PatchListEntry newMergeList(RawTextComparator cmp, + ObjectReader reader, RevCommit aCommit, RevCommit bCommit, + ComparisonType comparisonType) throws IOException { + Text aText = aCommit != null + ? Text.forMergeList(comparisonType, reader, aCommit) + : Text.EMPTY; + Text bText = + Text.forMergeList(comparisonType, reader, bCommit); + return createPatchListEntry(cmp, aCommit, aText, bText, Patch.MERGE_LIST); + } + + private static PatchListEntry createPatchListEntry(RawTextComparator cmp, + RevCommit aCommit, Text aText, Text bText, String fileName) { + byte[] rawHdr = getRawHeader(aCommit != null, fileName); byte[] aContent = aText.getContent(); byte[] bContent = bText.getContent(); long size = bContent.length; @@ -322,6 +327,26 @@ public class PatchListLoader implements Callable { return new PatchListEntry(fh, edits, size, sizeDelta); } + private static byte[] getRawHeader(boolean hasA, String fileName) { + StringBuilder hdr = new StringBuilder(); + hdr.append("diff --git"); + if (hasA) { + hdr.append(" a/").append(fileName); + } else { + hdr.append(" ").append(FileHeader.DEV_NULL); + } + hdr.append(" b/").append(fileName); + hdr.append("\n"); + + if (hasA) { + hdr.append("--- a/").append(fileName).append("\n"); + } else { + hdr.append("--- ").append(FileHeader.DEV_NULL).append("\n"); + } + hdr.append("+++ b/").append(fileName).append("\n"); + return hdr.toString().getBytes(UTF_8); + } + private PatchListEntry newEntry(RevTree aTree, FileHeader fileHeader, long size, long sizeDelta) { if (aTree == null // want combined diff 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 5b30f8c768..7eee6a3096 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 @@ -79,7 +79,8 @@ class PatchScriptBuilder { private int context; @Inject - PatchScriptBuilder(final FileTypeRegistry ftr, final PatchListCache plc) { + PatchScriptBuilder(FileTypeRegistry ftr, + PatchListCache plc) { a = new Side(); b = new Side(); registry = ftr; @@ -454,7 +455,26 @@ class PatchScriptBuilder { } } reuse = false; - + } else if (Patch.MERGE_LIST.equals(path)) { + if (comparisonType.isAgainstParentOrAutoMerge() + && (aId == within || within.equals(aId))) { + id = ObjectId.zeroId(); + src = Text.EMPTY; + srcContent = Text.NO_BYTES; + mode = FileMode.MISSING; + displayMethod = DisplayMethod.NONE; + } else { + id = within; + src = Text.forMergeList(comparisonType, reader, within); + srcContent = src.getContent(); + if (src == Text.EMPTY) { + mode = FileMode.MISSING; + displayMethod = DisplayMethod.NONE; + } else { + mode = FileMode.REGULAR_FILE; + } + } + reuse = false; } else { final TreeWalk tw = find(within); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java index 7982479808..a84dd9237b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java @@ -87,6 +87,36 @@ public class Text extends RawText { } } + public static Text forMergeList(ComparisonType comparisonType, + ObjectReader reader, AnyObjectId commitId) throws IOException { + try (RevWalk rw = new RevWalk(reader)) { + RevCommit c = rw.parseCommit(commitId); + StringBuilder b = new StringBuilder(); + switch (c.getParentCount()) { + case 0: + break; + case 1: { + break; + } + default: + int uniterestingParent = comparisonType.isAgainstParent() + ? comparisonType.getParentNum() + : 1; + + b.append("Merge List:\n\n"); + for (RevCommit commit : MergeListBuilder.build(rw, c, + uniterestingParent)) { + b.append("* "); + b.append(reader.abbreviate(commit, 8).name()); + b.append(" "); + b.append(commit.getShortMessage()); + b.append("\n"); + } + } + return new Text(b.toString().getBytes(UTF_8)); + } + } + private static void appendPersonIdent(StringBuilder b, String field, PersonIdent person) { if (person != null) { diff --git a/gerrit-server/src/main/java/gerrit/PRED_commit_stats_3.java b/gerrit-server/src/main/java/gerrit/PRED_commit_stats_3.java index 1dbdb68cd6..a85586813d 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_commit_stats_3.java +++ b/gerrit-server/src/main/java/gerrit/PRED_commit_stats_3.java @@ -14,8 +14,10 @@ package gerrit; +import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.rules.StoredValues; import com.google.gerrit.server.patch.PatchList; +import com.google.gerrit.server.patch.PatchListEntry; import com.googlecode.prolog_cafe.exceptions.PrologException; import com.googlecode.prolog_cafe.lang.IntegerTerm; @@ -24,6 +26,8 @@ import com.googlecode.prolog_cafe.lang.Predicate; import com.googlecode.prolog_cafe.lang.Prolog; import com.googlecode.prolog_cafe.lang.Term; +import java.util.List; + /** * Exports basic commit statistics. * @@ -48,7 +52,11 @@ public class PRED_commit_stats_3 extends Predicate.P3 { Term a3 = arg3.dereference(); PatchList pl = StoredValues.PATCH_LIST.get(engine); - if (!a1.unify(new IntegerTerm(pl.getPatches().size() - 1),engine.trail)) { //Account for /COMMIT_MSG. + // Account for magic files + if (!a1.unify( + new IntegerTerm( + pl.getPatches().size() - countMagicFiles(pl.getPatches())), + engine.trail)) { return engine.fail(); } if (!a2.unify(new IntegerTerm(pl.getInsertions()),engine.trail)) { @@ -59,4 +67,14 @@ public class PRED_commit_stats_3 extends Predicate.P3 { } return cont; } + + private int countMagicFiles(List entries) { + int count = 0; + for (PatchListEntry e : entries) { + if (Patch.isMagic(e.getNewName())) { + count++; + } + } + return count; + } } From bf5381df7dd96f70a50d86ca98e4e17c6b1713d0 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 16 Sep 2016 10:45:24 +0200 Subject: [PATCH 4/4] GetMergeList: Also allow caching of responses for non-merges Change-Id: I66527dd14f0ce53015b6548c86eb8634cf3942ac Signed-off-by: Edwin Kempin --- .../gerrit/server/change/GetMergeList.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java index b40daa6d77..b15810c248 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java @@ -77,7 +77,7 @@ public class GetMergeList implements RestReadView { } if (commit.getParentCount() < 2) { - return Response.> ok(ImmutableList. of()); + return createResponse(rsrc, ImmutableList. of()); } List commits = @@ -87,12 +87,16 @@ public class GetMergeList implements RestReadView { for (RevCommit c : commits) { result.add(changeJson.toCommit(rsrc.getControl(), rw, c, addLinks, true)); } - - Response> r = Response.ok(result); - if (rsrc.isCacheable()) { - r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS)); - } - return r; + return createResponse(rsrc, result); } } + + private static Response> createResponse( + RevisionResource rsrc, List result) { + Response> r = Response.ok(result); + if (rsrc.isCacheable()) { + r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS)); + } + return r; + } }