Merge "Merge branch 'stable-2.11'"

ReleaseNotes/ReleaseNotes-2.10.7.txt

@@ -0,0 +1,19 @@
+Release notes for Gerrit 2.10.7
+===============================
+
+There are no schema changes from link:ReleaseNotes-2.10.6.html[2.10.6].
+
+Download:
+link:https://gerrit-releases.storage.googleapis.com/gerrit-2.10.7.war[
+https://gerrit-releases.storage.googleapis.com/gerrit-2.10.7.war]
+
+Bug Fixes
+---------
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3361[Issue 3361]:
+Synchronize Myers diff and Histogram diff invocations to prevent pack file
+corruption.
++
+See also the link:https://bugs.eclipse.org/bugs/show_bug.cgi?id=467467[
+bug report on JGit].
+

ReleaseNotes/ReleaseNotes-2.11.4.txt

@@ -0,0 +1,111 @@
+Release notes for Gerrit 2.11.4
+===============================
+
+Gerrit 2.11.4 is now available:
+
+link:https://gerrit-releases.storage.googleapis.com/gerrit-2.11.4.war[
+https://gerrit-releases.storage.googleapis.com/gerrit-2.11.4.war]
+
+Gerrit 2.11.4 includes the bug fixes done with
+link:ReleaseNotes-2.10.7.html[Gerrit 2.10.7]. These bug fixes are *not* listed
+in these release notes.
+
+There are no schema changes from link:ReleaseNotes-2.11.3.html[2.11.3].
+
+
+Bug Fixes
+---------
+
+* Fix NullPointerException in `ls-project` command with `--has-acl-for` option.
++
+Using the `--has-acl-for` option for external groups (e.g. LDAP groups) was
+causing a NullPointerException.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3328[Issue 3328]:
+Allow to push a tag that points to a non-commit object.
++
+When pushing a tag that points to a non-commit object, like
+link:https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tag/?id=v2.6.11[
+`v2.6.11` on linux-stable] which points to a tree, or
+link:https://git.eclipse.org/c/jgit/jgit.git/tag/?id=spearce-gpg-pub[
+`spearce-gpg-pub` on jgit] which points to a blob, Gerrit rejected the push with
+the error message 'missing object(s)'.
++
+Note: This was previously fixed in Gerrit version 2.11.1, but was inadvertently
+reverted in 2.11.2 and 2.11.3.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=2817[Issue 2817]:
+Insert Change-Id into access right changes.
++
+When modifications of access rights were saved for review, the change
+did not have a Change-Id in the commit message.
+
+* Fix duplicated log lines after reloading a plugin.
++
+If a plugin was reloaded, logs emitted from the plugin were duplicated.
+
+* Remove `--recheck-mergeable` option from `reindex` command documentation.
++
+The `--recheck-mergeable` option was removed in Gerrit version 2.11.
+
+* Use the correct validation policy for commits created by Gerrit.
++
+Commits created by Gerrit were being validated in the same way as commits
+received from users.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3557[Issue 3557]:
+Disallow invalid reference patterns in project configuration.
++
+When editing a project configuration by using the UI or by submitting a change
+to `refs/meta/config`, it was possible to add a permission to an invalid
+reference pattern. This caused the project to be unavailable and the `ls-projects`
+command to fail whenever this project was encountered.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3574[Issue 3574]:
+Fix review labels with `AnyWithBlock` function.
++
+The review labels with `AnyWithBlock` with 0 and +1 values blocked submit when
+reviewers were added.
+
+* Fix ref in tag list for signed/annotated tags.
++
+The tag name from the header was used, rather than the ref name. In some cases
+this resulted in the wrong tag ref being listed.
+
+* singleusergroup plugin: enable to add a user within a project's ACL using `user/username`.
++
+A user could not be added to a project's ACL unless the user already had READ
+permission in the project's ACL.
+
+* Prevent user from bypassing ref-update hook through gerrit-created commits.
++
+If the user used the cherry-pick ability in the UI or via the REST API, they
+could put a commit on a branch that bypassed the requirements of the ref-update
+hook (such as that certain branches require QA-tickets to be referenced in the
+commit message).
+
+* Allow InternalUsers to see drafts.
++
+According to the documentation, InternalUsers should have full read access.
+This was not true, since InternalUsers could not see drafts.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=2683[Issue 2683]:
+Fix non-ASCII password authentication failure under tomcat (LDAP).
++
+The authentication with LDAP failed when the password contained non-ASCII
+characters such as ä, ö, Ä, and Ö.
+
+* Do not double decode the login URL token.
++
+The login URL token used to redirect from the login servlet to the target page
+is already decoded and should not be decoded again.
+
+* Include approvals from magic branch in change message.
++
+When using the `%l` option to apply a review label on uploaded changes or
+patch sets, the applied label was not mentioned in the change message.
+
+* Fire the `comment-added` hook for approvals from the magic branch.
++
+When using the `%l` option to apply a review label on uploaded changes or
+patch sets, the `comment-added` hook was not being fired.

ReleaseNotes/index.txt

@@ -4,6 +4,7 @@ Gerrit Code Review - Release Notes
 [[2_11]]
 Version 2.11.x
 --------------
+* link:ReleaseNotes-2.11.4.html[2.11.4]
 * link:ReleaseNotes-2.11.3.html[2.11.3]
 * link:ReleaseNotes-2.11.2.html[2.11.2]
 * link:ReleaseNotes-2.11.1.html[2.11.1]
@@ -12,6 +13,7 @@ Version 2.11.x
 [[2_10]]
 Version 2.10.x
 --------------
+* link:ReleaseNotes-2.10.7.html[2.10.7]
 * link:ReleaseNotes-2.10.6.html[2.10.6]
 * link:ReleaseNotes-2.10.5.html[2.10.5]
 * link:ReleaseNotes-2.10.4.html[2.10.4]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -16,8 +16,11 @@ package com.google.gerrit.acceptance.git;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -32,10 +35,16 @@ import com.google.gerrit.testutil.ConfigSuite;
 import com.google.inject.Inject;
 
 import org.eclipse.jgit.lib.Config;
+import org.joda.time.DateTime;
+import org.joda.time.DateTimeUtils;
+import org.joda.time.DateTimeUtils.MillisProvider;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicLong;
 
 public abstract class AbstractPushForReview extends AbstractDaemonTest {
   @ConfigSuite.Config
@@ -53,6 +62,24 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
   private String sshUrl;
 
+  @BeforeClass
+  public static void setTimeForTesting() {
+    final long clockStepMs = MILLISECONDS.convert(1, SECONDS);
+    final AtomicLong clockMs = new AtomicLong(
+        new DateTime(2009, 9, 30, 17, 0, 0).getMillis());
+    DateTimeUtils.setCurrentMillisProvider(new MillisProvider() {
+      @Override
+      public long getMillis() {
+        return clockMs.getAndAdd(clockStepMs);
+      }
+    });
+  }
+
+  @AfterClass
+  public static void restoreTime() {
+    DateTimeUtils.setCurrentMillisSystem();
+  }
+
   @Before
   public void setUp() throws Exception {
     sshUrl = sshSession.getUrl();
@@ -177,6 +204,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(cr.all).hasSize(1);
     assertThat(cr.all.get(0).name).isEqualTo("Administrator");
     assertThat(cr.all.get(0).value).isEqualTo(1);
+    assertThat(Iterables.getLast(ci.messages).message).isEqualTo(
+        "Uploaded patch set 1: Code-Review+1.");
 
     PushOneCommit push =
         pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
@@ -185,9 +214,20 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
     ci = get(r.getChangeId());
     cr = ci.labels.get("Code-Review");
+    assertThat(Iterables.getLast(ci.messages).message).isEqualTo(
+        "Uploaded patch set 2: Code-Review+2.");
+
     assertThat(cr.all).hasSize(1);
     assertThat(cr.all.get(0).name).isEqualTo("Administrator");
     assertThat(cr.all.get(0).value).isEqualTo(2);
+
+    push =
+        pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
+            "c.txt", "moreContent", r.getChangeId());
+    r = push.to("refs/for/master/%l=Code-Review+2");
+    ci = get(r.getChangeId());
+    assertThat(Iterables.getLast(ci.messages).message).isEqualTo(
+        "Uploaded patch set 3.");
   }
 
   @Test

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK

@@ -22,7 +22,10 @@ acceptance_tests(
 java_library(
   name = 'push_for_review',
   srcs = ['AbstractPushForReview.java'],
-  deps = ['//gerrit-acceptance-tests:lib'],
+  deps = [
+    '//gerrit-acceptance-tests:lib',
+    '//lib/joda:joda-time',
+  ],
 )
 
 java_library(

gerrit-httpd/src/main/java/com/google/gerrit/httpd/LoginUrlToken.java

@@ -17,7 +17,6 @@ package com.google.gerrit.httpd;
 import com.google.common.base.CharMatcher;
 import com.google.common.base.Strings;
 import com.google.gerrit.common.PageLinks;
-import com.google.gerrit.extensions.restapi.Url;
 
 import javax.servlet.http.HttpServletRequest;
 
@@ -25,11 +24,11 @@ public class LoginUrlToken {
   private static final String DEFAULT_TOKEN = '#' + PageLinks.MINE;
 
   public static String getToken(final HttpServletRequest req){
-    String encodedToken = req.getPathInfo();
+    String token = req.getPathInfo();
-    if (Strings.isNullOrEmpty(encodedToken)) {
+    if (Strings.isNullOrEmpty(token)) {
       return DEFAULT_TOKEN;
     } else {
-      return CharMatcher.is('/').trimLeadingFrom(Url.decode(encodedToken));
+      return CharMatcher.is('/').trimLeadingFrom(token);
     }
   }
 

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

@@ -341,6 +341,12 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
             accountCache.get(change.getOwner()).getAccount(),
             hashtags, null, hashtags, db);
       }
+
+      if (approvals != null && !approvals.isEmpty()) {
+        hooks.doCommentAddedHook(change,
+            ((IdentifiedUser) refControl.getCurrentUser()).getAccount(),
+            patchSet, null, approvals, db);
+      }
     }
   }
 

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

@@ -71,6 +71,7 @@ import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -1768,6 +1769,8 @@ public class ReceiveCommits {
       }
       recipients.add(getRecipientsFromFooters(accountResolver, ps, footerLines));
       recipients.remove(me);
+      StringBuilder msgs = renderMessageWithApprovals(ps.getPatchSetId(),
+          approvals, Collections.<String, PatchSetApproval>emptyMap());
       try (ObjectInserter oi = repo.newObjectInserter();
           BatchUpdate bu = batchUpdateFactory.create(threadLocalDb,
             change.getProject(), currentUser, change.getCreatedOn())) {
@@ -1776,7 +1779,7 @@ public class ReceiveCommits {
             .setReviewers(recipients.getReviewers())
             .setExtraCC(recipients.getCcOnly())
             .setApprovals(approvals)
-            .setMessage("Uploaded patch set " + ps.getPatchSetId() + ".")
+            .setMessage(msgs.toString() + ".")
             .setRequestScopePropagator(requestScopePropagator)
             .setSendMail(true)
             .setUpdateRef(false));
@@ -1888,6 +1891,27 @@ public class ReceiveCommits {
     }
   }
 
+  private StringBuilder renderMessageWithApprovals(int patchSetId,
+      Map<String, Short> n, Map<String, PatchSetApproval> c) {
+    StringBuilder msgs = new StringBuilder("Uploaded patch set " + patchSetId);
+    if (!n.isEmpty()) {
+      boolean first = true;
+      for (Map.Entry<String, Short> e : n.entrySet()) {
+        if (c.containsKey(e.getKey())
+            && c.get(e.getKey()).getValue() == e.getValue()) {
+          continue;
+        }
+        if (first) {
+          msgs.append(":");
+          first = false;
+        }
+        msgs.append(" ")
+            .append(LabelVote.create(e.getKey(), e.getValue()).format());
+      }
+    }
+    return msgs;
+  }
+
   private class ReplaceRequest {
     final Change.Id ontoChange;
     final RevCommit newCommit;
@@ -2125,29 +2149,52 @@ public class ReceiveCommits {
       return Futures.makeChecked(future, INSERT_EXCEPTION);
     }
 
-    private ChangeMessage newChangeMessage(ReviewDb db, ChangeKind changeKind)
+    private ChangeMessage newChangeMessage(ReviewDb db, ChangeKind changeKind,
+        Map<String, Short> approvals)
         throws OrmException {
       msg =
           new ChangeMessage(new ChangeMessage.Key(change.getId(), ChangeUtil
               .messageUUID(db)), currentUser.getAccountId(), newPatchSet.getCreatedOn(),
               newPatchSet.getId());
-      String message = "Uploaded patch set " + newPatchSet.getPatchSetId();
+      StringBuilder msgs = renderMessageWithApprovals(
+          newPatchSet.getPatchSetId(), approvals, scanLabels(db, approvals));
       switch (changeKind) {
         case TRIVIAL_REBASE:
         case NO_CHANGE:
-          message += ": Patch Set " + priorPatchSet.get() + " was rebased";
+          msgs.append(": Patch Set " + priorPatchSet.get() + " was rebased");
           break;
         case NO_CODE_CHANGE:
-          message += ": Commit message was updated";
+          msgs.append(": Commit message was updated");
           break;
         case REWORK:
         default:
           break;
       }
-      msg.setMessage(message + ".");
+      msg.setMessage(msgs.toString() + ".");
       return msg;
     }
 
+    private Map<String, PatchSetApproval> scanLabels(ReviewDb db,
+        Map<String, Short> approvals)
+        throws OrmException {
+      Map<String, PatchSetApproval> current = new HashMap<>();
+      // We optimize here and only retrieve current when approvals provided
+      if (!approvals.isEmpty()) {
+        for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
+            db, changeCtl, priorPatchSet, currentUser.getAccountId())) {
+          if (a.isSubmit()) {
+            continue;
+          }
+
+          LabelType lt = labelTypes.byLabel(a.getLabelId());
+          if (lt != null) {
+            current.put(lt.getName(), a);
+          }
+        }
+      }
+      return current;
+    }
+
     PatchSet.Id upsertEdit() {
       if (cmd.getResult() == NOT_ATTEMPTED) {
         cmd.execute(rp);
@@ -2212,7 +2259,8 @@ public class ReceiveCommits {
         changeKind = changeKindCache.getChangeKind(
             projectControl.getProjectState(), repo, priorCommit, newCommit);
 
-        cmUtil.addChangeMessage(db, update, newChangeMessage(db, changeKind));
+        cmUtil.addChangeMessage(db, update, newChangeMessage(db, changeKind,
+            approvals));
 
         if (mergedIntoRef == null) {
           // Change should be new, so it can go through review again.
@@ -2311,6 +2359,11 @@ public class ReceiveCommits {
             change, currentUser.getAccount(), newPatchSet, db, newCommit.getName());
       }
 
+      if (!approvals.isEmpty()) {
+        hooks.doCommentAddedHook(change, currentUser.getAccount(), newPatchSet,
+            null, approvals, db);
+      }
+
       if (magicBranch != null && magicBranch.submit) {
         submit(changeCtl, newPatchSet);
       }

gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -14,12 +14,17 @@
 
 package com.google.gerrit.server.git.validators;
 
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
+import static org.eclipse.jgit.lib.Constants.R_HEADS;
+
 import com.google.common.base.CharMatcher;
+import com.google.gerrit.common.ChangeHookRunner.HookResult;
+import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.PageLinks;
 import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -39,6 +44,7 @@ import com.jcraft.jsch.HostKey;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.notes.NoteMap;
@@ -82,6 +88,7 @@ public class CommitValidators {
   private final String installCommitMsgHookCommand;
   private final SshInfo sshInfo;
   private final Repository repo;
+  private final ChangeHooks hooks;
   private final DynamicSet<CommitValidationListener> commitValidationListeners;
 
   @Inject
@@ -89,6 +96,7 @@ public class CommitValidators {
       @CanonicalWebUrl @Nullable final String canonicalWebUrl,
       @GerritServerConfig final Config config,
       final DynamicSet<CommitValidationListener> commitValidationListeners,
+      final ChangeHooks hooks,
       @Assisted final SshInfo sshInfo,
       @Assisted final Repository repo, @Assisted final RefControl refControl) {
     this.gerritIdent = gerritIdent;
@@ -98,6 +106,7 @@ public class CommitValidators {
         config.getString("gerrit", null, "installCommitMsgHookCommand");
     this.sshInfo = sshInfo;
     this.repo = repo;
+    this.hooks = hooks;
     this.commitValidationListeners = commitValidationListeners;
   }
 
@@ -155,6 +164,7 @@ public class CommitValidators {
     }
     validators.add(new ConfigValidator(refControl, repo));
     validators.add(new PluginCommitValidationListener(commitValidationListeners));
+    validators.add(new ChangeHookValidator(refControl, hooks));
 
     List<CommitValidationMessage> messages = new LinkedList<>();
 
@@ -308,7 +318,7 @@ public class CommitValidators {
         CommitReceivedEvent receiveEvent) throws CommitValidationException {
       IdentifiedUser currentUser = (IdentifiedUser) refControl.getCurrentUser();
 
-      if (RefNames.REFS_CONFIG.equals(refControl.getRefName())) {
+      if (REFS_CONFIG.equals(refControl.getRefName())) {
         List<CommitValidationMessage> messages = new LinkedList<>();
 
         try {
@@ -535,6 +545,48 @@ public class CommitValidators {
     }
   }
 
+  /** Reject commits that don't pass user-supplied ref-update hook. */
+  public static class ChangeHookValidator implements
+      CommitValidationListener {
+    private final RefControl refControl;
+    private final ChangeHooks hooks;
+
+    public ChangeHookValidator(RefControl refControl, ChangeHooks hooks) {
+      this.refControl = refControl;
+      this.hooks = hooks;
+    }
+
+    @Override
+    public List<CommitValidationMessage> onCommitReceived(
+        CommitReceivedEvent receiveEvent) throws CommitValidationException {
+
+      if (refControl.getCurrentUser().isIdentifiedUser()) {
+        IdentifiedUser user = (IdentifiedUser) refControl.getCurrentUser();
+
+        String refname = receiveEvent.refName;
+        ObjectId old = receiveEvent.commit.getParent(0);
+
+        if (receiveEvent.command.getRefName().startsWith(REFS_CHANGES)) {
+          /*
+           * If the ref-update hook tries to distinguish behavior between pushes to
+           * refs/heads/... and refs/for/..., make sure we send it the correct refname.
+           * Also, if this is targetting refs/for/, make sure we behave the same as
+           * what a push to refs/for/ would behave; in particular, setting oldrev to
+           * 0000000000000000000000000000000000000000.
+           */
+          refname = refname.replace(R_HEADS, "refs/for/refs/heads/");
+          old = ObjectId.zeroId();
+        }
+        HookResult result = hooks.doRefUpdateHook(receiveEvent.project, refname,
+            user.getAccount(), old, receiveEvent.commit);
+        if (result != null && result.getExitValue() != 0) {
+            throw new CommitValidationException(result.toString().trim());
+        }
+      }
+      return Collections.emptyList();
+    }
+  }
+
   private static CommitValidationMessage getInvalidEmailError(RevCommit c, String type,
       PersonIdent who, IdentifiedUser currentUser, String canonicalWebUrl) {
     StringBuilder sb = new StringBuilder();