Merge branch 'stable-5.9' into stable-5.10 * stable-5.9: Retry loose object read upon "Stale file handle" exception Ignore missing javadoc in test bundles Change-Id: I56fc2c47193a891285a705d44b3507f23982dc8a
diff --git a/org.eclipse.jgit.lfs.server.test/.settings/org.eclipse.jdt.core.prefs b/org.eclipse.jgit.lfs.server.test/.settings/org.eclipse.jdt.core.prefs index 3dd5840..4c4634a 100644 --- a/org.eclipse.jgit.lfs.server.test/.settings/org.eclipse.jdt.core.prefs +++ b/org.eclipse.jgit.lfs.server.test/.settings/org.eclipse.jdt.core.prefs
@@ -52,7 +52,7 @@ org.eclipse.jdt.core.compiler.problem.missingJavadocCommentsOverriding=disabled org.eclipse.jdt.core.compiler.problem.missingJavadocCommentsVisibility=protected org.eclipse.jdt.core.compiler.problem.missingJavadocTagDescription=return_tag -org.eclipse.jdt.core.compiler.problem.missingJavadocTags=error +org.eclipse.jdt.core.compiler.problem.missingJavadocTags=ignore org.eclipse.jdt.core.compiler.problem.missingJavadocTagsMethodTypeParameters=disabled org.eclipse.jdt.core.compiler.problem.missingJavadocTagsOverriding=disabled org.eclipse.jdt.core.compiler.problem.missingJavadocTagsVisibility=private
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java index d269457..a0bc63a 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java
@@ -44,8 +44,12 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import java.io.File; import java.io.IOException; @@ -69,6 +73,7 @@ import org.eclipse.jgit.util.FS; import org.junit.Assume; import org.junit.Test; +import org.mockito.Mockito; public class ObjectDirectoryTest extends RepositoryTestCase { @@ -195,6 +200,42 @@ public void testShallowFile() } @Test + public void testOpenLooseObjectSuppressStaleFileHandleException() + throws Exception { + ObjectId id = ObjectId + .fromString("873fb8d667d05436d728c52b1d7a09528e6eb59b"); + WindowCursor curs = new WindowCursor(db.getObjectDatabase()); + + ObjectDirectory mock = mock(ObjectDirectory.class); + UnpackedObjectCache unpackedObjectCacheMock = mock( + UnpackedObjectCache.class); + + Mockito.when(mock.getObjectLoader(any(), any(), any())) + .thenThrow(new IOException("Stale File Handle")); + Mockito.when(mock.openLooseObject(curs, id)).thenCallRealMethod(); + Mockito.when(mock.unpackedObjectCache()) + .thenReturn(unpackedObjectCacheMock); + + assertNull(mock.openLooseObject(curs, id)); + verify(unpackedObjectCacheMock).remove(id); + } + + @Test + public void testOpenLooseObjectPropagatesIOExceptions() throws Exception { + ObjectId id = ObjectId + .fromString("873fb8d667d05436d728c52b1d7a09528e6eb59b"); + WindowCursor curs = new WindowCursor(db.getObjectDatabase()); + + ObjectDirectory mock = mock(ObjectDirectory.class); + + Mockito.when(mock.getObjectLoader(any(), any(), any())) + .thenThrow(new IOException("some IO failure")); + Mockito.when(mock.openLooseObject(curs, id)).thenCallRealMethod(); + + assertThrows(IOException.class, () -> mock.openLooseObject(curs, id)); + } + + @Test public void testShallowFileCorrupt() throws Exception { FileRepository repository = createBareRepository(); ObjectDirectory dir = repository.getObjectDatabase();
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index c52e307..861ebca 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -419,6 +419,7 @@ logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement. logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}. logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {} +looseObjectHandleIsStale=loose-object {0} file handle is stale. retry {1} of {2} maxCountMustBeNonNegative=max count must be >= 0 mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2} mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index d12a243..66d54c0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -447,6 +447,7 @@ public static JGitText get() { /***/ public String logLargerFiletimeDiff; /***/ public String logSmallerFiletime; /***/ public String logXDGConfigHomeInvalid; + /***/ public String looseObjectHandleIsStale; /***/ public String maxCountMustBeNonNegative; /***/ public String mergeConflictOnNonNoteEntries; /***/ public String mergeConflictOnNotes;
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 d321828..106cddd 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
@@ -85,6 +85,10 @@ public class ObjectDirectory extends FileObjectDatabase { /** Maximum number of candidates offered as resolutions of abbreviation. */ private static final int RESOLVE_ABBREV_LIMIT = 256; + /** Maximum number of attempts to read a loose object for which a stale file + * handle exception is thrown */ + final static int MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS = 5; + private final AlternateHandle handle = new AlternateHandle(this); private final Config config; @@ -212,7 +216,7 @@ public PackInserter newPackInserter() { /** {@inheritDoc} */ @Override public void close() { - unpackedObjectCache.clear(); + unpackedObjectCache().clear(); final PackList packs = packList.get(); if (packs != NO_PACKS && packList.compareAndSet(packs, NO_PACKS)) { @@ -277,7 +281,7 @@ public String toString() { /** {@inheritDoc} */ @Override public boolean has(AnyObjectId objectId) { - return unpackedObjectCache.isUnpacked(objectId) + return unpackedObjectCache().isUnpacked(objectId) || hasPackedInSelfOrAlternate(objectId, null) || hasLooseInSelfOrAlternate(objectId, null); } @@ -395,7 +399,7 @@ private void resolve(Set<ObjectId> matches, AbbreviatedObjectId id, @Override ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId) throws IOException { - if (unpackedObjectCache.isUnpacked(objectId)) { + if (unpackedObjectCache().isUnpacked(objectId)) { ObjectLoader ldr = openLooseObject(curs, objectId); if (ldr != null) { return ldr; @@ -473,23 +477,71 @@ ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId) { @Override ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id) throws IOException { - File path = fileFor(id); - try (FileInputStream in = new FileInputStream(path)) { - unpackedObjectCache.add(id); - return UnpackedObject.open(in, path, id, curs); - } catch (FileNotFoundException noFile) { - if (path.exists()) { - throw noFile; + int readAttempts = 0; + while (readAttempts < MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS) { + readAttempts++; + File path = fileFor(id); + try { + return getObjectLoader(curs, path, id); + } catch (FileNotFoundException noFile) { + if (path.exists()) { + throw noFile; + } + break; + } catch (IOException e) { + if (!FileUtils.isStaleFileHandleInCausalChain(e)) { + throw e; + } + if (LOG.isDebugEnabled()) { + LOG.debug(MessageFormat.format( + JGitText.get().looseObjectHandleIsStale, id.name(), + Integer.valueOf(readAttempts), Integer.valueOf( + MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS))); + } } - unpackedObjectCache.remove(id); - return null; } + unpackedObjectCache().remove(id); + return null; + } + + /** + * Provides a loader for an objectId + * + * @param curs + * cursor on the database + * @param path + * the path of the loose object + * @param id + * the object id + * @return a loader for the loose file object + * @throws IOException + * when file does not exist or it could not be opened + */ + ObjectLoader getObjectLoader(WindowCursor curs, File path, AnyObjectId id) + throws IOException { + try (FileInputStream in = new FileInputStream(path)) { + unpackedObjectCache().add(id); + return UnpackedObject.open(in, path, id, curs); + } + } + + /** + * <p> + * Getter for the field <code>unpackedObjectCache</code>. + * </p> + * This accessor is particularly useful to allow mocking of this class for + * testing purposes. + * + * @return the cache of the objects currently unpacked. + */ + UnpackedObjectCache unpackedObjectCache() { + return unpackedObjectCache; } @Override long getObjectSize(WindowCursor curs, AnyObjectId id) throws IOException { - if (unpackedObjectCache.isUnpacked(id)) { + if (unpackedObjectCache().isUnpacked(id)) { long len = getLooseObjectSize(curs, id); if (0 <= len) { return len; @@ -567,13 +619,13 @@ private long getLooseObjectSize(WindowCursor curs, AnyObjectId id) throws IOException { File f = fileFor(id); try (FileInputStream in = new FileInputStream(f)) { - unpackedObjectCache.add(id); + unpackedObjectCache().add(id); return UnpackedObject.getSize(in, id, curs); } catch (FileNotFoundException noFile) { if (f.exists()) { throw noFile; } - unpackedObjectCache.remove(id); + unpackedObjectCache().remove(id); return -1; } } @@ -667,7 +719,7 @@ InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id, boolean createDuplicate) throws IOException { // If the object is already in the repository, remove temporary file. // - if (unpackedObjectCache.isUnpacked(id)) { + if (unpackedObjectCache().isUnpacked(id)) { FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.EXISTS_LOOSE; } @@ -723,7 +775,7 @@ private InsertLooseObjectResult tryMove(File tmp, File dst, Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst), StandardCopyOption.ATOMIC_MOVE); dst.setReadOnly(); - unpackedObjectCache.add(id); + unpackedObjectCache().add(id); return InsertLooseObjectResult.INSERTED; }