Jonathan Nieder [Thu, 26 Apr 2018 22:14:50 +0000 (15:14 -0700)]
LargePackedWholeObject: Do not reuse released inflater
LargePackedWholeObject.openStream produces a stream that allows
reading a large object. This stream holds a DfsReader that takes care
of caching delta bases etc and in particular holds zlib Inflater for
use while reading the each delta in the packfile.
At DfsReader creation time, the Inflater is acquired from a global
InflaterCache to avoid initialization overhead in case there is an
existing Inflater available for reuse. When done with the Inflater,
the DfsReader is responsible for returning it to the cache for reuse.
The DfsReader is AutoClosable to remind the caller to close it and
release the Inflater when finished with it.
b0ac5f9c8907a4034612543a92eb465e88a9c6f2 (LargePackedWholeObject:
Refactor to open DfsReader in try-with-resource, 2018-04-11) tried to
clarify the lifetime of the DfsReader but was too aggressive: when
this function returns, PackInputStream owns the DfsReader and is
already going to release it. Worse, the returned InflaterInputStream
holds a reference to the DfsReader's inflater, making releasing the
DfsReader not only unnecessary but unsafe.
The Inflater gets released into the InflaterCache's pool, to be
acquired by another caller that uses it concurrently with the
InflaterInputStream. This results in errors, such as
java.util.zip.ZipException: incorrect header check
at java.util.zip.InflaterInputStream.read(InflaterInputStream.java:164)
at java.util.zip.InflaterInputStream.skip(InflaterInputStream.java:208)
at java.io.BufferedInputStream.skip(BufferedInputStream.java:377)
and
java.util.zip.DataFormatException: incorrect header check
at java.util.zip.Inflater.inflateBytes(Native Method)
at java.util.zip.Inflater.inflate(Inflater.java:259)
at org.eclipse.jgit.internal.storage.dfs.DfsReader.inflate(DfsReader.java:783)
at org.eclipse.jgit.internal.storage.dfs.DfsPackFile.decompress(DfsPackFile.java:420)
at org.eclipse.jgit.internal.storage.dfs.DfsPackFile.load(DfsPackFile.java:767)
and
Caused by: java.util.zip.ZipException: incorrect header check
at java.util.zip.InflaterInputStream.read(InflaterInputStream.java:164)
at java.io.BufferedInputStream.fill(BufferedInputStream.java:246)
at java.io.BufferedInputStream.read1(BufferedInputStream.java:286)
at java.io.BufferedInputStream.read(BufferedInputStream.java:345)
at org.eclipse.jgit.lib.ObjectStream$Filter.read(ObjectStream.java:219)
at org.eclipse.jgit.util.IO.readFully(IO.java:233)
at org.eclipse.jgit.transport.PackParser.checkObjectCollision(PackParser.java:1173)
Verified in production. It should be possible to make a
straightforward unit test for this using the InflaterCache state but
that can wait for a followup commit.
Matthias Sohn [Wed, 25 Apr 2018 01:09:05 +0000 (03:09 +0200)]
Use eclipse compiler in Maven build
Found instructions for configuring maven-compiler-plugin with ecj in
[1]. Verified that ecj run in this way raises build errors when executed
on commit d3ef5213.
Define profiles "ecj" for using Eclipse compiler and "javac" for using
javac including errorprone. By default ecj will be used.
use ecj:
$ mvn -Pecj clean install
use javac:
$ mvn -Pjavac clean install
TODO: find out how to run ecj with errorprone from Maven.
Michael Keppler [Mon, 23 Apr 2018 20:00:27 +0000 (22:00 +0200)]
File compile and API errors in JGit
* Photon throws null analysis errors on the repeated invocation of those
previously null checked methods. Extract them to a local variable to
avoid this. (the null analysis is configured in project properties)
* setUseProtocolV2() misses @since tag. Problem was introduced with 332bc611249d21f9b604f2c0207bf0bdfbfc3a78. Might be caused by the long
delay of 2 months from creation to merging.
Change-Id: Ibbb1a1580b604b8e7cd4bf7edc4643e292b6b4a8 Signed-off-by: Michael Keppler <Michael.Keppler@gmx.de>
Jonathan Tan [Fri, 23 Feb 2018 22:07:02 +0000 (14:07 -0800)]
Teach UploadPack basic "fetch" command
Add basic support for the "fetch" command in the fetch-pack/upload-pack
protocol v2. This patch teaches "have" and "done".
The protocol specification (Documentation/technical/protocol-v2.txt in
the Git project) states:
want <oid>
Indicates to the server an object which the client wants to
retrieve. Wants can be anything and are not limited to
advertised objects.
It is unspecified whether the server should respect the
uploadpack.allowtipsha1inwant option etc. when serving packfiles. This
patch is conservative in that the server respects them.
Change-Id: I3dbec172239712ef9286a15b8407e86b87ea7863 Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Jonathan Nieder [Fri, 20 Apr 2018 23:34:15 +0000 (16:34 -0700)]
UploadPack: Don't buffer ls-refs output
Once we have read the user's entire command, there is no more need to
buffer our response --- even the strictest servlet engine allows
writing output once the input has been consumed. Noticed when the
analogous code in the "fetch" command (introduced in a later patch)
overflowed its buffer:
java.lang.OutOfMemoryError
at java.io.ByteArrayOutputStream.hugeCapacity(ByteArrayOutputStream.java:123)
[...]
at org.eclipse.jgit.transport.UploadPack.sendPack(UploadPack.java:1905)
at org.eclipse.jgit.transport.UploadPack.sendPack(UploadPack.java:1741)
at org.eclipse.jgit.transport.UploadPack.fetchV2(UploadPack.java:1001)
at org.eclipse.jgit.transport.UploadPack.serviceV2(UploadPack.java:1030)
at org.eclipse.jgit.transport.UploadPack.upload(UploadPack.java:726)
at org.eclipse.jgit.http.server.UploadPackServlet.doPost(UploadPackServlet.java:195)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:637)
Jonathan Tan [Thu, 22 Feb 2018 18:24:19 +0000 (10:24 -0800)]
Implement ls-refs in UploadPack
Implement support for Git protocol v2's "ls-refs" command and its
"symrefs" and "peel" parameters.
This adds support for this command to UploadPack but the git://,
ssh://, and git:// transports do not make use of it yet. That will
have to wait for later patches.
Change-Id: I8abc6bcc6ed4a88c165677ff1245625aca01267b Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Jonathan Nieder <jrn@google.com>
Jonathan Tan [Thu, 22 Feb 2018 18:24:19 +0000 (10:24 -0800)]
Implement protocol v2 with no capabilities in UploadPack
Add initial support for protocol v2 of the fetch-pack/upload-pack
protocol. This protocol is described in the Git project in
"Documentation/technical/protocol-v2.txt".
This patch adds support for protocol v2 (without any capabilities) to
UploadPack. Adaptations of callers to make use of this support will
come in subsequent patches.
[jn: split from a larger patch; tweaked the API to make UploadPack
handle parsing the extra parameters and config instead of requiring
each caller to do such parsing]
Change-Id: I79399fa0dce533fdc8c1dbb6756748818cee45b0 Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Jonathan Nieder <jrn@google.com>
Jonathan Tan [Thu, 22 Feb 2018 18:24:19 +0000 (10:24 -0800)]
PacketLineIn, PacketLineOut: Add support for delim-pkt
Most pkt-lines (data-pkts) have the form
pkt-len pkt-payload
where pkt-len is a string of 4 hexadecimal digits representing the
size in bytes of the pkt-line. Since this size includes the size of
the pkt-len, no data-pkt has a length less than 4.
A pkt-line with a length field less than 4 can thus be used for
other purposes. In Git protocol v1, the only such pkt-line was
flush-pkt = "0000"
which was used to mark the end of a stream. Protocol v2 (see
Documentation/technical/protocol-v2.txt in git.git) introduces a
second special pkt-line type:
delim-pkt = "0001"
used to mark the end of a section within a stream, for example to
separate capabilities from the content of a command.
[jn: split out from a larger patch that made use of this support]
Change-Id: I10e7824fa24ed74c4f45624bd490bba978cf5c34 Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Jonathan Nieder <jrn@google.com>
Jonathan Tan [Fri, 6 Apr 2018 22:50:01 +0000 (15:50 -0700)]
Add RefDatabase#getRefsByPrefix method
The existing RefDatabase#getRefs abstract method (to be implemented by
ref database backends) has the following issues:
- It returns a map with a key (the name of the ref with the prefix
removed) which is potentially superfluous (it can be derived by the
caller if need be) and confusing (in that the prefix is removed).
- The prefix is required to end with a '/', but some backends (e.g.
reftable) have fast search by prefix regardless of what the last
character of the prefix is.
Add a new method #getRefsByPrefix that does not have these issues. This
is non-abstract with a default implementation that uses #getRefs (for
backwards compatibility), but ref database backends can reimplement it.
This also prepares for supporting "ref-prefix" in the "ls-refs" command
in the fetch-pack/upload-pack protocol v2, which does not require that
the prefix end with a '/'.
Change-Id: I4c92f852e8c1558095dd460b5fd7b602c1d82df1 Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Jonathan Nieder <jrn@google.com>
Dave Borowitz [Thu, 12 Apr 2018 15:43:50 +0000 (11:43 -0400)]
Push: Ensure ref updates are processed in input order
Various places on the client side of the push were creating unordered
maps and sets of ref names, resulting in ReceivePack processing commands
in an order other than what the client provided. This is normally not
problematic for clients, who don't typically care about the order in
which ref updates are applied to the storage layer.
However, it does make it difficult to write deterministic tests of
ReceivePack or hooks whose output depends on the order in which commands
are processed, for example if informational per-ref messages are written
to a sideband.[1]
Add a test that ensures the ordering of commands both internally in
ReceivePack and in the output PushResult.
David Pursehouse [Wed, 11 Apr 2018 12:31:07 +0000 (21:31 +0900)]
DirCache: Use constant from StandardCharsets
Instead of hard-coding the encoding name, use the constant from
StandardCharsets. As a result it is no longer necessary to catch
the UnsupportedEncodingException.
Change-Id: I3cb6de921a78e05e2a894c220e0d5a5c85e172cc Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Thomas Wolf [Sun, 18 Mar 2018 22:29:59 +0000 (23:29 +0100)]
Significantly speed up FileTreeIterator on Windows
Getting attributes of files on Windows is an expensive operation.
Windows stores file attributes in the directory, so they are
basically available "for free" when a directory is listed. The
implementation of Java's Files.walkFileTree() takes advantage of
that (at least in the OpenJDK implementation for Windows) and
provides the attributes from the directory to a FileVisitor.
Using Files.walkFileTree() with a maximum depth of 1 is thus a
good approach on Windows to get both the file names and the
attributes in one go.
In my tests, this gives a significant speed-up of FileTreeIterator
over the "normal" way: using File.listFiles() and then reading the
attributes of each file individually. The speed-up is hard to
quantify exactly, but in my tests I've observed consistently 30-40%
for staging 500 files one after another, each individually, and up
to 50% for individual TreeWalks with a FileTreeIterator.
On Unix, this technique is detrimental. Unix stores file attributes
differently, and getting attributes of individual files is not costly.
On Unix, the old way of doing a listFiles() and getting individual
attributes (both native operations) is about three times faster than
using walkFileTree, which is implemented in Java.
Therefore, move the operation to FS/FS_Win32 and call it from
FileTreeIterator, so that we can have different implementations
depending on the file system.
A little performance test program is included as a JUnit test (to be
run manually).
While this does speed up things on Windows, it doesn't solve the basic
problem of bug 532300: the iterator always gets the full directory
listing and the attributes of all files, and the more files there are
the longer that takes.
Bug: 532300
Change-Id: Ic5facb871c725256c2324b0d97b95e6efc33282a Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Matthias Sohn [Sat, 24 Mar 2018 21:05:53 +0000 (22:05 +0100)]
Add missing @since tags for new API
These methods were added after 4.11 so strictly speaking they violate
semantic versioning since new API requires increasing the minor version
number. Hence pretend these methods were introduced in 5.0
Change-Id: I7793ead16577dc1f2ddea09ba6b055103c783555 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn [Sat, 24 Mar 2018 21:14:15 +0000 (22:14 +0100)]
Fix API problem filter warnings
Silence warnings for bundles which haven't broken API since 4.11 but
we increased major version to 5 since we always use the same version
for all jgit bundles
Change-Id: If4f9a6aa4ef21f9b511946c5fc4bd7c0af6094c4 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
David Pursehouse [Tue, 20 Mar 2018 02:43:15 +0000 (11:43 +0900)]
DfsInserter#openStream: Suppress resource warning about DfsReader
DfsReader is not opened in a try-with-resource because in the case where
the method returns an ObjectStream.Filter, the DfsReader should only be
closed from within the Filter's close() method.
Suppress the resource warning.
Change-Id: Ifcaf5e4a326bd1d03c6331b476c645ca43943b34 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Tue, 20 Mar 2018 02:28:18 +0000 (11:28 +0900)]
CloneCommand: Suppress resource warning about Repository
Repository is not opened in try-with-resource because it is wrapped
in a Git instance which should be closed by the caller. On exeptions
during fetch, it is explicitly closed in the catch blocks.
Suppress the warning with an explanatory comment.
Change-Id: Ib32c74ce39bb810077ab84db33002bdde806f3b6 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Mon, 19 Mar 2018 12:10:04 +0000 (21:10 +0900)]
ResolveMergerTest: Use @DataPoints instead of @DataPoint
Define strategiesUnderTest as an array of MergeStrategy using the
@DataPoints annotation, rather than two separate variables each
annotated as @DataPoint.
This makes the implementation consistent with RecursiveMergerTest.
Change-Id: I9f1d525b38cb59634ba054c7779dc4af1fc46e25 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Thu, 15 Mar 2018 04:44:00 +0000 (13:44 +0900)]
FS#runProcess: Fix OutputStream left unclosed after IOException
The runProcess method creates an OutputStream that is not managed by
a try-with-resource because it's manually closed and any IOException
raised by the close() method is explicitly ignored.
Suppress the resource warning with an explanatory comment.
Enclose the call to StreamGobbler#copy in an inner try-block, and move
the call to close() inside its finally block. This prevents the stream
from being left unclosed if StreamGobbler#copy raises IOException.
Change-Id: Idca9adfc4d87e0989d787ad8239c055c0c849814 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Minh Thai [Fri, 23 Mar 2018 00:11:45 +0000 (17:11 -0700)]
scanPacks to return reftables even if no packs
An empty repository may have a dangling symref HEAD pointing to
refs/heads/master. In this case, there will be a reftable even though
there are no packs yet.
David Pursehouse [Wed, 14 Mar 2018 04:11:16 +0000 (13:11 +0900)]
PushCommand: Suppress resource warning for Transport in for loop
A list of Transport instances is provided by Transport.openAll, and
then iterated over in a for loop. Eclipse warns that the Transport
in the for-loop should be managed by try-with-resource.
The Transport is explicitly closed in the finally block, so just
suppress the warning.
Change-Id: I92b73cd90902637cf1ac1ab7b02b5ee5ed6bdb1e Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Jonathan Tan [Tue, 17 Oct 2017 22:48:53 +0000 (15:48 -0700)]
Teach UploadPack to support filtering by blob size
Teach UploadPack to advertise the filter capability and support a
"filter" line in the request, accepting blob sizes only, if the
configuration variable "uploadpack.allowfilter" is true. This feature is
currently in the "master" branch of Git, and as of the time of writing,
this feature is to be released in Git 2.17.
This is incomplete in that the filter-by-sparse-specification feature
also supported by Git is not included in this patch.
If a JGit server were to be patched with this commit, and a repository
on that server configured with RequestPolicy.ANY or
RequestPolicy.REACHABLE_COMMIT_TIP, a Git client built from the "master"
branch would be able to perform a partial clone.
Change-Id: If72b4b422c06ab432137e9e5272d353b14b73259 Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
David Pursehouse [Wed, 14 Mar 2018 23:25:43 +0000 (08:25 +0900)]
ObjectIdSerializer: Support serialization of known non-null ObjectId
The implementation of ObjectIdSerializer, added in change I7599cf8bd,
is not equivalent to the original implementation in Gerrit [1].
The Gerrit implementation provides separate methods to (de)serialize
instances of ObjectId that are known to be non-null. In these methods,
no "marker" is written to the stream. Replacing Gerrit's implementation
with ObjectIdSerializer [2] broke persistent caches because it started
writing markers where they were not expected [3].
Since ObjectIdSerializer is included in JGit 4.11 we can't change the
existing #write and #read methods. Keep those as-is, but extend the
Javadoc to clarify that they support possibly null ObjectId instances.
Add new methods #writeWithoutMarker and #readWithoutMarker to support
the cases where the ObjectId is known to be non-null and the marker
should not be written to the serialization stream.
Also:
- Replace the hard-coded `0` and `1` markers with constants that can
be linked from the Javadocs.
- Include the marker value in the "Invalid flag before ObjectId"
exception message.
David Pursehouse [Wed, 14 Mar 2018 01:56:18 +0000 (10:56 +0900)]
DirCache: Suppress resource warning related to TemporaryBuffer
In #writeTo, the TemporaryBuffer can't be opened in try-with-resource
because it's referenced in the finally block. Instead it is explicitly
closed within the try block. Suppress the warning with an explanatory
comment.
Change-Id: I02009f77f9630d5d55afc34eb86d304ff103b8b0 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Tue, 13 Mar 2018 07:11:40 +0000 (16:11 +0900)]
Add SilentFileInputStream to allow ignoring exceptions raised by close()
There are several cases where a FileInputStream is opened outside of
a try-with-resource because we want to explicitly close it and ignore
any IOException that is raised by the close() method.
Introduce a helper class, SilentFileInputStream, that overrides the
close method and ignores the exceptions. This allows to open the stream
in a try-with-resource block and remove the explicit handling of the
close method.
Change-Id: I8612f948a1a5b3d1031344922ad75ce4492cfc61 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Wed, 14 Mar 2018 00:39:48 +0000 (09:39 +0900)]
UploadPack: Suppress resource warning about PackWriter
PackWriter is auto-closeable and should be opened in try-with-resource,
but this is not possible since the variable is being referenced in the
finally block before being explicitly closed there.
Suppress the warning and add an explanatory comment.
Change-Id: I161923f35142132234fd951c0146d3cb30920b7b Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Tue, 13 Mar 2018 11:31:38 +0000 (20:31 +0900)]
SubmoduleWalk#forIndex: Suppress resource warning and update Javadoc
SubmoduleWalk is auto-closeable, and Eclipse warns that is is not
managed by try-with-resource. However in this case the resource should
not be closed, because the caller needs to use it. Instead, it is the
responsibility of the caller to close it after use.
Update the Javadoc to clarify this, and suppress the warning.
Change-Id: Ib7ba349353bfd3341bdcbe4bb19abaeb9f3aeba5 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Tue, 13 Mar 2018 08:24:10 +0000 (17:24 +0900)]
TemporaryBufferTest: Suppress "should be managed by try-with-resource"
In most of the tests, the temporary buffer is explicitly destroyed in
a finally block after being closed. This is not possible if using the
try-with-resource construct, because the variable is not accessible in
the finally block scope.
Change-Id: I3bab30695ddd12e1a0ae107989638428fe3ef551 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse [Tue, 13 Mar 2018 02:44:23 +0000 (11:44 +0900)]
Open auto-closeable resources in try-with-resource
When an auto-closeable resources is not opened in try-with-resource,
the warning "should be managed by try-with-resource" is emitted by
Eclipse.
Fix the ones that can be silenced simply by moving the declaration of
the variable into a try-with-resource.
In cases where we explicitly call the close() method, for example in
tests where we are testing specific behavior caused by the close(),
suppress the warning.
Leave the ones that will require more significant refcactoring to fix.
They can be done in separate commits that can be reviewed and tested
in isolation.
Change-Id: I9682cd20fb15167d3c7f9027cecdc82bc50b83c4 Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Markus Duft [Thu, 8 Mar 2018 07:29:03 +0000 (08:29 +0100)]
Introduce SshSupport to centralize SSH related utility code
As discussed with Thomas here:
https://git.eclipse.org/r/#/c/83506/31/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/SmudgeFilter.java@349
Move the code from ConfigureGerritAfterCloneTask to SshSupport and
eliminate the slightly modified copy of the code from
LfsConnectionFactory. Separate EGit commit will eliminate the code from
ConfigureGerritAfterCloneTask.
Change-Id: Ifb5adb1342e0fc1f2a70cddf693408d4e0ef7906 Signed-off-by: Markus Duft <markus.duft@ssi-schaefer.com>
Matthias Sohn [Fri, 9 Mar 2018 23:54:51 +0000 (00:54 +0100)]
Remove deprecated StreamCopyThread#flush
StreamCopyThread: Do not let flush interrupt a write.
flush calls interrupt() to interrupt a pending read and trigger a
flush. Unfortunately that interrupt() call can also interrupt a
pending write, putting Jsch in a bad state and triggering "Short read
of block" errors.
Change-Id: I11f8a014fd72df06617cc8731d992eb14cc32a67 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn [Fri, 9 Mar 2018 23:52:22 +0000 (00:52 +0100)]
Remove deprecated SafeBufferedOutputStream
Use Java 8 BufferedOutputStream instead. Java 8 fixed the silent flush
during close issue by FilterOutputStream (base class of
BufferedOutputStream) using try-with-resources to close the stream,
getting a behavior matching what JGit's SafeBufferedOutputStream
was doing
Change-Id: Ieeab59f49b44519585abda213d287b19c7863b17 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Use the more-clearly-named
FileUtils#relativizeNativePath(String, String)
instead, or directly call
FileUtils#relativizePath(String, String, String, boolean).
Change-Id: I9b56302c94630c75293d8cf5510e1d2f22f2b778 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
projects / jgit.git / log