Use Map interface instead of ConcurrentHashMap class
On Android, the co-variant override of ConcurrentHashMap.keySet()
introduced in Java 8 was undone. [1] If compiled Java code calls that
co-variant override directly, one gets a NoSuchMethodError exception
at run-time on Android.
Making the code call that method via Map.keySet() side-steps this
problem.
This is similar to bug 496262, where the same problem cropped up when
compiling with Java 8 against a Java 7 target, but here we cannot use
bootclasspath. We build against Java 8, not against the Android version
of it.
Recent Android versions should have some bytecode "magic" that adds the
co-variant override in bytecode (see the commit referenced in [1]), but
on older Android version this problem may still occur. (Or perhaps the
"magic" is ineffective...) There are two pull requests on Github for
this problem, both from 2020, [2][3] while the Android commit [1] is
from March 2018. Apparently people still occasionally run into this
problem in the wild.
[1] 0e8b937ded/ojluni/src/main/java/java/util/concurrent/ConcurrentHashMap.java (1244)
[2] https://github.com/eclipse/jgit/pull/104
[3] https://github.com/eclipse/jgit/pull/100
Change-Id: I7c07e0cc59871cb7fe60795e22867827fa9c2458
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
If a file exists in head, merge, and the working tree, but not in
the index, and we're doing a force checkout, the checkout must be
an "update", not a "keep".
This is a follow-up on If3a9b9e60064459d187c7db04eb4471a72c6cece.
Bug: 569962
Change-Id: I59a7ac41898ddc1dd90e86b09b621a41fdf45667
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
[spotbugs] Fix incorrect lazy initialization in SystemReader
This fixes two warnings of type LI_LAZY_INIT_STATIC.
Change-Id: I26a7a48aed9d0a0547e908a56b7014a7620fadd8
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Don't use class from java.util.concurrent for locking
Use a dedicated Lock object to lock the scheduler in
RepositoryCache#configureEviction to fix spotbugs warning
JLM_JSR166_UTILCONCURRENT_MONITORENTER.
Change-Id: I003dcf0ed1a0a3f4eea5d8a2f51a07473d28a928
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Fix potential NPE in FS.FileStoreAttributes#get
Path#getParent can return null, return fallback filestore attributes in
that case.
Change-Id: Ic09484d527bc87b27964b625e07373b82412f2da
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Fix FileReftableStack#equals to check for null
This fixes spotbugs warning NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT.
This implementation violated the contract defined by
java.lang.Object.equals() because it did not check for null being passed
as the argument. All equals() methods should return false if passed a
null value.
Change-Id: I607f6979613d390aae2f3546b587f63133d6d73c
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] FileReftableDatabase: extract lock to local variable
This fixes UL_UNRELEASED_LOCK_EXCEPTION_PATH raised by spotbugs in
#compactFully.
Change-Id: I370578ad9a027c5c9709d60a1dfafdac0cfca908
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] DfsReftableDatabase: extract lock to local variable
This fixes UL_UNRELEASED_LOCK_EXCEPTION_PATH raised by spotbugs in
#DfsReftableDatabase and #clearCache.
Change-Id: Ifd3189288d2a8e64139c02cd105eb335fa2f68cf
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Silence NP_BOOLEAN_RETURN_NULL in IgnoreNode#checkIgnored
Also mark the return value @Nullable to enable null analysis in Eclipse.
Change-Id: I5b286d657d432f4b32afd4dd370f76892b115422
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Fix potential NPE in WorkingTreeIterator#isModified
File#list can return null. Fix the potential NPE by using Files#list
which is also faster since it retrieves directory entries lazily while
File#list retrieves them eagerly.
Change-Id: Idf4bda398861c647587e357326b8bc8b587a2584
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
[spotbugs] Fix potential NPE in FileRepository#convertToReftable
File#listFiles can return null. Use Files#list which does not return
null and should be faster since it's returning directory entries lazily
while File#listFiles fetches them eagerly.
Change-Id: I3bfe2a52278244fc469143692c06b05d9af0d0d4
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If TransportException is thrown in the finally block of execute()
ensure that the exception handled in the previous catch block isn't
suppressed.
Change-Id: I670acdfb4d36e7a419a9a79ae9faab2e085a43ee
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If TransportException is thrown in the finally block of
downloadPackedObject() ensure that the exception handled in the previous
catch block isn't suppressed.
Change-Id: I23982a2b215e38f681cc1719788985e60232699a
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
This was experimental code and never used in production.
Change-Id: Ia3da7f2b82d9e365cec2ccf9397cbc47439cd150
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Split out loose object handling from ObjectDirectory
The ObjectDirectory class manages the interactions for the entire object
database, this includes loose objects, packfiles, alternates, and
shallow commits. To help reduce the complexity of this class, abstract
some of the loose object specific details into a class which understands
just this, leaving the ObjectDirectory to focus more on the interactions
between the different mechanisms.
Change-Id: I39f3a74d6308f042a2a2baa57769f4acde5ba5e0
Signed-off-by: Martin Fick <mfick@codeaurora.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
The ObjectDirectory class manages the interactions for the entire object
database, this includes loose objects, packfiles, alternates, and
shallow commits. To help reduce the complexity of this class, abstract
some of the packfile specific details into a class which understands
just this, leaving the ObjectDirectory to focus more on the interactions
between the different mechanisms.
Change-Id: I5cc87b964434b0afa860b3fe23867a77b3c3a4f2
Signed-off-by: Martin Fick <mfick@codeaurora.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Some clients may wish to allow NO_CHANGE lightweight tag updates
without setting the force flag. (For instance EGit does so.)
Command-line git does not allow this.
Propagate the RefUpdate result via the RefAlreadyExistsException.
That way a client has the possibility to catch it and check the
failure reason without having to parse the exception message, and
take appropriate action, like ignoring the exception on NO_CHANGE.
Change-Id: I60e7a15a3c309db4106cab87847a19b6d24866f6
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
TagCommand: make -f work with lightweight tags for NO_CHANGE
JGit treated a NO_CHANGE RefUpdate as an error in all cases. But when
updating a lightweight tag, this is a successful result if -f was
specified.
Change-Id: Iddfa6d6a6dc8bf8fed81138a008ebc32d5f960bd
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Add the two config constants from C git that can switch on signing
of annotated tags. Add them to the GpgConfig, and implement actually
signing a tag in TagCommand.
The interactions between command line options for "git tag" and config
options is a bit murky in C git. There are two config settings for it:
* tag.gpgSign is the main option, if set to true, it kicks in if
neither -s nor -u are given on the command line.
* tag.forceSignAnnotated signs only tags created via "git tag -m",
but only if command-line option "-a" is not present. It applies
even if tag.gpgSign is set explicitly to false.
Giving -s or -u on the command line also forces an annotated tag
since lightweight tags cannot be signed.
Bug: 386908
Change-Id: Ic8a1a44b5f12f47d5cdf3aae2456c1f6ca9ef057
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Factor out a common ObjectBuilder as super class of CommitBuilder
and TagBuilder, and make the GpgSigner work on ObjectBuilder.
In order not to break API, add the new method for signing an
ObjectBuilder in a new interface GpgObjectSigner.
The signature for a tag is just tacked onto the end of the tag
message. The message of a signed tag must end in LF.
Bug: 386908
Change-Id: I5e021e3c927f4051825cd7355b129113b949455e
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Added check for null on DirCacheEntry in checkoutEntry method
Observed the error when trying to force checkout from a branch
that had no changes on it. When the 'keep()' method from 'DirCacheCheckout'
method was called the 'DirCacheEntry e' was null and was passed like
this to the 'checkoutEntry()' method where the 'getObjectId()' is
being called on the 'e' object
Change-Id: If3a9b9e60064459d187c7db04eb4471a72c6cece
Fix PackInvalidException when fetch and repack run concurrently
We are running several servers with jGit. We need to run repack from
time to time to keep the repos performant. I.e. after push we test how
many small packs are in the repo and when a threshold is reached we run
the repack.
After upgrading jGit version we've found that if someone does the clone
at the time repack is running the clone sometimes (not always) fails
because the repack removes .pack file used by the clone. Server
exception and client error attached.
I've tracked down the cause and it seems to be introduced between jGit
5.2 (which we upgraded from) and 5.3 and being caused by this commit:
Move throw of PackInvalidException outside the catch -
afef866a44
The problem is that when the throw was inside of the try block the last
catch block catched the exception and called openFailed(false) method.
It is true that it called it with invalidate = false, which is wrong.
The real problem though is that with the throw outside of the try block
the openFail is not called at all and the fields activeWindows and
activeCopyRawData are not set to 0. Which affects the later called tests
like: if (++activeCopyRawData == 1 && activeWindows == 0).
The fix for this is relatively simple keeping the throw outside of the
try block and still having the invalid field set to true. I did
exhaustive testing of the change running concurrent clones and pushes
indefinitely and with the patch applied it never fails while without the
patch it takes relatively short to get the error.
See: https://www.eclipse.org/lists/jgit-dev/msg04014.html
Bug: 569349
Change-Id: I9dbf8801c8d3131955ad7124f42b62095d96da54
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add constants for parsing git wire protocol version
This would allow other JGit users to access and reuse the constants.
Change-Id: I1608802f45586af5f8582afa592e26679e9cebe3
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add support for reading symrefs from pack capabilities
A SymbolicRef is added to the advertised refs for any symref in
capabilities whose target is an advertised ref; this may replace an
existing entry, such as HEAD.
When cloning, if any advertised HEAD is symbolic then use the target
rather than looking for an advertised ref with a matching objectId.
Add --symref option to LsRemote command.
Bug: 514052
Change-Id: Idfb48e6f6e8dcfe57a6896883fe6d84d533aa9d0
Signed-off-by: Lee Worrall <worrall.la@gmail.com>
GC#deleteOrphans: handle failure to list files in pack directory
- log an error
- either there is no list or it is incomplete hence return immediately
Change-Id: Ieee5378ca06304056b9ccc30c1acd5f52360052d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If pack or index files are guarded by a pack lock (.keep file)
deleteOrphans() should not touch the respective files protected by the
lock file. Otherwise it may interfere with PackInserter concurrently
inserting a new pack file and its index.
The problem was caused by the following race.
All mentioned files are located in "objects/pack/".
File endings relevant in "pack" dir:
.pack
.keep
.idx
.bitmap
When ReceivePack receives a pack file it executes the following steps:
ReceivePack.service():
receivePackAndCheckConnectivity():
receivePack():
receive the pack
parse the pack, returns packLock (.keep file)
PackInserter.flush():
write tmpPck file: "insert_<random>.pack"
write tmpIdx file: "insert_<random>.idx"
real pack name: "pack-<SHA1>.pack"
real index name: "pack-<SHA1>.idx"
atomic rename tmpPack to realPack
atomic rename tmpIdx to tmpIdx
execute commands
unlock pack by removing .keep file
trigger auto gc if enabled
When PackInserter.flush() renames the temporary pack to the final
"pack-xxx.pack" file the temporary pack index file "insert_xxx.idx"
has no matching .pack file with the same base name for a short interval.
If deleteOrphans() ran during that interval it deduced the pack index
file was orphaned. Subsequently the missing pack index caused
MissingObjectExceptions since objects contained in the pack couldn't be
looked up anymore.
Bug: https://bugs.chromium.org/p/gerrit/issues/detail?id=13544
Change-Id: I559c81e4b1d7c487f92a751bd78b987d32c98719
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Just allocate the two string objects directly. The previously used
new StringBuilder(0).toString() returns the same object for both END
and DELIM when run on Java 15, which breaks the wire protocol since
then END and DELIM cannot be distinguished anymore.
Bug: 568950
Change-Id: I9d54d7bf484948c24b51a094256bd9d38b085f35
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
(cherry picked from commit 7da0f0a8f3)
Just allocate the two string objects directly. The previously used
new StringBuilder(0).toString() returns the same object for both END
and DELIM when run on Java 15, which breaks the wire protocol since
then END and DELIM cannot be distinguished anymore.
Bug: 568950
Change-Id: I9d54d7bf484948c24b51a094256bd9d38b085f35
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
(cherry picked from commit 7da0f0a8f3)
Just allocate the two string objects directly. The previously used
new StringBuilder(0).toString() returns the same object for both END
and DELIM when run on Java 15, which breaks the wire protocol since
then END and DELIM cannot be distinguished anymore.
Bug: 568950
Change-Id: I9d54d7bf484948c24b51a094256bd9d38b085f35
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Allow to resolve a conflict by checking out a file
DirCacheEditor unconditionally applied a PathEdit to all stages in the
index. This gives wrong results if one wants to check out a file from
some commit to resolve a conflict: JGit would update the working tree
file multiple times (once per stage), and set all stages to point to
the checked-out blob.
C git replaces the stages by the entry for the checked-out file.
To support this, add a DirCacheEntry.setStage() method so that
CheckoutCommand can force the stage to zero. In DirCacheEditor, keep
only the zero stage if the PathEdit re-set the stage.
Bug: 568038
Change-Id: Ic7c635bb5aaa06ffaaeed50bc5e45702c56fc6d1
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Document that setLastModified sets time of symlink target
Due to Java bug JDK-8220793 [1] Java cannot set timestamps of a symlink
but only of the symlink target. This bug was fixed in Java 13. Since we
don't have a use case to set the timestamp of the symlink itself simply
document the current behavior of setLastModified methods.
[1] https://bugs.openjdk.java.net/browse/JDK-8220793
Change-Id: Ibc28c1702a1b9845602bd257606fbd44803a43fb
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
PerformanceLogContext threw NullPointerException when multiple threads
tried to add an event to the PerformanceLogContext. The cause for this
is that the ThreadLocal was initialized only in the class constructor
for the first thread; for subsequent threads it was null.
To fix this initialize eventRecords for each thread.
Change-Id: I18ef67dff8f0488e3ad28c9bbc18ce73d5168cf9
Signed-off-by: Alexa Panfil <alexapizza@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Fix IOException occurring when calling
GC on a repository with absent objects/pack folder.
Change-Id: I5be1333a0726f4d7491afd25ddac85451686c30a
Signed-off-by: Nail Samatov <sanail@yandex.ru>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Revert "Client-side protocol V2 support for fetching"
This reverts commit f802f06e7f.
I had misunderstood how protocol V2 works. This implementation only
works if the negotiation during fetch is done in one round.
Fixing this is substantial work in BasePackFetchConnection. Basically
I think I'd have to change back negotiate to the V0 version, and have
a doFetch() that does
if protocol V2
doFetchV2()
else
doFetchV0()
with doFetchV0 the old code, and doFetchV2 completely new.
Plus there would need to be a HTTP test case requiring several
negotiation rounds.
This is a couple of days work at least, and I don't know when I will
have the time to revisit this. So although the rest of the code is
fine I prefer to back this out completely and not leave a only half
working implementation in the code for an indeterminate time.
Bug: 553083
Change-Id: Icbbbb09882b3b83f9897deac4a06d5f8dc99d84e
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Make all transports request protocol V2 when fetching. Depending on
the transport, set the GIT_PROTOCOL environment variable (file and
ssh), pass the Git-Protocol header (http), or set the hidden
"\0version=2\0" (git anon). We'll fall back to V0 if the server
doesn't reply with a version 2 answer.
A user can control which protocol the client requests via the git
config protocol.version; if not set, JGit requests protocol V2 for
fetching. Pushing always uses protocol V0 still.
In the API, there is only a new Transport.openFetch() version that
takes a collection of RefSpecs plus additional patterns to construct
the Ref prefixes for the "ls-refs" command in protocol V2. If none
are given, the server will still advertise all refs, even in protocol
V2.
BasePackConnection.readAdvertisedRefs() handles falling back to
protocol V0. It newly returns true if V0 was used and the advertised
refs were read, and false if V2 is used and an explicit "ls-refs" is
needed. (This can't be done transparently inside readAdvertisedRefs()
because a "stateless RPC" transport like TransportHttp may need to
open a new connection for writing.)
BasePackFetchConnection implements the changes needed for the protocol
V2 "fetch" command (simplified ACK handling, delimiters, section
headers).
In TransportHttp, change readSmartHeaders() to also recognize the
"version 2" packet line as a valid smart server indication.
Adapt tests, and run all the HTTP tests not only with both HTTP
connection factories (JDK and Apache HttpClient) but also with both
protocol V0 and V2. Do the same for the SSH transport tests.
Bug: 553083
Change-Id: Ice9866aa78020f5ca8f397cde84dc224bf5d41b4
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Ensure .gitmodules is loaded when accessing submodule name
This problem occurred when calling SubmoduleWalk#getModuleName if the
first submodule processed has a name and a path which do not match
SubmoduleWalk#getModuleName returned the module path instead of the
module name. In order to fix this SubmoduleWalk#getModuleName needs to
ensure that the modules config is loaded.
Bug: 565776
Change-Id: I36ce1fbc64c4849f9d8e39864b825c6e28d344f8
Signed-off-by: John Dallaway <john@dallaway.org.uk>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add new performance logging to register events of type duration.
The proposed logging is similar to the performance logging
in OS Gerrit https://gerrit-review.googlesource.com/c/gerrit/+/225628:
a global Singleton (LoggingContext in Gerrit) is
collecting the performance logs in a thread-safe events list,
and at the end of the monitored command the list of events is
retrieved and written to a log, after which it is cleared.
What this patch does:
The main component is the Singleton (PerformanceLogContext), which
is used to collect the records (PerformanceLogRecord) in one place
(ThreadLocal eventsList) from anywhere using
PerformanceLogContext.getInstance().addEvent().
Reason why this change is needed:
The current monitoring in JGit has several issues:
1. git fetch and git push events are handled separately
(PackStatistics and ReceivedPackStatistics), with no unified way
of writing or reading the statistics.
2. PostUploadHook is only invoked on the event of sending the
pack, which means that the PackStatistics is not available for
the fetch requests that did not end with sending the pack
(negotiation requests).
3. The way the logs are created is different from the performance
log approach, so the long-running operations need to be collected
from both performance log (for JGit DFS overridden operations and
Gerrit operations) and gitlog (for JGit ones).
The proposed performance logging is going to solve the above
mentioned issues: it collects all of the performance logs in one
place, thus accounting for the commands that do not result in
sending a pack. The logs are compatible with the ones on Gerrit.
Moreover, the Singleton is accessible anywhere in the call stack,
which proved to be successful in other projects like Dapper
(https://research.google/pubs/pub36356/).
Signed-off-by: Alexa Panfil <alexapizza@google.com>
Change-Id: Iabfe667a3412d8a9db94aabb0f39b57f43469c41
This enables jgit to use any refs in the refs/ namespace when describing
commits.
Signed-off-by: Jason Yeo <jasonyeo88@gmail.com>
Change-Id: I1fa22d1c39c0e2f5e4c2938c9751d8556494ac26
Reason why this change is needed:
Currently the durations of fetch events are computed by
registering time instants with System.currentTimeMillis()
and calculating the differences with simple minus operation,
but multiple sources suggest that the best practice is to use
the Java 8 Duration and Instant objects.
What this patch does:
Get time measurements with Instant.now() instead of
System.currentTimeMillis() and calculate the duration of fetch
events (Reachability checks and Negotiation) using
Duration.between().toMillis().
Signed-off-by: Alexa Panfil <alexapizza@google.com>
Change-Id: I573a0a0562070083cf5a5a196d9710f69a7fa587
Fix OperatorPrecedence warning flagged by error prone
Building with Bazel is failing with this error message:
HttpSupport.java:480: error: [OperatorPrecedence] Use grouping
parenthesis to make the operator precedence explicit
if (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z') {
^
(see https://errorprone.info/bugpattern/OperatorPrecedence)
Did you mean 'if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) {'?
Change-Id: I96089d3158d06ba981cfdf2b03865261b328de23
Support "http.userAgent" and "http.extraHeader" from the git config
Validate the extra headers and log but otherwise ignore invalid
headers. An empty http.extraHeader starts the list afresh.
The http.userAgent is restricted to printable 7-bit ASCII, other
characters are replaced by '.'.
Moves a support method from the ssh.apache bundle to HttpSupport in
the main JGit bundle.
Bug:541500
Change-Id: Id2d8df12914e2cdbd936ff00dc824d8f871bd580
Signed-off-by: James Wynn <james@jameswynn.com>
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
This enables EGit to override with a lenient variant that logs the
problem and continues with the default value. EGit needs this because
otherwise a corrupt core.excludesFile entry can render the whole UI
unusable (until the git config is fixed) because any use of the
WorkingTreeIterator will throw an InvalidPathException.
This is not a problem on OS X, where all characters are allowed in
file names. But on Windows some characters are forbidden... see bug
567296. The message of the InvalidPathException is not helpful since
it doesn't point to the origin of the problem. EGit can log a much
better message indicating the offending config file and entry in the
Eclipse error log, where the user can see it.
Bug: 567309
Change-Id: I4e57afa715ff3aaa52cd04b5733f69e53af5b1e0
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Thomas Wolf
Matthias Sohn
Han-Wen Nienhuys
Martin Fick
Tudor Matrescu
Petr Hrebejk
David Ostrovsky
Lee Worrall
Alexa Panfil
Nail Samatov
John Dallaway
Jason Yeo
James Wynn