Error Prone reports:
[ClassNewInstance] Class.newInstance() bypasses exception checking;
prefer getDeclaredConstructor().newInstance()
See https://errorprone.info/bugpattern/ClassNewInstance
This was the only occurrence of the warning in the code base; now it's
fixed, increase the severity to ERROR to prevent future occurrences.
Change-Id: Ic04d1c5d2bd458bbb4bb399d6ce9d147bd48d0b1
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
The placeholders in manifest and plugin.properties did not match. To
avoid similar issues, all placeholders have been changed to
Bundle-Vendor and Bundle-Name now.
Bug:548503
Change-Id: Ibd4b9bc237b323e614506b97e5fbc99416365040
Signed-off-by: Michael Keppler <Michael.Keppler@gmx.de>
The deleted code is not required as removed files are deleted correctly in
doCheckout() anyway.
The deleted code failed in case a non-empty directory had to be deleted.
file.delete() returned false, triggering an exception.
Bug: 479266
Change-Id: I011bb3882ff0c35b238aa3eccad7889041210277
Signed-off-by: René Scheibe <rene.scheibe@gmail.com>
Creating a folder failed in case a file with the same name already
existed.
Bug: 479266
Change-Id: Ia987660ec0968ad4081dbd5a60e80660539497e3
Signed-off-by: René Scheibe <rene.scheibe@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Archives created by the ArchiveCommand didn't produce deterministic
archive hashes. For RevCommits RevWalk.parseTree returns the root tree
instead of the RevCommit hence retrieving the commit's timestamp didn't
work. Instead use RevWalk.parseAny and extract the tree manually.
Archive entries store timestamps with 1 second resolution hence we need
to wait longer when creating the same archive twice and compare archive
hashes. Otherwise hash comparison in tests wouldn't fail without this
patch.
Bug: 548312
Change-Id: I437d515de51cf68265584d28a8446cebe6341b79
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
update Maven plugins
- jacoco-maven-plugin to 0.8.4
- japicmp-maven-plugin to 0.14.1
- maven-compiler-plugin to 3.8.1
- maven-deploy-plugin to 3.0.0-M1
- maven-enforcer-plugin to 3.0.0-M2
- maven-install-plugin to 3.0.0-M1
- maven-jar-plugin to 3.1.2
- maven-javadoc-plugin to 3.1.0
- maven-jxr-plugin to 3.0.0
- maven-pmd-plugin to 3.12.0
- maven-resources-plugin to 3.1.0
- maven-shade-plugin to 3.2.1
- maven-source-plugin to 3.1.0
- maven-surefire-plugin to 3.0.0-M3
- spotbugs-maven-plugin to 3.1.12
- tycho to 1.3.0
- tycho-pack200a-plugin to 1.3.0
- tycho-pack200b-plugin to 1.3.0
Cleanup Maven warnings
- pin version of all used Maven plugins
- remove deprecated way to declare minimum Maven version
Change-Id: If23e2e2bb03e5e1e7b1eb9d4924a8faa0aa3704e
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Replace javax.xml.bind.DatatypeConverter, that is not available any
more in Java 11 and later with Hex utility from non optional Bouncy
Castle library.
Bug: 540790
Change-Id: I9903c00ecc1a434e9795b8ba9267f02628fdc0e9
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
As reported by Error Prone:
An inner class should be static unless it references members of its
enclosing class. An inner class that is made non-static unnecessarily
uses more memory and does not make the intent of the class clear.
See https://errorprone.info/bugpattern/ClassCanBeStatic
Change-Id: Ib99d120532630dba63cf400cc1c61c318286fc41
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
(cherry picked from commit ee40efcea4)
Error Prone reports the warning on several classes:
[NonOverridingEquals] equals method doesn't override Object.equals;
if this is a type-specific helper for a method that does override
Object.equals, either inline it into the callers or rename it to
avoid ambiguity.
See https://errorprone.info/bugpattern/NonOverridingEquals
Most of these are in the public API, so we can't rename or inline them
without breaking the API. FileSnapshot is not part of the public API,
but clients may be using it anyway, so we also shouldn't change that.
Suppress all the warnings instead. Having the check at severity ERROR
will at least make sure we don't introduce any new occurrences.
Change-Id: I92345c11256f06b4fa03ccc13337f72af5a43591
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
ParseableSimpleDateFormat is an enum, and enums must be immutable,
hence the member should be final. At the same time, make it private
since it does not need to be publicly visible.
Change-Id: I7e181f591038d556f1123b6e37adf8441059e99a
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Reference comparison is intentional. The END and DELIM string
constants are used as sentinels and will always be the same
instances.
Suppress both ReferenceEquality and StringEquality warnings.
Change-Id: I4ce0495702c56b3911f42f26c2f81d28073cbe19
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Reference comparison with EMPTY and MISSING_FILE is intended; these
are static instances used as markers, and will always be the same
instances.
Change-Id: Ic27f5b797bdb9370cf8f6b3b7bb3f1523d4a454c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
When cloning repository with --single-branch option, tag chains are not
packed and pack file is broken in some cases.
Typical test-case:
git tag -a test_tag <commit-id>
git tag -a test_prev_tag test_tag
git tag -d test_tag
git clone --single-branch <repository>
fatal: did not receive expected object <test_tag_id>
The reason for that is missing object for original test_tag reference,
which was deleted.
Problem description:
When pack-objects is given --include-tag, it peels each tag reference
down to a commit. If the commit is prepared to be packed, we we have to
include such tag too. The problem is when the tag points to through some
chain of other tag to commit. Then, the inner tags are not added leading
to broken pack.
Fix:
When going to commit, we have to check and add any of the tags on the
way (if they were not selected, which may happen with --single-branch
option).
Change-Id: I1682d4a2c52d674f90a1b021e0f6c3524c5ce5bc
Signed-off-by: Pavel Flaška <Pavel.Flaska@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Issues reported by downstream analyzers.
The "hash" method can be static.
It is a good practice to group overloaded methods. Move the write(URL)
method with the other writes.
Change-Id: Ia42c0d7081333edcb77e58d5e627929d29672490
Signed-off-by: Ivan Frade <ifrade@google.com>
Downstream analyzers reported empty fields in the javadoc. I corrected
few more details:
* Fill empty javadoc fields.
* Use <p> to separate description paragraphs.
* End description paragraphs with a period.
* Remove period at the end of field descriptions.
Change-Id: I749e4b821fc855999caddc442ac788fa514386ea
Signed-off-by: Ivan Frade <ifrade@google.com>
Reported by downstream analyzers. Suppress the warning on reference
equality for isMissing and fill an empty javadoc field.
Change-Id: I3494423daf2a53ca10e0a9c66553f00204c35396
Signed-off-by: Ivan Frade <ifrade@google.com>
Using the | and & operators in boolean conditions results in a warning
from Error Prone:
[ShortCircuitBoolean]
Prefer the short-circuiting boolean operators && and || to & and |.
see https://errorprone.info/bugpattern/ShortCircuitBoolean
Change-Id: I182f986263b8b9ac189907f4bd1662b4092a52d8
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Using the | and & operators in boolean conditions results in a warning
from Error Prone:
[ShortCircuitBoolean]
Prefer the short-circuiting boolean operators && and || to & and |.
see https://errorprone.info/bugpattern/ShortCircuitBoolean
Change-Id: I6cccca3fdd28bf93b302a9b8a66e68ac912cb60d
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Using the | and & operators in boolean conditions results in a warning
from Error Prone:
[ShortCircuitBoolean]
Prefer the short-circuiting boolean operators && and || to & and |.
see https://errorprone.info/bugpattern/ShortCircuitBoolean
Change-Id: I4275c60306e43c74030c4465ba02cb853ad444e1
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Fix all remaining instances of the OperatorPrededence warning, by adding
parentheses to make the precedence explicit.
Change-Id: Ib296dfed09f9be042d0ff0f7fad8214e4dd766b4
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
In a delete-modify conflict with the deletion as "ours" there may be
no stage 2 in the index. Add appropriate null checks. Add a new test
for this case, and verify that the file gets added with a single LF
after conflict resolution with core.autocrlf=true. This matches the
behavior of canonical git for this case.
Bug: 547724
Change-Id: I1bafdb83d9b78bf85294c78325e818e72fae53bc
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Rename MAGIC_EMPTY_VALUE to MISSING_ENTRY, make it private, and add
a helper method to check if a given string is that value.
This avoids that callers trigger the "reference equality" warning
from Error Prone.
Change-Id: Idc76f78c0cf1828aa48d02ee33911a4b5df50355
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
The TODO comments say "in 5.0", but 5.0 was already released without
resolving them. Remove "in 5.0" on the assumption that the mentioned
improvements still need to be done at some point.
Change-Id: I3eb429803e2266de3fc490e1f3912991c08aa1ad
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
When loading the config, a FileNotFoundException may occur if the file
exists but cannot be read (see [1]). This is the case on Windows with a
virus scanner checking the file. Therefore if the file exists and that
exception is thrown, retry multiple times, similar to how this was
already implemented for IOException.
[1] https://docs.oracle.com/javase/8/docs/api/java/io/FileNotFoundException.html
Bug: 529522
Change-Id: Ic5dc3b7b24bb0005d6256ed00513bc7c0b91e613
Signed-off-by: Michael Keppler <Michael.Keppler@gmx.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
A merging pull on an unborn branch was already supported. But a
rebasing pull failed. If the user has pull.rebase = true in his
user config, the pull would try to rebase. Rebasing needs a parent
commit, though. Native git handles this case:
git init
git remote add origin <URI>
git pull --rebase origin master
Check up front in PullCommand for the unborn head and just do a
checkout in this case. MergeCommand already has similar code.
Bug: 544965
Change-Id: I1277e1ac0b0364b4623fd791f3d6b07bd5f58fca
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
We logged a stack trace if the configured http.cookieFile was missing.
Instead only log a warning.
Bug: 548081
Change-Id: I42e39f5ad8ffce7b43162e5068f60af073b8a126
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Canonical git treats CR-LF in config files as LF.[1][2] JGit does so,
too, except when escaped as a line continuation. Correct this and
treat the sequence \-CR-LF as a line continuation.
[1] https://github.com/git/git/commit/db2c075d9
[2] https://github.com/git/git/blob/v2.21.0/config.c#L485
Bug: 545850
Change-Id: I51e7378a22c21b3baa3701163c423d04c900af5a
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Otherwise tags may fail to match if their name contains slashes.
Canonical git also uses its wildcard matcher in glob mode.[1]
[1] https://github.com/git/git/blob/v2.21.0/builtin/describe.c#L182
Bug: 546703
Change-Id: I122c7959974fa1fc6a53dfc65837e4314a8badd4
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Commit messages of length 1 were not read. 'lineEnd' is the offset
of the last character in the line before the terminating LF or CR-LF,
and 'nextSpace' is actually the offset of the character _after_ the
next space. With a one-character commit message, nextSpace == lineEnd.
The code also assumes the commit message to be optional, but actually
failed in that case because it read beyond the line ending. Fix that,
too.
Add a test case for reading a todo file.
Bug: 546245
Change-Id: I368d63615930ea2398a6230e756442fd88870654
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Replace "size() > 0" with "!isEmpty()" where appropriate.
In the Status implementation we can drop the check; the subsequent
loop will only execute when the list is non-empty anyway.
Change-Id: I355aff551a603373e702a9d44304f087b476263c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Error Prone reports:
[SizeGreaterThanOrEqualsZero] Comparison of a size >= 0 is always true,
did you intend to check for non-emptiness?
see https://errorprone.info/bugpattern/SizeGreaterThanOrEqualsZero
Change-Id: Ie964771cacca4b15569eb45f6e273ad2a7e2e49c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
It's quite possible that JGit can use the hard-linking mechanism
for atomic file creation on some volumes but not on others.
Ultimately it depends on the file systems on the mounted volumes.
Cache the information per FileStore instead of using a single
global flag. Also catch FileSystemException, it may be thrown
if the operating system reports a failure. The previously caught
AccessDeniedException is a sub-class of FileSystemException.
Bug: 547332
Change-Id: I1ef672b3468b0be79e71674344f16f28f9d11ba1
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
The git config entries "http.cookieFile" and
"http.saveCookies" are correctly evaluated.
Bug: 488572
Change-Id: Icfeeea95e1a5bac3fa4438849d4ac2306d7d5562
Signed-off-by: Konrad Windszus <konrad_w@gmx.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
When Error Prone checks are enabled, the "ClassCanBeStatic" warning is
triggered:
Inner class is non-static but does not reference enclosing class
see https://errorprone.info/bugpattern/ClassCanBeStatic
Change-Id: I5a0e3bf0cf8c28176d9c98914c1c0dfab9c5736f
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
This change is needed to implement permission aware ref database in
Gerrit: [1], that is a pre-requisite to re-enable Git v2 protocol in
Gerrit: [2].
Background: Last year Git v2 protocol was enabled in Gerrit. The fact,
that JGit layer was not calling ref advertise filter for Git v2
protocol, introduced security vulnerability.
The lesson learned from this security incident: Gerrit should not rely
on ref advertise filter being called by JGit to implement crictical
security checks. Instead, the idea is to use the same approach as
currently used by Google's internal code on googlesource.com that
didn't suffer from this vulnerability: provide a custom repository to
JGit. The repository provides a RefDatabase that is permission-aware
and will only ever return refs that the user has access to.
However, due to hard coded instanceof operator usages in JGit code
base, some tests in Gerrit are failing with: [1] in place. This change
addresses this problem.
[1] https://gerrit-review.googlesource.com/c/gerrit/+/212874
[2] https://gerrit-review.googlesource.com/c/gerrit/+/226754
Change-Id: I67c0f53ca33b149442e7ee3e51910d19e3f348d5
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
David Pursehouse
Michael Keppler
René Scheibe
Matthias Sohn
David Ostrovsky
Pavel Flaška
Ivan Frade
Thomas Wolf
Konrad Windszus