Merge changes Ia82a9abe,Ie2ab916d,I0be4856f,If0b7fe4c,I15647018, ... * changes: PerformanceMetrics: Use cfg section that doesn't conflict with tracing cfg Stop using LazyArgs for logging operation metadata RestApiServlet: Remove usage of LazyArgs to log response JSON Remove unnecessary usage of LazyArgs for logging Drop remaining debug logs for known groups Warn about too broad tracing configs Disallow tracing configs that trigger tracing for too many requests
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/Documentation/user-upload.txt b/Documentation/user-upload.txt index 625c2e9..7cc9e29 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt
@@ -205,6 +205,15 @@ notify them of new changes will be automatically sent an email message when the push is completed. +Pushing for review requires that the target branch exists, except for +the branch to which "HEAD" points, e.g. "master", and the +"link:config-project-config.html#refs-meta-config[refs/meta/config]" +branch that contains the link:config-project-config.html[project +configuration]. For these branches Gerrit allows pushing an initial +commit for review even if they don't exist yet. The push creates a +change for the initial commit and when this change gets submitted the +target branch gets created automatically. + [[push_options]] === Push Options
diff --git a/java/com/google/gerrit/metrics/Timer0.java b/java/com/google/gerrit/metrics/Timer0.java index 2d91515..a18a6ed 100644 --- a/java/com/google/gerrit/metrics/Timer0.java +++ b/java/com/google/gerrit/metrics/Timer0.java
@@ -49,6 +49,8 @@ } } + private boolean suppressLogging; + protected final String name; public Timer0(String name) { @@ -74,14 +76,22 @@ public final void record(long value, TimeUnit unit) { long durationNanos = unit.toNanos(value); - LoggingContext.getInstance() - .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos)); - logger.atFinest().log("%s took %.2f ms", name, durationNanos / 1000000.0); + if (!suppressLogging) { + LoggingContext.getInstance() + .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos)); + logger.atFinest().log("%s took %.2f ms", name, durationNanos / 1000000.0); + } doRecord(value, unit); RequestStateContext.abortIfCancelled(); } + /** Suppress logging (debug log and performance log) when values are recorded. */ + public final Timer0 suppressLogging() { + this.suppressLogging = true; + return this; + } + /** * Record a value in the distribution. *
diff --git a/java/com/google/gerrit/server/comment/CommentContextKey.java b/java/com/google/gerrit/server/comment/CommentContextKey.java index ce9aa78..2ba74cc 100644 --- a/java/com/google/gerrit/server/comment/CommentContextKey.java +++ b/java/com/google/gerrit/server/comment/CommentContextKey.java
@@ -64,7 +64,7 @@ public abstract Builder patchset(Integer patchset); - public abstract Builder contextPadding(Integer numLines); + public abstract Builder contextPadding(int numLines); public abstract CommentContextKey build(); }
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/patch/gitfilediff/GitFileDiffCacheKey.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java index 2e18e93..d127817 100644 --- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java +++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
@@ -87,7 +87,7 @@ public abstract Builder newFilePath(String value); - public abstract Builder renameScore(Integer value); + public abstract Builder renameScore(int value); public Builder disableRenameDetection() { renameScore(-1);
diff --git a/java/com/google/gerrit/server/project/CreateRefControl.java b/java/com/google/gerrit/server/project/CreateRefControl.java index ab134b5..c3d3978 100644 --- a/java/com/google/gerrit/server/project/CreateRefControl.java +++ b/java/com/google/gerrit/server/project/CreateRefControl.java
@@ -112,7 +112,9 @@ RefPermission.READ.describeForException())); throw e; } - } else if (object instanceof RevTag) { + return; + } + if (object instanceof RevTag) { RevTag tag = (RevTag) object; try (RevWalk rw = new RevWalk(repo)) { rw.parseBody(tag); @@ -145,7 +147,11 @@ } else { forRef.check(RefPermission.CREATE_TAG); } + return; } + throw new AuthException( + String.format( + "Ref creation not allowed. Object %s is neither Commit or Tag.", object.getId())); } /**
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/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java index f94aa12..fa6d2e4 100644 --- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
@@ -45,6 +45,7 @@ import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.RemoteRefUpdate; @@ -405,6 +406,33 @@ assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } + @Test + public void pushTreeIsNotAllowed() throws Exception { + RevCommit commit = testRepo.branch("HEAD").commit().create(); + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.CREATE).ref("refs/*").group(REGISTERED_USERS)) + .add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS)) + .update(); + + // We use "refs/main" instead of "refs/heads/main", because the latter only allows commits. + { + // An extra colon (:) makes it a tree reference + PushResult r = push(commit.getId().getName() + "::refs/main"); + RemoteRefUpdate refUpdate = r.getRemoteUpdates().stream().findFirst().get(); + assertThat(refUpdate.getStatus()).isEqualTo(Status.REJECTED_OTHER_REASON); + assertThat(refUpdate.getMessage()).contains("is neither Commit or Tag"); + } + + { + PushResult r = push(commit.getTree().getId().getName() + ":refs/main"); + RemoteRefUpdate refUpdate = r.getRemoteUpdates().stream().findFirst().get(); + assertThat(refUpdate.getStatus()).isEqualTo(Status.REJECTED_OTHER_REASON); + assertThat(refUpdate.getMessage()).contains("is neither Commit or Tag"); + } + } + private static void removeAllBranchPermissions(ProjectConfig cfg, String... permissions) { for (AccessSection s : cfg.getAccessSections()) { if (s.getName().startsWith("refs/heads/")
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/plugins/replication b/plugins/replication index 3982574..cf0aea5 160000 --- a/plugins/replication +++ b/plugins/replication
@@ -1 +1 @@ -Subproject commit 3982574d28f254b525b093c2ecae8caa47825910 +Subproject commit cf0aea56d2efad79bcb84df37edc32c01bf8afb7
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts index 90cf17b..945afe4 100644 --- a/polygerrit-ui/app/api/diff.ts +++ b/polygerrit-ui/app/api/diff.ts
@@ -306,12 +306,12 @@ code_range: LineRange; } -export interface FileRange { +export declare interface FileRange { basePath?: string; path: string; } -export interface PatchRange { +export declare interface PatchRange { patchNum: RevisionPatchSetNum; basePatchNum: BasePatchSetNum; }
diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts index a99771b..044693e 100644 --- a/polygerrit-ui/app/api/rest-api.ts +++ b/polygerrit-ui/app/api/rest-api.ts
@@ -522,7 +522,7 @@ * The CommentInfo entity contains information about an inline comment. * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-info */ -export interface CommentInfo { +export declare interface CommentInfo { id: UrlEncodedCommentId; updated: Timestamp; // TODO(TS): Make this required. Every comment must have patch_set set. @@ -613,7 +613,7 @@ * The ContextLine entity contains the line number and line text of a single line of the source file content. * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#context-line */ -export interface ContextLine { +export declare interface ContextLine { line_number: number; context_line: string; } @@ -1076,8 +1076,6 @@ change: ChangeConfigInfo; download: DownloadInfo; gerrit: GerritInfo; - // docs mentions index property, but it doesn't exists in Java class - // index: IndexConfigInfo; note_db_enabled?: boolean; plugin: PluginConfigInfo; receive?: ReceiveInfo; @@ -1301,7 +1299,7 @@ * The FixSuggestionInfo entity represents a suggested fix * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#fix-suggestion-info */ -export interface FixSuggestionInfoInput { +export declare interface FixSuggestionInfoInput { description: string; replacements: FixReplacementInfo[]; } @@ -1310,7 +1308,7 @@ * The FixReplacementInfo entity describes how the content of a file should be replaced by another content * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#fix-replacement-info */ -export interface FixReplacementInfo { +export declare interface FixReplacementInfo { path: string; range: CommentRange; replacement: string; @@ -1318,7 +1316,7 @@ // The UUID of the suggested fix. export type FixId = BrandType<string, '_fixId'>; -export interface FixSuggestionInfo extends FixSuggestionInfoInput { +export declare interface FixSuggestionInfo extends FixSuggestionInfoInput { fix_id: FixId; description: string; replacements: FixReplacementInfo[];
diff --git a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts index 1285664..4f4963a 100644 --- a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts +++ b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
@@ -30,7 +30,6 @@ @customElement('gr-commit-info') export class GrCommitInfo extends LitElement { - // TODO(TS): Maybe limit to StandaloneCommitInfo. @property({type: Object}) commitInfo?: Partial<CommitInfo>;
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts index 720899f..d227b9d 100644 --- a/polygerrit-ui/app/types/common.ts +++ b/polygerrit-ui/app/types/common.ts
@@ -54,8 +54,6 @@ ConfigParameterInfoBase, ContextLine, ContributorAgreementInfo, - CustomKey, - CustomKeyedValues, DetailedLabelInfo, DownloadInfo, DownloadSchemeInfo, @@ -464,40 +462,11 @@ visible_to_all: boolean; } -/** - * The GroupsInput entity contains information about groups that should be - * included into a group or that should be deleted from a group. - * https://gerrit-review.googlesource.com/Documentation/rest-api-groups.html - */ -export interface GroupsInput { - _one_group?: string; - groups?: string[]; -} - -/** - * The MembersInput entity contains information about accounts that should be - * added as members to a group or that should be deleted from the group. - * https://gerrit-review.googlesource.com/Documentation/rest-api-groups.html - */ -export interface MembersInput { - _one_member?: string; - members?: string[]; -} - export interface CommitInfoWithRequiredCommit extends CommitInfo { commit: CommitId; } /** - * Standalone Commit Info. - * Same as CommitInfo, except `commit` is required - * as it is only optional when used inside of the RevisionInfo. - */ -export interface StandaloneCommitInfo extends CommitInfo { - commit: CommitId; -} - -/** * The GpgKeysInput entity contains information for adding/deleting GPG keys. * https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#gpg-keys-input */ @@ -507,28 +476,6 @@ } /** - * The CacheInfo entity contains information about a cache. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface CacheInfo { - name: string; - type: string; - entries: EntriesInfo; - average_get?: string; - hit_ratio: HitRatioInfo; -} - -/** - * The CacheOperationInput entity contains information about an operation that - * should be executed on caches. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface CacheOperationInput { - operation: string; - caches?: string[]; -} - -/** * The CapabilityInfo entity contains information about a capability. * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html#capability-info */ @@ -540,217 +487,6 @@ export type CapabilityInfoMap = {[id: string]: CapabilityInfo}; /** - * The ChangeIndexConfigInfo entity contains information about Gerrit - * configuration from the index.change section. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html#change-index-config-info - */ -export interface ChangeIndexConfigInfo { - index_mergeable?: boolean; -} - -/** - * The CheckAccountExternalIdsResultInfo entity contains the result of running - * the account external ID consistency check. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface CheckAccountExternalIdsResultInfo { - problems: string; -} - -/** - * The CheckAccountsResultInfo entity contains the result of running the account - * consistency check. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface CheckAccountsResultInfo { - problems: string; -} - -/** - * The CheckGroupsResultInfo entity contains the result of running the group - * consistency check. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface CheckGroupsResultInfo { - problems: string; -} - -/** - * The ConsistencyCheckInfo entity contains the results of running consistency - * checks. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ConsistencyCheckInfo { - check_accounts_result?: CheckAccountsResultInfo; - check_account_external_ids_result?: CheckAccountExternalIdsResultInfo; - check_groups_result?: CheckGroupsResultInfo; -} - -/** - * The ConsistencyCheckInput entity contains information about which consistency - * checks should be run. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ConsistencyCheckInput { - check_accounts?: string; - check_account_external_ids?: string; - check_groups?: string; -} - -/** - * The ConsistencyProblemInfo entity contains information about a consistency - * problem. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ConsistencyProblemInfo { - status: string; - message: string; -} - -/** - * The entity describes the result of a reload of gerrit.config. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ConfigUpdateInfo { - applied: string; - rejected: string; -} - -/** - * The entity describes an updated config value. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ConfigUpdateEntryInfo { - config_key: string; - old_value: string; - new_value: string; -} - -/** - * The EmailConfirmationInput entity contains information for confirming an - * email address. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface EmailConfirmationInput { - token: string; -} - -/** - * The EntriesInfo entity contains information about the entries in acache. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface EntriesInfo { - mem?: string; - disk?: string; - space?: string; -} - -/** - * The IndexConfigInfo entity contains information about Gerrit configuration - * from the index section. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html#index-config-info - */ -export interface IndexConfigInfo { - change: ChangeIndexConfigInfo; -} - -/** - * The HitRatioInfo entity contains information about the hit ratio of a cache. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface HitRatioInfo { - mem: string; - disk?: string; -} - -/** - * The IndexChangesInput contains a list of numerical changes IDs to index. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface IndexChangesInput { - changes: string; -} - -/** - * The JvmSummaryInfo entity contains information about the JVM. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface JvmSummaryInfo { - vm_vendor: string; - vm_name: string; - vm_version: string; - os_name: string; - os_version: string; - os_arch: string; - user: string; - host?: string; - current_working_directory: string; - site: string; -} - -/** - * The MemSummaryInfo entity contains information about the current memory - * usage. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface MemSummaryInfo { - total: string; - used: string; - free: string; - buffers: string; - max: string; - open_files?: string; -} - -/** - * The SummaryInfo entity contains information about the current state of the - * server. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface SummaryInfo { - task_summary: TaskSummaryInfo; - mem_summary: MemSummaryInfo; - thread_summary: ThreadSummaryInfo; - jvm_summary?: JvmSummaryInfo; -} - -/** - * The TaskInfo entity contains information about a task in a background work - * queue. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface TaskInfo { - id: string; - state: string; - start_time: string; - delay: string; - command: string; - remote_name?: string; - project?: string; -} - -/** - * The TaskSummaryInfo entity contains information about the current tasks. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface TaskSummaryInfo { - total?: string; - running?: string; - ready?: string; - sleeping?: string; -} - -/** - * The ThreadSummaryInfo entity contains information about the current threads. - * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html - */ -export interface ThreadSummaryInfo { - cpus: string; - threads: string; - counts: string; -} - -/** * The TopMenuEntryInfo entity contains information about a top menu entry. * https://gerrit-review.googlesource.com/Documentation/rest-api-config.html#top-menu-entry-info */ @@ -1177,15 +913,6 @@ } /** - * The CustomKeyedValuesInput entity contains information about hashtags to add to, and/or remove from, a change - * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#custom-keyed-values-input - */ -export interface CustomKeyedValuesInput { - add?: CustomKeyedValues; - remove?: CustomKey[]; -} - -/** * The HashtagsInput entity contains information about hashtags to add to, and/or remove from, a change * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#hashtags-input */ @@ -1633,7 +1360,7 @@ * The RelatedChangesInfo entity contains information about related changes. * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#related-changes-info */ -export interface RelatedChangesInfo { +export declare interface RelatedChangesInfo { changes: RelatedChangeAndCommitInfo[]; } @@ -1641,7 +1368,7 @@ * The RelatedChangeAndCommitInfo entity contains information about a related change and commit. * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#related-change-and-commit-info */ -export interface RelatedChangeAndCommitInfo { +export declare interface RelatedChangeAndCommitInfo { project: RepoName; change_id?: ChangeId; commit: CommitInfoWithRequiredCommit;