opendev/gerrit
Code Issues Proposed changes

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

Browse Source 
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
This commit is contained in:
Sven Selberg
2016-05-12 09:55:30 +02:00
committed by David Pursehouse
parent f79ac18f20
commit 9775399ea2
3 changed files with 68 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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;
@@ -378,6 +379,34 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
"not Signed-off-by author/committer/uploader in commit message footer");
}
@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 {
@@ -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);

5
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -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.
}

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

@@ -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;