Fix three bugs with FileChannel.transferFrom:

1. `transferFrom` was not behaving according to the spec when given a position that was greater than the size of that file. The spec says that nothing is transferred in that case, but Jimfs was using the same behavior as `write` in this case: the file was first zero-filled from the start until the given position and then bytes were transferred starting there. Jimfs now simply returns 0 in this case.
2. When given a non-blocking channel that can return 0 bytes from a call to `read`, `transferFrom` was spinning until the input channel returned -1. The spec is for `transferFrom` to return immediately in that case.
3. When a transfer ended on a block boundary (i.e. after exactly filling one block) but still had bytes remaining based on the given `count`, an extra (empty) block would be left allocated on the end of the file. That block is now correctly de-allocated so it can be used elsewhere. Fixes #163.

Also simplify the implementation of `transferFrom` a bit.

RELNOTES=Fixed several bugs with the behavior of `FileChannel.transferFrom`.
PiperOrigin-RevId: 390208683

jimfs/src/main/java/com/google/common/jimfs/RegularFile.java

@@ -385,70 +385,66 @@ public long write(long pos, Iterable<ByteBuffer> bufs) throws IOException {
   /**
    * Transfers up to {@code count} bytes from the given channel to this file starting at position
    * {@code pos}. Returns the number of bytes transferred. If {@code pos} is greater than the
-   * current size of this file, the file is truncated up to size {@code pos} before writing.
+   * current size of this file, then no bytes are transferred.
    *
    * @throws IOException if the file needs more blocks but the disk is full or if reading from src
    *     throws an exception
    */
-  public long transferFrom(ReadableByteChannel src, long pos, long count) throws IOException {
-    prepareForWrite(pos, 0); // don't assume the full count bytes will be written
-
-    if (count == 0) {
+  public long transferFrom(ReadableByteChannel src, long startPos, long count) throws IOException {
+    if (count == 0
+        // Unlike the write() methods, attempting to transfer to a position that is greater than the
+        // current file size simply does nothing.
+        || startPos > size) {
       return 0;
     }
 
     long remaining = count;
+    long currentPos = startPos;
 
-    int blockIndex = blockIndex(pos);
-    byte[] block = blockForWrite(blockIndex);
-    int off = offsetInBlock(pos);
-
-    ByteBuffer buf = ByteBuffer.wrap(block, off, length(off, remaining));
-
-    long currentPos = pos;
-    int read = 0;
-    while (buf.hasRemaining()) {
-      read = src.read(buf);
-      if (read == -1) {
-        break;
-      }
-
-      currentPos += read;
-      remaining -= read;
-    }
-
-    // update size before trying to get next block in case the disk is out of space
-    if (currentPos > size) {
-      size = currentPos;
-    }
+    int blockIndex = blockIndex(startPos);
+    int off = offsetInBlock(startPos);
 
-    if (read != -1) {
-      outer:
-      while (remaining > 0) {
-        block = blockForWrite(++blockIndex);
+    outer:
+    while (remaining > 0) {
+      byte[] block = blockForWrite(blockIndex);
 
-        buf = ByteBuffer.wrap(block, 0, length(remaining));
-        while (buf.hasRemaining()) {
-          read = src.read(buf);
-          if (read == -1) {
-            break outer;
+      ByteBuffer buf = ByteBuffer.wrap(block, off, length(off, remaining));
+      while (buf.hasRemaining()) {
+        int read = src.read(buf);
+        // Note: we stop if we read 0 bytes from the src; even though the src is not at EOF, the
+        // spec of transferFrom is to stop immediately when reading from a non-blocking channel that
+        // has no bytes available rather than continuing until it reaches EOF. This makes sense
+        // because we'd otherwise just spin attempting to read bytes from the src repeatedly.
+        if (read < 1) {
+          if (currentPos >= size && buf.position() == 0) {
+            // The current position is at or beyond the end of file (prior to transfer start) and
+            // the current buffer position is 0. This means that a new block must have just been
+            // allocated to hold any potential transferred bytes, but no bytes were transferred to
+            // it because the src had no remaining bytes. So we need to de-allocate that block.
+            // It's possible that it would be preferable to always transfer to a temporary block
+            // first and then copy that block to a newly allocated block when it's full or src
+            // doesn't have any further bytes. Then if we hadn't read anything into the temporary
+            // block, we could simply discard it. But I think this scenario is likely rare enough
+            // that it's fine to temporarily allocate a block that _might_ not get used.
+            disk.free(this, 1);
           }
-
-          currentPos += read;
-          remaining -= read;
+          break outer;
         }
 
-        if (currentPos > size) {
-          size = currentPos;
-        }
+        currentPos += read;
+        remaining -= read;
       }
+
+      // Current write block is full
+      blockIndex++;
+      off = 0;
     }
 
     if (currentPos > size) {
       size = currentPos;
     }
 
-    return currentPos - pos;
+    return currentPos - startPos;
   }
 
   /**

jimfs/src/test/java/com/google/common/jimfs/RegularFileBlocksTest.java

@@ -19,6 +19,9 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.primitives.Bytes;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.channels.Channels;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -32,6 +35,8 @@
 @RunWith(JUnit4.class)
 public class RegularFileBlocksTest {
 
+  private static final int BLOCK_SIZE = 2;
+
   private RegularFile file;
 
   @Before
@@ -40,7 +45,7 @@ public void setUp() {
   }
 
   private static RegularFile createFile() {
-    return RegularFile.create(-1, new HeapDisk(2, 2, 2));
+    return RegularFile.create(-1, new HeapDisk(BLOCK_SIZE, 2, 2));
   }
 
   @Test
@@ -142,4 +147,33 @@ public void testTransferTo() {
     assertThat(Bytes.asList(file.getBlock(0))).isEqualTo(Bytes.asList(new byte[] {1, 2, 3}));
     assertThat(file.getBlock(1)).isNull();
   }
+
+  @Test
+  public void testTransferFrom() throws IOException {
+    // Test that when a transferFrom ends on a block boundary because the input has no further bytes
+    // and not because count bytes have been transferred, we don't leave an extra empty block
+    // allocated on the end of the file.
+    // https://github.com/google/jimfs/issues/163
+    byte[] bytes = new byte[BLOCK_SIZE];
+    RegularFile file = createFile();
+
+    long transferred =
+        file.transferFrom(Channels.newChannel(new ByteArrayInputStream(bytes)), 0, Long.MAX_VALUE);
+    assertThat(transferred).isEqualTo(bytes.length);
+    assertThat(file.blockCount()).isEqualTo(1);
+  }
+
+  @Test
+  public void testTransferFrom_noBytesNoAllocation() throws IOException {
+    // Similar to the previous test but ensures that if no bytes are transferred at all, no new
+    // blocks remain allocated.
+    // https://github.com/google/jimfs/issues/163
+    byte[] bytes = new byte[0];
+    RegularFile file = createFile();
+
+    long transferred =
+        file.transferFrom(Channels.newChannel(new ByteArrayInputStream(bytes)), 0, Long.MAX_VALUE);
+    assertThat(transferred).isEqualTo(0);
+    assertThat(file.blockCount()).isEqualTo(0);
+  }
 }

jimfs/src/test/java/com/google/common/jimfs/RegularFileTest.java

@@ -334,23 +334,31 @@ public void testEmpty_transferFrom_fromStart_countGreaterThanSrcSize() throws IO
       assertContentEquals("111111", file);
     }
 
-    public void testEmpty_transferFrom_fromBeyondStart_countEqualsSrcSize() throws IOException {
+    public void testEmpty_transferFrom_positionGreaterThanSize() throws IOException {
       long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 6);
-      assertEquals(6, transferred);
-      assertContentEquals("0000111111", file);
+      assertEquals(0, transferred);
+      assertContentEquals(bytes(), file);
     }
 
-    public void testEmpty_transferFrom_fromBeyondStart_countLessThanSrcSize() throws IOException {
+    public void testEmpty_transferFrom_positionGreaterThanSize_countEqualsSrcSize()
+        throws IOException {
+      long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 6);
+      assertEquals(0, transferred);
+      assertContentEquals(bytes(), file);
+    }
+
+    public void testEmpty_transferFrom_positionGreaterThanSize_countLessThanSrcSize()
+        throws IOException {
       long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 3);
-      assertEquals(3, transferred);
-      assertContentEquals("0000111", file);
+      assertEquals(0, transferred);
+      assertContentEquals(bytes(), file);
     }
 
-    public void testEmpty_transferFrom_fromBeyondStart_countGreaterThanSrcSize()
+    public void testEmpty_transferFrom_positionGreaterThanSize_countGreaterThanSrcSize()
         throws IOException {
       long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 12);
-      assertEquals(6, transferred);
-      assertContentEquals("0000111111", file);
+      assertEquals(0, transferred);
+      assertContentEquals(bytes(), file);
     }
 
     public void testEmpty_transferFrom_fromStart_noBytes_countEqualsSrcSize() throws IOException {
@@ -366,18 +374,18 @@ public void testEmpty_transferFrom_fromStart_noBytes_countGreaterThanSrcSize()
       assertContentEquals(bytes(), file);
     }
 
-    public void testEmpty_transferFrom_fromBeyondStart_noBytes_countEqualsSrcSize()
+    public void testEmpty_transferFrom_postionGreaterThanSrcSize_noBytes_countEqualsSrcSize()
         throws IOException {
       long transferred = file.transferFrom(new ByteBufferChannel(buffer("")), 5, 0);
       assertEquals(0, transferred);
-      assertContentEquals(bytes("00000"), file);
+      assertContentEquals(bytes(), file);
     }
 
-    public void testEmpty_transferFrom_fromBeyondStart_noBytes_countGreaterThanSrcSize()
+    public void testEmpty_transferFrom_postionGreaterThanSrcSize_noBytes_countGreaterThanSrcSize()
         throws IOException {
       long transferred = file.transferFrom(new ByteBufferChannel(buffer("")), 5, 10);
       assertEquals(0, transferred);
-      assertContentEquals(bytes("00000"), file);
+      assertContentEquals(bytes(), file);
     }
 
     public void testEmpty_transferTo() throws IOException {
@@ -793,11 +801,11 @@ public void testNonEmpty_transferFrom_toEnd() throws IOException {
       assertContentEquals("222222111111", file);
     }
 
-    public void testNonEmpty_transferFrom_toPastEnd() throws IOException {
+    public void testNonEmpty_transferFrom_positionGreaterThanSize() throws IOException {
       fillContent("222222");
       ByteBufferChannel channel = new ByteBufferChannel(buffer("111111"));
-      assertEquals(6, file.transferFrom(channel, 10, 6));
-      assertContentEquals("2222220000111111", file);
+      assertEquals(0, file.transferFrom(channel, 10, 6));
+      assertContentEquals("222222", file);
     }
 
     public void testNonEmpty_transferFrom_hugeOverestimateCount() throws IOException {