In NoteDb, we store draft comments in the All-Users repository. This
improves performance in the googlesource.com deployment, as
high-traffic repositories can be configured to bypass Gerrit ACL
checks for the ref advertisement.
This makes publishing comments non-atomic: after publishing comments
successfully, the update of the draft branch may fail. For this
reason, any NoteDb update that touches the draft ref in All-Users will
schedule a deletion of all published comments for the change.
If there were multiple patchsets, scheduling deletions of items that
were not in the draft ref triggered a logic error in
ChangeDraftUpdate, which caused the branch to become empty rather than
deleted. This had the following consequences:
* unused draft refs persisted in the All-Users repository, polluting
the namespace.
* published draft comments as well as deleted draft comments were kept
in the history of the draft ref, keeping them alive for GC, and
causing a steady increase of repository size.
In other words, for this to trigger, a change would have to publish
draft comments on a change with multiple patchsets, and a previously
published comment. The problem was not visible to users of Gerrit.
This changes fixes the problem by simply looking at the notemap to be
serialized, and deleting the draft ref if the new notemap is empty.
Existing refs must still be cleaned up. We could introduce code,
perhaps in the ProjectConsistencyChecker, to remove the stale refs
from All-Users.
Change-Id: I6f7b65828bb047bbff041b4d78f6b40190e76219
* stable-3.0:
ChangeEmail: add project to email headers
OutgoingEmail: use consistently va.smtpRcptTo reference
NotificationEmail: use MailHeader for 'Gerrit-Branch' footer name
Change-Id: Ia76859ff003745f97df620aced267f6aeda28c2d
* stable-2.16:
ChangeEmail: add project to email headers
OutgoingEmail: use consistently va.smtpRcptTo reference
NotificationEmail: use MailHeader for 'Gerrit-Branch' footer name
Change-Id: I1e1bc425748bea6c5509ebf7c441ab85a04f0d06
This generated some questions repo-discuss@ . This strategy probably
doesn't work on googlesource.com, but we are less concerned about log
spam.
Change-Id: I3746edb79db67408fa66f7e4bb906b23d67acc80
(cherry picked from commit 596da9d839d510b35da15e87f09f02e6e2ad1518)
It would simplify the email filtering: email body wouldn't have to
be checked against project name. This would be also useful for project
specific validation rules.
Change-Id: I795c80d21d6768c14be7726f4450f6b1b5cb9be5
The checkState is asserting that file is not null after being resolved,
but then in the case where it *is* null, attempts to dereference it to
build the error message, which will result in NPE without the expected
message.
Fix it so that it doesn't dereference the null file, but uses the path
that was previously used in the resolve attempt.
At the same time, replace usage of checkState and checkArgument with
requireNonNull. The former methods are not recognized as null checks by
some static checkers, SpotBugs for example, and thus result in false
positive warnings about null dereferences. This also means there is a
slight behavioral change: NullPointerException is raised rather than
IllegalStateException.
Change-Id: If66d32afb1d46ec451a2634c8eac82b46e4d5b8f
* stable-3.0:
Add copy-to-clipboard for generated password
Fix assertions to use isEmpty() rather than hasSize(0)
Fix gr-syntax-params css class
ChangeUpdate#setStatus: Fix error message
Enable doctag in syntax highlighter
Use '2g' when referring to 2 Gigabytes of streamFileThreshold
Change-Id: I8b36bcf6e8ede46fd1572fc3f7a32407ef0ee3ba
* stable-2.16:
Fix gr-syntax-params css class
ChangeUpdate#setStatus: Fix error message
Enable doctag in syntax highlighter
Use '2g' when referring to 2 Gigabytes of streamFileThreshold
Change-Id: I2ca928992b0893484015c3d442691fa9eb70ff8a
Support legacy format decoding was removed in I8c6ac497 ("Remove
unused classes to read/write legacy change notes").
Change-Id: Ia3506c9b1615e6da68ecb71ac2f233f8361ab085
(cherry picked from commit 945f491192a1da8b427befd7b44c98405d9afbfd)
* stable-3.0:
QueryAccount/QueryGroups: Enable retries
MergeSuperSet: Include change ID into error message
DeleteBranch/DeleteBranches: Disallow deletion of HEAD to prevent NPE
DeleteBranches: Disallow deletion of refs/meta/config branch
Change-Id: I9e5fb05c39b69847c8af9bca09a2d7d3979b184d
* stable-2.16:
QueryAccount/QueryGroups: Enable retries
MergeSuperSet: Include change ID into error message
DeleteBranch/DeleteBranches: Disallow deletion of HEAD to prevent NPE
DeleteBranches: Disallow deletion of refs/meta/config branch
Change-Id: I9b33eb83d83fbb4f7d97d0522c7df9882fff3b7f
If there is an error in QueryAccount/QueryGroups we automatically retry
the request, but the retry always fails because
AccountQueryProcessor/GroupQueryProcessor is not reusable [1]. Allow
retries by creating a fresh query processor instance on each try.
[1]
AutoRetry: auto-retry of restapi.account.QueryAccounts has failed [CONTEXT forced=true TRACE_ID="retry-on-failure-1573791374279-221012e6" ]
java.lang.IllegalStateException: AccountQueryProcessor has already been used
at com.google.common.base.Preconditions.checkState(Preconditions.java:589)
at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:206)
at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:194)
at com.google.gerrit.index.query.QueryProcessor.query(QueryProcessor.java:178)
at com.google.gerrit.server.restapi.account.QueryAccounts.apply(QueryAccounts.java:211)
at com.google.gerrit.server.restapi.account.QueryAccounts.apply(QueryAccounts.java:55)
...
Change-Id: I2f7b02962d65d98e83531ffb5b81d4e9287d6fbb
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit bfce38654f1eb352cbcbd24d9bc18957770864af)
This makes the investigation of such issues easier.
Change-Id: I53557825fe11f2b594ba7b21a0a8c16b310bd183
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit 4891360e4a95c306e567b50ce72f5df3630b0873)
Attempting to delete HEAD fails with a NullPointerException [1]. This is
because 'HEAD' gets prefixed with 'refs/heads/' and then the ref is not
found.
We may consider to support deleting HEAD, but that's outside the scope
of this change. E.g. deleting HEAD may rather be separate REST endpoint
that lives next to SetHead since setting HEAD requires a different
permission than deleting a ref.
[1]
java.lang.NullPointerException
at com.google.gerrit.server.restapi.project.DeleteRef.deleteSingleRef(DeleteRef.java:122)
at com.google.gerrit.server.restapi.project.DeleteBranch.apply(DeleteBranch.java:59)
at com.google.gerrit.server.restapi.project.DeleteBranch.apply(DeleteBranch.java:34)
at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestModifyViewWithRetry$4(RestApiServlet.java:740)
at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
at com.github.rholder.retry.Retryer.call(Retryer.java:160)
at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:417)
at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:368)
at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:271)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:820)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestModifyViewWithRetry(RestApiServlet.java:735)
at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:511)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
...
Change-Id: I44c23f4553f05300b7c79bf257b417ec0b496b09
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit 40e1ab1bf084f0b7da66e84b26a84cc4604cbeeb)
The DeleteBranch REST endpoint disallows to delete the refs/meta/config
branch, but using the DeleteBranches REST endpoint one could still
delete it.
Change-Id: I90518a118152be2330c7e533cddd36df9ef774c1
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit 2b80772030a3ecb67a5267ad35567119431b2b23)
* stable-3.0:
Increase 'execution.defaultThreadPoolSize' default and min to 2
Fix formatting of submission IDs
Change-Id: Id8bb9ab54a11cf8a76ae5bbe5f13a23a9df0741a
* stable-2.16:
Increase 'execution.defaultThreadPoolSize' default and min to 2
Fix formatting of submission IDs
Change-Id: Ifca4eb1425a7562534fdad67f59f4553ac3ca9c7
GC operation might be time consuming and it prevents the other
tasks to be executed in WorkQueue (e.g. reviewers plugin cannot add
reviewers). Set default and minimum value to 2 so that running GC
(either scheduled or on-demand) is not blocking other background tasks.
Change-Id: I234e8754f235c1e10837dc98b9c4e3b9568676aa
Signed-off-by: Jacek Centkowski <jcentkowski@collab.net>
If a file doesn't exist in a commit we expect the GetBlame REST endpoint
to return an empty BlameInfo list. This works well unless the file
existed once and had been deleted. E.g. if there is a change that
recreates a file and the user clicks on 'SHOW BLAME' on the diff screen
the request to get the blame info for the base commit fails with 500
Internal Server Error. For example the newly added
GetBlameIT#forRecreatedFileFromBase test was failing like this:
com.google.common.util.concurrent.UncheckedExecutionException: java.lang.NullPointerException
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2051)
at com.google.common.cache.LocalCache.get(LocalCache.java:3953)
at com.google.common.cache.LocalCache$LocalManualCache.get(LocalCache.java:4873)
at com.google.gitiles.blame.cache.BlameCacheImpl.get(BlameCacheImpl.java:114)
at com.google.gerrit.server.restapi.change.GetBlame.blame(GetBlame.java:145)
at com.google.gerrit.server.restapi.change.GetBlame.apply(GetBlame.java:118)
at com.google.gerrit.server.api.changes.FileApiImpl$2.get(FileApiImpl.java:149)
at com.google.gerrit.acceptance.api.revision.GetBlameIT.forRecreatedFileFromBase(GetBlameIT.java:123)
...
Caused by: java.lang.NullPointerException
at com.google.gitiles.blame.cache.BlameCacheImpl.loadRegions(BlameCacheImpl.java:156)
at com.google.gitiles.blame.cache.BlameCacheImpl.loadBlame(BlameCacheImpl.java:139)
at com.google.gitiles.blame.cache.BlameCacheImpl.lambda$newLoader$1(BlameCacheImpl.java:103)
at com.google.common.cache.LocalCache$LocalManualCache$1.load(LocalCache.java:4878)
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529)
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278)
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155)
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045)
... 48 more
The updated version of blame-cache includes a fix for this.
Likely we want to have more tests for the GetBlame REST endpoint, but
adding those is outside the scope of this change.
Bug: Issue 5082
Change-Id: I11b529b48495e563148040dfb7f56379de13d128
Signed-off-by: Edwin Kempin <ekempin@google.com>
When changes get submitted we use the RequestId as submission ID, but
the method that formatted the RequestId for storage was cutting off the
first and last characters from the RequestId. Earlier this was needed
because the RequestId string included square brackets around the actual
request ID. Change I116a188cf removed the square brackets but forgot to
adapt the toStringForStorage() method.
Change-Id: I2d46ede7096eec796a6d99987b7dfff20c3bcb33
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit e3c248c8852bbe4aa047b4db576780b46fcea257)
ChangeNotesParser parses the commits in a change meta ref starting from
the most recent commit. While doing so it keeps track of patch set
creations. If during the parsing we observe meta data for a patch set in
a meta commit that was created before the patch set got created we fail
with a ConfigInvalidException. This works well, unless a patch set got
deleted and then recreated. In this case both patch sets have the same
patch set ID and when we hit the meta data for the deleted patch set we
find that it was created before the patch got created the second time,
and hence the exception is thrown. This means we currently cannot load
any change for which a patch set got deleted and then recreated. This
can affect all kind of operations that need to load such a change. To
fix this we are now skipping the parsing of meta data for deleted patch
sets completely, since it's anyway of no relevance.
Patch set deletions are no longer possible via the REST API, but they
were possible when we supported draft patch sets. Hence this bug affects
mostly old changes. Still even nowadays it can happen that a patch set
is marked as deleted. This happens if the ConsistencyChecker is run to
fix change inconsistencies. If the ConsistencyChecker finds a patch set
for which the commit is not found, the patch set is marked as deleted.
At Google we observed more than 200 exceptions that are caused by this,
just in the last month.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I4cd9f9a1787f41469a6515890b1c919017b24e01
(cherry picked from commit 4b9f792153fbe45e79d66fa8527f13291188886e)
fd798d326 - BaseReceivePack: Add hasReceivedPack method
566a46e9e - Silence API errors for new API introduced in 5.5.2
ee00877a5 - Upgrade wagon-ssh to 3.3.4
22e153177 - Fix NPE in SystemReader in tests
ca800b55c - BaseReceivePack: Fix the format
e102bbed9 - Prepend hostname to subsection used to store file timestamp resolution
838b5a84b - Store filesystem timestamp resolution in extra jgit config
a7412b544 - Update bouncycastle version to 1.64 and Orbit to I20191106190530
ffe74210d - SystemReader: extract updating config and its parents if outdated
Adjust DelegateSystemReader to the updated SystemReader interface.
Change-Id: I4ae1352a8ea736d752ef3dc98f79efb5b792b880
The solution in change I0643af77d was incomplete. The pack size can
be uninitialized in cases other than ref deletion. Just try to get
the size and handle the exception, rather than attempt to cover all
the possible cases.
This reverts commit 90305b4b2aeadb70f2e2437c054fe787b7d1004a.
Bug: Issue 11918
Change-Id: I5b7683664a0ff549cce7c83d5c7a9aa8b351a7cc
It seems that when the receive pack only contains DELETE commands, the
pack size is not set, and this results in IllegalStateException when
calling getPackSize on it. This is then returned to the client console
as an error message, even though the ref was successfully deleted.
Bug: Issue 11918
Change-Id: I0643af77dee9cddd63986702593aed80433015cc
Rather than passing in the change Id, which is a breaking API change,
let the implementation get the change Id from the patch set Id.
This reverts commit 593a5007052c340920d5715e62b4eeec617a6f54.
Bug: Issue 11905
Change-Id: Ieba95cadcf7a4f60664120c59916ebce4a1caca4
JsonElement.toString returns the value wrapped in an extra pair of
quotes, while getAsString does not.
The base64 decoder that we are currently using seems to be tolerant
of this, but others may not be which could cause issues.
Also, we already use getAsString in other places, so changing this
makes it consistent.
Change-Id: I42c71444782aa467321027856b017df62106c38c
