You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/03 01:22:47 UTC
[GitHub] [maven-shared-utils] roxspring commented on a change in pull request #28: [MSHARED-884] - Don't always overwrite filtered resources
roxspring commented on a change in pull request #28:
URL: https://github.com/apache/maven-shared-utils/pull/28#discussion_r419030042
##########
File path: src/main/java/org/apache/maven/shared/utils/io/FileUtils.java
##########
@@ -1826,34 +1831,101 @@ public static void copyFile( @Nonnull File from, @Nonnull File to, @Nullable Str
@Nullable FilterWrapper[] wrappers, boolean overwrite )
throws IOException
{
- if ( wrappers != null && wrappers.length > 0 )
+ if ( wrappers == null || wrappers.length <= 0 )
Review comment:
Flipped the condition around to get the simple case out of the way and ease the cognitive load when reading the else clause. Maybe I should split this out into a separate commit and make that final check `==` since negative lengths shouldn't be possible!
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1,29 +1,49 @@
package org.apache.maven.shared.utils.io;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.CoreMatchers.hasItems;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-import static org.junit.Assume.assumeFalse;
-import static org.junit.Assume.assumeThat;
-import static org.junit.Assume.assumeTrue;
+import org.apache.maven.shared.utils.Os;
+import org.apache.maven.shared.utils.testhelpers.FileTestHelper;
+import org.codehaus.plexus.util.InterpolationFilterReader;
+import org.hamcrest.CoreMatchers;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+import javax.annotation.Nonnull;
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
+import java.io.FileReader;
+import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.io.Reader;
+import java.io.Writer;
import java.net.URL;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Map;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.hasItems;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeThat;
+import static org.junit.Assume.assumeTrue;
Review comment:
Looks like Intellij moved some imports when I wasn't looking. Happy to fix if it's an issue.
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -438,9 +448,300 @@ public void copyFile1()
testFile1.lastModified() == destination.lastModified());*/
}
+ private static long MILLIS_PER_DAY = 24 * 60 * 60 * 1000L;
+
+ /** A time today, rounded down to the previous minute */
+ private static long MODIFIED_TODAY = (System.currentTimeMillis() / 60_000) * 60_000;
Review comment:
Throwing away seconds here allows these precise times to be used accurately even on low resolution file systems such as FAT32.
##########
File path: src/main/java/org/apache/maven/shared/utils/io/FileUtils.java
##########
@@ -1826,34 +1831,101 @@ public static void copyFile( @Nonnull File from, @Nonnull File to, @Nullable Str
@Nullable FilterWrapper[] wrappers, boolean overwrite )
throws IOException
{
- if ( wrappers != null && wrappers.length > 0 )
+ if ( wrappers == null || wrappers.length <= 0 )
{
-
- if ( encoding == null || encoding.isEmpty() )
+ if ( to.lastModified() < from.lastModified() || overwrite )
+ {
+ copyFile( from, to );
+ }
+ }
+ else
+ {
+ if ( encoding == null || encoding.isEmpty() )
{
encoding = Charset.defaultCharset().name();
}
-
+
// buffer so it isn't reading a byte at a time!
try ( Reader fileReader =
- new BufferedReader( new InputStreamReader( new FileInputStream( from ), encoding ) );
- Writer fileWriter = new OutputStreamWriter( new FileOutputStream( to ), encoding ) )
+ new BufferedReader( new InputStreamReader( new FileInputStream( from ), encoding ) ) )
{
-
Reader wrapped = fileReader;
for ( FilterWrapper wrapper : wrappers )
{
wrapped = wrapper.getReader( wrapped );
}
- IOUtil.copy( wrapped, fileWriter );
- }
- }
- else
- {
- if ( to.lastModified() < from.lastModified() || overwrite )
- {
- copyFile( from, to );
+ if ( overwrite || !to.exists() )
+ {
+ try ( Writer fileWriter = new OutputStreamWriter( new FileOutputStream( to ), encoding ) )
+ {
+ IOUtil.copy( wrapped, fileWriter );
+ }
+ }
+ else
+ {
+ CharsetEncoder encoder = Charset.forName( encoding ).newEncoder();
+
+ int totalBufferSize = FILE_COPY_BUFFER_SIZE;
+
+ int charBufferSize = ( int ) Math.floor( totalBufferSize / ( 2 + 2 * encoder.maxBytesPerChar() ) );
+ int byteBufferSize = ( int ) Math.ceil( charBufferSize * encoder.maxBytesPerChar() );
Review comment:
This is attempting to consume the same amount of buffer memory in total, but split between a `char` buffer and a pair of `byte` buffers - each big enough to contain the encoded `char` buffer.
##########
File path: src/main/java/org/apache/maven/shared/utils/io/FileUtils.java
##########
@@ -109,7 +114,7 @@ protected FileUtils()
/**
* The file copy buffer size (30 MB)
*/
- private static final long FILE_COPY_BUFFER_SIZE = ONE_MB * 30;
+ private static final int FILE_COPY_BUFFER_SIZE = ONE_MB * 30;
Review comment:
Don't think there was any need for this to be a `long` other than the only previous use case, where it's now automatically cast to a long anyway.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org