Merge branch 'stable-2.9' into stable-2.10

* stable-2.9:
  Add full names for options on list groups REST API
  Add full names for options on list projects REST API
  Make `-S` an alias of `--start` in changes query REST API
  Mention deprecation of sortkey parameters in 2.9 release notes
  Run change hooks and ref-updated events after indexing is done.
  Fix broken formatting in changes REST documentation
  Restrict the input of plugin_archetype_deploy.sh
  Gracefully handle `buck audit` failure
  Revert "Make VisibleRefFilter.Filter reuse the refs passed from JGit."

Conflicts:
	gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
	gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
	tools/pack_war.py

Change-Id: I89a9b42c049ec1365ef6dec53c73c7a10a41e888

Documentation/rest-api-changes.txt

@@ -135,11 +135,10 @@ Query for open changes of watched projects:
 
 If the `n` query parameter is supplied and additional changes exist
 that match the query beyond the end, the last change object has a
-`_more_changes: true` JSON field set. Callers can resume a query with
+`_more_changes: true` JSON field set.
-the `N` query parameter, supplying the last change's `_sortkey` field
+
-as the value. When going in the reverse direction with the `P` query
+The `S` or `start` query parameter can be supplied to skip a number
-parameter a `_more_changes: true` is put in the first change object if
+of changes from the list.
-there are results *before* the first change returned.
 
 Clients are allowed to specify more than one query by setting the `q`
 parameter multiple times. In this case the result is an array of
@@ -3407,7 +3406,7 @@ In addition `ReviewerInfo` has the following fields:
 |Field Name    |Description
 |`approvals`   |
 The approvals of the reviewer as a map that maps the label names to the
-approval values ("`-2`", "`-1`", " `0`", "`+1`", "`+2`").
+approval values ("`-2`", "`-1`", "`0`", "`+1`", "`+2`").
 |==========================
 
 [[reviewer-input]]

ReleaseNotes/ReleaseNotes-2.9.txt

@@ -33,6 +33,9 @@ Important Notes
 *WARNING:* Support for query via the SQL index is removed. The usage of
 a secondary index is now mandatory.
 
+*WARNING:* The `sortkey` and `sortkey_prev` options on the query changes
+REST endpoint are link:#sortkey-deprecation[deprecated].
+
 *WARNING:* The new change screen only displays download commands if the
 `download-commands` core plugin or any other plugin providing download
 commands is installed. The `download-commands` plugin provides the
@@ -299,6 +302,22 @@ REST API
 ~~~~~~~~
 
 
+Changes
+^^^^^^^
+
+
+[[sortkey-deprecation]]
+* Results returned by the
+link:https://gerrit-documentation.storage.googleapis.com/Documentation/2.9/rest-api-changes.html#list-changes[
+query changes] endpoint are now paginated using offsets instead of sortkeys.
++
+The `sortkey` and `sortkey_prev` parameters on the endpoint are deprecated.  The
+results are now paginated using the `--limit` (`-n`) option to limit the number
+of results, and the `-S` option to set the start point.
++
+Queries with sortkeys are still supported against old index versions, to enable
+online reindexing while clients have an older JS version.
+
 Projects
 ^^^^^^^^
 

gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java

@@ -133,13 +133,13 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
     } catch (Exception e) {
       log.error("Cannot email update for change " + change.getChangeId(), e);
     }
+    indexFuture.checkedGet();
     hooks.doChangeAbandonedHook(change,
         caller.getAccount(),
         db.patchSets().get(change.currentPatchSetId()),
         Strings.emptyToNull(input.message),
         db);
     ChangeInfo result = json.format(change);
-    indexFuture.checkedGet();
     return result;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java

@@ -200,12 +200,6 @@ public class ChangeInserter {
     if(!messageIsForChange()) {
       commitMessageNotForChange();
     }
-    gitRefUpdated.fire(change.getProject(), patchSet.getRefName(),
-        ObjectId.zeroId(), commit);
-
-    if (runHooks) {
-      hooks.doPatchsetCreatedHook(change, patchSet, db);
-    }
 
     if (sendMail) {
       try {
@@ -221,6 +215,14 @@ public class ChangeInserter {
       }
     }
     f.checkedGet();
+
+    gitRefUpdated.fire(change.getProject(), patchSet.getRefName(),
+        ObjectId.zeroId(), commit);
+
+    if (runHooks) {
+      hooks.doPatchsetCreatedHook(change, patchSet, db);
+    }
+
     return change;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.collect.SetMultimap;
-import com.google.common.util.concurrent.CheckedFuture;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -316,14 +315,13 @@ public class PatchSetInserter {
     } finally {
       db.rollback();
     }
-    CheckedFuture<?, IOException> f = mergeabilityChecker.newCheck()
+    mergeabilityChecker.newCheck()
         .addChange(updatedChange)
         .reindex()
-        .runAsync();
+        .run();
     if (runHooks) {
       hooks.doPatchsetCreatedHook(updatedChange, patchSet, db);
     }
-    f.checkedGet();
     return updatedChange;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -179,22 +179,22 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     } else {
       indexWrite = Futures.<Void, IOException> immediateCheckedFuture(null);
     }
-    if (message != null) {
+    if (message != null && input.notify.compareTo(NotifyHandling.NONE) > 0) {
-      if (input.notify.compareTo(NotifyHandling.NONE) > 0) {
+      email.create(
-        email.create(
+          input.notify,
-            input.notify,
+          change,
-            change,
+          revision.getPatchSet(),
-            revision.getPatchSet(),
+          revision.getAccountId(),
-            revision.getAccountId(),
+          message,
-            message,
+          comments).sendAsync();
-            comments).sendAsync();
-      }
-      fireCommentAddedHook(revision);
     }
 
     Output output = new Output();
     output.labels = input.labels;
     indexWrite.checkedGet();
+    if (message != null) {
+      fireCommentAddedHook(revision);
+    }
     return output;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java

@@ -249,24 +249,23 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
           ImmutableList.of(psa)));
     }
     accountLoaderFactory.create(true).fill(result.reviewers);
-    postAdd(rsrc.getChange(), added);
+    emailReviewers(rsrc.getChange(), added);
     indexFuture.checkedGet();
+    if (!added.isEmpty()) {
+      PatchSet patchSet = dbProvider.get().patchSets().get(rsrc.getChange().currentPatchSetId());
+      for (PatchSetApproval psa : added) {
+        Account account = accountCache.get(psa.getAccountId()).getAccount();
+        hooks.doReviewerAddedHook(rsrc.getChange(), account, patchSet, dbProvider.get());
+      }
+    }
   }
 
-  private void postAdd(Change change, List<PatchSetApproval> added)
+  private void emailReviewers(Change change, List<PatchSetApproval> added)
       throws OrmException, EmailException {
     if (added.isEmpty()) {
       return;
     }
 
-    // Execute hook for added reviewers
-    //
-    PatchSet patchSet = dbProvider.get().patchSets().get(change.currentPatchSetId());
-    for (PatchSetApproval psa : added) {
-      Account account = accountCache.get(psa.getAccountId()).getAccount();
-      hooks.doReviewerAddedHook(change, account, patchSet, dbProvider.get());
-    }
-
     // Email the reviewers
     //
     // The user knows they added themselves, don't bother emailing them.

gerrit-server/src/main/java/com/google/gerrit/server/change/Publish.java

@@ -96,12 +96,13 @@ public class Publish implements RestModifyView<RevisionResource, Input>,
           || updatedChange.getStatus() == Change.Status.NEW) {
         CheckedFuture<?, IOException> indexFuture =
             indexer.indexAsync(updatedChange.getId());
-        hooks.doDraftPublishedHook(updatedChange, updatedPatchSet, dbProvider.get());
         sender.send(rsrc.getNotes(), update,
             rsrc.getChange().getStatus() == Change.Status.DRAFT,
             rsrc.getUser(), updatedChange, updatedPatchSet,
             rsrc.getControl().getLabelTypes());
         indexFuture.checkedGet();
+        hooks.doDraftPublishedHook(updatedChange, updatedPatchSet,
+            dbProvider.get());
       }
     } catch (PatchSetInfoNotAvailableException e) {
       throw new ResourceNotFoundException(e.getMessage());

gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.server.change;
 
 import com.google.common.base.Strings;
-import com.google.common.util.concurrent.CheckedFuture;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
@@ -123,11 +122,9 @@ class PutTopic implements RestModifyView<ChangeResource, Input>,
         db.rollback();
       }
       update.commit();
-      CheckedFuture<?, IOException> indexFuture =
+      indexer.index(db, change);
-          indexer.indexAsync(change.getId());
       hooks.doTopicChangedHook(change, currentUser.getAccount(),
           oldTopicName, db);
-      indexFuture.checkedGet();
     }
     return Strings.isNullOrEmpty(newTopicName)
         ? Response.<String>none()

gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java

@@ -134,13 +134,13 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
     } catch (Exception e) {
       log.error("Cannot email update for change " + change.getChangeId(), e);
     }
+    f.checkedGet();
     hooks.doChangeRestoredHook(change,
         caller.getAccount(),
         db.patchSets().get(change.currentPatchSetId()),
         Strings.emptyToNull(input.message),
         dbProvider.get());
     ChangeInfo result = json.format(change);
-    f.checkedGet();
     return result;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -859,6 +859,7 @@ public class MergeOp {
       db.commit();
 
       sendMergedEmail(c, submitter);
+      indexer.index(db, c);
       if (submitter != null) {
         try {
           hooks.doChangeMergedHook(c,
@@ -1085,6 +1086,14 @@ public class MergeOp {
       }
     }));
 
+    if (indexFuture != null) {
+      try {
+        indexFuture.checkedGet();
+      } catch (IOException e) {
+        log.error("Failed to index new change message", e);
+      }
+    }
+
     if (submitter != null) {
       try {
         hooks.doMergeFailedHook(c,
@@ -1094,13 +1103,6 @@ public class MergeOp {
         log.error("Cannot run hook for merge failed " + c.getId(), ex);
       }
     }
-    if (indexFuture != null) {
-      try {
-        indexFuture.checkedGet();
-      } catch (IOException e) {
-        log.error("Failed to index new change message", e);
-      }
-    }
   }
 
   private void abandonAllOpenChanges() throws NoSuchChangeException {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -2080,13 +2080,6 @@ public class ReceiveCommits {
           .addChange(change)
           .reindex()
           .runAsync();
-      gitRefUpdated.fire(project.getNameKey(), newPatchSet.getRefName(),
-          ObjectId.zeroId(), newCommit);
-      hooks.doPatchsetCreatedHook(change, newPatchSet, db);
-      if (mergedIntoRef != null) {
-        hooks.doChangeMergedHook(
-            change, currentUser.getAccount(), newPatchSet, db);
-      }
       workQueue.getDefaultQueue()
           .submit(requestScopePropagator.wrap(new Runnable() {
         @Override
@@ -2115,6 +2108,14 @@ public class ReceiveCommits {
       }));
       f.checkedGet();
 
+      gitRefUpdated.fire(project.getNameKey(), newPatchSet.getRefName(),
+          ObjectId.zeroId(), newCommit);
+      hooks.doPatchsetCreatedHook(change, newPatchSet, db);
+      if (mergedIntoRef != null) {
+        hooks.doChangeMergedHook(
+            change, currentUser.getAccount(), newPatchSet, db);
+      }
+
       if (magicBranch != null && magicBranch.isSubmit()) {
         submit(changeCtl, newPatchSet);
       }

gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java

@@ -113,7 +113,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
       TagMatcher tags = tagCache.get(projectName).matcher(
           tagCache,
           db,
-          filterTagsSeperately ? filter(refs).values() : result.values());
+          filterTagsSeperately ? filter(db.getAllRefs()).values() : result.values());
       for (Ref tag : deferredTags) {
         if (tags.isReachable(tag)) {
           result.put(tag.getName(), tag);

gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java

@@ -90,10 +90,10 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   @Option(name = "--limit", aliases = {"-n"}, metaVar = "CNT", usage = "maximum number of groups to list")
   private int limit;
 
-  @Option(name = "-S", metaVar = "CNT", usage = "number of groups to skip")
+  @Option(name = "--start", aliases = {"-S"}, metaVar = "CNT", usage = "number of groups to skip")
   private int start;
 
-  @Option(name = "-m", metaVar = "MATCH", usage = "match group substring")
+  @Option(name = "--match", aliases = {"-m"}, metaVar = "MATCH", usage = "match group substring")
   private String matchSubstring;
 
   @Option(name = "-o", usage = "Output options per group")

gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java

@@ -152,17 +152,17 @@ public class ListProjects implements RestReadView<TopLevelResource> {
     this.limit = limit;
   }
 
-  @Option(name = "-S", metaVar = "CNT", usage = "number of projects to skip")
+  @Option(name = "--start", aliases = {"-S"}, metaVar = "CNT", usage = "number of projects to skip")
   public void setStart(int start) {
     this.start = start;
   }
 
-  @Option(name = "-p", metaVar = "PREFIX", usage = "match project prefix")
+  @Option(name = "--prefix", aliases = {"-p"}, metaVar = "PREFIX", usage = "match project prefix")
   public void setMatchPrefix(String matchPrefix) {
     this.matchPrefix = matchPrefix;
   }
 
-  @Option(name = "-m", metaVar = "MATCH", usage = "match project substring")
+  @Option(name = "--match", aliases = {"-m"}, metaVar = "MATCH", usage = "match project substring")
   public void setMatchSubstring(String matchSubstring) {
     this.matchSubstring = matchSubstring;
   }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java

@@ -80,7 +80,7 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
     imp.setSortkeyBefore(key);
   }
 
-  @Option(name = "-S", metaVar = "CNT", usage = "Number of changes to skip")
+  @Option(name = "--start", aliases = {"-S"}, metaVar = "CNT", usage = "Number of changes to skip")
   public void setStart(int start) {
     imp.setStart(start);
   }

tools/pack_war.py

@@ -35,7 +35,11 @@ def link_jars(libs, directory):
   makedirs(directory)
   while not path.isfile('.buckconfig'):
     chdir('..')
-  cp = check_output(['buck', 'audit', 'classpath'] + libs)
+  try:
+    cp = check_output(['buck', 'audit', 'classpath'] + libs)
+  except Exception as e:
+    print('call to buck audit failed: %s' % e, file=sys.stderr)
+    exit(1)
   for j in cp.strip().splitlines():
     if j not in jars:
       jars.add(j)

tools/plugin_archetype_deploy.sh

@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-help()
+function help
 {
   cat <<'eof'
 Usage: plugin_archetype_deploy [option]
@@ -21,8 +21,9 @@ Usage: plugin_archetype_deploy [option]
 Deploys Gerrit plugin Maven archetypes to Maven Central
 
 Valid options:
-  --help                     show this message
+  --help                     show this message.
-  --dry-run                  don't execute commands, just print them
+  --dry-run                  don't execute commands, just print them.
+
 eof
 exit
 }
@@ -62,22 +63,39 @@ function build_and_deploy
     -Dfile=target/$module-$ver.jar
 }
 
-while [ $# -gt 0 ]; do
+function confirm
-  test "$1" == --dry-run && dryRun=true
+{
-  test "$1" == --help && help
+  read -n1 -p "Are you sure you want to deploy? [N/y]: " ready
-  shift
+  if [[ ! $ready == [Yy] ]]; then
-done
+    if [[ $ready == [Nn] || -z $ready ]]; then
+      echo; exit
+    else
+      echo; confirm
+    fi
+  fi
+}
 
-root=$(instroot)
+function run
-cd "$root"
+{
-ver=$(getver GERRIT_VERSION)
+  test ${dryRun:-'false'} == 'false'  && confirm
-[[ $ver == *-SNAPSHOT ]] \
+  root=$(instroot)
-  && url="https://oss.sonatype.org/content/repositories/snapshots" \
+  cd "$root"
-  || url="https://oss.sonatype.org/service/local/staging/deploy/maven2"
+  ver=$(getver GERRIT_VERSION)
+  [[ $ver == *-SNAPSHOT ]] \
+    && url="https://oss.sonatype.org/content/repositories/snapshots" \
+    || url="https://oss.sonatype.org/service/local/staging/deploy/maven2"
 
-for d in gerrit-plugin-archetype \
+  for d in gerrit-plugin-archetype \
-         gerrit-plugin-js-archetype \
+           gerrit-plugin-js-archetype \
-         gerrit-plugin-gwt-archetype ; do
+           gerrit-plugin-gwt-archetype ; do
-  (cd "$d"; build_and_deploy)
+    (cd "$d"; build_and_deploy)
-done
+  done
+}
 
+if [ "$1" == "--dry-run" ]; then
+  dryRun=true && run
+elif [ -z "$1" ]; then
+  run
+else
+  help
+fi