opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-3.0'

Browse Source 
* stable-3.0:
  ReceiveCommits: Don't instantiate BranchCommitValidator repeatedly
  ElasticVersionTest: Add missing version 7.1 asserts
  ElasticVersionTest: Improve test method ordering
  ElasticVersionTest: Make test method names accurate
  Upgrade gitiles to 0.2-10
  PermissionRange: Interpret allowMin > allowMax as disallow
  Fix formatting issue in project config documentation
  ReceiveCommits: Validate ref operation before commits for non-ff case
  Add test coverage for ref operation validation extension point
  Allow CommitValidationListener to ignore 'skip validation' push option
  Upgrade JGit to 4.11.8.201904181247-r
  CreateProject: Expose createProject method
  ElasticContainer: Switch to Alpine-based images from blacktop
  DeleteVote.soy: Make wording consistent with DeleteVoteHtml.soy
  DeleteVote.soy add missing review URL
  Update git submodules
  NewChange.soy: add missing closing parenthesis
  Remove use of "NoteDB" config in PolyGerrit
  Always show "NoteDB" config under gr-repo
  Fix hiding "enable signed push" and "require signed push" under gr-repo
  Update git submodules
  gerrit.sh: Fix message about JRE
  Allow CommitValidationListener to ignore 'skip validation' push option
  ChangeEmail: Stop using deprecated SoyListData and SoyMapData
  IndexServlet: Stop using deprecated SoyMapData
  Bazel: Fix lint warning flagged by buildifier

Change-Id: Icadfa2bb77929e0aa5118ea8492fdb24e0e9aa7e
This commit is contained in:
Marco Miller
2019-06-27 12:51:40 -04:00
parent 5c4302c783 999dd71295
commit 9552ba0ed5
35 changed files with 476 additions and 258 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/common/data/PermissionRange.java
View File

@@ -74,8 +74,8 @@ public class PermissionRange implements Comparable<PermissionRange> {
this.min = min;
this.max = max;
} else {
this.min = max;
this.max = min;
this.min = 0;
this.max = 0;
}
}

6
java/com/google/gerrit/extensions/registration/DynamicSet.java
View File

@@ -36,6 +36,8 @@ import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
/**
* A set of members that can be modified as plugins reload.
@@ -284,6 +286,10 @@ public class DynamicSet<T> implements Iterable<T> {
return new ReloadableHandle(ref, key, ref.get());
}
public Stream<T> stream() {
return StreamSupport.stream(spliterator(), false);
}
private class ReloadableHandle implements ReloadableRegistrationHandle<T> {
private final AtomicReference<Extension<T>> ref;
private final Key<T> key;

26
java/com/google/gerrit/server/git/receive/BranchCommitValidator.java
View File

@@ -107,6 +107,29 @@ public class BranchCommitValidator {
NoteMap rejectCommits,
@Nullable Change change)
throws IOException {
return validateCommit(objectReader, cmd, commit, isMerged, rejectCommits, change, false);
}
/**
* Validates a single commit. If the commit does not validate, the command is rejected.
*
* @param objectReader the object reader to use.
* @param cmd the ReceiveCommand executing the push.
* @param commit the commit being validated.
* @param isMerged whether this is a merge commit created by magicBranch --merge option
* @param change the change for which this is a new patchset.
* @param skipValidation whether 'skip-validation' was requested.
* @return The validation {@link Result}.
*/
Result validateCommit(
ObjectReader objectReader,
ReceiveCommand cmd,
RevCommit commit,
boolean isMerged,
NoteMap rejectCommits,
@Nullable Change change,
boolean skipValidation)
throws IOException {
ImmutableList.Builder<CommitValidationMessage> messages = new ImmutableList.Builder<>();
try (CommitReceivedEvent receiveEvent =
new CommitReceivedEvent(cmd, project, branch.branch(), objectReader, commit, user)) {
@@ -123,7 +146,8 @@ public class BranchCommitValidator {
sshInfo,
rejectCommits,
receiveEvent.revWalk,
change);
change,
skipValidation);
}
for (CommitValidationMessage m : validators.validate(receiveEvent)) {

38
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -1355,11 +1355,8 @@ class ReceiveCommits {
private void parseRewind(ReceiveCommand cmd) throws PermissionBackendException {
try (TraceTimer traceTimer = newTimer("parseRewind")) {
RevCommit newObject;
try {
newObject = receivePack.getRevWalk().parseCommit(cmd.getNewId());
} catch (IncorrectObjectTypeException notCommit) {
newObject = null;
receivePack.getRevWalk().parseCommit(cmd.getNewId());
} catch (IOException err) {
logger.atSevere().withCause(err).log(
"Invalid object %s for %s forced update", cmd.getNewId().name(), cmd.getRefName());
@@ -1368,18 +1365,16 @@ class ReceiveCommits {
}
logger.atFine().log("Rewinding %s", cmd);
if (newObject != null) {
validateRegularPushCommits(
BranchNameKey.create(project.getNameKey(), cmd.getRefName()), cmd);
if (cmd.getResult() != NOT_ATTEMPTED) {
return;
}
if (!validRefOperation(cmd)) {
return;
}
validateRegularPushCommits(BranchNameKey.create(project.getNameKey(), cmd.getRefName()), cmd);
if (cmd.getResult() != NOT_ATTEMPTED) {
return;
}
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.FORCE_UPDATE);
if (!err.isPresent()) {
validRefOperation(cmd);
} else {
if (err.isPresent()) {
rejectProhibited(cmd, err.get());
}
}
@@ -3218,10 +3213,12 @@ class ReceiveCommits {
throws PermissionBackendException {
try (TraceTimer traceTimer =
newTimer("validateRegularPushCommits", "branch", branch.branch())) {
if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) {
boolean skipValidation =
!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION);
if (skipValidation) {
if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) {
reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION);
return;
@@ -3236,8 +3233,6 @@ class ReceiveCommits {
if (!Iterables.isEmpty(rejectCommits)) {
reject(cmd, "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION);
}
logger.atFine().log("Short-circuiting new commit validation");
return;
}
BranchCommitValidator validator = commitValidatorFactory.create(projectState, branch, user);
@@ -3255,7 +3250,7 @@ class ReceiveCommits {
int limit = receiveConfig.maxBatchCommits;
int n = 0;
for (RevCommit c; (c = walk.next()) != null; ) {
if (++n > limit) {
if (++n > limit && !skipValidation) {
logger.atFine().log("Number of new commits exceeds limit of %d", limit);
reject(
cmd,
@@ -3268,7 +3263,8 @@ class ReceiveCommits {
}
BranchCommitValidator.Result validationResult =
validator.validateCommit(walk.getObjectReader(), cmd, c, false, rejectCommits, null);
validator.validateCommit(
walk.getObjectReader(), cmd, c, false, rejectCommits, null, skipValidation);
messages.addAll(validationResult.messages());
if (!validationResult.isValid()) {
break;

10
java/com/google/gerrit/server/git/validators/CommitValidationListener.java
View File

@@ -35,4 +35,14 @@ public interface CommitValidationListener {
*/
List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException;
/**
* Whether this validator should validate all commits.
*
* @return {@code true} if this validator should validate all commits, even when the {@code
* skip-validation} push option was specified.
*/
default boolean shouldValidateAllCommits() {
return false;
}
}

32
java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -132,7 +132,8 @@ public class CommitValidators {
SshInfo sshInfo,
NoteMap rejectCommits,
RevWalk rw,
@Nullable Change change)
@Nullable Change change,
boolean skipValidation)
throws IOException {
PermissionBackend.ForRef perm = forProject.ref(branch.branch());
ProjectState projectState = projectCache.checkedGet(branch.project());
@@ -153,7 +154,7 @@ public class CommitValidators {
change),
new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new PluginCommitValidationListener(pluginValidators, skipValidation),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountCommitValidator(repoManager, allUsers, accountValidator),
new GroupCommitValidator(allUsers)));
@@ -476,27 +477,50 @@ public class CommitValidators {
/** Execute commit validation plug-ins */
public static class PluginCommitValidationListener implements CommitValidationListener {
private boolean skipValidation;
private final PluginSetContext<CommitValidationListener> commitValidationListeners;
public PluginCommitValidationListener(
final PluginSetContext<CommitValidationListener> commitValidationListeners) {
this(commitValidationListeners, false);
}
public PluginCommitValidationListener(
final PluginSetContext<CommitValidationListener> commitValidationListeners,
boolean skipValidation) {
this.skipValidation = skipValidation;
this.commitValidationListeners = commitValidationListeners;
}
private void runValidator(
CommitValidationListener validator,
List<CommitValidationMessage> messages,
CommitReceivedEvent receiveEvent)
throws CommitValidationException {
if (skipValidation && !validator.shouldValidateAllCommits()) {
return;
}
messages.addAll(validator.onCommitReceived(receiveEvent));
}
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
List<CommitValidationMessage> messages = new ArrayList<>();
try {
commitValidationListeners.runEach(
l -> messages.addAll(l.onCommitReceived(receiveEvent)),
CommitValidationException.class);
l -> runValidator(l, messages, receiveEvent), CommitValidationException.class);
} catch (CommitValidationException e) {
messages.addAll(e.getMessages());
throw new CommitValidationException(e.getMessage(), messages);
}
return messages;
}
@Override
public boolean shouldValidateAllCommits() {
return commitValidationListeners.stream().anyMatch(v -> v.shouldValidateAllCommits());
}
}
public static class SignedOffByValidator implements CommitValidationListener {

20
java/com/google/gerrit/server/mail/send/ChangeEmail.java
View File

@@ -15,6 +15,8 @@
package com.google.gerrit.server.mail.send;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ListMultimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -44,8 +46,6 @@ import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.template.soy.data.SoyListData;
import com.google.template.soy.data.SoyMapData;
import java.io.IOException;
import java.sql.Timestamp;
import java.text.MessageFormat;
@@ -556,15 +556,15 @@ public abstract class ChangeEmail extends NotificationEmail {
}
/**
* Generate a Soy list of maps representing each line of the unified diff. The line maps will have
* a 'type' key which maps to one of 'common', 'add' or 'remove' and a 'text' key which maps to
* the line's content.
* Generate a list of maps representing each line of the unified diff. The line maps will have a
* 'type' key which maps to one of 'common', 'add' or 'remove' and a 'text' key which maps to the
* line's content.
*/
private SoyListData getDiffTemplateData() {
SoyListData result = new SoyListData();
private ImmutableList<ImmutableMap<String, String>> getDiffTemplateData() {
ImmutableList.Builder<ImmutableMap<String, String>> result = ImmutableList.builder();
Splitter lineSplitter = Splitter.on(System.getProperty("line.separator"));
for (String diffLine : lineSplitter.split(getUnifiedDiff())) {
SoyMapData lineData = new SoyMapData();
ImmutableMap.Builder<String, String> lineData = ImmutableMap.builder();
lineData.put("text", diffLine);
// Skip empty lines and lines that look like diff headers.
@@ -583,8 +583,8 @@ public abstract class ChangeEmail extends NotificationEmail {
break;
}
}
result.add(lineData);
result.add(lineData.build());
}
return result;
return result.build();
}
}

5
java/com/google/gerrit/server/plugincontext/PluginSetContext.java
View File

@@ -23,6 +23,7 @@ import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
import com.google.inject.Inject;
import java.util.Iterator;
import java.util.SortedSet;
import java.util.stream.Stream;
/**
* Context to invoke extensions from a {@link DynamicSet}.
@@ -147,6 +148,10 @@ public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
.forEach(p -> PluginContext.runLogExceptions(pluginMetrics, p, extensionImplConsumer));
}
public Stream<T> stream() {
return dynamicSet.stream();
}
/**
* Invokes each extension in the set. All exceptions from the plugin extensions except exceptions
* of the specified type are caught and logged.