Fix GC for FileRepo in case packfile renames fail

Only on Windows the rename operation which renames temporary Packfiles
(and index-files and bitmap-files) sometime fails. This happens only
when renaming a temporary Packfile to a Packfile which already exists.
Such situations occur if you run GC twice on a repo without modifying
the repo inbetween.

In such situations there was bug in GC which led to a corrupted repo
whithout any packfiles anymore. This commit fixes the problem by
introducing a utility method which renames a file and throws an
IOException if it fails. This method also takes care to repeat a
failing rename if our FS class has found out we are running on a
platform with a unreliable File.renameTo() method.

I am searching for a better solution because even with this utility
method in hand a GC on a already GC'ed repo will fail on Windows. But
at least with this fix we will not produce corrupted repos anymore.

Bug: 389305
Change-Id: Iac1ab3e0b8c419c90404f2e2f3559672eb8f6d28
Signed-off-by: Christian Halstrick <christian.halstrick@sap.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java
index bc60f64886472855d382a1e40c92439f3b2274ea..d5c8f12ef69015c0dc77b3b6b5f810ded230851e 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java
@@ -435,6 +435,13 @@ public class GCTest extends LocalDiskRepositoryTestCase {
                assertEquals(0, stats.numberOfLooseObjects);
                assertEquals(4, stats.numberOfPackedObjects);
                assertEquals(1, stats.numberOfPackFiles);
+
+               // Do the gc again and check that it hasn't changed anything
+               gc.gc();
+               stats = gc.getStatistics();
+               assertEquals(0, stats.numberOfLooseObjects);
+               assertEquals(4, stats.numberOfPackedObjects);
+               assertEquals(1, stats.numberOfPackFiles);
        }
 
        @Test

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
index 22fa827044ba1dfb09445fd62f1f3de26239ceeb..bc14d88d60d9e04f91d3202f963a525f724c8f8f 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
@@ -735,11 +735,21 @@ public class GC {
 
                        // rename the temporary files to real files
                        File realPack = nameFor(id, ".pack"); //$NON-NLS-1$
+
+                       // if the packfile already exists (because we are rewriting a
+                       // packfile for the same set of objects maybe with different
+                       // PackConfig) then make sure we get rid of all handles on the file.
+                       // Windows will not allow for rename otherwise.
+                       if (realPack.exists())
+                               for (PackFile p : repo.getObjectDatabase().getPacks())
+                                       if (realPack.getPath().equals(p.getPackFile().getPath())) {
+                                               p.close();
+                                               break;
+                                       }
                        tmpPack.setReadOnly();
                        boolean delete = true;
                        try {
-                               if (!tmpPack.renameTo(realPack))
-                                       return null;
+                               FileUtils.rename(tmpPack, realPack);
                                delete = false;
                                for (Map.Entry<PackExt, File> tmpEntry : tmpExts.entrySet()) {
                                        File tmpExt = tmpEntry.getValue();
@@ -747,7 +757,9 @@ public class GC {
 
                                        File realExt = nameFor(
                                                        id, "." + tmpEntry.getKey().getExtension()); //$NON-NLS-1$
-                                       if (!tmpExt.renameTo(realExt)) {
+                                       try {
+                                               FileUtils.rename(tmpExt, realExt);
+                                       } catch (IOException e) {
                                                File newExt = new File(realExt.getParentFile(),
                                                                realExt.getName() + ".new"); //$NON-NLS-1$
                                                if (!tmpExt.renameTo(newExt))

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
index 67f371b618da4072c65fdd33fdb1a6375c460b71..acc1a2c31c0be5b13e211ecfb8fe81e38f2ac901 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
@@ -167,6 +167,39 @@ public class FileUtils {
                }
        }
 
+       /**
+        * Rename a file or folder. If the rename fails and if we are running on a
+        * filesystem where it makes sense to repeat a failing rename then repeat
+        * the rename operation up to 9 times with 100ms sleep time between two
+        * calls
+        *
+        * @see FS#retryFailedLockFileCommit()
+        * @param src
+        *            the old {@code File}
+        * @param dst
+        *            the new {@code File}
+        * @throws IOException
+        *             if the rename has failed
+        */
+       public static void rename(final File src, final File dst)
+                       throws IOException {
+               int attempts = FS.DETECTED.retryFailedLockFileCommit() ? 10 : 1;
+               while (--attempts >= 0) {
+                       if (src.renameTo(dst))
+                               return;
+                       try {
+                               Thread.sleep(100);
+                       } catch (InterruptedException e) {
+                               throw new IOException(MessageFormat.format(
+                                               JGitText.get().renameFileFailed, src.getAbsolutePath(),
+                                               dst.getAbsolutePath()));
+                       }
+               }
+               throw new IOException(MessageFormat.format(
+                               JGitText.get().renameFileFailed, src.getAbsolutePath(),
+                               dst.getAbsolutePath()));
+       }
+
        /**
         * Creates the directory named by this abstract pathname.
         *