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
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 3f36a2d..34679dd 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.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 @@
}
@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 @@
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 ad1576b..7094858 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 @@
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 293c93b..e618fcb 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 @@
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 @@
} else {
markHeadsAsUninteresting(
rp.getRevWalk(),
- existing,
magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
}
@@ -1553,10 +1552,14 @@
// 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 @@
}
}
- 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 @@
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 @@
}
}
- 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 @@
}
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 @@
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;
