From c2ab332e81caec773055da24c4a9d0ef58f04142 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 4 Jun 2020 13:51:17 +0900 Subject: [PATCH 1/5] ObjectDirectory: Fail immediately when atomic move is not supported If atomic move is not supported, AtomicMoveNotSupportedException will be thrown on the first attempt to move the temp file. There is no point attempting the move operation a second time because it will only fail for the same reason. Add an immediate return of FAILURE on the first occasion. Remove the unnecessary handling of the exception in the second block. Change-Id: I4658a8b37cfec2d7ef0217c8346e512968d0964c Signed-off-by: David Pursehouse --- .../eclipse/jgit/internal/storage/file/ObjectDirectory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index 6a822d570..651cf1911 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -693,6 +693,8 @@ public class ObjectDirectory extends FileObjectDatabase { return InsertLooseObjectResult.INSERTED; } catch (AtomicMoveNotSupportedException e) { LOG.error(e.getMessage(), e); + FileUtils.delete(tmp, FileUtils.RETRY); + return InsertLooseObjectResult.FAILURE; } catch (IOException e) { // ignore } @@ -708,8 +710,6 @@ public class ObjectDirectory extends FileObjectDatabase { dst.setReadOnly(); unpackedObjectCache.add(id); return InsertLooseObjectResult.INSERTED; - } catch (AtomicMoveNotSupportedException e) { - LOG.error(e.getMessage(), e); } catch (IOException e) { LOG.debug(e.getMessage(), e); } From 949ee670c6a0cdb7b0ecaaaa751799148899c709 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 4 Jun 2020 14:07:34 +0900 Subject: [PATCH 2/5] ObjectDirectory: Explicitly handle NoSuchFileException On the first attempt to move the temp file, NoSuchFileException can be raised if the destination folder does not exist. Instead of handling this implicitly in the catch of IOException and then continuing to create the destination folder and try again, explicitly catch it and create the destination folder. If any other IOException occurs, treat it as an unexpected error and return FAILURE. Subsequently, on the second attempt to move the temp file, if ANY kind of IOException occurs, also consider this an unexpected error and return FAILURE. In both catch blocks for IOException, add logging at ERROR level. Change-Id: I9de9ee3d2b368be36e02ee1c0daf8e844f7e46c8 Signed-off-by: David Pursehouse --- .../storage/file/ObjectDirectory.java | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index 651cf1911..33f7853ab 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -21,6 +21,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.util.ArrayList; @@ -695,15 +696,19 @@ public class ObjectDirectory extends FileObjectDatabase { LOG.error(e.getMessage(), e); FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.FAILURE; + } catch (NoSuchFileException e) { + // It's possible the directory doesn't exist yet as the object + // directories are always lazily created. Note that we try the + // rename/move first as the directory likely does exist. + + // Create the directory + FileUtils.mkdir(dst.getParentFile(), true); } catch (IOException e) { - // ignore + LOG.error(e.getMessage(), e); + FileUtils.delete(tmp, FileUtils.RETRY); + return InsertLooseObjectResult.FAILURE; } - // Maybe the directory doesn't exist yet as the object - // directories are always lazily created. Note that we - // try the rename first as the directory likely does exist. - // - FileUtils.mkdir(dst.getParentFile(), true); try { Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst), StandardCopyOption.ATOMIC_MOVE); @@ -711,21 +716,10 @@ public class ObjectDirectory extends FileObjectDatabase { unpackedObjectCache.add(id); return InsertLooseObjectResult.INSERTED; } catch (IOException e) { - LOG.debug(e.getMessage(), e); - } - - if (!createDuplicate && has(id)) { + LOG.error(e.getMessage(), e); FileUtils.delete(tmp, FileUtils.RETRY); - return InsertLooseObjectResult.EXISTS_PACKED; + return InsertLooseObjectResult.FAILURE; } - - // The object failed to be renamed into its proper - // location and it doesn't exist in the repository - // either. We really don't know what went wrong, so - // fail. - // - FileUtils.delete(tmp, FileUtils.RETRY); - return InsertLooseObjectResult.FAILURE; } boolean searchPacksAgain(PackList old) { From dac6801b47ef7924d6b265800816da1b571e6d70 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 5 Jun 2020 11:34:49 +0900 Subject: [PATCH 3/5] ObjectDirectory: Further clean up insertUnpackedObject - The code to move the file is repeated. Split it out into a utility method. - Remove the catch block for AtomicMoveNotSupportedException which is redundant because it's handled in exactly the same way as the IOException further down. The only exception we need to explicitly handle differently in this block is NoSuchFileException. - Improve the comments. Change-Id: Ifc5490953ffb25ecd1c48a06289eccb3f19910c6 Signed-off-by: David Pursehouse --- .../storage/file/ObjectDirectory.java | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index 33f7853ab..265b71dd2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -19,7 +19,6 @@ import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; -import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.StandardCopyOption; @@ -686,42 +685,48 @@ public class ObjectDirectory extends FileObjectDatabase { FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.EXISTS_LOOSE; } + try { - Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst), - StandardCopyOption.ATOMIC_MOVE); - dst.setReadOnly(); - unpackedObjectCache.add(id); - return InsertLooseObjectResult.INSERTED; - } catch (AtomicMoveNotSupportedException e) { - LOG.error(e.getMessage(), e); - FileUtils.delete(tmp, FileUtils.RETRY); - return InsertLooseObjectResult.FAILURE; + return tryMove(tmp, dst, id); } catch (NoSuchFileException e) { // It's possible the directory doesn't exist yet as the object // directories are always lazily created. Note that we try the // rename/move first as the directory likely does exist. - - // Create the directory + // + // Create the directory. + // FileUtils.mkdir(dst.getParentFile(), true); } catch (IOException e) { + // Any other IO error is considered a failure. + // LOG.error(e.getMessage(), e); FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.FAILURE; } try { - Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst), - StandardCopyOption.ATOMIC_MOVE); - dst.setReadOnly(); - unpackedObjectCache.add(id); - return InsertLooseObjectResult.INSERTED; + return tryMove(tmp, dst, id); } catch (IOException e) { + // The object failed to be renamed into its proper location and + // it doesn't exist in the repository either. We really don't + // know what went wrong, so fail. + // LOG.error(e.getMessage(), e); FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.FAILURE; } } + private InsertLooseObjectResult tryMove(File tmp, File dst, + ObjectId id) + throws IOException { + Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst), + StandardCopyOption.ATOMIC_MOVE); + dst.setReadOnly(); + unpackedObjectCache.add(id); + return InsertLooseObjectResult.INSERTED; + } + boolean searchPacksAgain(PackList old) { // Whether to trust the pack folder's modification time. If set // to false we will always scan the .git/objects/pack folder to From f4f5d448b691c06688cb3636dc2294308b1a433f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 5 Jun 2020 14:46:08 +0900 Subject: [PATCH 4/5] ObjectDirectoryInserter: Remove redundant 'throws' declarations ObjectWritingException and FileNotFoundException are subclasses of IOException, which is already declared. Error does not need to be explicitly declared. Change-Id: I879820a33e10ec3a7ef676adc9c9148d2b3c4b27 Signed-off-by: David Pursehouse --- .../internal/storage/file/ObjectDirectoryInserter.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java index c908e6a24..2d2401b5b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java @@ -14,7 +14,6 @@ package org.eclipse.jgit.internal.storage.file; import java.io.EOFException; import java.io.File; -import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.FilterOutputStream; import java.io.IOException; @@ -116,7 +115,7 @@ class ObjectDirectoryInserter extends ObjectInserter { private ObjectId insertOneObject( File tmp, ObjectId id, boolean createDuplicate) - throws IOException, ObjectWritingException { + throws IOException { switch (db.insertUnpackedObject(tmp, id, createDuplicate)) { case INSERTED: case EXISTS_PACKED: @@ -165,8 +164,7 @@ class ObjectDirectoryInserter extends ObjectInserter { @SuppressWarnings("resource" /* java 7 */) private File toTemp(final SHA1 md, final int type, long len, - final InputStream is) throws IOException, FileNotFoundException, - Error { + final InputStream is) throws IOException { boolean delete = true; File tmp = newTempFile(); try { @@ -205,7 +203,7 @@ class ObjectDirectoryInserter extends ObjectInserter { @SuppressWarnings("resource" /* java 7 */) private File toTemp(final int type, final byte[] buf, final int pos, - final int len) throws IOException, FileNotFoundException { + final int len) throws IOException { boolean delete = true; File tmp = newTempFile(); try { From c0c7f445f4e8daf2900f8735b088edb2288882b0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 5 Jun 2020 14:59:54 +0900 Subject: [PATCH 5/5] ObjectDirectoryInserter: Open FileOutputStream in try-with-resource Change-Id: Icc569aeefdc79baee5dfb71fb34d881c561dcf52 Signed-off-by: David Pursehouse --- .../storage/file/ObjectDirectoryInserter.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java index 2d2401b5b..e6b2cc12f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryInserter.java @@ -162,17 +162,16 @@ class ObjectDirectoryInserter extends ObjectInserter { } } - @SuppressWarnings("resource" /* java 7 */) private File toTemp(final SHA1 md, final int type, long len, final InputStream is) throws IOException { boolean delete = true; File tmp = newTempFile(); - try { - FileOutputStream fOut = new FileOutputStream(tmp); + try (FileOutputStream fOut = new FileOutputStream(tmp)) { try { OutputStream out = fOut; - if (config.getFSyncObjectFiles()) + if (config.getFSyncObjectFiles()) { out = Channels.newOutputStream(fOut.getChannel()); + } DeflaterOutputStream cOut = compress(out); SHA1OutputStream dOut = new SHA1OutputStream(cOut, md); writeHeader(dOut, type, len); @@ -180,53 +179,54 @@ class ObjectDirectoryInserter extends ObjectInserter { final byte[] buf = buffer(); while (len > 0) { int n = is.read(buf, 0, (int) Math.min(len, buf.length)); - if (n <= 0) + if (n <= 0) { throw shortInput(len); + } dOut.write(buf, 0, n); len -= n; } dOut.flush(); cOut.finish(); } finally { - if (config.getFSyncObjectFiles()) + if (config.getFSyncObjectFiles()) { fOut.getChannel().force(true); - fOut.close(); + } } delete = false; return tmp; } finally { - if (delete) + if (delete) { FileUtils.delete(tmp, FileUtils.RETRY); + } } } - @SuppressWarnings("resource" /* java 7 */) private File toTemp(final int type, final byte[] buf, final int pos, final int len) throws IOException { boolean delete = true; File tmp = newTempFile(); - try { - FileOutputStream fOut = new FileOutputStream(tmp); + try (FileOutputStream fOut = new FileOutputStream(tmp)) { try { OutputStream out = fOut; - if (config.getFSyncObjectFiles()) + if (config.getFSyncObjectFiles()) { out = Channels.newOutputStream(fOut.getChannel()); + } DeflaterOutputStream cOut = compress(out); writeHeader(cOut, type, len); cOut.write(buf, pos, len); cOut.finish(); } finally { - if (config.getFSyncObjectFiles()) + if (config.getFSyncObjectFiles()) { fOut.getChannel().force(true); - fOut.close(); + } } - delete = false; return tmp; } finally { - if (delete) + if (delete) { FileUtils.delete(tmp, FileUtils.RETRY); + } } }