Merge "Capture an auto-trace when sending email fails"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 32867b6..90c69b6 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4454,8 +4454,7 @@
[[retry.retryWithTraceOnFailure]]retry.retryWithTraceOnFailure::
+
Whether Gerrit should automatically retry operations on failure with tracing
-enabled. The automatically generated traces can help with debugging. Please
-note that only some of the REST endpoints support automatic retry.
+enabled. The automatically generated traces can help with debugging.
+
By default this is set to false.
diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java
index 93e04880..defa4c7 100644
--- a/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -45,7 +45,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -144,7 +143,7 @@
.get(ExternalId.Key.create(SCHEME_USERNAME, username))
.map(e -> get(e.accountId()))
.orElseGet(Optional::empty);
- } catch (IOException | ConfigInvalidException e) {
+ } catch (IOException e) {
logger.atWarning().withCause(e).log("Cannot load AccountState for username %s", username);
return Optional.empty();
}
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 2152e1e..5a89a862 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -114,7 +114,7 @@
public Optional<Account.Id> lookup(String externalId) throws AccountException {
try {
return externalIds.get(ExternalId.Key.parse(externalId)).map(ExternalId::accountId);
- } catch (IOException | ConfigInvalidException e) {
+ } catch (IOException e) {
throw new AccountException("Cannot lookup account " + externalId, e);
}
}
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIds.java b/java/com/google/gerrit/server/account/externalids/ExternalIds.java
index 7b0dc57..4e1e524 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIds.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIds.java
@@ -53,7 +53,7 @@
}
/** Returns the specified external ID. */
- public Optional<ExternalId> get(ExternalId.Key key) throws IOException, ConfigInvalidException {
+ public Optional<ExternalId> get(ExternalId.Key key) throws IOException {
return externalIdCache.byKey(key);
}
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 65662ba..5e9ce97 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -32,6 +32,14 @@
public static final String GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_PUSH_CANCELLATION =
"GerritBackendRequestFeature__enable_push_cencallation";
+ /**
+ * Allow legacy {@link com.google.gerrit.entities.SubmitRecord}s to be converted and returned as
+ * submit requirements by the {@link
+ * com.google.gerrit.server.project.SubmitRequirementsEvaluator}.
+ */
+ public static final String GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS =
+ "GerritBackendRequestFeature__enable_legacy_submit_requirements";
+
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES =
ImmutableSet.of(UI_FEATURE_PATCHSET_COMMENTS);
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index d08f6c4..7d743dc 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -174,6 +174,10 @@
private static final Pattern REMOVED_VOTE_PATTERN = Pattern.compile("Removed (.*) by (.*)");
+ private static final String REMOVED_VOTES_CHANGE_MESSAGE_START = "Removed the following votes:";
+ private static final Pattern REMOVED_VOTES_CHANGE_MESSAGE_PATTERN =
+ Pattern.compile("\\* (.*) by (.*)");
+
private static final Pattern REMOVED_CHANGE_MESSAGE_PATTERN =
Pattern.compile("Change message removed by: (.*)(\nReason: .*)?");
@@ -600,6 +604,34 @@
return Optional.empty();
}
+ private Optional<String> fixRemoveVotesChangeMessage(
+ ChangeFixProgress changeFixProgress, String originalChangeMessage) {
+ if (Strings.isNullOrEmpty(originalChangeMessage)
+ || !originalChangeMessage.startsWith(REMOVED_VOTES_CHANGE_MESSAGE_START)) {
+ return Optional.empty();
+ }
+ String[] lines = originalChangeMessage.split("\\r?\\n");
+ StringBuilder fixedLines = new StringBuilder();
+ for (int i = 1; i < lines.length; i++) {
+ if (lines[i].isEmpty()) {
+ continue;
+ }
+ Matcher matcher = REMOVED_VOTES_CHANGE_MESSAGE_PATTERN.matcher(lines[i]);
+ if (matcher.matches() && !NON_REPLACE_ACCOUNT_PATTERN.matcher(matcher.group(2)).matches()) {
+ fixedLines.append(
+ String.format(
+ "* %s by %s\n",
+ matcher.group(1),
+ getPossibleAccountReplacement(
+ changeFixProgress, Optional.empty(), getNameFromNameEmail(matcher.group(2)))));
+ }
+ }
+ if (fixedLines.length() == 0) {
+ return Optional.empty();
+ }
+ return Optional.of(REMOVED_VOTES_CHANGE_MESSAGE_START + "\n" + fixedLines);
+ }
+
private Optional<String> fixDeleteChangeMessageCommitMessage(String originalChangeMessage) {
if (Strings.isNullOrEmpty(originalChangeMessage)) {
return Optional.empty();
@@ -887,6 +919,9 @@
fixedChangeMessage = fixReviewerChangeMessage(originalChangeMessage);
}
if (!fixedChangeMessage.isPresent()) {
+ fixedChangeMessage = fixRemoveVotesChangeMessage(fixProgress, originalChangeMessage);
+ }
+ if (!fixedChangeMessage.isPresent()) {
fixedChangeMessage =
fixRemoveVoteChangeMessage(fixProgress, Optional.empty(), originalChangeMessage);
}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 28dde3b..6fa14e1 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -57,11 +57,9 @@
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
-import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutorService;
import org.eclipse.jgit.diff.Edit;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
@@ -115,8 +113,6 @@
private final PatchSetUtil psUtil;
private final Provider<PatchScriptBuilder> builderFactory;
private final PatchListCache patchListCache;
- private final Metrics metrics;
- private final ExecutorService executor;
private final String fileName;
@Nullable private final PatchSet.Id psa;
@@ -147,8 +143,6 @@
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
- Metrics metrics,
- @DiffExecutor ExecutorService executor,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted("patchSetA") @Nullable PatchSet.Id patchSetA,
@@ -164,8 +158,6 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.diffOperations = diffOperations;
- this.metrics = metrics;
- this.executor = executor;
this.fileName = fileName;
this.psa = patchSetA;
@@ -191,8 +183,6 @@
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
- Metrics metrics,
- @DiffExecutor ExecutorService executor,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted int parentNum,
@@ -208,8 +198,6 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.diffOperations = diffOperations;
- this.metrics = metrics;
- this.executor = executor;
this.fileName = fileName;
this.psa = null;
@@ -299,61 +287,6 @@
return newBuilder().toPatchScriptNew(git, fileDiffOutput);
}
- /**
- * The comparison is not exhaustive but is using the most important fields. Comparing all fields
- * will require some work in {@link PatchScript} to, e.g., convert it to autovalue. This
- * comparison method shall give a strong signal that both patchscripts are almost identical.
- */
- private static boolean areEqualPatchscripts(PatchScript ps1, PatchScript ps2) {
- boolean equal = true;
- if (!ps1.getChangeType().equals(ps2.getChangeType())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching change type: old = %s, new = %s.", ps1.getChangeType(), ps2.getChangeType());
- }
- if (!ps1.getPatchHeader().equals(ps2.getPatchHeader())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching patch header: old = %s, new = %s.",
- ps1.getPatchHeader(), ps2.getPatchHeader());
- }
- if (!Objects.equals(ps1.getOldName(), ps2.getOldName())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching old name: old = %s, new = %s.", ps1.getOldName(), ps2.getOldName());
- }
- if (!Objects.equals(ps1.getNewName(), ps2.getNewName())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching new name: old = %s, new = %s.", ps1.getNewName(), ps2.getNewName());
- }
- if (!ps1.getEdits().containsAll(ps2.getEdits())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching edits: old = %s, new = %s.", ps1.getEdits(), ps2.getEdits());
- }
- if (!ps2.getEdits().containsAll(ps1.getEdits())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching edits: old = %s, new = %s.", ps1.getEdits(), ps2.getEdits());
- }
- if (!ps1.getEditsDueToRebase().equals(ps2.getEditsDueToRebase())) {
- equal = false;
- logger.atWarning().log(
- "Mismatching edits due to rebase: old = %s, new = %s.",
- ps1.getEditsDueToRebase(), ps2.getEditsDueToRebase());
- }
- if (!ps1.getA().equals(ps2.getA())) {
- equal = false;
- logger.atWarning().log("Mismatching sparse file content in old commit.");
- }
- if (!ps1.getB().equals(ps2.getB())) {
- equal = false;
- logger.atWarning().log("Mismatching sparse file content in new commit.");
- }
- return equal;
- }
-
private Optional<ObjectId> getAId() {
if (psa == null) {
return Optional.empty();
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index 6f93f2e..f293a64 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -381,11 +381,13 @@
}
/**
- * Create a single {@link GitFileDiff} with {@link Patch.ChangeType} equals {@link
- * Patch.ChangeType#REWRITE}, assuming the input list contains two entries.
+ * Create a single {@link GitFileDiff} with {@link com.google.gerrit.entities.Patch.ChangeType}
+ * equals {@link com.google.gerrit.entities.Patch.ChangeType#REWRITE}, assuming the input list
+ * contains two entries.
*
* @param gitDiffs input list of exactly two {@link GitFileDiff} for same file path.
- * @return a single {@link GitFileDiff} with change type equals {@link Patch.ChangeType#REWRITE}.
+ * @return a single {@link GitFileDiff} with change type equals {@link
+ * com.google.gerrit.entities.Patch.ChangeType#REWRITE}.
* @throws DiffNotAvailableException if input list contains git diffs with change types other than
* {ADDED, DELETED}. This is a JGit error.
*/
diff --git a/java/com/google/gerrit/server/project/ProjectCreator.java b/java/com/google/gerrit/server/project/ProjectCreator.java
index c38cdfa..4e778a4 100644
--- a/java/com/google/gerrit/server/project/ProjectCreator.java
+++ b/java/com/google/gerrit/server/project/ProjectCreator.java
@@ -30,7 +30,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.events.NewProjectCreatedListener;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.server.GerritPersonIdent;
@@ -105,8 +104,7 @@
}
public ProjectState createProject(CreateProjectArgs args)
- throws BadRequestException, ResourceConflictException, IOException, ConfigInvalidException,
- PreconditionFailedException {
+ throws BadRequestException, ResourceConflictException, IOException, ConfigInvalidException {
final Project.NameKey nameKey = args.getProject();
try {
final String head = args.permissionsOnly ? RefNames.REFS_CONFIG : args.branch.get(0);
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
index f43e6b4..5d8b99a 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
@@ -132,6 +132,9 @@
return ImmutableList.of(String.format(String.format("-label:%s=MIN", lt.getName())));
case MAX_NO_BLOCK:
return ImmutableList.of(String.format(String.format("label:%s=MAX", lt.getName())));
+ case NO_BLOCK:
+ case NO_OP:
+ case PATCH_SET_LOCK:
default:
return ImmutableList.of();
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index 36377e9..9be50c7 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -17,6 +17,8 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
@@ -24,6 +26,8 @@
import com.google.gerrit.entities.SubmitRequirementResult;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.inject.AbstractModule;
@@ -31,13 +35,21 @@
import com.google.inject.Module;
import com.google.inject.Provider;
import com.google.inject.Scopes;
+import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
/** Evaluates submit requirements for different change data. */
public class SubmitRequirementsEvaluatorImpl implements SubmitRequirementsEvaluator {
+
private final Provider<ChangeQueryBuilder> changeQueryBuilderProvider;
private final ProjectCache projectCache;
+ private final SubmitRuleEvaluator.Factory legacyEvaluator;
+ private final ExperimentFeatures experimentFeatures;
public static Module module() {
return new AbstractModule() {
@@ -52,9 +64,14 @@
@Inject
private SubmitRequirementsEvaluatorImpl(
- Provider<ChangeQueryBuilder> changeQueryBuilderProvider, ProjectCache projectCache) {
+ Provider<ChangeQueryBuilder> changeQueryBuilderProvider,
+ ProjectCache projectCache,
+ SubmitRuleEvaluator.Factory legacyEvaluator,
+ ExperimentFeatures experimentFeatures) {
this.changeQueryBuilderProvider = changeQueryBuilderProvider;
this.projectCache = projectCache;
+ this.legacyEvaluator = legacyEvaluator;
+ this.experimentFeatures = experimentFeatures;
}
@Override
@@ -65,14 +82,13 @@
@Override
public Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(ChangeData cd) {
- ProjectState state = projectCache.get(cd.project()).orElseThrow(illegalState(cd.project()));
- Map<String, SubmitRequirement> requirements = state.getSubmitRequirements();
- ImmutableMap.Builder<SubmitRequirement, SubmitRequirementResult> result =
- ImmutableMap.builderWithExpectedSize(requirements.size());
- for (SubmitRequirement requirement : requirements.values()) {
- result.put(requirement, evaluateRequirement(requirement, cd));
+ Map<SubmitRequirement, SubmitRequirementResult> result = getRequirements(cd);
+ if (experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
+ result.putAll(getLegacyRequirements(cd));
}
- return result.build();
+ return ImmutableMap.copyOf(result);
}
@Override
@@ -113,6 +129,34 @@
}
}
+ /** Evaluate and return submit requirements stored in this project's config and its parents. */
+ private Map<SubmitRequirement, SubmitRequirementResult> getRequirements(ChangeData cd) {
+ ProjectState state = projectCache.get(cd.project()).orElseThrow(illegalState(cd.project()));
+ Map<String, SubmitRequirement> requirements = state.getSubmitRequirements();
+ Map<SubmitRequirement, SubmitRequirementResult> result = new HashMap<>();
+ for (SubmitRequirement requirement : requirements.values()) {
+ result.put(requirement, evaluateRequirement(requirement, cd));
+ }
+ return result;
+ }
+
+ /**
+ * Convert and return legacy submit records (created by label functions and other {@link
+ * com.google.gerrit.server.rules.SubmitRule}s to submit requirement results.
+ */
+ private Map<SubmitRequirement, SubmitRequirementResult> getLegacyRequirements(ChangeData cd) {
+ // We use SubmitRuleOptions.defaults() which does not recompute submit rules for closed changes.
+ // This doesn't have an effect since we never call this class (i.e. to evaluate submit
+ // requirements) for closed changes.
+ List<SubmitRecord> records = legacyEvaluator.create(SubmitRuleOptions.defaults()).evaluate(cd);
+ List<LabelType> labelTypes = cd.getLabelTypes().getLabelTypes();
+ ObjectId commitId = cd.currentPatchSet().commitId();
+ return records.stream()
+ .map(r -> SubmitRequirementsAdapter.createResult(r, labelTypes, commitId))
+ .flatMap(List::stream)
+ .collect(Collectors.toMap(sr -> sr.submitRequirement(), Function.identity()));
+ }
+
/** Evaluate the predicate recursively using change data. */
private PredicateResult evaluatePredicateTree(
Predicate<ChangeData> predicate, ChangeData changeData) {
diff --git a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
index 8995740..054a69e 100644
--- a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
+++ b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
@@ -81,9 +81,8 @@
}
if (r.isEmpty()) {
return ImmutableList.of(ChangeIndexPredicate.none());
- } else {
- return ImmutableList.of(or(r));
}
+ return ImmutableList.of(or(r));
}
protected static Collection<ProjectWatchKey> getWatches(ChangeQueryBuilder.Arguments args)
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index b846662..2249b0e 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -548,8 +548,8 @@
* @param retryer the retryer
* @param listener metric listener
* @return the result of executing the action
- * @throws Throwable any error or exception that made the action fail, callers are expected to
- * catch and inspect this Throwable to decide carefully whether it should be re-thrown
+ * @throws Exception any exception that made the action fail, callers are expected to catch and
+ * inspect this exception to decide carefully whether it should be re-thrown
*/
private <T> T executeWithTimeoutCount(
String actionType,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 9d9c411..ac43078 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -169,6 +169,7 @@
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.testing.TestChangeETagComputation;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.index.change.ChangeIndex;
@@ -4404,6 +4405,62 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ public void submitRequirements_ReturnForLegacySubmitRecords_IfEnabled() throws Exception {
+ configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("build-cop-override")
+ .ref("refs/heads/master")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // 1. Project has two legacy requirements: Code-Review and bco. Both unsatisfied.
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "build-cop-override", Status.UNSATISFIED, /* isLegacy= */ true);
+
+ // 2. Vote +1 on bco. bco becomes satisfied
+ voteLabel(changeId, "build-cop-override", 1);
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "build-cop-override", Status.SATISFIED, /* isLegacy= */ true);
+
+ // 3. Vote +1 on Code-Review. Code-Review becomes satisfied
+ voteLabel(changeId, "Code-Review", 2);
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "build-cop-override", Status.SATISFIED, /* isLegacy= */ true);
+
+ // 4. Merge the change. Submit requirements status is presented from NoteDb.
+ gApi.changes().id(changeId).current().submit();
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "build-cop-override", Status.SATISFIED, /* isLegacy= */ true);
+ }
+
+ @Test
public void submitRequirement_backFilledFromIndexForActiveChanges() throws Exception {
configSubmitRequirement(
project,
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
index 64ad900..0fc42ff 100644
--- a/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
@@ -24,7 +24,6 @@
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.server.ServerInitiated;
-import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdNotes;
@@ -50,7 +49,6 @@
@Inject private Sequences sequences;
@Inject private @ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider;
@Inject private ExternalIdNotes.Factory extIdNotesFactory;
- @Inject private Accounts accounts;
public static class Module extends AbstractModule {
@Override
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 68db634..26e1881 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -837,6 +837,67 @@
}
@Test
+ public void fixRemoveVotesChangeMessage() throws Exception {
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(otherUserId, VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+
+ // Even though footer is missing, accounts are matched among the account in change updates.
+ getChangeUpdateBody(
+ c,
+ /*changeMessage=*/ "Removed the following votes:\n"
+ + String.format("* Verified-1 by %s\n", otherUser.getNameEmail())),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c,
+ /*changeMessage=*/ "Removed the following votes:\n"
+ + String.format("* Verified+2 by %s\n", changeOwner.getNameEmail())
+ + String.format("* Verified-1 by %s\n", changeOwner.getNameEmail())
+ + String.format("* Code-Review by %s\n", otherUser.getNameEmail())),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ // No rewrite for default
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c,
+ /*changeMessage=*/ "Removed the following votes:\n"
+ + "* Verified+2 by Gerrit Account\n"
+ + "* Verified-1 by <GERRIT_ACCOUNT_2>\n"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -7 +7 @@\n"
+ + "-* Verified-1 by Other Account <other@account.com>\n"
+ + "+* Verified-1 by <GERRIT_ACCOUNT_2>\n",
+ "@@ -7,3 +7,3 @@\n"
+ + "-* Verified+2 by Change Owner <change@owner.com>\n"
+ + "-* Verified-1 by Change Owner <change@owner.com>\n"
+ + "-* Code-Review by Other Account <other@account.com>\n"
+ + "+* Verified+2 by <GERRIT_ACCOUNT_1>\n"
+ + "+* Verified-1 by <GERRIT_ACCOUNT_1>\n"
+ + "+* Code-Review by <GERRIT_ACCOUNT_2>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
public void fixAttentionFooter() throws Exception {
Change c = newChange();
ImmutableList.Builder<ObjectId> commitsToFix = new ImmutableList.Builder<>();
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 06e3502..5358d01 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -630,7 +630,7 @@
Change change1 = createChange(repo, johnDoe);
Change change2 = createChange(repo, john);
Change change3 = createChange(repo, doeSmith);
- Change change4 = createChange(repo, selfName);
+ createChange(repo, selfName);
// Only email address.
assertQuery(searchOperator + "john.doe@example.com", change1);
@@ -2292,7 +2292,7 @@
try (Registration registration =
extensionRegistry.newRegistration().add(new FakeSubmitRule())) {
TestRepository<Repo> repo = createProject("repo");
- Change change = insert(repo, newChange(repo));
+ insert(repo, newChange(repo));
assertQuery("rule:non-existent-rule");
}
}
diff --git a/plugins/package.json b/plugins/package.json
index f761be9..4e3c376 100644
--- a/plugins/package.json
+++ b/plugins/package.json
@@ -5,7 +5,7 @@
"dependencies": {
"@polymer/decorators": "^3.0.0",
"@polymer/polymer": "^3.4.1",
- "@gerritcodereview/typescript-api": "3.4.2",
+ "@gerritcodereview/typescript-api": "3.4.4",
"lit": "2.0.0-rc.3"
},
"license": "Apache-2.0",
diff --git a/plugins/yarn.lock b/plugins/yarn.lock
index 1faa71a..3ff1cc4 100644
--- a/plugins/yarn.lock
+++ b/plugins/yarn.lock
@@ -2,10 +2,10 @@
# yarn lockfile v1
-"@gerritcodereview/typescript-api@3.4.2":
- version "3.4.2"
- resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.4.2.tgz#79e8ff336608cbf18e651bfa9541d7bdead5e1f9"
- integrity sha512-iqHd6G43pV4Wk5iNw95AQmWUBuIrY+dvQ1Ne8ZYkOwRhdruh4BAPhMtsmqWDlcVQbfcwZD5F2zFkGB4J4htggw==
+"@gerritcodereview/typescript-api@3.4.4":
+ version "3.4.4"
+ resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.4.4.tgz#9f09687038088dd7edd3b4e30d249502eb21bfbc"
+ integrity sha512-MAiQwntcQ59b92yYDsVIXj3oBbAB4C7HELkLFFbYs4ZjzC43XqqtR9VF0dh5OUC8wzFZttgUiOmGehk9edpPuw==
"@lit/reactive-element@^1.0.0-rc.2":
version "1.0.0-rc.2"
diff --git a/polygerrit-ui/app/api/BUILD_for_publishing_api_only b/polygerrit-ui/app/api/BUILD_for_publishing_api_only
new file mode 100644
index 0000000..9d3029b
--- /dev/null
+++ b/polygerrit-ui/app/api/BUILD_for_publishing_api_only
@@ -0,0 +1,50 @@
+# This BUILD file is only for publishing the
+# "Gerrit Frontend Plugin TypeScript API" as an npm package.
+#
+# Publishing procedure:
+# - Execute the `publish.sh` script from the Gerrit root dir.
+# - Verify that the contents look good.
+# - Increment the version in package.json.
+# - Execute `publish.sh --upload`.
+#
+# NB: Renaming to 'BUILD' breaks the app/BUILD, because then the api/ sources
+# are not visible anymore to the parent BUILD. And if ts_projects depend on each
+# other, then the api/ files would have to be imported with their full package
+# names.
+load("@build_bazel_rules_nodejs//:index.bzl", "pkg_npm")
+load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_project")
+
+filegroup(
+ name = "js_plugin_api_srcs",
+ srcs = glob(["**/*.ts"]),
+)
+
+ts_config(
+ name = "ts_config",
+ src = "tsconfig.json",
+ deps = [
+ "//plugins:tsconfig-plugins-base.json",
+ ],
+)
+
+ts_project(
+ name = "js_plugin_api_compiled",
+ srcs = glob(["**/*.ts"]),
+ incremental = True,
+ tsc = "//tools/node_tools:tsc-bin",
+ tsconfig = ":ts_config",
+)
+
+# Use this rule for publishing the js plugin api as a package to the npm repo.
+pkg_npm(
+ name = "js_plugin_api_npm_package",
+ srcs = glob(
+ ["**/*"],
+ exclude = [
+ "BUILD",
+ "tsconfig.json",
+ "publish.sh",
+ ],
+ ),
+ deps = [":js_plugin_api_compiled"],
+)
diff --git a/polygerrit-ui/app/api/README.md b/polygerrit-ui/app/api/README.md
index 550063f..b5710bf 100644
--- a/polygerrit-ui/app/api/README.md
+++ b/polygerrit-ui/app/api/README.md
@@ -1,23 +1,25 @@
-# API
+# Gerrit TypeScript Plugin API
-In this folder, we declare the API of various parts of the Gerrit webclient.
-There are two primary use cases for this:
+This package contains the types for developing browser plugins for the
+Gerrit Code Review web application. General documentation for plugin
+developers can be found at
+[gerrit-review.googlesource.com](https://gerrit-review.googlesource.com/Documentation/pg-plugin-dev.html).
-* apps that embed our diff viewer, gr-diff
-* Gerrit plugins that need to access some part of Gerrit to extend it
+The `.ts` files only contain types, interfaces and enums, and thus the compiled
+`.js` files only contain the enums. For JavaScript plugins this package is not
+really useful or necessary, but it also serves as the source of truth for
+what plugin APIs are actually supported.
-Both may be built as a separate bundle, but would like to type check against
-the same types the Gerrit/gr-diff bundle uses. For this reason, this folder
-should contain only types, with the exception of enums, where having the
-value side is deemed an acceptable duplication.
+Versioning of this API matches the MAJOR and MINOR versions of the general
+Gerrit releases, but the PATCH version is independent. When you are building
+a plugin for Gerrit x.y.z, then you should use the API package x.y.n, where
+n is the highest available patch version of the API. Patch versions will only
+contain additions and fixes, minor versions may include API removals.
All types in here should use the `declare` keyword to prevent bundlers from
renaming fields, which would break communication across separately built
-bundles. Again enums are the exception, because their keys are not referenced
+bundles. enums are the exception, because their keys are not referenced
across bundles, and values will not be renamed by bundlers as they are strings.
-This API is used by other apps embedding gr-diff and any breaking changes
+This API is also used by other apps embedding gr-diff and any breaking changes
should be discussed with the Gerrit core team and properly versioned.
-
-Gerrit types should either directly use or extend these types, so that
-breaking changes to the implementation require changes to these files.
diff --git a/polygerrit-ui/app/api/package.json b/polygerrit-ui/app/api/package.json
new file mode 100644
index 0000000..8af6832
--- /dev/null
+++ b/polygerrit-ui/app/api/package.json
@@ -0,0 +1,9 @@
+{
+ "name": "@gerritcodereview/typescript-api",
+ "version": "3.4.4",
+ "description": "Gerrit Code Review - TypeScript API",
+ "homepage": "https://www.gerritcodereview.com/",
+ "browser": true,
+ "dependencies": {},
+ "license": "Apache-2.0"
+}
diff --git a/polygerrit-ui/app/api/publish.sh b/polygerrit-ui/app/api/publish.sh
new file mode 100755
index 0000000..16de4c9
--- /dev/null
+++ b/polygerrit-ui/app/api/publish.sh
@@ -0,0 +1,29 @@
+#!/usr/bin/env bash
+
+# Should be executed from the root of the Gerrit repo:
+# polygerrit-ui/app/api/publish.sh
+#
+# Builds the npm package @gerritcodereview/typescript-api
+#
+# Adding the `--upload` argument will also publish the package.
+
+bazel_bin=$(which bazelisk 2>/dev/null)
+if [[ -z "$bazel_bin" ]]; then
+ echo "Warning: bazelisk is not installed; falling back to bazel."
+ bazel_bin=bazel
+fi
+api_path=polygerrit-ui/app/api
+
+function cleanup() {
+ echo "Cleaning up ..."
+ rm -f ${api_path}/BUILD
+}
+trap cleanup EXIT
+cp ${api_path}/BUILD_for_publishing_api_only ${api_path}/BUILD
+
+${bazel_bin} build //${api_path}:js_plugin_api_npm_package
+
+if [ "$1" == "--upload" ]; then
+ echo 'Uploading npm package @gerritcodereview/typescript-api'
+ ${bazel_bin} run //${api_path}:js_plugin_api_npm_package.publish
+fi
diff --git a/polygerrit-ui/app/api/tsconfig.json b/polygerrit-ui/app/api/tsconfig.json
new file mode 100644
index 0000000..4d8ecac
--- /dev/null
+++ b/polygerrit-ui/app/api/tsconfig.json
@@ -0,0 +1,6 @@
+{
+ "extends": "../../../plugins/tsconfig-plugins-base.json",
+ "include": [
+ "**/*",
+ ],
+}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-action.ts b/polygerrit-ui/app/elements/checks/gr-checks-action.ts
index e8760b1..87cab46 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-action.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-action.ts
@@ -43,20 +43,13 @@
white-space: nowrap;
}
gr-button {
- /* It is not fully understood why this is needed, but otherwise the
- paper-tooltip may render under some iron-icons of the content
- below. Maybe this has to do with a z-index:0 setting for
- paper-button, such that a stacking context is created. And the high
- z-index of the paper-tooltip will then only be interpreted within
- that stacking context. */
- z-index: 1;
--padding: var(--spacing-s) var(--spacing-m);
}
- gr-button paper-tooltip {
+ paper-tooltip {
text-transform: none;
text-align: center;
white-space: normal;
- width: 200px;
+ max-width: 200px;
}
`,
];
@@ -71,13 +64,17 @@
@click="${(e: Event) => this.handleClick(e)}"
>
${this.action.name}
- <paper-tooltip
- ?hidden="${!this.action.tooltip}"
- offset="5"
- fit-to-visible-bounds="true"
- >${this.action.tooltip}</paper-tooltip
- >
</gr-button>
+ ${this.renderTooltip()}
+ `;
+ }
+
+ private renderTooltip() {
+ if (!this.action.tooltip) return;
+ return html`
+ <paper-tooltip offset="5" fit-to-visible-bounds="true">
+ ${this.action.tooltip}
+ </paper-tooltip>
`;
}
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index a68de38..f5d6e35 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -623,7 +623,7 @@
actions: [
{
name: 'Re-Run',
- tooltip: 'More powerful run than before',
+ tooltip: 'small',
primary: true,
callback: () => Promise.resolve({message: 'fake "re-run" triggered'}),
},