Skip the diff cache for persisting copied votes: use RevWalk
We currently persist copied votes on upload and then need to consult
with the diff cache whether the list of files has changed (if the vote
should be copied if list of files has not changed). If we do that using
the diff cache, there are some visibility problems since the commit does
not yet exist in the repository, as it's just being created.
To fix it, we add new methods to the diff interface that accept a
RevWalk object. We load the modified files directly instead of looking
them up from the cache. RevWalk has visibility for the newly created
commit consistently.
This bug is not currently reproducible but we create this change as a
measure to fix potential issues that could arise from this bug. One
example from this bug is Ibc61b3908 which is resolved.
Change-Id: I1dea31cbff3f113083ec30d649d5a56f0ba1fa29
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 27870a1..fba2fcb 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -40,7 +40,7 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.approval.ApprovalContext;
@@ -126,9 +126,9 @@
PatchSet.Id psId,
ChangeKind kind,
LabelType type,
- @Nullable Map<String, FileDiffOutput> baseVsCurrentDiff,
- @Nullable Map<String, FileDiffOutput> baseVsPriorDiff,
- @Nullable Map<String, FileDiffOutput> priorVsCurrentDiff) {
+ @Nullable Map<String, ModifiedFile> baseVsCurrentDiff,
+ @Nullable Map<String, ModifiedFile> baseVsPriorDiff,
+ @Nullable Map<String, ModifiedFile> priorVsCurrentDiff) {
int n = psa.key().patchSetId().get();
checkArgument(n != psId.get());
@@ -316,11 +316,14 @@
PatchSetApproval psa,
PatchSet patchSet,
LabelType type,
- ChangeKind changeKind) {
+ ChangeKind changeKind,
+ RevWalk revWalk,
+ Config repoConfig) {
if (!type.getCopyCondition().isPresent()) {
return false;
}
- ApprovalContext ctx = ApprovalContext.create(changeNotes, psa, patchSet, changeKind);
+ ApprovalContext ctx =
+ ApprovalContext.create(changeNotes, psa, patchSet, changeKind, revWalk, repoConfig);
try {
// Use a request context to run checks as an internal user with expanded visibility. This is
// so that the output of the copy condition does not depend on who is running the current
@@ -381,9 +384,9 @@
priorPatchSet.getValue().id().changeId(),
changeKind);
- Map<String, FileDiffOutput> baseVsCurrent = null;
- Map<String, FileDiffOutput> baseVsPrior = null;
- Map<String, FileDiffOutput> priorVsCurrent = null;
+ Map<String, ModifiedFile> baseVsCurrent = null;
+ Map<String, ModifiedFile> baseVsPrior = null;
+ Map<String, ModifiedFile> priorVsCurrent = null;
LabelTypes labelTypes = project.getLabelTypes();
for (PatchSetApproval psa : priorApprovalsIncludingCopied) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
@@ -394,10 +397,11 @@
if (baseVsCurrent == null
&& type.isPresent()
&& type.get().isCopyAllScoresIfListOfFilesDidNotChange()) {
- baseVsCurrent = listModifiedFiles(project, patchSet);
- baseVsPrior = listModifiedFiles(project, priorPatchSet.getValue());
+ baseVsCurrent = listModifiedFiles(project, patchSet, rw, repoConfig);
+ baseVsPrior = listModifiedFiles(project, priorPatchSet.getValue(), rw, repoConfig);
priorVsCurrent =
- listModifiedFiles(project, priorPatchSet.getValue().commitId(), patchSet.commitId());
+ listModifiedFiles(
+ project, priorPatchSet.getValue().commitId(), patchSet.commitId(), rw, repoConfig);
}
if (!type.isPresent()) {
logger.atFine().log(
@@ -420,7 +424,8 @@
baseVsCurrent,
baseVsPrior,
priorVsCurrent)
- && !canCopyBasedOnCopyCondition(notes, psa, patchSet, type.get(), changeKind)) {
+ && !canCopyBasedOnCopyCondition(
+ notes, psa, patchSet, type.get(), changeKind, rw, repoConfig)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(patchSet.id()));
@@ -432,14 +437,20 @@
* Gets the modified files between the two latest patch-sets. Can be used to compute difference in
* files between those two patch-sets .
*/
- private Map<String, FileDiffOutput> listModifiedFiles(ProjectState project, PatchSet ps) {
+ private Map<String, ModifiedFile> listModifiedFiles(
+ ProjectState project, PatchSet ps, RevWalk revWalk, Config repoConfig) {
try {
Integer parentNum =
listOfFilesUnchangedPredicate.isInitialCommit(project.getNameKey(), ps.commitId())
? 0
: 1;
- return diffOperations.listModifiedFilesAgainstParent(
- project.getNameKey(), ps.commitId(), parentNum, DiffOptions.DEFAULTS);
+ return diffOperations.loadModifiedFilesAgainstParent(
+ project.getNameKey(),
+ ps.commitId(),
+ parentNum,
+ DiffOptions.DEFAULTS,
+ revWalk,
+ repoConfig);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
@@ -453,11 +464,20 @@
* Gets the modified files between two commits corresponding to different patchsets of the same
* change.
*/
- private Map<String, FileDiffOutput> listModifiedFiles(
- ProjectState project, ObjectId sourceCommit, ObjectId targetCommit) {
+ private Map<String, ModifiedFile> listModifiedFiles(
+ ProjectState project,
+ ObjectId sourceCommit,
+ ObjectId targetCommit,
+ RevWalk revWalk,
+ Config repoConfig) {
try {
- return diffOperations.listModifiedFiles(
- project.getNameKey(), sourceCommit, targetCommit, DiffOptions.DEFAULTS);
+ return diffOperations.loadModifiedFiles(
+ project.getNameKey(),
+ sourceCommit,
+ targetCommit,
+ DiffOptions.DEFAULTS,
+ revWalk,
+ repoConfig);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index c84186d..a265337 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -18,10 +18,14 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import java.util.Map;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevWalk;
/**
* An interface for all file diff related operations. Clients should use this interface to request:
@@ -60,6 +64,29 @@
throws DiffNotAvailableException;
/**
+ * This method is similar to {@link #listModifiedFilesAgainstParent(NameKey, ObjectId, int,
+ * DiffOptions)} but loads the modified files directly instead of retrieving them from the diff
+ * cache.
+ *
+ * <p>A RevWalk and repoConfig are also supplied and are used to look up the commit IDs. This is
+ * useful in case one the commits is currently being created, that's why the {@code revWalk}
+ * parameter is needed.
+ *
+ * <p>Note that rename detection is disabled for this method.
+ *
+ * @return a map of file paths to {@link ModifiedFile}. The {@link ModifiedFile} contains the
+ * old/new file paths and the change type (added, deleted, etc...).
+ */
+ Map<String, ModifiedFile> loadModifiedFilesAgainstParent(
+ Project.NameKey project,
+ ObjectId newCommit,
+ int parentNum,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException;
+
+ /**
* Returns the list of added, deleted or modified files between two commits (patchsets). The
* commit message and merge list (for merge commits) are also returned.
*
@@ -77,6 +104,29 @@
throws DiffNotAvailableException;
/**
+ * This method is similar to {@link #listModifiedFilesAgainstParent(NameKey, ObjectId, int,
+ * DiffOptions)} but loads the modified files directly instead of retrieving them from the diff
+ * cache.
+ *
+ * <p>A RevWalk and repoConfig are also supplied and are used to look up the commit IDs. This is
+ * useful in case one the commits is currently being created, that's why the {@code revWalk}
+ * parameter is needed.
+ *
+ * <p>Note that rename detection is disabled for this method.
+ *
+ * @return a map of file paths to {@link ModifiedFile}. The {@link ModifiedFile} contains the
+ * old/new file paths and the change type (added, deleted, etc...).
+ */
+ Map<String, ModifiedFile> loadModifiedFiles(
+ Project.NameKey project,
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ 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. This method will return {@link FileDiffOutput#empty(String, ObjectId,
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 3d230f8..012a91c 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -47,7 +47,15 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.eclipse.jgit.diff.DiffEntry;
+import org.eclipse.jgit.diff.DiffFormatter;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.io.DisabledOutputStream;
/**
* Provides different file diff operations. Uses the underlying Git/Gerrit caches to speed up the
@@ -57,6 +65,19 @@
public class DiffOperationsImpl implements DiffOperations {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final ImmutableMap<DiffEntry.ChangeType, Patch.ChangeType> changeTypeMap =
+ ImmutableMap.of(
+ DiffEntry.ChangeType.ADD,
+ Patch.ChangeType.ADDED,
+ DiffEntry.ChangeType.MODIFY,
+ Patch.ChangeType.MODIFIED,
+ DiffEntry.ChangeType.DELETE,
+ Patch.ChangeType.DELETED,
+ DiffEntry.ChangeType.RENAME,
+ Patch.ChangeType.RENAMED,
+ DiffEntry.ChangeType.COPY,
+ Patch.ChangeType.COPIED);
+
private static final int RENAME_SCORE = 60;
private static final DiffAlgorithm DEFAULT_DIFF_ALGORITHM =
DiffAlgorithm.HISTOGRAM_WITH_FALLBACK_MYERS;
@@ -103,6 +124,27 @@
}
@Override
+ public Map<String, ModifiedFile> loadModifiedFilesAgainstParent(
+ Project.NameKey project,
+ ObjectId newCommit,
+ int parentNum,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException {
+ try {
+ DiffParameters diffParams = computeDiffParameters(project, newCommit, parentNum);
+ return loadModifiedFilesWithoutCache(project, diffParams, revWalk, repoConfig);
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(
+ String.format(
+ "Failed to evaluate the parent/base commit for commit '%s' with parentNum=%d",
+ newCommit, parentNum),
+ e);
+ }
+ }
+
+ @Override
public Map<String, FileDiffOutput> listModifiedFiles(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, DiffOptions diffOptions)
throws DiffNotAvailableException {
@@ -117,6 +159,25 @@
}
@Override
+ public Map<String, ModifiedFile> loadModifiedFiles(
+ Project.NameKey project,
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException {
+ DiffParameters params =
+ DiffParameters.builder()
+ .project(project)
+ .newCommit(newCommit)
+ .baseCommit(oldCommit)
+ .comparisonType(ComparisonType.againstOtherPatchSet())
+ .build();
+ return loadModifiedFilesWithoutCache(project, params, revWalk, repoConfig);
+ }
+
+ @Override
public FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project,
ObjectId newCommit,
@@ -329,6 +390,50 @@
.build();
}
+ /** Loads the modified file paths between two commits without inspecting the diff cache. */
+ private static Map<String, ModifiedFile> loadModifiedFilesWithoutCache(
+ Project.NameKey project, DiffParameters diffParams, RevWalk revWalk, Config repoConfig)
+ throws DiffNotAvailableException {
+ ObjectId newCommit = diffParams.newCommit();
+ ObjectId oldCommit = diffParams.baseCommit();
+ try {
+ ObjectReader reader = revWalk.getObjectReader();
+ List<DiffEntry> diffEntries;
+ try (DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
+ df.setReader(reader, repoConfig);
+ df.setDetectRenames(false);
+ diffEntries = df.scan(oldCommit.equals(ObjectId.zeroId()) ? null : oldCommit, newCommit);
+ }
+ return diffEntries.stream()
+ .map(
+ entry ->
+ ModifiedFile.builder()
+ .changeType(toChangeType(entry.getChangeType()))
+ .oldPath(getGitPath(entry.getOldPath()))
+ .newPath(getGitPath(entry.getNewPath()))
+ .build())
+ .collect(ImmutableMap.toImmutableMap(ModifiedFile::getDefaultPath, Function.identity()));
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(
+ String.format(
+ "Failed to compute the modified files for project '%s',"
+ + " old commit '%s', new commit '%s'.",
+ project, oldCommit.name(), newCommit.name()),
+ e);
+ }
+ }
+
+ private static Optional<String> getGitPath(String path) {
+ return path.equals(DiffEntry.DEV_NULL) ? Optional.empty() : Optional.of(path);
+ }
+
+ private static Patch.ChangeType toChangeType(DiffEntry.ChangeType changeType) {
+ if (!changeTypeMap.containsKey(changeType)) {
+ throw new IllegalArgumentException("Unsupported type " + changeType);
+ }
+ return changeTypeMap.get(changeType);
+ }
+
@AutoValue
abstract static class DiffParameters {
abstract Project.NameKey project();
diff --git a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
index f4e7ca3..3596a54 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
@@ -47,6 +47,10 @@
*/
public abstract Optional<String> newPath();
+ public String getDefaultPath() {
+ return newPath().isPresent() ? newPath().get() : oldPath().get();
+ }
+
public static Builder builder() {
return new AutoValue_ModifiedFile.Builder();
}
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
index 4dedbb5..2839eb7 100644
--- a/java/com/google/gerrit/server/query/approval/ApprovalContext.java
+++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -21,6 +21,8 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.server.notedb.ChangeNotes;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevWalk;
/** Entity representing all required information to match predicates for copying approvals. */
@AutoValue
@@ -41,8 +43,19 @@
/** {@link ChangeKind} of the delta between the origin and target patch set. */
public abstract ChangeKind changeKind();
+ /** {@link RevWalk} of the repository for the current commit. */
+ public abstract RevWalk revWalk();
+
+ /** {@link RevWalk} of the repository for the current commit. */
+ public abstract Config repoConfig();
+
public static ApprovalContext create(
- ChangeNotes changeNotes, PatchSetApproval psa, PatchSet patchSet, ChangeKind changeKind) {
+ ChangeNotes changeNotes,
+ PatchSetApproval psa,
+ PatchSet patchSet,
+ ChangeKind changeKind,
+ RevWalk revWalk,
+ Config repoConfig) {
checkState(
psa.patchSetId().changeId().equals(patchSet.id().changeId()),
"approval and target must be the same change. got: %s, %s",
@@ -54,6 +67,7 @@
// As explained in the commit message of this change doing this check is only possible if there
// are no changes with gaps in patch set numbers. Since it's planned to fix-up old changes with
// gaps in patch set numbers, this todo is a reminder to add back the check once this is done.
- return new AutoValue_ApprovalContext(psa, patchSet, changeNotes, changeKind);
+ return new AutoValue_ApprovalContext(
+ psa, patchSet, changeNotes, changeKind, revWalk, repoConfig);
}
}
diff --git a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
index c35fa1c..2a72c49 100644
--- a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
+++ b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
@@ -24,7 +24,7 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -59,24 +59,30 @@
Integer parentNum =
isInitialCommit(ctx.changeNotes().getProjectName(), targetPatchSet.commitId()) ? 0 : 1;
try {
- Map<String, FileDiffOutput> baseVsCurrent =
- diffOperations.listModifiedFilesAgainstParent(
+ Map<String, ModifiedFile> baseVsCurrent =
+ diffOperations.loadModifiedFilesAgainstParent(
ctx.changeNotes().getProjectName(),
targetPatchSet.commitId(),
parentNum,
- DiffOptions.DEFAULTS);
- Map<String, FileDiffOutput> baseVsPrior =
- diffOperations.listModifiedFilesAgainstParent(
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
+ Map<String, ModifiedFile> baseVsPrior =
+ diffOperations.loadModifiedFilesAgainstParent(
ctx.changeNotes().getProjectName(),
sourcePatchSet.commitId(),
parentNum,
- DiffOptions.DEFAULTS);
- Map<String, FileDiffOutput> priorVsCurrent =
- diffOperations.listModifiedFiles(
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
+ Map<String, ModifiedFile> priorVsCurrent =
+ diffOperations.loadModifiedFiles(
ctx.changeNotes().getProjectName(),
sourcePatchSet.commitId(),
targetPatchSet.commitId(),
- DiffOptions.DEFAULTS);
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
return match(baseVsCurrent, baseVsPrior, priorVsCurrent);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
@@ -92,9 +98,9 @@
* {@link ChangeType} matches for each modified file.
*/
public boolean match(
- Map<String, FileDiffOutput> baseVsCurrent,
- Map<String, FileDiffOutput> baseVsPrior,
- Map<String, FileDiffOutput> priorVsCurrent) {
+ Map<String, ModifiedFile> baseVsCurrent,
+ Map<String, ModifiedFile> baseVsPrior,
+ Map<String, ModifiedFile> priorVsCurrent) {
Set<String> allFiles = new HashSet<>();
allFiles.addAll(baseVsCurrent.keySet());
allFiles.addAll(baseVsPrior.keySet());
@@ -102,17 +108,17 @@
if (Patch.isMagic(file)) {
continue;
}
- FileDiffOutput fileDiffOutput1 = baseVsCurrent.get(file);
- FileDiffOutput fileDiffOutput2 = baseVsPrior.get(file);
+ ModifiedFile modifiedFile1 = baseVsCurrent.get(file);
+ ModifiedFile modifiedFile2 = baseVsPrior.get(file);
if (!priorVsCurrent.containsKey(file)) {
// If the file is not modified between prior and current patchsets, then scan safely skip
- // it. The file might has been modified due to rebase.
+ // it. The file might have been modified due to rebase.
continue;
}
- if (fileDiffOutput1 == null || fileDiffOutput2 == null) {
+ if (modifiedFile1 == null || modifiedFile2 == null) {
return false;
}
- if (!fileDiffOutput2.changeType().equals(fileDiffOutput1.changeType())) {
+ if (!modifiedFile2.changeType().equals(modifiedFile1.changeType())) {
return false;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
index 537c7d8..97f072e 100644
--- a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -36,6 +36,8 @@
import com.google.gerrit.server.query.approval.ApprovalQueryBuilder;
import com.google.inject.Inject;
import java.util.Date;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Test;
public class ApprovalQueryIT extends AbstractDaemonTest {
@@ -250,7 +252,7 @@
}
private ApprovalContext contextForCodeReviewLabel(
- int value, PatchSet.Id psId, Account.Id approver) {
+ int value, PatchSet.Id psId, Account.Id approver) throws Exception {
ChangeNotes changeNotes = changeNotesFactory.create(project, psId.changeId());
PatchSet.Id newPsId = PatchSet.id(psId.changeId(), psId.get() + 1);
ChangeKind changeKind =
@@ -263,7 +265,15 @@
.key(PatchSetApproval.key(psId, approver, LabelId.create("Code-Review")))
.value(value)
.build();
- return ApprovalContext.create(
- changeNotes, approval, changeNotes.getPatchSets().get(newPsId), changeKind);
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo.newObjectReader())) {
+ return ApprovalContext.create(
+ changeNotes,
+ approval,
+ changeNotes.getPatchSets().get(newPsId),
+ changeKind,
+ rw,
+ repo.getConfig());
+ }
}
}
diff --git a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
index 9acb701..8b62628 100644
--- a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
@@ -19,10 +19,12 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.InMemoryModule;
import com.google.inject.Guice;
@@ -30,6 +32,7 @@
import com.google.inject.Injector;
import java.io.IOException;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
@@ -37,6 +40,7 @@
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
import org.eclipse.jgit.lib.TreeFormatter;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
@@ -112,6 +116,66 @@
assertThat(repo.getRefDatabase().exactRef(autoMergeRef)).isNotNull();
}
+ @Test
+ public void loadModifiedFiles() throws Exception {
+ ImmutableMap<String, String> oldFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2);
+ ObjectId oldCommitId = createCommit(repo, null, oldFiles);
+
+ ImmutableMap<String, String> newFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2 + "\nnew line here");
+ ObjectId newCommitId = createCommit(repo, oldCommitId, newFiles);
+
+ Repository repository = repoManager.openRepository(testProjectName);
+ ObjectReader objectReader = repository.newObjectReader();
+ RevWalk rw = new RevWalk(objectReader);
+ StoredConfig repoConfig = repository.getConfig();
+
+ // This call loads modified files directly without going through the diff cache.
+ Map<String, ModifiedFile> modifiedFiles =
+ diffOperations.loadModifiedFiles(
+ testProjectName, newCommitId, oldCommitId, DiffOptions.DEFAULTS, rw, repoConfig);
+
+ assertThat(modifiedFiles)
+ .containsExactly(
+ fileName2,
+ ModifiedFile.builder()
+ .changeType(ChangeType.MODIFIED)
+ .oldPath(Optional.of(fileName2))
+ .newPath(Optional.of(fileName2))
+ .build());
+ }
+
+ @Test
+ public void loadModifiedFilesAgainstParent() throws Exception {
+ ImmutableMap<String, String> oldFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2);
+ ObjectId oldCommitId = createCommit(repo, null, oldFiles);
+
+ ImmutableMap<String, String> newFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2 + "\nnew line here");
+ ObjectId newCommitId = createCommit(repo, oldCommitId, newFiles);
+
+ Repository repository = repoManager.openRepository(testProjectName);
+ ObjectReader objectReader = repository.newObjectReader();
+ RevWalk rw = new RevWalk(objectReader);
+ StoredConfig repoConfig = repository.getConfig();
+
+ // This call loads modified files directly without going through the diff cache.
+ Map<String, ModifiedFile> modifiedFiles =
+ diffOperations.loadModifiedFilesAgainstParent(
+ testProjectName, newCommitId, /* parentNum=*/ 0, DiffOptions.DEFAULTS, rw, repoConfig);
+
+ assertThat(modifiedFiles)
+ .containsExactly(
+ fileName2,
+ ModifiedFile.builder()
+ .changeType(ChangeType.MODIFIED)
+ .oldPath(Optional.of(fileName2))
+ .newPath(Optional.of(fileName2))
+ .build());
+ }
+
private ObjectId createMergeCommit(
Repository repo,
ImmutableMap<String, String> fileNameToContent,
