opendev/gerrit
Code Issues Proposed changes

Support diffing for octopus merges against a specific parent

Browse Source 
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
This commit is contained in:
Youssef Elghareeb
2020-11-06 14:25:43 +01:00
parent 5626c6ca1b
commit 27ce6452a9
3 changed files with 110 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -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<String> 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<PushOneCommit.Result> 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<InMemoryRepository> repo,
String ref,

9
java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@@ -611,15 +611,16 @@ public class PatchListLoader implements Callable<PatchList> {
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;
}
}

51
javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
View File

@@ -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<String, FileInfo> 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<String, FileInfo> changedFiles = gApi.changes().id(r.getChangeId()).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, MERGE_LIST);
}
@Test
public void diffBetweenPatchSetsOfMergeCommitCanBeRetrievedForCommitMessageAndMergeList()
throws Exception {