From 054f320455e3b5cc0adf80bd12df08d6a3781a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 19 May 2020 23:08:19 +0200 Subject: [PATCH] Make change types explicit Without explicit change types, we cannot tell copy and rename apart. --- .../sonia/scm/repository/api/DiffFile.java | 8 ++- .../scm/repository/spi/GitDiffCommand.java | 6 ++- .../repository/spi/GitDiffResultCommand.java | 18 +++++++ .../DiffResultToDiffResultDtoMapper.java | 49 +++++++++---------- .../DiffResultToDiffResultDtoMapperTest.java | 28 ++++++++++- 5 files changed, 81 insertions(+), 28 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/api/DiffFile.java b/scm-core/src/main/java/sonia/scm/repository/api/DiffFile.java index b5cbd08ec7..fb1d102846 100644 --- a/scm-core/src/main/java/sonia/scm/repository/api/DiffFile.java +++ b/scm-core/src/main/java/sonia/scm/repository/api/DiffFile.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.api; public interface DiffFile extends Iterable { @@ -33,4 +33,10 @@ public interface DiffFile extends Iterable { String getOldPath(); String getNewPath(); + + ChangeType getChangeType(); + + enum ChangeType { + ADD, MODIFY, DELETE, RENAME, COPY + } } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffCommand.java index 0e851456a7..adb7a7bd0e 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffCommand.java @@ -57,7 +57,7 @@ public class GitDiffCommand extends AbstractGitCommand implements DiffCommand { formatter.setRepository(repository); for (DiffEntry e : diff.getEntries()) { - if (!e.getOldId().equals(e.getNewId()) || !e.getNewPath().equals(e.getOldPath())) { + if (idOrPathChanged(e)) { formatter.format(e); } } @@ -67,6 +67,10 @@ public class GitDiffCommand extends AbstractGitCommand implements DiffCommand { }; } + private boolean idOrPathChanged(DiffEntry e) { + return !e.getOldId().equals(e.getNewId()) || !e.getNewPath().equals(e.getOldPath()); + } + static class DequoteOutputStream extends OutputStream { private static final String[] DEQUOTE_STARTS = { diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffResultCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffResultCommand.java index e0cbffbc2a..fed865c576 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffResultCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffResultCommand.java @@ -108,6 +108,24 @@ public class GitDiffResultCommand extends AbstractGitCommand implements DiffResu return diffEntry.getNewPath(); } + @Override + public ChangeType getChangeType() { + switch (diffEntry.getChangeType()) { + case ADD: + return ChangeType.ADD; + case MODIFY: + return ChangeType.MODIFY; + case RENAME: + return ChangeType.RENAME; + case DELETE: + return ChangeType.DELETE; + case COPY: + return ChangeType.COPY; + default: + throw new IllegalArgumentException("Unknown change type: " + diffEntry.getChangeType()); + } + } + @Override public Iterator iterator() { String content = format(repository, diffEntry); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapper.java index 2c17abd30f..5057950149 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapper.java @@ -26,7 +26,6 @@ package sonia.scm.api.v2.resources; import com.github.sdorra.spotter.ContentTypes; import com.github.sdorra.spotter.Language; -import com.google.common.base.Strings; import com.google.inject.Inject; import sonia.scm.repository.Repository; import sonia.scm.repository.api.DiffFile; @@ -42,7 +41,7 @@ import java.util.OptionalInt; import static de.otto.edison.hal.Links.linkingTo; /** - * TODO conflicts, copy and rename + * TODO conflicts */ class DiffResultToDiffResultDtoMapper { @@ -83,21 +82,29 @@ class DiffResultToDiffResultDtoMapper { String oldPath = file.getOldPath(); String path; - if (isFilePath(newPath) && isFileNull(oldPath)) { - path = newPath; - dto.setType("add"); - } else if (isFileNull(newPath) && isFilePath(oldPath)) { - path = oldPath; - dto.setType("delete"); - } else if (!newPath.equals(oldPath)) { - path = newPath; - dto.setType("rename"); - } else if (isFilePath(newPath) && isFilePath(oldPath)) { - path = newPath; - dto.setType("modify"); - } else { - // TODO copy? - throw new IllegalStateException("no file without path"); + switch (file.getChangeType()) { + case ADD: + path = newPath; + dto.setType("add"); + break; + case DELETE: + path = oldPath; + dto.setType("delete"); + break; + case RENAME: + path = newPath; + dto.setType("rename"); + break; + case MODIFY: + path = newPath; + dto.setType("modify"); + break; + case COPY: + path = newPath; + dto.setType("copy"); + break; + default: + throw new IllegalArgumentException("unknown change type: " + file.getChangeType()); } dto.setNewPath(newPath); @@ -119,14 +126,6 @@ class DiffResultToDiffResultDtoMapper { return dto; } - private boolean isFilePath(String path) { - return !isFileNull(path); - } - - private boolean isFileNull(String path) { - return Strings.isNullOrEmpty(path) || "/dev/null".equals(path); - } - private DiffResultDto.HunkDto mapHunk(Hunk hunk) { DiffResultDto.HunkDto dto = new DiffResultDto.HunkDto(); dto.setContent(hunk.getRawHeader()); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapperTest.java index 5b0ff6eff9..291b04e00c 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapperTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DiffResultToDiffResultDtoMapperTest.java @@ -63,6 +63,7 @@ class DiffResultToDiffResultDtoMapperTest { assertModifiedFile(files.get(1), "B.ts", "abc", "def", "typescript"); assertDeletedFile(files.get(2), "C.go", "ghi", "golang"); assertRenamedFile(files.get(3), "typo.ts", "okay.ts", "def", "fixed", "typescript"); + assertCopiedFile(files.get(4), "good.ts", "better.ts", "def", "fixed", "typescript"); DiffResultDto.HunkDto hunk = files.get(1).getHunks().get(0); assertHunk(hunk, "@@ -3,4 1,2 @@", 1, 2, 3, 4); @@ -108,7 +109,8 @@ class DiffResultToDiffResultDtoMapperTest { ) ), deletedFile("C.go", "ghi"), - renamedFile("okay.ts", "typo.ts", "fixed", "def") + renamedFile("okay.ts", "typo.ts", "fixed", "def"), + copiedFile("better.ts", "good.ts", "fixed", "def") ); } @@ -174,6 +176,15 @@ class DiffResultToDiffResultDtoMapperTest { assertThat(file.getLanguage()).isEqualTo(language); } + private void assertCopiedFile(DiffResultDto.FileDto file, String oldPath, String newPath, String oldRevision, String newRevision, String language) { + assertThat(file.getOldPath()).isEqualTo(oldPath); + assertThat(file.getNewPath()).isEqualTo(newPath); + assertThat(file.getOldRevision()).isEqualTo(oldRevision); + assertThat(file.getNewRevision()).isEqualTo(newRevision); + assertThat(file.getType()).isEqualTo("copy"); + assertThat(file.getLanguage()).isEqualTo(language); + } + private DiffResult result(DiffFile... files) { DiffResult result = mock(DiffResult.class); when(result.iterator()).thenReturn(Arrays.asList(files).iterator()); @@ -184,6 +195,7 @@ class DiffResultToDiffResultDtoMapperTest { DiffFile file = mock(DiffFile.class); when(file.getNewPath()).thenReturn(path); when(file.getNewRevision()).thenReturn(revision); + when(file.getChangeType()).thenReturn(DiffFile.ChangeType.ADD); when(file.iterator()).thenReturn(Arrays.asList(hunks).iterator()); return file; } @@ -192,6 +204,7 @@ class DiffResultToDiffResultDtoMapperTest { DiffFile file = mock(DiffFile.class); when(file.getOldPath()).thenReturn(path); when(file.getOldRevision()).thenReturn(revision); + when(file.getChangeType()).thenReturn(DiffFile.ChangeType.DELETE); when(file.iterator()).thenReturn(Arrays.asList(hunks).iterator()); return file; } @@ -202,6 +215,7 @@ class DiffResultToDiffResultDtoMapperTest { when(file.getNewRevision()).thenReturn(newRevision); when(file.getOldPath()).thenReturn(path); when(file.getOldRevision()).thenReturn(oldRevision); + when(file.getChangeType()).thenReturn(DiffFile.ChangeType.MODIFY); when(file.iterator()).thenReturn(Arrays.asList(hunks).iterator()); return file; } @@ -212,6 +226,18 @@ class DiffResultToDiffResultDtoMapperTest { when(file.getNewRevision()).thenReturn(newRevision); when(file.getOldPath()).thenReturn(oldPath); when(file.getOldRevision()).thenReturn(oldRevision); + when(file.getChangeType()).thenReturn(DiffFile.ChangeType.RENAME); + when(file.iterator()).thenReturn(emptyIterator()); + return file; + } + + private DiffFile copiedFile(String newPath, String oldPath, String newRevision, String oldRevision) { + DiffFile file = mock(DiffFile.class); + when(file.getNewPath()).thenReturn(newPath); + when(file.getNewRevision()).thenReturn(newRevision); + when(file.getOldPath()).thenReturn(oldPath); + when(file.getOldRevision()).thenReturn(oldRevision); + when(file.getChangeType()).thenReturn(DiffFile.ChangeType.COPY); when(file.iterator()).thenReturn(emptyIterator()); return file; }