opendev/gerrit
Code Issues Proposed changes

Add single file diff to the DiffOperations interface

Browse Source 
Adding the diff for a single file to the DiffOperations interface. Also
renamed the existing methods for getting the list of modified files to
"listModifiedFiles" for clarity. The code for computing the base commit
for diffs against parent or auto-merge is also extracted because it's
used by the listModifiedFiles and the getModifiedFile methods.

Change-Id: I23f616d1c89f7ddad8e8fd0f526394475099d157
This commit is contained in:
Youssef Elghareeb
2021-02-01 13:27:27 +01:00
parent 5a2fc8289d
commit b34ed026f2
4 changed files with 181 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -47,10 +47,9 @@ public class FileInfoJsonNewImpl implements FileInfoJson {
try {
if (base == null) {
return asFileInfo(
diffs.getModifiedFilesAgainstParentOrAutoMerge(change.getProject(), objectId, null));
diffs.listModifiedFilesAgainstParent(change.getProject(), objectId, null));
} else {
return asFileInfo(
diffs.getModifiedFilesBetweenPatchsets(change.getProject(), base.commitId(), objectId));
return asFileInfo(diffs.listModifiedFiles(change.getProject(), base.commitId(), objectId));
}
} catch (DiffNotAvailableException e) {
convertException(e);
@@ -64,7 +63,7 @@ public class FileInfoJsonNewImpl implements FileInfoJson {
throws ResourceConflictException, PatchListNotAvailableException {
try {
Map<String, FileDiffOutput> modifiedFiles =
diffs.getModifiedFilesAgainstParentOrAutoMerge(project, objectId, parent + 1);
diffs.listModifiedFilesAgainstParent(project, objectId, parent + 1);
return asFileInfo(modifiedFiles);
} catch (DiffNotAvailableException e) {
convertException(e);

4
java/com/google/gerrit/server/patch/DiffNotAvailableException.java
View File

@@ -29,4 +29,8 @@ public class DiffNotAvailableException extends Exception {
public DiffNotAvailableException(String message) {
super(message);
}
public DiffNotAvailableException(String message, Throwable cause) {
super(message, cause);
}
}

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

@@ -27,7 +27,7 @@ import org.eclipse.jgit.lib.ObjectId;
* <ul>
* <li>The list of modified files between two commits.
* <li>The list of modified files between a commit and its parent or the auto-merge.
* <li>The detailed file diff for a single file path (TODO:ghareeb).
* <li>The detailed file diff for a single file path.
* <li>The Intra-line diffs for a single file path (TODO:ghareeb).
* </ul>
*/
@@ -51,7 +51,7 @@ public interface DiffOperations {
* parents, if the {@code newCommit} could not be parsed for extracting the base commit, or if
* an internal error occurred in Git while evaluating the diff.
*/
Map<String, FileDiffOutput> getModifiedFilesAgainstParentOrAutoMerge(
Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
throws DiffNotAvailableException;
@@ -66,7 +66,41 @@ public interface DiffOperations {
* @throws DiffNotAvailableException if an internal error occurred in Git while evaluating the
* diff.
*/
Map<String, FileDiffOutput> getModifiedFilesBetweenPatchsets(
Map<String, FileDiffOutput> listModifiedFiles(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
throws DiffNotAvailableException;
/**
* 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.
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
* @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.
* @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)
throws DiffNotAvailableException;
/**
* 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.
*
* @param project a project name representing a git repository.
* @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.
* @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)
throws DiffNotAvailableException;
}

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

@@ -14,6 +14,10 @@
package com.google.gerrit.server.patch;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.flogger.FluentLogger;
@@ -37,11 +41,10 @@ import com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl.DiffAlgor
import com.google.inject.Inject;
import com.google.inject.Module;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevObject;
/**
* Provides different file diff operations. Uses the underlying Git/Gerrit caches to speed up the
@@ -81,73 +84,84 @@ public class DiffOperationsImpl implements DiffOperations {
}
@Override
public Map<String, FileDiffOutput> getModifiedFilesAgainstParentOrAutoMerge(
public Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
Project.NameKey project, ObjectId newCommit, @Nullable Integer parent)
throws DiffNotAvailableException {
try {
if (parent != null) {
RevObject base = baseCommitUtil.getBaseCommit(project, newCommit, parent);
return getModifiedFiles(project, base, newCommit, ComparisonType.againstParent(parent));
}
int numParents = baseCommitUtil.getNumParents(project, newCommit);
if (numParents == 1) {
RevObject base = baseCommitUtil.getBaseCommit(project, newCommit, parent);
ComparisonType cmp = ComparisonType.againstParent(1);
return getModifiedFiles(project, base, newCommit, cmp);
}
if (numParents > 2) {
logger.atFine().log(
"Diff against auto-merge for merge commits "
+ "with more than two parents is not supported. Commit "
+ newCommit
+ " has "
+ numParents
+ " parents. Falling back to the diff against the first parent.");
ObjectId firstParentId = baseCommitUtil.getBaseCommit(project, newCommit, 1).getId();
ImmutableList.Builder<FileDiffCacheKey> keys = ImmutableList.builder();
keys.add(createFileDiffCacheKey(project, firstParentId, newCommit, Patch.COMMIT_MSG));
keys.add(createFileDiffCacheKey(project, firstParentId, newCommit, Patch.MERGE_LIST));
return getModifiedFilesForKeys(keys.build());
}
RevObject autoMerge = baseCommitUtil.getBaseCommit(project, newCommit, null);
return getModifiedFiles(project, autoMerge, newCommit, ComparisonType.againstAutoMerge());
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
return getModifiedFiles(project, newCommit, diffParams);
} catch (IOException e) {
throw new DiffNotAvailableException(e);
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
}
}
@Override
public Map<String, FileDiffOutput> getModifiedFilesBetweenPatchsets(
public Map<String, FileDiffOutput> listModifiedFiles(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
throws DiffNotAvailableException {
ComparisonType cmp = ComparisonType.againstOtherPatchSet();
return getModifiedFiles(project, oldCommit, newCommit, cmp);
DiffParameters params =
DiffParameters.builder()
.project(project)
.newCommit(newCommit)
.baseCommit(oldCommit)
.comparisonType(ComparisonType.againstOtherPatchSet())
.build();
return getModifiedFiles(project, newCommit, params);
}
@Override
public FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project, ObjectId newCommit, @Nullable Integer parent, String fileName)
throws DiffNotAvailableException {
try {
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
FileDiffCacheKey key =
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName);
return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
}
}
@Override
public FileDiffOutput getModifiedFile(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, String fileName)
throws DiffNotAvailableException {
FileDiffCacheKey key = createFileDiffCacheKey(project, oldCommit, newCommit, fileName);
return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
}
private Map<String, FileDiffOutput> getModifiedFiles(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, ComparisonType cmp)
Project.NameKey project, ObjectId newCommit, DiffParameters diffParams)
throws DiffNotAvailableException {
try {
ObjectId oldCommit = diffParams.baseCommit();
ComparisonType cmp = diffParams.comparisonType();
ImmutableList<ModifiedFile> modifiedFiles =
modifiedFilesCache.get(createModifiedFilesKey(project, oldCommit, newCommit));
List<FileDiffCacheKey> fileCacheKeys =
modifiedFiles.stream()
.map(
entity ->
createFileDiffCacheKey(
project,
oldCommit,
newCommit,
entity.newPath().isPresent()
? entity.newPath().get()
: entity.oldPath().get()))
.collect(Collectors.toList());
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, Patch.COMMIT_MSG));
List<FileDiffCacheKey> fileCacheKeys = new ArrayList<>();
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, COMMIT_MSG));
if (cmp.isAgainstAutoMerge() || isMergeAgainstParent(cmp, project, newCommit)) {
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, Patch.MERGE_LIST));
fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, MERGE_LIST));
}
if (diffParams.skipFiles() == null) {
modifiedFiles.stream()
.map(
entity ->
createFileDiffCacheKey(
project,
oldCommit,
newCommit,
entity.newPath().isPresent()
? entity.newPath().get()
: entity.oldPath().get()))
.forEach(fileCacheKeys::add);
}
return getModifiedFilesForKeys(fileCacheKeys);
} catch (IOException e) {
@@ -206,4 +220,78 @@ public class DiffOperationsImpl implements DiffOperations {
.whitespace(Whitespace.IGNORE_NONE)
.build();
}
@AutoValue
abstract static class DiffParameters {
abstract Project.NameKey project();
abstract ObjectId newCommit();
abstract ObjectId baseCommit();
abstract ComparisonType comparisonType();
@Nullable
abstract Integer parent();
/** Compute the diff for {@value Patch#COMMIT_MSG} and {@link Patch#MERGE_LIST} only. */
@Nullable
abstract Boolean skipFiles();
static Builder builder() {
return new AutoValue_DiffOperationsImpl_DiffParameters.Builder();
}
@AutoValue.Builder
abstract static class Builder {
abstract Builder project(Project.NameKey project);
abstract Builder newCommit(ObjectId newCommit);
abstract Builder baseCommit(ObjectId baseCommit);
abstract Builder parent(@Nullable Integer parent);
abstract Builder skipFiles(@Nullable Boolean skipFiles);
abstract Builder comparisonType(ComparisonType comparisonType);
public abstract DiffParameters build();
}
}
/** Compute Diff parameters - the base commit and the comparison type - using the input args. */
private DiffParameters computeDiffParameters(
Project.NameKey project, ObjectId newCommit, Integer parent) throws IOException {
DiffParameters.Builder result =
DiffParameters.builder().project(project).newCommit(newCommit).parent(parent);
if (parent != null) {
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, parent));
result.comparisonType(ComparisonType.againstParent(parent));
return result.build();
}
int numParents = baseCommitUtil.getNumParents(project, newCommit);
if (numParents == 1) {
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, parent));
result.comparisonType(ComparisonType.againstParent(1));
return result.build();
}
if (numParents > 2) {
logger.atFine().log(
"Diff against auto-merge for merge commits "
+ "with more than two parents is not supported. Commit "
+ newCommit
+ " has "
+ numParents
+ " parents. Falling back to the diff against the first parent.");
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, 1).getId());
result.comparisonType(ComparisonType.againstParent(1));
result.skipFiles(true);
} else {
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, null));
result.comparisonType(ComparisonType.againstAutoMerge());
}
return result.build();
}
}