Merge "Add a test for a diff case of a symlink converted to regular file"

java/com/google/gerrit/acceptance/PushOneCommit.java

@@ -43,8 +43,12 @@ import java.util.List;
 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 @@ public class PushOneCommit {
     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());

javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java

@@ -2744,6 +2744,55 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     assertThat(diffInfo).content().element(1).isDueToRebase();
   }
 
+  @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"));