Merge "CreateNewChangeForAllNotInTarget: Fix working with change series"

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

@@ -442,9 +442,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
   @Test
   public void pushForMasterWithMessageTwiceWithDifferentMessages() throws Exception {
-    ProjectConfig config = projectCache.checkedGet(project).getConfig();
+    enableCreateNewChangeForAllNotInTarget();
-    config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
-    saveProjectConfig(project, config);
 
     PushOneCommit push =
         pushFactory.create(
@@ -781,6 +779,33 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertTwoChangesWithSameRevision(r);
   }
 
+  @Test
+  public void pushChangeBasedOnChangeOfOtherUserWithCreateNewChangeForAllNotInTarget()
+      throws Exception {
+    enableCreateNewChangeForAllNotInTarget();
+
+    // create a change as admin
+    PushOneCommit push =
+        pushFactory.create(
+            db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "a.txt", "content");
+    PushOneCommit.Result r = push.to("refs/for/master");
+    r.assertOkStatus();
+    RevCommit commitChange1 = r.getCommit();
+
+    // create a second change as user (depends on the change from admin)
+    TestRepository<?> userRepo = cloneProject(project, user);
+    GitUtil.fetch(userRepo, r.getPatchSet().getRefName() + ":change");
+    userRepo.reset("change");
+    push =
+        pushFactory.create(
+            db, user.getIdent(), userRepo, PushOneCommit.SUBJECT, "b.txt", "anotherContent");
+    r = push.to("refs/for/master");
+    r.assertOkStatus();
+
+    // assert that no new change was created for the commit of the predecessor change
+    assertThat(query(commitChange1.name())).hasSize(1);
+  }
+
   @Test
   public void pushSameCommitTwiceUsingMagicBranchBaseOption() throws Exception {
     grant(Permission.PUSH, project, "refs/heads/master");

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

@@ -1725,7 +1725,7 @@ public class ReceiveCommits {
         return;
       }
 
-      List<ChangeLookup> pending = new ArrayList<>();
+      LinkedHashMap<RevCommit, ChangeLookup> pending = new LinkedHashMap<>();
       Set<Change.Key> newChangeIds = new HashSet<>();
       int maxBatchChanges = receiveConfig.getEffectiveMaxBatchChangesLimit(user);
       int total = 0;
@@ -1759,7 +1759,8 @@ public class ReceiveCommits {
           mergedParents.remove(c);
         }
 
-        if (!existingRefs.isEmpty()) { // Commit is already tracked.
+        boolean commitAlreadyTracked = !existingRefs.isEmpty();
+        if (commitAlreadyTracked) {
           alreadyTracked++;
           // Corner cases where an existing commit might need a new group:
           // A) Existing commit has a null group; wasn't assigned during schema
@@ -1780,6 +1781,39 @@ public class ReceiveCommits {
           if (!(newChangeForAllNotInTarget || magicBranch.base != null)) {
             continue;
           }
+        }
+
+        List<String> idList = c.getFooterLines(CHANGE_ID);
+
+        String idStr = !idList.isEmpty() ? idList.get(idList.size() - 1).trim() : null;
+
+        if (idStr != null) {
+          pending.put(c, new ChangeLookup(c, new Change.Key(idStr)));
+        } else {
+          pending.put(c, new ChangeLookup(c));
+        }
+        int n = pending.size() + newChanges.size();
+        if (maxBatchChanges != 0 && n > maxBatchChanges) {
+          logDebug("{} changes exceeds limit of {}", n, maxBatchChanges);
+          reject(
+              magicBranch.cmd,
+              "the number of pushed changes in a batch exceeds the max limit " + maxBatchChanges);
+          newChanges = Collections.emptyList();
+          return;
+        }
+
+        if (commitAlreadyTracked) {
+          boolean changeExistsOnDestBranch = false;
+          for (ChangeData cd : pending.get(c).destChanges) {
+            if (cd.change().getDest().equals(magicBranch.dest)) {
+              changeExistsOnDestBranch = true;
+              break;
+            }
+          }
+          if (changeExistsOnDestBranch) {
+            continue;
+          }
+
           logDebug("Creating new change for {} even though it is already tracked", name);
         }
 
@@ -1800,23 +1834,10 @@ public class ReceiveCommits {
           // TODO(dborowitz): Should we early return here?
         }
 
-        List<String> idList = c.getFooterLines(CHANGE_ID);
         if (idList.isEmpty()) {
           newChanges.add(new CreateRequest(c, magicBranch.dest.get()));
           continue;
         }
-
-        String idStr = idList.get(idList.size() - 1).trim();
-        pending.add(new ChangeLookup(c, new Change.Key(idStr)));
-        int n = pending.size() + newChanges.size();
-        if (maxBatchChanges != 0 && n > maxBatchChanges) {
-          logDebug("{} changes exceeds limit of {}", n, maxBatchChanges);
-          reject(
-              magicBranch.cmd,
-              "the number of pushed changes in a batch exceeds the max limit " + maxBatchChanges);
-          newChanges = Collections.emptyList();
-          return;
-        }
       }
       logDebug(
           "Finished initial RevWalk with {} commits total: {} already"
@@ -1831,8 +1852,12 @@ public class ReceiveCommits {
         rejectImplicitMerges(mergedParents);
       }
 
-      for (Iterator<ChangeLookup> itr = pending.iterator(); itr.hasNext(); ) {
+      for (Iterator<ChangeLookup> itr = pending.values().iterator(); itr.hasNext(); ) {
         ChangeLookup p = itr.next();
+        if (p.changeKey == null) {
+          continue;
+        }
+
         if (newChangeIds.contains(p.changeKey)) {
           logDebug("Multiple commits with Change-Id {}", p.changeKey);
           reject(magicBranch.cmd, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES);
@@ -2071,6 +2096,12 @@ public class ReceiveCommits {
       changeKey = key;
       destChanges = queryProvider.get().byBranchKey(magicBranch.dest, key);
     }
+
+    ChangeLookup(RevCommit c) throws OrmException {
+      commit = c;
+      destChanges = queryProvider.get().byBranchCommit(magicBranch.dest, c.getName());
+      changeKey = null;
+    }
   }
 
   private class CreateRequest {