Merge changes I5a30dad3,I1fc60d2a,I3c5e7c15
* changes:
Add REST endpoint for reindexing an index version
Add REST API for getting info about indexes
Optionally allow to reuse existing documents during reindexing
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d9d4643..9ce2eee 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4363,6 +4363,33 @@
+
Defaults to `true`.
+[[log.daysToKeep]]log.timeToKeep::
++
+Time that logs should be kept until they are being deleted. Values should use common
+suffixes to express their setting:
++
+* d, day, days
+* w, week, weeks (`1 week` is treated as `7 days`)
+* mon, month, months (`1 month` is treated as `30 days`)
+* y, year, years (`1 year` is treated as `365 days`)
++
+The minimum granularity is days. Using a smaller time unit will result in deletion of
+all old logs, as if `0d` would have been configured.
++
+Actively used logs will never be deleted. Thus, this feature only works in combination
+with enabled link:#log.rotate[log.rotate]. Log deletion happens at server startup and
+then daily at 11pm (in the server's local time zone).
++
+Depending on the filesystem the following file times will be used, in order of priority:
++
+* Time of file creation
+* Time when the file was last modified
+* Date added to the filename as part of log file rotation. Time will be set to `00:00:00Z`.
++
+If none of the above is available, the log file won't be deleted.
++
+Defaults to `-1`, i.e. being disabled.
+
[[metrics]]
=== Section metrics
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index f059bf9..091d229 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -63,7 +63,8 @@
"_number": 4711,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -126,6 +127,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2
},
{
"id": "demo~master~I09c8041b5867d5b33170316e2abc34b79bbb8501",
@@ -143,6 +145,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2,
"_more_changes": true
}
]
@@ -214,7 +217,8 @@
"labels": {
"Verified": {},
"Code-Review": {}
- }
+ },
+ "current_revision_number": 2
}
],
[],
@@ -437,6 +441,7 @@
"owner": {
"name": "Shawn Pearce"
},
+ "current_revision_number": 1,
"current_revision": "184ebe53805e102605d11f6b143486d15c23a09c",
"revisions": {
"184ebe53805e102605d11f6b143486d15c23a09c": {
@@ -598,7 +603,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -680,7 +686,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
},
"new_change_info": {
"id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940",
@@ -709,7 +716,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
},
}
----
@@ -978,7 +986,8 @@
"message": "Patch Set 1:\n\nThis is the second message.\n\nWith a line break.",
"_revision_number": 1
}
- ]
+ ],
+ "current_revision_number": 2
}
----
@@ -1040,6 +1049,7 @@
"owner": {
"_account_id": 1000000
},
+ "current_revision_number": 1,
"current_revision": "27cc4558b5a3d3387dd11ee2df7a117e7e581822"
}
----
@@ -1268,7 +1278,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -1339,7 +1350,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -1411,6 +1423,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2,
"current_revision": "27cc4558b5a3d3387dd11ee2df7a117e7e581822",
"revisions": {
"27cc4558b5a3d3387dd11ee2df7a117e7e581822": {
@@ -1561,6 +1574,7 @@
"owner": {
"_account_id": 1000000
},
+ "current_revision_number": 2,
"current_revision": "c3b2ba222d42a56e05c90f88d4509a124620517d",
"revisions": {
"c3b2ba222d42a56e05c90f88d4509a124620517d": {
@@ -1638,6 +1652,7 @@
"owner": {
"_account_id": 1000000
},
+ "current_revision_number": 2,
"current_revision": "77eb17a9501a5c21963bc6af56085e60f281acbb",
"revisions": {
"77eb17a9501a5c21963bc6af56085e60f281acbb": {
@@ -1762,7 +1777,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -1841,7 +1857,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -1935,7 +1952,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
},
{
"id": "anyProject~master~1eee2c9d8f352483781e772f35dc586a69ff5646",
@@ -1953,7 +1971,8 @@
"_number": 3966,
"owner": {
"name": "Jane Doe"
- }
+ },
+ "current_revision_number": 2
}
]
----
@@ -2038,7 +2057,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -2191,6 +2211,7 @@
}
]
},
+ "current_revision_number": 1,
"current_revision": "9adb9f4c7b40eeee0646e235de818d09164d7379",
"revisions": {
"9adb9f4c7b40eeee0646e235de818d09164d7379": {
@@ -2288,6 +2309,7 @@
}
]
},
+ "current_revision_number": 1,
"current_revision": "1bd7c12a38854a2c6de426feec28800623f492c4",
"revisions": {
"1bd7c12a38854a2c6de426feec28800623f492c4": {
@@ -2397,6 +2419,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 1,
"current_revision": "184ebe53805e102605d11f6b143486d15c23a09c"
}
----
@@ -2661,6 +2684,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2,
"problems": [
{
"message": "Current patch set 1 not found"
@@ -2713,6 +2737,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2,
"problems": [
{
"message": "Current patch set 2 not found"
@@ -4427,6 +4452,7 @@
}
]
},
+ "current_revision_number": 2,
"current_revision": "674ac754f91e64a0efb8087e59a176484bd534d1",
"revisions": {
"674ac754f91e64a0efb8087e59a176484bd534d1": {
@@ -4818,6 +4844,7 @@
"owner": {
"name": "John Doe"
},
+ "current_revision_number": 2,
"current_revision": "27cc4558b5a3d3387dd11ee2df7a117e7e581822",
"revisions": {
"27cc4558b5a3d3387dd11ee2df7a117e7e581822": {
@@ -4918,7 +4945,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -6491,7 +6519,8 @@
"_number": 3965,
"owner": {
"name": "John Doe"
- }
+ },
+ "current_revision_number": 2
}
----
@@ -7260,6 +7289,8 @@
Messages associated with the change as a list of
link:#change-message-info[ChangeMessageInfo] entities. +
Only set if link:#messages[messages] are requested.
+|`current_revision_number`||The number of the current patch set of this
+change. +
|`current_revision` |optional|
The commit ID of the current patch set of this change. +
Only set if link:#current-revision[the current revision] is requested
@@ -8747,9 +8778,8 @@
action. Not set if false.
|`error` |optional|
Error message for non-200 responses.
-|`change_info` |optional|
-Post-update change information. Only set if `response_format_options` were
-set.
+|`change_info` ||
+Post-update change information.
|============================
[[reviewer-info]]
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index 148a4b5..d8ff80a 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -931,7 +931,7 @@
"state": "SLEEPING",
"start_time": "2014-06-11 12:58:51.508000000",
"delay": 3287966,
- "command": "Log File Compressor"
+ "command": "Log File Manager"
}
]
----
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index a2e2e8f..33a2f34 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -115,6 +115,7 @@
public Collection<ReviewerUpdateInfo> reviewerUpdates;
public Collection<ChangeMessageInfo> messages;
+ public Integer currentRevisionNumber;
public String currentRevision;
public Map<String, RevisionInfo> revisions;
public Boolean _moreChanges;
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index ba88617..fa67034 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -45,7 +45,7 @@
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker;
-import com.google.gerrit.pgm.util.LogFileCompressor.LogFileCompressorModule;
+import com.google.gerrit.pgm.util.LogFileManager.LogFileManagerModule;
import com.google.gerrit.server.DefaultRefLogIdentityProvider;
import com.google.gerrit.server.LibModuleLoader;
import com.google.gerrit.server.LibModuleType;
@@ -302,7 +302,7 @@
private Injector createSysInjector() {
final List<Module> modules = new ArrayList<>();
modules.add(new DropWizardMetricMaker.RestModule());
- modules.add(new LogFileCompressorModule());
+ modules.add(new LogFileManagerModule());
modules.add(new EventBrokerModule());
modules.add(new JdbcAccountPatchReviewStoreModule(config));
modules.add(cfgInjector.getInstance(GitRepositoryManagerModule.class));
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index d213a60..198eeaa 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -52,7 +52,7 @@
import com.google.gerrit.pgm.http.jetty.JettyModule;
import com.google.gerrit.pgm.http.jetty.ProjectQoSFilter.ProjectQoSFilterModule;
import com.google.gerrit.pgm.util.ErrorLogFile;
-import com.google.gerrit.pgm.util.LogFileCompressor.LogFileCompressorModule;
+import com.google.gerrit.pgm.util.LogFileManager.LogFileManagerModule;
import com.google.gerrit.pgm.util.RuntimeShutdown;
import com.google.gerrit.pgm.util.SiteProgram;
import com.google.gerrit.server.DefaultRefLogIdentityProvider;
@@ -450,7 +450,7 @@
final List<Module> modules = new ArrayList<>();
modules.add(NoteDbSchemaVersionCheck.module());
modules.add(new DropWizardMetricMaker.RestModule());
- modules.add(new LogFileCompressorModule());
+ modules.add(new LogFileManagerModule());
// Index module shutdown must happen before work queue shutdown, otherwise
// work queue can get stuck waiting on index futures that will never return.
diff --git a/java/com/google/gerrit/pgm/util/LogFileCompressor.java b/java/com/google/gerrit/pgm/util/LogFileCompressor.java
deleted file mode 100644
index 5e49312..0000000
--- a/java/com/google/gerrit/pgm/util/LogFileCompressor.java
+++ /dev/null
@@ -1,171 +0,0 @@
-// Copyright (C) 2009 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.pgm.util;
-
-import static java.util.concurrent.TimeUnit.HOURS;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
-
-import com.google.common.flogger.FluentLogger;
-import com.google.common.io.ByteStreams;
-import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.lifecycle.LifecycleModule;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.SitePaths;
-import com.google.gerrit.server.git.WorkQueue;
-import com.google.inject.Inject;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
-import java.nio.file.DirectoryStream;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.time.LocalDateTime;
-import java.time.ZoneId;
-import java.time.temporal.ChronoUnit;
-import java.util.concurrent.Future;
-import java.util.zip.GZIPOutputStream;
-import org.eclipse.jgit.lib.Config;
-
-/** Compresses the old error logs. */
-public class LogFileCompressor implements Runnable {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- public static class LogFileCompressorModule extends LifecycleModule {
- @Override
- protected void configure() {
- listener().to(Lifecycle.class);
- }
- }
-
- static class Lifecycle implements LifecycleListener {
- private final WorkQueue queue;
- private final LogFileCompressor compressor;
- private final boolean enabled;
-
- @Inject
- Lifecycle(WorkQueue queue, LogFileCompressor compressor, @GerritServerConfig Config config) {
- this.queue = queue;
- this.compressor = compressor;
- this.enabled = config.getBoolean("log", "compress", true);
- }
-
- @Override
- public void start() {
- if (!enabled) {
- return;
- }
- // compress log once and then schedule compression every day at 11:00pm
- queue.getDefaultQueue().execute(compressor);
- ZoneId zone = ZoneId.systemDefault();
- LocalDateTime now = LocalDateTime.now(zone);
- long milliSecondsUntil11pm =
- now.until(now.withHour(23).withMinute(0).withSecond(0).withNano(0), ChronoUnit.MILLIS);
- @SuppressWarnings("unused")
- Future<?> possiblyIgnoredError =
- queue
- .getDefaultQueue()
- .scheduleAtFixedRate(
- compressor, milliSecondsUntil11pm, HOURS.toMillis(24), MILLISECONDS);
- }
-
- @Override
- public void stop() {}
- }
-
- private final Path logs_dir;
-
- @Inject
- LogFileCompressor(SitePaths site) {
- logs_dir = resolve(site.logs_dir);
- }
-
- private static Path resolve(Path p) {
- try {
- return p.toRealPath().normalize();
- } catch (IOException e) {
- return p.toAbsolutePath().normalize();
- }
- }
-
- @Override
- public void run() {
- try {
- if (!Files.isDirectory(logs_dir)) {
- return;
- }
- try (DirectoryStream<Path> list = Files.newDirectoryStream(logs_dir)) {
- for (Path entry : list) {
- if (!isLive(entry) && !isCompressed(entry) && isLogFile(entry)) {
- compress(entry);
- }
- }
- } catch (IOException e) {
- logger.atSevere().withCause(e).log("Error listing logs to compress in %s", logs_dir);
- }
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Failed to compress log files: %s", e.getMessage());
- }
- }
-
- private boolean isLive(Path entry) {
- String name = entry.getFileName().toString();
- return name.endsWith("_log")
- || name.endsWith(".log")
- || name.endsWith(".run")
- || name.endsWith(".pid")
- || name.endsWith(".json");
- }
-
- private boolean isCompressed(Path entry) {
- String name = entry.getFileName().toString();
- return name.endsWith(".gz") //
- || name.endsWith(".zip") //
- || name.endsWith(".bz2");
- }
-
- private boolean isLogFile(Path entry) {
- return Files.isRegularFile(entry);
- }
-
- private void compress(Path src) {
- Path dst = src.resolveSibling(src.getFileName() + ".gz");
- Path tmp = src.resolveSibling(".tmp." + src.getFileName());
- try {
- try (InputStream in = Files.newInputStream(src);
- OutputStream out = new GZIPOutputStream(Files.newOutputStream(tmp))) {
- ByteStreams.copy(in, out);
- }
- tmp.toFile().setReadOnly();
- try {
- Files.move(tmp, dst);
- } catch (IOException e) {
- throw new IOException("Cannot rename " + tmp + " to " + dst, e);
- }
- Files.delete(src);
- } catch (IOException e) {
- logger.atSevere().withCause(e).log("Cannot compress %s", src);
- try {
- Files.deleteIfExists(tmp);
- } catch (IOException e2) {
- logger.atWarning().withCause(e2).log("Failed to delete temporary log file %s", tmp);
- }
- }
- }
-
- @Override
- public String toString() {
- return "Log File Compressor";
- }
-}
diff --git a/java/com/google/gerrit/pgm/util/LogFileManager.java b/java/com/google/gerrit/pgm/util/LogFileManager.java
new file mode 100644
index 0000000..902f7d64
--- /dev/null
+++ b/java/com/google/gerrit/pgm/util/LogFileManager.java
@@ -0,0 +1,249 @@
+// Copyright (C) 2009 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.pgm.util;
+
+import static java.util.concurrent.TimeUnit.HOURS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.FluentLogger;
+import com.google.common.io.ByteStreams;
+import com.google.gerrit.extensions.events.LifecycleListener;
+import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.server.config.ConfigUtil;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.nio.file.attribute.FileTime;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.temporal.ChronoUnit;
+import java.util.Optional;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.zip.GZIPOutputStream;
+import org.eclipse.jgit.lib.Config;
+
+/** Compresses and eventually deletes the old logs. */
+public class LogFileManager implements Runnable {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final Pattern LOG_FILENAME_PATTERN =
+ Pattern.compile("^.+(?<date>\\d{4}-\\d{2}-\\d{2})(.gz)?");
+ protected final boolean compressionEnabled;
+ private final Duration timeToKeep;
+
+ public static class LogFileManagerModule extends LifecycleModule {
+ @Override
+ protected void configure() {
+ listener().to(Lifecycle.class);
+ }
+ }
+
+ static class Lifecycle implements LifecycleListener {
+ private final WorkQueue queue;
+ private final LogFileManager manager;
+
+ @Inject
+ Lifecycle(WorkQueue queue, LogFileManager manager) {
+ this.queue = queue;
+ this.manager = manager;
+ }
+
+ @Override
+ public void start() {
+ if (!manager.compressionEnabled && manager.timeToKeep.isNegative()) {
+ return;
+ }
+ // compress log once and then schedule compression every day at 11:00pm
+ queue.getDefaultQueue().execute(manager);
+ ZoneId zone = ZoneId.systemDefault();
+ LocalDateTime now = LocalDateTime.now(zone);
+ long milliSecondsUntil11pm =
+ now.until(now.withHour(23).withMinute(0).withSecond(0).withNano(0), ChronoUnit.MILLIS);
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ queue
+ .getDefaultQueue()
+ .scheduleAtFixedRate(
+ manager, milliSecondsUntil11pm, HOURS.toMillis(24), MILLISECONDS);
+ }
+
+ @Override
+ public void stop() {}
+ }
+
+ private final Path logs_dir;
+
+ @Inject
+ LogFileManager(SitePaths site, @GerritServerConfig Config config) {
+ this.logs_dir = resolve(site.logs_dir);
+ this.compressionEnabled = config.getBoolean("log", "compress", true);
+ this.timeToKeep = getTimeToKeep(config);
+ }
+
+ private Duration getTimeToKeep(Config config) {
+ try {
+ return Duration.ofDays(
+ ConfigUtil.getTimeUnit(config, "log", null, "timeToKeep", -1, TimeUnit.DAYS));
+ } catch (IllegalArgumentException e) {
+ logger.atWarning().withCause(e).log(
+ "Illegal duration value for log deletion. Disabling log deletion.");
+ return Duration.ofDays(-1L);
+ }
+ }
+
+ private static Path resolve(Path p) {
+ try {
+ return p.toRealPath().normalize();
+ } catch (IOException e) {
+ return p.toAbsolutePath().normalize();
+ }
+ }
+
+ @Override
+ public void run() {
+ logger.atInfo().log("Starting log file maintenance.");
+ try {
+ if (!Files.isDirectory(logs_dir)) {
+ return;
+ }
+ try (DirectoryStream<Path> list = Files.newDirectoryStream(logs_dir)) {
+ for (Path entry : list) {
+ if (isLive(entry) || !isLogFile(entry)) {
+ continue;
+ }
+ if (!timeToKeep.isNegative() && isExpired(entry)) {
+ if (delete(entry)) {
+ continue;
+ }
+ }
+ if (compressionEnabled && !isCompressed(entry)) {
+ compress(entry);
+ }
+ }
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Error listing logs to compress in %s", logs_dir);
+ }
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log("Failed to process log files: %s", e.getMessage());
+ }
+ logger.atInfo().log("Log file maintenance has finished.");
+ }
+
+ private boolean isLive(Path entry) {
+ String name = entry.getFileName().toString();
+ return name.endsWith("_log")
+ || name.endsWith(".log")
+ || name.endsWith(".run")
+ || name.endsWith(".pid")
+ || name.endsWith(".json");
+ }
+
+ private boolean isCompressed(Path entry) {
+ String name = entry.getFileName().toString();
+ return name.endsWith(".gz") //
+ || name.endsWith(".zip") //
+ || name.endsWith(".bz2");
+ }
+
+ private boolean isLogFile(Path entry) {
+ return Files.isRegularFile(entry);
+ }
+
+ @VisibleForTesting
+ boolean isExpired(Path entry) {
+ try {
+ FileTime creationTime = Files.readAttributes(entry, BasicFileAttributes.class).creationTime();
+
+ if (creationTime.toInstant().equals(Instant.EPOCH)) {
+ Optional<Instant> fileDate = getDateFromFilename(entry);
+ if (fileDate.isPresent()) {
+ return fileDate.get().isBefore(Instant.now().minus(timeToKeep));
+ }
+ return false;
+ }
+
+ return creationTime.toInstant().isBefore(Instant.now().minus(timeToKeep));
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Failed to get creation time of log file %s", entry);
+ }
+ return false;
+ }
+
+ @VisibleForTesting
+ Optional<Instant> getDateFromFilename(Path entry) {
+ Matcher filenameMatcher = LOG_FILENAME_PATTERN.matcher(entry.getFileName().toString());
+ if (filenameMatcher.matches()) {
+ String rotationDate = filenameMatcher.group("date");
+ if (rotationDate != null && !rotationDate.isBlank()) {
+ return Optional.of(Instant.parse(rotationDate + "T00:00:00.00Z"));
+ }
+ }
+ return Optional.empty();
+ }
+
+ private boolean delete(Path entry) {
+ try {
+ Files.deleteIfExists(entry);
+ logger.atInfo().log("Log file %s has been deleted.", entry);
+ return true;
+ } catch (IOException e) {
+ logger.atWarning().withCause(e).log("Failed to delete log file %s", entry);
+ }
+ return false;
+ }
+
+ private void compress(Path src) {
+ Path dst = src.resolveSibling(src.getFileName() + ".gz");
+ Path tmp = src.resolveSibling(".tmp." + src.getFileName());
+ try {
+ try (InputStream in = Files.newInputStream(src);
+ OutputStream out = new GZIPOutputStream(Files.newOutputStream(tmp))) {
+ ByteStreams.copy(in, out);
+ }
+ tmp.toFile().setReadOnly();
+ try {
+ Files.move(tmp, dst);
+ } catch (IOException e) {
+ throw new IOException("Cannot rename " + tmp + " to " + dst, e);
+ }
+ Files.delete(src);
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Cannot compress %s", src);
+ try {
+ Files.deleteIfExists(tmp);
+ } catch (IOException e2) {
+ logger.atWarning().withCause(e2).log("Failed to delete temporary log file %s", tmp);
+ }
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "Log File Manager";
+ }
+}
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 29f5a7c..d08aeb7 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -365,7 +365,8 @@
+ e.getDuplicateKey().get()
+ "\" to account "
+ newId
- + "; external ID already in use.");
+ + "; external ID already in use.",
+ e);
} finally {
// If adding the account failed, it may be that it actually was the
// first account. So we reset the 'check for first account'-guard, as
diff --git a/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbReadStorageModule.java b/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbReadStorageModule.java
index 9253133..c861bc0 100644
--- a/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbReadStorageModule.java
+++ b/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbReadStorageModule.java
@@ -16,6 +16,8 @@
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdNoteDbReadStorageModule;
+import com.google.gerrit.server.mail.send.MessageIdGenerator;
+import com.google.gerrit.server.mail.send.MessageIdGeneratorImpl;
import com.google.inject.AbstractModule;
import com.google.inject.Singleton;
@@ -25,5 +27,6 @@
install(new ExternalIdNoteDbReadStorageModule());
bind(Accounts.class).to(AccountsNoteDbImpl.class).in(Singleton.class);
+ bind(MessageIdGenerator.class).to(MessageIdGeneratorImpl.class);
}
}
diff --git a/java/com/google/gerrit/server/change/ArchiveFormatInternal.java b/java/com/google/gerrit/server/change/ArchiveFormatInternal.java
index 0ed1f11..b5ac87f5 100644
--- a/java/com/google/gerrit/server/change/ArchiveFormatInternal.java
+++ b/java/com/google/gerrit/server/change/ArchiveFormatInternal.java
@@ -63,8 +63,8 @@
return format.suffixes();
}
- public ArchiveOutputStream createArchiveOutputStream(OutputStream o) throws IOException {
- return (ArchiveOutputStream) this.format.createArchiveOutputStream(o);
+ public ArchiveOutputStream<?> createArchiveOutputStream(OutputStream o) throws IOException {
+ return (ArchiveOutputStream<?>) this.format.createArchiveOutputStream(o);
}
public <T extends Closeable> void putEntry(T out, String path, byte[] data) throws IOException {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 3b07829..0483031 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -639,6 +639,7 @@
Change in = cd.change();
out.project = in.getProject().get();
out.branch = in.getDest().shortName();
+ out.currentRevisionNumber = in.currentPatchSetId().get();
out.topic = in.getTopic();
if (!cd.attentionSet().isEmpty()) {
out.removedFromAttentionSet =
diff --git a/java/com/google/gerrit/server/config/CachedPreferences.java b/java/com/google/gerrit/server/config/CachedPreferences.java
index 169d9ec..6e957e6 100644
--- a/java/com/google/gerrit/server/config/CachedPreferences.java
+++ b/java/com/google/gerrit/server/config/CachedPreferences.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.config;
import com.google.auto.value.AutoValue;
-import com.google.common.base.Function;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
@@ -23,6 +22,10 @@
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.proto.Entities.UserPreferences;
import com.google.gerrit.server.cache.proto.Cache.CachedPreferencesProto;
+import com.google.gerrit.server.config.PreferencesParserUtil.DiffPreferencesParser;
+import com.google.gerrit.server.config.PreferencesParserUtil.EditPreferencesParser;
+import com.google.gerrit.server.config.PreferencesParserUtil.GeneralPreferencesParser;
+import com.google.gerrit.server.config.PreferencesParserUtil.PreferencesParser;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -67,38 +70,17 @@
public static GeneralPreferencesInfo general(
Optional<CachedPreferences> defaultPreferences, CachedPreferences userPreferences) {
- return getPreferences(
- defaultPreferences,
- userPreferences,
- PreferencesParserUtil::parseGeneralPreferences,
- p ->
- UserPreferencesConverter.GeneralPreferencesInfoConverter.fromProto(
- p.getGeneralPreferencesInfo()),
- GeneralPreferencesInfo.defaults());
+ return getPreferences(defaultPreferences, userPreferences, GeneralPreferencesParser.Instance);
}
public static DiffPreferencesInfo diff(
Optional<CachedPreferences> defaultPreferences, CachedPreferences userPreferences) {
- return getPreferences(
- defaultPreferences,
- userPreferences,
- PreferencesParserUtil::parseDiffPreferences,
- p ->
- UserPreferencesConverter.DiffPreferencesInfoConverter.fromProto(
- p.getDiffPreferencesInfo()),
- DiffPreferencesInfo.defaults());
+ return getPreferences(defaultPreferences, userPreferences, DiffPreferencesParser.Instance);
}
public static EditPreferencesInfo edit(
Optional<CachedPreferences> defaultPreferences, CachedPreferences userPreferences) {
- return getPreferences(
- defaultPreferences,
- userPreferences,
- PreferencesParserUtil::parseEditPreferences,
- p ->
- UserPreferencesConverter.EditPreferencesInfoConverter.fromProto(
- p.getEditPreferencesInfo()),
- EditPreferencesInfo.defaults());
+ return getPreferences(defaultPreferences, userPreferences, EditPreferencesParser.Instance);
}
public Config asConfig() {
@@ -135,34 +117,26 @@
return cachedPreferences.map(CachedPreferences::asConfig).orElse(null);
}
- @FunctionalInterface
- private interface ComputePreferencesFn<PreferencesT> {
- PreferencesT apply(Config cfg, @Nullable Config defaultCfg, @Nullable PreferencesT input)
- throws ConfigInvalidException;
- }
-
private static <PreferencesT> PreferencesT getPreferences(
Optional<CachedPreferences> defaultPreferences,
CachedPreferences userPreferences,
- ComputePreferencesFn<PreferencesT> computePreferencesFn,
- Function<UserPreferences, PreferencesT> fromUserPreferencesFn,
- PreferencesT javaDefaults) {
+ PreferencesParser<PreferencesT> preferencesParser) {
try {
CachedPreferencesProto userPreferencesProto = userPreferences.config();
switch (userPreferencesProto.getPreferencesCase()) {
case USER_PREFERENCES:
PreferencesT pref =
- fromUserPreferencesFn.apply(userPreferencesProto.getUserPreferences());
- return computePreferencesFn.apply(new Config(), configOrNull(defaultPreferences), pref);
+ preferencesParser.fromUserPreferences(userPreferencesProto.getUserPreferences());
+ return preferencesParser.parse(pref, configOrNull(defaultPreferences));
case LEGACY_GIT_CONFIG:
- return computePreferencesFn.apply(
+ return preferencesParser.parse(
userPreferences.asConfig(), configOrNull(defaultPreferences), null);
case PREFERENCES_NOT_SET:
throw new ConfigInvalidException("Invalid config " + userPreferences);
}
} catch (ConfigInvalidException e) {
- return javaDefaults;
+ return preferencesParser.getJavaDefaults();
}
- return javaDefaults;
+ return preferencesParser.getJavaDefaults();
}
}
diff --git a/java/com/google/gerrit/server/config/ConfigUtil.java b/java/com/google/gerrit/server/config/ConfigUtil.java
index 44bfa48..e76207c 100644
--- a/java/com/google/gerrit/server/config/ConfigUtil.java
+++ b/java/com/google/gerrit/server/config/ConfigUtil.java
@@ -397,6 +397,50 @@
}
/**
+ * Merges config by inspecting Java class attributes, similar to {@link #loadSection}.
+ *
+ * <p>Config values are stored optimized: no default values are stored. The loading is performed
+ * eagerly: all values are set, except default boolean values.
+ *
+ * <p>Fields marked with final or transient modifiers are skipped.
+ *
+ * @param cfg config from which the values are loaded
+ * @param s instance of class in which the values are set
+ * @param defaults instance of class with default values
+ * @return loaded instance
+ */
+ @CanIgnoreReturnValue
+ public static <T> T mergeWithDefaults(T cfg, T s, T defaults) throws ConfigInvalidException {
+ try {
+ for (Field f : s.getClass().getDeclaredFields()) {
+ if (skipField(f)) {
+ continue;
+ }
+ Class<?> t = f.getType();
+ String n = f.getName();
+ f.setAccessible(true);
+
+ Object val = f.get(cfg);
+ if (val == null) {
+ val = f.get(defaults);
+ if (!isString(t) && !isCollectionOrMap(t)) {
+ requireNonNull(val, "Default cannot be null for: " + n);
+ }
+ }
+ if (!isBoolean(t) || (boolean) val) {
+ // To reproduce the same behavior as in the loadSection method above, values are
+ // explicitly set for all types, except the boolean type. For the boolean type, the value
+ // is set only if it is 'true' (so, the false value is omitted in the result object).
+ f.set(s, val);
+ }
+ }
+ } catch (SecurityException | IllegalArgumentException | IllegalAccessException e) {
+ throw new ConfigInvalidException("cannot load values", e);
+ }
+ return s;
+ }
+
+ /**
* Update user config by applying the specified delta
*
* <p>As opposed to {@link com.google.gerrit.server.config.ConfigUtil#storeSection}, this method
diff --git a/java/com/google/gerrit/server/config/PreferencesParserUtil.java b/java/com/google/gerrit/server/config/PreferencesParserUtil.java
index fbdb324..93df926 100644
--- a/java/com/google/gerrit/server/config/PreferencesParserUtil.java
+++ b/java/com/google/gerrit/server/config/PreferencesParserUtil.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.config;
import static com.google.gerrit.server.config.ConfigUtil.loadSection;
+import static com.google.gerrit.server.config.ConfigUtil.mergeWithDefaults;
import static com.google.gerrit.server.config.ConfigUtil.skipField;
import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE;
import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE_COLUMN;
@@ -30,6 +31,7 @@
import com.google.gerrit.extensions.client.EditPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.MenuItem;
+import com.google.gerrit.proto.Entities.UserPreferences;
import com.google.gerrit.server.git.UserConfigSections;
import java.lang.reflect.Field;
import java.util.ArrayList;
@@ -66,13 +68,31 @@
r.my = input.my;
} else {
r.changeTable = parseChangeTableColumns(cfg, defaultCfg);
- r.my = parseMyMenus(cfg, defaultCfg);
+ r.my = parseMyMenus(my(cfg), defaultCfg);
}
return r;
}
/**
* Returns a {@link GeneralPreferencesInfo} that is the result of parsing {@code defaultCfg} for
+ * the server's default configs and {@code cfg} for the user's config.
+ */
+ public static GeneralPreferencesInfo parseGeneralPreferences(
+ GeneralPreferencesInfo cfg, @Nullable Config defaultCfg) throws ConfigInvalidException {
+ GeneralPreferencesInfo r =
+ mergeWithDefaults(
+ cfg,
+ new GeneralPreferencesInfo(),
+ defaultCfg != null
+ ? parseDefaultGeneralPreferences(defaultCfg, null)
+ : GeneralPreferencesInfo.defaults());
+ r.changeTable = cfg.changeTable != null ? cfg.changeTable : Lists.newArrayList();
+ r.my = parseMyMenus(cfg.my, defaultCfg);
+ return r;
+ }
+
+ /**
+ * Returns a {@link GeneralPreferencesInfo} that is the result of parsing {@code defaultCfg} for
* the server's default configs. These configs are then overlaid to inherit values (default ->
* input (if provided).
*/
@@ -110,6 +130,20 @@
/**
* Returns a {@link DiffPreferencesInfo} that is the result of parsing {@code defaultCfg} for the
+ * server's default configs and {@code cfg} for the user's config.
+ */
+ public static DiffPreferencesInfo parseDiffPreferences(
+ DiffPreferencesInfo cfg, @Nullable Config defaultCfg) throws ConfigInvalidException {
+ return mergeWithDefaults(
+ cfg,
+ new DiffPreferencesInfo(),
+ defaultCfg != null
+ ? parseDefaultDiffPreferences(defaultCfg, null)
+ : DiffPreferencesInfo.defaults());
+ }
+
+ /**
+ * Returns a {@link DiffPreferencesInfo} that is the result of parsing {@code defaultCfg} for the
* server's default configs. These configs are then overlaid to inherit values (default -> input
* (if provided).
*/
@@ -147,6 +181,20 @@
/**
* Returns a {@link EditPreferencesInfo} that is the result of parsing {@code defaultCfg} for the
+ * server's default configs and {@code cfg} for the user's config.
+ */
+ public static EditPreferencesInfo parseEditPreferences(
+ EditPreferencesInfo cfg, @Nullable Config defaultCfg) throws ConfigInvalidException {
+ return mergeWithDefaults(
+ cfg,
+ new EditPreferencesInfo(),
+ defaultCfg != null
+ ? parseDefaultEditPreferences(defaultCfg, null)
+ : EditPreferencesInfo.defaults());
+ }
+
+ /**
+ * Returns a {@link EditPreferencesInfo} that is the result of parsing {@code defaultCfg} for the
* server's default configs. These configs are then overlaid to inherit values (default -> input
* (if provided).
*/
@@ -171,11 +219,14 @@
return changeTable;
}
- private static List<MenuItem> parseMyMenus(Config cfg, @Nullable Config defaultCfg) {
- List<MenuItem> my = my(cfg);
- if (my.isEmpty() && defaultCfg != null) {
+ private static List<MenuItem> parseMyMenus(
+ @Nullable List<MenuItem> my, @Nullable Config defaultCfg) {
+ if (defaultCfg != null && (my == null || my.isEmpty())) {
my = my(defaultCfg);
}
+ if (my == null) {
+ my = new ArrayList<>();
+ }
if (my.isEmpty()) {
my.add(new MenuItem("Dashboard", "#/dashboard/self", null));
my.add(new MenuItem("Draft Comments", "#/q/has:draft", null));
@@ -264,4 +315,110 @@
String val = cfg.getString(UserConfigSections.MY, subsection, key);
return !Strings.isNullOrEmpty(val) ? val : defaultValue;
}
+
+ /** Provides methods for parsing user configs */
+ public interface PreferencesParser<T> {
+ T parse(Config cfg, @Nullable Config defaultConfig, @Nullable T input)
+ throws ConfigInvalidException;
+
+ T parse(T cfg, @Nullable Config defaultConfig) throws ConfigInvalidException;
+
+ T fromUserPreferences(UserPreferences userPreferences);
+
+ T getJavaDefaults();
+ }
+
+ /** Provides methods for parsing GeneralPreferencesInfo configs */
+ public static class GeneralPreferencesParser
+ implements PreferencesParser<GeneralPreferencesInfo> {
+ public static GeneralPreferencesParser Instance = new GeneralPreferencesParser();
+
+ private GeneralPreferencesParser() {}
+
+ @Override
+ public GeneralPreferencesInfo parse(
+ Config cfg, @Nullable Config defaultCfg, @Nullable GeneralPreferencesInfo input)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseGeneralPreferences(cfg, defaultCfg, input);
+ }
+
+ @Override
+ public GeneralPreferencesInfo parse(GeneralPreferencesInfo cfg, @Nullable Config defaultCfg)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseGeneralPreferences(cfg, defaultCfg);
+ }
+
+ @Override
+ public GeneralPreferencesInfo fromUserPreferences(UserPreferences p) {
+ return UserPreferencesConverter.GeneralPreferencesInfoConverter.fromProto(
+ p.getGeneralPreferencesInfo());
+ }
+
+ @Override
+ public GeneralPreferencesInfo getJavaDefaults() {
+ return GeneralPreferencesInfo.defaults();
+ }
+ }
+
+ /** Provides methods for parsing EditPreferencesInfo configs */
+ public static class EditPreferencesParser implements PreferencesParser<EditPreferencesInfo> {
+ public static EditPreferencesParser Instance = new EditPreferencesParser();
+
+ private EditPreferencesParser() {}
+
+ @Override
+ public EditPreferencesInfo parse(
+ Config cfg, @Nullable Config defaultCfg, @Nullable EditPreferencesInfo input)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseEditPreferences(cfg, defaultCfg, input);
+ }
+
+ @Override
+ public EditPreferencesInfo parse(EditPreferencesInfo cfg, @Nullable Config defaultCfg)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseEditPreferences(cfg, defaultCfg);
+ }
+
+ @Override
+ public EditPreferencesInfo fromUserPreferences(UserPreferences p) {
+ return UserPreferencesConverter.EditPreferencesInfoConverter.fromProto(
+ p.getEditPreferencesInfo());
+ }
+
+ @Override
+ public EditPreferencesInfo getJavaDefaults() {
+ return EditPreferencesInfo.defaults();
+ }
+ }
+
+ /** Provides methods for parsing DiffPreferencesInfo configs */
+ public static class DiffPreferencesParser implements PreferencesParser<DiffPreferencesInfo> {
+ public static DiffPreferencesParser Instance = new DiffPreferencesParser();
+
+ private DiffPreferencesParser() {}
+
+ @Override
+ public DiffPreferencesInfo parse(
+ Config cfg, @Nullable Config defaultCfg, @Nullable DiffPreferencesInfo input)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseDiffPreferences(cfg, defaultCfg, input);
+ }
+
+ @Override
+ public DiffPreferencesInfo parse(DiffPreferencesInfo cfg, @Nullable Config defaultCfg)
+ throws ConfigInvalidException {
+ return PreferencesParserUtil.parseDiffPreferences(cfg, defaultCfg);
+ }
+
+ @Override
+ public DiffPreferencesInfo fromUserPreferences(UserPreferences p) {
+ return UserPreferencesConverter.DiffPreferencesInfoConverter.fromProto(
+ p.getDiffPreferencesInfo());
+ }
+
+ @Override
+ public DiffPreferencesInfo getJavaDefaults() {
+ return DiffPreferencesInfo.defaults();
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/config/UserPreferencesConverter.java b/java/com/google/gerrit/server/config/UserPreferencesConverter.java
index 4a052d7..7eae7d0 100644
--- a/java/com/google/gerrit/server/config/UserPreferencesConverter.java
+++ b/java/com/google/gerrit/server/config/UserPreferencesConverter.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.EditPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
@@ -180,20 +181,24 @@
MenuItem javaItem) {
UserPreferences.GeneralPreferencesInfo.MenuItem.Builder builder =
UserPreferences.GeneralPreferencesInfo.MenuItem.newBuilder();
- builder = setIfNotNull(builder, builder::setName, javaItem.name);
- builder = setIfNotNull(builder, builder::setUrl, javaItem.url);
- builder = setIfNotNull(builder, builder::setTarget, javaItem.target);
- builder = setIfNotNull(builder, builder::setId, javaItem.id);
+ builder = setIfNotNull(builder, builder::setName, trimSafe(javaItem.name));
+ builder = setIfNotNull(builder, builder::setUrl, trimSafe(javaItem.url));
+ builder = setIfNotNull(builder, builder::setTarget, trimSafe(javaItem.target));
+ builder = setIfNotNull(builder, builder::setId, trimSafe(javaItem.id));
return builder.build();
}
+ private static @Nullable String trimSafe(@Nullable String s) {
+ return s == null ? s : s.trim();
+ }
+
private static MenuItem menuItemFromProto(
UserPreferences.GeneralPreferencesInfo.MenuItem proto) {
return new MenuItem(
- proto.hasName() ? proto.getName() : null,
- proto.hasUrl() ? proto.getUrl() : null,
- proto.hasTarget() ? proto.getTarget() : null,
- proto.hasId() ? proto.getId() : null);
+ proto.hasName() ? proto.getName().trim() : null,
+ proto.hasUrl() ? proto.getUrl().trim() : null,
+ proto.hasTarget() ? proto.getTarget().trim() : null,
+ proto.hasId() ? proto.getId().trim() : null);
}
private GeneralPreferencesInfoConverter() {}
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 7caa988..4399ad4 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -184,6 +184,7 @@
import com.google.gerrit.server.submit.MergeOpRepoManager;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
@@ -367,9 +368,9 @@
private final AccountResolver accountResolver;
private final AllProjectsName allProjectsName;
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final CancellationMetrics cancellationMetrics;
private final ChangeEditUtil editUtil;
- private final ChangeData.Factory changeDataFactory;
private final ChangeIndexer indexer;
private final ChangeInserter.Factory changeInserterFactory;
private final ChangeNotes.Factory notesFactory;
@@ -456,11 +457,11 @@
AccountResolver accountResolver,
AllProjectsName allProjectsName,
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
CancellationMetrics cancellationMetrics,
ProjectConfig.Factory projectConfigFactory,
@GerritServerConfig Config config,
ChangeEditUtil editUtil,
- ChangeData.Factory changeDataFactory,
ChangeIndexer indexer,
ChangeInserter.Factory changeInserterFactory,
ChangeNotes.Factory notesFactory,
@@ -514,6 +515,7 @@
this.accountResolver = accountResolver;
this.allProjectsName = allProjectsName;
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.cancellationMetrics = cancellationMetrics;
this.changeFormatter = changeFormatterProvider.get();
this.changeUtil = changeUtil;
@@ -527,7 +529,6 @@
this.deadlineCheckerFactory = deadlineCheckerFactory;
this.diffOperationsForCommitValidationFactory = diffOperationsForCommitValidationFactory;
this.editUtil = editUtil;
- this.changeDataFactory = changeDataFactory;
this.hashtagsFactory = hashtagsFactory;
this.setTopicFactory = setTopicFactory;
this.indexer = indexer;
@@ -887,7 +888,7 @@
logger.atFine().log("Added %d additional ref updates", added);
SubmissionExecutor submissionExecutor =
- new SubmissionExecutor(changeDataFactory, false, superprojectUpdateSubmissionListeners);
+ new SubmissionExecutor(batchUpdates, false, superprojectUpdateSubmissionListeners);
submissionExecutor.execute(ImmutableList.of(bu));
diff --git a/java/com/google/gerrit/server/mail/send/MessageIdGenerator.java b/java/com/google/gerrit/server/mail/send/MessageIdGenerator.java
index b32c43a..29b914f 100644
--- a/java/com/google/gerrit/server/mail/send/MessageIdGenerator.java
+++ b/java/com/google/gerrit/server/mail/send/MessageIdGenerator.java
@@ -1,58 +1,26 @@
-// Copyright (C) 2020 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
package com.google.gerrit.server.mail.send;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.auto.value.AutoValue;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.mail.MailMessage;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.update.RepoView;
-import com.google.inject.Inject;
-import java.io.IOException;
import java.time.Instant;
-import java.util.Optional;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
-/** A generator class that creates a {@link MessageId} */
-public class MessageIdGenerator {
- private final GitRepositoryManager repositoryManager;
- private final AllUsersName allUsersName;
-
- @Inject
- public MessageIdGenerator(GitRepositoryManager repositoryManager, AllUsersName allUsersName) {
- this.repositoryManager = repositoryManager;
- this.allUsersName = allUsersName;
- }
-
+public interface MessageIdGenerator {
/**
* A unique id used which is a part of the header of all emails sent through by Gerrit. All of the
* emails are sent via {@link OutgoingEmail#send()}.
*/
@AutoValue
- public abstract static class MessageId {
+ abstract class MessageId {
public abstract String id();
+
+ public static MessageId create(String id) {
+ return new AutoValue_MessageIdGenerator_MessageId(id);
+ }
}
/**
@@ -60,43 +28,19 @@
*
* @return MessageId that depends on the patchset.
*/
- public MessageId fromChangeUpdate(RepoView repoView, PatchSet.Id patchsetId) {
- return fromChangeUpdateAndReason(repoView, patchsetId, null);
- }
+ MessageId fromChangeUpdate(RepoView repoView, PatchSet.Id patchsetId);
- public MessageId fromChangeUpdateAndReason(
- RepoView repoView, PatchSet.Id patchsetId, @Nullable String reason) {
- String suffix = (reason != null) ? ("-" + reason) : "";
- String metaRef = patchsetId.changeId().toRefPrefix() + "meta";
- Optional<ObjectId> metaSha1;
- try {
- metaSha1 = repoView.getRef(metaRef);
- } catch (IOException ex) {
- throw new StorageException("unable to extract info for Message-Id", ex);
- }
- return metaSha1
- .map(optional -> new AutoValue_MessageIdGenerator_MessageId(optional.getName() + suffix))
- .orElseThrow(() -> new IllegalStateException(metaRef + " doesn't exist"));
- }
+ MessageId fromChangeUpdateAndReason(
+ RepoView repoView, PatchSet.Id patchsetId, @Nullable String reason);
- public MessageId fromChangeUpdate(Project.NameKey project, PatchSet.Id patchsetId) {
- String metaRef = patchsetId.changeId().toRefPrefix() + "meta";
- Ref ref = getRef(metaRef, project);
- checkState(ref != null, metaRef + " must exist");
- return new AutoValue_MessageIdGenerator_MessageId(ref.getObjectId().getName());
- }
+ MessageId fromChangeUpdate(Project.NameKey project, PatchSet.Id patchsetId);
/**
* Create a {@link MessageId} as a result of an account update
*
* @return {@link MessageId} that depends on the account id.
*/
- public MessageId fromAccountUpdate(Account.Id accountId) {
- String userRef = RefNames.refsUsers(accountId);
- Ref ref = getRef(userRef, allUsersName);
- checkState(ref != null, userRef + " must exist");
- return new AutoValue_MessageIdGenerator_MessageId(ref.getObjectId().getName());
- }
+ MessageId fromAccountUpdate(Account.Id accountId);
/**
* Create a {@link MessageId} from a mail message.
@@ -104,9 +48,7 @@
* @param mailMessage The message that was sent but was rejected.
* @return MessageId that depends on the MailMessage that was rejected.
*/
- public MessageId fromMailMessage(MailMessage mailMessage) {
- return new AutoValue_MessageIdGenerator_MessageId(mailMessage.id() + "-REJECTION");
- }
+ MessageId fromMailMessage(MailMessage mailMessage);
/**
* Create a {@link MessageId} from a reason, Account.Id, and timestamp.
@@ -114,17 +56,5 @@
* @param reason for performing this account update
* @return MessageId that depends on the reason, accountId, and timestamp.
*/
- public MessageId fromReasonAccountIdAndTimestamp(
- String reason, Account.Id accountId, Instant timestamp) {
- return new AutoValue_MessageIdGenerator_MessageId(
- reason + "-" + accountId.toString() + "-" + timestamp.toString());
- }
-
- private Ref getRef(String userRef, Project.NameKey project) {
- try (Repository repository = repositoryManager.openRepository(project)) {
- return repository.getRefDatabase().findRef(userRef);
- } catch (IOException ex) {
- throw new StorageException("unable to extract info for Message-Id", ex);
- }
- }
+ MessageId fromReasonAccountIdAndTimestamp(String reason, Account.Id accountId, Instant timestamp);
}
diff --git a/java/com/google/gerrit/server/mail/send/MessageIdGeneratorImpl.java b/java/com/google/gerrit/server/mail/send/MessageIdGeneratorImpl.java
new file mode 100644
index 0000000..292d042
--- /dev/null
+++ b/java/com/google/gerrit/server/mail/send/MessageIdGeneratorImpl.java
@@ -0,0 +1,103 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.mail.send;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.mail.MailMessage;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.update.RepoView;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.time.Instant;
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+
+/** A generator class that creates a {@link MessageId} */
+public class MessageIdGeneratorImpl implements MessageIdGenerator {
+ private final GitRepositoryManager repositoryManager;
+ private final AllUsersName allUsersName;
+
+ @Inject
+ public MessageIdGeneratorImpl(GitRepositoryManager repositoryManager, AllUsersName allUsersName) {
+ this.repositoryManager = repositoryManager;
+ this.allUsersName = allUsersName;
+ }
+
+ @Override
+ public MessageId fromChangeUpdate(RepoView repoView, PatchSet.Id patchsetId) {
+ return fromChangeUpdateAndReason(repoView, patchsetId, null);
+ }
+
+ @Override
+ public MessageId fromChangeUpdateAndReason(
+ RepoView repoView, PatchSet.Id patchsetId, @Nullable String reason) {
+ String suffix = (reason != null) ? ("-" + reason) : "";
+ String metaRef = patchsetId.changeId().toRefPrefix() + "meta";
+ Optional<ObjectId> metaSha1;
+ try {
+ metaSha1 = repoView.getRef(metaRef);
+ } catch (IOException ex) {
+ throw new StorageException("unable to extract info for Message-Id", ex);
+ }
+ return metaSha1
+ .map(optional -> MessageId.create(optional.getName() + suffix))
+ .orElseThrow(() -> new IllegalStateException(metaRef + " doesn't exist"));
+ }
+
+ @Override
+ public MessageId fromChangeUpdate(Project.NameKey project, PatchSet.Id patchsetId) {
+ String metaRef = patchsetId.changeId().toRefPrefix() + "meta";
+ Ref ref = getRef(metaRef, project);
+ checkState(ref != null, metaRef + " must exist");
+ return MessageId.create(ref.getObjectId().getName());
+ }
+
+ @Override
+ public MessageId fromAccountUpdate(Account.Id accountId) {
+ String userRef = RefNames.refsUsers(accountId);
+ Ref ref = getRef(userRef, allUsersName);
+ checkState(ref != null, userRef + " must exist");
+ return MessageId.create(ref.getObjectId().getName());
+ }
+
+ @Override
+ public MessageId fromMailMessage(MailMessage mailMessage) {
+ return MessageId.create(mailMessage.id() + "-REJECTION");
+ }
+
+ @Override
+ public MessageId fromReasonAccountIdAndTimestamp(
+ String reason, Account.Id accountId, Instant timestamp) {
+ return MessageId.create(reason + "-" + accountId.toString() + "-" + timestamp.toString());
+ }
+
+ private Ref getRef(String userRef, Project.NameKey project) {
+ try (Repository repository = repositoryManager.openRepository(project)) {
+ return repository.getRefDatabase().findRef(userRef);
+ } catch (IOException ex) {
+ throw new StorageException("unable to extract info for Message-Id", ex);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
index 6fe464d..79038af 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.restapi.change.CommentJson;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
@@ -64,6 +65,7 @@
@Singleton
public class DeleteDraftCommentsUtil {
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final Supplier<ChangeQueryBuilder> queryBuilderSupplier;
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeData.Factory changeDataFactory;
@@ -77,6 +79,7 @@
@Inject
public DeleteDraftCommentsUtil(
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
Provider<ChangeQueryBuilder> queryBuilderProvider,
Provider<InternalChangeQuery> queryProvider,
ChangeData.Factory changeDataFactory,
@@ -86,6 +89,7 @@
DraftCommentsReader draftCommentsReader,
PatchSetUtil psUtil) {
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.queryBuilderSupplier = Suppliers.memoize(queryBuilderProvider::get);
this.queryProvider = queryProvider;
this.changeDataFactory = changeDataFactory;
@@ -122,7 +126,7 @@
// were,
// all updates from this operation only happen in All-Users and thus are fully atomic, so
// allowing partial failure would have little value.
- BatchUpdate.execute(changeDataFactory, updates.values(), ImmutableList.of(), false);
+ batchUpdates.execute(updates.values(), ImmutableList.of(), false);
}
return ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList());
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 709d6a7..3c30b84 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -94,6 +94,8 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdates;
+import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -143,7 +145,7 @@
public static final String ERROR_WIP_READY_MUTUALLY_EXCLUSIVE =
"work_in_progress and ready are mutually exclusive";
- private final BatchUpdate.Factory updateFactory;
+ private final RetryHelper retryHelper;
private final PostReviewOp.Factory postReviewOpFactory;
private final ChangeResource.Factory changeResourceFactory;
private final AccountCache accountCache;
@@ -167,7 +169,7 @@
@Inject
PostReview(
- BatchUpdate.Factory updateFactory,
+ RetryHelper retryHelper,
PostReviewOp.Factory postReviewOpFactory,
ChangeResource.Factory changeResourceFactory,
AccountCache accountCache,
@@ -186,7 +188,7 @@
ReviewerAdded reviewerAdded,
ChangeJson.Factory changeJsonFactory,
CommentsValidator commentsValidator) {
- this.updateFactory = updateFactory;
+ this.retryHelper = retryHelper;
this.postReviewOpFactory = postReviewOpFactory;
this.changeResourceFactory = changeResourceFactory;
this.accountCache = accountCache;
@@ -290,100 +292,56 @@
}
output.labels = input.labels;
- BatchUpdate.Result batchUpdateResult;
-
- // Notify based on ReviewInput, ignoring the notify settings from any ReviewerInputs.
- NotifyResolver.Result notify = notifyResolver.resolve(input.notify, input.notifyDetails);
- try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
- try (BatchUpdate bu =
- updateFactory.create(revision.getChange().getProject(), revision.getUser(), ts)) {
- bu.setNotify(notify);
-
- Account account = revision.getUser().asIdentifiedUser().getAccount();
- boolean ccOrReviewer = false;
- if (input.labels != null && !input.labels.isEmpty()) {
- ccOrReviewer = input.labels.values().stream().anyMatch(v -> v != 0);
- if (ccOrReviewer) {
- logger.atFine().log(
- "calling user is cc/reviewer on the change due to voting on a label");
- }
- }
-
- if (!ccOrReviewer) {
- // Check if user was already CCed or reviewing prior to this review.
- ReviewerSet currentReviewers =
- approvalsUtil.getReviewers(revision.getChangeResource().getNotes());
- ccOrReviewer = currentReviewers.all().contains(account.id());
- if (ccOrReviewer) {
- logger.atFine().log("calling user is already cc/reviewer on the change");
- }
- }
-
- // Apply reviewer changes first. Revision emails should be sent to the
- // updated set of reviewers. Also keep track of whether the user added
- // themselves as a reviewer or to the CC list.
- logger.atFine().log("adding reviewer additions");
- for (ReviewerModification reviewerResult : reviewerResults) {
- reviewerResult.op.suppressEmail(); // Send a single batch email below.
- reviewerResult.op.suppressEvent(); // Send events below, if possible as batch.
- bu.addOp(revision.getChange().getId(), reviewerResult.op);
- if (!ccOrReviewer && reviewerResult.reviewers.contains(account)) {
- logger.atFine().log("calling user is explicitly added as reviewer or CC");
- ccOrReviewer = true;
- }
- }
-
- if (!ccOrReviewer) {
- // User posting this review isn't currently in the reviewer or CC list,
- // isn't being explicitly added, and isn't voting on any label.
- // Automatically CC them on this change so they receive replies.
- logger.atFine().log("CCing calling user");
- ReviewerModification selfAddition =
- reviewerModifier.ccCurrentUser(revision.getUser(), revision);
- selfAddition.op.suppressEmail();
- selfAddition.op.suppressEvent();
- bu.addOp(revision.getChange().getId(), selfAddition.op);
- }
-
- // Add WorkInProgressOp if requested.
- if ((input.ready || input.workInProgress)
- && didWorkInProgressChange(revision.getChange().isWorkInProgress(), input)) {
- if (input.ready && input.workInProgress) {
- output.error = ERROR_WIP_READY_MUTUALLY_EXCLUSIVE;
- return Response.withStatusCode(SC_BAD_REQUEST, output);
- }
-
- revision
- .getChangeResource()
- .permissions()
- .check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
-
- if (input.ready) {
- output.ready = true;
- }
-
- logger.atFine().log("setting work-in-progress to %s", input.workInProgress);
- WorkInProgressOp wipOp =
- workInProgressOpFactory.create(input.workInProgress, new WorkInProgressOp.Input());
- wipOp.suppressEmail();
- bu.addOp(revision.getChange().getId(), wipOp);
- }
-
- // Add the review ops.
- logger.atFine().log("posting review");
- PostReviewOp postReviewOp =
- postReviewOpFactory.create(
- projectState, revision.getPatchSet().id(), input, revision.getAccountId());
- bu.addOp(revision.getChange().getId(), postReviewOp);
-
- // Adjust the attention set based on the input
- replyAttentionSetUpdates.updateAttentionSetOnPostReview(
- bu, postReviewOp, revision.getNotes(), input, revision.getUser());
-
- batchUpdateResult = bu.execute();
+ Account account = revision.getUser().asIdentifiedUser().getAccount();
+ boolean ccOrReviewer = false;
+ if (input.labels != null && !input.labels.isEmpty()) {
+ ccOrReviewer = input.labels.values().stream().anyMatch(v -> v != 0);
+ if (ccOrReviewer) {
+ logger.atFine().log("calling user is cc/reviewer on the change due to voting on a label");
}
}
+ if (!ccOrReviewer) {
+ // Check if user was already CCed or reviewing prior to this review.
+ ReviewerSet currentReviewers =
+ approvalsUtil.getReviewers(revision.getChangeResource().getNotes());
+ ccOrReviewer = currentReviewers.all().contains(account.id());
+ if (ccOrReviewer) {
+ logger.atFine().log("calling user is already cc/reviewer on the change");
+ }
+ }
+
+ for (ReviewerModification reviewerResult : reviewerResults) {
+ reviewerResult.op.suppressEmail(); // Send a single batch email below.
+ reviewerResult.op.suppressEvent(); // Send events below, if possible as batch.
+ if (!ccOrReviewer && reviewerResult.reviewers.contains(account)) {
+ logger.atFine().log("calling user is explicitly added as reviewer or CC");
+ ccOrReviewer = true;
+ }
+ }
+
+ // Notify based on ReviewInput, ignoring the notify settings from any ReviewerInputs.
+ NotifyResolver.Result notify = notifyResolver.resolve(input.notify, input.notifyDetails);
+
+ if ((input.ready || input.workInProgress)
+ && didWorkInProgressChange(revision.getChange().isWorkInProgress(), input)) {
+ if (input.ready && input.workInProgress) {
+ output.error = ERROR_WIP_READY_MUTUALLY_EXCLUSIVE;
+ return Response.withStatusCode(SC_BAD_REQUEST, output);
+ }
+
+ revision
+ .getChangeResource()
+ .permissions()
+ .check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
+
+ if (input.ready) {
+ output.ready = true;
+ }
+ }
+
+ BatchUpdates.Result batchUpdateResult =
+ runBatchUpdate(projectState, revision, input, ts, notify, reviewerResults, ccOrReviewer);
ChangeData cd =
batchUpdateResult.getChangeData(revision.getProject(), revision.getChange().getId());
for (ReviewerModification reviewerResult : reviewerResults) {
@@ -397,11 +355,83 @@
if (input.responseFormatOptions != null) {
output.changeInfo = changeJsonFactory.create(input.responseFormatOptions).format(cd);
+ } else {
+ output.changeInfo = changeJsonFactory.noOptions().format(cd);
}
return Response.ok(output);
}
+ private BatchUpdates.Result runBatchUpdate(
+ ProjectState projectState,
+ RevisionResource revision,
+ ReviewInput input,
+ Instant ts,
+ NotifyResolver.Result notify,
+ List<ReviewerModification> reviewerResults,
+ boolean ccOrReviewer)
+ throws UpdateException, RestApiException {
+ return retryHelper
+ .changeUpdate(
+ "batchUpdate",
+ updateFactory -> {
+ try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
+ try (BatchUpdate bu =
+ updateFactory.create(
+ revision.getChange().getProject(), revision.getUser(), ts)) {
+ bu.setNotify(notify);
+
+ // Apply reviewer changes first. Revision emails should be sent to the
+ // updated set of reviewers. Also keep track of whether the user added
+ // themselves as a reviewer or to the CC list.
+ logger.atFine().log("adding reviewer additions");
+ reviewerResults.forEach(
+ reviewerResult -> bu.addOp(revision.getChange().getId(), reviewerResult.op));
+
+ if (!ccOrReviewer) {
+ // User posting this review isn't currently in the reviewer or CC list,
+ // isn't being explicitly added, and isn't voting on any label.
+ // Automatically CC them on this change so they receive replies.
+ logger.atFine().log("CCing calling user");
+ ReviewerModification selfAddition =
+ reviewerModifier.ccCurrentUser(revision.getUser(), revision);
+ selfAddition.op.suppressEmail();
+ selfAddition.op.suppressEvent();
+ bu.addOp(revision.getChange().getId(), selfAddition.op);
+ }
+
+ // Add WorkInProgressOp if requested.
+ if ((input.ready || input.workInProgress)
+ && didWorkInProgressChange(revision.getChange().isWorkInProgress(), input)) {
+ logger.atFine().log("setting work-in-progress to %s", input.workInProgress);
+ WorkInProgressOp wipOp =
+ workInProgressOpFactory.create(
+ input.workInProgress, new WorkInProgressOp.Input());
+ wipOp.suppressEmail();
+ bu.addOp(revision.getChange().getId(), wipOp);
+ }
+
+ // Add the review ops.
+ logger.atFine().log("posting review");
+ PostReviewOp postReviewOp =
+ postReviewOpFactory.create(
+ projectState,
+ revision.getPatchSet().id(),
+ input,
+ revision.getAccountId());
+ bu.addOp(revision.getChange().getId(), postReviewOp);
+
+ // Adjust the attention set based on the input
+ replyAttentionSetUpdates.updateAttentionSetOnPostReview(
+ bu, postReviewOp, revision.getNotes(), input, revision.getUser());
+
+ return bu.execute();
+ }
+ }
+ })
+ .call();
+ }
+
private boolean didWorkInProgressChange(boolean currentWorkInProgress, ReviewInput input) {
return input.ready == currentWorkInProgress || input.workInProgress != currentWorkInProgress;
}
diff --git a/java/com/google/gerrit/server/restapi/change/PutMessage.java b/java/com/google/gerrit/server/restapi/change/PutMessage.java
index ddec6bd..91f1575 100644
--- a/java/com/google/gerrit/server/restapi/change/PutMessage.java
+++ b/java/com/google/gerrit/server/restapi/change/PutMessage.java
@@ -174,7 +174,7 @@
String commitMessage,
Instant timestamp,
String committerEmail)
- throws IOException, BadRequestException, ResourceConflictException {
+ throws IOException, BadRequestException {
CommitBuilder builder = new CommitBuilder();
builder.setTreeId(basePatchSetCommit.getTree());
builder.setParentIds(basePatchSetCommit.getParents());
diff --git a/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java b/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java
deleted file mode 100644
index fe28d6f..0000000
--- a/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java
+++ /dev/null
@@ -1,36 +0,0 @@
-// Copyright (C) 2014 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.schema;
-
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.server.git.meta.VersionedConfigFile;
-
-/** Preferences for user accounts during schema migrations. */
-class VersionedAccountPreferences extends VersionedConfigFile {
- static final String PREFERENCES = "preferences.config";
-
- static VersionedAccountPreferences forUser(Account.Id id) {
- return new VersionedAccountPreferences(RefNames.refsUsers(id));
- }
-
- static VersionedAccountPreferences forDefault() {
- return new VersionedAccountPreferences(RefNames.REFS_USERS_DEFAULT);
- }
-
- protected VersionedAccountPreferences(String ref) {
- super(ref, PREFERENCES, "Updated preferences\n");
- }
-}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index bb1f25f..3fa4157 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -94,6 +94,7 @@
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.SubmissionExecutor;
@@ -277,6 +278,7 @@
private final ChangeMessagesUtil cmUtil;
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final InternalUser.Factory internalUserFactory;
private final MergeSuperSet mergeSuperSet;
private final MergeValidators.Factory mergeValidatorsFactory;
@@ -316,6 +318,7 @@
MergeOp(
ChangeMessagesUtil cmUtil,
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
InternalUser.Factory internalUserFactory,
MergeSuperSet mergeSuperSet,
MergeValidators.Factory mergeValidatorsFactory,
@@ -337,6 +340,7 @@
@GerritServerConfig Config config) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.internalUserFactory = internalUserFactory;
this.mergeSuperSet = mergeSuperSet;
this.mergeValidatorsFactory = mergeValidatorsFactory;
@@ -558,8 +562,7 @@
}
SubmissionExecutor submissionExecutor =
- new SubmissionExecutor(
- changeDataFactory, dryrun, superprojectUpdateSubmissionListeners);
+ new SubmissionExecutor(batchUpdates, dryrun, superprojectUpdateSubmissionListeners);
RetryTracker retryTracker = new RetryTracker();
@SuppressWarnings("unused")
var unused =
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java
index 02afedb..2402357 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -24,9 +24,8 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
-import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.inject.Inject;
@@ -40,16 +39,16 @@
@Singleton
public static class Factory {
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final SubscriptionGraph.Factory subscriptionGraphFactory;
private final SubmoduleCommits.Factory submoduleCommitsFactory;
@Inject
Factory(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
SubscriptionGraph.Factory subscriptionGraphFactory,
SubmoduleCommits.Factory submoduleCommitsFactory) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.subscriptionGraphFactory = subscriptionGraphFactory;
this.submoduleCommitsFactory = submoduleCommitsFactory;
}
@@ -58,7 +57,7 @@
Map<BranchNameKey, ReceiveCommand> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleConflictException {
return new SubmoduleOp(
- changeDataFactory,
+ batchUpdates,
updatedBranches,
orm,
subscriptionGraphFactory.compute(updatedBranches.keySet(), orm),
@@ -66,7 +65,7 @@
}
}
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final Map<BranchNameKey, ReceiveCommand> updatedBranches;
private final MergeOpRepoManager orm;
private final SubscriptionGraph subscriptionGraph;
@@ -74,12 +73,12 @@
private final UpdateOrderCalculator updateOrderCalculator;
private SubmoduleOp(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
Map<BranchNameKey, ReceiveCommand> updatedBranches,
MergeOpRepoManager orm,
SubscriptionGraph subscriptionGraph,
SubmoduleCommits submoduleCommits) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.updatedBranches = updatedBranches;
this.orm = orm;
this.subscriptionGraph = subscriptionGraph;
@@ -115,8 +114,7 @@
}
}
try (RefUpdateContext ctx = RefUpdateContext.open(UPDATE_SUPERPROJECT)) {
- BatchUpdate.execute(
- changeDataFactory,
+ batchUpdates.execute(
orm.batchUpdates(superProjects, /* refLogMessage= */ "merged"),
ImmutableList.of(),
dryrun);
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 0e29a4e..813246c 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
import static com.google.common.flogger.LazyArgs.lazy;
import static com.google.gerrit.common.UsedAt.Project.GOOGLE;
import static java.util.Comparator.comparing;
@@ -35,7 +34,6 @@
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
-import com.google.common.collect.Multiset;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
@@ -51,9 +49,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
@@ -73,12 +68,7 @@
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.LimitExceededException;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
-import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.NoSuchRefException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -91,10 +81,8 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
import java.util.Optional;
import java.util.TreeMap;
-import java.util.function.Function;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectInserter;
@@ -141,151 +129,6 @@
BatchUpdate create(Project.NameKey project, CurrentUser user, Instant when);
}
- public static class Result {
- private final ChangeData.Factory changeDataFactory;
- private final Map<Change.Id, ChangeData> changeDatas;
-
- private Result(ChangeData.Factory changeDataFactory) {
- this(changeDataFactory, new HashMap<>());
- }
-
- private Result(ChangeData.Factory changeDataFactory, Map<Change.Id, ChangeData> changeDatas) {
- this.changeDataFactory = changeDataFactory;
- this.changeDatas = changeDatas;
- }
-
- /**
- * Returns the updated {@link ChangeData} for the given project and change ID.
- *
- * <p>If the requested {@link ChangeData} was already loaded after the {@link BatchUpdate} has
- * been executed the cached {@link ChangeData} instance is returned, otherwise the requested
- * {@link ChangeData} is loaded and put into the cache.
- */
- public ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId) {
- return changeDatas.computeIfAbsent(
- changeId, id -> changeDataFactory.create(projectName, changeId));
- }
- }
-
- @CanIgnoreReturnValue
- public static Result execute(
- ChangeData.Factory changeDataFactory,
- Collection<BatchUpdate> updates,
- ImmutableList<BatchUpdateListener> listeners,
- boolean dryrun)
- throws UpdateException, RestApiException {
- requireNonNull(listeners);
- if (updates.isEmpty()) {
- return new Result(changeDataFactory);
- }
-
- checkDifferentProject(updates);
-
- try {
- List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>();
- List<ChangesHandle> changesHandles = new ArrayList<>(updates.size());
- try {
- for (BatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- notifyAfterUpdateRepo(listeners);
- for (BatchUpdate u : updates) {
- changesHandles.add(u.executeChangeOps(listeners, dryrun));
- }
- for (ChangesHandle h : changesHandles) {
- h.execute();
- if (h.requiresReindex()) {
- indexFutures.addAll(h.startIndexFutures());
- }
- }
- notifyAfterUpdateRefs(listeners);
- notifyAfterUpdateChanges(listeners);
- } finally {
- for (ChangesHandle h : changesHandles) {
- h.close();
- }
- }
-
- Map<Change.Id, ChangeData> changeDatas =
- Futures.allAsList(indexFutures).get().stream()
- // filter out null values that were returned for change deletions
- .filter(Objects::nonNull)
- .collect(toMap(cd -> cd.change().getId(), Function.identity()));
-
- // Fire ref update events only after all mutations are finished, since callers may assume a
- // patch set ref being created means the change was created, or a branch advancing meaning
- // some changes were closed.
- updates.forEach(BatchUpdate::fireRefChangeEvents);
-
- if (!dryrun) {
- for (BatchUpdate u : updates) {
- u.executePostOps(changeDatas);
- }
- }
-
- return new Result(changeDataFactory, changeDatas);
- } catch (Exception e) {
- wrapAndThrowException(e);
- return new Result(changeDataFactory);
- }
- }
-
- private static void notifyAfterUpdateRepo(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateRepos();
- }
- }
-
- private static void notifyAfterUpdateRefs(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateRefs();
- }
- }
-
- private static void notifyAfterUpdateChanges(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateChanges();
- }
- }
-
- private static void checkDifferentProject(Collection<BatchUpdate> updates) {
- Multiset<Project.NameKey> projectCounts =
- updates.stream().map(u -> u.project).collect(toImmutableMultiset());
- checkArgument(
- projectCounts.entrySet().size() == updates.size(),
- "updates must all be for different projects, got: %s",
- projectCounts);
- }
-
- private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
- // Convert common non-REST exception types with user-visible messages to corresponding REST
- // exception types.
- if (e instanceof InvalidChangeOperationException || e instanceof LimitExceededException) {
- throw new ResourceConflictException(e.getMessage(), e);
- } else if (e instanceof NoSuchChangeException
- || e instanceof NoSuchRefException
- || e instanceof NoSuchProjectException) {
- throw new ResourceNotFoundException(e.getMessage(), e);
- } else if (e instanceof CommentsRejectedException) {
- // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
- // status code and it's isolated in monitoring.
- throw new BadRequestException(e.getMessage(), e);
- }
-
- Throwables.throwIfUnchecked(e);
-
- // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
- // ResourceConflictException to indicate an atomic update failure.
- Throwables.throwIfInstanceOf(e, UpdateException.class);
- Throwables.throwIfInstanceOf(e, RestApiException.class);
-
- // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message.
- throw new UpdateException(e);
- }
-
class ContextImpl implements Context {
private final CurrentUser contextUser;
@@ -441,6 +284,7 @@
DELETED
}
+ private final BatchUpdates batchUpdates;
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final ChangeData.Factory changeDataFactory;
@@ -477,6 +321,7 @@
@Inject
BatchUpdate(
+ BatchUpdates batchUpdates,
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
AccountCache accountCache,
@@ -493,6 +338,7 @@
@Assisted CurrentUser user,
@Assisted Instant when) {
this.gerritConfig = gerritConfig;
+ this.batchUpdates = batchUpdates;
this.repoManager = repoManager;
this.accountCache = accountCache;
this.changeDataFactory = changeDataFactory;
@@ -517,13 +363,14 @@
}
@CanIgnoreReturnValue
- public Result execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
- return execute(changeDataFactory, ImmutableList.of(this), ImmutableList.of(listener), false);
+ public BatchUpdates.Result execute(BatchUpdateListener listener)
+ throws UpdateException, RestApiException {
+ return batchUpdates.execute(ImmutableList.of(this), ImmutableList.of(listener), false);
}
@CanIgnoreReturnValue
- public Result execute() throws UpdateException, RestApiException {
- return execute(changeDataFactory, ImmutableList.of(this), ImmutableList.of(), false);
+ public BatchUpdates.Result execute() throws UpdateException, RestApiException {
+ return batchUpdates.execute(ImmutableList.of(this), ImmutableList.of(), false);
}
public boolean isExecuted() {
@@ -676,7 +523,7 @@
return this;
}
- private void executeUpdateRepo() throws UpdateException, RestApiException {
+ void executeUpdateRepo() throws UpdateException, RestApiException {
try {
logDebug("Executing updateRepo on %d ops", ops.size());
for (Map.Entry<Change.Id, OpData<BatchUpdateOp>> e : ops.entries()) {
@@ -718,7 +565,7 @@
&& gerritConfig.getBoolean("index", "indexChangesAsync", false);
}
- private void fireRefChangeEvents() {
+ void fireRefChangeEvents() {
batchRefUpdate.forEach(
(projectName, bru) -> gitRefUpdated.fire(projectName, bru, getAccount().orElse(null)));
}
@@ -735,7 +582,7 @@
}
}
- private class ChangesHandle implements AutoCloseable {
+ class ChangesHandle implements AutoCloseable {
private final NoteDbUpdateManager manager;
private final boolean dryrun;
private final Map<Change.Id, ChangeResult> results;
@@ -820,7 +667,7 @@
}
}
- private ChangesHandle executeChangeOps(
+ ChangesHandle executeChangeOps(
ImmutableList<BatchUpdateListener> batchUpdateListeners, boolean dryrun) throws Exception {
logDebug("Executing change ops");
initRepository();
@@ -861,9 +708,11 @@
ctx.distinctUpdates.values().forEach(changeUpdates::add);
ctx = newChangeContext(opData.user(), id);
}
+ Class<? extends BatchUpdateOp> opClass = opData.op().getClass();
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer(
- opData.getClass().getSimpleName() + "#updateChange",
+ (opClass.isAnonymousClass() ? opClass.getName() : opClass.getSimpleName())
+ + "#updateChange",
Metadata.builder().projectName(project.get()).changeId(id.get()).build())) {
dirty |= opData.op().updateChange(ctx);
deleted |= ctx.deleted;
@@ -948,7 +797,7 @@
return new ChangeContextImpl(contextUser, notes);
}
- private void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
+ void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
for (OpData<BatchUpdateOp> opData : ops.values()) {
PostUpdateContextImpl ctx = new PostUpdateContextImpl(opData.user(), changeDatas);
try (TraceContext.TraceTimer ignored =
diff --git a/java/com/google/gerrit/server/update/BatchUpdates.java b/java/com/google/gerrit/server/update/BatchUpdates.java
new file mode 100644
index 0000000..2f9ef84
--- /dev/null
+++ b/java/com/google/gerrit/server/update/BatchUpdates.java
@@ -0,0 +1,199 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.update;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toMap;
+
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Multiset;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.notedb.LimitExceededException;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.NoSuchRefException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.update.BatchUpdate.ChangesHandle;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
+
+@Singleton
+public class BatchUpdates {
+ public class Result {
+ private final Map<Change.Id, ChangeData> changeDatas;
+
+ private Result() {
+ this(new HashMap<>());
+ }
+
+ private Result(Map<Change.Id, ChangeData> changeDatas) {
+ this.changeDatas = changeDatas;
+ }
+
+ /**
+ * Returns the updated {@link ChangeData} for the given project and change ID.
+ *
+ * <p>If the requested {@link ChangeData} was already loaded after the {@link BatchUpdate} has
+ * been executed the cached {@link ChangeData} instance is returned, otherwise the requested
+ * {@link ChangeData} is loaded and put into the cache.
+ */
+ public ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId) {
+ return changeDatas.computeIfAbsent(
+ changeId, id -> changeDataFactory.create(projectName, changeId));
+ }
+ }
+
+ private final ChangeData.Factory changeDataFactory;
+
+ @Inject
+ BatchUpdates(ChangeData.Factory changeDataFactory) {
+ this.changeDataFactory = changeDataFactory;
+ }
+
+ @CanIgnoreReturnValue
+ public Result execute(
+ Collection<BatchUpdate> updates, ImmutableList<BatchUpdateListener> listeners, boolean dryrun)
+ throws UpdateException, RestApiException {
+ requireNonNull(listeners);
+ if (updates.isEmpty()) {
+ return new Result();
+ }
+
+ checkDifferentProject(updates);
+
+ try {
+ List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>();
+ List<ChangesHandle> changesHandles = new ArrayList<>(updates.size());
+ try {
+ for (BatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ notifyAfterUpdateRepo(listeners);
+ for (BatchUpdate u : updates) {
+ changesHandles.add(u.executeChangeOps(listeners, dryrun));
+ }
+ for (ChangesHandle h : changesHandles) {
+ h.execute();
+ if (h.requiresReindex()) {
+ indexFutures.addAll(h.startIndexFutures());
+ }
+ }
+ notifyAfterUpdateRefs(listeners);
+ notifyAfterUpdateChanges(listeners);
+ } finally {
+ for (ChangesHandle h : changesHandles) {
+ h.close();
+ }
+ }
+
+ Map<Change.Id, ChangeData> changeDatas =
+ Futures.allAsList(indexFutures).get().stream()
+ // filter out null values that were returned for change deletions
+ .filter(Objects::nonNull)
+ .collect(toMap(cd -> cd.change().getId(), Function.identity()));
+
+ // Fire ref update events only after all mutations are finished, since callers may assume a
+ // patch set ref being created means the change was created, or a branch advancing meaning
+ // some changes were closed.
+ updates.forEach(BatchUpdate::fireRefChangeEvents);
+
+ if (!dryrun) {
+ for (BatchUpdate u : updates) {
+ u.executePostOps(changeDatas);
+ }
+ }
+
+ return new Result(changeDatas);
+ } catch (Exception e) {
+ wrapAndThrowException(e);
+ return new Result();
+ }
+ }
+
+ private static void notifyAfterUpdateRepo(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateRepos();
+ }
+ }
+
+ private static void notifyAfterUpdateRefs(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateRefs();
+ }
+ }
+
+ private static void notifyAfterUpdateChanges(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateChanges();
+ }
+ }
+
+ private static void checkDifferentProject(Collection<BatchUpdate> updates) {
+ Multiset<Project.NameKey> projectCounts =
+ updates.stream().map(u -> u.getProject()).collect(toImmutableMultiset());
+ checkArgument(
+ projectCounts.entrySet().size() == updates.size(),
+ "updates must all be for different projects, got: %s",
+ projectCounts);
+ }
+
+ private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
+ // Convert common non-REST exception types with user-visible messages to corresponding REST
+ // exception types.
+ if (e instanceof InvalidChangeOperationException || e instanceof LimitExceededException) {
+ throw new ResourceConflictException(e.getMessage(), e);
+ } else if (e instanceof NoSuchChangeException
+ || e instanceof NoSuchRefException
+ || e instanceof NoSuchProjectException) {
+ throw new ResourceNotFoundException(e.getMessage(), e);
+ } else if (e instanceof CommentsRejectedException) {
+ // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
+ // status code and it's isolated in monitoring.
+ throw new BadRequestException(e.getMessage(), e);
+ }
+
+ Throwables.throwIfUnchecked(e);
+
+ // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
+ // ResourceConflictException to indicate an atomic update failure.
+ Throwables.throwIfInstanceOf(e, UpdateException.class);
+ Throwables.throwIfInstanceOf(e, RestApiException.class);
+
+ // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message.
+ throw new UpdateException(e);
+ }
+}
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index d9af527..c5621ed 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -479,6 +479,12 @@
String actionName = opts.actionName().orElse("N/A");
String cause = formatCause(t);
+ // Do not retry if retrying was already done and failed.
+ if (Throwables.getCausalChain(t).stream()
+ .anyMatch(RetryException.class::isInstance)) {
+ return false;
+ }
+
// exceptionPredicate checks for temporary errors for which the operation should be
// retried (e.g. LockFailure). The retry has good chances to succeed.
if (exceptionPredicate.test(t)) {
@@ -596,6 +602,9 @@
actionType,
opts.actionName().orElse("N/A"),
listener.getOriginalCause().map(this::formatCause).orElse("_unknown"));
+
+ // Re-throw the RetryException so that retrying is not re-attempted on an outer level.
+ throw e;
}
if (e.getCause() != null) {
Throwables.throwIfUnchecked(e.getCause());
diff --git a/java/com/google/gerrit/server/update/SubmissionExecutor.java b/java/com/google/gerrit/server/update/SubmissionExecutor.java
index bab6ddd..762de57 100644
--- a/java/com/google/gerrit/server/update/SubmissionExecutor.java
+++ b/java/com/google/gerrit/server/update/SubmissionExecutor.java
@@ -16,23 +16,22 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.submit.MergeOpRepoManager;
import java.util.Collection;
import java.util.Optional;
import java.util.stream.Collectors;
public class SubmissionExecutor {
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final ImmutableList<SubmissionListener> submissionListeners;
private final boolean dryrun;
private ImmutableList<BatchUpdateListener> additionalListeners = ImmutableList.of();
public SubmissionExecutor(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
boolean dryrun,
ImmutableList<SubmissionListener> submissionListeners) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.dryrun = dryrun;
this.submissionListeners = submissionListeners;
if (dryrun) {
@@ -63,7 +62,7 @@
.map(Optional::get)
.collect(Collectors.toList()))
.build();
- BatchUpdate.execute(changeDataFactory, updates, listeners, dryrun);
+ batchUpdates.execute(updates, listeners, dryrun);
}
/**
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 65954e4..8626a32 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -52,6 +52,7 @@
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
+import com.github.rholder.retry.RetryException;
import com.github.rholder.retry.StopStrategies;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
@@ -2281,9 +2282,12 @@
assertThat(accountInfo.status).isNull();
assertThat(accountInfo.name).isNotEqualTo(fullName);
- assertThrows(
- LockFailureException.class,
- () -> update.update("Set Full Name", admin.id(), u -> u.setFullName(fullName)));
+ StorageException exception =
+ assertThrows(
+ StorageException.class,
+ () -> update.update("Set Full Name", admin.id(), u -> u.setFullName(fullName)));
+ assertThat(exception).hasCauseThat().isInstanceOf(RetryException.class);
+ assertThat(exception.getCause()).hasCauseThat().isInstanceOf(LockFailureException.class);
assertThat(bgCounter.get()).isEqualTo(status.size());
Account updatedAccount = accounts.get(admin.id()).get().account();
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java
index 3ead608..b4c4595 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java
@@ -20,6 +20,7 @@
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.extensions.events.AccountActivationListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -49,16 +50,17 @@
@Override
protected void configure() {
DynamicSet.bind(binder(), AccountActivationValidationListener.class).to(Validator.class);
+ bind(AccountListenersITValidator.class).to(Validator.class);
DynamicSet.bind(binder(), AccountActivationListener.class).to(Listener.class);
}
}
- Validator validator;
+ AccountListenersITValidator validator;
Listener listener;
@Before
public void setUp() {
- validator = plugin.getSysInjector().getInstance(Validator.class);
+ validator = plugin.getSysInjector().getInstance(AccountListenersITValidator.class);
listener = plugin.getSysInjector().getInstance(Listener.class);
}
@@ -128,8 +130,21 @@
listener.assertNoMoreEvents();
}
+ @UsedAt(UsedAt.Project.GOOGLE)
+ protected interface AccountListenersITValidator extends AccountActivationValidationListener {
+ void failActivationValidations();
+
+ void failDeactivationValidations();
+
+ void assertNoMoreEvents();
+
+ void assertActivationValidation(int id);
+
+ void assertDeactivationValidation(int id);
+ }
+
@Singleton
- public static class Validator implements AccountActivationValidationListener {
+ public static final class Validator implements AccountListenersITValidator {
private Integer lastIdActivationValidation;
private Integer lastIdDeactivationValidation;
private boolean failActivationValidations;
@@ -153,25 +168,30 @@
}
}
+ @Override
public void failActivationValidations() {
failActivationValidations = true;
}
+ @Override
public void failDeactivationValidations() {
failDeactivationValidations = true;
}
- private void assertNoMoreEvents() {
+ @Override
+ public void assertNoMoreEvents() {
assertThat(lastIdActivationValidation).isNull();
assertThat(lastIdDeactivationValidation).isNull();
}
- private void assertActivationValidation(int id) {
+ @Override
+ public void assertActivationValidation(int id) {
assertThat(lastIdActivationValidation).isEqualTo(id);
lastIdActivationValidation = null;
}
- private void assertDeactivationValidation(int id) {
+ @Override
+ public void assertDeactivationValidation(int id) {
assertThat(lastIdDeactivationValidation).isEqualTo(id);
lastIdDeactivationValidation = null;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
index 7449a5c..efc7e0f 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.server.IdentifiedUser;
@@ -40,6 +41,7 @@
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.AuthResult;
import com.google.gerrit.server.account.SetInactiveFlag;
+import com.google.gerrit.server.account.externalids.DuplicateExternalIdKeyException;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdFactory;
import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
@@ -51,11 +53,9 @@
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.inject.Inject;
import com.google.inject.util.Providers;
-import java.io.IOException;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.api.Git;
-import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@@ -593,7 +593,7 @@
AccountException thrown =
assertThrows(AccountException.class, () -> accountManager.authenticate(whoOAuth));
- assertThat(thrown).hasMessageThat().contains("Cannot assign external ID \"username:foo\" to");
+ assertThat(thrown).hasCauseThat().isInstanceOf(DuplicateExternalIdKeyException.class);
}
@Test
@@ -647,9 +647,7 @@
AccountException thrown =
assertThrows(AccountException.class, () -> accountManager.authenticate(whoOAuth));
- assertThat(thrown)
- .hasMessageThat()
- .contains("Cannot assign external ID \"username:foo\" to account");
+ assertThat(thrown).hasCauseThat().isInstanceOf(DuplicateExternalIdKeyException.class);
}
@Test
@@ -798,29 +796,22 @@
"Create Test Account",
accountId,
u -> u.addExternalId(externalIdFactory.create(mailExtIdKey, accountId)));
-
accountManager.link(accountId, authRequestFactory.createForEmail(email1));
+ int initialCommits = countExternalIdsCommits();
- int initialCommits;
- try (Repository allUsersRepo = repoManager.openRepository(allUsers);
- Git git = new Git(allUsersRepo)) {
- initialCommits = getCommitsInExternalIds(git, allUsersRepo);
+ accountManager.updateLink(accountId, authRequestFactory.createForEmail(email2));
- accountManager.updateLink(accountId, authRequestFactory.createForEmail(email2));
- }
- // Reopen the repo again - this is required for git.log() operations (otherwise, git.log()
- // returns unmodified history on google internal infra).
- try (Repository allUsersRepo = repoManager.openRepository(allUsers);
- Git git = new Git(allUsersRepo)) {
- int afterUpdateCommits = getCommitsInExternalIds(git, allUsersRepo);
- assertThat(afterUpdateCommits).isEqualTo(initialCommits + 1);
- }
+ int afterUpdateCommits = countExternalIdsCommits();
+ assertThat(afterUpdateCommits).isEqualTo(initialCommits + 1);
}
- private static int getCommitsInExternalIds(Git git, Repository allUsersRepo)
- throws GitAPIException, IOException {
- ObjectId refsMetaExternalIdsHead = allUsersRepo.exactRef(REFS_EXTERNAL_IDS).getObjectId();
- return Iterables.size(git.log().add(refsMetaExternalIdsHead).call());
+ @UsedAt(UsedAt.Project.GOOGLE)
+ protected int countExternalIdsCommits() throws Exception {
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ Git git = new Git(allUsersRepo)) {
+ ObjectId refsMetaExternalIdsHead = allUsersRepo.exactRef(REFS_EXTERNAL_IDS).getObjectId();
+ return Iterables.size(git.log().add(refsMetaExternalIdsHead).call());
+ }
}
private void assertNoSuchExternalIds(ExternalId.Key... extIdKeys) throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/MessageIdGeneratorIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/MessageIdGeneratorIT.java
index 0309646..984b32d 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/MessageIdGeneratorIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/MessageIdGeneratorIT.java
@@ -18,6 +18,8 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.UsedAt;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.RefNames;
@@ -34,12 +36,8 @@
@Test
public void fromAccountUpdate() throws Exception {
- try (Repository repo = repoManager.openRepository(allUsers)) {
- String messageId = messageIdGenerator.fromAccountUpdate(admin.id()).id();
- String sha1 =
- repo.getRefDatabase().findRef(RefNames.refsUsers(admin.id())).getObjectId().getName();
- assertThat(sha1).isEqualTo(messageId);
- }
+ String messageId = messageIdGenerator.fromAccountUpdate(admin.id()).id();
+ validateAccountUpdateMessageId(messageId, admin.id());
}
@Test
@@ -78,4 +76,13 @@
messageIdGenerator.fromReasonAccountIdAndTimestamp(reason, admin.id(), timestamp).id())
.isEqualTo(reason + "-" + admin.id().toString() + "-" + timestamp.toString());
}
+
+ @UsedAt(UsedAt.Project.GOOGLE)
+ protected void validateAccountUpdateMessageId(String messageId, Account.Id id) throws Exception {
+ try (Repository repo = repoManager.openRepository(allUsers)) {
+ String sha1 =
+ repo.getRefDatabase().findRef(RefNames.refsUsers(admin.id())).getObjectId().getName();
+ assertThat(sha1).isEqualTo(messageId);
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 8d30d45..ab74a4a 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -268,6 +268,7 @@
assertThat(c.changeId).isEqualTo(r.getChangeId());
assertThat(c.created).isEqualTo(c.updated);
assertThat(c._number).isEqualTo(r.getChange().getId().get());
+ assertThat(c.currentRevisionNumber).isEqualTo(r.getPatchSetId().get());
assertThat(c.owner._accountId).isEqualTo(admin.id().get());
assertThat(c.owner.name).isNull();
@@ -513,7 +514,7 @@
.reviewer("byemail3@example.com", CC, false)
.reviewer("byemail4@example.com", CC, false);
ReviewResult result = gApi.changes().id(changeId).current().review(in);
- assertThat(result.changeInfo).isNull();
+ assertThat(result.changeInfo).isNotNull();
assertThat(result.reviewers).isNotEmpty();
ChangeInfo info = gApi.changes().id(changeId).get();
Function<Collection<AccountInfo>, Collection<String>> toEmails =
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 314dc44..4d8566d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -24,6 +24,7 @@
import static java.util.stream.Collectors.toList;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.any;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -64,6 +65,7 @@
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
@@ -154,6 +156,10 @@
@Override
public void configure() {
CommentValidator mockCommentValidator = mock(CommentValidator.class);
+
+ // by default return no validation errors
+ when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
+
bind(CommentValidator.class)
.annotatedWith(Exports.named(mockCommentValidator.getClass()))
.toInstance(mockCommentValidator);
@@ -759,6 +765,26 @@
}
@Test
+ public void currentRevisionNumberIsSetOnReturnedChangeInfo() throws Exception {
+ PushOneCommit.Result r = createChange();
+ ChangeInfo changeInfo =
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.dislike()).changeInfo;
+ assertThat(changeInfo.currentRevisionNumber).isEqualTo(1);
+ amendChange(r.getChangeId());
+ changeInfo =
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.recommend()).changeInfo;
+ assertThat(changeInfo.currentRevisionNumber).isEqualTo(2);
+
+ // Check that the current revision number is also returned when list changes options are
+ // requested.
+ ReviewInput reviewInput = ReviewInput.approve();
+ reviewInput.responseFormatOptions =
+ ImmutableList.copyOf(EnumSet.allOf(ListChangesOption.class));
+ changeInfo = gApi.changes().id(r.getChangeId()).current().review(reviewInput).changeInfo;
+ assertThat(changeInfo.currentRevisionNumber).isEqualTo(2);
+ }
+
+ @Test
public void submitRulesAreInvokedOnlyOnce() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/ConfigRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/ConfigRestApiBindingsIT.java
index 576a921..13382ef 100644
--- a/javatests/com/google/gerrit/acceptance/rest/binding/ConfigRestApiBindingsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/ConfigRestApiBindingsIT.java
@@ -110,7 +110,7 @@
Optional<String> id =
result.stream()
- .filter(t -> "Log File Compressor".equals(t.command))
+ .filter(t -> "Log File Manager".equals(t.command))
.map(t -> t.id)
.findFirst();
assertThat(id).isPresent();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 9723df1..b9c07a2 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -37,6 +37,7 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import com.github.rholder.retry.RetryException;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
@@ -1190,7 +1191,11 @@
testMetricMaker.reset();
Throwable thrown = assertThrows(StorageException.class, () -> submit(id, input));
- assertThat(thrown.getCause()).hasMessageThat().contains("missing from ChangeSet[][]");
+ assertThat(thrown.getCause()).hasMessageThat().contains("Computing mergeSuperset has failed");
+ assertThat(thrown.getCause()).hasCauseThat().isInstanceOf(RetryException.class);
+ assertThat(thrown.getCause().getCause().getCause())
+ .hasMessageThat()
+ .contains("missing from ChangeSet[][]");
// We retried more than once before giving up
assertThat(
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/GetTaskIT.java b/javatests/com/google/gerrit/acceptance/rest/config/GetTaskIT.java
index a9e3cf6..9ed6d15 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/GetTaskIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/GetTaskIT.java
@@ -33,7 +33,7 @@
TaskInfo info = newGson().fromJson(r.getReader(), new TypeToken<TaskInfo>() {}.getType());
assertThat(info.id).isNotNull();
Long.parseLong(info.id, 16);
- assertThat(info.command).isEqualTo("Log File Compressor");
+ assertThat(info.command).isEqualTo("Log File Manager");
assertThat(info.startTime).isNotNull();
}
@@ -49,7 +49,7 @@
newGson().fromJson(r.getReader(), new TypeToken<List<TaskInfo>>() {}.getType());
r.consume();
for (TaskInfo info : result) {
- if ("Log File Compressor".equals(info.command)) {
+ if ("Log File Manager".equals(info.command)) {
return info.id;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/KillTaskIT.java b/javatests/com/google/gerrit/acceptance/rest/config/KillTaskIT.java
index 2aa350e..ab3689b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/KillTaskIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/KillTaskIT.java
@@ -36,7 +36,7 @@
Optional<String> id =
result.stream()
- .filter(t -> "Log File Compressor".equals(t.command))
+ .filter(t -> "Log File Manager".equals(t.command))
.map(t -> t.id)
.findFirst();
assertThat(id).isPresent();
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ListTasksIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ListTasksIT.java
index 674ca79..cad0875 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/ListTasksIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/ListTasksIT.java
@@ -34,7 +34,7 @@
assertThat(result).isNotEmpty();
boolean foundLogFileCompressorTask = false;
for (TaskInfo info : result) {
- if ("Log File Compressor".equals(info.command)) {
+ if ("Log File Manager".equals(info.command)) {
foundLogFileCompressorTask = true;
}
assertThat(info.id).isNotNull();
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
index e62cb2b..809cee9 100644
--- a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
@@ -150,7 +150,7 @@
@Override
public void configure() {
// Forwarder.delegate is empty on start to protect test listener from non test tasks
- // (such as the "Log File Compressor") interference
+ // (such as the "Log File Manager") interference
forwarder = new ForwardingListener(); // Only gets bound once for all tests
bind(TaskListener.class).annotatedWith(Exports.named("listener")).toInstance(forwarder);
}
@@ -161,7 +161,7 @@
public void setupExecutorAndForwarder() throws InterruptedException {
executor = workQueue.createQueue(1, "TaskListeners");
- // "Log File Compressor"s are likely running and will interfere with tests
+ // "Log File Manager"s are likely running and will interfere with tests
while (0 != workQueue.getTasks().size()) {
for (Task<?> t : workQueue.getTasks()) {
@SuppressWarnings("unused")
diff --git a/javatests/com/google/gerrit/integration/git/UploadArchiveIT.java b/javatests/com/google/gerrit/integration/git/UploadArchiveIT.java
index b2be85f..573c050 100644
--- a/javatests/com/google/gerrit/integration/git/UploadArchiveIT.java
+++ b/javatests/com/google/gerrit/integration/git/UploadArchiveIT.java
@@ -110,12 +110,12 @@
outputPath);
try (InputStream fi = Files.newInputStream(outputPath);
InputStream bi = new BufferedInputStream(fi);
- ArchiveInputStream archive = archiveStreamForFormat(bi, format)) {
+ ArchiveInputStream<?> archive = archiveStreamForFormat(bi, format)) {
assertEntries(archive);
}
}
- private ArchiveInputStream archiveStreamForFormat(InputStream bi, String format)
+ private ArchiveInputStream<?> archiveStreamForFormat(InputStream bi, String format)
throws IOException {
switch (format) {
case "zip":
diff --git a/javatests/com/google/gerrit/pgm/BUILD b/javatests/com/google/gerrit/pgm/BUILD
index 0fe4fad..43b86cb 100644
--- a/javatests/com/google/gerrit/pgm/BUILD
+++ b/javatests/com/google/gerrit/pgm/BUILD
@@ -7,6 +7,7 @@
deps = [
"//java/com/google/gerrit/pgm/http/jetty",
"//java/com/google/gerrit/pgm/init/api",
+ "//java/com/google/gerrit/pgm/util",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/securestore/testing",
"//lib:guava",
diff --git a/javatests/com/google/gerrit/pgm/util/LogFileManagerTest.java b/javatests/com/google/gerrit/pgm/util/LogFileManagerTest.java
new file mode 100644
index 0000000..b3f59cc
--- /dev/null
+++ b/javatests/com/google/gerrit/pgm/util/LogFileManagerTest.java
@@ -0,0 +1,58 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.pgm.util;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.server.config.SitePaths;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.List;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Test;
+
+public class LogFileManagerTest {
+
+ @Test
+ public void testLogFilePattern() throws Exception {
+ List<String> filenamesWithDate =
+ List.of(
+ "error_log.2024-01-01",
+ "error_log.2024-01-01.gz",
+ "error_log.json.2024-01-01",
+ "error_log.json.2024-01-01.gz",
+ "sshd_log.2024-01-01",
+ "httpd_log.2024-01-01");
+
+ List<String> filenamesWithoutDate =
+ List.of(
+ "error_log",
+ "error_log.gz",
+ "error_log.json",
+ "error_log.json.gz",
+ "sshd_log",
+ "httpd_log");
+
+ LogFileManager manager = new LogFileManager(new SitePaths(Path.of("/gerrit")), new Config());
+ Instant expected = Instant.parse("2024-01-01T00:00:00.00Z");
+ for (String filename : filenamesWithDate) {
+ assertThat(manager.getDateFromFilename(Path.of(filename)).get()).isEqualTo(expected);
+ }
+
+ for (String filename : filenamesWithoutDate) {
+ assertThat(manager.getDateFromFilename(Path.of(filename)).isEmpty()).isTrue();
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/server/config/CachedPreferencesTest.java b/javatests/com/google/gerrit/server/config/CachedPreferencesTest.java
index 772f4b8..b149d09 100644
--- a/javatests/com/google/gerrit/server/config/CachedPreferencesTest.java
+++ b/javatests/com/google/gerrit/server/config/CachedPreferencesTest.java
@@ -130,6 +130,27 @@
}
@Test
+ public void userPreferencesProto_falseValueReturnsAsNull() throws Exception {
+ UserPreferences originalProto =
+ UserPreferences.newBuilder()
+ .setEditPreferencesInfo(
+ UserPreferences.EditPreferencesInfo.newBuilder()
+ .setTabSize(17)
+ .setHideTopMenu(false)
+ .setHideLineNumbers(false)
+ .setAutoCloseBrackets(true))
+ .build();
+
+ CachedPreferences pref = CachedPreferences.fromUserPreferencesProto(originalProto);
+ EditPreferencesInfo edit = CachedPreferences.edit(Optional.empty(), pref);
+
+ assertThat(edit.tabSize).isEqualTo(17);
+ assertThat(edit.hideTopMenu).isNull();
+ assertThat(edit.hideLineNumbers).isNull();
+ assertThat(edit.autoCloseBrackets).isTrue();
+ }
+
+ @Test
public void defaultPreferences_acceptingGitConfig() throws Exception {
Config cfg = new Config();
cfg.fromText("[general]\n\tchangesPerPage = 19");
diff --git a/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java b/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java
index c6ca3e4..8cfcdbd 100644
--- a/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java
+++ b/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java
@@ -118,6 +118,51 @@
}
@Test
+ public void generalPreferencesInfo_toProtoTrimsMyMenuSpaces() {
+ GeneralPreferencesInfo info = new GeneralPreferencesInfo();
+ info.my =
+ ImmutableList.of(
+ new com.google.gerrit.extensions.client.MenuItem(
+ " name1 ", " url1 ", " target1 ", " id1 "),
+ new com.google.gerrit.extensions.client.MenuItem(null, " url2 ", null, null));
+ UserPreferences.GeneralPreferencesInfo resProto = GeneralPreferencesInfoConverter.toProto(info);
+ assertThat(resProto)
+ .isEqualTo(
+ UserPreferences.GeneralPreferencesInfo.newBuilder()
+ .addAllMyMenuItems(
+ ImmutableList.of(
+ MenuItem.newBuilder()
+ .setUrl("url1")
+ .setName("name1")
+ .setTarget("target1")
+ .setId("id1")
+ .build(),
+ MenuItem.newBuilder().setUrl("url2").build()))
+ .build());
+ }
+
+ @Test
+ public void generalPreferencesInfo_fromProtoTrimsMyMenuSpaces() {
+ UserPreferences.GeneralPreferencesInfo originalProto =
+ UserPreferences.GeneralPreferencesInfo.newBuilder()
+ .addAllMyMenuItems(
+ ImmutableList.of(
+ MenuItem.newBuilder()
+ .setName(" name1 ")
+ .setUrl(" url1 ")
+ .setTarget(" target1 ")
+ .setId(" id1 ")
+ .build(),
+ MenuItem.newBuilder().setUrl(" url2 ").build()))
+ .build();
+ GeneralPreferencesInfo info = GeneralPreferencesInfoConverter.fromProto(originalProto);
+ assertThat(info.my)
+ .containsExactly(
+ new com.google.gerrit.extensions.client.MenuItem("name1", "url1", "target1", "id1"),
+ new com.google.gerrit.extensions.client.MenuItem(null, "url2", null, null));
+ }
+
+ @Test
public void generalPreferencesInfo_emptyJavaToProto() {
GeneralPreferencesInfo info = new GeneralPreferencesInfo();
UserPreferences.GeneralPreferencesInfo res = GeneralPreferencesInfoConverter.toProto(info);
diff --git a/javatests/com/google/gerrit/server/notedb/IntBlobTest.java b/javatests/com/google/gerrit/server/notedb/IntBlobTest.java
index 7ed2391..f335201 100644
--- a/javatests/com/google/gerrit/server/notedb/IntBlobTest.java
+++ b/javatests/com/google/gerrit/server/notedb/IntBlobTest.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
-import static com.google.gerrit.truth.OptionalSubject.assertThat;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
diff --git a/javatests/com/google/gerrit/server/update/BUILD b/javatests/com/google/gerrit/server/update/BUILD
index 345681d..a602ee9 100644
--- a/javatests/com/google/gerrit/server/update/BUILD
+++ b/javatests/com/google/gerrit/server/update/BUILD
@@ -12,11 +12,13 @@
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/entities",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/git",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//java/com/google/gerrit/testing:test-ref-update-context",
"//lib:guava",
+ "//lib:guava-retrying",
"//lib:jgit",
"//lib:jgit-junit",
"//lib/guice",
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index f7a2afa..79a688c 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -22,6 +22,9 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import com.github.rholder.retry.Attempt;
+import com.github.rholder.retry.RetryException;
+import com.github.rholder.retry.RetryListener;
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -37,7 +40,10 @@
import com.google.gerrit.extensions.events.AttentionSetListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.git.LockFailureException;
+import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.Sequences;
@@ -53,6 +59,7 @@
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.DiffSummary;
import com.google.gerrit.server.patch.DiffSummaryKey;
+import com.google.gerrit.server.update.RetryableAction.ActionType;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.InMemoryTestEnvironment;
@@ -61,12 +68,17 @@
import com.google.inject.name.Named;
import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
@@ -107,6 +119,8 @@
@Inject private IdentifiedUser.GenericFactory userFactory;
@Inject private InternalUser.Factory internalUserFactory;
@Inject private AbandonOp.Factory abandonOpFactory;
+ @Inject @GerritPersonIdent private PersonIdent serverIdent;
+ @Inject private RetryHelper retryHelper;
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@@ -566,6 +580,245 @@
assertThat(metaCommit.getParent(0)).isEqualTo(oldHead);
}
+ @Test
+ public void lockFailureOnConcurrentUpdate() throws Exception {
+ Change.Id changeId = createChange();
+ ObjectId metaId = getMetaId(changeId);
+
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+
+ AtomicBoolean doneBackgroundUpdate = new AtomicBoolean(false);
+
+ // Create a listener that updates the change meta ref concurrently on the first attempt.
+ BatchUpdateListener listener =
+ new BatchUpdateListener() {
+ @Override
+ public BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+ try (RevWalk rw = new RevWalk(repo.getRepository())) {
+ RevCommit old = rw.parseCommit(metaId);
+ RevCommit commit =
+ repo.commit()
+ .parent(old)
+ .author(serverIdent)
+ .committer(serverIdent)
+ .setTopLevelTree(old.getTree())
+ .message("Concurrent Update\n\nPatch-Set: 1")
+ .create();
+ RefUpdate ru = repo.getRepository().updateRef(RefNames.changeMetaRef(changeId));
+ ru.setExpectedOldObjectId(metaId);
+ ru.setNewObjectId(commit);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ doneBackgroundUpdate.set(true);
+ } catch (Exception e) {
+ // Ignore. If an exception happens doneBackgroundUpdate is false and we fail later
+ // when doneBackgroundUpdate is checked.
+ }
+ return bru;
+ }
+ };
+
+ // Do a batch update, expect that it fails with LOCK_FAILURE due to the concurrent update.
+ assertThat(doneBackgroundUpdate.get()).isFalse();
+ UpdateException exception =
+ assertThrows(
+ UpdateException.class,
+ () -> {
+ try (BatchUpdate bu =
+ batchUpdateFactory.create(project, user.get(), TimeUtil.now())) {
+ bu.addOp(changeId, abandonOpFactory.create(null, "abandon"));
+ bu.execute(listener);
+ }
+ });
+ assertThat(exception).hasCauseThat().isInstanceOf(LockFailureException.class);
+ assertThat(doneBackgroundUpdate.get()).isTrue();
+
+ // Check that the change was not updated.
+ notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+ }
+
+ @Test
+ public void useRetryHelperToRetryOnLockFailure() throws Exception {
+ Change.Id changeId = createChange();
+ ObjectId metaId = getMetaId(changeId);
+
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+
+ AtomicBoolean doneBackgroundUpdate = new AtomicBoolean(false);
+
+ // Create a listener that updates the change meta ref concurrently on the first attempt.
+ BatchUpdateListener listener =
+ new BatchUpdateListener() {
+ @Override
+ public BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+ if (!doneBackgroundUpdate.getAndSet(true)) {
+ try (RevWalk rw = new RevWalk(repo.getRepository())) {
+ RevCommit old = rw.parseCommit(metaId);
+ RevCommit commit =
+ repo.commit()
+ .parent(old)
+ .author(serverIdent)
+ .committer(serverIdent)
+ .setTopLevelTree(old.getTree())
+ .message("Concurrent Update\n\nPatch-Set: 1")
+ .create();
+ RefUpdate ru = repo.getRepository().updateRef(RefNames.changeMetaRef(changeId));
+ ru.setExpectedOldObjectId(metaId);
+ ru.setNewObjectId(commit);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ } catch (Exception e) {
+ // Ignore. If an exception happens doneBackgroundUpdate is false and we fail later
+ // when doneBackgroundUpdate is checked.
+ }
+ }
+ return bru;
+ }
+ };
+
+ // Do a batch update, expect that it succeeds due to retrying despite the LOCK_FAILURE on the
+ // first attempt.
+ assertThat(doneBackgroundUpdate.get()).isFalse();
+
+ @SuppressWarnings("unused")
+ var unused =
+ retryHelper
+ .changeUpdate(
+ "batchUpdate",
+ updateFactory -> {
+ try (BatchUpdate bu = updateFactory.create(project, user.get(), TimeUtil.now())) {
+ bu.addOp(changeId, abandonOpFactory.create(null, "abandon"));
+ bu.execute(listener);
+ }
+ return null;
+ })
+ .call();
+
+ // Check that the concurrent update was done.
+ assertThat(doneBackgroundUpdate.get()).isTrue();
+
+ // Check that the BatchUpdate updated the change.
+ notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.ABANDONED);
+ }
+
+ @Test
+ public void noRetryingOnOuterLevelIfRetryingWasAlreadyDoneOnInnerLevel() throws Exception {
+ Change.Id changeId = createChange();
+
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+
+ AtomicBoolean backgroundFailure = new AtomicBoolean(false);
+
+ // Create a listener that updates the change meta ref concurrently on all attempts.
+ BatchUpdateListener listener =
+ new BatchUpdateListener() {
+ @Override
+ public BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+ try (RevWalk rw = new RevWalk(repo.getRepository())) {
+ String changeMetaRef = RefNames.changeMetaRef(changeId);
+ ObjectId metaId = repo.getRepository().exactRef(changeMetaRef).getObjectId();
+ RevCommit old = rw.parseCommit(metaId);
+ RevCommit commit =
+ repo.commit()
+ .parent(old)
+ .author(serverIdent)
+ .committer(serverIdent)
+ .setTopLevelTree(old.getTree())
+ .message("Concurrent Update\n\nPatch-Set: 1")
+ .create();
+ RefUpdate ru = repo.getRepository().updateRef(changeMetaRef);
+ ru.setExpectedOldObjectId(metaId);
+ ru.setNewObjectId(commit);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ } catch (Exception e) {
+ backgroundFailure.set(true);
+ }
+
+ return bru;
+ }
+ };
+
+ AtomicInteger innerRetryOnExceptionCounter = new AtomicInteger();
+ AtomicInteger outerRetryOnExceptionCounter = new AtomicInteger();
+ UpdateException exception =
+ assertThrows(
+ UpdateException.class,
+ () ->
+ // Outer level retrying. We expect that no retrying is happens here because retrying
+ // is already done on the inner level.
+ retryHelper
+ .action(
+ ActionType.CHANGE_UPDATE,
+ "batchUpdate",
+ () ->
+ // Inner level retrying. We expect that retrying happens here.
+ retryHelper
+ .changeUpdate(
+ "batchUpdate",
+ updateFactory -> {
+ try (BatchUpdate bu =
+ updateFactory.create(
+ project, user.get(), TimeUtil.now())) {
+ bu.addOp(
+ changeId, abandonOpFactory.create(null, "abandon"));
+ bu.execute(listener);
+ }
+ return null;
+ })
+ .listener(
+ new RetryListener() {
+ @Override
+ public <V> void onRetry(Attempt<V> attempt) {
+ if (attempt.hasException()) {
+ @SuppressWarnings("unused")
+ var unused =
+ innerRetryOnExceptionCounter.incrementAndGet();
+ }
+ }
+ })
+ .call())
+ // give it enough time to potentially retry multiple times when each retry also
+ // does retrying
+ .defaultTimeoutMultiplier(5)
+ .listener(
+ new RetryListener() {
+ @Override
+ public <V> void onRetry(Attempt<V> attempt) {
+ if (attempt.hasException()) {
+ @SuppressWarnings("unused")
+ var unused = outerRetryOnExceptionCounter.incrementAndGet();
+ }
+ }
+ })
+ .call());
+ assertThat(backgroundFailure.get()).isFalse();
+
+ // Check that retrying was done on the inner level.
+ assertThat(innerRetryOnExceptionCounter.get()).isGreaterThan(1);
+
+ // Check that there was no retrying on the outer level since retrying was already done on the
+ // inner level.
+ // We expect 1 because RetryListener#onRetry is invoked before the rejection predicate and stop
+ // strategies are applied (i.e. before RetryHelper decides whether retrying should be done).
+ assertThat(outerRetryOnExceptionCounter.get()).isEqualTo(1);
+
+ assertThat(exception).hasCauseThat().isInstanceOf(RetryException.class);
+ assertThat(exception.getCause()).hasCauseThat().isInstanceOf(UpdateException.class);
+ assertThat(exception.getCause().getCause())
+ .hasCauseThat()
+ .isInstanceOf(LockFailureException.class);
+
+ // Check that the change was not updated.
+ notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+ }
+
private Change.Id createChange() throws Exception {
Change.Id id = Change.id(sequences.nextChangeId());
try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.now())) {
diff --git a/plugins/replication b/plugins/replication
index aac2528..2f6c7ce 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit aac252809094b8e4d4e26d69dab75a23d2da1770
+Subproject commit 2f6c7ceeb0cc50bc73d018cd9f990392d58804ab
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index f406511..5b2f951 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -493,6 +493,7 @@
// TODO: Do we still need docOnly bindings?
this.shortcutsController.addAbstract(Shortcut.EMOJI_DROPDOWN, () => {}); // docOnly
this.shortcutsController.addAbstract(Shortcut.MENTIONS_DROPDOWN, () => {}); // docOnly
+ this.shortcutsController.addAbstract(Shortcut.SAVE_COMMENT, () => {}); // docOnly
this.shortcutsController.addAbstract(Shortcut.REFRESH_CHANGE, () =>
this.getChangeModel().navigateToChangeResetReload()
);
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index bc0e015..613f907 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -241,6 +241,20 @@
display: inline-block;
margin-left: var(--spacing-s);
}
+ /* actions-shown-on-collapsed are shown only when .actions is hidden
+ and vice versa. */
+ tr.container td .summary-cell .actions-shown-on-collapsed,
+ tr.container.collapsed:focus-within
+ td
+ .summary-cell
+ .actions-shown-on-collapsed,
+ tr.container.collapsed:hover
+ td
+ .summary-cell
+ .actions-shown-on-collapsed,
+ :host(.dropdown-open) tr td .summary-cell .actions-shown-on-collapsed {
+ display: none;
+ }
tr.container.collapsed td .summary-cell .message {
color: var(--deemphasized-text-color);
}
@@ -248,6 +262,10 @@
tr.container.collapsed td .summary-cell .actions {
display: none;
}
+ tr.container.collapsed td .summary-cell .actions-shown-on-collapsed {
+ display: inline-block;
+ margin-left: var(--spacing-s);
+ }
tr.detailsRow.collapsed {
display: none;
}
@@ -278,6 +296,7 @@
td .summary-cell .tag.brown {
background-color: var(--tag-brown);
}
+ .actions-shown-on-collapsed gr-checks-action,
.actions gr-checks-action,
.actions gr-dropdown {
/* Fitting a 28px button into 20px line-height. */
@@ -549,24 +568,31 @@
const disabledItems = overflowItems
.filter(action => action.disabled)
.map(action => action.id);
- return html`<div class="actions">
- ${this.renderAction(actions[0])} ${this.renderAction(actions[1])}
- <gr-dropdown
- id="moreActions"
- link=""
- vertical-offset="32"
- horizontal-align="right"
- @tap-item=${this.handleAction}
- @opened-changed=${(e: ValueChangedEvent<boolean>) =>
- this.classList.toggle('dropdown-open', e.detail.value)}
- ?hidden=${overflowItems.length === 0}
- .items=${overflowItems}
- .disabledIds=${disabledItems}
- >
- <gr-icon icon="more_vert" aria-labelledby="moreMessage"></gr-icon>
- <span id="moreMessage">More</span>
- </gr-dropdown>
- </div>`;
+ return html` ${when(
+ fixAction,
+ () =>
+ html`<div class="actions-shown-on-collapsed">
+ ${this.renderAction(fixAction)}
+ </div> `
+ )}
+ <div class="actions">
+ ${this.renderAction(actions[0])} ${this.renderAction(actions[1])}
+ <gr-dropdown
+ id="moreActions"
+ link=""
+ vertical-offset="32"
+ horizontal-align="right"
+ @tap-item=${this.handleAction}
+ @opened-changed=${(e: ValueChangedEvent<boolean>) =>
+ this.classList.toggle('dropdown-open', e.detail.value)}
+ ?hidden=${overflowItems.length === 0}
+ .items=${overflowItems}
+ .disabledIds=${disabledItems}
+ >
+ <gr-icon icon="more_vert" aria-labelledby="moreMessage"></gr-icon>
+ <span id="moreMessage">More</span>
+ </gr-dropdown>
+ </div>`;
}
private handleAction(e: CustomEvent<Action>) {
@@ -585,24 +611,6 @@
></gr-checks-action>`;
}
- renderPrimaryActions() {
- const primaryActions = (this.result?.actions ?? []).slice(0, 2);
- if (primaryActions.length === 0) return;
- return html`
- <div class="primaryActions">${primaryActions.map(this.renderAction)}</div>
- `;
- }
-
- renderSecondaryActions() {
- const secondaryActions = (this.result?.actions ?? []).slice(2);
- if (secondaryActions.length === 0) return;
- return html`
- <div class="secondaryActions">
- ${secondaryActions.map(this.renderAction)}
- </div>
- `;
- }
-
renderTag(tag: Tag) {
return html`<button
class="tag ${tag.color}"
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
index 58b939c..89c7fd7 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
@@ -211,7 +211,6 @@
}
private renderActions() {
- if (!this.isExpanded) return nothing;
return html`<div class="actions">
${this.renderShowFixButton()}${this.renderPleaseFixButton()}
</div>`;
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
index b913c87..5aa266c 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
@@ -31,31 +31,42 @@
assert.shadowDom.equal(
element,
`
- <div class="container font-normal warning">
- <div class="header">
- <div class="icon">
- <gr-icon icon="warning" filled></gr-icon>
- </div>
- <div class="name">
- <gr-hovercard-run> </gr-hovercard-run>
- <div class="name" role="button" tabindex="0">FAKE Super Check</div>
- </div>
- <div class="summary">We think that you could improve this.</div>
- <div class="message">
- There is a lot to be said. A lot. I say, a lot.
+ <div class="container font-normal warning">
+ <div class="header">
+ <div class="icon">
+ <gr-icon icon="warning" filled></gr-icon>
+ </div>
+ <div class="name">
+ <gr-hovercard-run> </gr-hovercard-run>
+ <div class="name" role="button" tabindex="0">
+ FAKE Super Check
+ </div>
+ </div>
+ <div class="summary">We think that you could improve this.</div>
+ <div class="message">
+ There is a lot to be said. A lot. I say, a lot.
So please keep reading.
+ </div>
+ <div
+ aria-checked="false"
+ aria-label="Expand result row"
+ class="show-hide"
+ role="switch"
+ tabindex="0"
+ >
+ <gr-icon icon="expand_more"></gr-icon>
+ </div>
</div>
- <div aria-checked="false"
- aria-label="Expand result row"
- class="show-hide"
- role="switch"
- tabindex="0">
- <gr-icon icon="expand_more"></gr-icon>
+ <div class="details">
+ <div class="actions">
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ </div>
</div>
</div>
- <div class="details"></div>
- </div>
- `
+ `
);
});
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
index 98e9eba..368eb22 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
@@ -126,7 +126,11 @@
const MAX_AUTOCOMPLETE_RESULTS = 10;
-const TOKENIZE_REGEX = /(?:[^\s"]+|"[^"]*")+\s*/g;
+// 3 types of tokens
+// 1. predicate:expression (?:[^\s":]+:\s*[^\s"]+)
+// 2. quotes with anything inside "[^"]*"
+// 3. anything else like unfinished predicate [^\s"]+
+const TOKENIZE_REGEX = /(?:(?:[^\s":]+:\s*[^\s"]+)|[^\s"]+|"[^"]*")+\s*/g;
export type SuggestionProvider = (
predicate: string,
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.ts
index f67024f..bc8da05 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.ts
@@ -256,6 +256,18 @@
const s = await element.getSearchSuggestions('is:mergeab');
assert.isEmpty(s);
});
+
+ test('Autocompletes correctly second condition', async () => {
+ const s = await element.getSearchSuggestions('is:open me');
+ assert.equal(s[0].value, 'mergedafter:');
+ });
+
+ test('Autocomplete handles space before expression correctly', async () => {
+ // This previously suggested "mergedafter" (incorrectly) due to the
+ // leading space.
+ const s = await element.getSearchSuggestions('author: me');
+ assert.isEmpty(s);
+ });
});
[
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index bb5030c..fc18968 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -145,7 +145,7 @@
if (replacements.length === 0) return undefined;
return {
- description: fix.description ?? `Fix provided by ${checkName}`,
+ description: fix.description || `Fix provided by ${checkName}`,
fix_id: PROVIDED_FIX_ID,
replacements,
};
diff --git a/polygerrit-ui/app/models/checks/checks-util_test.ts b/polygerrit-ui/app/models/checks/checks-util_test.ts
index ead7dd01..3b5c108 100644
--- a/polygerrit-ui/app/models/checks/checks-util_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-util_test.ts
@@ -93,6 +93,29 @@
assert.equal(rectified?.fix_id, PROVIDED_FIX_ID);
});
+ test('rectifyFix changes description when description is empty', () => {
+ const rectified = rectifyFix(
+ {
+ replacements: [
+ {
+ path: 'test-path',
+ range: {
+ start_line: 1,
+ end_line: 1,
+ start_character: 0,
+ end_character: 1,
+ } as CommentRange,
+ replacement: 'test-replacement-string',
+ },
+ ],
+ description: '',
+ },
+ 'test-check-name'
+ );
+ assert.isDefined(rectified);
+ assert.equal(rectified?.description, 'Fix provided by test-check-name');
+ });
+
test('sortAttemptChoices', () => {
const unsorted: (AttemptChoice | undefined)[] = [
3,
