Implement a helper method suppressing the ReferenceEquality error prone
warning and use it to fix this warning in static equals methods where
this comparison is used to implement fast path of static equals
implementation.
See https://errorprone.info/bugpattern/ReferenceEquality
Change-Id: I33538a3406007d24efec3a504e031ca1069572ed
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Move the implementation of the static equals() method to a new method
and suppress the error. Deprecate the old method to signal that we
intend to remove it in the next major release.
See https://errorprone.info/bugpattern/AmbiguousMethodReference
Change-Id: I5e29c97f4db3e11770be589a6ccd785e2c9ac7f2
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Error Prone reports:
[ReferenceEquality] Comparison using reference equality instead of
value equality
The END_EDIT instance is used as a marker, and thus it's OK to use
a reference equality comparison. Factor the comparison to a method
and add a suppression.
Change-Id: I7d9dc1fa21f46c984787056b0b5d163e313026a6
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Error Prone reports:
[NarrowingCompoundAssignment] Compound assignments from long to int
hide lossy casts
and
[NarrowingCompoundAssignment] Compound assignments from int to byte
hide lossy casts
See https://errorprone.info/bugpattern/NarrowingCompoundAssignment
Fix the warnings by adding explicit casts or changing types as
necessary.
Now that all occurrences of the warning are fixed, increase its
severity to ERROR.
Change-Id: Idb3670e6047b146ae37daee07212ff9455512623
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Android unconditionally throws a SecurityException;[1] getFileStore()
is not supported. Catch the exception and don't attempt the hard-
linking atomic file mechanism.
[1] https://android.googlesource.com/platform/libcore/+/21e6175e25
Bug: 548947
Change-Id: Idfba2d9dbcbc80ea15ab2ae7889e5142444c1581
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Increase the safety factor to 2.5x for extra safety if max of measured
timestamp resolution and measured minimal racy threshold is < 100ms, use
1.25 otherwise since for large filesystem resolution values the
influence of finite resolution of the system clock should be negligible.
Before, not yet using the newly introduced minRacyThreshold measurement,
the threshold was 1.1x FS resolution, and we could issue the
following sequence of events,
start
create-file
read-file (currentTime)
end
which had the following timestamps:
create-file 1564589081998
start 1564589082002
read 1564589082003
end 1564589082004
In this case, the difference between create-file and read is 5ms,
which exceeded the 4ms FS resolution, even though the events together
took just 2ms of runtime.
Reproduce with:
bazel test --runs_per_test=100 \
//org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_FileSnapshotTest
The file system timestamp resolution is 4ms in this case.
This code assumes that the kernel and the JVM use the same clock that
is synchronized with the file system clock. This seems plausible,
given the resolution of System.currentTimeMillis() and the latency for
a gettimeofday system call (typically ~1us), but it would be good to
justify this with specifications.
Also cover a source of flakiness: if the test runs under extreme load,
then we could have
start
create-file
<long delay>
read
end
which would register as an unmodified file. Avoid this by skipping the
test if end-start is too big.
[msohn]:
- downported from master to stable-5.1
- skip test if resolution is below 10ms
- adjust safety factor to 1.25 for resolutions above 100ms
Change-Id: I87d2cf035e01c44b7ba8364c410a860aa8e312ef
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Since we now measure file time resolution we can use it to replace the
hard coded wait time of 25ms. FileSnapshot#equals will return true until
the mtime of the old (o) and the new FileSnapshot (n) differ by at least
one file time resolution.
Change-Id: Icb713a80ce9eb929242ed083406bfb6650c72223
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Cache FileStoreAttributeCache entries since looking up FileStore for a
file may be expensive on some platforms.
Implement a simple LRU cache based on ConcurrentHashMap using a simple
long counter to order access to cache entries.
Change-Id: I4881fa938ad2f17712c05da857838073a2fc4ddb
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Signed-off-by: Marc Strapetz <marc.strapetz@syntevo.com>
Also-By: Marc Strapetz <marc.strapetz@syntevo.com>
Use the fallback timestamp resolution as already described in the
javadoc of these methods. Using zero file timestamp resolution doesn't
make sense.
Change-Id: Iaad2a0f99c3be3678e94980a0a368181b6aed38c
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
To enable persisting the minimal racy threshold per FileStore add a
new config option to the user global git configuration:
- Config section is "filesystem"
- Config subsection is concatenation of
- Java vendor (system property "java.vendor")
- Java version (system property "java.version")
- FileStore's name, on Windows we use the attribute volume:vsn instead
since the name is not necessarily unique.
- separated by '|'
e.g.
"AdoptOpenJDK|1.8.0_212-b03|/dev/disk1s1"
The same prefix is used as for filesystem timestamp resolution, so
both values are stored in the same config section
- The config key for minmal racy threshold is "minRacyThreshold" as a
time value, supported time units are those supported by
DefaultTypedConfigGetter#getTimeUnit
- measure for 3 seconds to limit runtime which depends on hardware, OS
and Java version being used
If the minimal racy threshold is configured for a given FileStore the
configured value is used instead of measuring it.
When the minimal racy threshold was measured it is persisted in the user
global git configuration.
Rename FileStoreAttributeCache to FileStoreAttributes since this class
is now declared public in order to enable exposing all attributes in one
object.
Example:
[filesystem "AdoptOpenJDK|11.0.3|/dev/disk1s1"]
timestampResolution = 7000 nanoseconds
minRacyThreshold = 3440 microseconds
Change-Id: I22195e488453aae8d011b0a8e3276fe3d99deaea
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Also-By: Marc Strapetz <marc.strapetz@syntevo.com>
By running FileSnapshotTest#detectFileModified we found that the sum of
measured filesystem timestamp resolution and measured clock resolution
may yield a too small interval after a file has been modified which we
need to consider racily clean. In our tests we didn't find this behavior
on all systems we tested on, e.g. on MacOS using APFS and Java 8 and 11
this effect was not observed.
On Linux (SLES 15, kernel 4.12.14-150.22-default) we collected the
following test results using Java 8 and 11:
In 23-98% of 10000 test runs (depending on filesystem type and Java
version) the test failed, which means the effective interval which needs
to be considered racily clean after a file was modified is larger than
the measured file timestamp resolution.
"delta" is the observed interval after a file has been modified but
FileSnapshot did not yet detect the modification:
"resolution" is the measured sum of file timestamp resolution and clock
resolution seen in Java.
Java version filesystem failures resolution min delta max delta
1.8.0_212-b04 btrfs 98.6% 1 ms 3.6 ms 6.6 ms
1.8.0_212-b04 ext4 82.6% 3 ms 1.1 ms 4.1 ms
1.8.0_212-b04 xfs 23.8% 4 ms 3.7 ms 3.9 ms
1.8.0_212-b04 zfs 23.1% 3 ms 4.8 ms 5.0 ms
11.0.3+7 btrfs 98.1% 3 us 0.7 ms 4.7 ms
11.0.3+7 ext4 98.1% 6 us 0.7 ms 4.7 ms
11.0.3+7 xfs 98.5% 7 us 0.1 ms 8.0 ms
11.0.3+7 zfs 98.4% 7 us 0.7 ms 5.2 ms
Mac OS
1.8.0_212 APFS 0% 1 s
11.0.3+7 APFS 0% 6 us
The observed delta is not distributed according to a normal gaussian
distribution but rather random in the observed range between "min delta"
and "max delta".
Run this test after measuring file timestamp resolution in
FS.FileAttributeCache to auto-configure JGit since it's unclear what
mechanism is causing this effect.
In FileSnapshot#isRacyClean use the maximum of the measured timestamp
resolution and the measured "delta" as explained above to decide if a
given FileSnapshot is to be considered racily clean. Add a 30% safety
margin to ensure we are on the safe side.
Change-Id: I1c8bb59f6486f174b7bbdc63072777ddbe06694d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
These are useful to avoid typos, and also for tab completion.
Change-Id: I0f2d267e46b36bc40297c9657c447f3fd8b9f831
Signed-off-by: David Turner <dturner@twosigma.com>
In a subsequent patch, in some cases, PackWriter#writePack will be
responsible for both the "packfile-uris" and "packfile" sections,
meaning that (in these cases) it must write the "packfile" section
header itself.
In preparation for that patch, move the writing of the "packfile"
section header closer to the invocation of PackWriter#writePack when the
entire fetch response is configured to use the sideband. This means that
"packfile" is written *after* objects are counted (and progress messages
sent to the client in sideband 2) when the "sideband-all" feature is
used (whether "packfile-uris" is used or not), and written *before*
objects are counted otherwise.
Having code to write "packfile" in two places is unfortunate but
necessary. When "sideband-all" is not used, object counting has to
happen after "packfile" is written, because "packfile" activates the
sideband that allows counting progress to be transmitted. When
"packfile-uris" is used, object counting has to happen before "packfile"
is written, because object counting determines whether to send
"packfile-uris" or "packfile". When "sideband-all" is used but
"packfile-uris" is not used, either way works; this commit uses
"packfile-uris" behavior in this case.
Also make the naming of the sideband-activating methods in PacketLineOut
more consistent.
Change-Id: Ifbfd26cc26af10c41b77758168833702d6983df1
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
We should not list the complete cache but only show the cache entry at
hand.
Change-Id: I22be2a4dcbf0145155e23f2389bfcf5662cf23a6
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Repeat the test 10000 times to get statistics if measured
fsTimestampResolution is working in practice to detect racy git
situations.
Add a class to compute statistics for this test. Log delta between
lastModified and time when FileSnapshot failed to detect modification.
This happens if the racy git limit determined by measuring filesystem
timestamp resolution and clock resolution is too small. If it would be
correct FileSnapshot would always detect modification or mark it
modified if time since modification is smaller than the racy git limit.
Change-Id: Iabe7af1a7211ca58480f8902d4fa4e366932fc77
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
The newly introduced ProtocolV2HookChain is implemented using lists
instead of arrays.
Update PostUploadHookChain to keep the hook chains implementation
consistent.
Change-Id: I5ae0c923f117ac48558a989464f5d5d868d81f76
Signed-off-by: Ivan Frade <ifrade@google.com>
The newly introduced ProtocolV2HookChain is implemented using lists
instead of arrays.
Update PostUploadHookChain to keep hook chain implementations
consistent.
Change-Id: Ic5694feab943e8949896b93103dbf427716c9bd7
Signed-off-by: Ivan Frade <ifrade@google.com>
Set synchronized to make the config access consistent.
> Inconsistent synchronization of
org.eclipse.jgit.transport.JschConfigSessionFactory.config; locked 80%
of time
In order to make JschConfigSessionFactory threadsafe, synchronize this
method as well.
Change-Id: I32d1bfc2e98363d254992144e795ce72fe1e8846
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
Measure granularity of timestamps stored in the filesystem by setting
and then getting lastModified timestamp until the read value changed.
Increase increment exponentially to limit number of iterations starting
with 1 microsecond since Java's FileTime (up to Java 12) truncates
timestamps to 1 microsecond resolution. The chosen algorithm yields 2000
steps between 1 ms and 2.5 s.
Also measure clock resolution and add that for the total timestamp
resolution. This avoids systematic measurement errors introduced by
doing IO to touch a file.
Change-Id: I9b37138619422452373e298d9d8c7cb2c384db3f
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
We should not use configuration when creating FileSnapshot when
accessing FileBasedConfig.
Change-Id: Ic521632870f18bb004751642b9d30648dd94049a
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
A caller cannot install a second hook in the UploadPack without
overwriting whatever is already there.
Offer a method to get the current protocol v2 hook, so it can be chained
with new hooks.
Change-Id: Icb06f94ec52b8c8714f509b5b8622d6db42960fb
Signed-off-by: Ivan Frade <ifrade@google.com>
UploadPack only supports one protocol-v2 hook. There are already cases
where more than one is needed.
Offer a Chain class to compose ProtocolV2Hooks, as other hooks do. It
looks like a single hook but it calls all its members.
Change-Id: Idd173ca7df6672079ac0de03c67f77abac376538
Signed-off-by: Ivan Frade <ifrade@google.com>
This enables higher file timestamp resolution on filesystems like ext4,
Mac APFS (1ns) or NTFS (100ns) providing high timestamp resolution on
filesystem level.
Note:
- on some OSes Java 8,9 truncate milliseconds, see
https://bugs.openjdk.java.net/browse/JDK-8177809, fixed in Java 10
- UnixFileAttributes truncates timestamp resolution to microseconds when
converting the internal representation to FileTime exposed in the API,
see https://bugs.openjdk.java.net/browse/JDK-8181493
- WindowsFileAttributes also provides only microsecond resolution
Change-Id: I25ffff31a3c6f725fc345d4ddc2f26da3b88f6f2
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
On Android FS#getFsTimestampResolution always throws a
SecurityException, handle this by falling back to the fallback timestamp
resolution.
Bug: 548947
Change-Id: I0ee6cb3c20e189bdc8d488434a930427ad6f2df2
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
It seems on cygwin creating a file under a writable directory can fail
with AccessDeniedException. Log a warning in this case and fallback to
worst case timestamp resolution of 2 seconds.
Bug: 548648
Change-Id: Ic50c31ce9dc9ccadd4db5247df929418ac62d45c
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Checking lastModified is time critical hence debug trace is the only way
to analyze issues since debugging is impractical.
Also add configuration for buffering of log4j output to reduce runtime
impact when debug trace is on. Limit buffer to 1MiB and comment this
configuration out since we may not always want to use buffering.
Change-Id: Ib1a0537b67c8dc3fac994a77b42badd974ce6c97
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Use options
- StandardOpenOption.CREATE to create touched file if not existing
- StandardOpenOption.SYNC to enforce synch of data and meta data changes
- StandardOpenOption.WRITE
Also set mtime explicitly in FileUtils#touch to the current system time.
This should fix that the previous implementation didn't work on
- locally cached Windows network share (CSC-CACHE filesystem) mapped as
a drive
- nfsv4 mounts on Linux
and that it didn't create unborn file like Linux command "touch".
Apache common's and Guava's touch() use the same approach.
Immediately after creating the probe file used to measure timestamp
resolution touch it. This ensures we always use the local system clock
when measuring filesystem timestamp resolution. This should prevent that
clock skew could influence the measured timestamp resolution in case of
a mounted network filesystem.
Bug: 548598
Change-Id: Iaeaf5967963f582395a195aa637b8188bfadac60
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
To enable persisting filesystem timestamp resolution per FileStore add a
new config section to the user global git configuration:
- Config section is "filesystem"
- Config subsection is concatenation of
- Java vendor (system property "java.vm.vendor")
- runtime version (system property "java.vm.version")
- FileStore's name
- separated by '|'
e.g.
"AdoptOpenJDK|1.8.0_212-b03|/dev/disk1s1"
The prefix is needed since some Java versions do not expose the full
timestamp resolution of the underlying filesystem. This may also
depend on the underlying operating system hence concrete key values
may not be portable.
- Config key for timestamp resolution is "timestampResolution" as a time
value, supported time units are those supported by
DefaultTypedConfigGetter#getTimeUnit
If timestamp resolution is already configured for a given FileStore
the configured value is used instead of measuring the resolution.
When timestamp resolution was measured it is persisted in the user
global git configuration.
Example:
[filesystem "AdoptOpenJDK|1.8.0_212-b03|/dev/disk1s1"]
timestampResolution = 1 seconds
If locking the git config file fails retry saving the resolution up to 5
times in order to workaround races with another thread.
In order to avoid stack overflow use the fallback filesystem timestamp
resolution when loading FileBasedConfig which creates itself a
FileSnapshot to help checking if the config changed.
Note:
- on some OSes Java 8,9 truncate to milliseconds or seconds, see
https://bugs.openjdk.java.net/browse/JDK-8177809, fixed in Java 10
- UnixFileAttributes up to Java 12 truncates timestamp resolution to
microseconds when converting the internal representation to FileTime
exposed in the API, see https://bugs.openjdk.java.net/browse/JDK-8181493
- WindowsFileAttributes also provides only microsecond resolution up to
Java 12
Hence do not attempt to manually configure a higher timestamp resolution
than supported by the Java version being used at runtime.
Bug: 546891
Bug: 548188
Change-Id: Iff91b8f9e6e5e2295e1463f87c8e95edf4abbcf8
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Allow the client to specify "sideband-all" in a fetch v2 request,
indicating that the whole response is to be multiplexed (with a sideband
indicator on every non-flush and non-delim pkt) instead of only the
packfile being multiplexed. This allows, for example, progress messages
to be sent at any point in the response.
This implements the "sideband-all" feature documented in
Documentation/technical/protocol-v2.txt in Git.
Change-Id: I3e7f21c88ff0982b1b7ebb09c9ad6c742c4483c8
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
It is difficult to track what's happening with the pckOut instance
field, so replace it with a local variable in #upload instead.
Change-Id: Ibd9225b28334b7133eccdc6d82b26fc96cbde299
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Use brackets in all "if" statements and remove the "final" from local
variables and method arguments.
https://wiki.eclipse.org/EGit/Contributor_Guide#Coding_standards
Change-Id: I185f3112848fc1218cd7adb9828488f03fa4ddfc
Signed-off-by: Ivan Frade <ifrade@google.com>
In order to avoid blocking on the main thread during measurement
interactive applications like EGit may want to measure the filesystem
timestamp resolution asynchronously.
In order to enable measurement in the background call
FileStoreAttributeCache.setAsyncfileStoreAttrCache(true)
before the first access to cached FileStore attributes.
Bug: 548188
Change-Id: I8c9a2dbfc3f1d33441edea18b90e36b1dc0156c7
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add a unittest.
In commit I5485db55 ("Fix FileSnapshot's consideration of file size"),
the special casing of UNKNOWN_SIZE was forgotten.
This change, together with I493f3b57b ("Measure file timestamp
resolution used in FileSnapshot") introduced a regression that would
occasionally surface in Gerrit integration tests marked UseLocalDisk,
with the symptom that creating the Admin user in NoteDb failed with a
LOCK_FAILURE.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I7ffd972581f815c144f810481103c7985af5feb0
Fixes PDE API checks complaining: the methods were added
in JGit 5.5.0.
Change-Id: I9ff860c3408c6bb3891fa0da7547394d0fe9d0b6
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
It was reported that measuring file timestamp resolution may hang
indefinitely on nfs. Hence timeout this measurement at the known worst
filesystem timestamp resolution (FAT) of 2 seconds.
Bug: 548188
Change-Id: I17004b0aa49d5b0e76360a008af3adb911b289c0
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
All other exceptions are handled in a wrapped sendPack method.
Consolidate the error handling code.
Change-Id: Ieac0ce64960534d009d1e6b025130b021b744794
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
By doing this, exceptions thrown by sendPack are also covered by the
same code.
Change-Id: I3509f2d832af1410f307e931577e4d07e32b014e
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
When another exception is thrown while handling another exception, that
exception can be attached to the original exception since Java 7
(Throwable#getSuppressed). Attach the secondary exception to the
original exception instead of throwing it away.
Change-Id: Ia093b8207714f2638e0343bc45a83d4342947505
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
RevWalk does not currently provide a --first-parent equivalent and the
feature has been requested.
Add a field to the RevWalk class to specify whether walks should
traverse first parents only. Modify Generator implementations to support
the feature.
Change-Id: I4a9a0d5767f82141dcf6d08659d7cb77c585fae4
Signed-off-by: Dave Borowitz <dborowitz@google.com>
Signed-off-by: Alex Spradlin <alexaspradlin@google.com>
If a client clones with "--filter=blob:none", the checkout that "git
clone" automatically does causes the client to fetch all blobs at HEAD.
When fetching from a non-bitmapped repository, this will fail if an
object walk is ever needed, because JGit currently rejects such requests
- see the commit message of d3021788d2 ("Use bitmaps for non-commit
reachability checks", 2017-11-10) for more information.
Rejecting such requests in the absence of bitmaps is probably
overzealous: it is true that the server would prefer to have bitmaps in
this case, but there might be a small proportion of repos (for example,
very small repos or newly created ones) that do not have bitmaps, yet
the server would still like to have partial clones for them.
So, allow such requests, performing the object walk reachability check
if necessary. Limit this to servers with "uploadpack.allowFilter"
configured, so that servers wanting to support partial clone have this
functionality, and servers that do not support partial clone do not have
to pay the object walk reachability check cost.
Change-Id: I51964bafec68696a799625d627615b4f45ddbbbf
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Error Prone reports:
[ClassNewInstance] Class.newInstance() bypasses exception checking;
prefer getDeclaredConstructor().newInstance()
See https://errorprone.info/bugpattern/ClassNewInstance
This was the only occurrence of the warning in the code base; now it's
fixed, increase the severity to ERROR to prevent future occurrences.
Change-Id: Ic04d1c5d2bd458bbb4bb399d6ce9d147bd48d0b1
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Matthias Sohn
David Pursehouse
Thomas Wolf
Han-Wen Nienhuys
David Turner
Jonathan Tan
Ivan Frade
Masaya Suzuki
Michael Keppler
Dave Borowitz