Merge branch 'stable-2.16' into stable-3.0

* stable-2.16:
  OutgoingEmail: Use UrlFormatter to get settings URL
  CommentSender: Use UrlFormatter to get URLs for file and comments
  Elasticsearch: Base the default number of shards on ES version
  Disallow change index task duplication
  AbstractPushForReview: Add tests for pushing with skip-validation option
  Set version to 2.16.10-SNAPSHOT
  Set version to 2.16.9
  Documentation: Fix the Elasticsearch shards/replicas link
  ProjectControl: Allow regexes ref strings for uploads
  ProjectControl: Allow regexes ref strings for tags
  ProjectControl: Reuse constants for ref strings
  Add extension point to gr-user-header

Change-Id: I6a38b9bfdc2bcbb8c457ce24883767f4579ca656

java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java

@@ -198,7 +198,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);
@@ -213,8 +213,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);

java/com/google/gerrit/elasticsearch/ElasticConfiguration.java

@@ -42,7 +42,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;
@@ -100,4 +100,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;
+  }
 }

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;
@@ -41,6 +42,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 =
@@ -98,6 +100,10 @@ public class ElasticQueryAdapter {
     return omitType;
   }
 
+  int getDefaultNumberOfShards() {
+    return defaultNumberOfShards;
+  }
+
   String getType() {
     return getType("");
   }

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;
     }

java/com/google/gerrit/server/config/UrlFormatter.java

@@ -47,10 +47,32 @@ 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() {
+    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. */

java/com/google/gerrit/server/index/change/ChangeIndexer.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.index.change;
 
 import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
 
+import com.google.common.base.Objects;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -40,7 +41,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.Future;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Config;
@@ -70,6 +73,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,
@@ -123,7 +131,11 @@ public class ChangeIndexer {
    * @return future for the indexing task.
    */
   public ListenableFuture<?> 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.immediateFuture(null);
   }
 
   /**
@@ -239,7 +251,11 @@ public class ChangeIndexer {
    * @return future for reindexing the change; returns true if the change was stale.
    */
   public ListenableFuture<Boolean> 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) {
@@ -278,6 +294,8 @@ public class ChangeIndexer {
 
     protected abstract T callImpl() throws Exception;
 
+    protected abstract void remove();
+
     @Override
     public abstract String toString();
 
@@ -308,15 +326,35 @@ public class ChangeIndexer {
 
     @Override
     public Void callImpl() throws Exception {
+      remove();
       ChangeData cd = changeDataFactory.create(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 a request context.
@@ -352,6 +390,7 @@ public class ChangeIndexer {
 
     @Override
     public Boolean callImpl() throws Exception {
+      remove();
       try {
         if (stalenessChecker.isStale(id)) {
           indexImpl(changeDataFactory.create(project, id));
@@ -368,10 +407,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) {

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.flogger.FluentLogger;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
@@ -42,8 +43,11 @@ import com.google.gerrit.server.util.RequestContext;
 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;
 
@@ -61,6 +65,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,
@@ -107,9 +113,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);
+              }
             }
           }
 
@@ -139,6 +148,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>> {
@@ -163,6 +174,9 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
           + " update of project "
           + event.getProjectName();
     }
+
+    @Override
+    protected void remove() {}
   }
 
   private class Index extends Task<Void> {
@@ -176,6 +190,7 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
     @Override
     protected Void impl(RequestContext ctx) throws IOException {
       // Reload change, as some time may have passed since GetChanges.
+      remove();
       try {
         Change c =
             notesFactory.createChecked(new Project.NameKey(event.getProjectName()), id).getChange();
@@ -186,9 +201,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);
+    }
   }
 }

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);
     }
 
     /**
@@ -391,7 +389,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);
 
@@ -420,11 +418,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.

java/com/google/gerrit/server/mail/send/OutgoingEmail.java

@@ -282,16 +282,10 @@ 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);
   }
 
-  public String getGerritUrl() {
+  private String getGerritUrl() {
     return args.urlFormatter.get().getWebUrl().orElse(null);
   }
 

java/com/google/gerrit/server/permissions/ProjectControl.java

@@ -15,7 +15,10 @@
 package com.google.gerrit.server.permissions;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.common.data.AccessSection.ALL;
+import static com.google.gerrit.common.data.AccessSection.REGEX_PREFIX;
 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.Sets;
 import com.google.gerrit.common.data.AccessSection;
@@ -139,7 +142,7 @@ class ProjectControl {
 
   /** Is this user a project owner? */
   boolean isOwner() {
-    return (isDeclaredOwner() && controlForRef("refs/*").canPerform(Permission.OWNER)) || isAdmin();
+    return (isDeclaredOwner() && controlForRef(ALL).canPerform(Permission.OWNER)) || isAdmin();
   }
 
   /**
@@ -200,7 +203,8 @@ class ProjectControl {
   private boolean canCreateChanges() {
     for (SectionMatcher matcher : access()) {
       AccessSection section = matcher.getSection();
-      if (section.getName().startsWith("refs/for/")) {
+      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;
@@ -222,7 +226,8 @@ class ProjectControl {
     for (SectionMatcher matcher : access()) {
       AccessSection section = matcher.getSection();
 
-      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;
@@ -276,7 +281,7 @@ 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.