From a336d2c6767b8e4c6a98d0c27d17bcc6ee3df2c7 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Thu, 29 Oct 2020 14:52:05 +0100 Subject: [PATCH 1/3] Fix svn diff with property changes --- CHANGELOG.md | 1 + .../repository/spi/SCMSvnDiffGenerator.java | 81 +++++++- .../repository/spi/SvnDiffCommandTest.java | 196 ++++++++++++++++++ 3 files changed, 271 insertions(+), 7 deletions(-) create mode 100644 scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ec0bf37b40..6ea46299c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Internal server error for git sub modules without tree object ([#1397](https://github.com/scm-manager/scm-manager/pull/1397)) - Do not expose subversion commit with id 0 ([#1395](https://github.com/scm-manager/scm-manager/pull/1395)) +- SVN diff with property changes ([#1400](https://github.com/scm-manager/scm-manager/pull/1400)) ## [2.8.0] - 2020-10-27 ### Added diff --git a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java index a353ea489f..2af1cb57ee 100644 --- a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java +++ b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.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.spi; import de.regnis.q.sequence.line.diff.QDiffGenerator; @@ -56,6 +56,7 @@ import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.RandomAccessFile; +import java.io.StringWriter; import java.io.UnsupportedEncodingException; import java.io.Writer; import java.util.ArrayList; @@ -344,16 +345,17 @@ public class SCMSvnDiffGenerator implements ISvnDiffGenerator { String label1 = getLabel(newTargetString1, revision1); String label2 = getLabel(newTargetString2, revision2); - boolean shouldStopDisplaying = displayHeader(outputStream, displayPath, false, fallbackToAbsolutePath, SvnDiffCallback.OperationKind.Modified); visitedPaths.add(displayPath); if (useGitFormat) { displayGitDiffHeader(outputStream, SvnDiffCallback.OperationKind.Modified, getRelativeToRootPath(target, originalTarget1), getRelativeToRootPath(target, originalTarget2), null); - } - if (shouldStopDisplaying) { - return; + } else { + boolean shouldStopDisplaying = displayHeader(outputStream, displayPath, false, fallbackToAbsolutePath, SvnDiffCallback.OperationKind.Modified); + if (shouldStopDisplaying) { + return; + } } // if (useGitFormat) { @@ -374,9 +376,74 @@ public class SCMSvnDiffGenerator implements ISvnDiffGenerator { } } - displayPropertyChangesOn(useGitFormat ? getRelativeToRootPath(target, originalTarget1) : displayPath, outputStream); + if (useGitFormat) { + displayGitPropDiffValues(outputStream, propChanges, originalProps); + } else { + displayPropertyChangesOn(displayPath, outputStream); + displayPropDiffValues(outputStream, propChanges, originalProps); + } + } + + private void displayGitPropDiffValues(OutputStream outputStream, SVNProperties diff, SVNProperties baseProps) throws SVNException { + for (Iterator changedPropNames = diff.nameSet().iterator(); changedPropNames.hasNext(); ) { + String name = (String) changedPropNames.next(); + SVNPropertyValue originalValue = baseProps != null ? baseProps.getSVNPropertyValue(name) : null; + SVNPropertyValue newValue = diff.getSVNPropertyValue(name); + + try { + byte[] originalValueBytes = getPropertyAsBytes(originalValue, getEncoding()); + byte[] newValueBytes = getPropertyAsBytes(newValue, getEncoding()); + + if (originalValueBytes == null) { + originalValueBytes = new byte[0]; + } else { + originalValueBytes = maybeAppendEOL(originalValueBytes); + } + + boolean newValueHadEol = newValueBytes != null && newValueBytes.length > 0 && + (newValueBytes[newValueBytes.length - 1] == SVNProperty.EOL_CR_BYTES[0] || + newValueBytes[newValueBytes.length - 1] == SVNProperty.EOL_LF_BYTES[0]); + + if (newValueBytes == null) { + newValueBytes = new byte[0]; + } else { + newValueBytes = maybeAppendEOL(newValueBytes); + } + + QDiffUniGenerator.setup(); + Map properties = new SVNHashMap(); + + properties.put(QDiffGeneratorFactory.IGNORE_EOL_PROPERTY, Boolean.valueOf(getDiffOptions().isIgnoreEOLStyle())); + properties.put(QDiffGeneratorFactory.EOL_PROPERTY, new String(getEOL())); + properties.put(QDiffGeneratorFactory.HUNK_DELIMITER, "@@"); + if (getDiffOptions().isIgnoreAllWhitespace()) { + properties.put(QDiffGeneratorFactory.IGNORE_SPACE_PROPERTY, QDiffGeneratorFactory.IGNORE_ALL_SPACE); + } else if (getDiffOptions().isIgnoreAmountOfWhitespace()) { + properties.put(QDiffGeneratorFactory.IGNORE_SPACE_PROPERTY, QDiffGeneratorFactory.IGNORE_SPACE_CHANGE); + } + + QDiffGenerator generator = new QDiffUniGenerator(properties, ""); + StringWriter writer = new StringWriter(); + QDiffManager.generateTextDiff(new ByteArrayInputStream(originalValueBytes), new ByteArrayInputStream(newValueBytes), + null, writer, generator); + writer.flush(); + + String lines[] = writer.toString().split("\\r?\\n"); + displayString(outputStream, lines[0] + "\n"); + displayString(outputStream, " # property " + name + " has changed\n"); + for (int i=1; i< lines.length; i++) { + displayString(outputStream, lines[i] + "\n"); + } + + if (!newValueHadEol) { + displayString(outputStream, "\\ No newline at end of property"); + displayEOL(outputStream); + } + } catch (IOException e) { + wrapException(e); + } + } - displayPropDiffValues(outputStream, propChanges, originalProps); } private void throwBadRelativePathException(String displayPath, String relativeToPath) throws SVNException { diff --git a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java new file mode 100644 index 0000000000..50fff9af4a --- /dev/null +++ b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java @@ -0,0 +1,196 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.repository.spi; + +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.tmatesoft.svn.core.SVNDepth; +import org.tmatesoft.svn.core.SVNException; +import org.tmatesoft.svn.core.SVNPropertyValue; +import org.tmatesoft.svn.core.SVNURL; +import org.tmatesoft.svn.core.io.SVNRepositoryFactory; +import org.tmatesoft.svn.core.wc.SVNClientManager; +import org.tmatesoft.svn.core.wc.SVNRevision; +import sonia.scm.repository.RepositoryTestData; +import sonia.scm.repository.api.DiffFormat; + +import javax.annotation.Nonnull; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +class SvnDiffCommandTest { + + private final SVNClientManager client = SVNClientManager.newInstance(); + + private File repository; + private File workingCopy; + + @BeforeEach + void setUpDirectories(@TempDir Path directory) { + repository = directory.resolve("repository").toFile(); + workingCopy = directory.resolve("working-copy").toFile(); + } + + @Test + void shouldCreateGitCompatibleDiffForSinglePropChanges() throws SVNException, IOException { + createRepository(); + commitProperty("scm:awesome", "shit"); + + String diff = gitDiff("1"); + + assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", + "diff --git a/ b/", + "--- a/", + "+++ b/", + "@@ -0,0 +1 @@", + " # property scm:awesome has changed", + "+shit", + "\\ No newline at end of property" + )); + } + + @Test + void shouldCreateGitCompatibleDiffForPropChanges() throws SVNException, IOException { + createRepository(); + commitProperties(ImmutableMap.of("one", "eins", "two", "zwei", "three", "drei")); + + String diff = gitDiff("1"); + + System.out.println(diff); + + assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", + "diff --git a/ b/", + "--- a/", + "+++ b/", + "@@ -0,0 +1 @@", + " # property one has changed", + "+eins", + "\\ No newline at end of property", + "@@ -0,0 +1 @@", + " # property two has changed", + "+zwei", + "\\ No newline at end of property", + "@@ -0,0 +1 @@", + " # property three has changed", + "+drei", + "\\ No newline at end of property" + )); + } + + @Test + void shouldCreateGitCompatibleDiffForModifiedProp() throws SVNException, IOException { + createRepository(); + commitProperty("scm:spaceship", "Razor Crest"); + commitProperty("scm:spaceship", "Heart Of Gold"); + + String diff = gitDiff("2"); + + System.out.println(diff); + + assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", + "diff --git a/ b/", + "--- a/", + "+++ b/", + "@@ -1 +1 @@", + " # property scm:spaceship has changed", + "-Razor Crest", + "+Heart Of Gold", + "\\ No newline at end of property" + )); + } + + @Nonnull + private String gitDiff(String revision) throws IOException { + SvnDiffCommand command = createCommand(); + DiffCommandRequest request = new DiffCommandRequest(); + request.setFormat(DiffFormat.GIT); + request.setRevision(revision); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + command.getDiffResult(request).accept(baos); + return baos.toString(); + } + + private SvnDiffCommand createCommand() { + return new SvnDiffCommand(new SvnContext(RepositoryTestData.createHeartOfGold(), repository)); + } + + private void commitProperty(String name, String value) throws SVNException { + setProperty(name, value); + commit("set property " + name + " = " + value); + } + + private void commit(String message) throws SVNException { + client.getCommitClient().doCommit( + new File[]{workingCopy}, + false, + message, + null, + null, + false, + false, + SVNDepth.UNKNOWN + ); + } + + private void setProperty(String name, String value) throws SVNException { + client.getWCClient().doSetProperty( + workingCopy, + name, + SVNPropertyValue.create(value), + true, + SVNDepth.UNKNOWN, + null, + null + ); + } + + private void commitProperties(Map properties) throws SVNException { + for (Map.Entry e : properties.entrySet()) { + setProperty(e.getKey(), e.getValue()); + } + commit("set " + properties.size() + " properties"); + } + + private void createRepository() throws SVNException { + SVNURL url = SVNRepositoryFactory.createLocalRepository(repository, true, false); + client.getUpdateClient().doCheckout( + url, + workingCopy, + SVNRevision.HEAD, + SVNRevision.HEAD, + SVNDepth.INFINITY, + true + ); + } + +} From 7e575036a3d645900baf221c85e0d0fc3ba2f5bb Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Thu, 29 Oct 2020 16:27:36 +0100 Subject: [PATCH 2/3] Exclude SCMSvnDiffGenerator from SonarQube duplication report SCMSvnDiffGenerator is a copy of an internal SVNKit class, with small changes for SCM-Manger. If we refactor the class, it could become very hard to merge it with upstream. So we decided to stop duplication reports for this class. --- scm-plugins/scm-svn-plugin/pom.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scm-plugins/scm-svn-plugin/pom.xml b/scm-plugins/scm-svn-plugin/pom.xml index e78a8221bb..254fa6730d 100644 --- a/scm-plugins/scm-svn-plugin/pom.xml +++ b/scm-plugins/scm-svn-plugin/pom.xml @@ -90,6 +90,15 @@ + + + **/SCMSvnDiffGenerator.java + + From 5de4b49392f8b941588e3bcd0b31b12c17ad906d Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 2 Nov 2020 09:16:47 +0100 Subject: [PATCH 3/3] Improve display of binary values in svn diff --- .../repository/spi/SCMSvnDiffGenerator.java | 14 ++++-- .../repository/spi/SvnDiffCommandTest.java | 43 ++++++++++++++++--- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java index 2af1cb57ee..81e40023f6 100644 --- a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java +++ b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SCMSvnDiffGenerator.java @@ -391,8 +391,8 @@ public class SCMSvnDiffGenerator implements ISvnDiffGenerator { SVNPropertyValue newValue = diff.getSVNPropertyValue(name); try { - byte[] originalValueBytes = getPropertyAsBytes(originalValue, getEncoding()); - byte[] newValueBytes = getPropertyAsBytes(newValue, getEncoding()); + byte[] originalValueBytes = getPropertyAsBytes(originalValue, getEncoding(), true); + byte[] newValueBytes = getPropertyAsBytes(newValue, getEncoding(), true); if (originalValueBytes == null) { originalValueBytes = new byte[0]; @@ -1190,6 +1190,10 @@ public class SCMSvnDiffGenerator implements ISvnDiffGenerator { } private byte[] getPropertyAsBytes(SVNPropertyValue value, String encoding) { + return getPropertyAsBytes(value, encoding, false); + } + + private byte[] getPropertyAsBytes(SVNPropertyValue value, String encoding, boolean replaceBinary) { if (value == null) { return null; } @@ -1200,7 +1204,11 @@ public class SCMSvnDiffGenerator implements ISvnDiffGenerator { return value.getString().getBytes(); } } - return value.getBytes(); + if (replaceBinary) { + return String.format("Binary value (%s bytes)", value.getBytes().length).getBytes(); + } else { + return value.getBytes(); + } } private void displayMergeInfoDiff(OutputStream outputStream, String oldValue, String newValue) throws SVNException, IOException { diff --git a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java index 50fff9af4a..e7a46b18be 100644 --- a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java +++ b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnDiffCommandTest.java @@ -43,12 +43,16 @@ import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; import java.nio.file.Path; +import java.util.Base64; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; class SvnDiffCommandTest { + // the smallest gif of the world + private static final String GIF = "R0lGODlhAQABAIABAP///wAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="; + private final SVNClientManager client = SVNClientManager.newInstance(); private File repository; @@ -85,8 +89,6 @@ class SvnDiffCommandTest { String diff = gitDiff("1"); - System.out.println(diff); - assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", "diff --git a/ b/", "--- a/", @@ -114,8 +116,6 @@ class SvnDiffCommandTest { String diff = gitDiff("2"); - System.out.println(diff); - assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", "diff --git a/ b/", "--- a/", @@ -128,6 +128,26 @@ class SvnDiffCommandTest { )); } + @Test + void shouldCreateGitCompatibleDiffForBinaryProps() throws SVNException, IOException { + createRepository(); + + byte[] gif = Base64.getDecoder().decode(GIF); + commitProperty("scm:gif", gif); + + String diff = gitDiff("1"); + + assertThat(diff).isEqualToIgnoringNewLines(String.join("\n", + "diff --git a/ b/", + "--- a/", + "+++ b/", + "@@ -0,0 +1 @@", + " # property scm:gif has changed", + "+Binary value (43 bytes)", + "\\ No newline at end of property" + )); + } + @Nonnull private String gitDiff(String revision) throws IOException { SvnDiffCommand command = createCommand(); @@ -145,6 +165,15 @@ class SvnDiffCommandTest { } private void commitProperty(String name, String value) throws SVNException { + setProperty(name, SVNPropertyValue.create(value)); + commit("set property " + name + " = " + value); + } + + private void commitProperty(String name, byte[] value) throws SVNException { + commitProperty(name, SVNPropertyValue.create(name, value)); + } + + private void commitProperty(String name, SVNPropertyValue value) throws SVNException { setProperty(name, value); commit("set property " + name + " = " + value); } @@ -162,11 +191,11 @@ class SvnDiffCommandTest { ); } - private void setProperty(String name, String value) throws SVNException { + private void setProperty(String name, SVNPropertyValue value) throws SVNException { client.getWCClient().doSetProperty( workingCopy, name, - SVNPropertyValue.create(value), + value, true, SVNDepth.UNKNOWN, null, @@ -176,7 +205,7 @@ class SvnDiffCommandTest { private void commitProperties(Map properties) throws SVNException { for (Map.Entry e : properties.entrySet()) { - setProperty(e.getKey(), e.getValue()); + setProperty(e.getKey(), SVNPropertyValue.create(e.getValue())); } commit("set " + properties.size() + " properties"); }