Fix %base option and "Create a new change for..." setting

Check if the pre-existing ref is a PatchSet of a change with a
different target branch than current push if
newChangeForAllNotInTarget == true or %base option is set and
create a new change for the new target.

Don't add the commits about to get merged to alreadyAccepted.

Else if a commit C is merged into branch A and you upload a new change
with the same commit to branch B, with the %base option or if
"Create a new change for every commit not in the target branch:" is
configured for the project, MergeOp will merge the commit into branch
B, but MergeUtil will not mark the status as "CLEAN_MERGE". The result
is that you get a "Change is new" error message, and in order to merge
the change you will need to upload a new PatchSet and merge that on top of
commit C.

Bug: Issue 3426
Change-Id: I01f4b9b04f1797d403671b27b8c2e61d1fd3bcc6

3 files changed, 68 insertions, 26 deletions

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 3f36a2df7e..34679ddc25 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
@@ -33,6 +33,7 @@ import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.testutil.TestTimeUtil;
+import com.google.gerrit.server.git.ProjectConfig;
 
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.PushResult;
@@ -379,6 +380,34 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
   }
 
   @Test
+  public void testCreateNewChangeForAllNotInTarget() throws Exception {
+    ProjectConfig config = projectCache.checkedGet(project).getConfig();
+    config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
+    saveProjectConfig(project, config);
+
+    PushOneCommit push =
+        pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
+            "a.txt", "content");
+    PushOneCommit.Result r = push.to("refs/for/master");
+    r.assertOkStatus();
+
+    push =
+        pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
+            "b.txt", "anotherContent");
+    r = push.to("refs/for/master");
+    r.assertOkStatus();
+
+    gApi.projects()
+        .name(project.get())
+        .branch("otherBranch")
+        .create(new BranchInput());
+
+    PushOneCommit.Result r2 = push.to("refs/for/otherBranch");
+    r2.assertOkStatus();
+    assertTwoChangesWithSameRevision(r);
+  }
+
+  @Test
   public void testPushSameCommitTwiceUsingMagicBranchBaseOption()
       throws Exception {
     grant(Permission.PUSH, project, "refs/heads/master");
@@ -401,7 +430,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
         testRepo, "refs/for/foo%base=" + rBase.getCommitId().name(), false, false);
     assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
 
-    List<ChangeInfo> changes = query(r.getCommitId().name());
+    assertTwoChangesWithSameRevision(r);
+  }
+
+  private void assertTwoChangesWithSameRevision(PushOneCommit.Result result)
+      throws Exception {
+    List<ChangeInfo> changes = query(result.getCommitId().name());
     assertThat(changes).hasSize(2);
     ChangeInfo c1 = get(changes.get(0).id);
     ChangeInfo c2 = get(changes.get(1).id);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index ad1576b173..709485839c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -572,7 +572,10 @@ public class MergeOp {
     try {
       for (Ref r : repo.getRefDatabase().getRefs(Constants.R_HEADS).values()) {
         try {
-          alreadyAccepted.add(rw.parseCommit(r.getObjectId()));
+          CodeReviewCommit aac = rw.parseCommit(r.getObjectId());
+          if (!commits.values().contains(aac)) {
+            alreadyAccepted.add(aac);
+          }
         } catch (IncorrectObjectTypeException iote) {
           // Not a commit? Skip over it.
         }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 293c93b203..e618fcb38e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1505,7 +1505,7 @@ public class ReceiveCommits {
   private void selectNewAndReplacedChangesFromMagicBranch() {
     newChanges = Lists.newArrayList();
 
-    SetMultimap<ObjectId, Ref> existing = HashMultimap.create();
+    SetMultimap<ObjectId, Ref> existing = changeRefsById();
     GroupCollector groupCollector = new GroupCollector(changeRefsById(), db);
 
     rp.getRevWalk().reset();
@@ -1526,7 +1526,6 @@ public class ReceiveCommits {
       } else {
         markHeadsAsUninteresting(
             rp.getRevWalk(),
-            existing,
             magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
       }
 
@@ -1553,10 +1552,14 @@ public class ReceiveCommits {
           //      B will be in existing so we aren't replacing the patch set. It
           //      used to have its own group, but now needs to to be changed to
           //      A's group.
+          // C) Commit is a PatchSet of a pre-existing change uploaded with a
+          //    different target branch.
           for (Ref ref : existingRefs) {
             updateGroups.add(new UpdateGroupsRequest(ref, c));
           }
-          continue;
+          if (!(newChangeForAllNotInTarget || magicBranch.base != null)) {
+            continue;
+          }
         }
 
         if (!validCommit(magicBranch.ctl, magicBranch.cmd, c)) {
@@ -1599,7 +1602,8 @@ public class ReceiveCommits {
         }
       }
 
-      for (ChangeLookup p : pending) {
+      for (Iterator<ChangeLookup> itr = pending.iterator(); itr.hasNext();) {
+        ChangeLookup p = itr.next();
         if (newChangeIds.contains(p.changeKey)) {
           reject(magicBranch.cmd, "squash commits first");
           newChanges = Collections.emptyList();
@@ -1621,6 +1625,21 @@ public class ReceiveCommits {
         if (changes.size() == 1) {
           // Schedule as a replacement to this one matching change.
           //
+
+          RevId currentPs = changes.get(0).currentPatchSet().getRevision();
+          // If Commit is already current PatchSet of target Change.
+          if (p.commit.name().equals(currentPs.get())) {
+            if (pending.size() == 1) {
+              // There are no commits left to check, all commits in pending were already
+              // current PatchSet of the corresponding target changes.
+              reject(magicBranch.cmd, "commit(s) already exists (as current patchset)");
+            } else {
+              // Commit is already current PatchSet.
+              // Remove from pending and try next commit.
+              itr.remove();
+              continue;
+            }
+          }
           if (requestReplace(
               magicBranch.cmd, false, changes.get(0).change(), p.commit)) {
             continue;
@@ -1684,23 +1703,15 @@ public class ReceiveCommits {
     }
   }
 
-  private void markHeadsAsUninteresting(
-      final RevWalk walk,
-      SetMultimap<ObjectId, Ref> existing,
-      @Nullable String forRef) {
+  private void markHeadsAsUninteresting(RevWalk rw, @Nullable String forRef) {
     for (Ref ref : allRefs.values()) {
-      if (ref.getObjectId() == null) {
-        continue;
-      } else if (ref.getName().startsWith(REFS_CHANGES)) {
-        existing.put(ref.getObjectId(), ref);
-      } else if (ref.getName().startsWith(R_HEADS)
-          || (forRef != null && forRef.equals(ref.getName()))) {
+      if ((ref.getName().startsWith(R_HEADS) || ref.getName().equals(forRef))
+          && ref.getObjectId() != null) {
         try {
-          walk.markUninteresting(walk.parseCommit(ref.getObjectId()));
+          rw.markUninteresting(rw.parseCommit(ref.getObjectId()));
         } catch (IOException e) {
           log.warn(String.format("Invalid ref %s in %s",
               ref.getName(), project.getName()), e);
-          continue;
         }
       }
     }
@@ -1994,12 +2005,6 @@ public class ReceiveCommits {
       }
 
       RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
-      if (newCommit == priorCommit) {
-        // Ignore requests to make the change its current state.
-        skip = true;
-        reject(inputCommand, "commit already exists (as current patchset)");
-        return false;
-      }
 
       changeCtl = projectControl.controlFor(change);
       if (!changeCtl.canAddPatchSet()) {
@@ -2578,9 +2583,9 @@ public class ReceiveCommits {
       if (!(parsedObject instanceof RevCommit)) {
         return;
       }
-      SetMultimap<ObjectId, Ref> existing = HashMultimap.create();
+      SetMultimap<ObjectId, Ref> existing = changeRefsById();
       walk.markStart((RevCommit)parsedObject);
-      markHeadsAsUninteresting(walk, existing, cmd.getRefName());
+      markHeadsAsUninteresting(walk, cmd.getRefName());
       for (RevCommit c; (c = walk.next()) != null;) {
         if (existing.keySet().contains(c)) {
           continue;