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 a4b2209a1a..72a391c4f4 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 @@ -442,9 +442,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { @Test public void pushForMasterWithMessageTwiceWithDifferentMessages() throws Exception { - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE); - saveProjectConfig(project, config); + enableCreateNewChangeForAllNotInTarget(); 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"); 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 e4bc86f103..aed59e7008 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 @@ -1725,7 +1725,7 @@ public class ReceiveCommits { return; } - List pending = new ArrayList<>(); + LinkedHashMap pending = new LinkedHashMap<>(); Set 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 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 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 itr = pending.iterator(); itr.hasNext(); ) { + for (Iterator 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 {