opendev/gerrit
Code Issues Proposed changes

Merge changes I45ee1aaf,I6c5574ed

Browse Source 
* changes:
  Convert ChangeType to non-optional in FileDiffOutput and GitFileDiff
  Add the whitespace parameter to the DiffOperations interface methods
This commit is contained in:
Youssef Elghareeb
2021-02-15 16:11:34 +00:00
committed by Gerrit Code Review
parent 0d61f20ed3 d016ec3be5
commit b7136548e3
8 changed files with 67 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
View File

@@ -89,8 +89,8 @@ public class FileInfoJsonNewImpl implements FileInfoJson {
FileDiffOutput fileDiff = fileDiffs.get(path);
FileInfo fileInfo = new FileInfo();
fileInfo.status =
fileDiff.changeType().get() != Patch.ChangeType.MODIFIED
? fileDiff.changeType().get().getCode()
fileDiff.changeType() != Patch.ChangeType.MODIFIED
? fileDiff.changeType().getCode()
: null;
fileInfo.oldPath = fileDiff.oldPath().orElse(null);
fileInfo.sizeDelta = fileDiff.sizeDelta();

15
java/com/google/gerrit/server/patch/DiffOperations.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.patch;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import java.util.Map;
import org.eclipse.jgit.lib.ObjectId;
@@ -80,12 +81,17 @@ public interface DiffOperations {
* @param parentNum integer specifying which parent to use as base. If null, the only parent will
* be used or the auto-merge if {@code newCommit} is a merge commit.
* @param fileName the file name for which the diff should be evaluated.
* @param whitespace preference controlling whitespace effect in diff computation.
* @return the diff for the single file between the two commits.
* @throws DiffNotAvailableException if an internal error occurred in Git while evaluating the
* diff, or if an exception happened while parsing the base commit.
*/
FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum, String fileName)
Project.NameKey project,
ObjectId newCommit,
@Nullable Integer parentNum,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException;
/**
@@ -96,11 +102,16 @@ public interface DiffOperations {
* @param oldCommit 20 bytes SHA-1 of the old commit used in the diff.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
* @param fileName the file name for which the diff should be evaluated.
* @param whitespace preference controlling whitespace effect in diff computation.
* @return the diff for the single file between the two commits.
* @throws DiffNotAvailableException if an internal error occurred in Git while evaluating the
* diff.
*/
FileDiffOutput getModifiedFile(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, String fileName)
Project.NameKey project,
ObjectId oldCommit,
ObjectId newCommit,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException;
}

45
java/com/google/gerrit/server/patch/DiffOperationsImpl.java
View File

@@ -25,6 +25,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.patch.diff.ModifiedFilesCache;
@@ -55,6 +56,7 @@ public class DiffOperationsImpl implements DiffOperations {
private static final int RENAME_SCORE = 60;
private static final DiffAlgorithm DEFAULT_DIFF_ALGORITHM = DiffAlgorithm.HISTOGRAM;
private static final Whitespace DEFAULT_WHITESPACE = Whitespace.IGNORE_NONE;
private final ModifiedFilesCache modifiedFilesCache;
private final FileDiffCache fileDiffCache;
@@ -112,12 +114,16 @@ public class DiffOperationsImpl implements DiffOperations {
@Override
public FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project, ObjectId newCommit, @Nullable Integer parent, String fileName)
Project.NameKey project,
ObjectId newCommit,
@Nullable Integer parent,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException {
try {
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
FileDiffCacheKey key =
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName);
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName, whitespace);
return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
} catch (IOException e) {
throw new DiffNotAvailableException(
@@ -127,9 +133,14 @@ public class DiffOperationsImpl implements DiffOperations {
@Override
public FileDiffOutput getModifiedFile(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, String fileName)
Project.NameKey project,
ObjectId oldCommit,
ObjectId newCommit,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException {
FileDiffCacheKey key = createFileDiffCacheKey(project, oldCommit, newCommit, fileName);
FileDiffCacheKey key =
createFileDiffCacheKey(project, oldCommit, newCommit, fileName, whitespace);
return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
}
@@ -144,10 +155,14 @@ public class DiffOperationsImpl implements DiffOperations {
modifiedFilesCache.get(createModifiedFilesKey(project, oldCommit, newCommit));
List<FileDiffCacheKey> fileCacheKeys = new ArrayList<>();
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, COMMIT_MSG));
fileCacheKeys.add(
createFileDiffCacheKey(
project, oldCommit, newCommit, COMMIT_MSG, /* whitespace= */ null));
if (cmp.isAgainstAutoMerge() || isMergeAgainstParent(cmp, project, newCommit)) {
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, MERGE_LIST));
fileCacheKeys.add(
createFileDiffCacheKey(
project, oldCommit, newCommit, MERGE_LIST, /*whitespace = */ null));
}
if (diffParams.skipFiles() == null) {
@@ -160,7 +175,8 @@ public class DiffOperationsImpl implements DiffOperations {
newCommit,
entity.newPath().isPresent()
? entity.newPath().get()
: entity.oldPath().get()))
: entity.oldPath().get(),
/* whitespace= */ null))
.forEach(fileCacheKeys::add);
}
return getModifiedFilesForKeys(fileCacheKeys);
@@ -178,7 +194,7 @@ public class DiffOperationsImpl implements DiffOperations {
if (fileDiffOutput.isEmpty() || allDueToRebase(fileDiffOutput)) {
continue;
}
if (fileDiffOutput.changeType().get() == Patch.ChangeType.DELETED) {
if (fileDiffOutput.changeType() == ChangeType.DELETED) {
files.put(fileDiffOutput.oldPath().get(), fileDiffOutput);
} else {
files.put(fileDiffOutput.newPath().get(), fileDiffOutput);
@@ -189,8 +205,8 @@ public class DiffOperationsImpl implements DiffOperations {
private static boolean allDueToRebase(FileDiffOutput fileDiffOutput) {
return fileDiffOutput.allEditsDueToRebase()
&& (!(fileDiffOutput.changeType().get() == ChangeType.RENAMED
|| fileDiffOutput.changeType().get() == ChangeType.COPIED));
&& (!(fileDiffOutput.changeType() == ChangeType.RENAMED
|| fileDiffOutput.changeType() == ChangeType.COPIED));
}
private boolean isMergeAgainstParent(ComparisonType cmp, Project.NameKey project, ObjectId commit)
@@ -209,7 +225,12 @@ public class DiffOperationsImpl implements DiffOperations {
}
private static FileDiffCacheKey createFileDiffCacheKey(
Project.NameKey project, ObjectId aCommit, ObjectId bCommit, String newPath) {
Project.NameKey project,
ObjectId aCommit,
ObjectId bCommit,
String newPath,
@Nullable Whitespace whitespace) {
whitespace = whitespace == null ? DEFAULT_WHITESPACE : whitespace;
return FileDiffCacheKey.builder()
.project(project)
.oldCommit(aCommit)
@@ -217,7 +238,7 @@ public class DiffOperationsImpl implements DiffOperations {
.newFilePath(newPath)
.renameScore(RENAME_SCORE)
.diffAlgorithm(DEFAULT_DIFF_ALGORITHM)
.whitespace(Whitespace.IGNORE_NONE)
.whitespace(whitespace)
.build();
}

2
java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
View File

@@ -288,7 +288,7 @@ public class FileDiffCacheImpl implements FileDiffCache {
return FileDiffOutput.builder()
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
.newPath(FileHeaderUtil.getNewPath(fileHeader))
.changeType(Optional.of(changeType))
.changeType(changeType)
.patchType(Optional.of(FileHeaderUtil.getPatchType(fileHeader)))
.headerLines(FileHeaderUtil.getHeaderLines(fileHeader))
.edits(

23
java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
View File

@@ -47,7 +47,7 @@ public abstract class FileDiffOutput implements Serializable {
public abstract Optional<String> newPath();
/** The change type of the underlying file, e.g. added, deleted, renamed, etc... */
public abstract Optional<Patch.ChangeType> changeType();
public abstract Patch.ChangeType changeType();
/** The patch type of the underlying file, e.g. unified, binary , etc... */
public abstract Optional<Patch.PatchType> patchType();
@@ -95,10 +95,11 @@ public abstract class FileDiffOutput implements Serializable {
}
/** Returns an entity representing an unchanged file between two commits. */
static FileDiffOutput empty(String filePath) {
public static FileDiffOutput empty(String filePath) {
return builder()
.oldPath(Optional.empty())
.newPath(Optional.of(filePath))
.changeType(ChangeType.MODIFIED)
.headerLines(ImmutableList.of())
.edits(ImmutableList.of())
.size(0)
@@ -123,9 +124,7 @@ public abstract class FileDiffOutput implements Serializable {
if (newPath().isPresent()) {
result += stringSize(newPath().get());
}
if (changeType().isPresent()) {
result += 4;
}
result += 4; // changeType
if (patchType().isPresent()) {
result += 4;
}
@@ -145,7 +144,7 @@ public abstract class FileDiffOutput implements Serializable {
public abstract Builder newPath(Optional<String> value);
public abstract Builder changeType(Optional<ChangeType> value);
public abstract Builder changeType(ChangeType value);
public abstract Builder patchType(Optional<PatchType> value);
@@ -169,9 +168,6 @@ public abstract class FileDiffOutput implements Serializable {
private static final FieldDescriptor NEW_PATH_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(2);
private static final FieldDescriptor CHANGE_TYPE_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(3);
private static final FieldDescriptor PATCH_TYPE_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(4);
@@ -182,6 +178,7 @@ public abstract class FileDiffOutput implements Serializable {
.setSize(fileDiff.size())
.setSizeDelta(fileDiff.sizeDelta())
.addAllHeaderLines(fileDiff.headerLines())
.setChangeType(fileDiff.changeType().name())
.addAllEdits(
fileDiff.edits().stream()
.map(
@@ -206,10 +203,6 @@ public abstract class FileDiffOutput implements Serializable {
builder.setNewPath(fileDiff.newPath().get());
}
if (fileDiff.changeType().isPresent()) {
builder.setChangeType(fileDiff.changeType().get().name());
}
if (fileDiff.patchType().isPresent()) {
builder.setPatchType(fileDiff.patchType().get().name());
}
@@ -225,6 +218,7 @@ public abstract class FileDiffOutput implements Serializable {
.size(proto.getSize())
.sizeDelta(proto.getSizeDelta())
.headerLines(proto.getHeaderLinesList().stream().collect(ImmutableList.toImmutableList()))
.changeType(ChangeType.valueOf(proto.getChangeType()))
.edits(
proto.getEditsList().stream()
.map(
@@ -244,9 +238,6 @@ public abstract class FileDiffOutput implements Serializable {
if (proto.hasField(NEW_PATH_DESCRIPTOR)) {
builder.newPath(Optional.of(proto.getNewPath()));
}
if (proto.hasField(CHANGE_TYPE_DESCRIPTOR)) {
builder.changeType(Optional.of(Patch.ChangeType.valueOf(proto.getChangeType())));
}
if (proto.hasField(PATCH_TYPE_DESCRIPTOR)) {
builder.patchType(Optional.of(Patch.PatchType.valueOf(proto.getPatchType())));
}

23
java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
View File

@@ -77,7 +77,7 @@ public abstract class GitFileDiff {
.fileHeader(FileHeaderUtil.toString(fileHeader))
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
.newPath(FileHeaderUtil.getNewPath(fileHeader))
.changeType(Optional.of(FileHeaderUtil.getChangeType(fileHeader)))
.changeType(FileHeaderUtil.getChangeType(fileHeader))
.patchType(Optional.of(FileHeaderUtil.getPatchType(fileHeader)))
.oldMode(Optional.of(mapFileMode(diffEntry.getOldMode())))
.newMode(Optional.of(mapFileMode(diffEntry.getNewMode())))
@@ -96,6 +96,7 @@ public abstract class GitFileDiff {
.oldId(oldId)
.newId(newId)
.newPath(Optional.of(newFilePath))
.changeType(ChangeType.MODIFIED)
.edits(ImmutableList.of())
.fileHeader("")
.build();
@@ -126,7 +127,7 @@ public abstract class GitFileDiff {
public abstract Optional<Patch.FileMode> newMode();
/** The change type associated with the file. */
public abstract Optional<ChangeType> changeType();
public abstract ChangeType changeType();
/** The patch type associated with the file. */
public abstract Optional<PatchType> patchType();
@@ -150,9 +151,7 @@ public abstract class GitFileDiff {
if (newPath().isPresent()) {
result += stringSize(newPath().get());
}
if (changeType().isPresent()) {
result += 4;
}
result += 4;
if (patchType().isPresent()) {
result += 4;
}
@@ -188,7 +187,7 @@ public abstract class GitFileDiff {
public abstract Builder newMode(Optional<Patch.FileMode> value);
public abstract Builder changeType(Optional<ChangeType> value);
public abstract Builder changeType(ChangeType value);
public abstract Builder patchType(Optional<PatchType> value);
@@ -223,7 +222,8 @@ public abstract class GitFileDiff {
GitFileDiffProto.newBuilder()
.setFileHeader(gitFileDiff.fileHeader())
.setOldId(idConverter.toByteString(gitFileDiff.oldId().toObjectId()))
.setNewId(idConverter.toByteString(gitFileDiff.newId().toObjectId()));
.setNewId(idConverter.toByteString(gitFileDiff.newId().toObjectId()))
.setChangeType(gitFileDiff.changeType().name());
gitFileDiff
.edits()
.forEach(
@@ -246,9 +246,6 @@ public abstract class GitFileDiff {
if (gitFileDiff.newMode().isPresent()) {
builder.setNewMode(gitFileDiff.newMode().get().name());
}
if (gitFileDiff.changeType().isPresent()) {
builder.setChangeType(gitFileDiff.changeType().get().name());
}
if (gitFileDiff.patchType().isPresent()) {
builder.setPatchType(gitFileDiff.patchType().get().name());
}
@@ -267,7 +264,8 @@ public abstract class GitFileDiff {
.collect(toImmutableList()))
.fileHeader(proto.getFileHeader())
.oldId(AbbreviatedObjectId.fromObjectId(idConverter.fromByteString(proto.getOldId())))
.newId(AbbreviatedObjectId.fromObjectId(idConverter.fromByteString(proto.getNewId())));
.newId(AbbreviatedObjectId.fromObjectId(idConverter.fromByteString(proto.getNewId())))
.changeType(ChangeType.valueOf(proto.getChangeType()));
if (proto.hasField(OLD_PATH_DESCRIPTOR)) {
builder.oldPath(Optional.of(proto.getOldPath()));
@@ -281,9 +279,6 @@ public abstract class GitFileDiff {
if (proto.hasField(NEW_MODE_DESCRIPTOR)) {
builder.newMode(Optional.of(Patch.FileMode.valueOf(proto.getNewMode())));
}
if (proto.hasField(CHANGE_TYPE_DESCRIPTOR)) {
builder.changeType(Optional.of(Patch.ChangeType.valueOf(proto.getChangeType())));
}
if (proto.hasField(PATCH_TYPE_DESCRIPTOR)) {
builder.patchType(Optional.of(Patch.PatchType.valueOf(proto.getPatchType())));
}

2
javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java vendored
View File

@@ -37,7 +37,7 @@ public class FileDiffOutputSerializerTest {
FileDiffOutput.builder()
.oldPath(Optional.of("old_file_path.txt"))
.newPath(Optional.empty())
.changeType(Optional.of(ChangeType.DELETED))
.changeType(ChangeType.DELETED)
.patchType(Optional.of(PatchType.UNIFIED))
.size(23)
.sizeDelta(10)

2
javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffSerializerTest.java vendored
View File

@@ -47,7 +47,7 @@ public class GitFileDiffSerializerTest {
.newPath(Optional.empty())
.oldId(AbbreviatedObjectId.fromObjectId(OLD_ID))
.newId(AbbreviatedObjectId.fromObjectId(NEW_ID))
.changeType(Optional.of(ChangeType.DELETED))
.changeType(ChangeType.DELETED)
.patchType(Optional.of(PatchType.UNIFIED))
.oldMode(Optional.of(FileMode.REGULAR_FILE))
.newMode(Optional.of(FileMode.REGULAR_FILE))