opendev/gerrit
Code Issues Proposed changes

ReceiveCommits: Fix merging multiple changes by push

Browse Source 
ReplaceRequest#validate is unsafe to call in the middle of a RevWalk,
because it calls isMergedInto, which resets the walk. This had the
unfortunate effect of marking only the change corresponding to the tip
commit as merged by the push, rather than all changes.

This was always a potentially unsafe use of isMergedInto, but it only
actually caused a bug due to the refactoring in Iadfcc6d9. Add some
Javadoc to validate to maybe head this off in the future.

Change-Id: I0d2423623c7fce6a4fc5dd91aaffbc250b6c6f5b
This commit is contained in:
Dave Borowitz 2016-06-22 16:28:18 -04:00
parent 598591536b
commit 9ea8b684b7
2 changed files with 94 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
View File

@ -15,6 +15,8 @@
package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@ -229,6 +231,55 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
assertThat(cd.patchSet(psId2).getRevision().get()).isEqualTo(c2.name());
}
@Test
public void mergeMultipleOnPushToBranchWithNewPatchset() throws Exception {
grant(Permission.PUSH, project, "refs/heads/master");
// Create 2 changes.
ObjectId initialHead = getRemoteHead();
PushOneCommit.Result r1 = createChange("Change 1", "a", "a");
r1.assertOkStatus();
PushOneCommit.Result r2 = createChange("Change 2", "b", "b");
r2.assertOkStatus();
RevCommit c1_1 = r1.getCommit();
RevCommit c2_1 = r2.getCommit();
PatchSet.Id psId1_1 = r1.getPatchSetId();
PatchSet.Id psId2_1 = r2.getPatchSetId();
assertThat(c1_1.getParent(0)).isEqualTo(initialHead);
assertThat(c2_1.getParent(0)).isEqualTo(c1_1);
// Amend both changes.
testRepo.reset(initialHead);
RevCommit c1_2 = testRepo.branch("HEAD").commit()
.message(c1_1.getShortMessage() + "v2")
.insertChangeId(r1.getChangeId().substring(1))
.create();
RevCommit c2_2 = testRepo.cherryPick(c2_1);
// Push directly to branch.
assertPushOk(
pushHead(testRepo, "refs/heads/master", false), "refs/heads/master");
ChangeData cd2 = r2.getChange();
assertThat(cd2.change().getStatus()).isEqualTo(Change.Status.MERGED);
PatchSet.Id psId2_2 = cd2.change().currentPatchSetId();
assertThat(psId2_2.get()).isEqualTo(2);
assertThat(cd2.patchSet(psId2_1).getRevision().get())
.isEqualTo(c2_1.name());
assertThat(cd2.patchSet(psId2_2).getRevision().get())
.isEqualTo(c2_2.name());
ChangeData cd1 = r1.getChange();
assertThat(cd1.change().getStatus()).isEqualTo(Change.Status.MERGED);
PatchSet.Id psId1_2 = cd1.change().currentPatchSetId();
assertThat(psId1_2.get()).isEqualTo(2);
assertThat(cd1.patchSet(psId1_1).getRevision().get())
.isEqualTo(c1_1.name());
assertThat(cd1.patchSet(psId1_2).getRevision().get())
.isEqualTo(c1_2.name());
}
private PatchSetApproval getSubmitter(PatchSet.Id patchSetId)
throws Exception {
ChangeNotes notes =

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

@ -1944,6 +1944,23 @@ public class ReceiveCommits {
}
}
/**
* Validate the new patch set commit for this change.
* <p>
* <strong>Side effects:</strong>
* <ul>
* <li>May add error or warning messages to the progress monitor</li>
* <li>Will reject {@code cmd} prior to returning false</li>
* <li>May reset {@code rp.getRevWalk()}; do not call in the middle of a
* walk.</li>
* </ul>
*
* @param autoClose whether the caller intends to auto-close the change
* after adding a new patch set.
* @return whether the new commit is valid
* @throws IOException
* @throws OrmException
*/
boolean validate(boolean autoClose) throws IOException, OrmException {
if (!autoClose && inputCommand.getResult() != NOT_ATTEMPTED) {
return false;
@ -2359,6 +2376,7 @@ public class ReceiveCommits {
SetMultimap<ObjectId, Ref> byCommit = changeRefsById();
Map<Change.Key, ChangeNotes> byKey = null;
List<ReplaceRequest> replaceAndClose = new ArrayList<>();
COMMIT: for (RevCommit c; (c = rw.next()) != null;) {
rw.parseBody(c);
@ -2379,28 +2397,36 @@ public class ReceiveCommits {
ChangeNotes onto = byKey.get(new Change.Key(changeId.trim()));
if (onto != null) {
Change.Id id = onto.getChangeId();
final ReplaceRequest req = new ReplaceRequest(id, c, cmd, false);
// Hold onto this until we're done with the walk, as the call to
// req.validate below calls isMergedInto which resets the walk.
ReplaceRequest req =
new ReplaceRequest(onto.getChangeId(), c, cmd, false);
req.notes = onto;
if (req.validate(true)) {
req.addOps(bu, null);
bu.addOp(
id,
mergedByPushOpFactory.create(
requestScopePropagator, req.psId, refName)
.setPatchSetProvider(new Provider<PatchSet>() {
@Override
public PatchSet get() {
return req.replaceOp.getPatchSet();
}
}));
bu.addOp(id, new ChangeProgressOp(closeProgress));
}
break;
replaceAndClose.add(req);
continue COMMIT;
}
}
}
for (final ReplaceRequest req : replaceAndClose) {
Change.Id id = req.notes.getChangeId();
if (!req.validate(true)) {
continue;
}
req.addOps(bu, null);
bu.addOp(
id,
mergedByPushOpFactory.create(
requestScopePropagator, req.psId, refName)
.setPatchSetProvider(new Provider<PatchSet>() {
@Override
public PatchSet get() {
return req.replaceOp.getPatchSet();
}
}));
bu.addOp(id, new ChangeProgressOp(closeProgress));
}
bu.execute();
} catch (RestApiException e) {
log.error("Can't insert patchset", e);