opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.12'

Browse Source 
* stable-2.12:
  AbstractPushForReview: Add test for message output
  ReceiveCommits: Prevent NPE setting updated change list for edit commit
  Fix %base option and "Create a new change for..." setting
  Organize imports

The fix done on stable-2.12 in I82983219a (ReceiveCommits: Prevent NPE
setting updated change list for edit commit) no longer works on master
due to modifications done in Id535eb513 (Don't share RevCommits between
threads) and is reworked in this commit.

Change-Id: Ib78c31ffa234fffd131d30789f07b5ffe0d92956
This commit is contained in:
David Pursehouse
2016-07-06 15:17:47 +09:00
parent bf71734416 e4f1396aba
commit 212414488a
3 changed files with 86 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -280,6 +280,10 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertOkStatus();
edit = getEdit(r.getChangeId());
assertThat(edit).isNotNull();
r.assertMessage("Updated Changes:\n "
+ canonicalWebUrl.get()
+ r.getChange().getId()
+ " " + edit.commit.subject + " [EDIT]\n");
}
@Test
@@ -538,6 +542,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 {
@@ -561,7 +593,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false);
assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
List<ChangeInfo> changes = query(r.getCommit().name());
assertTwoChangesWithSameRevision(r);
}
private void assertTwoChangesWithSameRevision(PushOneCommit.Result result)
throws Exception {
List<ChangeInfo> changes = query(result.getCommit().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

@@ -581,7 +581,10 @@ public class MergeOp implements AutoCloseable {
for (Ref r : or.repo.getRefDatabase().getRefs(Constants.R_HEADS)
.values()) {
try {
alreadyAccepted.add(or.rw.parseCommit(r.getObjectId()));
CodeReviewCommit aac = or.rw.parseCommit(r.getObjectId());
if (!commits.commits.values().contains(aac)) {
alreadyAccepted.add(aac);
}
} catch (IncorrectObjectTypeException iote) {
// Not a commit? Skip over it.
}

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

@@ -76,6 +76,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
@@ -695,8 +696,21 @@ public class ReceiveCommits {
addMessage("Updated Changes:");
boolean edit = magicBranch != null && magicBranch.edit;
for (ReplaceRequest u : updated) {
String subject;
if (edit) {
try {
subject =
rp.getRevWalk().parseCommit(u.newCommitId).getShortMessage();
} catch (IOException e) {
// Log and fall back to original change subject
log.warn("failed to get subject for edit patch set", e);
subject = u.notes.getChange().getSubject();
}
} else {
subject = u.info.getMessage();
}
addMessage(formatChangeUrl(canonicalWebUrl, u.notes.getChange(),
u.info.getSubject(), edit));
subject, edit));
}
addMessage("");
}
@@ -1516,7 +1530,7 @@ public class ReceiveCommits {
private void selectNewAndReplacedChangesFromMagicBranch() {
newChanges = new ArrayList<>();
SetMultimap<ObjectId, Ref> existing = HashMultimap.create();
SetMultimap<ObjectId, Ref> existing = changeRefsById();
GroupCollector groupCollector = GroupCollector.create(changeRefsById(), db, psUtil,
notesFactory, project.getNameKey());
@@ -1538,7 +1552,6 @@ public class ReceiveCommits {
} else {
markHeadsAsUninteresting(
rp.getRevWalk(),
existing,
magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
}
@@ -1565,10 +1578,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(
@@ -1610,7 +1627,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, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES);
newChanges = Collections.emptyList();
@@ -1632,6 +1650,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;
@@ -1694,23 +1727,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;
}
}
}
@@ -1984,12 +2009,6 @@ public class ReceiveCommits {
RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
if (newCommit.equals(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(notes);
if (!changeCtl.canAddPatchSet(db)) {
@@ -2298,9 +2317,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;