Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] JGit removes Pack from list *even* if exists on the filesystem
Now the right diff (previous one was the negative):

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 1aa71e5..e7308b5 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
@@ -569,6 +569,7 @@ void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 
        private void handlePackError(IOException e, PackFile p) {
                String warnTmpl = null;
+               String errTmpl = JGitText.get().exceptionWhileReadingPack;
                if ((e instanceof CorruptObjectException)
                                || (e instanceof PackInvalidException)) {
                        warnTmpl = JGitText.get().corruptPack;
@@ -576,11 +577,11 @@ private void handlePackError(IOException e, PackFile p) {
                        removePack(p);
                } else if (e instanceof FileNotFoundException) {
                        if (p.getPackFile().exists()) {
-                               warnTmpl = JGitText.get().packInaccessible;
+                               errTmpl = JGitText.get().packInaccessible;
                        } else {
                                warnTmpl = JGitText.get().packWasDeleted;
+                               removePack(p);
                        }
-                       removePack(p);
                } else if (FileUtils.isStaleFileHandleInCausalChain(e)) {
                        warnTmpl = JGitText.get().packHandleIsStale;
                        removePack(p);
@@ -597,8 +598,7 @@ private void handlePackError(IOException e, PackFile p) {
                        // Don't remove the pack from the list, as the error may be
                        // transient.
                        LOG.error(MessageFormat.format(
-                                       JGitText.get().exceptionWhileReadingPack, p.getPackFile()
-                                                       .getAbsolutePath()), e);
+                                       errTmpl, p.getPackFile().getAbsolutePath()), e);
                }
        }

On Friday, March 10, 2017 at 12:32:09 AM UTC, lucamilanesio wrote:
Actually the JGit wiki is broken ... can't sign the ECA and post the fix :-(

Anyway, here is the patch:

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 e7308b5..1aa71e5 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
@@ -569,7 +569,6 @@ void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 
        private void handlePackError(IOException e, PackFile p) {
                String warnTmpl = null;
-               String errTmpl = JGitText.get().exceptionWhileReadingPack;
                if ((e instanceof CorruptObjectException)
                                || (e instanceof PackInvalidException)) {
                        warnTmpl = JGitText.get().corruptPack;
@@ -577,11 +576,11 @@ private void handlePackError(IOException e, PackFile p) {
                        removePack(p);
                } else if (e instanceof FileNotFoundException) {
                        if (p.getPackFile().exists()) {
-                               errTmpl = JGitText.get().packInaccessible;
+                               warnTmpl = JGitText.get().packInaccessible;
                        } else {
                                warnTmpl = JGitText.get().packWasDeleted;
-                               removePack(p);
                        }
+                       removePack(p);
                } else if (FileUtils.isStaleFileHandleInCausalChain(e)) {
                        warnTmpl = JGitText.get().packHandleIsStale;
                        removePack(p);
@@ -598,7 +597,8 @@ private void handlePackError(IOException e, PackFile p) {
                        // Don't remove the pack from the list, as the error may be
                        // transient.
                        LOG.error(MessageFormat.format(
-                                       errTmpl, p.getPackFile().getAbsolutePath()), e);
+                                       JGitText.get().exceptionWhileReadingPack, p.getPackFile()
+                                                       .getAbsolutePath()), e);
                }
        }



On Friday, March 10, 2017 at 12:12:53 AM UTC, lucamilanesio wrote:
Hi Gerrit and JGit users,
I have found a small but serious bug in the way that JGit manages the file-system related errors when reading pack files.

Look at this broken management of the FileNotFoundException (from https://goo.gl/oidSwf)


The FileNotFoundException is typically raised in three conditions:
- file doesn't exist
- incompatible read vs. read/write open modes
- filesystem locking
- temporary lack of resources (e.g. too many open files)

... so ... *why on earth* do we always assume that Pack doesn't exist and we remove it from the in-memory list?

The "removePack(p)" should be IMHO inside the else() block whilst if FileNotFoundException happens when the pack file exists => log the error and try next time again.
Does it make sense?

Going to post a 1-liner fix tomorrow :-)

Luca.

Back to the top