Matthias Sohn [Tue, 24 Sep 2024 08:51:22 +0000 (10:51 +0200)]
AdvertisedRequestValidator: fix WantNotValidException caused by race
Fetch with protocol V2 failed under the following conditions
- fetch uses bidirectional protocol (git, ssh) which uses a shortcut
to determine invalid wants
- not all wants are advertised
- race condition: wanted ref is updated during fetch by another thread
after the thread serving upload-pack determined wants and before it
checks not advertised wants
Fix this by calling
`new ReachableCommitRequestValidator().checkWants(up, wants)`
instead of throwing WantNotValidException in [1]
if this race happened in the same way like it's done for unidirectional
protocols (http) [2].
Matthias Sohn [Tue, 20 Aug 2024 12:56:04 +0000 (14:56 +0200)]
Merge branch 'stable-6.7' into stable-6.8
* stable-6.7:
Update tycho to 4.0.8
Update org.eclipse.dash:license-tool-plugin to 1.1.0
Fix "Comparison of narrow type with wide type in loop condition"
JGit v5.13.3.202401111512-r
Matthias Sohn [Tue, 20 Aug 2024 12:54:08 +0000 (14:54 +0200)]
Merge branch 'stable-6.6' into stable-6.7
* stable-6.6:
Update tycho to 4.0.8
Update org.eclipse.dash:license-tool-plugin to 1.1.0
Fix "Comparison of narrow type with wide type in loop condition"
JGit v5.13.3.202401111512-r
Matthias Sohn [Tue, 20 Aug 2024 12:28:33 +0000 (14:28 +0200)]
Merge branch 'stable-6.5' into stable-6.6
* stable-6.5:
Update org.eclipse.dash:license-tool-plugin to 1.1.0
Fix "Comparison of narrow type with wide type in loop condition"
JGit v5.13.3.202401111512-r
Matthias Sohn [Sun, 18 Aug 2024 16:35:29 +0000 (18:35 +0200)]
Merge branch 'stable-6.4' into stable-6.5
* stable-6.4:
Update org.eclipse.dash:license-tool-plugin to 1.1.0
Fix "Comparison of narrow type with wide type in loop condition"
JGit v5.13.3.202401111512-r
Matthias Sohn [Fri, 9 Aug 2024 09:53:01 +0000 (11:53 +0200)]
Fix "Comparison of narrow type with wide type in loop condition"
This issue was detected by a GitHub CodeQL security scan run on JGit
source code.
Description of the error raised by the security scan:
"In a loop condition, comparison of a value of a narrow type with a
value of a wide type may always evaluate to true if the wider value is
sufficiently large (or small). This is because the narrower value may
overflow. This can lead to an infinite loop."
Fix this by using type `long` for the local variable `done`.
Luca Milanesio [Wed, 10 Jan 2024 19:38:46 +0000 (19:38 +0000)]
Allow to discover bitmap on disk created after the packfile
When the bitmap file was created *after* a packfile had been
loaded into the memory, JGit was unable to discover them.
That happed because of two problems:
1. The PackDirectory.getPacks() does not implement the usual
while loop that is scanning through the packs directory
as in the other parts of JGit.
2. The scan packs does not look for newly created bitmap files
if the packfile is already loaded in memory.
Implement the normal packfiles scanning whenever the PackDirectory
needs to return a list of packs, and make sure that any reused
Pack object would have its associated bitmap properly refreshed
from disk.
Adapt the assertions in GcConcurrentTest with the rescanned list
of Pack from the objects/packs directory.
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
projects / jgit.git / log