From 27ce6452a99d50f00a13957f234e6bbb94777e6d Mon Sep 17 00:00:00 2001 From: Youssef Elghareeb Date: Fri, 6 Nov 2020 14:25:43 +0100 Subject: [PATCH] Support diffing for octopus merges against a specific parent Our status quo for octopus merges is not to compute the diff, regardless if the parentNum parameter is set or not. This used to return the COMMIT_MSG and MERGE_LIST entries only. This change adds support for diffing against a specifc parent for octopus merge commits. If no specific parent is specified, we do not compute the auto-merge commit and still follow the old behavior. This change is important to avoid merging changes with potentially unreviewed files, since they did not appear in the Gerrit UI. We still can follow up on this change by throwing an exception with a meaningful message if the auto-merge is requested for an octopus merge, so that the Gerrit UI can display that this operation is not supported. Change-Id: I6670585c7860719a4c226aa054ca635f67d90106 --- .../gerrit/acceptance/AbstractDaemonTest.java | 54 +++++++++++++++++++ .../gerrit/server/patch/PatchListLoader.java | 9 ++-- .../api/revision/RevisionDiffIT.java | 51 ++++++++++++++++++ 3 files changed, 110 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 4ab5d51c35..f59daf52c2 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -168,6 +168,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; @@ -767,6 +769,58 @@ public abstract class AbstractDaemonTest { return result; } + protected PushOneCommit.Result createNParentsMergeCommitChange(String ref, List fileNames) + throws Exception { + // This method creates n different commits and creates a merge commit pointing to all n parents. + // Each commit will contain all the fileNames. Commit i will have the following file names and + // their contents: + // {$file_1_name, ${file_1_name}-1} + // {$file_2_name, ${file_2_name}-1}, etc... + // The merge commit will have: + // {$file_1_name, ${file_1_name}-1} + // {$file_2_name, ${file_2_name}-2}, + // {$file_3_name, ${file_3_name}-3}, etc... + // i.e. taking the ith file from the ith commit. + int n = fileNames.size(); + ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId(); + + List pushResults = new ArrayList<>(); + + for (int i = 1; i <= n; i++) { + int finalI = i; + pushResults.add( + pushFactory + .create( + admin.newIdent(), + testRepo, + "parent " + i, + fileNames.stream().collect(Collectors.toMap(f -> f, f -> f + "-" + finalI))) + .to(ref)); + + // reset HEAD in order to create a sibling of the first change + if (i < n) { + testRepo.reset(initial); + } + } + + PushOneCommit m = + pushFactory.create( + admin.newIdent(), + testRepo, + "merge", + IntStream.range(1, n + 1) + .boxed() + .collect( + Collectors.toMap( + i -> fileNames.get((int) i - 1), + i -> fileNames.get((int) i - 1) + "-" + i))); + + m.setParents(pushResults.stream().map(PushOneCommit.Result::getCommit).collect(toList())); + PushOneCommit.Result result = m.to(ref); + result.assertOkStatus(); + return result; + } + protected PushOneCommit.Result createCommitAndPush( TestRepository repo, String ref, diff --git a/java/com/google/gerrit/server/patch/PatchListLoader.java b/java/com/google/gerrit/server/patch/PatchListLoader.java index be0895b7b1..2e9d58cf9f 100644 --- a/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -611,15 +611,16 @@ public class PatchListLoader implements Callable { rw.parseBody(r); return r; } - case 2: + default: if (key.getParentNum() != null) { RevCommit r = b.getParent(key.getParentNum() - 1); rw.parseBody(r); return r; } - return autoMerger.merge(repo, rw, ins, b, mergeStrategy); - default: - // TODO(sop) handle an octopus merge. + // Only support auto-merge for 2 parents, not octopus merges + if (b.getParentCount() == 2) { + return autoMerger.merge(repo, rw, ins, b, mergeStrategy); + } return null; } } diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index 9fa562aba6..435007289a 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -451,6 +451,57 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(diff.metaB.lines).isEqualTo(1); } + @Test + public void diffWithThreeParentsMergeCommitChange() throws Exception { + // Create a merge commit of 3 files: foo, bar, baz. The merge commit is pointing to 3 different + // parents: the merge commit contains foo of parent1, bar of parent2 and baz of parent3. + PushOneCommit.Result r = + createNParentsMergeCommitChange("refs/for/master", ImmutableList.of("foo", "bar", "baz")); + + DiffInfo diff; + + // parent 1 + Map changedFiles = gApi.changes().id(r.getChangeId()).current().files(1); + assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, MERGE_LIST, "bar", "baz"); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "foo").withParent(1).get(); + assertThat(diff.diffHeader).isNull(); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "bar").withParent(1).get(); + assertThat(diff.diffHeader).hasSize(4); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "baz").withParent(1).get(); + assertThat(diff.diffHeader).hasSize(4); + + // parent 2 + changedFiles = gApi.changes().id(r.getChangeId()).current().files(2); + assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, MERGE_LIST, "foo", "baz"); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "foo").withParent(2).get(); + assertThat(diff.diffHeader).hasSize(4); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "bar").withParent(2).get(); + assertThat(diff.diffHeader).isNull(); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "baz").withParent(2).get(); + assertThat(diff.diffHeader).hasSize(4); + + // parent 3 + changedFiles = gApi.changes().id(r.getChangeId()).current().files(3); + assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, MERGE_LIST, "foo", "bar"); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "foo").withParent(3).get(); + assertThat(diff.diffHeader).hasSize(4); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "bar").withParent(3).get(); + assertThat(diff.diffHeader).hasSize(4); + diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "baz").withParent(3).get(); + assertThat(diff.diffHeader).isNull(); + } + + @Test + public void diffWithThreeParentsMergeCommitAgainstAutoMergeIsNotSupported() throws Exception { + PushOneCommit.Result r = + createNParentsMergeCommitChange("refs/for/master", ImmutableList.of("foo", "bar", "baz")); + + // Diff against auto-merge returns COMMIT_MSG and MERGE_LIST only + // todo(ghareeb): We could throw an exception in this case for better handling at the client. + Map changedFiles = gApi.changes().id(r.getChangeId()).current().files(); + assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, MERGE_LIST); + } + @Test public void diffBetweenPatchSetsOfMergeCommitCanBeRetrievedForCommitMessageAndMergeList() throws Exception {