diff --git a/scm-core/src/main/java/sonia/scm/repository/api/ModifyCommandBuilder.java b/scm-core/src/main/java/sonia/scm/repository/api/ModifyCommandBuilder.java index 501a2ac3b0..9cfcaebd6a 100644 --- a/scm-core/src/main/java/sonia/scm/repository/api/ModifyCommandBuilder.java +++ b/scm-core/src/main/java/sonia/scm/repository/api/ModifyCommandBuilder.java @@ -4,10 +4,13 @@ import com.google.common.base.Preconditions; import com.google.common.io.ByteSource; import com.google.common.io.ByteStreams; import com.google.common.io.Files; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.repository.Person; import sonia.scm.repository.spi.ModifyCommand; import sonia.scm.repository.spi.ModifyCommandRequest; import sonia.scm.repository.util.WorkdirProvider; +import sonia.scm.util.IOUtil; import java.io.File; import java.io.IOException; @@ -40,6 +43,8 @@ import java.util.function.Consumer; */ public class ModifyCommandBuilder { + private static final Logger LOG = LoggerFactory.getLogger(ModifyCommandBuilder.class); + private final ModifyCommand command; private final File workdir; @@ -109,8 +114,16 @@ public class ModifyCommandBuilder { * @return The revision of the new commit. */ public String execute() { - Preconditions.checkArgument(request.isValid(), "commit message, author and branch are required"); - return command.execute(request); + try { + Preconditions.checkArgument(request.isValid(), "commit message, author and branch are required"); + return command.execute(request); + } finally { + try { + IOUtil.delete(workdir); + } catch (IOException e) { + LOG.warn("could not delete temporary workdir '{}'", workdir, e); + } + } } /** diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/ModifyCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/ModifyCommandRequest.java index 1ef92dab0a..958cd4a485 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/ModifyCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/ModifyCommandRequest.java @@ -1,8 +1,11 @@ package sonia.scm.repository.spi; import org.apache.commons.lang.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.Validateable; import sonia.scm.repository.Person; +import sonia.scm.util.IOUtil; import java.io.File; import java.io.IOException; @@ -12,6 +15,8 @@ import java.util.List; public class ModifyCommandRequest implements Resetable, Validateable { + private static final Logger LOG = LoggerFactory.getLogger(ModifyCommandRequest.class); + private final List requests = new ArrayList<>(); private Person author; @@ -108,7 +113,11 @@ public class ModifyCommandRequest implements Resetable, Validateable { } void cleanup() { - content.delete(); // TODO Handle errors + try { + IOUtil.delete(content); + } catch (IOException e) { + LOG.warn("could not delete temporary file {}", content, e); + } } } diff --git a/scm-core/src/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java b/scm-core/src/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java index e2dfb955a5..d88d720cdb 100644 --- a/scm-core/src/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java +++ b/scm-core/src/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java @@ -6,7 +6,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junitpioneer.jupiter.TempDirectory; import org.mockito.ArgumentCaptor; -import org.mockito.Captor; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.jupiter.MockitoExtension; @@ -43,20 +42,27 @@ class ModifyCommandBuilderTest { @Mock ModifyCommand.Worker worker; - @Captor - ArgumentCaptor requestCaptor; - ModifyCommandBuilder commandBuilder; + Path workdir; @BeforeEach void initWorkdir(@TempDirectory.TempDir Path temp) throws IOException { - lenient().when(workdirProvider.createNewWorkdir()).thenReturn(temp.toFile()); + workdir = Files.createDirectory(temp.resolve("workdir")); + lenient().when(workdirProvider.createNewWorkdir()).thenReturn(workdir.toFile()); commandBuilder = new ModifyCommandBuilder(command, workdirProvider); } @BeforeEach void initRequestCaptor() { - when(command.execute(requestCaptor.capture())).thenReturn("target"); + when(command.execute(any())).thenAnswer( + invocation -> { + ModifyCommandRequest request = invocation.getArgument(0); + for (ModifyCommandRequest.PartialRequest r : request.getRequests()) { + r.execute(worker); + } + return "target"; + } + ); } @Test @@ -74,8 +80,6 @@ class ModifyCommandBuilderTest { .deleteFile("toBeDeleted") .execute(); - executeRequest(); - verify(worker).delete("toBeDeleted"); } @@ -85,8 +89,6 @@ class ModifyCommandBuilderTest { .moveFile("source", "target") .execute(); - executeRequest(); - verify(worker).move("source", "target"); } @@ -100,8 +102,6 @@ class ModifyCommandBuilderTest { .createFile("toBeCreated").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeCreated"); assertThat(contentCaptor).contains("content"); } @@ -116,8 +116,6 @@ class ModifyCommandBuilderTest { .createFile("toBeCreated").withData(new ByteArrayInputStream("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeCreated"); assertThat(contentCaptor).contains("content"); } @@ -133,8 +131,6 @@ class ModifyCommandBuilderTest { .createFile("toBeCreated_2").withData(new ByteArrayInputStream("content_2".getBytes())) .execute(); - executeRequest(); - List createdNames = nameCaptor.getAllValues(); assertThat(createdNames.get(0)).isEqualTo("toBeCreated_1"); assertThat(createdNames.get(1)).isEqualTo("toBeCreated_2"); @@ -151,19 +147,10 @@ class ModifyCommandBuilderTest { .modifyFile("toBeModified").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeModified"); assertThat(contentCaptor).contains("content"); } - private void executeRequest() throws IOException { - ModifyCommandRequest request = requestCaptor.getValue(); - for (ModifyCommandRequest.PartialRequest r : request.getRequests()) { - r.execute(worker); - } - } - private ModifyCommandBuilder initCommand() { return commandBuilder .setBranch("branch") @@ -181,8 +168,6 @@ class ModifyCommandBuilderTest { .modifyFile("toBeModified").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(Files.list(temp)).isEmpty(); } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java index 5001c87e9b..e8574b7baf 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java @@ -66,6 +66,8 @@ public class GitModifyCommandTest extends AbstractGitCommandTestBase { request.addRequest(new ModifyCommandRequest.CreateFileRequest("new_file", newFile)); request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det")); + command.execute(request); + TreeAssertions assertions = canonicalTreeParser -> assertThat(canonicalTreeParser.findFile("new_file")).isTrue(); assertInTree(assertions);