From c9586d19ab97e46239f1f9a4a522b914a5016107 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Fri, 11 Oct 2024 13:25:57 -0700 Subject: DfsReaderOptions: read loadRevIndexInParallel from config The options have the field but it isn't loaded from the config. This forces a workaround downstream. Read the option from the config, as the others. Change-Id: I7720812e0577d8f45f6b7f5b8495a8b64729125e --- Documentation/config-options.md | 1 + 1 file changed, 1 insertion(+) (limited to 'Documentation') diff --git a/Documentation/config-options.md b/Documentation/config-options.md index 78930e6267..eeb78ff550 100644 --- a/Documentation/config-options.md +++ b/Documentation/config-options.md @@ -31,6 +31,7 @@ For details on native git options see also the official [git config documentatio | `core.dfs.blockSize` | `64 kiB` | ⃞ | Size in bytes of a single window read in from the pack file into the DFS block cache. | | `core.dfs.concurrencyLevel` | `32` | ⃞ | The estimated number of threads concurrently accessing the DFS block cache. | | `core.dfs.deltaBaseCacheLimit` | `10 MiB` | ⃞ | Maximum number of bytes to hold in per-reader DFS delta base cache. | +| `core.dfs.loadRevIndexInParallel` | false; | ⃞ | Try to load the reverse index in parallel with the bitmap index. | | `core.dfs.streamFileThreshold` | `50 MiB` | ⃞ | The size threshold beyond which objects must be streamed. | | `core.dfs.streamBuffer` | Block size of the pack | ⃞ | Number of bytes to use for buffering when streaming a pack file during copying. If 0 the block size of the pack is used| | `core.dfs.streamRatio` | `0.30` | ⃞ | Ratio of DFS block cache to occupy with a copied pack. Values between `0` and `1.0`. | -- cgit v1.2.3 From ff3a150495ad466169a7fb0cfaed06aace50da7b Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Wed, 15 Jan 2025 13:15:28 +0100 Subject: Improve configuration of trusting file attributes in FileSnapshot FileSnapshot relies on File attributes (which can be retrieved by the stat() function on Unix) to quickly determine if a File was modified without reading file content or listing content of a directory. On NFS this doesn't work reliably due to NFS client caching behavior. Hence we introduced the option core.trustFolderStat to control if FileSnapshot can trust File attributes to ensure we don't miss modifications on NFS. Later more specific options for handling packed and loose refs were added which also support another config value AFTER_OPEN, in addition to ALWAYS and NEVER, which refreshes File attributes by opening a FileInputStream on the file instead of reading its content and then trusts the File attributes of the refreshed File. We discussed in jgit-127 how to extend these options for other scenarios where file attributes are used to detect modifications and came to the conclusion to improve the existing trustXXX config options in the following way: - replace the not well defined "trustFolderStat" option by a general option "trustStat" which allows to configure all these scenarios with a single option - introduce a new enum TrustStat and use it for all scenarios. It has the values - NEVER don't trust File attributes - ALWAYS always trust File attributes - AFTER_OPEN open a FileInputStream on the respective file or folder to ensure its File attributes are refreshed and then trust the refreshed File attributes - INHERIT only used for specific options to signal it should inherit its value from the "trustStat" option - deprecate the old, now unused enums "TrustPackedRefsStat" and "TrustLooseRefStat" - deprecate "trustFolderStat", if set, translate it to the corresponding value of the new option "trustStat" - if both "trustFolderStat" and "trustStat" are configured the value configured for "trustStat" takes precedence and "trustFolderStat" is ignored - add one specific option for each scenario which can override the global setting - add new options "trustLooseObjectStat" and "trustPackStat" which allow to override the global setting for handling of loose objects and pack files - implement option AFTER_OPEN for "trustLooseObjectStat" and "trustPackStat" Bug: jgit-127 Change-Id: I662982258bc4494f146805875e52838394673c8f --- Documentation/config-options.md | 9 +- .../internal/storage/file/ObjectDirectoryTest.java | 17 +- .../org/eclipse/jgit/internal/JGitText.properties | 3 + .../src/org/eclipse/jgit/internal/JGitText.java | 3 + .../jgit/internal/storage/file/LooseObjects.java | 46 ++++-- .../jgit/internal/storage/file/PackDirectory.java | 38 +++-- .../jgit/internal/storage/file/RefDirectory.java | 39 ++--- .../src/org/eclipse/jgit/lib/ConfigConstants.java | 23 +++ .../src/org/eclipse/jgit/lib/CoreConfig.java | 172 ++++++++++++++++++++- 9 files changed, 278 insertions(+), 72 deletions(-) (limited to 'Documentation') diff --git a/Documentation/config-options.md b/Documentation/config-options.md index eeb78ff550..807d4a8002 100644 --- a/Documentation/config-options.md +++ b/Documentation/config-options.md @@ -55,9 +55,12 @@ For details on native git options see also the official [git config documentatio | `core.streamFileThreshold` | `50 MiB` | ⃞ | The size threshold beyond which objects must be streamed. | | `core.supportsAtomicFileCreation` | `true` | ⃞ | Whether the filesystem supports atomic file creation. | | `core.symlinks` | Auto detect if filesystem supports symlinks| ✅ | If false, symbolic links are checked out as small plain files that contain the link text. | -| `core.trustFolderStat` | `true` | ⃞ | Whether to trust the pack folder's, packed-refs file's and loose-objects folder's file attributes (Java equivalent of stat command on *nix). When looking for pack files, if `false` JGit will always scan the `.git/objects/pack` folder and if set to `true` it assumes that pack files are unchanged if the file attributes of the pack folder are unchanged. When getting the list of packed refs, if `false` JGit will always read the packed-refs file and if set to `true` it uses the file attributes of the packed-refs file and will only read it if a file attribute has changed. When looking for loose objects, if `false` and if a loose object is not found, JGit will open and close a stream to `.git/objects` folder (which can refresh its directory listing, at least on some NFS clients) and retry looking for that loose object. Setting this option to `false` can help to workaround caching issues on NFS, but reduces performance. | -| `core.trustPackedRefsStat` | `unset` | ⃞ | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the packed-refs file. If `never` JGit will ignore the file attributes of the packed-refs file and always read it. If `always` JGit will trust the file attributes of the packed-refs file and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, except that the packed-refs file is opened and closed before its file attributes are considered. An open/close of the packed-refs file is known to refresh its file attributes, at least on some NFS clients. If `unset`, JGit will use the behavior described in `trustFolderStat`. | -| `core.trustLooseRefStat` | `always` | ⃞ | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the loose ref. If `always` JGit will trust the file attributes of the loose ref and its parent directories. `after_open` behaves similar to `always`, except that all parent directories of the loose ref up to the repository root are opened and closed before its file attributes are considered. An open/close of these parent directories is known to refresh the file attributes, at least on some NFS clients. | +| ~~`core.trustFolderStat`~~ | `true` | ⃞ | __Deprecated__, use `core.trustStat` instead. If set to `true` translated to `core.trustStat=always`, if `false` translated to `core.trustStat=never`, see below. If both `core.trustFolderStat` and `core.trustStat` are configured then `trustStat` takes precedence and `trustFolderStat` is ignored. | +| `core.trustLooseRefStat` | `inherit` | ⃞ | Whether to trust the file attributes of loose refs and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `trustStat`. | +| `core.trustPackedRefsStat` | `inherit` | ⃞ | Whether to trust the file attributes of the packed-refs file. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | +| `core.trustLooseObjectStat` | `inherit` | ⃞ | Whether to trust the file attributes of the loose object file and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | +| `core.trustPackStat` | `inherit` | ⃞ | Whether to trust the file attributes of the `objects/pack` directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | +| `core.trustStat` | `always` | ⃞ | Global option to configure whether to trust file attributes (Java equivalent of stat command on Unix) of files storing git objects. Can be overridden for specific files by configuring `core.trustLooseRefStat, core.trustPackedRefsStat, core.trustLooseObjectStat, core.trustPackStat`. If `never` JGit will ignore the file attributes of the file and always read it. If `always` JGit will trust the file attributes and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, but file attributes are only considered *after* the file itself and any transient parent directories have been opened and closed. An open/close of the file/directory is known to refresh its file attributes, at least on some NFS clients. | | `core.worktree` | Root directory of the working tree if it is not the parent directory of the `.git` directory | ✅ | The path to the root of the working tree. | ## __fetch__ options diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java index 7d298ee6f1..33cbc868ca 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java @@ -50,6 +50,7 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; @@ -210,17 +211,17 @@ public class ObjectDirectoryTest extends RepositoryTestCase { .fromString("873fb8d667d05436d728c52b1d7a09528e6eb59b"); WindowCursor curs = new WindowCursor(db.getObjectDatabase()); - LooseObjects mock = mock(LooseObjects.class); + Config config = new Config(); + config.setString("core", null, "trustLooseObjectStat", "ALWAYS"); + LooseObjects spy = Mockito.spy(new LooseObjects(config, trash)); UnpackedObjectCache unpackedObjectCacheMock = mock( UnpackedObjectCache.class); - Mockito.when(mock.getObjectLoader(any(), any(), any())) - .thenThrow(new IOException("Stale File Handle")); - Mockito.when(mock.open(curs, id)).thenCallRealMethod(); - Mockito.when(mock.unpackedObjectCache()) - .thenReturn(unpackedObjectCacheMock); + doThrow(new IOException("Stale File Handle")).when(spy) + .getObjectLoader(any(), any(), any()); + doReturn(unpackedObjectCacheMock).when(spy).unpackedObjectCache(); - assertNull(mock.open(curs, id)); + assertNull(spy.open(curs, id)); verify(unpackedObjectCacheMock).remove(id); } @@ -231,7 +232,7 @@ public class ObjectDirectoryTest extends RepositoryTestCase { WindowCursor curs = new WindowCursor(db.getObjectDatabase()); Config config = new Config(); - config.setString("core", null, "trustFolderStat", "false"); + config.setString("core", null, "trustLooseObjectStat", "NEVER"); LooseObjects spy = spy(new LooseObjects(config, db.getObjectDatabase().getDirectory())); diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index acfe812a20..7c47d3a04f 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -267,6 +267,7 @@ deletingBranches=Deleting branches... deletingNotSupported=Deleting {0} not supported. depthMustBeAt1=Depth must be >= 1 depthWithUnshallow=Depth and unshallow can\'t be used together +deprecatedTrustFolderStat=Option core.trustFolderStat is deprecated, replace it by core.trustStat. destinationIsNotAWildcard=Destination is not a wildcard. detachedHeadDetected=HEAD is detached diffToolNotGivenError=No diff tool provided and no defaults configured. @@ -464,6 +465,7 @@ invalidTimestamp=Invalid timestamp in {0} invalidTimeUnitValue2=Invalid time unit value: {0}.{1}={2} invalidTimeUnitValue3=Invalid time unit value: {0}.{1}.{2}={3} invalidTreeZeroLengthName=Cannot append a tree entry with zero-length name +invalidTrustStat=core.trustStat must not be set to TrustStat.INHERIT, falling back to TrustStat.ALWAYS. invalidURL=Invalid URL {0} invalidWildcards=Invalid wildcards {0} invalidRefSpec=Invalid refspec {0} @@ -615,6 +617,7 @@ peerDidNotSupplyACompleteObjectGraph=peer did not supply a complete object graph personIdentEmailNonNull=E-mail address of PersonIdent must not be null. personIdentNameNonNull=Name of PersonIdent must not be null. postCommitHookFailed=Execution of post-commit hook failed: {0}. +precedenceTrustConfig=Both core.trustFolderStat and core.trustStat are set, ignoring trustFolderStat since trustStat takes precedence. Remove core.trustFolderStat from your configuration. prefixRemote=remote: problemWithResolvingPushRefSpecsLocally=Problem with resolving push ref specs locally: {0} progressMonUploading=Uploading {0} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 2d9d2c527c..07e882498c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -295,6 +295,7 @@ public class JGitText extends TranslationBundle { /***/ public String deleteTagUnexpectedResult; /***/ public String deletingBranches; /***/ public String deletingNotSupported; + /***/ public String deprecatedTrustFolderStat; /***/ public String depthMustBeAt1; /***/ public String depthWithUnshallow; /***/ public String destinationIsNotAWildcard; @@ -493,6 +494,7 @@ public class JGitText extends TranslationBundle { /***/ public String invalidTimeUnitValue2; /***/ public String invalidTimeUnitValue3; /***/ public String invalidTreeZeroLengthName; + /***/ public String invalidTrustStat; /***/ public String invalidURL; /***/ public String invalidWildcards; /***/ public String invalidRefSpec; @@ -645,6 +647,7 @@ public class JGitText extends TranslationBundle { /***/ public String personIdentEmailNonNull; /***/ public String personIdentNameNonNull; /***/ public String postCommitHookFailed; + /***/ public String precedenceTrustConfig; /***/ public String prefixRemote; /***/ public String problemWithResolvingPushRefSpecsLocally; /***/ public String progressMonUploading; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LooseObjects.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LooseObjects.java index 74ef6e139c..909b3e3082 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LooseObjects.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LooseObjects.java @@ -26,8 +26,9 @@ import org.eclipse.jgit.internal.storage.file.FileObjectDatabase.InsertLooseObje import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.CoreConfig; +import org.eclipse.jgit.lib.CoreConfig.TrustStat; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.util.FileUtils; @@ -55,7 +56,7 @@ class LooseObjects { private final UnpackedObjectCache unpackedObjectCache; - private final boolean trustFolderStat; + private final TrustStat trustLooseObjectStat; /** * Initialize a reference to an on-disk object directory. @@ -68,9 +69,8 @@ class LooseObjects { LooseObjects(Config config, File dir) { directory = dir; unpackedObjectCache = new UnpackedObjectCache(); - trustFolderStat = config.getBoolean( - ConfigConstants.CONFIG_CORE_SECTION, - ConfigConstants.CONFIG_KEY_TRUSTFOLDERSTAT, true); + trustLooseObjectStat = config.get(CoreConfig.KEY) + .getTrustLooseObjectStat(); } /** @@ -108,7 +108,8 @@ class LooseObjects { */ boolean has(AnyObjectId objectId) { boolean exists = hasWithoutRefresh(objectId); - if (trustFolderStat || exists) { + if (trustLooseObjectStat == TrustStat.ALWAYS + || exists) { return exists; } try (InputStream stream = Files.newInputStream(directory.toPath())) { @@ -165,9 +166,29 @@ class LooseObjects { ObjectLoader open(WindowCursor curs, AnyObjectId id) throws IOException { File path = fileFor(id); for (int retries = 0; retries < MAX_STALE_READ_RETRIES; retries++) { - if (trustFolderStat && !path.exists()) { + boolean reload = true; + switch (trustLooseObjectStat) { + case NEVER: break; + case AFTER_OPEN: + try (InputStream stream = Files + .newInputStream(path.getParentFile().toPath())) { + // open the loose object's fanout directory to refresh + // attributes (on some NFS clients) + } catch (FileNotFoundException | NoSuchFileException e) { + // ignore + } + //$FALL-THROUGH$ + case ALWAYS: + if (!path.exists()) { + reload = false; + } + break; + case INHERIT: + // only used in CoreConfig internally + throw new IllegalStateException(); } + if (reload) { try { return getObjectLoader(curs, path, id); } catch (FileNotFoundException noFile) { @@ -181,9 +202,10 @@ class LooseObjects { } if (LOG.isDebugEnabled()) { LOG.debug(MessageFormat.format( - JGitText.get().looseObjectHandleIsStale, id.name(), - Integer.valueOf(retries), Integer.valueOf( - MAX_STALE_READ_RETRIES))); + JGitText.get().looseObjectHandleIsStale, + id.name(), Integer.valueOf(retries), + Integer.valueOf(MAX_STALE_READ_RETRIES))); + } } } } @@ -209,7 +231,7 @@ class LooseObjects { try { return getObjectLoaderWithoutRefresh(curs, path, id); } catch (FileNotFoundException e) { - if (trustFolderStat) { + if (trustLooseObjectStat == TrustStat.ALWAYS) { throw e; } try (InputStream stream = Files @@ -246,7 +268,7 @@ class LooseObjects { return getSizeWithoutRefresh(curs, id); } catch (FileNotFoundException noFile) { try { - if (trustFolderStat) { + if (trustLooseObjectStat == TrustStat.ALWAYS) { throw noFile; } try (InputStream stream = Files diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java index 1ce14b3a18..ef877d8a48 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java @@ -17,6 +17,8 @@ import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Arrays; @@ -42,7 +44,8 @@ import org.eclipse.jgit.internal.storage.pack.PackWriter; import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ConfigConstants; +import org.eclipse.jgit.lib.CoreConfig; +import org.eclipse.jgit.lib.CoreConfig.TrustStat; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.util.FileUtils; @@ -72,7 +75,7 @@ class PackDirectory { private final AtomicReference packList; - private final boolean trustFolderStat; + private final TrustStat trustPackStat; /** * Initialize a reference to an on-disk 'pack' directory. @@ -86,14 +89,7 @@ class PackDirectory { this.config = config; this.directory = directory; packList = new AtomicReference<>(NO_PACKS); - - // Whether to trust the pack folder's modification time. If set to false - // we will always scan the .git/objects/pack folder to check for new - // pack files. If set to true (default) we use the folder's size, - // modification time, and key (inode) and assume that no new pack files - // can be in this folder if these attributes have not changed. - trustFolderStat = config.getBoolean(ConfigConstants.CONFIG_CORE_SECTION, - ConfigConstants.CONFIG_KEY_TRUSTFOLDERSTAT, true); + trustPackStat = config.get(CoreConfig.KEY).getTrustPackStat(); } /** @@ -365,8 +361,26 @@ class PackDirectory { } boolean searchPacksAgain(PackList old) { - return (!trustFolderStat || old.snapshot.isModified(directory)) - && old != scanPacks(old); + switch (trustPackStat) { + case NEVER: + break; + case AFTER_OPEN: + try (InputStream stream = Files + .newInputStream(directory.toPath())) { + // open the pack directory to refresh attributes (on some NFS clients) + } catch (IOException e) { + // ignore + } + //$FALL-THROUGH$ + case ALWAYS: + if (!old.snapshot.isModified(directory)) { + return false; + } + break; + case INHERIT: + // only used in CoreConfig internally + } + return old != scanPacks(old); } void insert(Pack pack) { 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 33996519f7..05f1ef53a1 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 @@ -64,10 +64,9 @@ import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.ObjectWritingException; import org.eclipse.jgit.events.RefsChangedEvent; import org.eclipse.jgit.internal.JGitText; -import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.CoreConfig.TrustLooseRefStat; -import org.eclipse.jgit.lib.CoreConfig.TrustPackedRefsStat; +import org.eclipse.jgit.lib.CoreConfig; +import org.eclipse.jgit.lib.CoreConfig.TrustStat; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdRef; import org.eclipse.jgit.lib.ProgressMonitor; @@ -185,11 +184,7 @@ public class RefDirectory extends RefDatabase { private List retrySleepMs = RETRY_SLEEP_MS; - private final boolean trustFolderStat; - - private final TrustPackedRefsStat trustPackedRefsStat; - - private final TrustLooseRefStat trustLooseRefStat; + private final CoreConfig coreConfig; RefDirectory(RefDirectory refDb) { parent = refDb.parent; @@ -201,9 +196,7 @@ public class RefDirectory extends RefDatabase { packedRefsFile = refDb.packedRefsFile; looseRefs.set(refDb.looseRefs.get()); packedRefs.set(refDb.packedRefs.get()); - trustFolderStat = refDb.trustFolderStat; - trustPackedRefsStat = refDb.trustPackedRefsStat; - trustLooseRefStat = refDb.trustLooseRefStat; + coreConfig = refDb.coreConfig; inProcessPackedRefsLock = refDb.inProcessPackedRefsLock; } @@ -219,17 +212,7 @@ public class RefDirectory extends RefDatabase { looseRefs.set(RefList. emptyList()); packedRefs.set(NO_PACKED_REFS); - trustFolderStat = db.getConfig() - .getBoolean(ConfigConstants.CONFIG_CORE_SECTION, - ConfigConstants.CONFIG_KEY_TRUSTFOLDERSTAT, true); - trustPackedRefsStat = db.getConfig() - .getEnum(ConfigConstants.CONFIG_CORE_SECTION, null, - ConfigConstants.CONFIG_KEY_TRUST_PACKED_REFS_STAT, - TrustPackedRefsStat.UNSET); - trustLooseRefStat = db.getConfig() - .getEnum(ConfigConstants.CONFIG_CORE_SECTION, null, - ConfigConstants.CONFIG_KEY_TRUST_LOOSE_REF_STAT, - TrustLooseRefStat.ALWAYS); + coreConfig = db.getConfig().get(CoreConfig.KEY); inProcessPackedRefsLock = new ReentrantLock(true); } @@ -979,7 +962,7 @@ public class RefDirectory extends RefDatabase { PackedRefList getPackedRefs() throws IOException { final PackedRefList curList = packedRefs.get(); - switch (trustPackedRefsStat) { + switch (coreConfig.getTrustPackedRefsStat()) { case NEVER: break; case AFTER_OPEN: @@ -995,12 +978,8 @@ public class RefDirectory extends RefDatabase { return curList; } break; - case UNSET: - if (trustFolderStat - && !curList.snapshot.isModified(packedRefsFile)) { - return curList; - } - break; + case INHERIT: + // only used in CoreConfig internally } return refreshPackedRefs(curList); @@ -1186,7 +1165,7 @@ public class RefDirectory extends RefDatabase { LooseRef scanRef(LooseRef ref, String name) throws IOException { final File path = fileFor(name); - if (trustLooseRefStat.equals(TrustLooseRefStat.AFTER_OPEN)) { + if (coreConfig.getTrustLooseRefStat() == TrustStat.AFTER_OPEN) { refreshPathToLooseRef(Paths.get(name)); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index 5068a6cce0..30e7de47c3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -600,10 +600,20 @@ public final class ConfigConstants { /** * The "trustfolderstat" key in the "core" section + * * @since 3.6 + * @deprecated use {CONFIG_KEY_TRUST_STAT} instead */ + @Deprecated(since = "7.2", forRemoval = true) public static final String CONFIG_KEY_TRUSTFOLDERSTAT = "trustfolderstat"; + /** + * The "trustfilestat" key in the "core"section + * + * @since 7.2 + */ + public static final String CONFIG_KEY_TRUST_STAT = "truststat"; + /** * The "supportsAtomicFileCreation" key in the "core" section * @@ -1022,6 +1032,19 @@ public final class ConfigConstants { */ public static final String CONFIG_KEY_TRUST_LOOSE_REF_STAT = "trustLooseRefStat"; + /** + * The "trustLooseRefStat" key + * + * @since 7.2 + */ + public static final String CONFIG_KEY_TRUST_PACK_STAT = "trustPackStat"; + + /** + * The "trustLooseObjectFileStat" key + * + * @since 7.2 + */ + public static final String CONFIG_KEY_TRUST_LOOSE_OBJECT_STAT = "trustLooseObjectStat"; /** * The "pack.preserveOldPacks" key * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java index 49602a75eb..e43c9653dd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java @@ -17,12 +17,16 @@ package org.eclipse.jgit.lib; import static java.util.zip.Deflater.DEFAULT_COMPRESSION; +import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.Config.SectionParser; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * This class keeps git repository core parameters. */ public class CoreConfig { + private static final Logger LOG = LoggerFactory.getLogger(CoreConfig.class); /** Key for {@link Config#get(SectionParser)}. */ public static final Config.SectionParser KEY = CoreConfig::new; @@ -127,7 +131,9 @@ public class CoreConfig { * Permissible values for {@code core.trustPackedRefsStat}. * * @since 6.1.1 + * @deprecated use {@link TrustStat} instead */ + @Deprecated(since = "7.2", forRemoval = true) public enum TrustPackedRefsStat { /** Do not trust file attributes of the packed-refs file. */ NEVER, @@ -135,12 +141,15 @@ public class CoreConfig { /** Trust file attributes of the packed-refs file. */ ALWAYS, - /** Open and close the packed-refs file to refresh its file attributes - * and then trust it. */ + /** + * Open and close the packed-refs file to refresh its file attributes + * and then trust it. + */ AFTER_OPEN, - /** {@code core.trustPackedRefsStat} defaults to this when it is - * not set */ + /** + * {@code core.trustPackedRefsStat} defaults to this when it is not set + */ UNSET } @@ -148,17 +157,44 @@ public class CoreConfig { * Permissible values for {@code core.trustLooseRefStat}. * * @since 6.9 + * @deprecated use {@link TrustStat} instead */ + @Deprecated(since = "7.2", forRemoval = true) public enum TrustLooseRefStat { /** Trust file attributes of the loose ref. */ ALWAYS, - /** Open and close parent directories of the loose ref file until the - * repository root to refresh its file attributes and then trust it. */ + /** + * Open and close parent directories of the loose ref file until the + * repository root to refresh its file attributes and then trust it. + */ AFTER_OPEN, } + /** + * Values for {@code core.trustXXX} options. + * + * @since 7.2 + */ + public enum TrustStat { + /** Do not trust file attributes of a File. */ + NEVER, + + /** Always trust file attributes of a File. */ + ALWAYS, + + /** Open and close the File to refresh its file attributes + * and then trust it. */ + AFTER_OPEN, + + /** + * Used for specific options to inherit value from value set for + * core.trustStat. + */ + INHERIT + } + private final int compression; private final int packIndexVersion; @@ -169,6 +205,16 @@ public class CoreConfig { private final boolean commitGraph; + private final TrustStat trustStat; + + private final TrustStat trustPackedRefsStat; + + private final TrustStat trustLooseRefStat; + + private final TrustStat trustPackStat; + + private final TrustStat trustLooseObjectStat; + /** * Options for symlink handling * @@ -198,7 +244,13 @@ public class CoreConfig { DOTGITONLY } - private CoreConfig(Config rc) { + /** + * Create a new core configuration from the passed configuration. + * + * @param rc + * git configuration + */ + CoreConfig(Config rc) { compression = rc.getInt(ConfigConstants.CONFIG_CORE_SECTION, ConfigConstants.CONFIG_KEY_COMPRESSION, DEFAULT_COMPRESSION); packIndexVersion = rc.getInt(ConfigConstants.CONFIG_PACK_SECTION, @@ -210,6 +262,60 @@ public class CoreConfig { commitGraph = rc.getBoolean(ConfigConstants.CONFIG_CORE_SECTION, ConfigConstants.CONFIG_COMMIT_GRAPH, DEFAULT_COMMIT_GRAPH_ENABLE); + + trustStat = parseTrustStat(rc); + trustPackedRefsStat = parseTrustPackedRefsStat(rc); + trustLooseRefStat = parseTrustLooseRefStat(rc); + trustPackStat = parseTrustPackFileStat(rc); + trustLooseObjectStat = parseTrustLooseObjectFileStat(rc); + } + + private static TrustStat parseTrustStat(Config rc) { + Boolean tfs = rc.getBoolean(ConfigConstants.CONFIG_CORE_SECTION, + ConfigConstants.CONFIG_KEY_TRUSTFOLDERSTAT); + TrustStat ts = rc.getEnum(TrustStat.values(), + ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_TRUST_STAT); + if (tfs != null) { + if (ts == null) { + LOG.warn(JGitText.get().deprecatedTrustFolderStat); + return tfs.booleanValue() ? TrustStat.ALWAYS : TrustStat.NEVER; + } + LOG.warn(JGitText.get().precedenceTrustConfig); + } + if (ts == null) { + ts = TrustStat.ALWAYS; + } else if (ts == TrustStat.INHERIT) { + LOG.warn(JGitText.get().invalidTrustStat); + ts = TrustStat.ALWAYS; + } + return ts; + } + + private TrustStat parseTrustPackedRefsStat(Config rc) { + return inheritParseTrustStat(rc, + ConfigConstants.CONFIG_KEY_TRUST_PACKED_REFS_STAT); + } + + private TrustStat parseTrustLooseRefStat(Config rc) { + return inheritParseTrustStat(rc, + ConfigConstants.CONFIG_KEY_TRUST_LOOSE_REF_STAT); + } + + private TrustStat parseTrustPackFileStat(Config rc) { + return inheritParseTrustStat(rc, + ConfigConstants.CONFIG_KEY_TRUST_PACK_STAT); + } + + private TrustStat parseTrustLooseObjectFileStat(Config rc) { + return inheritParseTrustStat(rc, + ConfigConstants.CONFIG_KEY_TRUST_LOOSE_OBJECT_STAT); + } + + private TrustStat inheritParseTrustStat(Config rc, String key) { + TrustStat t = rc.getEnum(ConfigConstants.CONFIG_CORE_SECTION, null, key, + TrustStat.INHERIT); + return t == TrustStat.INHERIT ? trustStat : t; } /** @@ -260,4 +366,56 @@ public class CoreConfig { public boolean enableCommitGraph() { return commitGraph; } + + /** + * Get how far we can trust file attributes of packed-refs file which is + * used to store {@link org.eclipse.jgit.lib.Ref}s in + * {@link org.eclipse.jgit.internal.storage.file.RefDirectory}. + * + * @return how far we can trust file attributes of packed-refs file. + * + * @since 7.2 + */ + public TrustStat getTrustPackedRefsStat() { + return trustPackedRefsStat; + } + + /** + * Get how far we can trust file attributes of loose ref files which are + * used to store {@link org.eclipse.jgit.lib.Ref}s in + * {@link org.eclipse.jgit.internal.storage.file.RefDirectory}. + * + * @return how far we can trust file attributes of loose ref files. + * + * @since 7.2 + */ + public TrustStat getTrustLooseRefStat() { + return trustLooseRefStat; + } + + /** + * Get how far we can trust file attributes of packed-refs file which is + * used to store {@link org.eclipse.jgit.lib.Ref}s in + * {@link org.eclipse.jgit.internal.storage.file.RefDirectory}. + * + * @return how far we can trust file attributes of packed-refs file. + * + * @since 7.2 + */ + public TrustStat getTrustPackStat() { + return trustPackStat; + } + + /** + * Get how far we can trust file attributes of loose ref files which are + * used to store {@link org.eclipse.jgit.lib.Ref}s in + * {@link org.eclipse.jgit.internal.storage.file.RefDirectory}. + * + * @return how far we can trust file attributes of loose ref files. + * + * @since 7.2 + */ + public TrustStat getTrustLooseObjectStat() { + return trustLooseObjectStat; + } } -- cgit v1.2.3 From 1ff9c2a1cbd5ac718559eef152d954755f257bc8 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 15 Nov 2024 08:37:10 +0100 Subject: FileReftableDatabase: consider ref updates by another process FileReftableDatabase didn't consider that refs might be changed by another process e.g. using git (which started supporting reftable with version 2.45). Add a test creating a light-weight tag which is updated using git running in another process and assert that FileReftableDatabase recognizes the tag modification. FileReftableStack#addReftable checks if the stack is up-to-date while it holds the FileLock for tables.list, if it is not up-to-date the RefUpdate fails with a LOCK_FAILURE to protect against lost ref updates if another instance of FileReftableDatabase or another thread or process updated the reftable stack since we last read it. If option `reftable.autoRefresh = true` or `setAutoRefresh(true)` was called check before each ref resolution if the reftable stack is up-to-date and, if necessary, reload the reftable stack automatically. Calling `setAutoRefresh(true)` takes precedence over the configured value for option `reftable.autoRefresh`. Add a testConcurrentRacyReload which tests that updates still abort ref updates if the reftable stack the update is based on was outdated. Bug: jgit-102 Change-Id: I1f9faa2afdbfff27e83ff295aef6d572babed4fe --- Documentation/config-options.md | 7 ++ org.eclipse.jgit.benchmarks/pom.xml | 4 + .../eclipse/jgit/benchmarks/GetRefsBenchmark.java | 15 +++ .../internal/storage/file/FileReftableTest.java | 134 ++++++++++++++++++++- .../storage/file/FileReftableDatabase.java | 54 +++++++++ .../src/org/eclipse/jgit/lib/ConfigConstants.java | 14 +++ 6 files changed, 227 insertions(+), 1 deletion(-) (limited to 'Documentation') diff --git a/Documentation/config-options.md b/Documentation/config-options.md index 807d4a8002..1dfe95f850 100644 --- a/Documentation/config-options.md +++ b/Documentation/config-options.md @@ -134,6 +134,13 @@ Proxy configuration uses the standard Java mechanisms via class `java.net.ProxyS | `pack.window` | `10` | ✅ | Number of objects to try when looking for a delta base per thread searching for deltas. | | `pack.windowMemory` | `0` (unlimited) | ✅ | Maximum number of bytes to put into the delta search window. | +## reftable options + +| option | default | git option | description | +|---------|---------|------------|-------------| +| `reftable.autoRefresh` | `false` | ⃞ | Whether to auto-refresh the reftable stack if it is out of date. | + + ## __repack__ options | option | default | git option | description | diff --git a/org.eclipse.jgit.benchmarks/pom.xml b/org.eclipse.jgit.benchmarks/pom.xml index c92fa9f156..e531245971 100644 --- a/org.eclipse.jgit.benchmarks/pom.xml +++ b/org.eclipse.jgit.benchmarks/pom.xml @@ -52,6 +52,10 @@ org.eclipse.jgit.junit ${project.version} + + junit + junit + diff --git a/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java b/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java index ea279fbba9..44e862e7c8 100644 --- a/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java +++ b/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java @@ -24,6 +24,7 @@ import java.util.stream.IntStream; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.internal.storage.file.FileReftableDatabase; import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ConfigConstants; @@ -39,8 +40,10 @@ import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FileUtils; +import org.junit.Assume; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; import org.openjdk.jmh.annotations.Measurement; import org.openjdk.jmh.annotations.Mode; import org.openjdk.jmh.annotations.OutputTimeUnit; @@ -67,6 +70,9 @@ public class GetRefsBenchmark { @Param({ "true", "false" }) boolean useRefTable; + @Param({ "true", "false" }) + boolean autoRefresh; + @Param({ "100", "1000", "10000", "100000" }) int numBranches; @@ -82,6 +88,9 @@ public class GetRefsBenchmark { @Setup @SuppressWarnings("boxing") public void setupBenchmark() throws IOException, GitAPIException { + // if we use RefDirectory skip autoRefresh = false + Assume.assumeTrue(useRefTable || autoRefresh); + String firstBranch = "firstbranch"; testDir = Files.createDirectory(Paths.get("testrepos")); String repoName = "branches-" + numBranches + "-trustStat-" @@ -98,6 +107,9 @@ public class GetRefsBenchmark { ((FileRepository) git.getRepository()).convertRefStorage( ConfigConstants.CONFIG_REF_STORAGE_REFTABLE, false, false); + FileReftableDatabase refdb = (FileReftableDatabase) git + .getRepository().getRefDatabase(); + refdb.setAutoRefresh(autoRefresh); } else { cfg.setEnum(ConfigConstants.CONFIG_CORE_SECTION, null, ConfigConstants.CONFIG_KEY_TRUST_STAT, @@ -113,6 +125,7 @@ public class GetRefsBenchmark { System.out.println("Preparing test"); System.out.println("- repository: \t\t" + repoPath); System.out.println("- refDatabase: \t\t" + refDatabaseType()); + System.out.println("- autoRefresh: \t\t" + autoRefresh); System.out.println("- trustStat: \t" + trustStat); System.out.println("- branches: \t\t" + numBranches); @@ -154,6 +167,7 @@ public class GetRefsBenchmark { @OutputTimeUnit(TimeUnit.MICROSECONDS) @Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS) @Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS) + @Fork(2) public void testGetExactRef(Blackhole blackhole, BenchmarkState state) throws IOException { String branchName = state.branches @@ -166,6 +180,7 @@ public class GetRefsBenchmark { @OutputTimeUnit(TimeUnit.MICROSECONDS) @Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS) @Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS) + @Fork(2) public void testGetRefsByPrefix(Blackhole blackhole, BenchmarkState state) throws IOException { String branchPrefix = "refs/heads/branch/" + branchIndex.nextInt(100) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java index 758ee93c30..5756b41442 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import java.io.File; import java.io.FileOutputStream; @@ -33,8 +34,15 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; - import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jgit.api.Git; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; @@ -51,6 +59,10 @@ import org.eclipse.jgit.lib.RepositoryCache; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.transport.ReceiveCommand; +import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.FS.ExecutionResult; +import org.eclipse.jgit.util.RawParseUtils; +import org.eclipse.jgit.util.TemporaryBuffer; import org.junit.Test; public class FileReftableTest extends SampleDataRepositoryTestCase { @@ -64,6 +76,30 @@ public class FileReftableTest extends SampleDataRepositoryTestCase { db.convertToReftable(false, false); } + @SuppressWarnings("boxing") + @Test + public void testReloadIfNecessary() throws Exception { + ObjectId id = db.resolve("master"); + try (FileRepository repo1 = new FileRepository(db.getDirectory()); + FileRepository repo2 = new FileRepository(db.getDirectory())) { + ((FileReftableDatabase) repo1.getRefDatabase()) + .setAutoRefresh(true); + ((FileReftableDatabase) repo2.getRefDatabase()) + .setAutoRefresh(true); + FileRepository repos[] = { repo1, repo2 }; + for (int i = 0; i < 10; i++) { + for (int j = 0; j < 2; j++) { + FileRepository repo = repos[j]; + RefUpdate u = repo.getRefDatabase().newUpdate( + String.format("branch%d", i * 10 + j), false); + u.setNewObjectId(id); + RefUpdate.Result r = u.update(); + assertEquals(Result.NEW, r); + } + } + } + } + @SuppressWarnings("boxing") @Test public void testRacyReload() throws Exception { @@ -97,6 +133,54 @@ public class FileReftableTest extends SampleDataRepositoryTestCase { } } + @Test + public void testConcurrentRacyReload() throws Exception { + ObjectId id = db.resolve("master"); + final CyclicBarrier barrier = new CyclicBarrier(2); + + class UpdateRef implements Callable { + + private RefUpdate u; + + UpdateRef(FileRepository repo, String branchName) + throws IOException { + u = repo.getRefDatabase().newUpdate(branchName, + false); + u.setNewObjectId(id); + } + + @Override + public RefUpdate.Result call() throws Exception { + barrier.await(); // wait for the other thread to prepare + return u.update(); + } + } + + ExecutorService pool = Executors.newFixedThreadPool(2); + try (FileRepository repo1 = new FileRepository(db.getDirectory()); + FileRepository repo2 = new FileRepository(db.getDirectory())) { + ((FileReftableDatabase) repo1.getRefDatabase()) + .setAutoRefresh(true); + ((FileReftableDatabase) repo2.getRefDatabase()) + .setAutoRefresh(true); + for (int i = 0; i < 10; i++) { + String branchName = String.format("branch%d", + Integer.valueOf(i)); + Future ru1 = pool + .submit(new UpdateRef(repo1, branchName)); + Future ru2 = pool + .submit(new UpdateRef(repo2, branchName)); + assertTrue((ru1.get() == Result.NEW + && ru2.get() == Result.LOCK_FAILURE) + || (ru1.get() == Result.LOCK_FAILURE + && ru2.get() == Result.NEW)); + } + } finally { + pool.shutdown(); + pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS); + } + } + @Test public void testCompactFully() throws Exception { ObjectId c1 = db.resolve("master^^"); @@ -651,6 +735,54 @@ public class FileReftableTest extends SampleDataRepositoryTestCase { checkContainsRef(refs, db.exactRef("HEAD")); } + @Test + public void testExternalUpdate_bug_102() throws Exception { + ((FileReftableDatabase) db.getRefDatabase()).setAutoRefresh(true); + assumeTrue(atLeastGitVersion(2, 45)); + Git git = Git.wrap(db); + git.tag().setName("foo").call(); + Ref ref = db.exactRef("refs/tags/foo"); + assertNotNull(ref); + runGitCommand("tag", "--force", "foo", "e"); + Ref e = db.exactRef("refs/heads/e"); + Ref foo = db.exactRef("refs/tags/foo"); + assertEquals(e.getObjectId(), foo.getObjectId()); + } + + private String toString(TemporaryBuffer b) throws IOException { + return RawParseUtils.decode(b.toByteArray()); + } + + private ExecutionResult runGitCommand(String... args) + throws IOException, InterruptedException { + FS fs = db.getFS(); + ProcessBuilder pb = fs.runInShell("git", args); + pb.directory(db.getWorkTree()); + System.err.println("PATH=" + pb.environment().get("PATH")); + ExecutionResult result = fs.execute(pb, null); + assertEquals(0, result.getRc()); + String err = toString(result.getStderr()); + if (!err.isEmpty()) { + System.err.println(err); + } + String out = toString(result.getStdout()); + if (!out.isEmpty()) { + System.out.println(out); + } + return result; + } + + private boolean atLeastGitVersion(int minMajor, int minMinor) + throws IOException, InterruptedException { + String version = toString(runGitCommand("version").getStdout()) + .split(" ")[2]; + System.out.println(version); + String[] digits = version.split("\\."); + int major = Integer.parseInt(digits[0]); + int minor = Integer.parseInt(digits[1]); + return (major >= minMajor) && (minor >= minMinor); + } + private RefUpdate updateRef(String name) throws IOException { final RefUpdate ref = db.updateRef(name); ref.setNewObjectId(db.resolve(Constants.HEAD)); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java index 6040ad554d..d70fa839e9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java @@ -16,6 +16,7 @@ import static org.eclipse.jgit.lib.Ref.Storage.PACKED; import java.io.File; import java.io.IOException; +import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -37,6 +38,7 @@ import org.eclipse.jgit.internal.storage.reftable.ReftableBatchRefUpdate; import org.eclipse.jgit.internal.storage.reftable.ReftableDatabase; import org.eclipse.jgit.internal.storage.reftable.ReftableWriter; import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdRef; @@ -70,6 +72,8 @@ public class FileReftableDatabase extends RefDatabase { private final FileReftableStack reftableStack; + private boolean autoRefresh; + FileReftableDatabase(FileRepository repo) throws IOException { this(repo, new File(new File(repo.getCommonDirectory(), Constants.REFTABLE), Constants.TABLES_LIST)); @@ -77,6 +81,9 @@ public class FileReftableDatabase extends RefDatabase { FileReftableDatabase(FileRepository repo, File refstackName) throws IOException { this.fileRepository = repo; + this.autoRefresh = repo.getConfig().getBoolean( + ConfigConstants.CONFIG_REFTABLE_SECTION, + ConfigConstants.CONFIG_KEY_AUTOREFRESH, false); this.reftableStack = new FileReftableStack(refstackName, new File(fileRepository.getCommonDirectory(), Constants.REFTABLE), () -> fileRepository.fireEvent(new RefsChangedEvent()), @@ -183,6 +190,7 @@ public class FileReftableDatabase extends RefDatabase { @Override public Ref exactRef(String name) throws IOException { + autoRefresh(); return reftableDatabase.exactRef(name); } @@ -193,6 +201,7 @@ public class FileReftableDatabase extends RefDatabase { @Override public Map getRefs(String prefix) throws IOException { + autoRefresh(); List refs = reftableDatabase.getRefsByPrefix(prefix); RefList.Builder builder = new RefList.Builder<>(refs.size()); for (Ref r : refs) { @@ -205,6 +214,7 @@ public class FileReftableDatabase extends RefDatabase { @Override public List getRefsByPrefixWithExclusions(String include, Set excludes) throws IOException { + autoRefresh(); return reftableDatabase.getRefsByPrefixWithExclusions(include, excludes); } @@ -223,6 +233,50 @@ public class FileReftableDatabase extends RefDatabase { } + /** + * Whether to auto-refresh the reftable stack if it is out of date. + * + * @param autoRefresh + * whether to auto-refresh the reftable stack if it is out of + * date. + */ + public void setAutoRefresh(boolean autoRefresh) { + this.autoRefresh = autoRefresh; + } + + /** + * Whether the reftable stack is auto-refreshed if it is out of date. + * + * @return whether the reftable stack is auto-refreshed if it is out of + * date. + */ + public boolean isAutoRefresh() { + return autoRefresh; + } + + private void autoRefresh() { + if (autoRefresh) { + refresh(); + } + } + + /** + * Check if the reftable stack is up to date, and if not, reload it. + *

+ * {@inheritDoc} + */ + @Override + public void refresh() { + try { + if (!reftableStack.isUpToDate()) { + reftableDatabase.clearCache(); + reftableStack.reload(); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + private Ref doPeel(Ref leaf) throws IOException { try (RevWalk rw = new RevWalk(fileRepository)) { RevObject obj = rw.parseAny(leaf.getObjectId()); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index 30e7de47c3..f62bf161a7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -1093,4 +1093,18 @@ public final class ConfigConstants { * @since 7.1 */ public static final String CONFIG_KEY_LOAD_REV_INDEX_IN_PARALLEL = "loadRevIndexInParallel"; + + /** + * The "reftable" section + * + * @since 7.2 + */ + public static final String CONFIG_REFTABLE_SECTION = "reftable"; + + /** + * The "autorefresh" key + * + * @since 7.2 + */ + public static final String CONFIG_KEY_AUTOREFRESH = "autorefresh"; } -- cgit v1.2.3 From ac5146ffbf9d331e852059b2eb14841927f301a1 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Wed, 8 Jan 2025 18:38:31 +0100 Subject: FileReftableStack: use FileSnapshot to detect modification Reading file attributes is faster than reading file content hence use FileSnapshot to speedup detecting if FileReftableStack is up-to-date. Introduce new option "core.trustTablesListStat" allowing to configure if we can trust file attributes of the "tables.list" file to speedup detection of file modifications. This file is used to store the list of filenames of the files storing Reftables in FileReftableDatabase. If this option is set to "ALWAYS" we trust File attributes and use them to speedup detection of file modifications. If set to "NEVER" the content of the "tables.list" file is always read unconditionally. This can help to avoid caching issues on some filesystems. If set to "AFTER_OPEN" we will open a FileInputStream to refresh File attributes of the "tables.list" file before relying on the refreshed File attributes to detect modifications. This works on some NFS filesystems and is faster than using "NEVER". Change-Id: I3e288d90fb07edf4fa2a03c707a333b26f0c458d --- Documentation/config-options.md | 3 +- .../internal/storage/file/FileReftableStack.java | 40 ++++++++++++++++++++-- .../src/org/eclipse/jgit/lib/ConfigConstants.java | 8 +++++ .../src/org/eclipse/jgit/lib/CoreConfig.java | 24 +++++++++++++ 4 files changed, 71 insertions(+), 4 deletions(-) (limited to 'Documentation') diff --git a/Documentation/config-options.md b/Documentation/config-options.md index 1dfe95f850..4dde8f8c15 100644 --- a/Documentation/config-options.md +++ b/Documentation/config-options.md @@ -58,9 +58,10 @@ For details on native git options see also the official [git config documentatio | ~~`core.trustFolderStat`~~ | `true` | ⃞ | __Deprecated__, use `core.trustStat` instead. If set to `true` translated to `core.trustStat=always`, if `false` translated to `core.trustStat=never`, see below. If both `core.trustFolderStat` and `core.trustStat` are configured then `trustStat` takes precedence and `trustFolderStat` is ignored. | | `core.trustLooseRefStat` | `inherit` | ⃞ | Whether to trust the file attributes of loose refs and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `trustStat`. | | `core.trustPackedRefsStat` | `inherit` | ⃞ | Whether to trust the file attributes of the packed-refs file. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | +| `core.trustTablesListStat` | `inherit` | ⃞ | Whether to trust the file attributes of the `tables.list` file used by the reftable ref storage backend to store the list of reftable filenames. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. The reftable backend is used if `extensions.refStorage = reftable`. | | `core.trustLooseObjectStat` | `inherit` | ⃞ | Whether to trust the file attributes of the loose object file and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | | `core.trustPackStat` | `inherit` | ⃞ | Whether to trust the file attributes of the `objects/pack` directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. | -| `core.trustStat` | `always` | ⃞ | Global option to configure whether to trust file attributes (Java equivalent of stat command on Unix) of files storing git objects. Can be overridden for specific files by configuring `core.trustLooseRefStat, core.trustPackedRefsStat, core.trustLooseObjectStat, core.trustPackStat`. If `never` JGit will ignore the file attributes of the file and always read it. If `always` JGit will trust the file attributes and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, but file attributes are only considered *after* the file itself and any transient parent directories have been opened and closed. An open/close of the file/directory is known to refresh its file attributes, at least on some NFS clients. | +| `core.trustStat` | `always` | ⃞ | Global option to configure whether to trust file attributes (Java equivalent of stat command on Unix) of files storing git objects. Can be overridden for specific files by configuring `core.trustLooseRefStat, core.trustPackedRefsStat, core.trustLooseObjectStat, core.trustPackStat,core.trustTablesListStat`. If `never` JGit will ignore the file attributes of the file and always read it. If `always` JGit will trust the file attributes and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, but file attributes are only considered *after* the file itself and any transient parent directories have been opened and closed. An open/close of the file/directory is known to refresh its file attributes, at least on some NFS clients. | | `core.worktree` | Root directory of the working tree if it is not the parent directory of the `.git` directory | ✅ | The path to the root of the working tree. | ## __fetch__ options diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java index 0f5ff0f9f7..b2c88922b8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java @@ -18,8 +18,10 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.io.InputStream; import java.io.InputStreamReader; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.StandardCopyOption; import java.security.SecureRandom; import java.util.ArrayList; @@ -27,6 +29,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -39,6 +42,8 @@ import org.eclipse.jgit.internal.storage.reftable.ReftableConfig; import org.eclipse.jgit.internal.storage.reftable.ReftableReader; import org.eclipse.jgit.internal.storage.reftable.ReftableWriter; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.CoreConfig; +import org.eclipse.jgit.lib.CoreConfig.TrustStat; import org.eclipse.jgit.util.FileUtils; import org.eclipse.jgit.util.SystemReader; @@ -59,6 +64,9 @@ public class FileReftableStack implements AutoCloseable { private List stack; + private AtomicReference snapshot = new AtomicReference<>( + FileSnapshot.DIRTY); + private long lastNextUpdateIndex; private final File stackPath; @@ -98,6 +106,8 @@ public class FileReftableStack implements AutoCloseable { private final CompactionStats stats; + private final TrustStat trustTablesListStat; + /** * Creates a stack corresponding to the list of reftables in the argument * @@ -126,6 +136,8 @@ public class FileReftableStack implements AutoCloseable { reload(); stats = new CompactionStats(); + trustTablesListStat = configSupplier.get().get(CoreConfig.KEY) + .getTrustTablesListStat(); } CompactionStats getStats() { @@ -272,8 +284,9 @@ public class FileReftableStack implements AutoCloseable { } private List readTableNames() throws IOException { + FileSnapshot old; List names = new ArrayList<>(stack.size() + 1); - + old = snapshot.get(); try (BufferedReader br = new BufferedReader( new InputStreamReader(new FileInputStream(stackPath), UTF_8))) { String line; @@ -282,8 +295,10 @@ public class FileReftableStack implements AutoCloseable { names.add(line); } } + snapshot.compareAndSet(old, FileSnapshot.save(stackPath)); } catch (FileNotFoundException e) { // file isn't there: empty repository. + snapshot.compareAndSet(old, FileSnapshot.MISSING_FILE); } return names; } @@ -294,9 +309,28 @@ public class FileReftableStack implements AutoCloseable { * on IO problem */ boolean isUpToDate() throws IOException { - // We could use FileSnapshot to avoid reading the file, but the file is - // small so it's probably a minor optimization. try { + switch (trustTablesListStat) { + case NEVER: + break; + case AFTER_OPEN: + try (InputStream stream = Files + .newInputStream(stackPath.toPath())) { + // open the tables.list file to refresh attributes (on some + // NFS clients) + } catch (FileNotFoundException | NoSuchFileException e) { + // ignore + } + //$FALL-THROUGH$ + case ALWAYS: + if (!snapshot.get().isModified(stackPath)) { + return true; + } + break; + case INHERIT: + // only used in CoreConfig internally + throw new IllegalStateException(); + } List names = readTableNames(); if (names.size() != stack.size()) { return false; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index f62bf161a7..c4550329d3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -1045,6 +1045,14 @@ public final class ConfigConstants { * @since 7.2 */ public static final String CONFIG_KEY_TRUST_LOOSE_OBJECT_STAT = "trustLooseObjectStat"; + + /** + * The "trustTablesListStat" key + * + * @since 7.2 + */ + public static final String CONFIG_KEY_TRUST_TABLESLIST_STAT = "trustTablesListStat"; + /** * The "pack.preserveOldPacks" key * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java index e43c9653dd..0e27b2743c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java @@ -215,6 +215,8 @@ public class CoreConfig { private final TrustStat trustLooseObjectStat; + private final TrustStat trustTablesListStat; + /** * Options for symlink handling * @@ -268,6 +270,7 @@ public class CoreConfig { trustLooseRefStat = parseTrustLooseRefStat(rc); trustPackStat = parseTrustPackFileStat(rc); trustLooseObjectStat = parseTrustLooseObjectFileStat(rc); + trustTablesListStat = parseTablesListStat(rc); } private static TrustStat parseTrustStat(Config rc) { @@ -318,6 +321,13 @@ public class CoreConfig { return t == TrustStat.INHERIT ? trustStat : t; } + private TrustStat parseTablesListStat(Config rc) { + TrustStat t = rc.getEnum(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_TRUST_TABLESLIST_STAT, + TrustStat.INHERIT); + return t == TrustStat.INHERIT ? trustStat : t; + } + /** * Get the compression level to use when storing loose objects * @@ -418,4 +428,18 @@ public class CoreConfig { public TrustStat getTrustLooseObjectStat() { return trustLooseObjectStat; } + + /** + * Get how far we can trust file attributes of the "tables.list" file which + * is used to store the list of filenames of the files storing + * {@link org.eclipse.jgit.internal.storage.reftable.Reftable}s in + * {@link org.eclipse.jgit.internal.storage.file.FileReftableDatabase}. + * + * @return how far we can trust file attributes of the "tables.list" file. + * + * @since 7.2 + */ + public TrustStat getTrustTablesListStat() { + return trustTablesListStat; + } } -- cgit v1.2.3