From d83e85736e8bcc57af74011cdc084c2bbe4fc86c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Fri, 4 Jan 2019 16:02:07 -0800 Subject: [PATCH] Don't swallow IOException Swallowing intermittent errors and trying to recover from them makes JGit's behavior hard to predict and difficult to debug. Propagate the errors instead. This doesn't violate JGit's usual backward compatibility promise for clients because in these contexts an IOException indicates either repository corruption or a true I/O error. Let's consider these cases one at a time. In the case of repository corruption, falling back e.g. to an empty set of refs or a missing ref will not serve a caller well. The fallback does not indicate the nature of the corruption, so they are not in a good place to recover from the error. This is analogous to Git, which attempts to provide sufficient support to recover from corruption (by ensuring commands like "git branch -D" cope with corruption) but little else. In the case of an I/O error, the best we can do is to propagate the error so that the user sees a dialog and has an opportunity to try again. As in the corruption case, the fallback behavior does not provide enough information for a caller to rely on the current error handling, and callers such as EGit already need to be able to handle runtime exceptions. To be conservative, keep the existing behavior for the deprecated Repository#peel method. In this example, the fallback behavior is to return an unpeeled ref, which is distinguishable from the ref not existing and should thus at least be possible to debug. Change-Id: I0eb58eb8c77519df7f50d21d1742016b978e67a3 Signed-off-by: Jonathan Nieder --- .../src/org/eclipse/jgit/lib/Repository.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index c57669960d..a61897a652 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -57,6 +57,7 @@ import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStream; +import java.io.UncheckedIOException; import java.net.URISyntaxException; import java.text.MessageFormat; import java.util.Collection; @@ -326,8 +327,7 @@ public abstract class Repository implements AutoCloseable { try { return getObjectDatabase().has(objectId); } catch (IOException e) { - // Legacy API, assume error means "no" - return false; + throw new UncheckedIOException(e); } } @@ -1105,7 +1105,7 @@ public abstract class Repository implements AutoCloseable { try { return getRefDatabase().getRefs(RefDatabase.ALL); } catch (IOException e) { - return new HashMap<>(); + throw new UncheckedIOException(e); } } @@ -1123,7 +1123,7 @@ public abstract class Repository implements AutoCloseable { try { return getRefDatabase().getRefs(Constants.R_TAGS); } catch (IOException e) { - return new HashMap<>(); + throw new UncheckedIOException(e); } } @@ -1322,9 +1322,7 @@ public abstract class Repository implements AutoCloseable { return RepositoryState.MERGING_RESOLVED; } } catch (IOException e) { - // Can't decide whether unmerged paths exists. Return - // MERGING state to be on the safe side (in state MERGING - // you are not allow to do anything) + throw new UncheckedIOException(e); } return RepositoryState.MERGING; } @@ -1339,7 +1337,7 @@ public abstract class Repository implements AutoCloseable { return RepositoryState.CHERRY_PICKING_RESOLVED; } } catch (IOException e) { - // fall through to CHERRY_PICKING + throw new UncheckedIOException(e); } return RepositoryState.CHERRY_PICKING; @@ -1352,7 +1350,7 @@ public abstract class Repository implements AutoCloseable { return RepositoryState.REVERTING_RESOLVED; } } catch (IOException e) { - // fall through to REVERTING + throw new UncheckedIOException(e); } return RepositoryState.REVERTING; -- 2.39.5