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>
The NPE occurred in conjunction with a symbolic ref (origin/HEAD).
Change-Id: I291636818a121ca00e0df25de5b6fc71a48d447f
Signed-off-by: Markus Duft <markus.duft@ssi-schaefer.com>
* stable-4.11:
Remove package import for javax.servlet.http from org.eclipse.jgit
Add missing @since tag and silence API error
Change-Id: I2783a15ead26ab19de31a8fb3bfb148ef19de91a
Signed-off-by: Matthias Sohn <matthias.sohn@sap.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>
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>
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 class names imply that RecursiveMergerTest tests the RecursiveMerger
and ResolveMergerTest tests the ResolveMerger.
In fact, both of them include coverage of both strategies; the difference
is that RecursiveMergerTest is only testing criss-cross merges.
The tests cannot be combined into a single class because the criss-cross
test methods have additional data points.
Instead, rename the classes to more meaningful names.
Change-Id: I7ca8a03a3b7e351e2d4fcaca3b3186c098a3ca66
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
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>
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>
The local variable 'remote' hides the class scope variable
of the same name.
Change-Id: I7410c33678677ce2a14691772466d91e8139e3fa
Signed-off-by: David Pursehouse <david.pursehouse@gmail.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>
* stable-4.11:
ObjectIdSerializer: Support serialization of known non-null ObjectId
Change-Id: Ie430fa2c5d13ae698d884a37d0d03884ebbf25ec
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>
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>
Remove the resource warning suppression and refactor the code to open
the TemporaryBuffer and InflaterInputStream in a try-with-resource.
Change-Id: I3082e5ac7565c5000d5a4364f750dd0a0952fc6e
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
David Pursehouse
Christian Halstrick
Dave Borowitz
Markus Duft
Thomas Wolf
Matthias Sohn
Jonathan Nieder
Minh Thai
Jonathan Tan