From af5d4d37ae3ef6612a7530112d5ff15f6e3f5d8d Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 15 Sep 2023 11:44:09 +0200 Subject: [errorprone] Remove unnecessary comparison Raised by errorprone: org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java:406: error: [ComparisonOutOfRange] chars may have a value in the range 0 to 65535; therefore, this comparison to 0 will always evaluate to true if (ch >= 0 && ch < inUse.length) { ^ see https://errorprone.info/bugpattern/ComparisonOutOfRange Change-Id: I9625aa7894a34dbffd77d39a40c6e285c86b56d5 (cherry picked from commit cf5ec856bda907c0537ce5a80246b9ab18195c8b) --- org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java index 6a9b45b065..f701a41d67 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitConfig.java @@ -403,7 +403,7 @@ public class CommitConfig { for (int i = 0; i < len; i++) { char ch = line.charAt(i); if (!Character.isWhitespace(ch)) { - if (ch >= 0 && ch < inUse.length) { + if (ch < inUse.length) { inUse[ch] = true; } break; -- cgit v1.2.3 From 99333e75bc002711fe4f2ddcaf5740e20c3f7e92 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 15 Sep 2023 11:48:05 +0200 Subject: [errorprone] Fix wrong comparison which always evaluated to false org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java:59: error: [ComparisonOutOfRange] ints may have a value in the range -2147483648 to 2147483647; therefore, this comparison to Integer.MAX_VALUE will always evaluate to false if (table[k] > Integer.MAX_VALUE) { ^ See https://errorprone.info/bugpattern/ComparisonOutOfRange We need to check if variable `uint` of type `long` exceeds the maximum possible int value before casting it to `int` below. This was introduced in Ib5c0d6678cb242870a0f5841bd413ad3885e95f6 Change-Id: I675d594f523084be4c1678328cc343065e32d998 (cherry picked from commit 916200e278ebeaa2602b5dc23143c8eedf11858b) --- .../org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java index 22b4011f9f..a15602a168 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/GraphObjectIndex.java @@ -56,7 +56,7 @@ class GraphObjectIndex { long uint32; for (int k = 0; k < table.length; k++) { uint32 = NB.decodeUInt32(oidFanout, k * 4); - if (table[k] > Integer.MAX_VALUE) { + if (uint32 > Integer.MAX_VALUE) { throw new CommitGraphFormatException( JGitText.get().commitGraphFileIsTooLargeForJgit); } -- cgit v1.2.3 From 8b49e01abf44e21e38504ba31a68670710ca6610 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Mon, 12 Feb 2024 16:29:47 -0700 Subject: SnapshottingRefDir: Reduce casts with overrides Overriding getRefDirectory() and getRefDatabase() lets us skip casting to SnapshottingRefDirectory in several places. Change-Id: I61ba12fb6f066b1a9c4ea5ec9538978cbf040acd Signed-off-by: Nasser Grainawi --- .../storage/file/SnapshottingRefDirectory.java | 26 +++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java index 0b9748096e..10ef7fbf3e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java @@ -13,7 +13,6 @@ package org.eclipse.jgit.internal.storage.file; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.revwalk.RevWalk; @@ -148,29 +147,29 @@ class SnapshottingRefDirectory extends RefDirectory { } private static T invalidateSnapshotOnError( - SupplierThrowsException f, RefDatabase refDb) + SupplierThrowsException f, SnapshottingRefDirectory refDb) throws IOException { return invalidateSnapshotOnError(a -> f.call(), null, refDb); } private static R invalidateSnapshotOnError( FunctionThrowsException f, A a, - RefDatabase refDb) throws IOException { + SnapshottingRefDirectory refDb) throws IOException { try { return f.apply(a); } catch (IOException e) { - ((SnapshottingRefDirectory) refDb).invalidateSnapshot(); + refDb.invalidateSnapshot(); throw e; } } private static void invalidateSnapshotOnError( TriConsumerThrowsException f, A1 a1, A2 a2, - A3 a3, RefDatabase refDb) throws IOException { + A3 a3, SnapshottingRefDirectory refDb) throws IOException { try { f.accept(a1, a2, a3); } catch (IOException e) { - ((SnapshottingRefDirectory) refDb).invalidateSnapshot(); + refDb.invalidateSnapshot(); throw e; } } @@ -215,6 +214,11 @@ class SnapshottingRefDirectory extends RefDirectory { return invalidateSnapshotOnError(t -> super.link(t), target, getRefDatabase()); } + + @Override + public SnapshottingRefDirectory getRefDatabase() { + return (SnapshottingRefDirectory) super.getRefDatabase(); + } } private static class SnapshotRefDirectoryRename extends RefDirectoryRename { @@ -228,6 +232,11 @@ class SnapshottingRefDirectory extends RefDirectory { return invalidateSnapshotOnError(() -> super.rename(), getRefDirectory()); } + + @Override + public SnapshottingRefDirectory getRefDirectory() { + return (SnapshottingRefDirectory) super.getRefDirectory(); + } } private static class SnapshotPackedBatchRefUpdate @@ -254,5 +263,10 @@ class SnapshottingRefDirectory extends RefDirectory { invalidateSnapshotOnError((rw, m, a3) -> super.execute(rw, m), walk, monitor, null, getRefDatabase()); } + + @Override + public SnapshottingRefDirectory getRefDatabase() { + return (SnapshottingRefDirectory) super.getRefDatabase(); + } } } -- cgit v1.2.3 From 8197cab33758677eacd2cf253e912f0d23068934 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Mon, 12 Feb 2024 16:32:30 -0700 Subject: SnapshottingRefDir: Replace lambas with method refs Method references are shorter and easier to read in this case. Change-Id: Ia6809fa0e3f282acbe7b7f7e3813a34f3cf40c43 Signed-off-by: Nasser Grainawi --- .../storage/file/SnapshottingRefDirectory.java | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java index 10ef7fbf3e..70eeb2c82a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java @@ -181,37 +181,35 @@ class SnapshottingRefDirectory extends RefDirectory { @Override public Result forceUpdate() throws IOException { - return invalidateSnapshotOnError(() -> super.forceUpdate(), + return invalidateSnapshotOnError(super::forceUpdate, getRefDatabase()); } @Override public Result update() throws IOException { - return invalidateSnapshotOnError(() -> super.update(), - getRefDatabase()); + return invalidateSnapshotOnError(super::update, getRefDatabase()); } @Override public Result update(RevWalk walk) throws IOException { - return invalidateSnapshotOnError(rw -> super.update(rw), walk, + return invalidateSnapshotOnError(super::update, walk, getRefDatabase()); } @Override public Result delete() throws IOException { - return invalidateSnapshotOnError(() -> super.delete(), - getRefDatabase()); + return invalidateSnapshotOnError(super::delete, getRefDatabase()); } @Override public Result delete(RevWalk walk) throws IOException { - return invalidateSnapshotOnError(rw -> super.delete(rw), walk, + return invalidateSnapshotOnError(super::delete, walk, getRefDatabase()); } @Override public Result link(String target) throws IOException { - return invalidateSnapshotOnError(t -> super.link(t), target, + return invalidateSnapshotOnError(super::link, target, getRefDatabase()); } @@ -229,8 +227,7 @@ class SnapshottingRefDirectory extends RefDirectory { @Override public RefUpdate.Result rename() throws IOException { - return invalidateSnapshotOnError(() -> super.rename(), - getRefDirectory()); + return invalidateSnapshotOnError(super::rename, getRefDirectory()); } @Override @@ -253,7 +250,7 @@ class SnapshottingRefDirectory extends RefDirectory { @Override public void execute(RevWalk walk, ProgressMonitor monitor, List options) throws IOException { - invalidateSnapshotOnError((rw, m, o) -> super.execute(rw, m, o), + invalidateSnapshotOnError(super::execute, walk, monitor, options, getRefDatabase()); } -- cgit v1.2.3 From 29c89d1f02e84d0395e12f55204bd129e61c43bf Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Thu, 25 Jan 2024 16:29:05 -0700 Subject: SnapshottingRefDirectory: Invalidate snapshot after locking ref for update When using the SnapshottingRefDirectory, if a thread has already read packed-refs, then another actor updates packed-refs, the original thread may create an update that is based on the old cached/snapshotted packed-refs content. That update could effectively perform a forced update unintentionally because it is unaware of the new content. This seems particularly likely to happen in a scenario where a loose ref was just packed. If the ref was loose, our thread would see the current ref value (because we don't snapshot loose refs and always read them from disk), but since there is no loose ref, we expect to find the current value in packed-refs. However, (before this change) we rely on our snapshot of packed-refs which does not contain the updated ref value. Invalidating the cache after the loose ref is locked ensures that the ref value does not change again before we read it to perform the update. Bug: jgit-21 Change-Id: Id10900a99bfd0401a1b9c39d997093af0289649e Signed-off-by: Nasser Grainawi --- .../jgit/internal/storage/file/RefDirectoryUpdate.java | 11 +++++++++++ .../internal/storage/file/SnapshottingRefDirectory.java | 15 +++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java index 0dcb3196c5..62373a0a7b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java @@ -54,6 +54,7 @@ class RefDirectoryUpdate extends RefUpdate { String name = dst.getName(); lock = new LockFile(database.fileFor(name)); if (lock.lock()) { + doAfterLocking(name); dst = database.findRef(name); setOldObjectId(dst != null ? dst.getObjectId() : null); return true; @@ -141,4 +142,14 @@ class RefDirectoryUpdate extends RefUpdate { return Result.NEW; return Result.FORCED; } + + /** + * Do any actions needed immediately after a lock on the ref is acquired + * + * @param name + * the name of the reference. + */ + protected void doAfterLocking(String name) { + // No actions by default + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java index 70eeb2c82a..b973aeb8ef 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java @@ -213,6 +213,21 @@ class SnapshottingRefDirectory extends RefDirectory { getRefDatabase()); } + /** + * Invalidate the SnapshottingRefDirectory snapshot after locking the + * ref. + *

+ * Doing this after locking the ref ensures that the upcoming write is + * not based on a cached value. + * + * @param name + * the name of the reference. + */ + @Override + protected void doAfterLocking(String name) { + getRefDatabase().invalidateSnapshot(); + } + @Override public SnapshottingRefDirectory getRefDatabase() { return (SnapshottingRefDirectory) super.getRefDatabase(); -- cgit v1.2.3 From 86ef2d5311d99fe1b2f4961608ec1ec5fd0ae3ea Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 13 Feb 2024 15:49:54 +0100 Subject: Add missing javadoc description for declared exception Change-Id: I16305bc15d2ddff1ce055772a711658ef81858e2 --- .../org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java | 1 + 1 file changed, 1 insertion(+) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java index b973aeb8ef..eaa26ed4f3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java @@ -125,6 +125,7 @@ class SnapshottingRefDirectory extends RefDirectory { * threads use this snapshot. * * @throws IOException + * if an IO error occurred */ private synchronized void refreshSnapshot() throws IOException { compareAndSetPackedRefs(packedRefs.get(), refDb.getPackedRefs()); -- cgit v1.2.3 From acf21c0bc6a63a3d20fca92757b992a1f2d55f41 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Thu, 25 Jan 2024 18:59:15 -0700 Subject: RefDirectory: Do not unlock until after deleting loose ref Fix a potential race condition where we would remove our loose ref lock file before deleting the loose ref itself. This race could result in the current thread deleting a loose ref newly written by another thread. Other callers seem to be following the correct pattern, but improve the method naming to try to help future callers. Change-Id: I80cebe4788edd855e15381336d980c41498fca80 Signed-off-by: Nasser Grainawi --- .../jgit/internal/storage/file/RefDirectory.java | 35 ++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index 9f31e688c3..695721ec42 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -706,8 +706,7 @@ public class RefDirectory extends RefDatabase { int levels = levelsIn(name) - 2; delete(logFor(name), levels); if (dst.getStorage().isLoose()) { - update.unlock(); - delete(fileFor(name), levels); + deleteAndUnlock(fileFor(name), levels, update); } modCnt.incrementAndGet(); @@ -825,7 +824,7 @@ public class RefDirectory extends RefDatabase { newLoose = curLoose.remove(idx); } while (!looseRefs.compareAndSet(curLoose, newLoose)); int levels = levelsIn(refName) - 2; - delete(refFile, levels, rLck); + deleteAndUnlock(refFile, levels, rLck); } } finally { if (shouldUnlock) { @@ -1302,19 +1301,37 @@ public class RefDirectory extends RefDatabase { } static void delete(File file, int depth) throws IOException { - delete(file, depth, null); + delete(file); + deleteEmptyParentDirs(file, depth); } - private static void delete(File file, int depth, LockFile rLck) - throws IOException { + private static void delete(File file) throws IOException { if (!file.delete() && file.isFile()) { - throw new IOException(MessageFormat.format( - JGitText.get().fileCannotBeDeleted, file)); + throw new IOException( + MessageFormat.format(JGitText.get().fileCannotBeDeleted, + file)); } + } + private static void deleteAndUnlock(File file, int depth, + RefDirectoryUpdate refUpdate) throws IOException { + delete(file); + if (refUpdate != null) { + refUpdate.unlock(); // otherwise cannot delete parent directories emptied by the update + } + deleteEmptyParentDirs(file, depth); + } + + private static void deleteAndUnlock(File file, int depth, LockFile rLck) + throws IOException { + delete(file); if (rLck != null) { - rLck.unlock(); // otherwise cannot delete dir below + rLck.unlock(); // otherwise cannot delete parent directories of the lock file } + deleteEmptyParentDirs(file, depth); + } + + private static void deleteEmptyParentDirs(File file, int depth) { File dir = file.getParentFile(); for (int i = 0; i < depth; ++i) { try { -- cgit v1.2.3