opendev/gerrit
Code Issues Proposed changes

Include a magic /MERGE_LIST file for merge commits

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-09-13 12:33:08 +02:00
parent b5b60f3aed
commit c6ea7bb81c
27 changed files with 610 additions and 205 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -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))

4
gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java
View File

@@ -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);

26
gerrit-server/src/main/java/com/google/gerrit/server/change/GetContent.java
View File

@@ -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<FileResource> {
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<FileResource> {
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);
}
}
}

33
gerrit-server/src/main/java/com/google/gerrit/server/change/GetMergeList.java
View File

@@ -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<RevisionResource> {
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<RevisionResource> {
@Override
public Response<List<CommitInfo>> apply(RevisionResource rsrc)
throws BadRequestException, IOException {
List<CommitInfo> 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<RevisionResource> {
return Response.<List<CommitInfo>> ok(ImmutableList.<CommitInfo> 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<RevCommit> commits =
MergeListBuilder.build(rw, commit, uninterestingParent);
List<CommitInfo> 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<List<CommitInfo>> r = Response.ok(result);
if (rsrc.isCacheable()) {
r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
Response<List<CommitInfo>> r = Response.ok(result);
if (rsrc.isCacheable()) {
r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
}
return r;
}
return r;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java
View File

@@ -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",

12
gerrit-server/src/main/java/com/google/gerrit/server/patch/ComparisonType.java
View File

@@ -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);

52
gerrit-server/src/main/java/com/google/gerrit/server/patch/MergeListBuilder.java Normal file
View File

@@ -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<RevCommit> 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<RevCommit> result = new ArrayList<>();
RevCommit c;
while ((c = rw.next()) != null) {
result.add(c);
}
return result;
}
}

14
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java
View File

@@ -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());

12
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java
View File

@@ -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);

6
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java
View File

@@ -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, Character> 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();

81
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@@ -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<PatchList> {
// 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<PatchList> {
List<PatchListEntry> 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<PatchList> {
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<PatchList> {
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<PatchList> {
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

24
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
View File

@@ -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);

30
gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java
View File

@@ -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) {

20
gerrit-server/src/main/java/gerrit/PRED_commit_stats_3.java
View File

@@ -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<PatchListEntry> entries) {
int count = 0;
for (PatchListEntry e : entries) {
if (Patch.isMagic(e.getNewName())) {
count++;
}
}
return count;
}
}