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 53f4599ae0..82de9f832e 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; @@ -48,6 +50,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; @@ -67,6 +71,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; @@ -546,21 +551,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(); @@ -1086,4 +1096,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-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-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 c0879e76c2..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)); + } } } } @@ -538,7 +551,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 +641,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 +672,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()); @@ -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) { @@ -784,7 +811,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 +840,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/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 e29048ac06..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,12 +422,17 @@ 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); 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/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-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..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,22 @@ 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 + * 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) || MERGE_LIST.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/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index 11389e6871..8f4c4f8038 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; import static java.util.stream.Collectors.toList; @@ -27,6 +29,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; @@ -210,10 +213,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/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 9bd5e443c9..76c70d3b73 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 @@ -1056,6 +1056,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..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 @@ -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)) { @@ -76,26 +77,22 @@ public class GetMergeList implements RestReadView { } if (commit.getParentCount() < 2) { - 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)); - } + return createResponse(rsrc, ImmutableList. of()); } + 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)); } + 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)); 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..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,11 +324,19 @@ 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.isMagic(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/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index 676f9ffd44..afd85ce45f 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 @@ -496,7 +496,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..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 @@ -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()); } } @@ -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 new file mode 100644 index 0000000000..abbb680ee6 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java @@ -0,0 +1,77 @@ +// 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.common.base.Preconditions.checkNotNull; +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; + + public static ComparisonType againstOtherPatchSet() { + return new ComparisonType(null, false); + } + + public static ComparisonType againstParent(int parentNum) { + return new ComparisonType(parentNum, false); + } + + public 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; + } + + public int getParentNum() { + checkNotNull(parentNum); + return parentNum; + } + + 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/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 f8c8b49abc..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); @@ -55,7 +54,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 @@ -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 2a4afb337d..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,16 +58,19 @@ public class PatchList implements Serializable { @Nullable private transient ObjectId oldId; private transient ObjectId newId; - private transient boolean againstParent; + private transient boolean isMerge; + 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, + boolean isMerge, ComparisonType comparisonType, + PatchListEntry[] patches) { this.oldId = oldId != null ? oldId.copy() : null; this.newId = newId.copy(); - this.againstParent = againstParent; + this.isMerge = isMerge; + this.comparisonType = comparisonType; // We assume index 0 contains the magic commit message entry. if (patches.length > 1) { @@ -97,9 +100,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. */ @@ -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,7 +172,8 @@ public class PatchList implements Serializable { try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) { writeCanBeNull(out, oldId); writeNotNull(out, newId); - writeVarInt32(out, againstParent ? 1 : 0); + writeVarInt32(out, isMerge ? 1 : 0); + comparisonType.writeTo(out); writeVarInt32(out, insertions); writeVarInt32(out, deletions); writeVarInt32(out, patches.length); @@ -182,7 +189,8 @@ public class PatchList implements Serializable { try (InflaterInputStream in = new InflaterInputStream(buf)) { oldId = readCanBeNull(in); newId = readNotNull(in); - againstParent = readVarInt32(in) != 0; + isMerge = 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..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 = 22L; + 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 0ae9a99ffb..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; @@ -156,14 +155,19 @@ 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]; + ComparisonType comparisonType = ComparisonType.againstParent(1); + PatchListEntry[] entries = new PatchListEntry[2]; entries[0] = newCommitMessage(cmp, reader, null, b); - return new PatchList(a, b, true, entries); + entries[1] = newMergeList(cmp, reader, null, b, comparisonType); + return new PatchList(a, b, true, comparisonType, 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 +194,13 @@ 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)); + 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()) @@ -204,19 +214,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, isMerge, 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, @@ -278,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; @@ -315,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 e09d26fac9..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 @@ -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; @@ -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; @@ -106,8 +107,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 +436,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; @@ -453,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/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; } 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/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 c4e8bd733d..5560b86eed 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()) { 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; + } }