From 5e7fbf0e6f69c9e760b6384dfdb3d8595fc63434 Mon Sep 17 00:00:00 2001
From: Paladox none <thomasmulhall410@yahoo.com>
Date: Sun, 16 Jun 2019 20:24:31 +0000
Subject: [PATCH 01/12] Add extension point to gr-user-header

This is so that someone can create a plugin that adds extra links
e.g "lock/unlock account".

This extension point would be useful to add a "lock" and "unlock" button
to the user page, making it easier to lock/unlock a user account from the UI.

Change-Id: Ibb327af0ae03f0718549cce198f6449456a15f89
---
 .../change-list/gr-user-header/gr-user-header.html     | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.html b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.html
index d3d073668e..568578ca63 100644
--- a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.html
+++ b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.html
@@ -15,11 +15,13 @@ limitations under the License.
 -->
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../../styles/shared-styles.html">
 <link rel="import" href="../../core/gr-navigation/gr-navigation.html">
+<link rel="import" href="../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.html">
+<link rel="import" href="../../plugins/gr-endpoint-param/gr-endpoint-param.html">
 <link rel="import" href="../../shared/gr-avatar/gr-avatar.html">
 <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
-<link rel="import" href="../../../styles/shared-styles.html">
 
 <dom-module id="gr-user-header">
   <template>
@@ -83,6 +85,12 @@ limitations under the License.
             date-str="[[_computeDetail(_accountDetails, 'registered_on')]]">
         </gr-date-formatter>
       </div>
+      <gr-endpoint-decorator name="user-header">
+        <gr-endpoint-param name="accountDetails" value="[[_accountDetails]]">
+        </gr-endpoint-param>
+        <gr-endpoint-param name="loggedIn" value="[[loggedIn]]">
+        </gr-endpoint-param>
+      </gr-endpoint-decorator>
     </div>
     <div class="info">
       <div class$="[[_computeDashboardLinkClass(showDashboardLink, loggedIn)]]">

From 31c68090ba7ed53755a410f4de6db9669e5de5c2 Mon Sep 17 00:00:00 2001
From: Marco Miller <marco.miller@ericsson.com>
Date: Fri, 14 Jun 2019 13:58:07 -0400
Subject: [PATCH 02/12] ProjectControl: Reuse constants for ref strings

Remove the previous duplication that way while preparing for more use of
some of these constants in this class.

Change-Id: Iba89614c058b6105f7b9305f022d76ad153c5495
---
 .../com/google/gerrit/server/project/ProjectControl.java  | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index e97411e556..c77c1cdda9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -15,7 +15,9 @@
 package com.google.gerrit.server.project;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.common.data.AccessSection.ALL;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_TAGS;
+import static com.google.gerrit.server.util.MagicBranch.NEW_CHANGE;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -203,7 +205,7 @@ public class ProjectControl {
 
   /** Is this user a project owner? */
   public boolean isOwner() {
-    return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER)) || isAdmin();
+    return (isDeclaredOwner() && !controlForRef(ALL).isBlocked(Permission.OWNER)) || isAdmin();
   }
 
   /**
@@ -259,7 +261,7 @@ public class ProjectControl {
   private boolean canCreateChanges() {
     for (SectionMatcher matcher : access()) {
       AccessSection section = matcher.section;
-      if (section.getName().startsWith("refs/for/")) {
+      if (section.getName().startsWith(NEW_CHANGE)) {
         Permission permission = section.getPermission(Permission.PUSH);
         if (permission != null && controlForRef(section.getName()).canPerform(Permission.PUSH)) {
           return true;
@@ -344,7 +346,7 @@ public class ProjectControl {
   private boolean canPerformOnAllRefs(String permission, Set<String> ignore) {
     boolean canPerform = false;
     Set<String> patterns = allRefPatterns(permission);
-    if (patterns.contains(AccessSection.ALL)) {
+    if (patterns.contains(ALL)) {
       // Only possible if granted on the pattern that
       // matches every possible reference.  Check all
       // patterns also have the permission.

From 07031b0407e0d3812a142c94405f6333d0e6a8d8 Mon Sep 17 00:00:00 2001
From: Marco Miller <marco.miller@ericsson.com>
Date: Fri, 14 Jun 2019 14:39:54 -0400
Subject: [PATCH 03/12] ProjectControl: Allow regexes ref strings for tags

Before this change, only ref strings not starting with the regex prefix
for tags allowed to show the tag creation form. This was against the
documentation [1] saying that "Reference names can also be described
with a regular expression by prefixing the reference name with ^."

[1] https://gerrit-documentation.storage.googleapis.com/Documentation/2.15.14/access-control.html#_project_access_control_lists

Bug: Issue 10763
Change-Id: If6fbac1e2998c9fb63679c8a4fd2f5d311af4441
---
 .../java/com/google/gerrit/server/project/ProjectControl.java | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index c77c1cdda9..1a51424e1c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.common.data.AccessSection.ALL;
+import static com.google.gerrit.common.data.RefConfigSection.REGEX_PREFIX;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_TAGS;
 import static com.google.gerrit.server.util.MagicBranch.NEW_CHANGE;
 
@@ -292,7 +293,8 @@ public class ProjectControl {
     for (SectionMatcher matcher : access()) {
       AccessSection section = matcher.section;
 
-      if (section.getName().startsWith(REFS_TAGS)) {
+      if (section.getName().startsWith(REFS_TAGS)
+          || section.getName().startsWith(REGEX_PREFIX + REFS_TAGS)) {
         Permission permission = section.getPermission(permissionName);
         if (permission == null) {
           continue;

From e1de7a527729d1a3ff5bedfea474b4399be4281e Mon Sep 17 00:00:00 2001
From: Marco Miller <marco.miller@ericsson.com>
Date: Fri, 14 Jun 2019 16:07:46 -0400
Subject: [PATCH 04/12] ProjectControl: Allow regexes ref strings for uploads

Before this change, only ref strings not starting with the regex prefix
for uploads allowed the latter. This was against the documentation [1]
saying that "Reference names can also be described with a regular
expression by prefixing the reference name with ^."

[1] https://gerrit-documentation.storage.googleapis.com/Documentation/2.15.14/access-control.html#_project_access_control_lists

Bug: Issue 10763
Change-Id: I6b685a7f140d82f0278b8efec271a78d92608b94
---
 .../java/com/google/gerrit/server/project/ProjectControl.java  | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index 1a51424e1c..76e9e9816f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -262,7 +262,8 @@ public class ProjectControl {
   private boolean canCreateChanges() {
     for (SectionMatcher matcher : access()) {
       AccessSection section = matcher.section;
-      if (section.getName().startsWith(NEW_CHANGE)) {
+      if (section.getName().startsWith(NEW_CHANGE)
+          || section.getName().startsWith(REGEX_PREFIX + NEW_CHANGE)) {
         Permission permission = section.getPermission(Permission.PUSH);
         if (permission != null && controlForRef(section.getName()).canPerform(Permission.PUSH)) {
           return true;

From 13906f2c23ee015015a4b1be57f319c11e4e6825 Mon Sep 17 00:00:00 2001
From: Marco Miller <marco.miller@ericsson.com>
Date: Mon, 17 Jun 2019 14:56:10 -0400
Subject: [PATCH 05/12] Documentation: Fix the Elasticsearch shards/replicas
 link

Replace the broken shards/replicas link with the currently working one.

Change-Id: Ie5abd681e29c49b3d12a772458812eb0ef93d55c
---
 Documentation/config-gerrit.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 1e68b04c68..77d18dbf12 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2927,7 +2927,7 @@ manually.
 [[elasticsearch.numberOfShards]]elasticsearch.numberOfShards::
 +
 Sets the number of shards to use per index. Refer to the
-link:https://www.elastic.co/guide/en/elasticsearch/reference/current/_basic_concepts.html#getting-started-shards-and-replicas[
+link:https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-concepts.html#getting-started-shards-and-replicas[
 Elasticsearch documentation] for details.
 +
 Defaults to 5.
@@ -2935,7 +2935,7 @@ Defaults to 5.
 [[elasticsearch.numberOfReplicas]]elasticsearch.numberOfReplicas::
 +
 Sets the number of replicas to use per index. Refer to the
-link:https://www.elastic.co/guide/en/elasticsearch/reference/current/_basic_concepts.html#getting-started-shards-and-replicas[
+link:https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-concepts.html#getting-started-shards-and-replicas[
 Elasticsearch documentation] for details.
 +
 Defaults to 1.

From 7e7a22b2689f0ef7ecd03113a147aa928a960b5c Mon Sep 17 00:00:00 2001
From: Luca Milanesio <luca.milanesio@gmail.com>
Date: Mon, 17 Jun 2019 21:05:14 +0000
Subject: [PATCH 06/12] Set version to 2.16.9

Change-Id: I65c4b998056100c43444d7ad2275e7c0d5c42719
---
 tools/maven/gerrit-acceptance-framework_pom.xml | 2 +-
 tools/maven/gerrit-extension-api_pom.xml        | 2 +-
 tools/maven/gerrit-plugin-api_pom.xml           | 2 +-
 tools/maven/gerrit-plugin-gwtui_pom.xml         | 2 +-
 tools/maven/gerrit-war_pom.xml                  | 2 +-
 version.bzl                                     | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml
index 403280765e..88b9eb2867 100644
--- a/tools/maven/gerrit-acceptance-framework_pom.xml
+++ b/tools/maven/gerrit-acceptance-framework_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-acceptance-framework</artifactId>
-  <version>2.16.9-SNAPSHOT</version>
+  <version>2.16.9</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Acceptance Test Framework</name>
   <description>Framework for Gerrit's acceptance tests</description>
diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml
index 322344a49c..e539a05fad 100644
--- a/tools/maven/gerrit-extension-api_pom.xml
+++ b/tools/maven/gerrit-extension-api_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-extension-api</artifactId>
-  <version>2.16.9-SNAPSHOT</version>
+  <version>2.16.9</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Extension API</name>
   <description>API for Gerrit Extensions</description>
diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml
index 6704e8759f..c7ec4caf64 100644
--- a/tools/maven/gerrit-plugin-api_pom.xml
+++ b/tools/maven/gerrit-plugin-api_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-plugin-api</artifactId>
-  <version>2.16.9-SNAPSHOT</version>
+  <version>2.16.9</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Plugin API</name>
   <description>API for Gerrit Plugins</description>
diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml
index f6eb352511..4f8d9e191c 100644
--- a/tools/maven/gerrit-plugin-gwtui_pom.xml
+++ b/tools/maven/gerrit-plugin-gwtui_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-plugin-gwtui</artifactId>
-  <version>2.16.9-SNAPSHOT</version>
+  <version>2.16.9</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Plugin GWT UI</name>
   <description>Common Classes for Gerrit GWT UI Plugins</description>
diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml
index 5148a7df71..962ff4d917 100644
--- a/tools/maven/gerrit-war_pom.xml
+++ b/tools/maven/gerrit-war_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-war</artifactId>
-  <version>2.16.9-SNAPSHOT</version>
+  <version>2.16.9</version>
   <packaging>war</packaging>
   <name>Gerrit Code Review - WAR</name>
   <description>Gerrit WAR</description>
diff --git a/version.bzl b/version.bzl
index f03d5481d6..b67a438cd0 100644
--- a/version.bzl
+++ b/version.bzl
@@ -2,4 +2,4 @@
 # Used by :api_install and :api_deploy targets
 # when talking to the destination repository.
 #
-GERRIT_VERSION = "2.16.9-SNAPSHOT"
+GERRIT_VERSION = "2.16.9"

From 379a2f3084da3153c7fd7ff2cf5551f6a9b19bd3 Mon Sep 17 00:00:00 2001
From: Luca Milanesio <luca.milanesio@gmail.com>
Date: Mon, 17 Jun 2019 22:52:19 +0000
Subject: [PATCH 07/12] Set version to 2.16.10-SNAPSHOT

Change-Id: I6da6f68cbcc3dc9a4c5fb2eeb3cae8169750718a
---
 tools/maven/gerrit-acceptance-framework_pom.xml | 2 +-
 tools/maven/gerrit-extension-api_pom.xml        | 2 +-
 tools/maven/gerrit-plugin-api_pom.xml           | 2 +-
 tools/maven/gerrit-plugin-gwtui_pom.xml         | 2 +-
 tools/maven/gerrit-war_pom.xml                  | 2 +-
 version.bzl                                     | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml
index 88b9eb2867..a130c2a17b 100644
--- a/tools/maven/gerrit-acceptance-framework_pom.xml
+++ b/tools/maven/gerrit-acceptance-framework_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-acceptance-framework</artifactId>
-  <version>2.16.9</version>
+  <version>2.16.10-SNAPSHOT</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Acceptance Test Framework</name>
   <description>Framework for Gerrit's acceptance tests</description>
diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml
index e539a05fad..b7526cfc33 100644
--- a/tools/maven/gerrit-extension-api_pom.xml
+++ b/tools/maven/gerrit-extension-api_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-extension-api</artifactId>
-  <version>2.16.9</version>
+  <version>2.16.10-SNAPSHOT</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Extension API</name>
   <description>API for Gerrit Extensions</description>
diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml
index c7ec4caf64..a4cd4d1114 100644
--- a/tools/maven/gerrit-plugin-api_pom.xml
+++ b/tools/maven/gerrit-plugin-api_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-plugin-api</artifactId>
-  <version>2.16.9</version>
+  <version>2.16.10-SNAPSHOT</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Plugin API</name>
   <description>API for Gerrit Plugins</description>
diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml
index 4f8d9e191c..1755211eed 100644
--- a/tools/maven/gerrit-plugin-gwtui_pom.xml
+++ b/tools/maven/gerrit-plugin-gwtui_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-plugin-gwtui</artifactId>
-  <version>2.16.9</version>
+  <version>2.16.10-SNAPSHOT</version>
   <packaging>jar</packaging>
   <name>Gerrit Code Review - Plugin GWT UI</name>
   <description>Common Classes for Gerrit GWT UI Plugins</description>
diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml
index 962ff4d917..4e386032f9 100644
--- a/tools/maven/gerrit-war_pom.xml
+++ b/tools/maven/gerrit-war_pom.xml
@@ -2,7 +2,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.gerrit</groupId>
   <artifactId>gerrit-war</artifactId>
-  <version>2.16.9</version>
+  <version>2.16.10-SNAPSHOT</version>
   <packaging>war</packaging>
   <name>Gerrit Code Review - WAR</name>
   <description>Gerrit WAR</description>
diff --git a/version.bzl b/version.bzl
index b67a438cd0..06f733ee6a 100644
--- a/version.bzl
+++ b/version.bzl
@@ -2,4 +2,4 @@
 # Used by :api_install and :api_deploy targets
 # when talking to the destination repository.
 #
-GERRIT_VERSION = "2.16.9"
+GERRIT_VERSION = "2.16.10-SNAPSHOT"

From da6d10f7d7a0a2a7c1951f3e96a9777ada451fd1 Mon Sep 17 00:00:00 2001
From: David Pursehouse <dpursehouse@collab.net>
Date: Thu, 13 Jun 2019 13:35:46 +0900
Subject: [PATCH 08/12] AbstractPushForReview: Add tests for pushing with
 skip-validation option

The option to skip commit validation was added in change I80ad47852,
with the name `bypass-review`, which was later renamed in I012e1ea42 to
`skip-validation`. In the former change it was noted in a post-submit
review that no tests were included.

Add test coverage for usage of the skip-validation option, i.e. the
following use cases:

1. Commit validator is invoked on a normal push.

2. Commit validator is not invoked when using the skip-validation option.

3. Push is rejected when using the skip-validation option without having
   the necessary permissions granted. The push is rejected before the
   validator is invoked.

Change-Id: Iac2f1a83ad755d1b29b964b3e42d735379965a60
---
 .../acceptance/git/AbstractPushForReview.java | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 420fc044a4..27065b38d5 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -65,6 +65,8 @@ import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.EditInfoSubject;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -72,16 +74,22 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.git.receive.ReceiveConstants;
+import com.google.gerrit.server.git.validators.CommitValidationException;
+import com.google.gerrit.server.git.validators.CommitValidationListener;
+import com.google.gerrit.server.git.validators.CommitValidationMessage;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.project.Util;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.testutil.FakeEmailSender.Message;
 import com.google.gerrit.testutil.TestTimeUtil;
+import com.google.inject.Inject;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.EnumSet;
 import java.util.HashMap;
@@ -89,6 +97,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import org.eclipse.jgit.api.errors.GitAPIException;
@@ -117,6 +126,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
   private LabelType patchSetLock;
 
+  @Inject private DynamicSet<CommitValidationListener> commitValidators;
+
   @BeforeClass
   public static void setTimeForTesting() {
     TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -1952,6 +1963,57 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
         .isEqualTo(Iterables.getLast(commits).name());
   }
 
+  private static class TestValidator implements CommitValidationListener {
+    private final AtomicInteger count = new AtomicInteger();
+
+    @Override
+    public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+        throws CommitValidationException {
+      count.incrementAndGet();
+      return Collections.emptyList();
+    }
+
+    public int count() {
+      return count.get();
+    }
+  }
+
+  @Test
+  public void skipValidation() throws Exception {
+    String master = "refs/heads/master";
+    TestValidator validator = new TestValidator();
+    RegistrationHandle handle = commitValidators.add(validator);
+
+    try {
+      // Validation listener is called on normal push
+      PushOneCommit push =
+          pushFactory.create(db, admin.getIdent(), testRepo, "change1", "a.txt", "content");
+      PushOneCommit.Result r = push.to(master);
+      r.assertOkStatus();
+      assertThat(validator.count()).isEqualTo(1);
+
+      // Push is rejected and validation listener is not called when not allowed
+      // to use skip option
+      PushOneCommit push2 =
+          pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
+      push2.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION));
+      r = push2.to(master);
+      r.assertErrorStatus("skip validation not permitted for " + master);
+      assertThat(validator.count()).isEqualTo(1);
+
+      // Validation listener is not called when skip option is used
+      grantSkipValidation(project, master, SystemGroupBackend.REGISTERED_USERS);
+      PushOneCommit push3 =
+          pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
+      push3.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION));
+      r = push3.to(master);
+      r.assertOkStatus();
+      assertThat(validator.count()).isEqualTo(1);
+    } finally {
+      handle.remove();
+    }
+  }
+
   @Test
   public void pushToPublishMagicBranchIsAllowed() throws Exception {
     // Push to "refs/publish/*" will be a synonym of "refs/for/*".

From 3ea4b005a2dac3108e69fe4c6ff3b2aa92a50375 Mon Sep 17 00:00:00 2001
From: Mihaly Petrenyi <e.mihaly.petrenyi@gmail.com>
Date: Fri, 14 Jun 2019 13:14:44 +0200
Subject: [PATCH 09/12] Disallow change index task duplication

Do not allow change index and reindex tasks to be queued multiple times
for a given change.

Make sure to remove the index or reindex task from the corresponding
queue when it gets executed.

Change-Id: Ia68c1e541a3ed36873d3aa3b13548e93393b78ac
---
 .../server/index/change/ChangeIndexer.java    | 62 ++++++++++++++++++-
 .../index/change/ReindexAfterRefUpdate.java   | 40 +++++++++++-
 2 files changed, 97 insertions(+), 5 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index f1a7e859cd..6d1971e2d4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.server.extensions.events.EventUtil.logEventListe
 import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
 
 import com.google.common.base.Function;
+import com.google.common.base.Objects;
 import com.google.common.util.concurrent.Atomics;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -50,7 +51,9 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicReference;
@@ -111,6 +114,11 @@ public class ChangeIndexer {
   private final StalenessChecker stalenessChecker;
   private final boolean autoReindexIfStale;
 
+  private final Set<IndexTask> queuedIndexTasks =
+      Collections.newSetFromMap(new ConcurrentHashMap<>());
+  private final Set<ReindexIfStaleTask> queuedReindexIfStaleTasks =
+      Collections.newSetFromMap(new ConcurrentHashMap<>());
+
   @AssistedInject
   ChangeIndexer(
       @GerritServerConfig Config cfg,
@@ -178,7 +186,11 @@ public class ChangeIndexer {
   @SuppressWarnings("deprecation")
   public com.google.common.util.concurrent.CheckedFuture<?, IOException> indexAsync(
       Project.NameKey project, Change.Id id) {
-    return submit(new IndexTask(project, id));
+    IndexTask task = new IndexTask(project, id);
+    if (queuedIndexTasks.add(task)) {
+      return submit(task);
+    }
+    return Futures.immediateCheckedFuture(null);
   }
 
   /**
@@ -309,7 +321,11 @@ public class ChangeIndexer {
   @SuppressWarnings("deprecation")
   public com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStale(
       Project.NameKey project, Change.Id id) {
-    return submit(new ReindexIfStaleTask(project, id), batchExecutor);
+    ReindexIfStaleTask task = new ReindexIfStaleTask(project, id);
+    if (queuedReindexIfStaleTasks.add(task)) {
+      return submit(task, batchExecutor);
+    }
+    return Futures.immediateCheckedFuture(false);
   }
 
   private void autoReindexIfStale(ChangeData cd) {
@@ -351,6 +367,8 @@ public class ChangeIndexer {
 
     protected abstract T callImpl(Provider<ReviewDb> db) throws Exception;
 
+    protected abstract void remove();
+
     @Override
     public abstract String toString();
 
@@ -405,15 +423,35 @@ public class ChangeIndexer {
 
     @Override
     public Void callImpl(Provider<ReviewDb> db) throws Exception {
+      remove();
       ChangeData cd = newChangeData(db.get(), project, id);
       index(cd);
       return null;
     }
 
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(IndexTask.class, id.get());
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      if (!(obj instanceof IndexTask)) {
+        return false;
+      }
+      IndexTask other = (IndexTask) obj;
+      return id.get() == other.id.get();
+    }
+
     @Override
     public String toString() {
       return "index-change-" + id;
     }
+
+    @Override
+    protected void remove() {
+      queuedIndexTasks.remove(this);
+    }
   }
 
   // Not AbstractIndexTask as it doesn't need ReviewDb.
@@ -445,6 +483,7 @@ public class ChangeIndexer {
 
     @Override
     public Boolean callImpl(Provider<ReviewDb> db) throws Exception {
+      remove();
       try {
         if (stalenessChecker.isStale(id)) {
           index(newChangeData(db.get(), project, id));
@@ -464,10 +503,29 @@ public class ChangeIndexer {
       return false;
     }
 
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(ReindexIfStaleTask.class, id.get());
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      if (!(obj instanceof ReindexIfStaleTask)) {
+        return false;
+      }
+      ReindexIfStaleTask other = (ReindexIfStaleTask) obj;
+      return id.get() == other.id.get();
+    }
+
     @Override
     public String toString() {
       return "reindex-if-stale-change-" + id;
     }
+
+    @Override
+    protected void remove() {
+      queuedReindexIfStaleTasks.remove(this);
+    }
   }
 
   private boolean isCausedByRepositoryNotFoundException(Throwable throwable) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
index a2a4507cc9..a59557f375 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -17,6 +17,7 @@ package com.google.gerrit.server.index.change;
 import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static com.google.gerrit.server.query.change.ChangeData.asChanges;
 
+import com.google.common.base.Objects;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListeningExecutorService;
@@ -42,8 +43,11 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Future;
 import org.eclipse.jgit.lib.Config;
 import org.slf4j.Logger;
@@ -62,6 +66,8 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
   private final ListeningExecutorService executor;
   private final boolean enabled;
 
+  private final Set<Index> queuedIndexTasks = Collections.newSetFromMap(new ConcurrentHashMap<>());
+
   @Inject
   ReindexAfterRefUpdate(
       @GerritServerConfig Config cfg,
@@ -109,9 +115,12 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
           @Override
           public void onSuccess(List<Change> changes) {
             for (Change c : changes) {
-              // Don't retry indefinitely; if this fails changes may be stale.
-              @SuppressWarnings("unused")
-              Future<?> possiblyIgnoredError = executor.submit(new Index(event, c.getId()));
+              Index task = new Index(event, c.getId());
+              if (queuedIndexTasks.add(task)) {
+                // Don't retry indefinitely; if this fails changes may be stale.
+                @SuppressWarnings("unused")
+                Future<?> possiblyIgnoredError = executor.submit(task);
+              }
             }
           }
 
@@ -141,6 +150,8 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
     }
 
     protected abstract V impl(RequestContext ctx) throws Exception;
+
+    protected abstract void remove();
   }
 
   private class GetChanges extends Task<List<Change>> {
@@ -165,6 +176,9 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
           + " update of project "
           + event.getProjectName();
     }
+
+    @Override
+    protected void remove() {}
   }
 
   private class Index extends Task<Void> {
@@ -179,6 +193,7 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
     protected Void impl(RequestContext ctx) throws OrmException, IOException {
       // Reload change, as some time may have passed since GetChanges.
       ReviewDb db = ctx.getReviewDbProvider().get();
+      remove();
       try {
         Change c =
             notesFactory
@@ -191,9 +206,28 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
       return null;
     }
 
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(Index.class, id.get());
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      if (!(obj instanceof Index)) {
+        return false;
+      }
+      Index other = (Index) obj;
+      return id.get() == other.id.get();
+    }
+
     @Override
     public String toString() {
       return "Index change " + id.get() + " of project " + event.getProjectName();
     }
+
+    @Override
+    protected void remove() {
+      queuedIndexTasks.remove(this);
+    }
   }
 }

From 082139a71d24cef4a0b632cb966a0e9b930e99ae Mon Sep 17 00:00:00 2001
From: Marco Miller <marco.miller@ericsson.com>
Date: Mon, 17 Jun 2019 15:00:35 -0400
Subject: [PATCH 10/12] Elasticsearch: Base the default number of shards on ES
 version

Before Elasticsearch 7.0, the default number of shards was 5. Since
version 6, there is a warning about that default number changing to 1
starting from 7.0.0. The related Issue 9768 first reported about it.

Set the default number of shards to 1 starting with 7.0, to remove that
index creation warning with V7, thus fit with what Elasticsearch 7.0+
expects. Stick to the previous default for versions earlier than 7, to
not potentially jeopardize existing deployments based on ES 6 or 5.

Making this value non-default requires a corresponding configuration
change on the Elasticsearch server side. And changing the number of
shards for the latter is a non-trivial procedure. [1] introduces that
and is already part of the Gerrit documentation.

[1] https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-concepts.html#getting-started-shards-and-replicas

Bug: Issue 10499
Change-Id: I9e91c471b48c4c4c893927c153a5661dfccc2bcb
---
 Documentation/config-gerrit.txt                          | 2 +-
 .../gerrit/elasticsearch/AbstractElasticIndex.java       | 6 +++---
 .../gerrit/elasticsearch/ElasticConfiguration.java       | 9 ++++++++-
 .../google/gerrit/elasticsearch/ElasticQueryAdapter.java | 6 ++++++
 .../com/google/gerrit/elasticsearch/ElasticSetting.java  | 8 ++++----
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 77d18dbf12..89ea2e876e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2930,7 +2930,7 @@ Sets the number of shards to use per index. Refer to the
 link:https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-concepts.html#getting-started-shards-and-replicas[
 Elasticsearch documentation] for details.
 +
-Defaults to 5.
+Defaults to 5 for Elasticsearch versions 5 and 6, and to 1 starting with Elasticsearch 7.
 
 [[elasticsearch.numberOfReplicas]]elasticsearch.numberOfReplicas::
 +
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
index d480e59376..a5db030474 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
@@ -162,7 +162,7 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
     }
 
     // Recreate the index.
-    String indexCreationFields = concatJsonString(getSettings(), getMappings());
+    String indexCreationFields = concatJsonString(getSettings(client.adapter()), getMappings());
     response =
         performRequest(
             "PUT", indexName + client.adapter().includeTypeNameParam(), indexCreationFields);
@@ -177,8 +177,8 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
 
   protected abstract String getMappings();
 
-  private String getSettings() {
-    return gson.toJson(ImmutableMap.of(SETTINGS, ElasticSetting.createSetting(config)));
+  private String getSettings(ElasticQueryAdapter adapter) {
+    return gson.toJson(ImmutableMap.of(SETTINGS, ElasticSetting.createSetting(config, adapter)));
   }
 
   protected abstract String getId(V v);
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
index 821e89f976..4ec5feb0e0 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
@@ -43,7 +43,7 @@ class ElasticConfiguration {
   static final String KEY_NUMBER_OF_REPLICAS = "numberOfReplicas";
   static final String DEFAULT_PORT = "9200";
   static final String DEFAULT_USERNAME = "elastic";
-  static final int DEFAULT_NUMBER_OF_SHARDS = 5;
+  static final int DEFAULT_NUMBER_OF_SHARDS = 0;
   static final int DEFAULT_NUMBER_OF_REPLICAS = 1;
 
   private final Config cfg;
@@ -101,4 +101,11 @@ class ElasticConfiguration {
   String getIndexName(String name, int schemaVersion) {
     return String.format("%s%s_%04d", prefix, name, schemaVersion);
   }
+
+  int getNumberOfShards(ElasticQueryAdapter adapter) {
+    if (numberOfShards == DEFAULT_NUMBER_OF_SHARDS) {
+      return adapter.getDefaultNumberOfShards();
+    }
+    return numberOfShards;
+  }
 }
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
index d762b571c7..dfb1cbf17c 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
@@ -27,6 +27,7 @@ public class ElasticQueryAdapter {
   private final boolean useV5Type;
   private final boolean useV6Type;
   private final boolean omitType;
+  private final int defaultNumberOfShards;
 
   private final String searchFilteringName;
   private final String indicesExistParams;
@@ -40,6 +41,7 @@ public class ElasticQueryAdapter {
     this.useV5Type = !version.isV6OrLater();
     this.useV6Type = version.isV6();
     this.omitType = version.isV7OrLater();
+    this.defaultNumberOfShards = version.isV7OrLater() ? 1 : 5;
     this.versionDiscoveryUrl = version.isV6OrLater() ? "/%s*" : "/%s*/_aliases";
     this.searchFilteringName = "_source";
     this.indicesExistParams =
@@ -92,6 +94,10 @@ public class ElasticQueryAdapter {
     return omitType;
   }
 
+  int getDefaultNumberOfShards() {
+    return defaultNumberOfShards;
+  }
+
   String getType() {
     return getType("");
   }
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticSetting.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticSetting.java
index 98c313c78a..14e462342d 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticSetting.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticSetting.java
@@ -22,18 +22,18 @@ class ElasticSetting {
   private static final ImmutableMap<String, String> CUSTOM_CHAR_MAPPING =
       ImmutableMap.of("\\u002E", "\\u0020", "\\u005F", "\\u0020");
 
-  static SettingProperties createSetting(ElasticConfiguration config) {
-    return new ElasticSetting.Builder().addCharFilter().addAnalyzer().build(config);
+  static SettingProperties createSetting(ElasticConfiguration config, ElasticQueryAdapter adapter) {
+    return new ElasticSetting.Builder().addCharFilter().addAnalyzer().build(config, adapter);
   }
 
   static class Builder {
     private final ImmutableMap.Builder<String, FieldProperties> fields =
         new ImmutableMap.Builder<>();
 
-    SettingProperties build(ElasticConfiguration config) {
+    SettingProperties build(ElasticConfiguration config, ElasticQueryAdapter adapter) {
       SettingProperties properties = new SettingProperties();
       properties.analysis = fields.build();
-      properties.numberOfShards = config.numberOfShards;
+      properties.numberOfShards = config.getNumberOfShards(adapter);
       properties.numberOfReplicas = config.numberOfReplicas;
       return properties;
     }

From 29ab7f8a5d9f1ca2b6ad7e406433679abbc15e66 Mon Sep 17 00:00:00 2001
From: David Pursehouse <dpursehouse@collab.net>
Date: Tue, 18 Jun 2019 14:40:16 +0900
Subject: [PATCH 11/12] CommentSender: Use UrlFormatter to get URLs for file
 and comments

CommentSender is hard coded to provide the direct URL to a filename
within a patch, and to the inline comments. The hard-coded URLs are
in the legacy format, meaning they do not include the project name
and are inconsistent with other links included in notifications and
console messages from Gerrit.

Hard-coding the URLs also means it's not possible for plugins to
provide alternative URLs.

Add new methods on the UrlFormatter interface, and provide default
implementations that include the project name, thus making the URLs
consistent with other URLs emitted by Gerrit, and also allowing
plugins to provide alternatives.

Change-Id: I0991dea120856c2c2bf1304749961e28b0b73a24
---
 .../gerrit/server/config/UrlFormatter.java    | 13 +++++++
 .../server/mail/send/CommentSender.java       | 34 ++++++++-----------
 .../server/mail/send/OutgoingEmail.java       |  2 +-
 .../acceptance/server/change/CommentsIT.java  | 32 ++++++++++++-----
 4 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/java/com/google/gerrit/server/config/UrlFormatter.java b/java/com/google/gerrit/server/config/UrlFormatter.java
index 066a3cabce..3e94c306fa 100644
--- a/java/com/google/gerrit/server/config/UrlFormatter.java
+++ b/java/com/google/gerrit/server/config/UrlFormatter.java
@@ -47,6 +47,19 @@ public interface UrlFormatter {
     return getWebUrl().map(url -> url + "c/" + project.get() + "/+/" + id.get());
   }
 
+  /** Returns the URL for viewing a file in a given patch set of a change. */
+  default Optional<String> getPatchFileView(Change change, int patchsetId, String filename) {
+    return getChangeViewUrl(change.getProject(), change.getId())
+        .map(url -> url + "/" + patchsetId + "/" + filename);
+  }
+
+  /** Returns the URL for viewing a comment in a file in a given patch set of a change. */
+  default Optional<String> getInlineCommentView(
+      Change change, int patchsetId, String filename, short side, int startLine) {
+    return getPatchFileView(change, patchsetId, filename)
+        .map(url -> url + String.format("@%s%d", side == 0 ? "a" : "", startLine));
+  }
+
   /** Returns a URL pointing to a section of the settings page. */
   default Optional<String> getSettingsUrl(@Nullable String section) {
     return getWebUrl()
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java
index 69a79266ec..e78a60a215 100644
--- a/java/com/google/gerrit/server/mail/send/CommentSender.java
+++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -75,21 +75,19 @@ public class CommentSender extends ReplyToChangeSender {
     public List<Comment> comments = new ArrayList<>();
 
     /** @return a web link to the given patch set and file. */
-    public String getLink() {
-      String url = getGerritUrl();
-      if (url == null) {
-        return null;
-      }
+    public String getFileLink() {
+      return args.urlFormatter
+          .get()
+          .getPatchFileView(change, patchSetId, KeyUtil.encode(filename))
+          .orElse(null);
+    }
 
-      return new StringBuilder()
-          .append(url)
-          .append("#/c/")
-          .append(change.getId())
-          .append('/')
-          .append(patchSetId)
-          .append('/')
-          .append(KeyUtil.encode(filename))
-          .toString();
+    /** @return a web link to a comment within a given patch set and file. */
+    public String getCommentLink(short side, int startLine) {
+      return args.urlFormatter
+          .get()
+          .getInlineCommentView(change, patchSetId, KeyUtil.encode(filename), side, startLine)
+          .orElse(null);
     }
 
     /**
@@ -392,7 +390,7 @@ public class CommentSender extends ReplyToChangeSender {
 
     for (CommentSender.FileCommentGroup group : getGroupedInlineComments(repo)) {
       Map<String, Object> groupData = new HashMap<>();
-      groupData.put("link", group.getLink());
+      groupData.put("link", group.getFileLink());
       groupData.put("title", group.getTitle());
       groupData.put("patchSetId", group.patchSetId);
 
@@ -421,11 +419,9 @@ public class CommentSender extends ReplyToChangeSender {
 
         // Set the comment link.
         if (comment.lineNbr == 0) {
-          commentData.put("link", group.getLink());
-        } else if (comment.side == 0) {
-          commentData.put("link", group.getLink() + "@a" + startLine);
+          commentData.put("link", group.getFileLink());
         } else {
-          commentData.put("link", group.getLink() + '@' + startLine);
+          commentData.put("link", group.getCommentLink(comment.side, startLine));
         }
 
         // Set robot comment data.
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index 1ef3b25c75..b0f1e752f6 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -298,7 +298,7 @@ public abstract class OutgoingEmail {
     return null;
   }
 
-  public String getGerritUrl() {
+  private String getGerritUrl() {
     return args.urlFormatter.get().getWebUrl().orElse(null);
   }
 
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index e9d2611143..535d461874 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -629,13 +629,17 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "comments\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/1/a.txt \n"
                 + "File a.txt:\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/1/a.txt@a2 \n"
                 + "PS1, Line 2: \n"
@@ -643,7 +647,9 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/1/a.txt@1 \n"
                 + "PS1, Line 1: boring\n"
@@ -651,13 +657,17 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/2/a.txt \n"
                 + "File a.txt:\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/2/a.txt@a1 \n"
                 + "PS2, Line 1: \n"
@@ -665,7 +675,9 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/2/a.txt@a2 \n"
                 + "PS2, Line 2: \n"
@@ -673,7 +685,9 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/2/a.txt@1 \n"
                 + "PS2, Line 1: interesting\n"
@@ -681,7 +695,9 @@ public class CommentsIT extends AbstractDaemonTest {
                 + "\n"
                 + "\n"
                 + url
-                + "#/c/"
+                + "c/"
+                + project.get()
+                + "/+/"
                 + c
                 + "/2/a.txt@2 \n"
                 + "PS2, Line 2: nten\n"

From 87b2bc691983a49b8d378c3e01bc4363d4911afe Mon Sep 17 00:00:00 2001
From: David Pursehouse <dpursehouse@collab.net>
Date: Tue, 18 Jun 2019 17:29:27 +0900
Subject: [PATCH 12/12] OutgoingEmail: Use UrlFormatter to get settings URL

The settings URL is hard coded based on the site URL, rather than
using the one provided by UrlFormatter.

Extend the UrlFormatter interface to support both the base settings
URL, and URLs for specific settings sections.

Change-Id: Ia4cf6566c3c786fa3822fe3bfd59aa9639717ed3
---
 .../google/gerrit/server/config/UrlFormatter.java   | 13 +++++++++++--
 .../gerrit/server/mail/send/OutgoingEmail.java      |  8 +-------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/java/com/google/gerrit/server/config/UrlFormatter.java b/java/com/google/gerrit/server/config/UrlFormatter.java
index 3e94c306fa..740daf02dc 100644
--- a/java/com/google/gerrit/server/config/UrlFormatter.java
+++ b/java/com/google/gerrit/server/config/UrlFormatter.java
@@ -61,9 +61,18 @@ public interface UrlFormatter {
   }
 
   /** Returns a URL pointing to a section of the settings page. */
+  default Optional<String> getSettingsUrl() {
+    return getWebUrl().map(url -> url + "settings");
+  }
+
+  /**
+   * Returns a URL pointing to a section of the settings page, or the settings page if {@code
+   * section} is null.
+   */
   default Optional<String> getSettingsUrl(@Nullable String section) {
-    return getWebUrl()
-        .map(url -> url + "settings" + (Strings.isNullOrEmpty(section) ? "" : "#" + section));
+    return Strings.isNullOrEmpty(section)
+        ? getSettingsUrl()
+        : getSettingsUrl().map(url -> url + "#" + section);
   }
 
   /** Returns a URL pointing to a documentation page, at a given named anchor. */
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index b0f1e752f6..7ad5329133 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -289,13 +289,7 @@ public abstract class OutgoingEmail {
   }
 
   public String getSettingsUrl() {
-    if (getGerritUrl() != null) {
-      final StringBuilder r = new StringBuilder();
-      r.append(getGerritUrl());
-      r.append("settings");
-      return r.toString();
-    }
-    return null;
+    return args.urlFormatter.get().getSettingsUrl().orElse(null);
   }
 
   private String getGerritUrl() {