Add a test for a diff case of a symlink converted to regular file
This change is adding a test for a corner bug that was found with
creating a symlink, deleting it, then recreating a regular file with the
same name.
The list files endpoint identifies the file as 'W' (short for REWRITE),
and the diff endpoint identifies it as DELETED. This is not good since
a malicious user could this to hide a bad change that the reviewer will
not be able to see. This test would help me understand and debug this
case further.
Change-Id: I62d3cbbc810ad53341e3a1d946a1be4710896c2e
diff --git a/java/com/google/gerrit/acceptance/PushOneCommit.java b/java/com/google/gerrit/acceptance/PushOneCommit.java
index afd451a..4215255 100644
--- a/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -43,8 +43,12 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import org.eclipse.jgit.api.TagCommand;
+import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit;
+import org.eclipse.jgit.dircache.DirCacheEntry;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevBlob;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RemoteRefUpdate;
@@ -277,6 +281,19 @@
return this;
}
+ public PushOneCommit addSymlink(String path, String target) throws Exception {
+ RevBlob blobId = testRepo.blob(target);
+ commitBuilder.edit(
+ new PathEdit(path) {
+ @Override
+ public void apply(DirCacheEntry ent) {
+ ent.setFileMode(FileMode.SYMLINK);
+ ent.setObjectId(blobId);
+ }
+ });
+ return this;
+ }
+
public Result to(String ref) throws Exception {
for (Map.Entry<String, String> e : files.entrySet()) {
commitBuilder.add(e.getKey(), e.getValue());
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 39366bd..ff785a2 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -2745,6 +2745,55 @@
}
@Test
+ public void symlinkConveredToRegularFileIsIdentifiedAsDeleted() throws Exception {
+ // TODO(ghareeb): See https://bugs.chromium.org/p/gerrit/issues/detail?id=13914.
+ // This test creates a corner scenario of replacing a symlink with a regular file
+ // of the same name. When both patchsets are diffed, the List Files endpoint identifies the
+ // file as a 'REWRITE', however the diff endpoint for the symlink file identifies the file as
+ // deleted. This case is a bit risky since it hides from the user the new content that was added
+ // in the new regular file. Ideally, the diff endpoint should show two entries for the deleted
+ // symlink and the added file, or only one entry "REWRITE" with the content that was added to
+ // the new file.
+ String target = "file.txt";
+ String symlink = "link.lnk";
+
+ // Create a change adding file "FileName" and a symlink "symLink" pointing to the file
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", target, "content")
+ .addSymlink(symlink, target);
+
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String initialRev = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ // Delete the symlink with patchset 2
+ gApi.changes().id(result.getChangeId()).edit().deleteFile(symlink);
+ gApi.changes().id(result.getChangeId()).edit().publish();
+
+ // Re-add the symlink as a regular file with patchset 3
+ gApi.changes()
+ .id(result.getChangeId())
+ .edit()
+ .modifyFile(symlink, RawInputUtil.create("Content of the new file named 'symlink'"));
+ gApi.changes().id(result.getChangeId()).edit().publish();
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).current().files(initialRev);
+
+ assertThat(changedFiles.keySet()).containsExactly("/COMMIT_MSG", symlink);
+ assertThat(changedFiles.get(symlink).status).isEqualTo('W'); // Rewrite
+
+ DiffInfo diffInfo =
+ gApi.changes().id(result.getChangeId()).current().file(symlink).diff(initialRev);
+
+ // TODO(ghareeb): This is not a desired behaviour. The diff endpoint treats the file as
+ // 'DELETED', hence hiding any new content that was added to the new regular file. It is better
+ // to show the file as 'ADDED'. See https://bugs.chromium.org/p/gerrit/issues/detail?id=13914
+ // for more details.
+ assertThat(diffInfo.changeType).isEqualTo(ChangeType.DELETED);
+ }
+
+ @Test
public void diffOfNonExistentFileIsAnEmptyDiffResult() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
