Merge "PostReviewOp: Sort votes in change messages"
diff --git a/java/com/google/gerrit/entities/LabelFunction.java b/java/com/google/gerrit/entities/LabelFunction.java
index f361741..d49ab0f 100644
--- a/java/com/google/gerrit/entities/LabelFunction.java
+++ b/java/com/google/gerrit/entities/LabelFunction.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.entities;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.SubmitRecord.Label;
 import java.util.Collections;
@@ -48,6 +49,16 @@
     ALL = Collections.unmodifiableMap(all);
   }
 
+  public static final Map<String, LabelFunction> ALL_NON_DEPRECATED;
+
+  static {
+    Map<String, LabelFunction> allNonDeprecated = new LinkedHashMap<>();
+    for (LabelFunction f : ImmutableSet.of(NO_BLOCK, NO_OP, PATCH_SET_LOCK)) {
+      allNonDeprecated.put(f.getFunctionName(), f);
+    }
+    ALL_NON_DEPRECATED = Collections.unmodifiableMap(allNonDeprecated);
+  }
+
   public static Optional<LabelFunction> parse(@Nullable String str) {
     return Optional.ofNullable(ALL.get(str));
   }
diff --git a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
index aa8a958..3cad7ce 100644
--- a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
@@ -16,9 +16,11 @@
 
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.kohsuke.args4j.CmdLineException;
 import org.kohsuke.args4j.CmdLineParser;
+import org.kohsuke.args4j.NamedOptionDef;
 import org.kohsuke.args4j.OptionDef;
 import org.kohsuke.args4j.spi.OptionHandler;
 import org.kohsuke.args4j.spi.Parameters;
@@ -37,7 +39,14 @@
   @Override
   public int parseArguments(Parameters params) throws CmdLineException {
     final String n = params.getParameter(0);
-    setter.addValue(ObjectId.fromString(n));
+    try {
+      setter.addValue(ObjectId.fromString(n));
+    } catch (InvalidObjectIdException e) {
+      throw new CmdLineException(
+          owner,
+          String.format("expected SHA1 for option %s: %s", ((NamedOptionDef) option).name(), n),
+          e);
+    }
     return 1;
   }
 
diff --git a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
index b1f9726..194a4f0 100644
--- a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
+++ b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
@@ -72,8 +72,8 @@
     }
 
     List<ChangeData> cds =
-        InternalChangeQuery.byProjectGroups(
-            queryProvider, indexConfig, changeData.project(), groups);
+        InternalChangeQuery.byBranchGroups(
+            queryProvider, indexConfig, changeData.change().getDest(), groups);
     if (cds.isEmpty()) {
       return Collections.emptyList();
     }
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index 79e2054..fb6b177 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -43,6 +43,16 @@
  * what labels are defined for the project. The label definition can change between the time a vote
  * is originally made and a later point, for example when a change is submitted. This class
  * normalizes old votes against current project configuration.
+ *
+ * <p>Normalizing a vote means making it compliant with the current label definition:
+ *
+ * <ul>
+ *   <li>If the voting value is greater than the max allowed value according to the label
+ *       definition, the voting value is changed to the max allowed value.
+ *   <li>If the voting value is lower than the min allowed value according to the label definition,
+ *       the voting value is changed to the min allowed value.
+ *   <li>If the label definition for a vote is missing, the vote is deleted.
+ * </ul>
  */
 @Singleton
 public class LabelNormalizer {
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 60e30bc..cd7e29a 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -295,8 +295,8 @@
     // Find changes in the same related group as this patch set, having a patch
     // set whose parent matches this patch set's revision.
     for (ChangeData cd :
-        InternalChangeQuery.byProjectGroups(
-            queryProvider, indexConfig, change.getProject(), currentPs.groups())) {
+        InternalChangeQuery.byBranchGroups(
+            queryProvider, indexConfig, change.getDest(), currentPs.groups())) {
       PATCH_SETS:
       for (PatchSet ps : cd.patchSets()) {
         RevCommit commit = rw.parseCommit(ps.commitId());
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index a964ee1..fc1256e 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -1143,9 +1143,11 @@
         error(
             String.format(
                 "Invalid %s for label \"%s\". Valid names are: %s",
-                KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet())));
+                KEY_FUNCTION,
+                name,
+                Joiner.on(", ").join(LabelFunction.ALL_NON_DEPRECATED.keySet())));
       }
-      label.setFunction(function.orElse(null));
+      function.ifPresent(label::setFunction);
       label.setCopyCondition(rc.getString(LABEL, name, KEY_COPY_CONDITION));
 
       if (!values.isEmpty()) {
diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java
index e86ad41..07f7ba5 100644
--- a/java/com/google/gerrit/server/project/RefUtil.java
+++ b/java/com/google/gerrit/server/project/RefUtil.java
@@ -23,6 +23,7 @@
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import java.io.IOException;
 import java.util.Collections;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.RevisionSyntaxException;
@@ -49,6 +50,9 @@
     } catch (RevisionSyntaxException e) {
       throw new UnprocessableEntityException(
           String.format("base revision \"%s\" is invalid", baseRevision), e);
+    } catch (AmbiguousObjectException e) {
+      throw new UnprocessableEntityException(
+          String.format("base revision \"%s\" is ambiguous", baseRevision), e);
     }
   }
 
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 99c1ca1..ccd645b 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -271,21 +271,21 @@
     return query(ChangePredicates.submissionId(cs));
   }
 
-  private static Predicate<ChangeData> byProjectGroupsPredicate(
-      IndexConfig indexConfig, Project.NameKey project, Collection<String> groups) {
-    int n = indexConfig.maxTerms() - 1;
+  private static Predicate<ChangeData> byBranchGroupsPredicate(
+      IndexConfig indexConfig, BranchNameKey branchAndProject, Collection<String> groups) {
+    int n = indexConfig.maxTerms() - 2;
     checkArgument(groups.size() <= n, "cannot exceed %s groups", n);
     List<GroupPredicate> groupPredicates = new ArrayList<>(groups.size());
     for (String g : groups) {
       groupPredicates.add(new GroupPredicate(g));
     }
-    return and(project(project), or(groupPredicates));
+    return and(project(branchAndProject.project()), ref(branchAndProject), or(groupPredicates));
   }
 
-  public static ImmutableList<ChangeData> byProjectGroups(
+  public static ImmutableList<ChangeData> byBranchGroups(
       Provider<InternalChangeQuery> queryProvider,
       IndexConfig indexConfig,
-      Project.NameKey project,
+      BranchNameKey branchAndProject,
       Collection<String> groups) {
     // These queries may be complex along multiple dimensions:
     //  * Many groups per change, if there are very many patch sets. This requires partitioning the
@@ -296,16 +296,17 @@
     // InternalChangeQuery is single-use.
 
     Supplier<InternalChangeQuery> querySupplier = () -> queryProvider.get().enforceVisibility(true);
-    int batchSize = indexConfig.maxTerms() - 1;
+    int batchSize = indexConfig.maxTerms() - 2;
     if (groups.size() <= batchSize) {
       return queryExhaustively(
-          querySupplier, byProjectGroupsPredicate(indexConfig, project, groups));
+          querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, groups));
     }
     Set<Change.Id> seen = new HashSet<>();
     ImmutableList.Builder<ChangeData> result = ImmutableList.builder();
     for (List<String> part : Iterables.partition(groups, batchSize)) {
       for (ChangeData cd :
-          queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
+          queryExhaustively(
+              querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, part))) {
         if (!seen.add(cd.getId())) {
           result.add(cd);
         }
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 52207db..5fd2159 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -134,6 +134,27 @@
   }
 
   @Test
+  public void rejectCreatingLabelWithInvalidFunction() throws Exception {
+    fetchRefsMetaConfig();
+    PushOneCommit push =
+        pushFactory.create(
+            admin.newIdent(),
+            testRepo,
+            "Test Change",
+            ProjectConfig.PROJECT_CONFIG,
+            "[label \"Foo\"]\n  function = INVALID");
+    PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+    r.assertErrorStatus(
+        String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+    r.assertMessage(
+        String.format(
+            "ERROR: commit %s: invalid project configuration:\n"
+                + "ERROR: commit %s:   project.config: Invalid function for label \"foo\"."
+                + " Valid names are: NoBlock, NoOp, PatchSetLock",
+            abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+  }
+
+  @Test
   public void rejectSettingCopyMinScore() throws Exception {
     testRejectSettingLabelFlag(
         LabelConfigValidator.KEY_COPY_MIN_SCORE, /* value= */ true, "is:MIN");
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 50bb8ae..b738324 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -3082,6 +3082,12 @@
     assertThat(r.getChange().attentionSet()).isEmpty();
   }
 
+  @Test
+  public void pushWithInvalidBaseIsRejected() throws Exception {
+    PushOneCommit.Result r = pushTo("refs/for/master%base=invalid");
+    r.assertErrorStatus("expected SHA1 for option --base: invalid");
+  }
+
   private DraftInput newDraft(String path, int line, String message) {
     DraftInput d = new DraftInput();
     d.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 70b5701..21db45c 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -39,11 +39,13 @@
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.extensions.api.changes.GetRelatedOption;
 import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.index.IndexConfig;
@@ -67,6 +69,8 @@
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
 import org.junit.Test;
 
 @NoHttpd
@@ -664,6 +668,71 @@
     }
   }
 
+  @Test
+  public void getRelatedLinearSameCommitPushedTwice() throws Exception {
+    RevCommit base = projectOperations.project(project).getHead("master");
+
+    // 1,1---2,1 on master
+    PushOneCommit.Result r1 =
+        createChange(
+            testRepo,
+            "master",
+            "subject: 1",
+            "a.txt",
+            "1",
+            /** topic= */
+            null);
+    RevCommit c1_1 = r1.getCommit();
+    PatchSet.Id ps1_1 = r1.getPatchSetId();
+
+    PushOneCommit.Result r2 =
+        createChange(
+            testRepo,
+            "master",
+            "subject: 2",
+            "b.txt",
+            "2",
+            /** topic= */
+            null);
+    RevCommit c2_1 = r2.getCommit();
+    PatchSet.Id ps2_1 = r2.getPatchSetId();
+
+    // 3,1---4,1 on stable
+    gApi.projects().name(project.get()).branch("stable").create(new BranchInput());
+    testRepo.reset(c1_1);
+    PushResult r3 = pushHead(testRepo, "refs/for/stable%base=" + base.getName());
+    assertThat(r3.getRemoteUpdate("refs/for/stable%base=" + base.getName()).getStatus())
+        .isEqualTo(RemoteRefUpdate.Status.OK);
+    ChangeData change3 =
+        Iterables.getOnlyElement(
+            queryProvider
+                .get()
+                .byBranchCommit(BranchNameKey.create(project, "stable"), c1_1.getName()));
+    assertThat(change3.currentPatchSet().commitId()).isEqualTo(c1_1);
+    RevCommit c3_1 = c1_1;
+    PatchSet.Id ps3_1 = change3.currentPatchSet().id();
+
+    PushOneCommit.Result r4 =
+        createChange(
+            testRepo,
+            "stable",
+            "subject: 4",
+            "d.txt",
+            "4",
+            /** topic= */
+            null);
+    RevCommit c4_1 = r4.getCommit();
+    PatchSet.Id ps4_1 = r4.getPatchSetId();
+
+    for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
+      assertRelated(ps, changeAndCommit(ps2_1, c2_1, 1), changeAndCommit(ps1_1, c1_1, 1));
+    }
+
+    for (PatchSet.Id ps : ImmutableList.of(ps4_1, ps3_1)) {
+      assertRelated(ps, changeAndCommit(ps4_1, c4_1, 1), changeAndCommit(ps3_1, c3_1, 1));
+    }
+  }
+
   private static Correspondence<RelatedChangeAndCommitInfo, String>
       getRelatedChangeToStatusCorrespondence() {
     return Correspondence.transforming(
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 1d10128..a1021a0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -1422,6 +1422,39 @@
         await element.reload();
       });
 
+      test('revert change payload', async () => {
+        await element.updateComplete;
+        queryAndAssert<GrButton>(
+          element,
+          'gr-button[data-action-key="revert"]'
+        ).click();
+        const revertAction = {
+          __key: 'revert',
+          __type: 'change',
+          __primary: false,
+          method: HttpMethod.POST,
+          label: 'Revert',
+          title: 'Revert the change',
+          enabled: true,
+        };
+        queryAndAssert(element, 'gr-confirm-revert-dialog').dispatchEvent(
+          new CustomEvent('confirm', {
+            detail: {
+              message: 'foo message',
+              revertType: 1,
+            },
+          })
+        );
+        assert.deepEqual(fireActionStub.lastCall.args, [
+          '/revert',
+          assertUIActionInfo(revertAction),
+          false,
+          {
+            message: 'foo message',
+          },
+        ]);
+      });
+
       test('revert change with plugin hook', async () => {
         const newRevertMsg = 'Modified revert msg';
         sinon
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
index 69fd142..a2beee2 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
@@ -99,7 +99,7 @@
   override render() {
     const change = this.change;
     if (!change) throw new Error('Missing change');
-    const linkClass = this._computeLinkClass(change);
+    const linkClass = this.computeLinkClass(change);
     return html`
       <div class="changeContainer">
         <a
@@ -118,16 +118,16 @@
               >✓</span
             >`
           : ''}
-        ${this.showChangeStatus && !isChangeInfo(change)
-          ? html`<span class=${this._computeChangeStatusClass(change)}>
-              (${this._computeChangeStatus(change)})
+        ${this.showChangeStatus
+          ? html`<span class=${this.computeChangeStatusClass(change)}>
+              (${this.computeChangeStatus(change)})
             </span>`
           : ''}
       </div>
     `;
   }
 
-  _computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
+  private computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
     const statuses = [];
     if (change.status === ChangeStatus.ABANDONED) {
       statuses.push('strikethrough');
@@ -138,11 +138,16 @@
     return statuses.join(' ');
   }
 
-  _computeChangeStatusClass(change: RelatedChangeAndCommitInfo) {
+  private computeChangeStatusClass(
+    change: RelatedChangeAndCommitInfo | ChangeInfo
+  ) {
     const classes = ['status'];
-    if (change._revision_number !== change._current_revision_number) {
+    if (
+      !isChangeInfo(change) &&
+      change._revision_number !== change._current_revision_number
+    ) {
       classes.push('notCurrent');
-    } else if (this._isIndirectAncestor(change)) {
+    } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
       classes.push('indirectAncestor');
     } else if (change.submittable) {
       classes.push('submittable');
@@ -152,16 +157,19 @@
     return classes.join(' ');
   }
 
-  _computeChangeStatus(change: RelatedChangeAndCommitInfo) {
+  private computeChangeStatus(change: RelatedChangeAndCommitInfo | ChangeInfo) {
     switch (change.status) {
       case ChangeStatus.MERGED:
         return 'Merged';
       case ChangeStatus.ABANDONED:
         return 'Abandoned';
     }
-    if (change._revision_number !== change._current_revision_number) {
+    if (
+      !isChangeInfo(change) &&
+      change._revision_number !== change._current_revision_number
+    ) {
       return 'Not current';
-    } else if (this._isIndirectAncestor(change)) {
+    } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
       return 'Indirect ancestor';
     } else if (change.submittable) {
       return 'Submittable';
@@ -169,7 +177,7 @@
     return '';
   }
 
-  _isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
+  private isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
     return (
       this.connectedRevisions &&
       !this.connectedRevisions.includes(change.commit.commit)
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index bdb95ea..cac9ae5 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -418,6 +418,7 @@
               )}<gr-related-change
                 .change=${change}
                 .href=${createChangeUrl({change, usp: 'cherry-pick'})}
+                show-change-status
                 >${change.branch}: ${change.subject}</gr-related-change
               >
             </div>`
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 676a468..3e90145 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -249,7 +249,7 @@
               <gr-related-collapse title="Cherry picks">
                 <div class="relatedChangeLine show-when-collapsed">
                   <span class="marker space"> </span>
-                  <gr-related-change>
+                  <gr-related-change show-change-status="">
                     test-branch: Test subject
                   </gr-related-change>
                 </div>
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 3ff0780..11406b4 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -606,13 +606,13 @@
       .patchsetLevelContainer.unresolved {
         background-color: var(--unresolved-comment-background-color);
       }
-      .infoMessage {
+      .privateVisiblityInfo {
         display: flex;
         justify-content: center;
         background-color: var(--info-background);
         padding: var(--spacing-s) 0;
       }
-      .infoMessage gr-icon {
+      .privateVisiblityInfo gr-icon {
         margin-right: var(--spacing-m);
         color: var(--info-foreground);
       }
@@ -802,7 +802,6 @@
           </gr-endpoint-decorator>
           ${this.renderCCList()} ${this.renderReviewConfirmation()}
           ${this.renderPrivateVisiblityInfo()}
-          ${this.renderNotificationForWipChangesInfo()}
         </section>
         <section class="labelsContainer">${this.renderLabels()}</section>
         <section class="newReplyDialog textareaContainer">
@@ -912,7 +911,7 @@
     ];
     if (!this.change?.is_private || !addedAccounts.length) return nothing;
     return html`
-      <div class="infoMessage">
+      <div class="privateVisiblityInfo">
         <gr-icon icon="info"></gr-icon>
         <div>
           Adding a reviewer/CC will make this private change visible to them
@@ -921,20 +920,6 @@
     `;
   }
 
-  private renderNotificationForWipChangesInfo() {
-    const addedAccounts = [
-      ...(this.reviewersList?.additions() ?? []),
-      ...(this.ccsList?.additions() ?? []),
-    ];
-    if (!this.change?.work_in_progress || !addedAccounts.length) return nothing;
-    return html`
-      <div class="infoMessage">
-        <gr-icon icon="info"></gr-icon>
-        <div>Reviewers will not be notified for WIP changes</div>
-      </div>
-    `;
-  }
-
   private renderLabels() {
     if (!this.change || !this.account || !this.permittedLabels) return;
     return html`
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 8582fd8..52ea252 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -432,7 +432,7 @@
             </gr-button>
           </div>
         </dialog>
-        <div class="infoMessage">
+        <div class="privateVisiblityInfo">
           <gr-icon icon="info">
           </gr-icon>
           <div>
@@ -444,123 +444,6 @@
     );
   });
 
-  test('renders wip change info when reviewer is added', async () => {
-    element.change!.work_in_progress = true;
-    element.requestUpdate();
-    await element.updateComplete;
-    const peopleContainer = queryAndAssert<HTMLDivElement>(
-      element,
-      '.peopleContainer'
-    );
-
-    // Info is rendered only if reviewer is added
-    assert.dom.equal(
-      peopleContainer,
-      `
-      <section class="peopleContainer">
-        <gr-endpoint-decorator name="reply-reviewers">
-          <gr-endpoint-param name="change"> </gr-endpoint-param>
-          <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
-          <div class="peopleList">
-            <div class="peopleListLabel">Reviewers</div>
-            <gr-account-list id="reviewers"> </gr-account-list>
-            <gr-endpoint-slot name="right"> </gr-endpoint-slot>
-          </div>
-          <gr-endpoint-slot name="below"> </gr-endpoint-slot>
-        </gr-endpoint-decorator>
-        <div class="peopleList">
-          <div class="peopleListLabel">CC</div>
-          <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
-        </div>
-        <dialog
-          tabindex="-1"
-          id="reviewerConfirmationModal"
-        >
-          <div class="reviewerConfirmation">
-            Group
-            <span class="groupName"> </span>
-            has
-            <span class="groupSize"> </span>
-            members.
-            <br />
-            Are you sure you want to add them all?
-          </div>
-          <div class="reviewerConfirmationButtons">
-            <gr-button aria-disabled="false" role="button" tabindex="0">
-              Yes
-            </gr-button>
-            <gr-button aria-disabled="false" role="button" tabindex="0">
-              No
-            </gr-button>
-          </div>
-        </dialog>
-      </section>
-    `
-    );
-
-    const account = createAccountWithId(22);
-    element.reviewersList!.accounts = [];
-    element.reviewersList!.addAccountItem({account, count: 1});
-    element.reviewersList!.dispatchEvent(
-      new CustomEvent('account-added', {
-        detail: {account},
-      })
-    );
-    element.requestUpdate();
-    await element.updateComplete;
-
-    assert.dom.equal(
-      peopleContainer,
-      `
-      <section class="peopleContainer">
-        <gr-endpoint-decorator name="reply-reviewers">
-          <gr-endpoint-param name="change"> </gr-endpoint-param>
-          <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
-          <div class="peopleList">
-            <div class="peopleListLabel">Reviewers</div>
-            <gr-account-list id="reviewers"> </gr-account-list>
-            <gr-endpoint-slot name="right"> </gr-endpoint-slot>
-          </div>
-          <gr-endpoint-slot name="below"> </gr-endpoint-slot>
-        </gr-endpoint-decorator>
-        <div class="peopleList">
-          <div class="peopleListLabel">CC</div>
-          <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
-        </div>
-        <dialog
-          tabindex="-1"
-          id="reviewerConfirmationModal"
-        >
-          <div class="reviewerConfirmation">
-            Group
-            <span class="groupName"> </span>
-            has
-            <span class="groupSize"> </span>
-            members.
-            <br />
-            Are you sure you want to add them all?
-          </div>
-          <div class="reviewerConfirmationButtons">
-            <gr-button aria-disabled="false" role="button" tabindex="0">
-              Yes
-            </gr-button>
-            <gr-button aria-disabled="false" role="button" tabindex="0">
-              No
-            </gr-button>
-          </div>
-        </dialog>
-        <div class="infoMessage">
-          <gr-icon icon="info">
-          </gr-icon>
-          <div>
-            Reviewers will not be notified for WIP changes
-          </div>
-        </div>
-      </section>
-    `
-    );
-  });
-
   test('default to publishing draft comments with reply', async () => {
     // Async tick is needed because iron-selector content is distributed and
     // distributed content requires an observer to be set up.
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
index 4005bb3..140b752 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
@@ -165,12 +165,7 @@
 
   override render() {
     return html`
-      <div
-        class="dropdown-content"
-        slot="dropdown-content"
-        id="suggestions"
-        role="listbox"
-      >
+      <div class="dropdown-content" id="suggestions" role="listbox">
         <ul>
           ${repeat(
             this.suggestions,
@@ -209,7 +204,7 @@
   }
 
   setPositionTarget(target: HTMLElement) {
-    this.fitController?.setPositionTarget(target);
+    this.fitController.setPositionTarget(target);
   }
 
   private handleUp() {
@@ -300,7 +295,7 @@
     } else {
       this.cursor.stops = [];
     }
-    this.fitController?.refit();
+    this.fitController.refit();
   }
 
   private setIndex() {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
index 641dd2d..cb83396 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
@@ -42,12 +42,7 @@
     assert.shadowDom.equal(
       element,
       /* HTML */ `
-        <div
-          class="dropdown-content"
-          id="suggestions"
-          role="listbox"
-          slot="dropdown-content"
-        >
+        <div class="dropdown-content" id="suggestions" role="listbox">
           <ul>
             <li
               aria-label="test name 1"
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 096c28e..804d101 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -523,7 +523,7 @@
           // If endIndex isn't present, continue to the end of the line.
           const endIndex =
             highlight.endIndex === undefined
-              ? line.text.length
+              ? GrAnnotation.getStringLength(line.text)
               : highlight.endIndex;
 
           GrAnnotation.annotateElement(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 5eabc59..0f02d71 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -271,10 +271,11 @@
 
       const str0 = slice(str, 0, 6);
       const str1 = slice(str, 6);
+      const numHighlightedChars = GrAnnotation.getStringLength(str1);
 
       layer.annotate(el, lineNumberEl, l, Side.LEFT);
 
-      assert.isTrue(annotateElementSpy.called);
+      assert.isTrue(annotateElementSpy.calledWith(el, 6, numHighlightedChars));
       assert.equal(el.childNodes.length, 2);
 
       assert.instanceOf(el.childNodes[0], Text);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index cc7cd49..92175eb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -22,8 +22,14 @@
     return this.getStringLength(node.textContent || '');
   },
 
+  /**
+   * Returns the number of Unicode code points in the given string
+   *
+   * This is not necessarily the same as the number of visible symbols.
+   * See https://mathiasbynens.be/notes/javascript-unicode for more details.
+   */
   getStringLength(str: string) {
-    return str.replace(REGEX_ASTRAL_SYMBOL, '_').length;
+    return [...str].length;
   },
 
   /**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
index 6c45f20..fdf1785 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
@@ -327,4 +327,20 @@
       assert.equal(el.getAttribute('class'), 'hello world');
     });
   });
+
+  suite('getStringLength', () => {
+    test('ASCII characters are counted correctly', () => {
+      assert.equal(GrAnnotation.getStringLength('ASCII'), 5);
+    });
+
+    test('Unicode surrogate pairs count as one symbol', () => {
+      assert.equal(GrAnnotation.getStringLength('Unic💢de'), 7);
+      assert.equal(GrAnnotation.getStringLength('💢💢'), 2);
+    });
+
+    test('Grapheme clusters count as multiple symbols', () => {
+      assert.equal(GrAnnotation.getStringLength('man\u0303ana'), 7); // mañana
+      assert.equal(GrAnnotation.getStringLength('q\u0307\u0323'), 3); // q̣̇
+    });
+  });
 });
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 9f874b8..22a71a5 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -21,6 +21,7 @@
 import {debounce, DelayedTask} from '../../../utils/async-util';
 import {RenderPreferences} from '../../../api/diff';
 import {assertIsDefined} from '../../../utils/common-util';
+import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
 
 const WHOLE_FILE = -1;
 
@@ -639,16 +640,19 @@
     rows: string[],
     intralineInfos: number[][]
   ): Highlights[] {
+    // +1 to account for the \n that is not part of the rows passed here
+    const lineLengths = rows.map(r => GrAnnotation.getStringLength(r) + 1);
+
     let rowIndex = 0;
     let idx = 0;
     const normalized = [];
     for (const [skipLength, markLength] of intralineInfos) {
-      let line = rows[rowIndex] + '\n';
+      let lineLength = lineLengths[rowIndex];
       let j = 0;
       while (j < skipLength) {
-        if (idx === line.length) {
+        if (idx === lineLength) {
           idx = 0;
-          line = rows[++rowIndex] + '\n';
+          lineLength = lineLengths[++rowIndex];
           continue;
         }
         idx++;
@@ -660,10 +664,10 @@
       };
 
       j = 0;
-      while (line && j < markLength) {
-        if (idx === line.length) {
+      while (lineLength && j < markLength) {
+        if (idx === lineLength) {
           idx = 0;
-          line = rows[++rowIndex] + '\n';
+          lineLength = lineLengths[++rowIndex];
           normalized.push(lineHighlight);
           lineHighlight = {
             contentIndex: rowIndex,
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index 6caeb62..f6f7052 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -723,6 +723,25 @@
           endIndex: 41,
         },
       ]);
+
+      content = ['🙈 a', '🙉 b', '🙊 c'];
+      highlights = [[2, 7]];
+      results = element.convertIntralineInfos(content, highlights);
+      assert.deepEqual(results, [
+        {
+          contentIndex: 0,
+          startIndex: 2,
+        },
+        {
+          contentIndex: 1,
+          startIndex: 0,
+        },
+        {
+          contentIndex: 2,
+          startIndex: 0,
+          endIndex: 1,
+        },
+      ]);
     });
 
     test('scrolling pauses rendering', () => {