Eclipse warns that DfsReader should be managed by try-with-resource.
As described in 1484d6e (LargePackedWholeObject: Do not reuse released
inflater, 2018-04-26), the DfsReader is owned and closed by the
PackInputStream or explicitly closed in the try block's finally.
Suppress the warning with a brief explanatory comment.
Change-Id: I4187c935742072f3ee7f2d3551a6a98d40fc2702
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
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.
b0ac5f9c89 (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.
Change-Id: Iaf1d6fd368b64f76c520d215fd270a6098a1f236
Eclipse reports these as errors, so remove them.
Change-Id: Ic53d8003f9faef38fe776af5a73794e7bb1dfc49
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
* 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
332bc61124. 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>
Add support for the "no-progress" parameter in the "fetch" command in
the fetch-pack/upload-pack protocol v2.
Change-Id: I6a6d6b1534f44845254b81d0e1f5c4ba2ac3d10b
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Add support for the "thin-pack" parameter in the "fetch" command in
the fetch-pack/upload-pack protocol v2.
Change-Id: I39a37b2b66a16929137d35c718a3acf2afb6b0b5
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
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>
Add support for the "ref-prefix" parameter in the "ls-refs" command in
the fetch-pack/upload-pack protocol v2.
Change-Id: If9cf93b2646f75d50a11b5f482594f014d59a836
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
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)
Change-Id: I33df56f1cb1c6c2c25ee95426cb7ad665134ac6b
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>
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>
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>
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>
Otherwise successful, non-conflicting merges will never get a
Gerrit Change-Id.
Bug: 358206
Change-Id: I9b599ad01d9f7332200c1d81a1ba6ce5ef990ab5
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
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.
[1] Real-world example:
https://gerrit-review.googlesource.com/c/gerrit/+/171871/1/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java#149
Change-Id: I7f1254b4ebf202d4dcfc8e59d7120427542d0d9e
Previously @ was allowed e.g. in branch names, but not as the last
character. The case that @ is the last character was not handled.
Change-Id: Ic33870b22236f7a5ec7b54007f1b0cefd9354bfb
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>
This breaks any scenario where native git (with LFS) clones a repository
(and thus installs the hook) and later on JGit is used to push changes.
Change-Id: I2a17753377265a0b612ba3451b9df63a577a1c38
Signed-off-by: Markus Duft <markus.duft@ssi-schaefer.com>
These methods were introduced for 4.11.1 so we have to silence the API
error adding API in a service release raises.
Change-Id: Ic847cebbed439912d3979ec2ec1809f77a28f61e
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
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>
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>
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>
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>
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>
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.
Change-Id: Ib759ffbbfc490953481853e74263dd46d2592888
Signed-off-by: Minh Thai <mthai@google.com>
Change If72b4b422 added a new method filterAndAddObject with a
partial Javadoc, which causes errors in Eclipse.
Since it's a private method, Javadoc is not strictly necessary, so
just convert it to a standard comment block.
Bug: 532540
Change-Id: I06aa79211d1223dccf6c931451ca885ca6d39cbc
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
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>
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>
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.
[1] https://gerrit-review.googlesource.com/c/gerrit/+/9792
[2] https://gerrit-review.googlesource.com/c/gerrit/+/165851
[3] https://gerrit-review.googlesource.com/c/gerrit/+/165952
Change-Id: Iaf84c3ec32ecf83efffb306fdb4940cc85740f3f
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Refactor getBitmapIndex to open ReadableChannel in try-with-resource
instead of closing the channel in the finally block.
The same cannot be done in copyPackThroughCache, so just suppress the
warning with an explanatory comment.
Change-Id: I9b95373d350728e85a159423d5ca80e8b215914d
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
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>
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
Jonathan Nieder
Matthias Sohn
Jonathan Tan
Michael Keppler
Thomas Wolf
Dave Borowitz
Christian Halstrick
Markus Duft
Minh Thai