Include old and new file modes in diffs
This was previously implemented and approved for the legacy diff cache
(see change I79f483fa8) that we abandonned since we redesigned the diff
cache. In this change, we add the old and new files modes to the diff
output and surface them with the diff endpoints.
Adding old/new modes to the response is essential such that if a file is
modified with a change to the file mode only, the file will be listed as
modified.
Google-Bug-Id: b/179792565
Release-Notes: add old/new file modes to the 'List Files' diff endpoint
Forward-Compatible: checked
Change-Id: I04f6fd9f4fba2658f3ee5f5529a78af93d211836
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index ab7fb3b..b160f59 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7471,8 +7471,13 @@
differ by one from details provided in <<diff-info,DiffInfo>>.
|`size_delta` ||
Number of bytes by which the file size increased/decreased.
-|`size` ||
-File size in bytes.
+|`size` || File size in bytes.
+|`old_mode` |optional|File mode in octal (e.g. 100644) at the old commit.
+The first three digits indicate the file type and the last three digits contain
+the file permission bits. For added files, this field will not be present.
+|`new_mode` |optional|File mode in octal (e.g. 100644) at the new commit.
+The first three digits indicate the file type and the last three digits contain
+the file permission bits. For deleted files, this field will not be present.
|=============================
[[fix-input]]
diff --git a/java/com/google/gerrit/acceptance/PushOneCommit.java b/java/com/google/gerrit/acceptance/PushOneCommit.java
index d46fb78..8a53184 100644
--- a/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -287,6 +287,19 @@
return this;
}
+ public PushOneCommit addFile(String path, String content, int fileMode) throws Exception {
+ RevBlob blobId = testRepo.blob(content);
+ commitBuilder.edit(
+ new PathEdit(path) {
+ @Override
+ public void apply(DirCacheEntry ent) {
+ ent.setFileMode(FileMode.fromBits(fileMode));
+ ent.setObjectId(blobId);
+ }
+ });
+ return this;
+ }
+
public PushOneCommit addSymlink(String path, String target) throws Exception {
RevBlob blobId = testRepo.blob(target);
commitBuilder.edit(
diff --git a/java/com/google/gerrit/entities/Patch.java b/java/com/google/gerrit/entities/Patch.java
index 2d28046..3c33490 100644
--- a/java/com/google/gerrit/entities/Patch.java
+++ b/java/com/google/gerrit/entities/Patch.java
@@ -168,33 +168,40 @@
*/
public enum FileMode implements CodedEnum {
/** Mode indicating an entry is a tree (aka directory). */
- TREE('T'),
+ TREE('T', 0040000),
/** Mode indicating an entry is a symbolic link. */
- SYMLINK('S'),
+ SYMLINK('S', 0120000),
/** Mode indicating an entry is a non-executable file. */
- REGULAR_FILE('R'),
+ REGULAR_FILE('R', 0100644),
/** Mode indicating an entry is an executable file. */
- EXECUTABLE_FILE('E'),
+ EXECUTABLE_FILE('E', 0100755),
/** Mode indicating an entry is a submodule commit in another repository. */
- GITLINK('G'),
+ GITLINK('G', 0160000),
/** Mode indicating an entry is missing during parallel walks. */
- MISSING('M');
+ MISSING('M', 0000000);
private final char code;
- FileMode(char c) {
+ private final int mode;
+
+ FileMode(char c, int m) {
code = c;
+ mode = m;
}
@Override
public char getCode() {
return code;
}
+
+ public int getMode() {
+ return mode;
+ }
}
private Patch() {}
diff --git a/java/com/google/gerrit/extensions/common/FileInfo.java b/java/com/google/gerrit/extensions/common/FileInfo.java
index c732663..9526fbb 100644
--- a/java/com/google/gerrit/extensions/common/FileInfo.java
+++ b/java/com/google/gerrit/extensions/common/FileInfo.java
@@ -18,6 +18,8 @@
public class FileInfo {
public Character status;
+ public Integer oldMode;
+ public Integer newMode;
public Boolean binary;
public String oldPath;
public Integer linesInserted;
diff --git a/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java b/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
index d011d5d..180a94691 100644
--- a/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
+++ b/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
@@ -45,6 +45,16 @@
return check("linesDeleted").that(fileInfo.linesDeleted);
}
+ public IntegerSubject oldMode() {
+ isNotNull();
+ return check("oldMode").that(fileInfo.oldMode);
+ }
+
+ public IntegerSubject newMode() {
+ isNotNull();
+ return check("newMode").that(fileInfo.newMode);
+ }
+
public ComparableSubject<Character> status() {
isNotNull();
return check("status").that(fileInfo.status);
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
index 44b4ded..0f9194f 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
@@ -102,6 +102,14 @@
fileInfo.oldPath = FilePathAdapter.getOldPath(fileDiff.oldPath(), fileDiff.changeType());
fileInfo.sizeDelta = fileDiff.sizeDelta();
fileInfo.size = fileDiff.size();
+ fileInfo.oldMode =
+ fileDiff.oldMode().isPresent() && !fileDiff.oldMode().get().equals(Patch.FileMode.MISSING)
+ ? fileDiff.oldMode().get().getMode()
+ : null;
+ fileInfo.newMode =
+ fileDiff.newMode().isPresent() && !fileDiff.newMode().get().equals(Patch.FileMode.MISSING)
+ ? fileDiff.newMode().get().getMode()
+ : null;
if (fileDiff.patchType().get() == Patch.PatchType.BINARY) {
fileInfo.binary = true;
} else {
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index 3e4e72d..d1bda5c 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -97,7 +97,7 @@
persist(DIFF, FileDiffCacheKey.class, FileDiffOutput.class)
.maximumWeight(10 << 20)
.weigher(FileDiffWeigher.class)
- .version(8)
+ .version(9)
.keySerializer(FileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(FileDiffOutput.Serializer.INSTANCE)
.loader(FileDiffLoader.class);
@@ -443,6 +443,8 @@
.patchType(mainGitDiff.patchType())
.oldPath(mainGitDiff.oldPath())
.newPath(mainGitDiff.newPath())
+ .oldMode(mainGitDiff.oldMode())
+ .newMode(mainGitDiff.newMode())
.headerLines(FileHeaderUtil.getHeaderLines(mainGitDiff.fileHeader()))
.edits(asTaggedEdits(mainGitDiff.edits(), rebaseEdits))
.size(newSize)
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index 31fe77a..9286f47 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -17,9 +17,12 @@
import static com.google.gerrit.server.patch.DiffUtil.stringSize;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Converter;
+import com.google.common.base.Enums;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.FileMode;
import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
@@ -61,6 +64,18 @@
*/
public abstract Optional<String> newPath();
+ /**
+ * The file mode of the old file at the old git tree diff identified by {@link #oldCommitId()}
+ * ()}.
+ */
+ public abstract Optional<Patch.FileMode> oldMode();
+
+ /**
+ * The file mode of the new file at the new git tree diff identified by {@link #newCommitId()}
+ * ()}.
+ */
+ public abstract Optional<Patch.FileMode> newMode();
+
/** The change type of the underlying file, e.g. added, deleted, renamed, etc... */
public abstract Patch.ChangeType changeType();
@@ -201,6 +216,10 @@
public abstract Builder newPath(Optional<String> value);
+ public abstract Builder oldMode(Optional<Patch.FileMode> oldMode);
+
+ public abstract Builder newMode(Optional<Patch.FileMode> newMode);
+
public abstract Builder changeType(ChangeType value);
public abstract Builder patchType(Optional<PatchType> value);
@@ -221,6 +240,9 @@
public enum Serializer implements CacheSerializer<FileDiffOutput> {
INSTANCE;
+ private static final Converter<String, FileMode> FILE_MODE_CONVERTER =
+ Enums.stringConverter(Patch.FileMode.class);
+
private static final FieldDescriptor OLD_PATH_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(1);
@@ -233,6 +255,12 @@
private static final FieldDescriptor NEGATIVE_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(12);
+ private static final FieldDescriptor OLD_MODE_DESCRIPTOR =
+ FileDiffOutputProto.getDescriptor().findFieldByNumber(13);
+
+ private static final FieldDescriptor NEW_MODE_DESCRIPTOR =
+ FileDiffOutputProto.getDescriptor().findFieldByNumber(14);
+
@Override
public byte[] serialize(FileDiffOutput fileDiff) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
@@ -277,6 +305,13 @@
builder.setNegative(fileDiff.negative().get());
}
+ if (fileDiff.oldMode().isPresent()) {
+ builder.setOldMode(FILE_MODE_CONVERTER.reverse().convert(fileDiff.oldMode().get()));
+ }
+ if (fileDiff.newMode().isPresent()) {
+ builder.setNewMode(FILE_MODE_CONVERTER.reverse().convert(fileDiff.newMode().get()));
+ }
+
return Protos.toByteArray(builder.build());
}
@@ -318,6 +353,12 @@
if (proto.hasField(NEGATIVE_DESCRIPTOR)) {
builder.negative(Optional.of(proto.getNegative()));
}
+ if (proto.hasField(OLD_MODE_DESCRIPTOR)) {
+ builder.oldMode(Optional.of(FILE_MODE_CONVERTER.convert(proto.getOldMode())));
+ }
+ if (proto.hasField(NEW_MODE_DESCRIPTOR)) {
+ builder.newMode(Optional.of(FILE_MODE_CONVERTER.convert(proto.getNewMode())));
+ }
return builder.build();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 1919810..fca2253 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -227,6 +227,66 @@
}
@Test
+ public void fileModeChangeIsIncludedInListFilesDiff() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev1 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+ push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, result.getChangeId())
+ .addFile(fileName, "content", /* fileMode= */ 0100755);
+ result = push.to("refs/for/master");
+ String commitRev2 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev2).files(commitRev1);
+
+ assertThat(changedFiles.get(fileName)).oldMode().isEqualTo(0100644);
+ assertThat(changedFiles.get(fileName)).newMode().isEqualTo(0100755);
+ }
+
+ @Test
+ public void fileMode_oldMode_isMissingInListFilesDiff_forAddedFile() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev).files();
+
+ assertThat(changedFiles.get(fileName)).oldMode().isNull();
+ assertThat(changedFiles.get(fileName)).newMode().isEqualTo(0100644);
+ }
+
+ @Test
+ public void fileMode_newMode_isMissingInListFilesDiff_forDeletedFile() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev1 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+ push = pushFactory.create(admin.newIdent(), testRepo, result.getChangeId()).rmFile(fileName);
+ result = push.to("refs/for/master");
+ String commitRev2 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev2).files(commitRev1);
+
+ assertThat(changedFiles.get(fileName)).oldMode().isEqualTo(0100644);
+ assertThat(changedFiles.get(fileName)).newMode().isNull();
+ }
+
+ @Test
public void numberOfLinesInDiffOfDeletedFileWithoutNewlineAtEndIsCorrect() throws Exception {
String filePath = "a_new_file.txt";
String fileContent = "Line 1\nLine 2\nLine 3";
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
index c5e8574..00272112 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.FileMode;
import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.filediff.Edit;
@@ -42,6 +43,8 @@
.comparisonType(ComparisonType.againstOtherPatchSet())
.oldPath(Optional.of("old_file_path.txt"))
.newPath(Optional.empty())
+ .oldMode(Optional.of(FileMode.REGULAR_FILE))
+ .newMode(Optional.of(FileMode.SYMLINK))
.changeType(ChangeType.DELETED)
.patchType(Optional.of(PatchType.UNIFIED))
.size(23)
diff --git a/proto/cache.proto b/proto/cache.proto
index 1c37567..10b78d9 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -693,7 +693,7 @@
// Serialized form of
// com.google.gerrit.server.patch.filediff.FileDiffOutput
-// Next ID: 13
+// Next ID: 15
message FileDiffOutputProto {
// Next ID: 5
message Edit {
@@ -725,4 +725,6 @@
bytes new_commit = 10;
ComparisonType comparison_type = 11;
bool negative = 12;
+ string old_mode = 13; // ENUM as string
+ string new_mode = 14; // ENUM as string
}
