diff options
| author | Martin Fick <mfick@nvidia.com> | 2025-02-12 19:07:42 -0800 |
|---|---|---|
| committer | Martin Fick <mfick@nvidia.com> | 2025-02-14 16:22:16 +0000 |
| commit | 5dae4a8351287526b323547f942da72b9e00f456 (patch) | |
| tree | 5eda85b3972288ff369dc98a927f8d0e043331e1 /org.eclipse.jgit/src/org | |
| parent | a8efd046fa5f47351f91b5dddd13becb83b9bdc8 (diff) | |
| download | jgit-5dae4a8351287526b323547f942da72b9e00f456.tar.gz jgit-5dae4a8351287526b323547f942da72b9e00f456.zip | |
Pack: no longer set invalid in openFail()
The intention of the 'invalidate' argument in openFail() is to
invalidate the Pack in certain situations. However, after moving
doOpen() to a lock instead of using synchronized, the invalidation
approach could also incorrectly mark an already invalid Pack valid,
which was never the intention since previously invalid would only ever
get set to false if it already was false. Fix this by never setting
invalid in openFail(), instead set invalid explicitly before calling
openFail when needed. This makes the intent clearer, and aligns better
with all the existing comments already trying to explain the boolean
(and some of them become obvious enough now that the comment is deleted
or shortened). This is also likely faster than adding a conditional in
openFail() to make 'invalidate' work properly.
Change-Id: Ie6182103ee2994724cb5cb0b64030fedba84b637
Signed-off-by: Martin Fick <mfick@nvidia.com>
Diffstat (limited to 'org.eclipse.jgit/src/org')
| -rw-r--r-- | org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java | 30 |
1 files changed, 16 insertions, 14 deletions
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 1a7b5de1d7..122d800582 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -694,7 +694,7 @@ public class Pack implements Iterable<PackIndex.MutableEntry> { private void doOpen() throws IOException { if (invalid) { - openFail(true, invalidatingCause); + openFail(invalidatingCause); throw new PackInvalidException(packFile, invalidatingCause); } try { @@ -705,39 +705,41 @@ public class Pack implements Iterable<PackIndex.MutableEntry> { } } catch (InterruptedIOException e) { // don't invalidate the pack, we are interrupted from another thread - openFail(false, e); + openFail(e); throw e; } catch (FileNotFoundException fn) { - // don't invalidate the pack if opening an existing file failed - // since it may be related to a temporary lack of resources (e.g. - // max open files) - openFail(!packFile.exists(), fn); + if (!packFile.exists()) { + // Failure to open an existing file may be related to a temporary lack of resources + // (e.g. max open files) + invalid = true; + } + openFail(fn); throw fn; } catch (EOFException | AccessDeniedException | NoSuchFileException | CorruptObjectException | NoPackSignatureException | PackMismatchException | UnpackException | UnsupportedPackIndexVersionException | UnsupportedPackVersionException pe) { - // exceptions signaling permanent problems with a pack - openFail(true, pe); + invalid = true; // exceptions signaling permanent problems with a pack + openFail(pe); throw pe; } catch (IOException ioe) { - // mark this packfile as invalid when NFS stale file handle error - // occur - openFail(FileUtils.isStaleFileHandleInCausalChain(ioe), ioe); + if (FileUtils.isStaleFileHandleInCausalChain(ioe)) { + invalid = true; + } + openFail(ioe); throw ioe; } catch (RuntimeException ge) { // generic exceptions could be transient so we should not mark the // pack invalid to avoid false MissingObjectExceptions - openFail(false, ge); + openFail(ge); throw ge; } } - private void openFail(boolean invalidate, Exception cause) { + private void openFail(Exception cause) { activeWindows = 0; activeCopyRawData = 0; - invalid = invalidate; invalidatingCause = cause; doClose(); } |