Merge "Migrate label fns to SRs: Adapt logic for 'branch' label attribute"
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index dcbd1ae..8acc925 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -37,7 +35,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
@@ -70,30 +67,28 @@
this.rebaseFactory = rebaseFactory;
}
- public static void verifyRebasePreconditions(
- ProjectCache projectCache, PatchSetUtil patchSetUtil, RevWalk rw, RevisionResource rsrc)
- throws ResourceConflictException, IOException, AuthException, PermissionBackendException {
+ /**
+ * Checks whether the given change fulfills all preconditions to be rebased.
+ *
+ * <p>This method does not check whether the calling user is allowed to rebase the change.
+ */
+ public void verifyRebasePreconditions(RevWalk rw, ChangeNotes changeNotes, PatchSet patchSet)
+ throws ResourceConflictException, IOException {
// Not allowed to rebase if the current patch set is locked.
- patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());
+ psUtil.checkPatchSetNotLocked(changeNotes);
- rsrc.permissions().check(ChangePermission.REBASE);
- projectCache
- .get(rsrc.getProject())
- .orElseThrow(illegalState(rsrc.getProject()))
- .checkStatePermitsWrite();
-
- if (!rsrc.getChange().isNew()) {
+ Change change = changeNotes.getChange();
+ if (!change.isNew()) {
throw new ResourceConflictException(
- String.format(
- "Change %s is %s", rsrc.getChange().getId(), ChangeUtil.status(rsrc.getChange())));
- } else if (!hasOneParent(rw, rsrc.getPatchSet())) {
+ String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
+ }
+
+ if (!hasOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
"Error rebasing %s. Cannot rebase %s",
- rsrc.getChange().getId(),
- countParents(rw, rsrc.getPatchSet()) > 1
- ? "merge commits"
- : "commit with no ancestor"));
+ change.getId(),
+ countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 714516c..ca18ab2 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -551,14 +551,14 @@
@Operator
public Predicate<ChangeData> mergedBefore(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
return new BeforePredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
}
@Operator
public Predicate<ChangeData> mergedAfter(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
return new AfterPredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
}
@@ -641,7 +641,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
return new IsAttentionPredicate();
}
@@ -684,7 +684,7 @@
}
if ("uploader".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
return ChangePredicates.uploader(self());
}
@@ -707,7 +707,7 @@
}
if ("merge".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.MERGE_SPEC, "is:merge");
+ checkOperatorAvailable(ChangeField.MERGE_SPEC, "is:merge");
return new BooleanPredicate(ChangeField.MERGE_SPEC);
}
@@ -716,7 +716,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
return new IsAttentionPredicate();
}
@@ -729,7 +729,7 @@
}
if ("pure-revert".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
+ checkOperatorAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
return ChangePredicates.pureRevert("1");
}
@@ -745,12 +745,12 @@
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.NOT_READY)),
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.RULE_ERROR)));
}
- checkFieldAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
+ checkOperatorAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
return new IsSubmittablePredicate();
}
if ("started".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.STARTED_SPEC, "is:started");
+ checkOperatorAvailable(ChangeField.STARTED_SPEC, "is:started");
return new BooleanPredicate(ChangeField.STARTED_SPEC);
}
@@ -759,7 +759,7 @@
}
if ("cherrypick".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
return new BooleanPredicate(ChangeField.CHERRY_PICK_SPEC);
}
@@ -894,7 +894,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
+ checkOperatorAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
return ChangePredicates.fuzzyHashtag(hashtag);
}
@@ -904,7 +904,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
+ checkOperatorAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
return ChangePredicates.prefixHashtag(hashtag);
}
@@ -930,7 +930,7 @@
return ChangePredicates.exactTopic(name);
}
- checkFieldAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
+ checkOperatorAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
return ChangePredicates.prefixTopic(name);
}
@@ -996,7 +996,7 @@
@Operator
public Predicate<ChangeData> hasfooter(String footerName) throws QueryParseException {
- checkFieldAvailable(ChangeField.FOOTER_NAME, "hasfooter");
+ checkOperatorAvailable(ChangeField.FOOTER_NAME, "hasfooter");
return ChangePredicates.hasFooter(footerName);
}
@@ -1147,7 +1147,9 @@
@Operator
public Predicate<ChangeData> message(String text) throws QueryParseException {
if (text.startsWith("^")) {
- checkFieldAvailable(ChangeField.COMMIT_MESSAGE_EXACT, "messageexact");
+ checkFieldAvailable(
+ ChangeField.COMMIT_MESSAGE_EXACT,
+ "'message' operator with regular expression is not supported on this gerrit host");
return new RegexMessagePredicate(text);
}
return ChangePredicates.message(text);
@@ -1155,13 +1157,14 @@
@Operator
public Predicate<ChangeData> subject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
+ checkOperatorAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
return ChangePredicates.subject(value);
}
@Operator
public Predicate<ChangeData> prefixsubject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
+ checkOperatorAvailable(
+ ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
return ChangePredicates.prefixSubject(value);
}
@@ -1249,7 +1252,7 @@
@Operator
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
return uploader(parseAccount(who, (AccountState s) -> true));
}
@@ -1264,7 +1267,7 @@
@Operator
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
return attention(parseAccount(who, (AccountState s) -> true));
}
@@ -1309,7 +1312,7 @@
@Operator
public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
if (g == null) {
@@ -1575,8 +1578,8 @@
@Operator
public Predicate<ChangeData> cherryPickOf(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
if (Ints.tryParse(value) != null) {
return ChangePredicates.cherryPickOf(Change.id(Ints.tryParse(value)));
}
@@ -1648,11 +1651,16 @@
return Predicate.or(predicates);
}
- protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator)
+ private void checkOperatorAvailable(SchemaField<ChangeData, ?> field, String operator)
+ throws QueryParseException {
+ checkFieldAvailable(
+ field, String.format("'%s' operator is not supported on this gerrit host", operator));
+ }
+
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String errorMessage)
throws QueryParseException {
if (!args.index.getSchema().hasField(field)) {
- throw new QueryParseException(
- String.format("'%s' operator is not supported on this gerrit host", operator));
+ throw new QueryParseException(errorMessage);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a8f07a..8a8d2ca 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -87,6 +87,13 @@
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
+ rsrc.permissions().check(ChangePermission.REBASE);
+
+ projectCache
+ .get(rsrc.getProject())
+ .orElseThrow(illegalState(rsrc.getProject()))
+ .checkStatePermitsWrite();
+
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
ObjectInserter oi = repo.newObjectInserter();
@@ -94,7 +101,7 @@
RevWalk rw = CodeReviewCommit.newRevWalk(reader);
BatchUpdate bu =
updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.now())) {
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, rsrc);
+ rebaseUtil.verifyRebasePreconditions(rw, rsrc.getNotes(), rsrc.getPatchSet());
RebaseChangeOp rebaseOp =
rebaseUtil.getRebaseOp(
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 4754c69..786bba7 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -113,7 +113,11 @@
@Override
public Response<RebaseChainInfo> apply(ChangeResource tipRsrc, RebaseInput input)
throws IOException, PermissionBackendException, RestApiException, UpdateException {
+ tipRsrc.permissions().check(ChangePermission.REBASE);
+
Project.NameKey project = tipRsrc.getProject();
+ projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
+
CurrentUser user = tipRsrc.getUser();
List<Change.Id> upToDateAncestors = new ArrayList<>();
@@ -136,7 +140,8 @@
RevisionResource revRsrc =
new RevisionResource(changeResourceFactory.create(changeData, user), ps);
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, revRsrc);
+ revRsrc.permissions().check(ChangePermission.REBASE);
+ rebaseUtil.verifyRebasePreconditions(rw, changeData.notes(), ps);
boolean isUpToDate = false;
RebaseChangeOp rebaseOp = null;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 2123ac2..15baa78 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -1262,7 +1262,7 @@
+ c
+ "/comment/"
+ ps1List.get(0).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: initial\n"
+ "what happened to this?\n"
+ "\n"
@@ -1274,7 +1274,7 @@
+ c
+ "/comment/"
+ ps1List.get(1).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: boring\n"
+ "Is it that bad?\n"
+ "\n"
@@ -1288,7 +1288,7 @@
+ c
+ "/comment/"
+ ps2List.get(0).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: initial content\n"
+ "comment 1 on base\n"
+ "\n"
@@ -1300,7 +1300,7 @@
+ c
+ "/comment/"
+ ps2List.get(1).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: \n"
+ "comment 2 on base\n"
+ "\n"
@@ -1312,7 +1312,7 @@
+ c
+ "/comment/"
+ ps2List.get(2).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: interesting\n"
+ "better now\n"
+ "\n"
@@ -1324,7 +1324,7 @@
+ c
+ "/comment/"
+ ps2List.get(3).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: cntent\n"
+ "typo: content\n"
+ "\n"
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
index 0a21da4..efc6efe 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
@@ -8,11 +8,21 @@
import {LitElement, css, html, PropertyValues, nothing} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
import {RunResult} from '../../models/checks/checks-model';
-import {createFixAction, iconFor} from '../../models/checks/checks-util';
+import {
+ createFixAction,
+ createPleaseFixComment,
+ iconFor,
+} from '../../models/checks/checks-util';
import {modifierPressed} from '../../utils/dom-util';
import './gr-checks-results';
import './gr-hovercard-run';
import {fontStyles} from '../../styles/gr-font-styles';
+import {Action} from '../../api/checks';
+import {assertIsDefined} from '../../utils/common-util';
+import {resolve} from '../../models/dependency';
+import {commentsModelToken} from '../../models/comments/comments-model';
+import {subscribe} from '../lit/subscription-controller';
+import {changeModelToken} from '../../models/change/change-model';
@customElement('gr-diff-check-result')
export class GrDiffCheckResult extends LitElement {
@@ -32,6 +42,13 @@
@state()
isExpandable = false;
+ @state()
+ isOwner = false;
+
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
+ private readonly getCommentsModel = resolve(this, commentsModelToken);
+
static override get styles() {
return [
fontStyles,
@@ -114,6 +131,15 @@
];
}
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getChangeModel().isOwner$,
+ x => (this.isOwner = x)
+ );
+ }
+
override render() {
if (!this.result) return;
const cat = this.result.category.toLowerCase();
@@ -182,14 +208,39 @@
private renderActions() {
if (!this.isExpanded) return nothing;
- return html`<div class="actions">${this.renderFixButton()}</div>`;
+ return html`<div class="actions">
+ ${this.renderPleaseFixButton()}${this.renderShowFixButton()}
+ </div>`;
}
- private renderFixButton() {
+ private renderPleaseFixButton() {
+ if (this.isOwner) return nothing;
+ const action: Action = {
+ name: 'Please Fix',
+ callback: () => {
+ assertIsDefined(this.result, 'result');
+ this.getCommentsModel().saveDraft(createPleaseFixComment(this.result));
+ return undefined;
+ },
+ };
+ return html`
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
+ `;
+ }
+
+ private renderShowFixButton() {
const action = createFixAction(this, this.result);
if (!action) return nothing;
return html`
- <gr-checks-action context="diff-fix" .action=${action}></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
`;
}
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
index 3892c9a..0377e0e 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
@@ -7,6 +7,7 @@
import {fakeRun1} from '../../models/checks/checks-fakes';
import {RunResult} from '../../models/checks/checks-model';
import '../../test/common-test-setup';
+import {queryAndAssert} from '../../utils/common-util';
import './gr-diff-check-result';
import {GrDiffCheckResult} from './gr-diff-check-result';
@@ -50,4 +51,30 @@
`
);
});
+
+ test('renders expanded', async () => {
+ element.result = {...fakeRun1, ...fakeRun1.results?.[2]} as RunResult;
+ element.isExpanded = true;
+ await element.updateComplete;
+
+ const details = queryAndAssert(element, 'div.details');
+ assert.dom.equal(
+ details,
+ /* HTML */ `
+ <div class="details">
+ <gr-result-expanded hidecodepointers=""></gr-result-expanded>
+ <div class="actions">
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ </div>
+ </div>
+ `
+ );
+ });
});
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index 6a5933c..ba43eb4 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -14,12 +14,13 @@
Replacement,
RunStatus,
} from '../../api/checks';
-import {PatchSetNumber} from '../../api/rest-api';
+import {PatchSetNumber, RevisionPatchSetNum} from '../../api/rest-api';
+import {CommentSide} from '../../constants/constants';
import {FixSuggestionInfo, FixReplacementInfo} from '../../types/common';
import {OpenFixPreviewEventDetail} from '../../types/events';
import {isDefined} from '../../types/types';
-import {PROVIDED_FIX_ID} from '../../utils/comment-util';
-import {assert, assertNever} from '../../utils/common-util';
+import {PROVIDED_FIX_ID, UnsavedInfo} from '../../utils/comment-util';
+import {assert, assertIsDefined, assertNever} from '../../utils/common-util';
import {fire} from '../../utils/event-util';
import {CheckResult, CheckRun, RunResult} from './checks-model';
@@ -86,6 +87,27 @@
}
}
+function pleaseFixMessage(result: RunResult) {
+ return `Please fix this ${result.category} reported by ${result.checkName}: ${result.summary}
+
+${result.message}`;
+}
+
+export function createPleaseFixComment(result: RunResult): UnsavedInfo {
+ const pointer = result.codePointers?.[0];
+ assertIsDefined(pointer, 'codePointer');
+ return {
+ __unsaved: true,
+ path: pointer.path,
+ patch_set: result.patchset as RevisionPatchSetNum,
+ side: CommentSide.REVISION,
+ line: pointer.range.end_line ?? pointer.range.start_line,
+ range: pointer.range,
+ message: pleaseFixMessage(result),
+ unresolved: true,
+ };
+}
+
export function createFixAction(
target: EventTarget,
result?: RunResult
diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy
index 98ab4b2..4b621b5 100644
--- a/resources/com/google/gerrit/server/mail/Comment.soy
+++ b/resources/com/google/gerrit/server/mail/Comment.soy
@@ -77,13 +77,8 @@
{for $line, $index in $comment.lines}
{if $index == 0}
{if $comment.startLine != 0}
- {$comment.link}
+ {$comment.link}{sp}:{\n}
{/if}
-
- // Insert a space before the newline so that Gmail does not mistakenly
- // link the following line with the file link. See issue 9201.
- {sp}{\n}
-
{$comment.linePrefix}
{else}
{$comment.linePrefixEmpty}
