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 <jrn@google.com>
This commit is contained in:
Jonathan Nieder 2019-01-04 16:02:07 -08:00
parent 752d5547e4
commit d83e85736e
1 changed files with 7 additions and 9 deletions

View File

@ -57,6 +57,7 @@
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 boolean hasObject(AnyObjectId objectId) {
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 Map<String, Ref> getAllRefs() {
try {
return getRefDatabase().getRefs(RefDatabase.ALL);
} catch (IOException e) {
return new HashMap<>();
throw new UncheckedIOException(e);
}
}
@ -1123,7 +1123,7 @@ public Map<String, Ref> getTags() {
try {
return getRefDatabase().getRefs(Constants.R_TAGS);
} catch (IOException e) {
return new HashMap<>();
throw new UncheckedIOException(e);
}
}
@ -1322,9 +1322,7 @@ public RepositoryState getRepositoryState() {
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 RepositoryState getRepositoryState() {
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 RepositoryState getRepositoryState() {
return RepositoryState.REVERTING_RESOLVED;
}
} catch (IOException e) {
// fall through to REVERTING
throw new UncheckedIOException(e);
}
return RepositoryState.REVERTING;