Merge changes I1bae3dcd,Id112f974,I01122bbc
* changes:
Fix comparison type against another patchset vs. auto-merge
Check for old and new file modes in FileDiffCacheImpl
Handle single file diffs for non-existent files
diff --git a/java/com/google/gerrit/server/patch/AutoMerger.java b/java/com/google/gerrit/server/patch/AutoMerger.java
index 2e0214c..fe915c5 100644
--- a/java/com/google/gerrit/server/patch/AutoMerger.java
+++ b/java/com/google/gerrit/server/patch/AutoMerger.java
@@ -67,6 +67,8 @@
* is that these refs should never be deleted.
*/
public class AutoMerger {
+ public static final String AUTO_MERGE_MSG_PREFIX = "Auto-merge of ";
+
@UsedAt(UsedAt.Project.GOOGLE)
public static boolean cacheAutomerge(Config cfg) {
return cfg.getBoolean("change", null, "cacheAutomerge", true);
@@ -198,7 +200,7 @@
cb.setAuthor(ident);
cb.setCommitter(ident);
cb.setTreeId(tree);
- cb.setMessage("Auto-merge of " + merge.name() + '\n');
+ cb.setMessage(AUTO_MERGE_MSG_PREFIX + merge.name() + '\n');
for (RevCommit p : merge.getParents()) {
cb.addParentId(p);
}
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index 87a43c0..8b90531 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -74,7 +74,8 @@
/**
* 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.
+ * 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.
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
@@ -96,7 +97,9 @@
/**
* 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.
+ * 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.
*
* @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 45ba789..16bd135 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -124,7 +124,8 @@
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
FileDiffCacheKey key =
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName, whitespace);
- return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
+ Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
+ return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -141,7 +142,8 @@
throws DiffNotAvailableException {
FileDiffCacheKey key =
createFileDiffCacheKey(project, oldCommit, newCommit, fileName, whitespace);
- return getModifiedFilesForKeys(ImmutableList.of(key)).get(fileName);
+ Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
+ return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
}
private Map<String, FileDiffOutput> getModifiedFiles(
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index fb2aa3d..63f311b 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -31,6 +31,7 @@
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.AutoMerger;
import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffUtil;
@@ -184,7 +185,8 @@
return result.build();
}
- private ComparisonType getComparisonType(RevWalk rw, ObjectId oldCommitId, ObjectId newCommitId)
+ private ComparisonType getComparisonType(
+ RevWalk rw, ObjectReader reader, ObjectId oldCommitId, ObjectId newCommitId)
throws IOException {
RevCommit oldCommit = DiffUtil.getRevCommit(rw, oldCommitId);
RevCommit newCommit = DiffUtil.getRevCommit(rw, newCommitId);
@@ -193,7 +195,16 @@
return ComparisonType.againstParent(i + 1);
}
}
- if (newCommit.getParentCount() > 0) {
+ // TODO(ghareeb): it's not trivial to distinguish if diff with old commit is against another
+ // patchset or auto-merge. Looking at the commit message of old commit gives a strong
+ // signal that we are diffing against auto-merge, though not 100% accurate (e.g. if old commit
+ // has the auto-merge prefix in the commit message). A better resolution would be to move the
+ // COMMIT_MSG and MERGE_LIST evaluations outside of the diff cache. For more details, see
+ // discussion in
+ // https://gerrit-review.googlesource.com/c/gerrit/+/280519/6..18/java/com/google/gerrit/server/patch/FileDiffCache.java#b540
+ Text oldCommitMsgTxt = Text.forCommit(reader, oldCommit);
+ if (oldCommitMsgTxt.size() > 0
+ && oldCommitMsgTxt.getString(0).startsWith(AutoMerger.AUTO_MERGE_MSG_PREFIX)) {
return ComparisonType.againstAutoMerge();
}
return ComparisonType.againstOtherPatchSet();
@@ -206,7 +217,8 @@
FileDiffCacheKey key, ObjectReader reader, RevWalk rw, MagicPath magicPath) {
try {
RawTextComparator cmp = comparatorFor(key.whitespace());
- ComparisonType comparisonType = getComparisonType(rw, key.oldCommit(), key.newCommit());
+ ComparisonType comparisonType =
+ getComparisonType(rw, reader, key.oldCommit(), key.newCommit());
RevCommit aCommit =
comparisonType.isAgainstParentOrAutoMerge()
? null
@@ -342,7 +354,7 @@
GitFileDiff mainGitDiff = allDiffs.mainDiff().gitDiff();
Long oldSize =
- mainGitDiff.oldPath().isPresent()
+ mainGitDiff.oldMode().isPresent() && mainGitDiff.oldPath().isPresent()
? new FileSizeEvaluator(reader, aTree)
.compute(
mainGitDiff.oldId(),
@@ -350,7 +362,7 @@
mainGitDiff.oldPath().get())
: 0;
Long newSize =
- mainGitDiff.newPath().isPresent()
+ mainGitDiff.newMode().isPresent() && mainGitDiff.newPath().isPresent()
? new FileSizeEvaluator(reader, bTree)
.compute(
mainGitDiff.newId(),
