From b0267d69097f3964cfa466b45bc1568a091fb0c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 28 Aug 2019 15:10:38 +0200 Subject: [PATCH 1/2] Handle error on delete --- .../scm/repository/spi/ModifyCommandRequest.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) 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 f3dcca18ff..e8f2223bdd 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,16 +1,22 @@ 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; import java.util.ArrayList; import java.util.Collections; 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; @@ -107,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); + } } } From bb0682fab59dd1163fcf7a43f774b68833137ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 28 Aug 2019 17:04:22 +0200 Subject: [PATCH 2/2] Delete temporary workdir after command was executed --- .../repository/api/ModifyCommandBuilder.java | 17 +++++++-- .../api/ModifyCommandBuilderTest.java | 35 ++++++------------- 2 files changed, 25 insertions(+), 27 deletions(-) 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/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java b/scm-core/src/test/java/sonia/scm/repository/api/ModifyCommandBuilderTest.java index adbefe7d2a..7d4d96642a 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,25 @@ 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); + request.getRequests().forEach(r -> r.execute(worker)); + return "target"; + } + ); } @Test @@ -74,8 +78,6 @@ class ModifyCommandBuilderTest { .deleteFile("toBeDeleted") .execute(); - executeRequest(); - verify(worker).delete("toBeDeleted"); } @@ -85,8 +87,6 @@ class ModifyCommandBuilderTest { .moveFile("source", "target") .execute(); - executeRequest(); - verify(worker).move("source", "target"); } @@ -100,8 +100,6 @@ class ModifyCommandBuilderTest { .createFile("toBeCreated").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeCreated"); assertThat(contentCaptor).contains("content"); } @@ -116,8 +114,6 @@ class ModifyCommandBuilderTest { .createFile("toBeCreated").withData(new ByteArrayInputStream("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeCreated"); assertThat(contentCaptor).contains("content"); } @@ -133,8 +129,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,17 +145,10 @@ class ModifyCommandBuilderTest { .modifyFile("toBeModified").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(nameCaptor.getValue()).isEqualTo("toBeModified"); assertThat(contentCaptor).contains("content"); } - private void executeRequest() { - ModifyCommandRequest request = requestCaptor.getValue(); - request.getRequests().forEach(r -> r.execute(worker)); - } - private ModifyCommandBuilder initCommand() { return commandBuilder .setBranch("branch") @@ -179,8 +166,6 @@ class ModifyCommandBuilderTest { .modifyFile("toBeModified").withData(ByteSource.wrap("content".getBytes())) .execute(); - executeRequest(); - assertThat(Files.list(temp)).isEmpty(); }