Merge changes I06f5811b,I488cd376
* changes:
Surface comparison type and old/new commit IDs in the file diff output
Convert ComparisonType to AutoValue
diff --git a/java/com/google/gerrit/server/patch/ComparisonType.java b/java/com/google/gerrit/server/patch/ComparisonType.java
index 260c507..eca2658 100644
--- a/java/com/google/gerrit/server/patch/ComparisonType.java
+++ b/java/com/google/gerrit/server/patch/ComparisonType.java
@@ -16,34 +16,40 @@
import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32;
-import static java.util.Objects.requireNonNull;
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.util.Optional;
-public class ComparisonType {
+/** Relation between the old and new commits used in the diff. */
+@AutoValue
+public abstract class ComparisonType {
- /** 1-based parent */
- private final Integer parentNum;
+ /**
+ * 1-based parent. Available if the old commit is the parent of the new commit and old commit is
+ * not the auto-merge.
+ */
+ abstract Optional<Integer> parentNum();
- private final boolean autoMerge;
+ abstract boolean autoMerge();
public static ComparisonType againstOtherPatchSet() {
- return new ComparisonType(null, false);
+ return new AutoValue_ComparisonType(Optional.empty(), false);
}
public static ComparisonType againstParent(int parentNum) {
- return new ComparisonType(parentNum, false);
+ return new AutoValue_ComparisonType(Optional.of(parentNum), false);
}
public static ComparisonType againstAutoMerge() {
- return new ComparisonType(null, true);
+ return new AutoValue_ComparisonType(Optional.empty(), true);
}
- private ComparisonType(Integer parentNum, boolean autoMerge) {
- this.parentNum = parentNum;
- this.autoMerge = autoMerge;
+ private static ComparisonType create(Optional<Integer> parent, boolean automerge) {
+ return new AutoValue_ComparisonType(parent, automerge);
}
public boolean isAgainstParentOrAutoMerge() {
@@ -51,27 +57,43 @@
}
public boolean isAgainstParent() {
- return parentNum != null;
+ return parentNum().isPresent();
}
public boolean isAgainstAutoMerge() {
- return autoMerge;
+ return autoMerge();
}
- public int getParentNum() {
- requireNonNull(parentNum);
- return parentNum;
+ public Optional<Integer> getParentNum() {
+ return parentNum();
}
void writeTo(OutputStream out) throws IOException {
- writeVarInt32(out, parentNum != null ? parentNum : 0);
- writeVarInt32(out, autoMerge ? 1 : 0);
+ writeVarInt32(out, isAgainstParent() ? parentNum().get() : 0);
+ writeVarInt32(out, autoMerge() ? 1 : 0);
}
static ComparisonType readFrom(InputStream in) throws IOException {
int p = readVarInt32(in);
- Integer parentNum = p > 0 ? p : null;
+ Optional<Integer> parentNum = p > 0 ? Optional.of(p) : Optional.empty();
boolean autoMerge = readVarInt32(in) != 0;
- return new ComparisonType(parentNum, autoMerge);
+ return create(parentNum, autoMerge);
+ }
+
+ public FileDiffOutputProto.ComparisonType toProto() {
+ FileDiffOutputProto.ComparisonType.Builder builder =
+ FileDiffOutputProto.ComparisonType.newBuilder().setAutoMerge(autoMerge());
+ if (parentNum().isPresent()) {
+ builder.setParentNum(parentNum().get());
+ }
+ return builder.build();
+ }
+
+ public static ComparisonType fromProto(FileDiffOutputProto.ComparisonType proto) {
+ Optional<Integer> parentNum = Optional.empty();
+ if (proto.hasField(FileDiffOutputProto.ComparisonType.getDescriptor().findFieldByNumber(1))) {
+ parentNum = Optional.of(proto.getParentNum());
+ }
+ return create(parentNum, proto.getAutoMerge());
}
}
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index 8b90531..93aefff 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -74,8 +74,9 @@
/**
* Returns the diff for a single file between a patchset commit against its parent or the
* auto-merge commit. For deleted files, the {@code fileName} parameter should contain the old
- * name of the file. This method will return {@link FileDiffOutput#empty(String)} if the requested
- * file identified by {@code fileName} has unchanged content or does not exist at both commits.
+ * name of the file. This method will return {@link FileDiffOutput#empty(String, ObjectId,
+ * ObjectId)} if the requested file identified by {@code fileName} has unchanged content or does
+ * not exist at both commits.
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
@@ -98,8 +99,8 @@
/**
* Returns the diff for a single file between two patchset commits. For deleted files, the {@code
* fileName} parameter should contain the old name of the file. This method will return {@link
- * FileDiffOutput#empty(String)} if the requested file identified by {@code fileName} has
- * unchanged content or does not exist at both commits.
+ * FileDiffOutput#empty(String, ObjectId, ObjectId)} if the requested file identified by {@code
+ * fileName} has unchanged content or does not exist at both commits.
*
* @param project a project name representing a git repository.
* @param oldCommit 20 bytes SHA-1 of the old commit used in the diff.
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 16bd135..efb64bc 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -125,7 +125,9 @@
FileDiffCacheKey key =
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName, whitespace);
Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
- return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
+ return result.containsKey(fileName)
+ ? result.get(fileName)
+ : FileDiffOutput.empty(fileName, key.oldCommit(), key.newCommit());
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -143,7 +145,9 @@
FileDiffCacheKey key =
createFileDiffCacheKey(project, oldCommit, newCommit, fileName, whitespace);
Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
- return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
+ return result.containsKey(fileName)
+ ? result.get(fileName)
+ : FileDiffOutput.empty(fileName, oldCommit, newCommit);
}
private Map<String, FileDiffOutput> getModifiedFiles(
diff --git a/java/com/google/gerrit/server/patch/MagicFile.java b/java/com/google/gerrit/server/patch/MagicFile.java
index aa6b11f..e42dd8c 100644
--- a/java/com/google/gerrit/server/patch/MagicFile.java
+++ b/java/com/google/gerrit/server/patch/MagicFile.java
@@ -93,7 +93,7 @@
}
default:
int uninterestingParent =
- comparisonType.isAgainstParent() ? comparisonType.getParentNum() : 1;
+ comparisonType.isAgainstParent() ? comparisonType.getParentNum().get() : 1;
b.append("Merge List:\n\n");
for (RevCommit commit : MergeListBuilder.build(rw, c, uninterestingParent)) {
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index 63f311b..1bb407d 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -93,6 +93,7 @@
persist(DIFF, FileDiffCacheKey.class, FileDiffOutput.class)
.maximumWeight(10 << 20)
.weigher(FileDiffWeigher.class)
+ .version(2)
.keySerializer(FileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(FileDiffOutput.Serializer.INSTANCE)
.loader(FileDiffLoader.class);
@@ -219,19 +220,16 @@
RawTextComparator cmp = comparatorFor(key.whitespace());
ComparisonType comparisonType =
getComparisonType(rw, reader, key.oldCommit(), key.newCommit());
- RevCommit aCommit =
- comparisonType.isAgainstParentOrAutoMerge()
- ? null
- : DiffUtil.getRevCommit(rw, key.oldCommit());
+ RevCommit aCommit = DiffUtil.getRevCommit(rw, key.oldCommit());
RevCommit bCommit = DiffUtil.getRevCommit(rw, key.newCommit());
return magicPath == MagicPath.COMMIT
- ? createCommitEntry(reader, aCommit, bCommit, cmp, key.diffAlgorithm())
+ ? createCommitEntry(reader, aCommit, bCommit, comparisonType, cmp, key.diffAlgorithm())
: createMergeListEntry(
reader, aCommit, bCommit, comparisonType, cmp, key.diffAlgorithm());
} catch (IOException e) {
logger.atWarning().log("Failed to compute commit entry for key %s", key);
}
- return FileDiffOutput.empty(key.newFilePath());
+ return FileDiffOutput.empty(key.newFilePath(), key.oldCommit(), key.newCommit());
}
private static RawTextComparator comparatorFor(Whitespace ws) {
@@ -255,13 +253,24 @@
ObjectReader reader,
RevCommit oldCommit,
RevCommit newCommit,
+ ComparisonType comparisonType,
RawTextComparator rawTextComparator,
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm)
throws IOException {
- Text aText = oldCommit != null ? Text.forCommit(reader, oldCommit) : Text.EMPTY;
+ Text aText =
+ comparisonType.isAgainstParentOrAutoMerge()
+ ? Text.EMPTY
+ : Text.forCommit(reader, oldCommit);
Text bText = Text.forCommit(reader, newCommit);
return createMagicFileDiffOutput(
- rawTextComparator, oldCommit, aText, bText, Patch.COMMIT_MSG, diffAlgorithm);
+ oldCommit,
+ newCommit,
+ comparisonType,
+ rawTextComparator,
+ aText,
+ bText,
+ Patch.COMMIT_MSG,
+ diffAlgorithm);
}
private FileDiffOutput createMergeListEntry(
@@ -273,20 +282,31 @@
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm)
throws IOException {
Text aText =
- oldCommit != null ? Text.forMergeList(comparisonType, reader, oldCommit) : Text.EMPTY;
+ comparisonType.isAgainstParentOrAutoMerge()
+ ? Text.EMPTY
+ : Text.forMergeList(comparisonType, reader, oldCommit);
Text bText = Text.forMergeList(comparisonType, reader, newCommit);
return createMagicFileDiffOutput(
- rawTextComparator, oldCommit, aText, bText, Patch.MERGE_LIST, diffAlgorithm);
+ oldCommit,
+ newCommit,
+ comparisonType,
+ rawTextComparator,
+ aText,
+ bText,
+ Patch.MERGE_LIST,
+ diffAlgorithm);
}
private static FileDiffOutput createMagicFileDiffOutput(
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ ComparisonType comparisonType,
RawTextComparator rawTextComparator,
- RevCommit aCommit,
Text aText,
Text bText,
String fileName,
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm) {
- byte[] rawHdr = getRawHeader(aCommit != null, fileName);
+ byte[] rawHdr = getRawHeader(!comparisonType.isAgainstParentOrAutoMerge(), fileName);
byte[] aContent = aText.getContent();
byte[] bContent = bText.getContent();
long size = bContent.length;
@@ -298,6 +318,9 @@
FileHeader fileHeader = new FileHeader(rawHdr, edits, PatchType.UNIFIED);
Patch.ChangeType changeType = FileHeaderUtil.getChangeType(fileHeader);
return FileDiffOutput.builder()
+ .oldCommitId(oldCommit)
+ .newCommitId(newCommit)
+ .comparisonType(comparisonType)
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
.newPath(FileHeaderUtil.getNewPath(fileHeader))
.changeType(changeType)
@@ -370,8 +393,13 @@
mainGitDiff.newPath().get())
: 0;
+ ObjectId oldCommit = augmentedKey.key().oldCommit();
+ ObjectId newCommit = augmentedKey.key().newCommit();
FileDiffOutput fileDiff =
FileDiffOutput.builder()
+ .oldCommitId(oldCommit)
+ .newCommitId(newCommit)
+ .comparisonType(getComparisonType(rw, reader, oldCommit, newCommit))
.changeType(mainGitDiff.changeType())
.patchType(mainGitDiff.patchType())
.oldPath(mainGitDiff.oldPath())
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index 3348033..e7f47ef 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -24,16 +24,28 @@
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
+import com.google.gerrit.server.patch.ComparisonType;
import com.google.protobuf.Descriptors.FieldDescriptor;
import java.io.Serializable;
import java.util.Optional;
import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
/** File diff for a single file path. Produced as output of the {@link FileDiffCache}. */
@AutoValue
public abstract class FileDiffOutput implements Serializable {
private static final long serialVersionUID = 1L;
+ /** The 20 bytes SHA-1 object ID of the old git commit used in the diff. */
+ public abstract ObjectId oldCommitId();
+
+ /** The 20 bytes SHA-1 object ID of the new git commit used in the diff. */
+ public abstract ObjectId newCommitId();
+
+ /** Comparison type of old and new commits: against another patchset, parent or auto-merge. */
+ public abstract ComparisonType comparisonType();
+
/**
* The file path at the old commit. Returns an empty Optional if {@link #changeType()} is equal to
* {@link ChangeType#ADDED}.
@@ -95,8 +107,11 @@
}
/** Returns an entity representing an unchanged file between two commits. */
- public static FileDiffOutput empty(String filePath) {
+ public static FileDiffOutput empty(String filePath, ObjectId oldCommitId, ObjectId newCommitId) {
return builder()
+ .oldCommitId(oldCommitId)
+ .newCommitId(newCommitId)
+ .comparisonType(ComparisonType.againstOtherPatchSet()) // not important
.oldPath(Optional.empty())
.newPath(Optional.of(filePath))
.changeType(ChangeType.MODIFIED)
@@ -124,6 +139,8 @@
if (newPath().isPresent()) {
result += stringSize(newPath().get());
}
+ result += 20 + 20; // old and new commit IDs
+ result += 4; // comparison type
result += 4; // changeType
if (patchType().isPresent()) {
result += 4;
@@ -140,6 +157,12 @@
@AutoValue.Builder
public abstract static class Builder {
+ public abstract Builder oldCommitId(ObjectId value);
+
+ public abstract Builder newCommitId(ObjectId value);
+
+ public abstract Builder comparisonType(ComparisonType value);
+
public abstract Builder oldPath(Optional<String> value);
public abstract Builder newPath(Optional<String> value);
@@ -173,8 +196,12 @@
@Override
public byte[] serialize(FileDiffOutput fileDiff) {
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
FileDiffOutputProto.Builder builder =
FileDiffOutputProto.newBuilder()
+ .setOldCommit(idConverter.toByteString(fileDiff.oldCommitId().toObjectId()))
+ .setNewCommit(idConverter.toByteString(fileDiff.newCommitId().toObjectId()))
+ .setComparisonType(fileDiff.comparisonType().toProto())
.setSize(fileDiff.size())
.setSizeDelta(fileDiff.sizeDelta())
.addAllHeaderLines(fileDiff.headerLines())
@@ -212,9 +239,13 @@
@Override
public FileDiffOutput deserialize(byte[] in) {
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
FileDiffOutputProto proto = Protos.parseUnchecked(FileDiffOutputProto.parser(), in);
FileDiffOutput.Builder builder = FileDiffOutput.builder();
builder
+ .oldCommitId(idConverter.fromByteString(proto.getOldCommit()))
+ .newCommitId(idConverter.fromByteString(proto.getNewCommit()))
+ .comparisonType(ComparisonType.fromProto(proto.getComparisonType()))
.size(proto.getSize())
.sizeDelta(proto.getSizeDelta())
.headerLines(proto.getHeaderLinesList().stream().collect(ImmutableList.toImmutableList()))
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
index 44ea55a..17fd959 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
@@ -19,10 +19,12 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Patch.PatchType;
+import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.patch.filediff.TaggedEdit;
import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class FileDiffOutputSerializerTest {
@@ -35,6 +37,9 @@
FileDiffOutput fileDiff =
FileDiffOutput.builder()
+ .oldCommitId(ObjectId.fromString("dd4d2a1498870ca5fe415b33f65d052d69d9eaf5"))
+ .newCommitId(ObjectId.fromString("0cfaab3f2ba76f71798da0a2651f41be8d45f842"))
+ .comparisonType(ComparisonType.againstOtherPatchSet())
.oldPath(Optional.of("old_file_path.txt"))
.newPath(Optional.empty())
.changeType(ChangeType.DELETED)
diff --git a/proto/cache.proto b/proto/cache.proto
index 4fd037d..a610e49 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -617,7 +617,7 @@
// Serialized form of
// com.google.gerrit.server.patch.filediff.FileDiffOutput
-// Next ID: 9
+// Next ID: 12
message FileDiffOutputProto {
// Next ID: 5
message Edit {
@@ -632,6 +632,11 @@
Edit edit = 1;
bool due_to_rebase = 2;
}
+ // Next ID: 3
+ message ComparisonType {
+ int32 parent_num = 1;
+ bool auto_merge = 2;
+ }
string old_path = 1;
string new_path = 2;
string change_type = 3;
@@ -640,4 +645,7 @@
int64 size = 6;
int64 size_delta = 7;
repeated TaggedEdit edits = 8;
+ bytes old_commit = 9;
+ bytes new_commit = 10;
+ ComparisonType comparison_type = 11;
}
