Dariusz Luksza [Thu, 15 Feb 2024 10:43:11 +0000 (10:43 +0000)]
Merge branch 'stable-6.7' into stable-6.8
* stable-6.7:
RefDirectory: Do not unlock until after deleting loose ref
Add missing javadoc description for declared exception
SnapshottingRefDirectory: Invalidate snapshot after locking ref for update
SnapshottingRefDir: Replace lambas with method refs
SnapshottingRefDir: Reduce casts with overrides
[errorprone] Fix wrong comparison which always evaluated to false
[errorprone] Remove unnecessary comparison
Dariusz Luksza [Thu, 15 Feb 2024 10:36:41 +0000 (10:36 +0000)]
Merge branch 'stable-6.6' into stable-6.7
* stable-6.6:
RefDirectory: Do not unlock until after deleting loose ref
Add missing javadoc description for declared exception
SnapshottingRefDirectory: Invalidate snapshot after locking ref for update
SnapshottingRefDir: Replace lambas with method refs
SnapshottingRefDir: Reduce casts with overrides
Nasser Grainawi [Fri, 26 Jan 2024 01:59:15 +0000 (18:59 -0700)]
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.
Nasser Grainawi [Thu, 25 Jan 2024 23:29:05 +0000 (16:29 -0700)]
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.
Matthias Sohn [Fri, 15 Sep 2023 09:48:05 +0000 (11:48 +0200)]
[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
Matthias Sohn [Fri, 15 Sep 2023 09:44:09 +0000 (11:44 +0200)]
[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
Dariusz Luksza [Mon, 12 Feb 2024 09:58:41 +0000 (09:58 +0000)]
Merge branch 'stable-6.7' into stable-6.8
* stable-6.7:
Improve handling of NFS stale handle errors
Fix handling of missing pack index file
Add tests for handling pack files removal during fetch
Dariusz Luksza [Mon, 12 Feb 2024 09:56:36 +0000 (09:56 +0000)]
Merge branch 'stable-6.6' into stable-6.7
* stable-6.6:
Improve handling of NFS stale handle errors
Fix handling of missing pack index file
Add tests for handling pack files removal during fetch
Dariusz Luksza [Mon, 20 Nov 2023 11:53:19 +0000 (11:53 +0000)]
Improve handling of NFS stale handle errors
Mark packfile as invalid when NFS stale handle error occurs.
This should fix broken fetch operations when the repo is located on the
NFS system and is GC'ed on a separate system (or process). Which may
result in the index, pack or bitmap file being removed when they are
accessed from the fetch operation.
Dariusz Luksza [Mon, 20 Nov 2023 11:00:51 +0000 (11:00 +0000)]
Fix handling of missing pack index file
As demonstrated in
`UploadPackHandleDeletedPackFile.testV2IdxFileRemovedDuringUploadPack`
the fetch operation will fail when the pack index file is removed.
This is due to a wrapping of `FileNotFoundException` (which is a
subclass of `IOExeption`) in an `IOException` at PackIndex L#68. This
is then changing the behaviour of error handling in
`Pack.file.getBitmapIndex()` where the `FileNotFoundException` is
swallowed and allows the fetch process to continue. With FNFE being
wrapped in IOE, this blows up and breaks the fetch operation.
Simply rethrowing `FileNotFoundException` from `PackFile.open()` fixes
the broken fetch operation. This will also mark the whole pack as
invalid in the `IOException` handler in `Pack.idx()` method.
Dariusz Luksza [Fri, 17 Nov 2023 19:28:53 +0000 (19:28 +0000)]
Add tests for handling pack files removal during fetch
Although this could sound like a corner case, it really can occur out
there in the real world. Especially in the Gerrit world where the
repositories could be GC'ed on a separate process or system.
The `FileNotFoundException` seems to be handled correctly in
`PackFile#doOpen` (line 671) and it will mark the pack as invalid. But
triggering that code path was not an easy task.
First of all, we need to add a new commit to the `master` branch of the
test repository after `UploadPack` object is created.
Secondly, in the refspec for fetch, commit id instead of "regular"
refspec must be used.
With both in place, we can see a warning log statement about deleted
pack file. And the fetch succeeds!
Also, tests for the removal of *.idx and *.bitmap files were added.
This unveiled a corner for the *.idx file deletion while fetching, as
the test will fail with "Unreachable pack index" IOException only
when the HEAD commit is empty.
Matthias Sohn [Fri, 19 Jan 2024 23:40:42 +0000 (00:40 +0100)]
Merge branch 'stable-6.7' into stable-6.8
* stable-6.7:
Introduce a PriorityQueue sorting RevCommits by commit timestamp
Remove org.eclipse.jgit.benchmark/.factorypath
Update jmh to 1.37 for org.eclipse.jgit.benchmark
Matthias Sohn [Fri, 19 Jan 2024 23:18:25 +0000 (00:18 +0100)]
Merge branch 'stable-6.6' into stable-6.7
* stable-6.6:
Introduce a PriorityQueue sorting RevCommits by commit timestamp
Remove org.eclipse.jgit.benchmark/.factorypath
Update jmh to 1.37 for org.eclipse.jgit.benchmark
Luca Milanesio [Mon, 13 Jun 2022 22:09:55 +0000 (23:09 +0100)]
Introduce a PriorityQueue sorting RevCommits by commit timestamp
The DateRevQueue uses a tailor-made algorithm to keep
RevCommits sorted by reversed commit timestamp, which has a O(n*n/2)
complexity and caused the explosion of the Git fetch times to
tens of seconds.
The standard Java PriorityQueue provides a O(n*log(n)) complexity
and scales much better with the increase of the number of
RevCommits.
Introduce a new implementation DateRevPriorityQueue of the DateRevQueue
based on PriorityQueue.
Enable usage of the new DateRevPriorityQueue implementation by setting
the system property REVWALK_USE_PRIORITY_QUEUE=true. By default the old
implementation DateRevQueue is used.
Revert commit 170244d05977491271a1cc234583d2e5ba75145d
"Checkout: better directory handling" which is the downport of the
original fix Ie12864c54c9f901a2ccee7caddec73027f353111 which was done
on stable-6.6. Merging this up to stable-6.6 would be a lot of work and
these branches aren't maintained anymore hence revert this change here.
This way the fix is available on stable-5.13 for those who still need
Java 8 and everybody else should upgrade to 6.6.1 or higher.
Fabio Ponciroli [Wed, 6 Dec 2023 13:38:21 +0000 (14:38 +0100)]
Make sure ref to prune is in packed refs
RefDirectory:pack might raise an NPE when deleting loose
refs as final part of the RefDirectory.pack().
This is what the code does:
1) packed ref update: update the list of refs which will be
persisted in packed-refs
2) persit packed-refs: flush on file the refs computed in #1
3) prune loose refs: prune loose refs that have been packed in #2
The code correctly locks the packed-refs file during phases 1 to 3.
However, it makes the wrong assumption of considering
the loose refs set as immutable between phases 1 and 3.
The number and values of loose refs on the filesystem can mutate
at any time whilst the RefDirectory.pack() is in progress.
Assuming the contrary can lead to an NPE when retrieving refs
from the mutable loose refs list during phase #3.
Make sure that the ref is not null before dereferencing its
object-id value.
Matthias Sohn [Mon, 27 Nov 2023 22:34:02 +0000 (23:34 +0100)]
Merge branch 'master' into stable-6.8
* master:
Adapt to type parameter added in commons-compress 1.25.0
Improve footer parsing to allow multiline footers.
Make the tests buildable by bazel test
BitmapIndex: Add interface to track bitmaps found (or not)
BitmapWalker: Remove BitmapWalkListener
Kamil Musin [Fri, 24 Nov 2023 14:17:26 +0000 (15:17 +0100)]
Improve footer parsing to allow multiline footers.
According to the https://git-scm.com/docs/git-interpret-trailers the
CGit supports multiline trailers. Subsequent lines of such multiline
trailers have to start with a whitespace.
We also rewrite the original parsing code to make it easier to work
with. The old code had pointers moving both backwards and forwards at
the same time. In the rewritten code we first find the start of the last
paragraph and then do all the parsing.
Since all the getters of the FooterLine return String, I've considered
rewriting the parsing code to operate on strings. However the original
code seems to be written with the idea, that the data is only lazily
copied in getters and no extra allocations should be performed during
original parsing (ex. during RevWalk). The changed code keeps to this
idea.
Bug: Google b/312440626
Change-Id: Ie1e3b17a4a5ab767b771c95f00c283ea6c300220
Ivan Frade [Mon, 20 Nov 2023 20:01:12 +0000 (12:01 -0800)]
BitmapIndex: Add interface to track bitmaps found (or not)
We want to know what objects had bitmaps in the walk of the
request. We can check their position in the history and evaluate our
bitmap selection algorithm.
Introduce a listener interface to the BitmapIndex to report which
getBitmap() calls returned a bitmap (or not) and a method to the
bitmap index to set the listener.
florian.signoret [Mon, 16 Oct 2023 14:08:14 +0000 (16:08 +0200)]
Fix branch ref exist check
When a tag with the same name as the branch exists, the branch creation
process should work too. We should detect that the branch already
exists, and allow to force create it when the force option is used.
Ivan Frade [Fri, 17 Nov 2023 18:11:17 +0000 (10:11 -0800)]
Adjust javadoc to pass errorprone checks
bazel build //org.eclipse.jgit.test:all doesn't build due to
errorprone errors. This leds to disabling the checks and find the
errors later in the jenkins builder.
Fix javadoc errors reported by errorprone. This doesn't fix completely
the build but it is a step towards it.
Ivan Frade [Thu, 16 Nov 2023 23:12:47 +0000 (15:12 -0800)]
BitmapWalkListener: Use plain interface with noop instance
In this new interface default methods are useful only to instantiate
noop instances. We rather reuse the same noop instance and save the
"default" to add backward compatible methods to existing interfaces.
Make the methods regular interface methods and provide a noop
instance.
Ivan Frade [Thu, 16 Nov 2023 18:46:07 +0000 (10:46 -0800)]
BitmapWalkListener: Add method and rename for commits
During the walk, the commit can be either
1. already in the walk bitmap
2. unvisited so far with bitmap in the bitmap index
3. unvisited so far without bitmap in the bitmap index
Expose these three states in the interface. This makes the interface
easier to explain: it reports the commits found during the walk.
As it is all about commits, rename the methods to onCommit***.
PatchApplier: wrap output's TemporaryBuffer with a CountingOutputStream
The documentation for TemporaryBuffer::length says:
"The length is only accurate after {@link #close()} has been invoked".
However, we need to have the stream open while accessing the length.
This prevents patches on large files to be applied correctly, as the
result get trimmed.
Bug: Google b/309500446
Change-Id: Ic1540f6d0044088f3b46f1fad5f6a28ec254b711
Matthias Sohn [Wed, 15 Nov 2023 17:05:54 +0000 (18:05 +0100)]
Merge branch 'master' into stable-6.8
* master: (42 commits)
Fix typo in constant name CONFIG_KEY_STREAM_FILE_TRESHOLD
Simplify StringUtils#commonPrefix
Optimize RefDirectory.getRefsByPrefix(String...)
Use try-with-resource to ensure UploadPack is closed
Fix hiding field warning
Fix warning for empty code blocks
Fix boxing warnings
errorprone: remove unnecessary parentheses
Update mockito to 5.7.0 and bytebuddy to 1.14.9
Enable Maven reproducible builds
Upgrade bazlets to the latest revision
Revert "Optimise Git protocol v2 `ref-prefix` scanning"
Document GIT_TRACE_PERFORMANCE to show timings
config-options.md: fix sort order
ComboBitset: Add Javadoc
CommitGraphWriter: Add progress monitor to bloom filter computation
CommitGraphWriter: Use ProgressMonitor from the OutputStream
CommitGraphWriter: Unnest generation-number progress
Optimise Git protocol v2 `ref-prefix` scanning
UploadPackTest: Cover using wanted-refs as advertised set
...
Dariusz Luksza [Thu, 9 Nov 2023 11:18:38 +0000 (11:18 +0000)]
Optimize RefDirectory.getRefsByPrefix(String...)
Currently for file-based repositories JGit will go over all refs in the
repository forach `ref-prefix` listed in the `ls-refs` command in git
protocol v2 request.
Native git, uses a different approach, where all refs are read once and
then for each ref, all `ref-prefix` filter values are checked in one
pass.
This change implements this approach in JGit only in the `RefDirectory`
backend. And makes `ref-prefix` filtering ~40% faster for repositories
with packed refs.
Different implementations were tested on a synthetic file repository
with 10k refs in `refs/heads/` and `290k` in `refs/changes`. Before
testing `git pack-refs` command was executed. All results are in
seconds.
The elapsed time was measured around `getRefByPrefix` call in
`Uploadapack.getFilteredRefs(Collection<String>)` (around lines 952 and
954). For testing a modified version of
`UploadPackTest.testV2LsRefsRefPrefix()` was used. The modifications
here included:
* shadowing protected `repo` variable with `FileRepository` pointing
to the synthetic repo with 300k refs described above,
* mimicking the git client clone request by adding `ref-prefix HEAD`,
`ref-prefix refs/heads/` and `ref-prefix refs/tags/`
Based on the above results, the implementation with parallel stream and
stream was selected.
Change [1] reduced the scope of the "writing commit graph" monitoring
task. This left some monitor#update() calls out of any task. When out
of a task, the #update call is a noop.
Delete this update calls as they are noops and misleading.
The affected chunks are usually small and quick to write, so probably
they don't need progress monitoring.
* changes:
Use try-with-resource to ensure UploadPack is closed
Fix hiding field warning
Fix warning for empty code blocks
Fix boxing warnings
errorprone: remove unnecessary parentheses
Update mockito to 5.7.0 and bytebuddy to 1.14.9
Enable Maven reproducible builds
Upgrade bazlets to the latest revision
Matthias Sohn [Thu, 5 Oct 2023 23:10:40 +0000 (01:10 +0200)]
Enable Maven reproducible builds
- configure Maven to run build reproducibly [1]
- use UTC timestamp of checked out commit as build timestamp
- add git-describe, git-commit-id, git-commit-id, git-tags,
git-remote-origin-url to MANIFEST.MF files
- configure cyclonedx-maven-plugin to also use UTC timestamp of
checked out commit
- for packaging build use tycho-buildtimestamp-jgit [2] to ensure
version uses the timestamp of the last commit
- SBOMs are not reproducible by design [3] they should have a build
timestamp matching the time when the build was executed and a serial
number which is a unique UUID per build run. Hence exclude them from
comparison [4].
- Use gmavenplus-plugin to format build timestamps. Maven expects
build timestamp in ISO-8601 format, to replace the qualifier in
versions the timestamp format must be compatible with rules for OSGi
version numbers. Didn't find a way to read the properties set by the
git-commit-id-maven-plugin from another plugin. Hence use JGit in a
groovy script to get the commit time of the current HEAD and provide
it in these two formats.
TODO: packaging build (features and p2 repository) is not yet binary
reproducible since that's not yet supported by Tycho [5], artefacts have
reproducible version numbers but file lastModified timestamps are not
yet reproducible.
Test plan for Maven build:
- build using
mvn clean install"
- verify second build is reproducible:
mvn -T1 clean verify artifact:compare
verification seems not to be thread-safe, hence run it with a single
thread using option -T1
For packaging build (still fails due to non-reproducible file
timestamps):
- build using
mvn -f org.eclipse.jgit.packaging/pom.xml clean install
- verify second build is reproducible:
mvn -T1 -f org.eclipse.jgit.packaging/pom.xml clean verify artifact:compare
Ivan Frade [Tue, 7 Nov 2023 22:00:05 +0000 (14:00 -0800)]
CommitGraphWriter: Use ProgressMonitor from the OutputStream
The same progress monitor is passed around as parameter and inside the
output stream. The functions use one to start tasks and another to
report progress, which is confusing. The stream needs the monitor to
check cancellations so we cannot remove it from there.
The ProgressMonitor task to track the calculation of generation
numbers is nested inside the task that follows the writing of all
lines in the commit-graph. ProgressMonitor doesn't support nested
tasks and this confuses the counting.
Move the start/end of the "writing commit graph" task to the
writeCommitData section, after calculating the generation
numbers. Make that task track by commits instead of by lines.
Moving the start/end of the progress task to the chunk-writing
functions is clearer and easier to extend.
Logging of GC before:
Writing out commit-graph in 3 passes: 51% ( 9807/19358)
Computing commit-graph generation numbers: 100% (9551/9551)
Logging of GC after:
Computing commit-graph generation numbers: 100% (9551/9551)
Writing out commit-graph: 100% (9551/9551)
Dariusz Luksza [Fri, 3 Nov 2023 11:52:03 +0000 (11:52 +0000)]
Optimise Git protocol v2 `ref-prefix` scanning
Currenty JGit will go over all refs in the repository for each
`ref-prefix`. This means that refs will be read multiple
times, which leads to subpar performance.
Native git, uses a different approach, where all refs are read once
and then for each ref, all `ref-prefix` filter values are checked in
one pass.
This change implements this approach in JGit. And makes `ref-prefix`
filtering ~28% faster for a repository with fully packed refs
and ~5% when RefTable is used instead of refdir.
Different implementations were tested on a synthetic file repository
with 300k refs. Different implementations were tested for unpacked and
fully packed refs (results are in seconds).
Ivan Frade [Tue, 24 Oct 2023 16:37:13 +0000 (09:37 -0700)]
UploadPackTest: Cover using wanted-refs as advertised set
Parent change introduced using "wanted-refs" as advertised set during
fetchV2, but it tested only a request without wantIds (only
wanted-refs).
Add a second where the request has wanted-refs AND wantId (which
disables the optimization). Change the test to measure the amount of
refs considered advertised, instead of relying in calls to the
refdb.
Patrick Hiesel [Fri, 13 May 2022 11:49:02 +0000 (13:49 +0200)]
UploadPack: use want-refs as advertised set in fetch v2
Protocol v2 introduced refs-in-wants and ls-remote with
prefixes. UploadPack already uses prefixes provided by the client
during a v2 ref advertisement (ls-refs). However, when the client
consequently sends another request to fetch a previously advertised
ref (with want-ref lines), the server uses the whole set of advertised
refs to compute reachability.
In repos with many refs, this slows down the reachability checks
setting up and walking through unnecessary refs. For gerrit it can
also break valid requests because in gerrit "all" means "recent" and
the wanted-ref could fall out of the "recent" range when reloading all
refs at fetch time.
Treat wanted-refs like a ref-prefix when calculating the advertised
refs on v2 fetch command. Less refs means a faster setup and less walk
for the reachability checks. Note that wanted-refs filters only over
the refs visible to the user, so this doesn't give any extra
visibility to the caller.
If the request contains also "want <oid>" lines, we cannot use this
optimization. Those objects could be reachable from any visible
branch, not necessarily in the wanted-refs.
projects / jgit.git / log