Merge "Timer0: Allow to suppress logging"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 7d531ff..a179905 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -918,6 +918,9 @@
`on_behalf_of` field in link:rest-api-changes.html#submit-input[SubmitInput]
when link:rest-api-changes.html#submit-change[submitting using the REST API].
+The user in the `on_behalf_of` field, does not need to have `Submit` permission
+themselves, however they should be able to `read` the changes being submitted.
+
Note that this permission is named `submitAs` in the `project.config`
file.
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidationListener.java b/java/com/google/gerrit/server/git/validators/MergeValidationListener.java
index 79d53ac..a51a425 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidationListener.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidationListener.java
@@ -39,7 +39,8 @@
* @param destProject the destination project
* @param destBranch the destination branch
* @param patchSetId the patch set ID
- * @param caller the user who initiated the merge request
+ * @param caller the identity of the user that is recorded as the one performing the merge. In
+ * case of impersonation {@code caller.getRealUser()} contains the user triggering the merge.
* @throws MergeValidationException if the commit fails to validate
*/
void onPreMerge(
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index 710e688..c8a3d1e 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidators.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidators.java
@@ -196,7 +196,9 @@
if (!oldParent.equals(newParent)) {
if (!allowProjectOwnersToChangeParent) {
try {
- if (!permissionBackend.user(caller).test(GlobalPermission.ADMINISTRATE_SERVER)) {
+ if (!permissionBackend
+ .user(caller.getRealUser())
+ .test(GlobalPermission.ADMINISTRATE_SERVER)) {
throw new MergeValidationException(SET_BY_ADMIN);
}
} catch (PermissionBackendException e) {
@@ -206,7 +208,7 @@
} else {
try {
permissionBackend
- .user(caller)
+ .user(caller.getRealUser())
.project(destProject.getNameKey())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException e) {
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index 36b859c..6b35175 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.git.ObjectIds.abbreviateName;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.util.stream.Collectors.joining;
@@ -71,10 +72,10 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
-import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -86,6 +87,14 @@
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
+/**
+ * REST API handler for triggering submit of the specific revision of the change.
+ *
+ * <p>See /Documentation/rest-api-changes.html#submit-revision for more information.
+ *
+ * <p>Even though the endpoint is defined for url including a revision, only revision corresponding
+ * to the latest patch set is allowed.
+ */
@Singleton
public class Submit
implements RestModifyView<RevisionResource, SubmitInput>, UiAction<RevisionResource> {
@@ -99,8 +108,6 @@
"Submit all ${topicSize} changes of the same topic "
+ "(${submitSize} changes including ancestors and other "
+ "changes related by topic)";
- private static final String BLOCKED_HIDDEN_SUBMIT_TOOLTIP =
- "This change depends on other hidden changes which are not ready";
private static final String CLICK_FAILURE_TOOLTIP = "Clicking the button would fail";
private static final String CHANGE_UNMERGEABLE = "Problems with integrating this change";
@@ -180,12 +187,12 @@
input = new SubmitInput();
}
input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf);
- IdentifiedUser submitter;
+ IdentifiedUser submitter = rsrc.getUser().asIdentifiedUser();
+ // It's possible that the user does not have permission to submit all changes in the superset,
+ // but we check the current change for an early exit.
+ rsrc.permissions().check(ChangePermission.SUBMIT);
if (input.onBehalfOf != null) {
submitter = onBehalfOf(rsrc, input);
- } else {
- rsrc.permissions().check(ChangePermission.SUBMIT);
- submitter = rsrc.getUser().asIdentifiedUser();
}
projectCache
.get(rsrc.getProject())
@@ -213,9 +220,7 @@
}
try (MergeOp op = mergeOpProvider.get()) {
- Change updatedChange;
-
- updatedChange = op.merge(change, submitter, true, input, false);
+ Change updatedChange = op.merge(change, submitter, true, input, false);
if (updatedChange.isMerged()) {
return updatedChange;
}
@@ -240,48 +245,15 @@
*/
@Nullable
private String problemsForSubmittingChangeset(ChangeData cd, ChangeSet cs, CurrentUser user) {
- try {
- if (cs.furtherHiddenChanges()) {
- logger.atFine().log(
- "Change %d cannot be submitted by user %s because it depends on hidden changes: %s",
- cd.getId().get(), user.getLoggableName(), cs.nonVisibleChanges());
- return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
- }
- for (ChangeData c : cs.changes()) {
- Set<ChangePermission> can =
- permissionBackend
- .user(user)
- .change(c)
- .test(EnumSet.of(ChangePermission.READ, ChangePermission.SUBMIT));
- if (!can.contains(ChangePermission.READ)) {
- logger.atFine().log(
- "Change %d cannot be submitted by user %s because it depends on change %d which the user cannot read",
- cd.getId().get(), user.getLoggableName(), c.getId().get());
- return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
- }
- if (!can.contains(ChangePermission.SUBMIT)) {
- return "You don't have permission to submit change " + c.getId();
- }
- if (c.change().isWorkInProgress()) {
- return "Change " + c.getId() + " is marked work in progress";
- }
- try {
- // The data in the change index may be stale (e.g. if submit requirements have been
- // changed). For that one change for which the submit action is computed, use the
- // freshly loaded ChangeData instance 'cd' instead of the potentially stale ChangeData
- // instance 'c' that was loaded from the index. This makes a difference if the ChangeSet
- // 'cs' only contains this one single change. If the ChangeSet contains further changes
- // those may still be stale.
- MergeOp.checkSubmitRequirements(cd.getId().equals(c.getId()) ? cd : c);
- } catch (ResourceConflictException e) {
- return (c.getId() == cd.getId())
- ? String.format("Change %s is not ready: %s", cd.getId(), e.getMessage())
- : String.format(
- "Change %s must be submitted with change %s but %s is not ready: %s",
- cd.getId(), c.getId(), c.getId(), e.getMessage());
- }
- }
+ Optional<String> reason =
+ MergeOp.checkCommonSubmitProblems(cd.change(), cs, false, permissionBackend, user).stream()
+ .findFirst()
+ .map(MergeOp.ChangeProblem::getProblem);
+ if (reason.isPresent()) {
+ return reason.get();
+ }
+ try {
if (!useMergeabilityCheck) {
return null;
}
@@ -298,7 +270,7 @@
return "Problems with change(s): "
+ unmergeable.stream().map(c -> c.getId().toString()).collect(joining(", "));
}
- } catch (PermissionBackendException | IOException e) {
+ } catch (IOException e) {
logger.atSevere().withCause(e).log("Error checking if change is submittable");
throw new StorageException("Could not determine problems for the change", e);
}
@@ -330,7 +302,16 @@
ChangeSet cs =
mergeSuperSet
.get()
- .completeChangeSet(cd.change(), resource.getUser(), /*includingTopicClosure= */ false);
+ .completeChangeSet(cd.change(), resource.getUser(), /* includingTopicClosure= */ false);
+ // Replace potentially stale ChangeData for the current change with the fresher one.
+ cs =
+ new ChangeSet(
+ cs.changes().stream()
+ .map(csChange -> csChange.getId().equals(cd.getId()) ? cd : csChange)
+ .collect(toImmutableList()),
+ cs.nonVisibleChanges());
+ String submitProblems = problemsForSubmittingChangeset(cd, cs, resource.getUser());
+
String topic = change.getTopic();
int topicSize = 0;
if (!Strings.isNullOrEmpty(topic)) {
@@ -338,8 +319,6 @@
}
boolean treatWithTopic = submitWholeTopic && !Strings.isNullOrEmpty(topic) && topicSize > 1;
- String submitProblems = problemsForSubmittingChangeset(cd, cs, resource.getUser());
-
if (submitProblems != null) {
return new UiAction.Description()
.setLabel(treatWithTopic ? submitTopicLabel : (cs.size() > 1) ? labelWithParents : label)
@@ -464,7 +443,8 @@
throws AuthException, UnprocessableEntityException, PermissionBackendException, IOException,
ConfigInvalidException {
PermissionBackend.ForChange perm = rsrc.permissions();
- perm.check(ChangePermission.SUBMIT);
+ // It's possible that the current user or on-behalf-of user does not have permission for all
+ // changes in the superset, but we check the current change for an early exit.
perm.check(ChangePermission.SUBMIT_AS);
CurrentUser caller = rsrc.getUser();
@@ -476,9 +456,17 @@
throw new UnprocessableEntityException(
String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()), e);
}
+ logger.atFine().log(
+ "Change %d is being submitted by %s on behalf of %s",
+ rsrc.getChange().getChangeId(), rsrc.getUser().getUserName(), submitter.getUserName());
return submitter;
}
+ /**
+ * Change-level REST API endpoint that calls submit for the latest revision on a change.
+ *
+ * <p>See /Documentation/rest-api-changes.html#submit-change for more information.
+ */
public static class CurrentRevision implements RestModifyView<ChangeResource, SubmitInput> {
private final Submit submit;
private final PatchSetUtil psUtil;
diff --git a/java/com/google/gerrit/server/submit/MergeMetrics.java b/java/com/google/gerrit/server/submit/MergeMetrics.java
index b56d9ef..ed07f2f 100644
--- a/java/com/google/gerrit/server/submit/MergeMetrics.java
+++ b/java/com/google/gerrit/server/submit/MergeMetrics.java
@@ -54,32 +54,36 @@
.setRate());
}
- public void countChangesThatWereSubmittedWithRebaserApproval(ChangeData cd) {
- if (isRebaseOnBehalfOfUploader(cd)
- && hasCodeReviewApprovalOfRealUploader(cd)
- && !hasCodeReviewApprovalOfUserThatIsNotTheRealUploader(cd)
- && ignoresCodeReviewApprovalsOfUploader(cd)) {
- // 1. The patch set that is being submitted was created by rebasing on behalf of the uploader.
- // The uploader of the patch set is the original uploader on whom's behalf the rebase was
- // done. The real uploader is the user that did the rebase on behalf of the uploader (e.g. by
- // clicking on the rebase button).
- //
- // 2. The change has a Code-Review approval of the real uploader (aka the rebaser).
- //
- // 3. The change doesn't have a Code-Review approval of any other user (a user that is not the
- // real uploader).
- //
- // 4. Code-Review approvals of the uploader are ignored.
- //
- // If instead of a rebase on behalf of the uploader a normal rebase would have been done the
- // rebaser would have been the uploader of the patch set. In this case the Code-Review
- // approval of the rebaser would not have counted since Code-Review approvals of the uploader
- // are ignored.
- //
- // In this case we assume that the change would not be submittable if a normal rebase had been
- // done. This is not always correct (e.g. if there are approvals of multiple reviewers) but
- // it's good enough for the metric.
- countChangesThatWereSubmittedWithRebaserApproval.increment();
+ public void countChangesThatWereSubmittedWithRebaserApproval(ChangeSet cs) {
+ for (ChangeData cd : cs.changes()) {
+ if (isRebaseOnBehalfOfUploader(cd)
+ && hasCodeReviewApprovalOfRealUploader(cd)
+ && !hasCodeReviewApprovalOfUserThatIsNotTheRealUploader(cd)
+ && ignoresCodeReviewApprovalsOfUploader(cd)) {
+ // 1. The patch set that is being submitted was created by rebasing on behalf of the
+ // uploader.
+ //
+ // The uploader of the patch set is the original uploader on whose behalf the rebase was
+ // done. The real uploader is the user that did the rebase on behalf of the uploader (e.g.
+ // by clicking on the rebase button).
+ //
+ // 2. The change has a Code-Review approval of the real uploader (aka the rebaser).
+ //
+ // 3. The change doesn't have a Code-Review approval of any other user (a user that is not
+ // the real uploader).
+ //
+ // 4. Code-Review approvals of the uploader are ignored.
+ //
+ // If instead of a rebase on behalf of the uploader a normal rebase would have been done the
+ // rebaser would have been the uploader of the patch set. In this case the Code-Review
+ // approval of the rebaser would not have counted since Code-Review approvals of the
+ // uploader are ignored.
+ //
+ // In this case we assume that the change would not be submittable if a normal rebase had
+ // been done. This is not always correct (e.g. if there are approvals of multiple reviewers)
+ // but it's good enough for the metric.
+ countChangesThatWereSubmittedWithRebaserApproval.increment();
+ }
}
}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index eb41690..d49638a 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -69,6 +69,7 @@
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.change.NotifyResolver;
@@ -83,6 +84,8 @@
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
@@ -112,6 +115,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Deque;
+import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
@@ -150,6 +154,10 @@
private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_ALLOW_CLOSED =
SUBMIT_RULE_OPTIONS.toBuilder().recomputeOnClosedChanges(true).build();
+ /**
+ * For each individual change in merge set aggregates issues and other details throughout the
+ * merge process.
+ */
public static class CommitStatus {
private final ImmutableMap<Change.Id, ChangeData> changes;
private final ImmutableSetMultimap<BranchNameKey, Change.Id> byBranch;
@@ -292,6 +300,7 @@
private final ChangeData.Factory changeDataFactory;
private final StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory;
private final MergeMetrics mergeMetrics;
+ private final PermissionBackend permissionBackend;
// Changes that were updated by this MergeOp.
private final Map<Change.Id, Change> updatedChanges;
@@ -336,7 +345,8 @@
MergeMetrics mergeMetrics,
ProjectCache projectCache,
ExperimentFeatures experimentFeatures,
- @GerritServerConfig Config config) {
+ @GerritServerConfig Config config,
+ PermissionBackend permissionBackend) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.batchUpdates = batchUpdates;
@@ -362,6 +372,7 @@
hasImplicitMergeTimeoutSeconds =
ConfigUtil.getTimeUnit(
config, "change", null, "implicitMergeCalculationTimeout", 60, TimeUnit.SECONDS);
+ this.permissionBackend = permissionBackend;
}
@Override
@@ -371,6 +382,12 @@
}
}
+ /**
+ * Check that SRs are fulfilled or throw otherwise
+ *
+ * @param cd change that is being checked
+ * @throws ResourceConflictException the exception that is thrown if the SR is not fulfilled
+ */
public static void checkSubmitRequirements(ChangeData cd) throws ResourceConflictException {
PatchSet patchSet = cd.currentPatchSet();
if (patchSet == null) {
@@ -425,32 +442,177 @@
return cd.submitRecords(submitRuleOptions(/* allowClosed= */ false));
}
- private void checkSubmitRulesAndState(ChangeSet cs, boolean allowMerged)
- throws ResourceConflictException {
- checkArgument(
- !cs.furtherHiddenChanges(), "checkSubmitRulesAndState called for topic with hidden change");
- for (ChangeData cd : cs.changes()) {
- try {
- if (!cd.change().isNew()) {
- if (!(cd.change().isMerged() && allowMerged)) {
- commitStatus.problem(
- cd.getId(), "Change " + cd.getId() + " is " + ChangeUtil.status(cd.change()));
- }
- } else if (cd.change().isWorkInProgress()) {
- commitStatus.problem(cd.getId(), "Change " + cd.getId() + " is work in progress");
- } else {
- checkSubmitRequirements(cd);
- mergeMetrics.countChangesThatWereSubmittedWithRebaserApproval(cd);
- }
- } catch (ResourceConflictException e) {
- commitStatus.problem(cd.getId(), e.getMessage());
- } catch (StorageException e) {
- String msg = "Error checking submit rules for change";
- logger.atWarning().withCause(e).log("%s %s", msg, cd.getId());
- commitStatus.problem(cd.getId(), msg);
- }
+ /** A problem preventing merge and change on which it occurred. */
+ @AutoValue
+ public abstract static class ChangeProblem {
+ public abstract Change.Id getChangeId();
+
+ public abstract String getProblem();
+
+ public static ChangeProblem create(Change.Id changeId, String problem) {
+ return new AutoValue_MergeOp_ChangeProblem(changeId, problem);
}
+ }
+
+ private static void addProblemForChange(
+ Change.Id triggeringChangeId,
+ ChangeData cd,
+ boolean allowMerged,
+ PermissionBackend permissionBackend,
+ CurrentUser caller,
+ ImmutableList.Builder<ChangeProblem> problems) {
+ try {
+ Set<ChangePermission> can =
+ permissionBackend
+ .user(caller.getRealUser())
+ .change(cd)
+ .test(
+ EnumSet.of(
+ ChangePermission.READ, ChangePermission.SUBMIT, ChangePermission.SUBMIT_AS));
+ if (!can.contains(ChangePermission.READ)) {
+ // The READ permission should already be handled during generation of ChangeSet, however
+ // MergeSuperSetComputation is an interface and on API level doesn't guarantee that this
+ // have been verified for all changes. Additionally, this protects against potential
+ // issues due to staleness.
+ logger.atFine().log(
+ "Change %d cannot be submitted by user %s because it depends on change %d which the"
+ + "user cannot read",
+ triggeringChangeId.get(), caller.getRealUser().getLoggableName(), cd.getId().get());
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format(
+ "Change %d depends on other hidden changes", triggeringChangeId.get())));
+ return;
+ }
+ if (!can.contains(ChangePermission.SUBMIT)) {
+ logger.atFine().log(
+ "Change %d cannot be submitted by user %s because it depends on change %d which the"
+ + "user cannot submit",
+ triggeringChangeId.get(), caller.getRealUser().getLoggableName(), cd.getId().get());
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format("Insufficient permission to submit change %d", cd.getId().get())));
+ return;
+ }
+ if (caller.isImpersonating()) {
+ if (!permissionBackend.user(caller).change(cd).test(ChangePermission.READ)) {
+ logger.atFine().log(
+ "Change %d cannot be submitted by user %s on behalf of user %s because it depends on"
+ + " change %d which the on-behalf-of user does not have READ permission for",
+ triggeringChangeId.get(),
+ caller.getRealUser().getLoggableName(),
+ caller.getLoggableName(),
+ cd.getId().get());
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format(
+ "On-behalf-of user %s lacks permission to read change %d",
+ caller.getLoggableName(), cd.getId().get())));
+ return;
+ }
+ if (!can.contains(ChangePermission.SUBMIT_AS)) {
+ logger.atFine().log(
+ "Change %d cannot be submitted by user %s on behalf of user %s because it depends on"
+ + " change %d which the user does not have SUBMIT_AS permission for",
+ triggeringChangeId.get(),
+ caller.getRealUser().getLoggableName(),
+ caller.getLoggableName(),
+ cd.getId().get());
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format(
+ "Insufficient permission to submit change %d on behalf of user %s",
+ cd.getId().get(), caller.getLoggableName())));
+ return;
+ }
+ }
+ if (!cd.change().isNew()) {
+ if (!(cd.change().isMerged() && allowMerged)) {
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format(
+ "Change %d is %s", cd.getId().get(), ChangeUtil.status(cd.change()))));
+ return;
+ }
+ }
+ if (cd.change().isWorkInProgress()) {
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ String.format("Change %d is marked work in progress", cd.getId().get())));
+ return;
+ }
+ try {
+ checkSubmitRequirements(cd);
+ } catch (ResourceConflictException e) {
+ // ResourceConflictException is thrown means submit requirement is not fulfilled.
+ problems.add(
+ ChangeProblem.create(
+ cd.getId(),
+ triggeringChangeId.equals(cd.getId())
+ ? String.format("Change %s is not ready: %s", cd.getId(), e.getMessage())
+ : String.format(
+ "Change %s must be submitted with change %s but %s is not ready: %s",
+ triggeringChangeId, cd.getId(), cd.getId(), e.getMessage())));
+ }
+ } catch (StorageException | PermissionBackendException e) {
+ String msg = "Error checking submit rules for change";
+ logger.atWarning().withCause(e).log("%s %s", msg, triggeringChangeId);
+ problems.add(ChangeProblem.create(cd.getId(), msg));
+ }
+ }
+
+ /**
+ * Returns a list of messages describing what prevents the current change from being submitted.
+ *
+ * <p>The method checks all changes in the {@code cs} for their current status, submitability and
+ * permissions and returns one change per change in the set that can't be submitted.
+ *
+ * @param triggeringChange Change for which merge/submit action was initiated
+ * @param cs Set of changes that the current change depends on
+ * @param allowMerged True if change being already merged is not a problem to be reported
+ * @param permissionBackend Interface for checking user ACLs
+ * @param caller the identity of the user that is recorded as the one performing the merge. In
+ * case of impersonation {@code caller.getRealUser()} contains the user triggering the merge.
+ * @return List of problems preventing merge
+ */
+ public static ImmutableList<ChangeProblem> checkCommonSubmitProblems(
+ Change triggeringChange,
+ ChangeSet cs,
+ boolean allowMerged,
+ PermissionBackend permissionBackend,
+ CurrentUser caller) {
+ ImmutableList.Builder<ChangeProblem> problems = ImmutableList.builder();
+ if (cs.furtherHiddenChanges()) {
+ logger.atFine().log(
+ "Change %d cannot be submitted by user %s because it depends on hidden changes: %s",
+ triggeringChange.getId().get(),
+ caller.getRealUser().getLoggableName(),
+ cs.nonVisibleChanges());
+ problems.add(
+ ChangeProblem.create(
+ triggeringChange.getId(),
+ String.format(
+ "Change %d depends on other hidden changes", triggeringChange.getId().get())));
+ }
+ for (ChangeData cd : cs.changes()) {
+ addProblemForChange(
+ triggeringChange.getId(), cd, allowMerged, permissionBackend, caller, problems);
+ }
+ return problems.build();
+ }
+
+ private void checkSubmitRulesAndState(Change triggeringChange, ChangeSet cs, boolean allowMerged)
+ throws ResourceConflictException {
+ checkCommonSubmitProblems(triggeringChange, cs, allowMerged, permissionBackend, caller).stream()
+ .forEach(cp -> commitStatus.problem(cp.getChangeId(), cp.getProblem()));
commitStatus.maybeFailVerbose();
+ mergeMetrics.countChangesThatWereSubmittedWithRebaserApproval(cs);
}
private void bypassSubmitRulesAndRequirements(ChangeSet cs) {
@@ -490,14 +652,21 @@
* integration strategy.
*
* @param change the change to be merged.
- * @param caller the identity of the caller
+ * @param caller the identity of the user that is recorded as the one performing the merge. In
+ * case of impersonation {@code caller.getRealUser()} contains the user triggering the merge.
* @param checkSubmitRules whether the prolog submit rules should be evaluated
* @param submitInput parameters regarding the merge
+ * @param dryrun if true, this includes calculating all projects affected by the submission,
+ * checking for possible submission problems (ACLs, merge conflicts, etc) but not the merge
+ * itself.
* @throws RestApiException if an error occurred.
* @throws PermissionBackendException if permissions can't be checked
* @throws IOException an error occurred reading from NoteDb.
* @return the merged change
*/
+ // TODO: dryrun was introduced in https://gerrit-review.git.corp.google.com/c/gerrit/+/82911 and
+ // has never been used. Consider removing it. Since it was never used and this file has been
+ // through many refactorings since, it's likely that the implementation is broken.
@CanIgnoreReturnValue
public Change merge(
Change change,
@@ -580,7 +749,7 @@
this.commitStatus = new CommitStatus(filteredNoteDbChangeSet, isRetry);
if (checkSubmitRules) {
logger.atFine().log("Checking submit rules and state");
- checkSubmitRulesAndState(filteredNoteDbChangeSet, isRetry);
+ checkSubmitRulesAndState(change, filteredNoteDbChangeSet, isRetry);
} else {
logger.atFine().log("Bypassing submit rules");
bypassSubmitRulesAndRequirements(filteredNoteDbChangeSet);
@@ -631,7 +800,7 @@
Change reloadChange = change;
ChangeSet indexBackedMergeChangeSet =
mergeSuperSet.completeChangeSet(
- reloadChange, caller, /* includingTopicClosure= */ false);
+ reloadChange, caller.getRealUser(), /* includingTopicClosure= */ false);
if (!indexBackedMergeChangeSet.ids().contains(reloadChange.getId())) {
// indexBackedChangeSet contains only open changes, if the change is missing
// in this set it might be that the change was concurrently submitted in the
@@ -1023,7 +1192,8 @@
.map(c -> ObjectId.toString(c))
.collect(joining(", "));
logger.atWarning().log(
- "Timeout during hasImplicitMerge calculation. Number of iterations: %s, commitsToSubmit: %s",
+ "Timeout during hasImplicitMerge calculation. Number of iterations: %s,"
+ + " commitsToSubmit: %s",
iterationCount, allCommits);
return true;
}
diff --git a/java/com/google/gerrit/server/update/BatchUpdates.java b/java/com/google/gerrit/server/update/BatchUpdates.java
index 2f9ef84..4716246 100644
--- a/java/com/google/gerrit/server/update/BatchUpdates.java
+++ b/java/com/google/gerrit/server/update/BatchUpdates.java
@@ -48,6 +48,13 @@
import java.util.Objects;
import java.util.function.Function;
+/**
+ * Runs execute methods of a collection of {@link BatchUpdate}s calling listeners when appropriate.
+ *
+ * <p>This class does not maintain any state about the updates it executes. The only reason it is
+ * non-static is to provide convenient access to {@link ChangeData.Factory} without needing to
+ * provide one as an argument.
+ */
@Singleton
public class BatchUpdates {
public class Result {
diff --git a/java/com/google/gerrit/server/update/SubmissionExecutor.java b/java/com/google/gerrit/server/update/SubmissionExecutor.java
index 762de57..ff46181 100644
--- a/java/com/google/gerrit/server/update/SubmissionExecutor.java
+++ b/java/com/google/gerrit/server/update/SubmissionExecutor.java
@@ -21,6 +21,7 @@
import java.util.Optional;
import java.util.stream.Collectors;
+/** Wrapper class for calling BatchUpdates.execute() that manages calls to submission listeners. */
public class SubmissionExecutor {
private final BatchUpdates batchUpdates;
private final ImmutableList<SubmissionListener> submissionListeners;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java b/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java
index fc8eaed..9af7a2d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java
@@ -41,7 +41,8 @@
.isEqualTo(
String.format(
"Failed to submit 1 change due to the following problems:\n"
- + "Change %s: submit requirement 'No-Unresolved-Comments' is unsatisfied.",
+ + "Change %1$s: Change %1$s is not ready: "
+ + "submit requirement 'No-Unresolved-Comments' is unsatisfied.",
r.getChange().getId().get()));
// Resolve the comment and check that the change can be submitted now.
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 2e706b8..7de689d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -689,6 +689,15 @@
}
@Test
+ @UseLocalDisk
+ public void submitOnBehalfOf_usersSubmitPermissionIgnored() throws Exception {
+ // user is part of the newGroup, block their submit permission
+ blockSubmit(newGroup);
+
+ testSubmitOnBehalfOf(project, admin2, user);
+ }
+
+ @Test
public void submitOnBehalfOfFailsWhenUserCannotSeeDestinationRef() throws Exception {
blockRead(newGroup);
@@ -926,6 +935,14 @@
.update();
}
+ private void blockSubmit(GroupInfo group) throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.SUBMIT).ref("refs/heads/*").group(AccountGroup.uuid(group.id)))
+ .update();
+ }
+
private void allowRunAs() throws Exception {
projectOperations
.allProjectsForUpdate()
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 25af040..0b86406 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -589,7 +589,7 @@
+ num
+ ": Change "
+ num
- + " is work in progress");
+ + " is marked work in progress");
}
@Test
@@ -607,7 +607,7 @@
+ num
+ ": Change "
+ num
- + " is work in progress");
+ + " is marked work in progress");
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
index 1d8e0b8..5f1a982 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
@@ -95,12 +95,15 @@
PushOneCommit.Result change2 = createChange();
Change.Id id1 = change1.getPatchSetId().changeId();
+ Change.Id id2 = change2.getPatchSetId().changeId();
submitWithConflict(
change2.getChangeId(),
- "Failed to submit 2 changes due to the following problems:\n"
- + "Change "
- + id1
- + ": submit requirement 'Code-Review' is unsatisfied.");
+ String.format(
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change %d"
+ + ": Change %d must be submitted with change %d but %d is not ready: "
+ + "submit requirement 'Code-Review' is unsatisfied.",
+ id1.get(), id2.get(), id1.get(), id1.get()));
RevCommit updatedHead = projectOperations.project(project).getHead("master");
assertThat(updatedHead.getId()).isEqualTo(initialHead.getId());
diff --git a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
index 5a4f073..6fabd1a 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
@@ -16,22 +16,35 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo;
+import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.FileInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Inject;
import java.util.EnumSet;
@@ -336,6 +349,174 @@
assertSubmittedTogether(id2, id2, id1);
}
+ @Test
+ public void permissionToSubmitForSomeChangesInTopic() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.SUBMIT).ref("refs/heads/testbranch").group(REGISTERED_USERS))
+ .update();
+
+ createBranch(BranchNameKey.create(getProject(), "testbranch"));
+ RevCommit initialHead = projectOperations.project(project).getHead("master");
+ // Create two independent commits and push.
+ RevCommit c1_1 = commitBuilder().add("a.txt", "1").message("subject: 1").create();
+ String id1 = getChangeId(c1_1);
+ pushHead(testRepo, "refs/for/master%topic=" + name("connectingTopic"), false);
+
+ testRepo.reset(initialHead);
+ RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create();
+ String id2 = getChangeId(c2_1);
+ pushHead(testRepo, "refs/for/testbranch%topic=" + name("connectingTopic"), false);
+
+ approve(id1);
+ approve(id2);
+ if (isSubmitWholeTopicEnabled()) {
+ ResourceConflictException e =
+ assertThrows(ResourceConflictException.class, () -> submit(id1));
+ assertThat(e.getMessage())
+ .contains(
+ String.format(
+ "Insufficient permission to submit change %d",
+ gApi.changes().id(id2).get()._number));
+ } else {
+ submit(id1);
+ }
+ }
+
+ @Test
+ public void permissionToSubmitAsForSomeChangesInTopic() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.SUBMIT_AS).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(block(Permission.SUBMIT_AS).ref("refs/heads/testbranch").group(REGISTERED_USERS))
+ .update();
+
+ createBranch(BranchNameKey.create(getProject(), "testbranch"));
+ RevCommit initialHead = projectOperations.project(project).getHead("master");
+ // Create two independent commits and push.
+ RevCommit c1_1 = commitBuilder().add("a.txt", "1").message("subject: 1").create();
+ String id1 = getChangeId(c1_1);
+ pushHead(testRepo, "refs/for/master%topic=" + name("connectingTopic"), false);
+
+ testRepo.reset(initialHead);
+ RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create();
+ String id2 = getChangeId(c2_1);
+ pushHead(testRepo, "refs/for/testbranch%topic=" + name("connectingTopic"), false);
+
+ approve(id1);
+ approve(id2);
+ SubmitInput in = new SubmitInput();
+ in.onBehalfOf = accountCreator.user2().email();
+ if (isSubmitWholeTopicEnabled()) {
+ ResourceConflictException e =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(id1).current().submit(in));
+ assertThat(e.getMessage())
+ .contains(
+ String.format(
+ "Insufficient permission to submit change %d on behalf of user %s",
+ gApi.changes().id(id2).get()._number, accountCreator.user2().username()));
+ } else {
+ gApi.changes().id(id1).current().submit(in);
+ assertMerged(id1);
+ assertNotMerged(id2);
+ }
+ }
+
+ @Test
+ public void submitAs_onBehalfOfUserNoReadPermissionToSomeChanges() throws Exception {
+ GroupInfo newGroup = createGroupForUser(accountCreator.user2());
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ block(Permission.READ)
+ .ref("refs/heads/testbranch")
+ .group(AccountGroup.uuid(newGroup.id)))
+ .add(allow(Permission.SUBMIT_AS).ref("refs/heads/*").group(REGISTERED_USERS))
+ .update();
+
+ createBranch(BranchNameKey.create(getProject(), "testbranch"));
+ RevCommit initialHead = projectOperations.project(project).getHead("master");
+ // Create two independent commits and push.
+ RevCommit c1_1 = commitBuilder().add("a.txt", "1").message("subject: 1").create();
+ String id1 = getChangeId(c1_1);
+ pushHead(testRepo, "refs/for/master%topic=" + name("connectingTopic"), false);
+
+ testRepo.reset(initialHead);
+ RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create();
+ String id2 = getChangeId(c2_1);
+ pushHead(testRepo, "refs/for/testbranch%topic=" + name("connectingTopic"), false);
+
+ approve(id1);
+ approve(id2);
+ SubmitInput in = new SubmitInput();
+ in.onBehalfOf = accountCreator.user2().email();
+ if (isSubmitWholeTopicEnabled()) {
+ ResourceConflictException e =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(id1).current().submit(in));
+ assertThat(e.getMessage())
+ .contains(
+ String.format(
+ "On-behalf-of user %s lacks permission to read change %d",
+ accountCreator.user2().username(), gApi.changes().id(id2).get()._number));
+ } else {
+ gApi.changes().id(id1).current().submit(in);
+ assertMerged(id1);
+ assertNotMerged(id2);
+ }
+ }
+
+ @Test
+ public void submitAs_succeeds() throws Exception {
+ GroupInfo newGroup = createGroupForUser(accountCreator.user2());
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ block(Permission.SUBMIT)
+ .ref("refs/heads/testbranch")
+ .group(AccountGroup.uuid(newGroup.id)))
+ .add(allow(Permission.SUBMIT_AS).ref("refs/heads/*").group(REGISTERED_USERS))
+ .update();
+
+ createBranch(BranchNameKey.create(getProject(), "testbranch"));
+ RevCommit initialHead = projectOperations.project(project).getHead("master");
+ // Create two independent commits and push.
+ RevCommit c1_1 = commitBuilder().add("a.txt", "1").message("subject: 1").create();
+ String id1 = getChangeId(c1_1);
+ pushHead(testRepo, "refs/for/master%topic=" + name("connectingTopic"), false);
+
+ testRepo.reset(initialHead);
+ RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create();
+ String id2 = getChangeId(c2_1);
+ pushHead(testRepo, "refs/for/testbranch%topic=" + name("connectingTopic"), false);
+
+ approve(id1);
+ approve(id2);
+ SubmitInput in = new SubmitInput();
+ in.onBehalfOf = accountCreator.user2().email();
+ if (isSubmitWholeTopicEnabled()) {
+ gApi.changes().id(id1).current().submit(in);
+ assertMerged(id1);
+ assertMerged(id2);
+ } else {
+ gApi.changes().id(id1).current().submit(in);
+ assertMerged(id1);
+ assertNotMerged(id2);
+ }
+ }
+
+ private GroupInfo createGroupForUser(TestAccount user) throws Exception {
+ GroupInput gi = new GroupInput();
+ gi.name = name("New-Group");
+ gi.members = ImmutableList.of(user.id().toString());
+ return gApi.groups().create(gi).get();
+ }
+
private String getChangeId(RevCommit c) throws Exception {
return GitUtil.getChangeId(testRepo, c).get();
}
@@ -347,4 +528,8 @@
private void assertMerged(String changeId) throws Exception {
assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED);
}
+
+ private void assertNotMerged(String changeId) throws Exception {
+ assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.NEW);
+ }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
index 642610a..1caaebd 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
@@ -573,11 +573,11 @@
// Note that `this.layersApplied` will wipe away the <gr-diff-text>, and
// another rendering cycle will be initiated in `updated()`.
// prettier-ignore
- const textElement = line?.text && !this.layersApplied
+ const textElement = !this.layersApplied
? html`<gr-diff-text
${ref(this.contentRef(side))}
data-side=${ifDefined(side)}
- .text=${line?.text}
+ .text=${line?.text ?? ''}
.tabSize=${this.tabSize}
.lineLimit=${this.lineLength}
.isResponsive=${isResponsive(this.responsiveMode)}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
index 1526ce3..346d3fe 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
@@ -164,7 +164,9 @@
>
<td class="blankLineNum left"></td>
<td class="blank left no-intraline-info">
- <div class="contentText" data-side="left"></div>
+ <div class="contentText" data-side="left">
+ <gr-diff-text data-side="left"></gr-diff-text>
+ </div>
</td>
<td class="lineNum right" data-value="1">
<button
@@ -226,7 +228,9 @@
</td>
<td class="blankLineNum right"></td>
<td class="blank no-intraline-info right">
- <div class="contentText" data-side="right"></div>
+ <div class="contentText" data-side="right">
+ <gr-diff-text data-side="right"></gr-diff-text>
+ </div>
</td>
</tr>
<slot name="post-left-line-1"></slot>
