opendev/gerrit
Code Issues Proposed changes

Add option to ReceiveCommits to create new merged changes

Browse Source 
Sometimes surprising commits unintentionally end up in history, for
example by direct push. It may be useful in such cases to open up a
new post-commit code review for those commits. Allow users to create
a new change for a *single* merged commit by appending %merged to a
push:

  git push origin merged-commit:refs/for/master%merged

Only a single commit is supported initially, to avoid a potentially
expensive walk of history to discover commits that don't have
associated changes, of which there may be very many.

Post-commit code review was historically not supported by Gerrit at
all, but this is a step in that direction.

Change-Id: I4de55806cb8614b8c4d86b5de9bebc2f6923b927
This commit is contained in:
Dave Borowitz 2016-09-09 10:18:26 -04:00
parent 3a4e10f3da
commit a01219a5cc
4 changed files with 336 additions and 69 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
Documentation/user-upload.txt
View File

@ -456,6 +456,23 @@ argument:
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/master%base=commit-id1,base=commit-id2
----
[[merged]]
=== Creating Changes for Merged Commits
Normally, changes are only created for commits that have not yet
been merged into the branch. In some cases, you may want to review a
change that has already been merged. A new change for a merged commit
can be created by using the '%merged' argument:
----
git push ssh://john.doe@git.example.com:29418/kernel/common my-merged-commit:refs/for/master%merged
----
This only creates one merged change at a time, corresponding to
exactly `my-merged-commit`. It doesn't walk all of history up to that
point, which could be slow and create lots of unintended new changes.
To create multiple new changes, run push multiple times.
== repo upload

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

@ -19,6 +19,7 @@ import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
import static com.google.gerrit.acceptance.GitUtil.assertPushRejected;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
@ -41,6 +42,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.EditInfo;
@ -62,6 +64,7 @@ import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RefSpec;
import org.eclipse.jgit.transport.RemoteRefUpdate;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
@ -69,6 +72,7 @@ import org.junit.Test;
import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@ -837,6 +841,162 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertOkStatus();
}
@Test
public void createChangeForMergedCommit() throws Exception {
String master = "refs/heads/master";
grant(Permission.PUSH, project, master, true);
// Update master with a direct push.
RevCommit c1 = testRepo.commit()
.message("Non-change 1")
.create();
RevCommit c2 = testRepo.parseBody(
testRepo.commit()
.parent(c1)
.message("Non-change 2")
.insertChangeId()
.create());
String changeId = Iterables.getOnlyElement(c2.getFooterLines(CHANGE_ID));
testRepo.reset(c2);
assertPushOk(pushHead(testRepo, master, false, true), master);
String q = "commit:" + c1.name()
+ " OR commit:" + c2.name()
+ " OR change:" + changeId;
assertThat(gApi.changes().query(q).get()).isEmpty();
// Push c2 as a merged change.
String r = "refs/for/master%merged";
assertPushOk(pushHead(testRepo, r, false), r);
EnumSet<ListChangesOption> opts =
EnumSet.of(ListChangesOption.CURRENT_REVISION);
ChangeInfo info = gApi.changes().id(changeId).get(opts);
assertThat(info.currentRevision).isEqualTo(c2.name());
assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
// Only c2 was created as a change.
String q1 = "commit: " + c1.name();
assertThat(gApi.changes().query(q1).get()).isEmpty();
// Push c1 as a merged change.
testRepo.reset(c1);
assertPushOk(pushHead(testRepo, r, false), r);
List<ChangeInfo> infos =
gApi.changes().query(q1).withOptions(opts).get();
assertThat(infos).hasSize(1);
info = infos.get(0);
assertThat(info.currentRevision).isEqualTo(c1.name());
assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
}
@Test
public void mergedOptionFailsWhenCommitIsNotMerged() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master%merged");
r.assertErrorStatus("not merged into branch");
}
@Test
public void mergedOptionFailsWhenCommitIsMergedOnOtherBranch()
throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master");
r.assertOkStatus();
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).current().submit();
try (Repository repo = repoManager.openRepository(project)) {
TestRepository<?> tr = new TestRepository<>(repo);
tr.branch("refs/heads/branch")
.commit()
.message("Initial commit on branch")
.create();
}
pushTo("refs/for/master%merged")
.assertErrorStatus("not merged into branch");
}
@Test
public void mergedOptionFailsWhenChangeExists() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master");
r.assertOkStatus();
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).current().submit();
testRepo.reset(r.getCommit());
String ref = "refs/for/master%merged";
PushResult pr = pushHead(testRepo, ref, false);
RemoteRefUpdate rru = pr.getRemoteUpdate(ref);
assertThat(rru.getStatus())
.isEqualTo(RemoteRefUpdate.Status.REJECTED_OTHER_REASON);
assertThat(rru.getMessage()).contains("no new changes");
}
@Test
public void mergedOptionWithNewCommitWithSameChangeIdFails()
throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master");
r.assertOkStatus();
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).current().submit();
RevCommit c2 = testRepo.amend(r.getCommit())
.message("New subject")
.insertChangeId(r.getChangeId().substring(1))
.create();
testRepo.reset(c2);
String ref = "refs/for/master%merged";
PushResult pr = pushHead(testRepo, ref, false);
RemoteRefUpdate rru = pr.getRemoteUpdate(ref);
assertThat(rru.getStatus())
.isEqualTo(RemoteRefUpdate.Status.REJECTED_OTHER_REASON);
assertThat(rru.getMessage()).contains("not merged into branch");
}
@Test
public void mergedOptionWithExistingChangeInsertsPatchSet()
throws Exception {
String master = "refs/heads/master";
grant(Permission.PUSH, project, master, true);
PushOneCommit.Result r = pushTo("refs/for/master");
r.assertOkStatus();
ObjectId c1 = r.getCommit().copy();
// Create a PS2 commit directly on master in the server's repo. This
// simulates the client amending locally and pushing directly to the branch,
// expecting the change to be auto-closed, but the change metadata update
// fails.
ObjectId c2;
try (Repository repo = repoManager.openRepository(project)) {
TestRepository<?> tr = new TestRepository<>(repo);
RevCommit commit2 = tr.amend(c1)
.message("New subject")
.insertChangeId(r.getChangeId().substring(1))
.create();
c2 = commit2.copy();
tr.update(master, c2);
}
testRepo.git().fetch()
.setRefSpecs(new RefSpec("refs/heads/master")).call();
testRepo.reset(c2);
String ref = "refs/for/master%merged";
assertPushOk(pushHead(testRepo, ref, false), ref);
EnumSet<ListChangesOption> opts =
EnumSet.of(ListChangesOption.ALL_REVISIONS);
ChangeInfo info = gApi.changes().id(r.getChangeId()).get(opts);
assertThat(info.currentRevision).isEqualTo(c2.name());
assertThat(info.revisions.keySet())
.containsExactly(c1.name(), c2.name());
// TODO(dborowitz): Fix ReceiveCommits to also auto-close the change.
assertThat(info.status).isEqualTo(ChangeStatus.NEW);
}
private void pushWithReviewerInFooter(String nameEmail,
TestAccount expectedReviewer) throws Exception {
int n = 5;

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

@ -1254,6 +1254,9 @@ public class ReceiveCommits {
@Option(name = "--submit", usage = "immediately submit the change")
boolean submit;
@Option(name = "--merged", usage = "create single change for a merged commit")
boolean merged;
@Option(name = "--notify",
usage = "Notify handling that defines to whom email notifications "
+ "should be sent. Allowed values are NONE, OWNER, "
@ -1478,10 +1481,32 @@ public class ReceiveCommits {
return;
}
String destBranch = magicBranch.dest.get();
try {
if (magicBranch.merged) {
if (magicBranch.draft) {
reject(cmd, "cannot be draft & merged");
return;
}
if (magicBranch.base != null) {
reject(cmd, "cannot use merged with base");
return;
}
RevCommit branchTip = readBranchTip(cmd, magicBranch.dest);
if (branchTip == null) {
return; // readBranchTip already rejected cmd.
}
if (!walk.isMergedInto(tip, branchTip)) {
reject(cmd, "not merged into branch");
return;
}
}
// If tip is a merge commit, or the root commit or
// if %base was specified, ignore newChangeForAllNotInTarget
// if %base or %merged was specified, ignore newChangeForAllNotInTarget.
if (tip.getParentCount() > 1
|| magicBranch.base != null
|| magicBranch.merged
|| tip.getParentCount() == 0) {
logDebug("Forcing newChangeForAllNotInTarget = false");
newChangeForAllNotInTarget = false;
@ -1509,26 +1534,19 @@ public class ReceiveCommits {
}
}
} else if (newChangeForAllNotInTarget) {
logDebug("Handling newChangeForAllNotInTarget");
String destBranch = magicBranch.dest.get();
try {
Ref r = repo.getRefDatabase().exactRef(destBranch);
if (r == null) {
reject(cmd, destBranch + " not found");
return;
RevCommit branchTip = readBranchTip(cmd, magicBranch.dest);
if (branchTip == null) {
return; // readBranchTip already rejected cmd.
}
ObjectId baseHead = r.getObjectId();
magicBranch.baseCommit =
Collections.singletonList(walk.parseCommit(baseHead));
magicBranch.baseCommit = Collections.singletonList(branchTip);
logDebug("Set baseCommit = {}", magicBranch.baseCommit.get(0).name());
}
} catch (IOException ex) {
logWarn(String.format("Project %s cannot read %s", project.getName(),
destBranch), ex);
logWarn(String.format("Error walking to %s in project %s",
destBranch, project.getName()), ex);
reject(cmd, "internal server error");
return;
}
}
// Validate that the new commits are connected with the target
// branch. If they aren't, we want to abort. We do this check by
@ -1574,6 +1592,16 @@ public class ReceiveCommits {
}
}
private RevCommit readBranchTip(ReceiveCommand cmd, Branch.NameKey branch)
throws IOException {
Ref r = allRefs.get(branch.get());
if (r == null) {
reject(cmd, branch.get() + " not found");
return null;
}
return rp.getRevWalk().parseCommit(r.getObjectId());
}
private void parseReplaceCommand(ReceiveCommand cmd, Change.Id changeId) {
logDebug("Parsing replace command");
if (cmd.getType() != ReceiveCommand.Type.CREATE) {
@ -1638,29 +1666,10 @@ public class ReceiveCommits {
GroupCollector groupCollector = GroupCollector.create(changeRefsById(), db, psUtil,
notesFactory, project.getNameKey());
rp.getRevWalk().reset();
rp.getRevWalk().sort(RevSort.TOPO);
rp.getRevWalk().sort(RevSort.REVERSE, true);
try {
RevCommit start = rp.getRevWalk().parseCommit(magicBranch.cmd.getNewId());
rp.getRevWalk().markStart(start);
if (magicBranch.baseCommit != null) {
logDebug("Marking {} base commits uninteresting",
magicBranch.baseCommit.size());
for (RevCommit c : magicBranch.baseCommit) {
rp.getRevWalk().markUninteresting(c);
}
Ref targetRef = allRefs.get(magicBranch.ctl.getRefName());
if (targetRef != null) {
logDebug("Marking target ref {} ({}) uninteresting",
magicBranch.ctl.getRefName(), targetRef.getObjectId().name());
rp.getRevWalk().markUninteresting(
rp.getRevWalk().parseCommit(targetRef.getObjectId()));
}
} else {
markHeadsAsUninteresting(
rp.getRevWalk(),
magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
RevCommit start = setUpWalkForSelectingChanges();
if (start == null) {
return;
}
List<ChangeLookup> pending = new ArrayList<>();
@ -1670,7 +1679,11 @@ public class ReceiveCommits {
int total = 0;
int alreadyTracked = 0;
boolean rejectImplicitMerges = start.getParentCount() == 1
&& projectCache.get(project.getNameKey()).isRejectImplicitMerges();
&& projectCache.get(project.getNameKey()).isRejectImplicitMerges()
// Don't worry about implicit merges when creating changes for
// already-merged commits; they're already in history, so it's too
// late.
&& !magicBranch.merged;
Set<RevCommit> mergedParents;
if (rejectImplicitMerges) {
mergedParents = new HashSet<>();
@ -1886,6 +1899,44 @@ public class ReceiveCommits {
}
}
private RevCommit setUpWalkForSelectingChanges() throws IOException {
RevWalk rw = rp.getRevWalk();
RevCommit start = rw.parseCommit(magicBranch.cmd.getNewId());
rw.reset();
rw.sort(RevSort.TOPO);
rw.sort(RevSort.REVERSE, true);
rp.getRevWalk().markStart(start);
if (magicBranch.baseCommit != null) {
markExplicitBasesUninteresting();
} else if (magicBranch.merged) {
logDebug(
"Marking parents of merged commit {} uninteresting", start.name());
for (RevCommit c : start.getParents()) {
rw.markUninteresting(c);
}
} else {
markHeadsAsUninteresting(
rw, magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
}
return start;
}
private void markExplicitBasesUninteresting() throws IOException {
logDebug("Marking {} base commits uninteresting",
magicBranch.baseCommit.size());
for (RevCommit c : magicBranch.baseCommit) {
rp.getRevWalk().markUninteresting(c);
}
Ref targetRef = allRefs.get(magicBranch.ctl.getRefName());
if (targetRef != null) {
logDebug("Marking target ref {} ({}) uninteresting",
magicBranch.ctl.getRefName(), targetRef.getObjectId().name());
rp.getRevWalk().markUninteresting(
rp.getRevWalk().parseCommit(targetRef.getObjectId()));
}
}
private void rejectImplicitMerges(Set<RevCommit> mergedParents)
throws IOException {
if (!mergedParents.isEmpty()) {
@ -1970,10 +2021,15 @@ public class ReceiveCommits {
private void setChangeId(int id) {
changeId = new Change.Id(id);
ins = changeInserterFactory.create(changeId, commit, refName)
.setDraft(magicBranch.draft)
.setTopic(magicBranch.topic)
// Changes already validated in validateNewCommits.
.setValidatePolicy(CommitValidators.Policy.NONE);
if (magicBranch.draft) {
ins.setDraft(magicBranch.draft);
} else if (magicBranch.merged) {
ins.setStatus(Change.Status.MERGED);
}
cmd = new ReceiveCommand(ObjectId.zeroId(), commit,
ins.getPatchSetId().toRefName());
ins.setUpdateRefCommand(cmd);
@ -2588,11 +2644,20 @@ public class ReceiveCommits {
rw.parseBody(c);
CommitReceivedEvent receiveEvent =
new CommitReceivedEvent(cmd, project, ctl.getRefName(), c, user);
CommitValidators commitValidators = commitValidatorsFactory.create(
CommitValidators.Policy.RECEIVE_COMMITS, ctl, sshInfo, repo);
CommitValidators.Policy policy;
if (magicBranch != null
&& cmd.getRefName().equals(magicBranch.cmd.getRefName())
&& magicBranch.merged) {
policy = CommitValidators.Policy.MERGED;
} else {
policy = CommitValidators.Policy.RECEIVE_COMMITS;
}
try {
messages.addAll(commitValidators.validate(receiveEvent));
messages.addAll(
commitValidatorsFactory.create(policy, ctl, sshInfo, repo)
.validate(receiveEvent));
} catch (CommitValidationException e) {
logDebug("Commit validation failed on {}", c.name());
messages.addAll(e.getMessages());

25
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@ -77,6 +77,9 @@ public class CommitValidators {
/** Use {@link Factory#forReceiveCommits}. */
RECEIVE_COMMITS,
/** Use {@link Factory#forMergedCommits}. */
MERGED,
/** Do not validate commits. */
NONE
}
@ -110,6 +113,8 @@ public class CommitValidators {
return forReceiveCommits(refControl, sshInfo, repo);
case GERRIT:
return forGerritCommits(refControl, sshInfo, repo);
case MERGED:
return forMergedCommits(refControl);
case NONE:
return none();
default:
@ -150,6 +155,26 @@ public class CommitValidators {
new PluginCommitValidationListener(pluginValidators)));
}
private CommitValidators forMergedCommits(RefControl refControl) {
// Generally only include validators that are based on permissions of the
// user creating a change for a merged commit; generally exclude
// validators that would require amending the change in order to correct.
//
// Examples:
// - Change-Id and Signed-off-by can't be added to an already-merged
// commit.
// - If the commit is banned, we can't ban it here. In fact, creating a
// review of a previously merged and recently-banned commit is a use
// case for post-commit code review: so reviewers have a place to
// discuss what to do about it.
// - Plugin validators may do things like require certain commit message
// formats, so we play it safe and exclude them.
return new CommitValidators(ImmutableList.of(
new UploadMergesPermissionValidator(refControl),
new AuthorUploaderValidator(refControl, canonicalWebUrl),
new CommitterUploaderValidator(refControl, canonicalWebUrl)));
}
private CommitValidators none() {
return new CommitValidators(ImmutableList.<CommitValidationListener>of());
}