Shawn Pearce [Mon, 14 Dec 2015 04:26:01 +0000 (20:26 -0800)]
push: Do not blindly overwrite peer
If an application uses PushConnection directly on the native Git wire
protocols JGit should send along the application's expected oldId, not
the advertised value. This allows the remote peer to compare-and-swap
since it was not tested inside JGit.
Discovered when I tried to use a PushConnection (bypassing the
standard PushProcess) and the client blindly overwrote the remote
reference, even though my app had supplied the wrong ObjectId for
the expectedOldObjectId. This was not expected and cost me over an
hour of debugging, plus "corruption" in the remote repository.
By passing along the exact expectedOldObjectId from the app the
remote side can do the check that the application skipped, and
avoid data loss.
Jonathan Nieder [Mon, 7 Dec 2015 20:56:32 +0000 (12:56 -0800)]
Use runtime retention for Nullable annotation
JGit's Nullable type was added[1] in the hope of being able to add
nullness annotations that (a) do not preclude building and running
with Java 7 and (b) could be shared by Gerrit, which uses a custom
Nullable type for other reasons[2]. Sharing a type is useful because
Eclipse's null analysis is only able to use one Nullable type at a
time in a given workspace (so for this analysis to function in a
workspace used to develop Gerrit, JGit and Gerrit would need to use
the same Nullable type).
The new Nullable type has CLASS instead of RUNTIME retention because
there wasn't any obvious use for the annotation at run time.
Gerrit uses the Nullable annotation to communicate with Guice. Guice
injection happens at runtime, so it needs to be able to read the
@Nullable annotations at run time[3]. Otherwise Guice produces
provisioning errors, such as
3) null returned by binding at com.google.gerrit.lucene.LuceneChangeIndex$Factory.create()
but parameter 7 of com.google.gerrit.lucene.LuceneChangeIndex.<init>() is not @Nullable
Switch to RUNTIME retention to avoid this.
While at it, update the javadoc to explain more clearly how this
annotation relates to other Nullable types[4]. This should make it
clearer why JGit needed another Nullable type:
A. Avoiding dependency on Java 8
B. RUNTIME retention to allow Guice to read the annotation at run time
C. Named Nullable so Guice can recognize the annotation
D. Not an addition to Java EE's javax.annotation package, to avoid
the split-package problem[2] that prevents the annotation from
being readable at run time when loaded from an OSGi container
E. Avoiding heavyweight dependencies, deprecated dependencies, and
dependencies on package internals
org.checkerframework.checker.nullness.qual.Nullable: A
com.sun.istack.internal.Nullable: B, E
*.CheckForNull, *.NullAllowed, etc: C
edu.umd.cs.findbugs.annotations.Nullable: B, E
javax.annotation.Nullable: D
org.eclipse.jdt.annotation.Nullable: B
org.jetbrains.annotations.Nullable: B
org.jmlspecs.annotation.Nullable: E
android.annotation.Nullable, android.support.annotation.Nullable: E
Shawn Pearce [Thu, 3 Dec 2015 05:47:58 +0000 (21:47 -0800)]
Support atomic push in JGit client
This should mirror the behavior of `git push --atomic` where the
client asks the server to apply all-or-nothing. Some JGit servers
already support this based on a custom DFS backend. InMemoryRepository
is extended to support atomic push for unit testing purposes.
Local disk server side support inside of JGit is a more complex animal
due to the excessive amount of file locking required to protect every
reference as a loose reference.
Shawn Pearce [Sat, 28 Nov 2015 17:23:59 +0000 (09:23 -0800)]
DirCacheBuilder: Speed up reading from trees
Recursively copying a tree into a DirCache is a bottleneck for some
algorithms like the in memory merge code in Gerrit Code Review. Drop
a layer down in the stack and use CanonicalTreeParser directly as the
addition logic only processes 1 tree at a time and does not need the
merge sorting feature (or overhead) of TreeWalk.
Combined with 761814fe9c ("DirCacheEntry: Speed up creation by
avoiding string cast") tree loading 38,900 entries nearly halves
in running time from 70ms to 36ms on some platforms.
Shawn Pearce [Sun, 29 Nov 2015 20:04:03 +0000 (12:04 -0800)]
Delay locating .gitattributes until requested
Instead of checking every entry for .gitattributes only look for the
entry on request by TreeWalk. This avoids impacting uses like RevWalk
filtering history.
When the attrs is requested skip to the start of the tree and look for
.gitattributes until either it is found, or it is impossible to be
present. Due to the sorting rules of tree entries .gitattributes
should be among the first or second entries in the tree so very few
entries will need to be considered.
Waiting to find the .gitattributes file by native ordering may miss
attrs for files like .config, which sorts before .gitattributes.
Starting from the front of the tree on demand ensures the attributes
are parsed as early as necessary to process any entry in the tree.
Due to TreeWalk recursively processing up the tree of iterators we
cannot just reset the current CanonicalTreeParser to the start as
parent parsers share the same path buffer as their children.
Resetting a parent to look for .gitattributes may overwrite path
buffer data used by a child iterator. Work around this by building a
new temporary CanonicalTreeParser instance.
Shawn Pearce [Sat, 28 Nov 2015 17:21:31 +0000 (09:21 -0800)]
DirCache: Fix bad code formatting
Line breaking before , is ugly to read. Most formatters and humans
expect line break after , so update a few offensive locations.
Use String.format() for the construction of the error message when
a bad DirCachEntry is being failed on. This simplifies the code and
its not a performance critical section.
Shawn Pearce [Sat, 28 Nov 2015 16:58:15 +0000 (08:58 -0800)]
DirCacheEntry: Speed up creation by avoiding string cast
The checkPath function is available as a byte[] form, in fact the
String form just converts to byte[] to run the algorithm.
Having DirCacheEntry take a byte[] -> String -> byte[] to check if
each path is valid is a huge waste of CPU time. On some systems it
can double the time required to read 38,999 files from trees to the
DirCache. This slows down any operation using a DirCache.
Expose the byte[] form and use it for DirCacheEntry creation.
Shawn Pearce [Sat, 28 Nov 2015 05:34:16 +0000 (21:34 -0800)]
DirCache: Fix getEntriesWithin("") to not include null entries
The internal array may be longer than entryCnt, in this case the tail
of the array is padded with null entries. Do not return those to the
caller of getEntriesWithin().
If defined in .gitattributes call smudge filter during checkout.
To support checkout where current HEAD,index do not contain attributes
we need to also consider attributes from the tree we checkout. Therefore
CanonicalTreeParser has to learn how to provide attributes.
Change-Id: I168fdb81a8e1a9f991587b3e95a36550ea845f0a Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Ivan Motsch [Tue, 17 Nov 2015 12:32:20 +0000 (13:32 +0100)]
Add the new class Attributes holding multiple Attribute(s)
Attributes represents a semantic collector of Attribute(s) and replaces
the anonymous Map<String,Attribute>. This class will be returned by
TreeWalk.getAttributes(). It offers convenient access to the attributes
wrapped in the Attributes object. Adds preparations for a future
Attribute Macro Expansion
Change-Id: I8348c8c457a2a7f1f0c48050e10399b0fa1cdbe1 Signed-off-by: Ivan Motsch <ivan.motsch@bsiag.com>
PushCommandTest and RunExternalScriptTest didn't succeed on Windows.
Fix this by expecting a simple line-feed as line ending (instead of the
platform dependent line separator. Additionally correct the computation
of expected URLs in PushCommandTest.
Change-Id: Idcdc41cd7e535ff88df33ea0a249333ed8fc91b0 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
This test expected that the test scripts emit a platform-dependent
newline (crlf on windows, lf on linux). But that's not true. Expected
result should always be a trailing "\n" because the test scripts
explicitly echo a "\n" in the end.
When FS.runProcess was called and an InputStream was given the method
tried to pump the whole InputStream to the process. When the method
ended the InputStream was not giving any data anymore. Consequently
close the InputStream inside the method.
Jonathan Nieder [Thu, 19 Nov 2015 20:47:18 +0000 (12:47 -0800)]
repo: Do not use search path to find refs/remotes/origin/<branch>
When running from a non-bare repository, "jgit repo" checks whether
the rev passed in is a sha1 or branch name and in the latter case will
check out origin/<branch>.
We are expecting refs/remotes/origin/<branch>, but as a side effect of
using getRef we also end up looking for refs/origin/<branch>,
refs/heads/origin/<branch>, and so on. Avoid that by using exactRef
instead.
Signed-off-by: Jonathan Nieder <jrn@google.com>
Change-Id: I670b2e48a88138a1f2104ea201baa958e9edbddb
Jonathan Nieder [Fri, 5 Jun 2015 22:20:24 +0000 (15:20 -0700)]
Repository: Introduce exactRef and findRef, deprecate getRef
The Repository class provides only one method to look up a ref by
name, getRef. If I request refs/heads/master and that ref does not
exist, getRef will look further in the search path:
Andrey Loskutov [Sun, 15 Nov 2015 22:59:41 +0000 (23:59 +0100)]
Null-annotated Repository class and fixed related compiler errors
org.eclipse.jgit.lib.Repository class is an example of the API which
should be written with Java 8 java.util.Optional<T> type. Unfortunately
this API is already released and widely used. The good clients are
currently doing their best with checking return values for null and bad
clients do not know how bad their code is.
I've tried not to change any logic and to be as less intrusive as
possible. Most of the JGit code was well prepared to this, only few
classes needed some smaller fixes.
This change fixes all compiler errors in JGit and replaces possible
NPE's with either appropriate exceptions, avoiding multiple "Nullable
return" method calls or early returning from the method.
Because annotating getDirectory() and getFS() as Nullable would cause
lot of additional changes in JGit and EGit they are postponed.
Matthias Sohn [Fri, 20 Nov 2015 13:40:41 +0000 (14:40 +0100)]
Raise error if FileNotFoundException is caught for an existing file
File, FileInputStream and friends may throw FileNotFoundException even
if the file is existing e.g. when file permissions don't allow to access
the file content. In most cases this is a severe error we should not
suppress hence rethrow the FileNotFoundException in this case.
This may also fix bug 451508.
Bug: 451508
Change-Id: If4a94217fb5b7cfd4c04d881902f3e86193c7008 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Let FS_Win32_Cygwin detect symlink support by creating temporary symlink
The class FS_Win32 was always trying out to create a temporary symlink
in order to find out whether symlinks are supported. FS_Win32_Cygwin was
overwriting this method and always returned true. But when the user
running JGit does not have administrative rights then the creation of
symlinks is forbidden even if he is running on FS_Win32_Cygwin. A lot of
tests failed only on the Windows platform because of this. It was
correctly detected that FS_Win32_Cygwin is the filesystem abstraction to
be used but creation of symlinks always failed because of lacking
privileges of the user running the tests.
This fix teaches FS_Win32_Cygwin to behave like FS_Win32 and to test
whether symlinks can be created in order to find out whether symlinks
are supported.
Change-Id: Ie2394631ffc4c489bd37c3ec142ed44bbfcac726 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn [Thu, 12 Nov 2015 10:28:01 +0000 (11:28 +0100)]
Avoid UnknownHostException in WalkEncryptionTest
Prevent that WalkEncryptionTest fails when it can't determine the public
IP address using http://checkip.amazonws.com. Also set timeouts when
determining IP address in order to prevent long wait times during tests.
Change-Id: I1d2fe09f99df2a5f75f8077811a72fb2271cdddb Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If readRef returns null (indicating that the ref does not exist), the
loop continues so we can find the ref later in the search path. And
resolve should never return null, so if we return null it should mean
we exhausted the entire search path and didn't find the ref.
... except that resolve can return null: it does so when it has
followed too many symrefs and concluded that there is a symref loop:
if (MAX_SYMBOLIC_REF_DEPTH <= depth)
return null; // claim it doesn't exist
Continue the loop instead of returning null immediately. This makes
the behavior more consistent.
Arguably getRef should throw an exception when a symref loop is
detected. That would be a more invasive change, so if it's a good
idea it will have to wait for another patch.
Change-Id: Icb1c7fafd4f1e34c9b43538e27ab5bbc17ad9eef Signed-off-by: Jonathan Nieder <jrn@google.com>
Throw IndexReadException if existing index can't be read
If the index file exists but can't be read for example because of wrong
filesystem permissions we should throw a specific exception. This allows
EGit to handle this error situation.
Bug: 482607
Change-Id: I50bfcb719c45caac3cb5550a8b16307c2ea9def4 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn [Tue, 17 Nov 2015 16:26:53 +0000 (17:26 +0100)]
Fix pre-push hook to not set null remoteName as first argument
According to [1] the pre-push hook expects two parameters which provide
the name and location of the destination remote, if a named remote is
not being used both values should be the same.
We did set the first parameter to null in that case which caused
ProcessBuilder to throw a NullPointerException since its start() method
doesn't accept null arguments.
[1] https://git-scm.com/docs/githooks#_pre_push
Bug: 482393
Change-Id: Idb9b0a48cefac01abfcfdf00f6d173f8fa1d9a7b Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Add an attribute accessor to CanonicalTreeParser and use it in Treewalk
When checking out a branch we need to access the attributes stored
in the tree to be checked out. E.g. directly after a clone we checkout
the remote HEAD. In this case index and workingtree are still empty.
So we have to search the tree to be checked out for attributes.
Arthur Daussy [Fri, 31 Oct 2014 16:46:36 +0000 (17:46 +0100)]
Adds the git attributes computation on the treewalk
Adds the getAttributes feature to the tree walk. The computation of
attributes needs to be done by the TreeWalk since it needs both a
WorkingTreeIterator and a DirCacheIterator.
Bug: 342372
CQ: 9120
Change-Id: I5e33257fd8c9895869a128bad3fd1e720409d361 Signed-off-by: Arthur Daussy <arthur.daussy@obeo.fr> Signed-off-by: Christian Halstrick <christian.halstrick@sap.com>
Andrey Loskutov [Mon, 16 Nov 2015 22:41:38 +0000 (23:41 +0100)]
Make jgit annotations accessible to other plugins
Other plugins which want to use JGit nullness annotations in their code
cannot do this if the annotations aren't part of the published API.
Unfortunately it looks like although Eclipse JDT allows to use custom
nullness annotation types per project, it does not understand if those
annotations are used mixed with other nullness annotations in other
projects. E.g. EGit can either configure JGit annotations for NPE
analysis and so "understand" nullness from JGit API but so it loses the
ability to use any other nullness annotations to annotate its own code.
Andrey Loskutov [Sun, 15 Nov 2015 21:55:58 +0000 (22:55 +0100)]
Added jgit own NonNull annotation type
The annotation is required for example in Repository case (patch
follows), where almost all non-void return methods return Nullable
except few returning NonNull. I definitely do not favor this style, but
it is a nightmare to clients to guess if the null check is needed or
not.
Matthias Sohn [Sun, 15 Nov 2015 21:56:55 +0000 (22:56 +0100)]
Handle InternalError during symlink support detection on Windows
When JGit tries to detect symlink support the attempt to create a
symlink may fail with a java.lang.InternalError. This was reported for a
case where the application using JGit runs in Windows XP compatibility
mode using Oracle JDK 1.8. Handle this by assuming symlinks are not
supported in this case.
Bug: 471027
Change-Id: I978288754dea0c6fffd3457fad7d4d971e27c6c2 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Matthias Sohn [Fri, 13 Nov 2015 23:59:36 +0000 (00:59 +0100)]
Merge branch 'stable-4.1'
* stable-4.1:
Prepare 4.1.2-SNAPSHOT builds
JGit v4.1.1.201511131810-r
Fallback exactRef: Do not ignore symrefs to unborn branch
RefDirectory.exactRef: Do not ignore symrefs to unborn branch
Change-Id: I66afb303f355aad8a7eaa7a6dff06de70ae9c490 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Jonathan Nieder [Wed, 11 Nov 2015 18:42:46 +0000 (10:42 -0800)]
Fallback exactRef: Do not ignore symrefs to unborn branch
When asked to read a symref pointing to a branch-yet-to-be-born (such
as HEAD in a newly initialized repository), getRef and getRefs provide
different results.
getRef: SymbolicRef[HEAD -> refs/heads/master=00000000]
getRefs and getAdditionalRefs: nothing
exactRef should match the getRef behavior: it is meant to be a
simpler, faster version of getRef that lets you search for a ref
without resolving it using the search path without other semantic
changes. But the fallback implementation of exactRef relies on getRefs
and produces null for this case.
Luckily the in-tree RefDatabase implementations override exactRef and
get the correct behavior. But any out-of-tree storage backend that
doesn't inherit from DfsRefDatabase or RefDirectory would still return
null when it shouldn't.
Let the fallback implementation use getRef instead to avoid this.
This means that exactRef would waste some effort traversing the ref
search path when the named ref is not found --- but subclasses tend to
override exactRef for performance already, so in the default
implementation correctness is more important.
Jonathan Nieder [Tue, 10 Nov 2015 23:11:04 +0000 (15:11 -0800)]
RefDirectory.exactRef: Do not ignore symrefs to unborn branch
When asked to read a symref pointing to a branch-yet-to-be-born (such
as HEAD in a newly initialized repository), DfsRepository and
FileRepository return different results.
getRef("HEAD") returns the same as DfsRepository's exactRef in both
backends.
The intended behavior is the DfsRepository one: exactRef() is supposed
to be like getRef(), but more exact because it doesn't need to
traverse the search path.
The discrepancy is because DfsRefDatabase implements exactRef()
directly with the intended semantics, while RefDirectory uses a
fallback implementation built on top of getRefs(). getRefs() skips
symrefs to an unborn branch.
Override the fallback implementation with a correct implementation
that is similar to getRef() to avoid this. A followup change will fix
the fallback.
Change-Id: Ic138a5564a099ebf32248d86b93e2de9ab3c94ee Reported-by: David Pursehouse <david.pursehouse@sonymobile.com> Improved-by: Christian Halstrick <christian.halstrick@sap.com>
Bug: 478865
BitmapBuilder is an interface to be implemented by implementors of this
interface. According to OSGi semantic versioning rules breaking API
changes require update of the minor version only if implementors of the
API have to be adapted and the changes do not affect clients of the API.
Change-Id: If45d204181ea9bc788b6b57693ca17b1847564c7 Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Jonathan Nieder [Mon, 9 Nov 2015 16:48:46 +0000 (08:48 -0800)]
Make BitmapIndexImpl.CompressedBitmap public
PackWriterBitmapPreparer (which is in another package) is already well
aware of the mapping between EWAHCompressedBitmaps and the
higher-level CompressedBitmap objects of the BitmapIndexImpl API.
Making the CompressedBitmap type public makes the translation more
obvious and wouldn't break any abstractions that aren't already
broken. So expose it.
This is all under org.eclipse.jgit.internal so there are no API
stability guarantees --- we can change the API if internals change
(for example if some day there are bitmaps spanning multiple packs).
In particular this means the confusing toBitmap helper can be removed.
Terry Parker [Mon, 9 Nov 2015 22:19:11 +0000 (14:19 -0800)]
Update dependencies to use the JGit-internal @Nullable
Update the project-specific Eclipse settings to replace the use of the
org.eclipse.jdt.annotation.Nullable class the new JGit-specific
@Nullable annotation. I verified that Eclipse reports errors when the
return value of a method annotated with
@org.eclipse.jgit.annotations.Nullable is dereferenced without a null
check.
Also remove the Maven and MANIFEST.MF dependencies on
org.eclipse.jdt.annotation.
Eclipse null analysis uses three annotations: @Nullable, @NonNull and
@NonNullByDefault. All three are updated in this patch because it is
invalid to set the Eclipse preferences to empty values. So far only
@Nullable has been introduced in org.eclipse.jgit.annotations.
My personal preference is to follow the advice in Effective Java and
avoid the null-return idiom, and to avoid passing null values in
general. This sets the expectation is that arguments and return types
are assumed non-null unless otherwise documented. If that is the
expectation, then consistent application of @NonNull is redundant and
hurts readability by cluttering the code, obscuring the occasional
@Nullable annotation that really requires attention.
If the JGit community decides there is value in using the @NonNull and
@NonNullByDefault annotations we can add them--this change configures
Eclipse to use them.
Change-Id: I9af1b786d1b44b9b0d9c609480dc842df79bf698 Signed-off-by: Terry Parker <tparker@google.com>
Terry Parker [Mon, 9 Nov 2015 21:40:39 +0000 (13:40 -0800)]
Add a JGit-internal Nullable type
Commit 847b3d1 enabled annotation-based NPE analysis to JGit.
In so doing, it introduced a new dependency on the org.eclipse.jdt that
is undesirable. Follow Gerrit's lead by adding an internal Nullable type
(see
https://gerrit.googlesource.com/gerrit/+/stable-2.11/gerrit-common/src/main/java/com/google/gerrit/common/Nullable.java).
The javax.annotations.Nullable class uses a retention policy of RUNTIME,
whereas the org.eclipse.jdt.annotation.Nullable class used a policy of
CLASS. Since I'm not aware of tools that make use of the annotation at
runtime I chose the CLASS policy. If in the future there is a benefit to
retaining the annotations in the running binary we can update this
class.
Change-Id: I63dc8f9a6dc46b517cbde211bb5e2f8521a54d04 Signed-off-by: Terry Parker <tparker@google.com>
Jonathan Nieder [Mon, 9 Nov 2015 21:14:13 +0000 (13:14 -0800)]
Make BitmapIndexImpl.CompressedBitmap, CompressedBitmapBuilder static
A CompressedBitmap represents a pair (EWAH bit vector, PackIndex
assigning bit positions to git objects). The bit vector is a member
field and the PackIndex is implicit via the 'this' reference to the
outer class.
Make this clearer by making CompressedBitmap a static class and
replacing the 'this' reference by an explicit field.
Likewise for CompressedBitmapBuilder.
Change-Id: Id85659fc4fc3ad82034db3370cce4cdbe0c5492c Suggested-by: Terry Parker <tparker@google.com>
projects / jgit.git / log