Fix porting comments when all edits are due to rebase
Porting comments relies on the diff logic to map positions of comments
from the source patchset to the target patchset. By default, the diff
logic in DiffOperations skips files whose edits are all due to rebase
between the old/new commits. This made sense for the diff view because
we don't want to bother users looking at a modified file in the files
tab when it was really unchanged.
For porting comments, this resulted in the porter thinking that the file
was unchanged, hence it mapped the comment to the same line in the
target patchset which is wrong. We need to account for all edits
(including rebase edits) when doing the mapping.
In this change, I added another parameter to the DiffOperations methods
named "DiffOptions" that controls whether we would like to skip files
whose edits are all due to rebase or not. I updated all callers to use
the current default option, hence this change is a no/op for these
callers. I only updated the CommentPorter caller to pass this options
with false.
Note that this change to the API can break external callers (e.g.
plugins) that rely on this interface. I'm aware of one such plugin which
is the code-owners plugin. I'll adapt it in a follow up change.
I added a test in PortedCommentsIT for this case: the test fails before
this change (comment is ported to the wrong line) and passes with it.
Bug: Google b/204837335
Change-Id: I22a465e493776772a1440c9c6294478335fa2bcc
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index 1d38877..735caff 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -44,6 +44,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
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.update.ChangeContext;
import com.google.inject.Inject;
@@ -440,7 +441,7 @@
// unignore the test in PortedCommentsIT.
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchset.commitId(), /* parentNum= */ 0);
+ change.getProject(), patchset.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
return modifiedFiles.isEmpty()
? null
: modifiedFiles.values().iterator().next().oldCommitId();
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 695997a..5517ab1 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -39,6 +39,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
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.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
@@ -463,7 +464,7 @@
? 0
: 1;
return diffOperations.listModifiedFilesAgainstParent(
- project.getNameKey(), ps.commitId(), parentNum);
+ project.getNameKey(), ps.commitId(), parentNum, DiffOptions.DEFAULTS);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
@@ -480,7 +481,8 @@
private Map<String, FileDiffOutput> listModifiedFiles(
ProjectState project, ObjectId sourceCommit, ObjectId targetCommit) {
try {
- return diffOperations.listModifiedFiles(project.getNameKey(), sourceCommit, targetCommit);
+ return diffOperations.listModifiedFiles(
+ project.getNameKey(), sourceCommit, targetCommit, DiffOptions.DEFAULTS);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
index b729c11..44b4ded 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
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.FilePathAdapter;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -51,9 +52,11 @@
// single-parent commits, or the auto-merge otherwise
return asFileInfo(
diffs.listModifiedFilesAgainstParent(
- change.getProject(), objectId, /* parentNum= */ 0));
+ change.getProject(), objectId, /* parentNum= */ 0, DiffOptions.DEFAULTS));
}
- return asFileInfo(diffs.listModifiedFiles(change.getProject(), base.commitId(), objectId));
+ return asFileInfo(
+ diffs.listModifiedFiles(
+ change.getProject(), base.commitId(), objectId, DiffOptions.DEFAULTS));
} catch (DiffNotAvailableException e) {
convertException(e);
return null; // unreachable. handleAndThrow will throw an exception anyway
@@ -66,7 +69,7 @@
throws ResourceConflictException, PatchListNotAvailableException {
try {
Map<String, FileDiffOutput> modifiedFiles =
- diffs.listModifiedFilesAgainstParent(project, objectId, parent);
+ diffs.listModifiedFilesAgainstParent(project, objectId, parent, DiffOptions.DEFAULTS);
return asFileInfo(modifiedFiles);
} catch (DiffNotAvailableException e) {
convertException(e);
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index a7fea3c..2a2bf73 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -58,6 +58,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
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.FilePathAdapter;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.query.change.ChangeData;
@@ -400,7 +401,7 @@
try {
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchSet.commitId(), /* parentNum= */ 0);
+ change.getProject(), patchSet.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
for (FileDiffOutput diff : modifiedFiles.values()) {
if (patchSetAttribute.files == null) {
@@ -457,7 +458,7 @@
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchSet.commitId(), /* parentNum= */ 0);
+ change.getProject(), patchSet.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
for (FileDiffOutput fileDiff : modifiedFiles.values()) {
p.sizeDeletions += fileDiff.deletions();
p.sizeInsertions += fileDiff.insertions();
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index 606fb28..9aec1f4 100644
--- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -42,6 +42,7 @@
import com.google.gerrit.server.mail.send.ProjectWatch.Watchers;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.DiffNotAvailableException;
+import com.google.gerrit.server.patch.DiffOptions;
import com.google.gerrit.server.patch.FilePathAdapter;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -319,14 +320,14 @@
}
}
return args.diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), ps.commitId(), /* parentNum= */ 0);
+ change.getProject(), ps.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
}
/** Get the patch list corresponding to this patch set. */
protected Map<String, FileDiffOutput> listModifiedFiles() throws DiffNotAvailableException {
if (patchSet != null) {
return args.diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchSet.commitId(), /* parentNum= */ 0);
+ change.getProject(), patchSet.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
}
throw new DiffNotAvailableException("no patchSet specified");
}
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index d2da736..c84186d 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -56,7 +56,8 @@
* an internal error occurred in Git while evaluating the diff.
*/
Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
- Project.NameKey project, ObjectId newCommit, int parentNum) throws DiffNotAvailableException;
+ Project.NameKey project, ObjectId newCommit, int parentNum, DiffOptions diffOptions)
+ throws DiffNotAvailableException;
/**
* Returns the list of added, deleted or modified files between two commits (patchsets). The
@@ -72,7 +73,7 @@
* diff.
*/
Map<String, FileDiffOutput> listModifiedFiles(
- Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
+ Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, DiffOptions diffOptions)
throws DiffNotAvailableException;
/**
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 3423b32..3d230f8 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -91,10 +91,11 @@
@Override
public Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
- Project.NameKey project, ObjectId newCommit, int parent) throws DiffNotAvailableException {
+ Project.NameKey project, ObjectId newCommit, int parent, DiffOptions diffOptions)
+ throws DiffNotAvailableException {
try {
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
- return getModifiedFiles(diffParams);
+ return getModifiedFiles(diffParams, diffOptions);
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -103,7 +104,7 @@
@Override
public Map<String, FileDiffOutput> listModifiedFiles(
- Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
+ Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, DiffOptions diffOptions)
throws DiffNotAvailableException {
DiffParameters params =
DiffParameters.builder()
@@ -112,7 +113,7 @@
.baseCommit(oldCommit)
.comparisonType(ComparisonType.againstOtherPatchSet())
.build();
- return getModifiedFiles(params);
+ return getModifiedFiles(params, diffOptions);
}
@Override
@@ -161,8 +162,8 @@
return getModifiedFileForKey(key);
}
- private ImmutableMap<String, FileDiffOutput> getModifiedFiles(DiffParameters diffParams)
- throws DiffNotAvailableException {
+ private ImmutableMap<String, FileDiffOutput> getModifiedFiles(
+ DiffParameters diffParams, DiffOptions diffOptions) throws DiffNotAvailableException {
try {
Project.NameKey project = diffParams.project();
ObjectId newCommit = diffParams.newCommit();
@@ -211,7 +212,7 @@
/* whitespace= */ null))
.forEach(fileCacheKeys::add);
}
- return getModifiedFilesForKeys(fileCacheKeys);
+ return getModifiedFilesForKeys(fileCacheKeys, diffOptions);
} catch (IOException e) {
throw new DiffNotAvailableException(e);
}
@@ -219,7 +220,8 @@
private FileDiffOutput getModifiedFileForKey(FileDiffCacheKey key)
throws DiffNotAvailableException {
- Map<String, FileDiffOutput> diffList = getModifiedFilesForKeys(ImmutableList.of(key));
+ Map<String, FileDiffOutput> diffList =
+ getModifiedFilesForKeys(ImmutableList.of(key), DiffOptions.DEFAULTS);
return diffList.containsKey(key.newFilePath())
? diffList.get(key.newFilePath())
: FileDiffOutput.empty(key.newFilePath(), key.oldCommit(), key.newCommit());
@@ -230,8 +232,8 @@
* results, e.g. due to timeouts in the cache loader, this method requests the diff again using
* the fallback algorithm {@link DiffAlgorithm#HISTOGRAM_NO_FALLBACK}.
*/
- private ImmutableMap<String, FileDiffOutput> getModifiedFilesForKeys(List<FileDiffCacheKey> keys)
- throws DiffNotAvailableException {
+ private ImmutableMap<String, FileDiffOutput> getModifiedFilesForKeys(
+ List<FileDiffCacheKey> keys, DiffOptions diffOptions) throws DiffNotAvailableException {
ImmutableMap<FileDiffCacheKey, FileDiffOutput> fileDiffs = fileDiffCache.getAll(keys);
List<FileDiffCacheKey> fallbackKeys = new ArrayList<>();
@@ -260,7 +262,7 @@
}
}
result.addAll(fileDiffCache.getAll(fallbackKeys).values());
- return mapByFilePath(result.build());
+ return mapByFilePath(result.build(), diffOptions);
}
/**
@@ -268,11 +270,12 @@
* represent the old file path for deleted files, or the new path otherwise.
*/
private ImmutableMap<String, FileDiffOutput> mapByFilePath(
- ImmutableCollection<FileDiffOutput> fileDiffOutputs) {
+ ImmutableCollection<FileDiffOutput> fileDiffOutputs, DiffOptions diffOptions) {
ImmutableMap.Builder<String, FileDiffOutput> diffs = ImmutableMap.builder();
for (FileDiffOutput fileDiffOutput : fileDiffOutputs) {
- if (fileDiffOutput.isEmpty() || allDueToRebase(fileDiffOutput)) {
+ if (fileDiffOutput.isEmpty()
+ || (diffOptions.skipFilesWithAllEditsDueToRebase() && allDueToRebase(fileDiffOutput))) {
continue;
}
if (fileDiffOutput.changeType() == ChangeType.DELETED) {
diff --git a/java/com/google/gerrit/server/patch/DiffOptions.java b/java/com/google/gerrit/server/patch/DiffOptions.java
new file mode 100644
index 0000000..4d54be1
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffOptions.java
@@ -0,0 +1,36 @@
+// Copyright (C) 2021 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.auto.value.AutoValue;
+
+@AutoValue
+public abstract class DiffOptions {
+ public static final DiffOptions DEFAULTS =
+ DiffOptions.builder().skipFilesWithAllEditsDueToRebase(true).build();
+
+ public abstract boolean skipFilesWithAllEditsDueToRebase();
+
+ public static DiffOptions.Builder builder() {
+ return new AutoValue_DiffOptions.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder skipFilesWithAllEditsDueToRebase(boolean value);
+
+ public abstract DiffOptions build();
+ }
+}
diff --git a/java/com/google/gerrit/server/patch/DiffSummaryLoader.java b/java/com/google/gerrit/server/patch/DiffSummaryLoader.java
index fcce672..246544b 100644
--- a/java/com/google/gerrit/server/patch/DiffSummaryLoader.java
+++ b/java/com/google/gerrit/server/patch/DiffSummaryLoader.java
@@ -48,8 +48,9 @@
ObjectId newId = key.toPatchListKey().getNewId();
Map<String, FileDiffOutput> diffList =
oldId == null
- ? diffOperations.listModifiedFilesAgainstParent(project, newId, /* parentNum= */ 0)
- : diffOperations.listModifiedFiles(project, oldId, newId);
+ ? diffOperations.listModifiedFilesAgainstParent(
+ project, newId, /* parentNum= */ 0, DiffOptions.DEFAULTS)
+ : diffOperations.listModifiedFiles(project, oldId, newId, DiffOptions.DEFAULTS);
return toDiffSummary(diffList);
}
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index 572d73d..1baae25 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -299,7 +299,8 @@
private Map<String, FileDiffOutput> listModifiedFiles(
Project.NameKey project, PatchSet ps, PatchSet priorPatchSet) {
try {
- return diffOperations.listModifiedFiles(project, priorPatchSet.commitId(), ps.commitId());
+ return diffOperations.listModifiedFiles(
+ project, priorPatchSet.commitId(), ps.commitId(), DiffOptions.DEFAULTS);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't post diff messsage on submit although "
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementExpressionsValidator.java b/java/com/google/gerrit/server/project/SubmitRequirementExpressionsValidator.java
index 738e71b..45f58d4 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementExpressionsValidator.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementExpressionsValidator.java
@@ -27,6 +27,7 @@
import com.google.gerrit.server.git.validators.ValidationMessage;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
+import com.google.gerrit.server.patch.DiffOptions;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -110,7 +111,10 @@
throws DiffNotAvailableException {
return diffOperations
.listModifiedFilesAgainstParent(
- receiveEvent.project.getNameKey(), receiveEvent.commit, /* parentNum=*/ 0)
+ receiveEvent.project.getNameKey(),
+ receiveEvent.commit,
+ /* parentNum=*/ 0,
+ DiffOptions.DEFAULTS)
.keySet().stream()
.anyMatch(fileName::equals);
}
diff --git a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
index de7dd0a..c35fa1c 100644
--- a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
+++ b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
@@ -23,6 +23,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
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.inject.Inject;
import com.google.inject.Singleton;
@@ -60,15 +61,22 @@
try {
Map<String, FileDiffOutput> baseVsCurrent =
diffOperations.listModifiedFilesAgainstParent(
- ctx.changeNotes().getProjectName(), targetPatchSet.commitId(), parentNum);
+ ctx.changeNotes().getProjectName(),
+ targetPatchSet.commitId(),
+ parentNum,
+ DiffOptions.DEFAULTS);
Map<String, FileDiffOutput> baseVsPrior =
diffOperations.listModifiedFilesAgainstParent(
- ctx.changeNotes().getProjectName(), sourcePatchSet.commitId(), parentNum);
+ ctx.changeNotes().getProjectName(),
+ sourcePatchSet.commitId(),
+ parentNum,
+ DiffOptions.DEFAULTS);
Map<String, FileDiffOutput> priorVsCurrent =
diffOperations.listModifiedFiles(
ctx.changeNotes().getProjectName(),
sourcePatchSet.commitId(),
- targetPatchSet.commitId());
+ targetPatchSet.commitId(),
+ DiffOptions.DEFAULTS);
return match(baseVsCurrent, baseVsPrior, priorVsCurrent);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
index 1a4eb18..4032dcf 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
@@ -40,6 +40,7 @@
import com.google.gerrit.server.patch.DiffMappings;
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.GitPositionTransformer;
import com.google.gerrit.server.patch.GitPositionTransformer.BestPositionOnConflict;
import com.google.gerrit.server.patch.GitPositionTransformer.FileMapping;
@@ -315,7 +316,11 @@
TraceContext.newTimer(
"Computing diffs", Metadata.builder().commit(originalCommit.name()).build())) {
Map<String, FileDiffOutput> modifiedFiles =
- diffOperations.listModifiedFiles(project, originalCommit, targetCommit);
+ diffOperations.listModifiedFiles(
+ project,
+ originalCommit,
+ targetCommit,
+ DiffOptions.builder().skipFilesWithAllEditsDueToRebase(false).build());
return modifiedFiles.values().stream()
.map(CommentPorter::getFileEdits)
.map(DiffMappings::toMapping)
diff --git a/java/com/google/gerrit/server/restapi/change/Files.java b/java/com/google/gerrit/server/restapi/change/Files.java
index 320e57d..6c85e15 100644
--- a/java/com/google/gerrit/server/restapi/change/Files.java
+++ b/java/com/google/gerrit/server/restapi/change/Files.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
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.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -280,11 +281,14 @@
Map<String, FileDiffOutput> oldList =
diffOperations.listModifiedFilesAgainstParent(
- project, patchSet.commitId(), /* parentNum= */ 0);
+ project, patchSet.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
Map<String, FileDiffOutput> curList =
diffOperations.listModifiedFilesAgainstParent(
- project, resource.getPatchSet().commitId(), /* parentNum= */ 0);
+ project,
+ resource.getPatchSet().commitId(),
+ /* parentNum= */ 0,
+ DiffOptions.DEFAULTS);
int sz = paths.size();
List<String> pathList = Lists.newArrayListWithCapacity(sz);
diff --git a/java/com/google/gerrit/server/rules/StoredValues.java b/java/com/google/gerrit/server/rules/StoredValues.java
index fd66a3a..6a802e86 100644
--- a/java/com/google/gerrit/server/rules/StoredValues.java
+++ b/java/com/google/gerrit/server/rules/StoredValues.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
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.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectState;
@@ -98,7 +99,7 @@
try {
diffList =
diffOperations.listModifiedFilesAgainstParent(
- project, ps.commitId(), /* parentNum= */ 0);
+ project, ps.commitId(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
} catch (DiffNotAvailableException e) {
throw new SystemException(
String.format(
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
index 7197425..cdde0b3 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
@@ -22,23 +22,30 @@
import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThatList;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.MapSubject.assertThatMap;
+import static java.util.stream.Collectors.joining;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation;
import com.google.gerrit.acceptance.testsuite.change.TestPatchset;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.DeleteCommentInput;
+import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
@@ -47,6 +54,8 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
+import java.util.stream.IntStream;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Ignore;
import org.junit.Test;
@@ -143,6 +152,39 @@
}
@Test
+ public void commentsArePortedWhenAllEditsAreDueToRebase() throws Exception {
+ String fileName = "f.txt";
+ String baseContent =
+ IntStream.rangeClosed(1, 50)
+ .mapToObj(number -> String.format("Line %d\n", number))
+ .collect(joining());
+ ObjectId headCommit = testRepo.getRepository().resolve("HEAD");
+ ObjectId baseCommit = addCommit(headCommit, fileName, baseContent);
+
+ // Create a change on top of baseCommit, modify line 1, then add comment on line 10.
+ PushOneCommit.Result r = createEmptyChange();
+ Change.Id changeId = r.getChange().getId();
+ addModifiedPatchSet(
+ changeId.toString(), fileName, baseContent.replace("Line 1\n", "Line one\n"));
+ PatchSet.Id ps2Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ newComment(ps2Id).message("Line comment").onLine(10).ofFile(fileName).create();
+
+ // Add a commit on top of baseCommit. Delete line 4. Rebase the change on top of this commit.
+ ObjectId newBase = addCommit(baseCommit, fileName, baseContent.replace("Line 4\n", ""));
+ rebaseChangeOn(changeId.toString(), newBase);
+ PatchSet.Id ps3Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+
+ List<CommentInfo> portedComments = flatten(getPortedComments(ps3Id));
+ assertThat(portedComments).hasSize(1);
+ int portedLine = portedComments.get(0).line;
+ BinaryResult fileContent = gApi.changes().id(changeId.get()).current().file(fileName).content();
+ String[] lines = fileContent.asString().split("\n");
+ // Comment has shifted to L9 instead of L10 because of the deletion of line 4.
+ assertThat(portedLine).isEqualTo(9);
+ assertThat(lines[portedLine - 1]).isEqualTo("Line 10");
+ }
+
+ @Test
public void resolvedAndUnresolvedDraftCommentsArePorted() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
// Set up change and patchsets.
@@ -1909,4 +1951,35 @@
private static Correspondence<CommentInfo, String> hasUuid() {
return NullAwareCorrespondence.transforming(comment -> comment.id, "hasUuid");
}
+
+ private Result createEmptyChange() throws Exception {
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Test change", ImmutableMap.of());
+ return push.to("refs/for/master");
+ }
+
+ private void rebaseChangeOn(String changeId, ObjectId newParent) throws Exception {
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.base = newParent.getName();
+ gApi.changes().id(changeId).current().rebase(rebaseInput);
+ }
+
+ private void addModifiedPatchSet(String changeId, String filePath, String content)
+ throws Exception {
+ gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(content));
+ gApi.changes().id(changeId).edit().publish();
+ }
+
+ private ObjectId addCommit(ObjectId parentCommit, String fileName, String fileContent)
+ throws Exception {
+ testRepo.reset(parentCommit);
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Adjust files of repo",
+ ImmutableMap.of(fileName, fileContent));
+ PushOneCommit.Result result = push.to("refs/for/master");
+ return result.getCommit();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 58dc0b0..d9531f0 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -51,6 +51,7 @@
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.webui.EditWebLink;
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.inject.Inject;
import java.awt.image.BufferedImage;
@@ -148,7 +149,7 @@
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- project, result.getCommit(), /* parentNum= */ 0);
+ project, result.getCommit(), /* parentNum= */ 0, DiffOptions.DEFAULTS);
assertThat(modifiedFiles.keySet()).containsExactly("/COMMIT_MSG", "f.txt");
assertThat(
diff --git a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
index d47afb0..9acb701 100644
--- a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
@@ -104,7 +104,8 @@
assertThat(repo.getRefDatabase().exactRef(autoMergeRef)).isNull();
Map<String, FileDiffOutput> changedFiles =
- diffOperations.listModifiedFilesAgainstParent(testProjectName, merge, /* parentNum=*/ 0);
+ diffOperations.listModifiedFilesAgainstParent(
+ testProjectName, merge, /* parentNum=*/ 0, DiffOptions.DEFAULTS);
assertThat(changedFiles.keySet()).containsExactly("/COMMIT_MSG", "/MERGE_LIST", "file_3.txt");
// Requesting diff against auto-merge had the side effect of updating the auto-merge ref
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index 46f9c5a..47bfb2a 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -38,6 +38,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
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.restapi.change.CommentPorter.Metrics;
import com.google.gerrit.truth.NullAwareCorrespondence;
import java.sql.Timestamp;
@@ -78,7 +79,10 @@
when(commentsUtil.determineCommitId(any(), any(), anyShort()))
.thenReturn(Optional.of(dummyObjectId));
when(diffOperations.listModifiedFiles(
- any(Project.NameKey.class), any(ObjectId.class), any(ObjectId.class)))
+ any(Project.NameKey.class),
+ any(ObjectId.class),
+ any(ObjectId.class),
+ any(DiffOptions.class)))
.thenThrow(DiffNotAvailableException.class);
ImmutableList<HumanComment> portedComments =
commentPorter.portComments(
@@ -101,7 +105,10 @@
when(commentsUtil.determineCommitId(any(), any(), anyShort()))
.thenReturn(Optional.of(dummyObjectId));
when(diffOperations.listModifiedFiles(
- any(Project.NameKey.class), any(ObjectId.class), any(ObjectId.class)))
+ any(Project.NameKey.class),
+ any(ObjectId.class),
+ any(ObjectId.class),
+ any(DiffOptions.class)))
.thenThrow(IllegalStateException.class);
ImmutableList<HumanComment> portedComments =
commentPorter.portComments(
@@ -144,7 +151,10 @@
when(commentsUtil.determineCommitId(any(), any(), anyShort()))
.thenReturn(Optional.of(dummyObjectId));
when(diffOperations.listModifiedFiles(
- any(Project.NameKey.class), any(ObjectId.class), any(ObjectId.class)))
+ any(Project.NameKey.class),
+ any(ObjectId.class),
+ any(ObjectId.class),
+ any(DiffOptions.class)))
.thenThrow(IllegalStateException.class);
ImmutableList<HumanComment> portedComments =
commentPorter.portComments(
@@ -173,7 +183,10 @@
.thenReturn(Optional.of(dummyObjectId));
// Throw an exception on the first diff request but return an actual value on the second.
when(diffOperations.listModifiedFiles(
- any(Project.NameKey.class), any(ObjectId.class), any(ObjectId.class)))
+ any(Project.NameKey.class),
+ any(ObjectId.class),
+ any(ObjectId.class),
+ any(DiffOptions.class)))
.thenThrow(IllegalStateException.class)
.thenReturn(ImmutableMap.of());
ImmutableList<HumanComment> portedComments =
@@ -200,7 +213,10 @@
when(commentsUtil.determineCommitId(any(), any(), anyShort()))
.thenReturn(Optional.of(dummyObjectId));
when(diffOperations.listModifiedFiles(
- any(Project.NameKey.class), any(ObjectId.class), any(ObjectId.class)))
+ any(Project.NameKey.class),
+ any(ObjectId.class),
+ any(ObjectId.class),
+ any(DiffOptions.class)))
.thenReturn(ImmutableMap.of());
ImmutableList<HumanComment> portedComments =
commentPorter.portComments(
